fix: gate Grafana ingress forward-auth annotation on site opt-in

[amdove]

Description

Grafana's in-cluster ingress returned HTTP 404 on workloads that don't enable traefik-forward-auth. The Grafana Helm ingress always stamped a traefik.ingress.kubernetes.io/router.middlewares annotation referencing the kube-system-traefik-forward-auth-main and kube-system-traefik-forward-auth-add-forwarded-headers middlewares. Those middlewares are only created by the clusters step when a site sets use_traefik_forward_auth: true, so on workloads without forward-auth the ingress referenced a non-existent middleware and Traefik invalidated the router.

The annotation is now gated on the main site's opt-in. When forward-auth is not enabled, Grafana routes straight through to its local-account login.

Root cause

Regression from the eks/cluster Python→Go migration (commit ae26379), which folded the previously-unconditional global traefik-forward-auth middleware into per-site, flag-gated logic — but the Grafana ingress kept stamping the annotation unconditionally. It surfaced on academy01-production (the only workload reaching Grafana via the public grafana.* ingress; others port-forward) after its cluster-version bump re-ran the Go steps.

Code Flow

The ingress-map construction in awsHelmGrafana (lib/steps/helm_aws.go) is extracted into a pure helper grafanaIngressValues(domain, sites). It builds the base ingress (enabled, hosts, path) and only adds the annotations key with the two-middleware string when sites["main"] is present and Spec.UseTraefikForwardAuth is true. Production behavior is byte-for-byte identical to the prior unconditional path when the flag is set.

Known edge case

The annotation is gated only on main's use_traefik_forward_auth, but the traefik-forward-auth-main middleware it references also requires the cluster to set traefik_forward_auth_version (the clusters step only deploys forward-auth when that version is present). So use_traefik_forward_auth: true on main without a traefik_forward_auth_version would still emit the annotation with no middleware behind it — i.e. the same 404 this PR fixes. This isn't reachable in current config (the workloads that enable forward-auth also set the version) and isn't a regression (the old unconditional code 404'd in that case too), so it's left as-is rather than plumbing the cluster components into the helper.

Test plan

• New unit test TestGrafanaIngressValues in lib/steps/helm_aws_test.go covers: flag on → annotation present and equals the exact two-middleware string; flag off → no annotation; no main site → no annotation; nil sites → no annotation; and basic hosts/path/enabled values.
• go test ./steps/... passes.

Category of change

• Bug fix (non-breaking change which fixes an issue)
• Version upgrade (upgrading the version of a service or product)
• New feature (non-breaking change which adds functionality)
• Build: a code change that affects the build system or external dependencies
• Performance: a code change that improves performance
• Refactor: a code change that neither fixes a bug nor adds a feature
• Documentation: documentation changes
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have reviewed my own diff and added inline comments on lines I want reviewers to focus on or that I am uncertain about

[claude]

Claude finished @amdove's task in 1m 16s —— View job

---

Code Review

• Gather context (review guidelines, changed files)
• Analyze the diff
• Post review feedback

---

The fix is correct and the extraction is clean. A few observations:

lib/steps/helm_aws.go

The grafanaIngressValues helper is the right move — pure function, no side-effects, trivially testable. The annotation string order in the new code matches the removed code exactly, which is what matters for Traefik middleware chaining.

The doc comment is multi-line and its first sentence (// grafanaIngressValues builds the Grafana Helm ingress values.) restates what the name already says. The rest (explaining why the annotation is conditional) carries real WHY signal and is worth keeping, but the comment could be trimmed to just:

// Adds the traefik-forward-auth middleware annotation only when the main site
// opts in — the middlewares are deployed by the clusters step only when
// use_traefik_forward_auth is set, so adding the annotation unconditionally
// causes a Traefik 404 on workloads without forward-auth.
func grafanaIngressValues(...) ...

That said, this is a minor style point; the existing comment is still accurate and useful.

lib/steps/helm_aws_test.go

Test coverage is solid: flag-on, flag-off, no-main-site, nil-sites, and basic structural values. One gap worth noting: there's no test that verifies grafanaIngressValues with both a non-nil sites map and UseTraefikForwardAuth: true also produces correct hosts/path/enabled values (the current "basic ingress values" test passes nil). Not a blocking concern since the helper is simple enough, but combining one of those assertions into the "flag on" case would give full coverage at no cost.

Correctness

The known edge case (forward-auth annotation emitted when use_traefik_forward_auth: true but no traefik_forward_auth_version) is correctly characterised as not a regression — the old unconditional code 404'd in that case too, and this PR makes things strictly better. Nothing needs to change there.

Overall this is a correct, well-scoped fix. The concerns above are minor.