PMREQ-821: Gateway access-grant finalizer and UIGateway extension - #5293
Open
vara2504 wants to merge 4 commits into
Open
PMREQ-821: Gateway access-grant finalizer and UIGateway extension#5293vara2504 wants to merge 4 commits into
vara2504 wants to merge 4 commits into
Conversation
The access Role/RoleBinding were protected only by delete ordering: emitted last so the operator kept its own write grant while deleting the gateway resources. A failed or reordered delete could strip the grant first and orphan them. Both grants now carry operator.tigera.io/gateway-rbac-finalizer. Teardown deletes only mark them; a sweep in the uigateway helper clears the finalizer once the Gateway, HTTPRoute, Backend and ReferenceGrant in that namespace are gone. The sweep lists grants by the gateway label, so a grant that outlives its Gateway is still found and never wedges. Also adds BackendName, ReferenceGrantName and ListenerName helpers so generated names have one definition each. Follow-on from tigera#5146 review.
…nsion The WAF filter ServiceAccount/RoleBinding reached the gateway render as a raw ExtraProxyObjects pass-through: the manager controller built them by hand and the create and delete paths each carried the field. A UIGatewayExtension in the variant extension set replaces that. The Enterprise implementation returns the WAF objects for the Manager prefix and nothing for other components, so behavior is unchanged on every variant. The uigateway Helper takes the extension and feeds one proxyObjects() result to the render and both deletion paths, removing the mirrored wiring. ExtraProxyObjects is gone from the shared Config. Follow-on from tigera#5146 review.
The operator's ClusterRole grants update on roles and rolebindings but not patch, so the finalizer sweep's Patch was forbidden and marked grants never finished deleting. Unit tests missed it: the fake client enforces no RBAC. Verified on a GKE Enterprise cluster: the fixed sweep cleared grants wedged by the old code, and a fresh enable/teardown cycle left nothing behind.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens UI ingress-gateway teardown by introducing an RBAC finalizer for per-namespace gateway access grants, and replaces ad-hoc Enterprise-only “extra proxy objects” plumbing with a formal UIGatewayExtension hook.
Changes:
- Add
operator.tigera.io/gateway-rbac-finalizerto gateway access Role/RoleBinding and implement a uigateway sweep to remove it once covered gateway resources are gone. - Introduce
extensions.UIGatewayExtensionand wire Enterprise’s WAF proxy-side objects through the extension (removing controller-side passthrough). - Centralize generated gateway-related names (
BackendName,ReferenceGrantName,ListenerName) to avoid brittle string duplication.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/uigateway/uigateway.go | Adds extension plumbing and RBAC-finalizer sweep to safely complete RBAC grant deletion. |
| pkg/uigateway/uigateway_test.go | Adds unit tests for extension flow and RBAC finalizer clearing behavior. |
| pkg/render/gateway/component.go | Defines RBAC finalizer constant, stamps grants, and centralizes gateway-related naming helpers. |
| pkg/render/gateway/component_test.go | Verifies RBAC finalizer is stamped onto access Role/RoleBinding. |
| pkg/extensions/uigateway.go | Introduces the UIGatewayExtension interface and a noop implementation. |
| pkg/extensions/extensions.go | Adds UIGateway() accessor with a default noop implementation. |
| pkg/enterprise/uigateway/uigateway.go | Implements the Enterprise UIGateway extension (WAF objects only for Manager prefix). |
| pkg/enterprise/register.go | Registers the Enterprise UIGateway extension in the extension set. |
| pkg/enterprise/controller/manager/manager_controller.go | Switches Manager gateway helper construction to use the UIGateway extension hook. |
| pkg/controller/whisker/controller.go | Plumbs the UIGateway extension into Whisker’s gateway helper construction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+356
to
+359
| var noMatch *apimeta.NoKindMatchError | ||
| if !errors.IsNotFound(err) && !stderrors.As(err, &noMatch) { | ||
| return false, err | ||
| } |
accessResourcesGone suppressed only NoKindMatchError, so a NoResourceMatchError from the mapper would block finalizer clearing. apimeta.IsNoMatchError covers both. Review follow-up on tigera#5293.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Follow-ups from the #5146 review (Whisker CIG), both requested by @caseydavenport:
1. A finalizer replaces delete-ordering for the gateway access grants (r3816269564). The access Role/RoleBinding were protected only by their position in the deletion list — emitted last so the operator kept its own write grant while deleting the Gateway/HTTPRoute/Backend. A failed or reordered delete could strip the grant first and orphan those resources. Both grants now carry
operator.tigera.io/gateway-rbac-finalizer; teardown deletes only mark them, and a sweep in the uigateway helper removes the finalizer once the resources they cover are gone. The sweep discovers grants by the gateway label, so a grant that outlives its Gateway is still found and can never wedge. UsesUpdaterather thanPatch: the operator's ClusterRole grantsupdateon roles/rolebindings but notpatch. No RBAC changes needed.2. The Enterprise-only gateway objects move behind a UIGateway extension (r3883122289). The WAF filter ServiceAccount/RoleBinding reached the shared render as a raw
ExtraProxyObjectspass-through the manager controller built by hand, mirrored across the create and delete paths. AUIGatewayExtensionin the variant extension set replaces that; the Enterprise implementation returns the WAF objects for the Manager prefix and nothing for other components, so behavior is unchanged on every variant.Also adds
BackendName,ReferenceGrantNameandListenerNamehelpers so each generated name has one definition (follow-up to the earlier "brittle string" comment).Testing: unit tests for the finalizer stamping, the sweep's gating (clears only marked grants whose resources are gone), and the extension flow. Verified end to end on a GKE Enterprise cluster: enable → Gateway Programmed, grants carry the finalizer, WAF objects present, UI serves HTTP 200 through the gateway; teardown → all gateway objects and grants removed in one reconcile cycle, nothing left Terminating; the sweep also recovered grants deliberately wedged by an earlier build.
This PR was written in part with the assistance of generative AI.
Release Note