feat(tls): per-app ListenerSet for TLS ownership - #170
Conversation
Required for the stable ListenerSet type (gateway.networking.k8s.io/v1, GA in Gateway API v1.5) and to align the SecurityPolicy types with the Envoy Gateway v1.8.x runtime. Pulls k8s.io libs to 0.36 and controller-runtime to 0.24; build, vet, and unit tests pass unchanged.
Introduce the per-app ListenerSet primitive (ADR-0011 Option 2): the operator creates a ListenerSet in the NebariApp's own namespace, attached to the shared Gateway via spec.parentRef, carrying the app's HTTPS listener with a co-located, owner-referenced Certificate. Staged, status-gated migration: the ListenerSet is always reconciled, but the legacy shared-Gateway listener keeps serving until the ListenerSet reports Accepted+Programmed. Only then do HTTPRoutes retarget to the ListenerSet and the shared listener is removed. On an Envoy Gateway that does not reconcile ListenerSet (pre-v1.8) the conditions never flip, so the operator stays on the legacy path and per-app TLS is unaffected. - TLSResult gains UseListenerSet; routing parentRef targets the ListenerSet (app namespace) once cut over, else the shared Gateway. - reconcileCertificate/isCertificateReady/checkUserProvidedSecret are namespace-parameterized; the ListenerSet cert is owner-referenced. - RBAC: add listenersets (+status); Owns(ListenerSet) so a Programmed transition drives the cutover reconcile. - Regenerated CRDs/RBAC. Closes #168.
Add unit tests for reconcileListenerSet (app-namespace object, parentRef to the shared Gateway, same-namespace cert ref, owner reference), isListenerSetProgrammed (Accepted+Programmed gating), the staged-migration attachment (stays on the legacy shared listener until Programmed, cuts over and removes it once Programmed), and route parentRef target selection. Document the ListenerSet ownership model and the routing.tls.secretName namespace change in docs/reconcilers/routing.md.
|
@viniciusdc I'm happy to test this on my cluster, will do that and report results here later |
|
Self-review from a full pass over this. Flagging what it turned up so nothing gets lost; I'll work through these before it leaves draft. Broken:
Risky / needs a call before merge:
Smaller, fixing in the same pass:
Listener/routing/GC wiring itself I'm happy with; it's the migration edges that need another lap. |
|
I'll wait until ready but I have a cluster we can test this on whenever |
|
@oldsj pushed a fmt fix ( One setup gotcha so the cutover actually gets exercised: the shared Still draft, so the edges from the self-review above are expected: the double cert while it hasn't cut over, and no status breadcrumb for the pending/stuck state. What I'd most want out of a real cluster: does the cutover actually happen and serve the app's own SNI cert, and how does an app behave when the ListenerSet never programs. Thanks for taking it. |
…s require go 1.26.4)
…ions and test goconst - build golangci-lint from source (install-mode goinstall) so it runs on the go 1.26 module (no released binary is built with go>=1.26 yet) - exclude SA1019 deprecations (controller-runtime 0.24 old events API, scheme.Builder) in cmd/main.go and api/groupversion_info.go; migration tracked separately - exclude goconst on _test.go fixture strings - align go.mod to go 1.26.5
The e2e/dev foundational stack installed Envoy Gateway v1.2.4, which does not ship the standard gateway.networking.k8s.io/v1 ListenerSet CRD, so the per-app ListenerSet reconcile fails on 'no matches for kind ListenerSet' and TLS never becomes Ready. Bump to v1.8.2 so e2e exercises the real ListenerSet path. Marked TEMPORARY: the operator is version-pinned alongside NIC, so its foundational Envoy Gateway version should track NIC's pin rather than being hardcoded here. Reconcile before approval.
|
E2E was red because @oldsj when you get to reviewing, it would be worth testing this on a cluster that already has |
Docker Images BuiltImages pushed to Quay.io for branch
Test the operator: kubectl apply -k https://github.com/nebari-dev/nebari-operator.git/config/default?ref=feat/per-app-listenerset
kubectl set image deployment/nebari-operator-controller-manager manager=quay.io/nebari/nebari-operator:feat-per-app-listenerset -n nebari-operator-system |
|
I ran this on Headline: the mechanism works — a brand-new app cuts over and serves its own SNI cert (T4b) — but the migration path deadlocks and 404s the app it's migrating (T4); the only way through is a manual delete/recreate procedure with a downtime window (T4c). The T4/T7/T9 failures turn out to be one mechanism, not three bugs, and the PR's stated motivation — T1 — never-programs path: pass, zero interruptionThe operator created spec:
parentRef: # shared Gateway, cross-namespace
group: gateway.networking.k8s.io
kind: Gateway
name: nebari-gateway
namespace: envoy-gateway-system
listeners:
- name: tls-nebari-landing-nebari-system
hostname: jamesolds-dev.example.com
port: 443
protocol: HTTPS
tls:
mode: Terminate
certificateRefs:
- {group: "", kind: Secret, name: nebari-landing-nebari-system-tls} # no namespace -> in-namespace, no ReferenceGrant
allowedRoutes:
namespaces: {from: Same}
Because EG v1.6.2 does not reconcile it, the status stayed at the CRD default — note the epoch-0 timestamp, which is the tell that no controller ever wrote it: status:
conditions:
- {type: Accepted, status: Unknown, reason: Pending, message: "Waiting for controller", lastTransitionTime: "1970-01-01T00:00:00Z"}
- {type: Programmed, status: Unknown, reason: Pending, message: "Waiting for controller", lastTransitionTime: "1970-01-01T00:00:00Z"}
I also checked that Two known issues reproduce:
T2 — missing ListenerSet CRD: the unmet dependency is invisibleI deleted
Two separate claims, to be precise about which evidence supports which: the This agrees with your hard-dependency conclusion, and my cluster is a data point for it rather than a counterexample. The standard CRD here was hand-applied during unrelated LBC work, not shipped by the EG chart — Where I'd push back is this:
I agree a cross-version fallback is not worth building. But the mismatch can occur outside the supported model — the v1.2.4 e2e run is an instance of exactly that, and so is anyone upgrading the operator ahead of NIC — and when it does, the result is not a visible failure. It is So my ask isn't a fallback, it's a loud failure. In order of preference:
Option 1 is the one I'd take. Either way the precondition should be visible in the deployment docs, not only in the PR description. The EG v1.8.2 upgrade itself: cleanBumped this cluster v1.6.2 → v1.8.2. Gateway API CRDs moved to bundle T3 —
|
|
Thanks, @oldsj, for the incredible, in-depth review and testing!!! |
|
@oldsj this is a genuinely great testing pass. It changed my mind on the mechanism, so let me lay out where I landed. You're right that the staged gate rests on a false premise. Creating the ListenerSet detaches the hostname's routes the moment it's allowed to attach, and holding the legacy listener is exactly what keeps it stuck on I wanted to see the condition semantics directly before rewriting anything, so I upgraded a local k3d NIC bed to EG v1.8.2 and reproduced your T4 with a hand-crafted ListenerSet colliding with an existing legacy listener. The set-level
So the rewrite is a reason-aware remove-first, not a On the user-provided-secret path (T9): agreed, and I'm pulling it back to the legacy shared-Gateway listener until the secret-copy work lands, so it stops creating a ListenerSet that claims a hostname it can't serve. The per-app ListenerSet cutover for On the dead-defense pushback, you convinced me. A missing/forbidden CRD presenting as One extra thing I hit that isn't in your notes: after a clean cutover EG raises Still an open question for the NIC side: I'll push the reworked cutover shortly and ping you when it's ready for another spin. |
Rework the per-app ListenerSet cutover (ADR-0011 Option 2) after review and live testing on Envoy Gateway v1.8.2 showed the Programmed-gated staged hold deadlocks an in-place migration: a ListenerSet detaches the hostname's routes from the shared Gateway the moment it is allowed to attach, so holding the legacy listener both takes the app offline and blocks the ListenerSet on a HostnameConflict it can never clear. Replace isListenerSetProgrammed with a reason-aware shouldCutOver that keys off the status EG actually reports: cut over when the set-level Programmed condition is True, or when our own listener reports HostnameConflict with refs resolving (our legacy listener is the only blocker, so remove it first and let the ListenerSet program); otherwise stay on the legacy listener. NotAllowed keeps the legacy listener, since removing it on a Gateway that refuses the attachment would strand the app. A conflict with a peer app's ListenerSet on the same hostname is indistinguishable by condition and is left to the conflict-handling rework (#168). Route the user-provided-secret path back onto the legacy shared-Gateway listener (the secret lives in the Gateway namespace; the app-namespace secret copy is TODO #168), and delete any ListenerSet a prior cert-manager reconcile left behind. Tear down the ListenerSet and legacy listener when TLS is disabled, so a cut-over app that turns TLS off does not keep terminating HTTPS and detaching the route. Add unit coverage for the decision matrix and the off-path teardown, and a Serial e2e spec that drives the real cutover and both transitions against EG v1.8.2.
Docker Images BuiltImages pushed to Quay.io for branch
Test the operator: kubectl apply -k https://github.com/nebari-dev/nebari-operator.git/config/default?ref=feat/per-app-listenerset
kubectl set image deployment/nebari-operator-controller-manager manager=quay.io/nebari/nebari-operator:feat-per-app-listenerset -n nebari-operator-system |
3 participants
Closes #168.
What
Implements ADR-0011 Option 2: the operator owns a per-app
ListenerSet(standardgateway.networking.k8s.io/v1) in the NebariApp's own namespace, attached to the shared Gateway viaspec.parentRef, instead of mutating the shared Gateway's listener list. The app'sCertificate+ TLS secret are co-located in that namespace and owner-referenced, so they garbage-collect with the app (no cross-namespace label bookkeeping, noReferenceGrant). This removes the shared-Gateway co-ownership that leavesgateway-configpermanently OutOfSync.Runtime dependency: Envoy Gateway v1.8.2+ (hard)
This needs the cluster to serve the standard
gateway.networking.k8s.io/v1ListenerSetCRD, which ships in the Gateway API v1.5 bundle carried by Envoy Gateway v1.8.2+. The operator does not add a fallback for older EG: it is foundational software pinned per NIC release, so NIC controls its EG version, and guarding against a mismatch that can't occur in the supported model would be dead defense.Today's NIC platform is on EG v1.6.2 (verified on two live clusters: they carry the experimental
xlistenersets.gateway.networking.x-k8s.iobut not the standard kind used here). So this must not be released ahead of the platform EG upgrade (nebari-dev/nebari-infrastructure-core#496); once a cluster is on EG v1.8.2+, it works.Cutover: reason-aware, remove-first (per NebariApp)
No user-facing strategy flag. The earlier draft of this PR described a staged hold (keep the legacy per-app listener until the ListenerSet reports
Programmed); review and live testing on EG v1.8.2 showed that deadlocks an in-place migration. A ListenerSet detaches the hostname's routes from the shared Gateway the moment it is allowed to attach - regardless of whether it is ever Accepted or Programmed - so holding the legacy listener on the same(port, hostname)both takes the app offline and blocks the ListenerSet on aHostnameConflictit can never clear.So
shouldCutOverdecides off the status Envoy Gateway actually reports:Programmed=True-> cut over.Conflicted=True/HostnameConflictwith refs resolving -> our own legacy listener is the only blocker, so remove it first and let the ListenerSet leave the conflict and program.Accepted=False/NotAllowed, unresolved refs) -> stay on the legacy listener.NotAllowedmeans the Gateway refuses the attachment (e.g.spec.allowedListenersunset), so removing the legacy listener would strand the app.Owns(&ListenerSet{})drives the reconcile when the status flips. App-namespace TLS plumbing (cert/secret co-location, owner refs, routing parentRef threading) is adapted from the per-app-Gateway prototype in #167.User-provided secret and TLS-disabled
routing.tls.secretNamestays on the legacy shared-Gateway listener (the secret lives in the Gateway namespace; the app-namespace copy isTODO(#168)). Switching to a user secret deletes any ListenerSet a prior cert-manager reconcile cut over to, so it stops claiming the hostname; there is a brief (~seconds) reprogram window on that switch.Validation
Validated live on a k3d NIC bed upgraded to EG v1.8.2, driving
nebari-landing(a real app with a real backend) through the full flow with the operator built from this branch: the ListenerSet reachesProgrammed, the HTTPRoute reparents tokind: ListenerSet, the legacy listener is removed, and the served TLS cert flips from the Gateway-namespace cert to the app-namespace cert. Then the user-secret and TLS-disable transitions both tear the ListenerSet down and fall back correctly. Every step stayed HTTP 200 - no outage on any transition. Also confirmed thelistenersetsRBAC is load-bearing: withoutlist/watch,Owns(&ListenerSet{})fails and the controller silently stops reconciling.Dependency / toolchain bump
sigs.k8s.io/gateway-apiv1.4.1 -> v1.5.1 (stableListenerSet),github.com/envoyproxy/gatewayv1.6.3 -> v1.8.2. These pull k8s libs to 0.36 / controller-runtime 0.24, which raise the Go floor to 1.26 (matches NIC'sgo 1.26.5). Carried here as a result: Dockerfile basegolang:1.25 -> golang:1.26, README Go badge/prereq to 1.26+, and golangci-lint viainstall-mode: goinstall(no released golangci binary is built with go >= 1.26 yet, so binary mode fails its built-with guard).SA1019deprecations from controller-runtime 0.24 (old events API,scheme.Builder) are excluded in.golangci.yml; the real migration is tracked in Migrate off deprecated controller-runtime events API (GetEventRecorderFor) and scheme.Builder #171.goconstexcluded on_test.go.listenersets(+/status).gatewayskeepsupdate;patchbecause the reason-aware cutover still upserts/removes the shared listener on the pre-cutover and user-secret paths; it can drop to read-only once those go away.TEMPORARY (remove before approval)
dev/scripts/services/install.shinstalled EG v1.2.4 (no ListenerSet CRD), so e2e could not exercise this path. Bumped to v1.8.2 so e2e runs the real cutover. The dev/e2e EG version should track NIC's pin rather than be hardcoded here, so reconcile againstnebari-dev/nebari-infrastructure-core#496before merging.Test plan
shouldCutOverdecision matrix (programmed / no-status /NotAllowed/ hostname-conflict with refs resolved and unresolved); remove-first cutover vs stay-on-legacy; and the off-path teardown (user-secret and TLS-disabled both delete the ListenerSet). tls/routing/auth suites pass.test/e2e/tls_listenerset_test.go(Serial) drives the real cutover and both transitions against EG v1.8.2, assertingProgrammed, route reparent, legacy-listener removal, and teardown. It enables the operatorTLS_CLUSTER_ISSUER_NAME+ the GatewayallowedListenersinBeforeAlland reverts both inAfterAll, since the rest of the suite assumes no per-app cert-manager TLS. (Correcting the earlier draft: the previous e2e did not exercise the cutover at all - the default app used the sharedhttpslistener - so this spec is what adds that coverage.)nebari-dev/nebari-infrastructure-core#496applied.Known follow-ups
1/1 Running/ Ready whileOwns(&ListenerSet{})blocks cache sync and nothing reconciles. It should fail fast at startup with a clear error instead. Not built here.ReasonGatewayListenerConflictsignal should be re-established.routing.tls.secretNamevia ListenerSet: needs the app-namespace secret copy so cert-manager adopts it without reissuing (TODO(#168)); until then this path stays on the legacy listener.shouldCutOverreads status without anobservedGeneration == generationguard. EG does populate it (verified on v1.8.2), so it is a clean future hardening; omitted here because a hostname edit reconciled fine without it in testing.