Fast-path provisioning: per-Gateway LoadBalancer Services and per-Gateway ports

[codebot-robot]

Fixes #620

[justinsb]

I think we should at least log the error, if we can't handle it.

[justinsb]

I think we should at least log the error, if we can't handle it.

[justinsb]

Let's rename to OnGatewayDeleted or something more self-descriptive

[justinsb]

Please log

[justinsb]

Thanks for this. It works, but reviewing it made it clear that dynamic per-Gateway pod ports put too much test-only complexity into the data plane. We've changed the design to one data-plane Deployment per Gateway; see #620 (latest comment).

Please rework this PR to that design (or close it and open a new one, whichever is easier):

• Drop: pkg/gari/dynamic_listeners.go, PortAllocator, ListenerAddressProvider, ModelUpdater, NewScopedProxy/scopedPort, and the Gateway finalizer.
• Keep and adapt: removing gari-proxy, per-Gateway LoadBalancer Services (now selecting that Gateway's own Deployment), reading the LB address with no fallback, the RBAC changes, the e2e harness using status.addresses, and enabling HTTPRouteMultipleGateways.
• Add: the provisioned Deployment, a data-plane mode (GatewayFilter for one Gateway, status writes disabled), Programmed gated on the Deployment being available, and label-based cleanup with an orphan sweep instead of a finalizer.

Points from my earlier review that still apply:

• Service ports should come from the Gateway's effective listeners (so ListenerSet listeners are included), not just gw.Spec.Listeners.
• Don't swallow startup errors, e.g. a List before the cache has synced. Use mgr.GetAPIReader() or run after cache sync.

[justinsb]

Thanks for the rework. The architecture now matches the #620 design: a Deployment and a LoadBalancer Service per Gateway in the controller namespace, label-based cleanup with an orphan sweep, no finalizer, Service ports built from effective listeners and mapped to 8000/8443 (TCP+UDP), Programmed held back until the Deployment is available, and a data-plane mode with status writes turned off. The dynamic listeners and port allocator are gone. Nice work.

Before we merge, there are some bugs and regressions to fix, plus some unrelated churn to revert:

Bugs and regressions

1. The Namespace watch was removed from gateway_controller.go. Without it, a Gateway using allowedListeners.namespaces.from: Selector won't react when a namespace's labels change. The ReferenceGrant mapper also lost the "grant from a Gateway in this namespace" check. Neither change is related to provisioning, so please restore both. (Incremental state step 2: central recompute and diff #628 will replace these mappers properly.)
2. Cleanup errors are only logged. When OnGatewayDeleted fails nothing retries it, and the sweep only runs at startup, so a failed delete leaks until the controller restarts. Please return the error so the request is requeued. SweepOrphans also discards List and Delete errors (if err == nil, _ = p.client.Delete(...)). Please return or aggregate them.
3. API calls on every foreign Gateway. Each reconcile of a Gateway with another class, or one out of scope, sends two DELETEs straight to the API server. Please check the cache first (Get, or List by label) and only delete what exists.
4. There's no readiness probe, so Programmed can be true too early. GARI doesn't register a readyz check, so "Deployment available" only means the container started, not that the data plane has synced and applied its config. Please add a readiness check that passes once the first proxy config has been applied, and a readinessProbe on the data-plane container. Otherwise conformance will flake.
5. The data plane uses the controller's ServiceAccount. DefaultDataplaneServiceAccount = "gari-controller" lets every data-plane pod create and delete Deployments and Services and write status. Please add a separate read-only gari-dataplane ServiceAccount, ClusterRole and binding in k8s/controller.yaml (get/list/watch on Gateway API resources, Services, Secrets, Namespaces, ConfigMaps and EndpointSlices, whatever the proxy reads), and use it by default.
6. --dataplane-mode without --gateway-namespace/--gateway-name quietly serves every Gateway. Please fail at startup in that case.
7. The image is resolved twice. GARI_IMAGE is read in both main.go and GatewayAddresses. The provider should just use the image it's given.
8. The Deployment update only compares image and args. Changes to the ServiceAccount, container ports, labels or probes are never applied. Please compare the whole desired template, or use server-side apply.

Smaller points

• GatewayAddresses now creates and updates resources. That's acceptable for this PR, but please add a comment saying so; we'll probably split out a provisioner interface as part of Incremental state step 2: central recompute and diff #628.
• --proxy-http3-advertised-port comes from the first HTTPS listener, which is wrong when a Gateway has HTTPS listeners on several ports. A TODO is fine for now.
• The e2e test still waits for cleanup with time.Sleep(3 * time.Second). Please poll with a timeout instead.

Unrelated churn (please revert)

• pkg/gari/gari.go: the Options doc comments were rewritten and lost information ("Ignored if HTTPListener is provided", "Set to 0 or empty to disable"). DefaultScheme was moved and switched from utilruntime.Must to _ =, which swallows errors. The logger was renamed. gatewayv1alpha3 was added; if it's actually needed, please say why.
• pkg/proxy/proxy.go: the p → pt rename.

Process

• Please rebase. The only conflict is in pkg/state/compiled.go.
• Please update the PR description. It still describes dynamic ports, the finalizer and ListenerAddressProvider.
• Please report the conformance runtime before and after this change, as asked in Fast-path provisioning: per-Gateway LoadBalancer Services and per-Gateway ports, no fallback (#587 step 1b) #620.

[justinsb]

Heads-up: #629 (incremental state step 2) is merging and changes the code this PR touches. Please rebase onto main after it lands, and in the rebase:

• Remove DisableStatusUpdates from the individual reconcilers. Reconcilers now only write status from State outputs, so data-plane mode can simply not register the status-writing reconcilers. Or keep one check in each reconciler, but there's no need to thread it through every Compute* call.
• Make Deployment readiness a State input, alongside the addresses. Instead of the extra infraReady parameter on ComputeDesiredGatewayStatus, record readiness in State from the Gateway reconciler (as is already done with SetGatewayAddresses). ComputeOutputs then gates Programmed on it, and the central diff re-enqueues the Gateway when it changes.
• GatewayFilter is applied inside State.UpsertGateway. Use that for --gateway-namespace/--gateway-name rather than filtering in the reconciler.
• Service ports come from compiled outputs. Effective listeners for the provisioner should come from the compiled model in State (for example, add them to Outputs), not from a second CompileModel call in the reconciler.

The points from my previous review still apply: the Namespace and ReferenceGrant watch regressions disappear with #629, but cleanup errors, foreign-Gateway DELETEs, the readiness probe, the gari-dataplane ServiceAccount, failing at startup without --gateway-name, image resolution, the template diff, the unrelated churn, the PR description and the conformance runtime numbers are all still open.

[justinsb]

Follow-up on where the per-Gateway resources live, prompted by filing #635 (GatewayInfrastructure).

Decision: provision the data-plane Deployment and Service into the Gateway's namespace, not the controller namespace.

• GatewayInfrastructure looks for the ServiceAccount/Pod/Service labelled gateway.networking.k8s.io/gateway-name=<name> in the Gateway's own namespace, so this test can't pass with the current layout. Your labels already match.
• This is also the more common choice. It avoids hashing namespace and name into one name: the Deployment and Service can be named after the Gateway, e.g. <gateway-name>-gari, truncated with a hash only if too long. Cleanup can also use an ownerReference from the Gateway, since owner and dependents are in the same namespace. With that, we can probably drop most of the orphan sweep.
• The data-plane ServiceAccount must exist in each Gateway namespace. Have the provisioner create it, with the same gateway-name labels, and copy the spec.infrastructure labels and annotations onto it too, so Pass GatewayInfrastructure conformance test #635 is straightforward.

RBAC caveat, and we must address it before this is safe outside test clusters:

• Each data-plane pod currently runs the full state computation for its Gateway, so gari-dataplane has cluster-wide read on Secrets, Services, EndpointSlices, etc.
• With a ServiceAccount in every Gateway namespace bound to that ClusterRole, anyone who can create pods in such a namespace could read every Secret in the cluster.
• The fix is to compute centrally and push each data plane only its own compiled config. For example, the controller writes a per-Gateway Secret, holding compiled routes, the certificates that Gateway needs and resolved endpoints, into the Gateway namespace. The data plane then only needs get/watch on that one object.
• That's a separate piece of work and I'll file an issue for it. It doesn't need to be in this PR, but please keep the data-plane RBAC as narrow as it can be today and add a comment on the ClusterRole pointing at the follow-up issue.

The earlier round-2 points and the rebase-on-#629 note still apply.

[justinsb]

Thanks for the quick turnaround. Moving to the Gateway namespace looks good overall, but there is one critical security problem and a few correctness issues. Several of these matter more now that we write into user namespaces.

Must fix

1. The gari-dataplane ClusterRoleBinding gives every ServiceAccount in the cluster read access to all Secrets. Its subject is the group system:serviceaccounts, so any pod in any namespace can read every Secret.
   • The controller should instead create one ClusterRoleBinding per Gateway, with only that Gateway's ServiceAccount as subject. Label it like the other per-Gateway resources and clean it up with the label sweep. (A cluster-scoped object can't have a namespaced ownerReference.)
   • The controller then needs create/delete on clusterrolebindings, plus bind on the gari-dataplane ClusterRole only (resourceNames: ["gari-dataplane"]).
   • Anyone who can create pods in a namespace that has a Gateway can still read every Secret even with this fix. That's what Central config distribution: data-plane pods should not need cluster-wide read access #637 is for, and we'll prioritise it after this PR.
2. The readiness gate never fires, so Programmed=True is reported before the Deployment is available.
   • SetGatewayReadiness returns early when s.gatewayReadiness[key] == ready. For a key that has never been set, the map gives false, so the first SetGatewayReadiness(key, false) is skipped. ComputeOutputs then treats the missing key as ready. (I confirmed this with a quick test.)
   • Use if cur, ok := s.gatewayReadiness[key]; ok && cur == ready { return }.
   • Also delete the entry in DeleteGateway and in the filtered-out branch of UpsertGateway, as is done for gatewayAddresses.
   • Please add unit tests: Programmed=False/Pending while the Deployment isn't available, and True once it is.
3. Only touch objects we own. GatewayAddresses takes over any existing ServiceAccount/Deployment/Service named <gw>-gari in the Gateway's namespace and overwrites it. OnGatewayDeleted deletes those names without checking who owns them, and it also runs for Gateways of other controllers, via the unmanaged branch.
   • Only update or delete an object that has a controller ownerReference to this Gateway, or at least our managed-by and gateway-name labels.
   • Otherwise leave it alone and report the conflict in the Gateway's status (Programmed=False with a clear message).
   • The update path should also reconcile OwnerReferences, so a Gateway recreated with the same name adopts its objects, instead of the garbage collector deleting them under it.
4. spec.infrastructure.labels can override our own labels. The user's labels are applied after app, gateway-name, gateway-namespace and managed-by, so they win.
   • For example, infrastructure.labels: {app: foo} changes the pod template labels so they no longer match the selector, and the API server rejects the Deployment.
   • Apply the user's labels first, then ours. Ignore user keys with the gateway.networking.k8s.io/ prefix.
   • Please also use a GARI-specific selector label (for example gateway.networking.k8s.io/gateway-name plus a gari label) instead of the generic app.

Should fix (raised in round 2, still open)

5. The Deployment is updated on every reconcile. It compares Spec.Template.Spec with reflect.DeepEqual, but the API server fills in defaults (terminationMessagePath, dnsPolicy, probe timeouts and thresholds, ...), so they never match. Use server-side apply, or store a hash of the desired template in an annotation and compare that.
6. The readiness probe passes too early. /readyz is healthz.Ping, so the pod reports ready as soon as the manager starts, before its caches have synced or the first config has been applied to the proxy. Readiness, and therefore Programmed, should mean "this pod is serving the Gateway's config". Make the check pass only after State has synced and the first config has been applied.
7. Please update the PR body. It still describes the controller-namespace Services, a port range and a finalizer.

Looks good

• DisableStatusUpdates is now one switch that skips registering reconcilers. That's fine.
• --gateway-name is validated.
• Service ports are built from the effective listeners.
• The e2e checks for the second Gateway's distinct address and cleanup.
• Thanks for the runtime numbers in the journal.

[justinsb]

Thanks, this round fixes the previous must-fix items:

• the system:serviceaccounts binding is gone;
• the readiness zero-value bug is fixed, with a test;
• objects are checked for ownership before update or delete, and ownerReferences are reconciled;
• user labels can no longer override our labels, and the selector uses our own labels;
• the template-hash annotation replaces the DeepEqual comparison;
• /readyz is gated on the first config being applied.

Remaining items:

1. The controller can now update or delete any ClusterRoleBinding in the cluster, including cluster-admin's. RBAC can't scope access by label, so CRUD on clusterrolebindings applies cluster-wide. Please use a single shared binding instead:
   • Ship one gari-dataplane ClusterRoleBinding in k8s/controller.yaml, bound to the gari-dataplane ClusterRole, with an empty subjects list.
   • The controller adds or removes each Gateway's ServiceAccount ({kind: ServiceAccount, namespace: <gw ns>, name: <gw>-gari}) in that binding's subjects. Use a patch or update with conflict retry, and keep subjects sorted so updates are deterministic.
   • Controller RBAC for this: get, update and patch on clusterrolebindings with resourceNames: ["gari-dataplane"] only. Remove the general clusterrolebindings CRUD rule and the bind rule; bind is not needed when only subjects change.
   • Cleanup: OnGatewayDeleted removes the subject. The orphan sweep removes subjects whose Gateway no longer exists, or whose ServiceAccount is not one of ours (match the <name>-gari pattern and check that the ServiceAccount has our managed-by label).
   • This also removes the per-Gateway binding names, so the name-collision problem with "%s-%s-gari" (namespace a-b + Gateway c vs namespace a + Gateway b-c) goes away.
   • Please add a unit test for adding and removing subjects, including two Gateways whose subjects are added at the same time.
   • Central config distribution: data-plane pods should not need cluster-wide read access #637 will remove this binding entirely once config is pushed centrally.
2. The template hash only covers template.Spec. Pod template labels and annotations are not included, so changing spec.infrastructure.labels or annotations on the Gateway updates the Deployment's own metadata but never the pod template: the hash is unchanged, so the template is not replaced. Please hash the whole PodTemplateSpec, metadata included, and add a unit test that changes infrastructure labels and checks that the pod template is updated.
3. Conflicts should be visible on the Gateway's status, not only as a reconcile error. When a same-named object exists that we don't own, we currently return an error: it is retried with backoff and logged, but the user never sees it. Please report it as Programmed=False with a message naming the conflicting object, for example through a provisioning-status input to State, as is done for readiness. Then stop requeueing until the object or the Gateway changes.
4. Please rewrite the PR body. It still describes the original design (controller-namespace Services named gari-gw-<hash>, a pod port range, dynamic listeners, a Gateway finalizer, --controller-namespace), none of which is in the PR now. It should describe:
   • the per-Gateway ServiceAccount, Deployment and LoadBalancer Service in the Gateway's namespace, with ownerReferences;
   • the shared data-plane ClusterRoleBinding, with a pointer to Central config distribution: data-plane pods should not need cluster-wide read access #637;
   • data-plane mode;
   • readiness gating;
   • label and annotation propagation;
   • the orphan sweep;
   • HTTPRouteMultipleGateways.

[justinsb]

Nearly there. Just two more things:

1. GatewayReconciler must return errors again. At the moment every error from GatewayAddresses is recorded on status and swallowed, so transient failures (API errors, update conflicts) are never retried. Return a typed error for ownership conflicts only. Record that one on status and don't return it. Return every other error from Reconcile, as before.
2. Replace the PR body with just Fixes #620. The current body describes an older design. Put anything worth keeping in commit messages or code comments.

[justinsb]

LGTM, thanks! A few small leftovers will go into a follow-up issue.