Skip to content

fix(helpers): Prefer an explicit externalUri over the Service lookup - #158

Merged
aaronmgn merged 1 commit into
mainfrom
fix/154-externaluri-restarts
Aug 3, 2026
Merged

fix(helpers): Prefer an explicit externalUri over the Service lookup#158
aaronmgn merged 1 commit into
mainfrom
fix/154-externaluri-restarts

Conversation

@aaronmgn

@aaronmgn aaronmgn commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Setting graylog.config.network.externalUri now takes precedence over the Ingress hostnames and the live Service lookup, so a LoadBalancer acquiring or changing its address no longer rolls the Graylog StatefulSet on the next upgrade. The value accepts a full URI or a bare hostname. Also documents that rollingUpdate.maxUnavailable only works where the MaxUnavailableStatefulSet feature gate is enabled.

Details

  • The explicit value preempts the lookup in the graylog.externalUri helper. The lookup does not run at all when the value is set. TLS cn keeps its precedence. Following review feedback on this PR, the explicit value now also comes before the Ingress hostnames: an Ingress hostname cannot carry a path prefix, a public port, or a scheme that differs from the Ingress. Installs without an explicit value render exactly as before.
  • Full URIs pass through with their scheme intact (previously a full URI rendered as http://https://...). Bare hostnames get the scheme and app port as before, with https when TLS is enabled.
  • Guarded the status.loadBalancer.ingress[0] access so a Service with empty LB status cannot fail the render.
  • Feature-gate note for maxUnavailable in values.yaml and both README rows.
  • New unit test suite tests/externaluri_test.yaml covering the precedence and formatting matrix.
  • Behavior change: a release that sets both externalUri and an Ingress hostname now renders the explicit value, so its config checksum changes once and its pods roll once on the next upgrade. Releases that do not set externalUri render identically and do not roll.

Linked issues

This fixes #154

PR Checklist

Please check the items that apply to your change.

  • Tests added/updated
  • Documentation updated
  • This PR includes a new feature
  • This PR includes a bugfix
  • This PR includes a refactor

Testing Checklist

Static Validation

  • Linter check passes: helm lint ./charts/graylog
  • Helm renders local template sucessfully: helm template graylog ./charts/graylog --validate

Installation

  • Fresh installation completes successfully: helm install graylog ./charts/graylog
  • All pods reach Running state: kubectl rollout status statefulset/graylog
  • Helm tests pass: helm test graylog

Functional (if applicable)

  • Web UI accessible and login works
  • DataNodes visible in System > Cluster Configuration
  • Inputs can be created and receive data

Upgrade (if applicable)

  • Upgrade from previous release succeeds
  • Scaling up/down works correctly
  • Configuration changes apply correctly

Specific to this PR

  • Live test on a Talos cluster with a working LoadBalancer pool: fresh install renders no external URI, the first upgrade picks it up from the Service lookup (unchanged fallback), and repeat upgrades hold the checksum steady. With externalUri pinned while LB status existed, the explicit value won, the config checksum stayed identical across repeated upgrades, and the StatefulSet generation never moved. Stack came up healthy (server, Datanode, external MongoDB 7.0.25) and /api/system/lbstatus answered ALIVE through the LB, with the pinned URI in the pod env. Full unit suite passes (177 tests).
  • Ingress precedence verified on a kind cluster with a real ingress-nginx controller and a real MetalLB LoadBalancer: with an Ingress hostname configured and externalUri set to a different host, the running container held the explicit URI and /api/system/lbstatus still answered ALIVE through the Ingress. Upgrading an existing release from this branch's previous state changed the config checksum once for the both-values-set case and not at all for releases without externalUri.

Notes for reviewers

  • Verify all applicable tests above pass
  • Validate that the linked issues are no longer reproducible, if applicable
  • Sync up with the author before merging
  • The commit history should be preserved - use rebase-merge or standard merge options when applicable

@aaronmgn
aaronmgn requested a review from alix-graylog July 31, 2026 00:22
@aaronmgn
aaronmgn force-pushed the fix/154-externaluri-restarts branch 3 times, most recently from 602538c to 5e4b254 Compare August 3, 2026 19:28

@alix-graylog alix-graylog left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@aaronmgn
aaronmgn force-pushed the fix/154-externaluri-restarts branch from 5e4b254 to 5aa8904 Compare August 3, 2026 19:34
@aaronmgn
aaronmgn merged commit e3c965d into main Aug 3, 2026
9 checks passed
@aaronmgn
aaronmgn deleted the fix/154-externaluri-restarts branch August 3, 2026 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[U-03][G-10] Avoidable rolling restarts

2 participants