Repository navigation
fix(kamaji): resolve Gateway API availability once at startup - #4501
Conversation
Kamaji checked for the Gateway API on every TenantControlPlane reconcile. That discovery call ignores the reconcile context, so it is bounded only by the client's 32s default timeout, which is longer than the 30s reconcile budget. A slow discovery endpoint could therefore use up the whole reconcile. The result could also disagree with the watches, which were set up once at startup. The patch carries the fix proposed upstream in clastix/kamaji#1327, not yet merged there, onto the Kamaji tag cozystack ships. Availability is resolved once when the manager starts, and the same value drives the watches, the indexer, the reconciled resources and the admission webhook's Gateway API check. A discovery error at startup now stops the manager, so it is no longer read as "Gateway API not installed". A Gateway API installed after Kamaji starts needs a Kamaji restart. On this platform Kamaji depends on cert-manager, which depends on gateway-api-crds, so the CRDs are already present when Kamaji starts. The patch keeps the upstream tests so that it matches the upstream commit. The docs hunk is left out because it is not part of the image. Drop this patch once Kamaji ships the change. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
area/kamaji
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughGateway API availability is resolved once during manager startup. The result controls GatewayListener setup, Gateway resource selection, route ownership watches, and webhook validation. Reconciliation no longer performs Gateway API availability discovery. ChangesGateway API availability
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ManagerStartup
participant GatewayDiscovery
participant TenantControlPlaneReconciler
participant TenantControlPlaneGatewayValidation
ManagerStartup->>GatewayDiscovery: Resolve Gateway API availability
GatewayDiscovery-->>ManagerStartup: Availability result or discovery error
ManagerStartup->>TenantControlPlaneReconciler: Pass GatewayAPIAvailable
ManagerStartup->>TenantControlPlaneGatewayValidation: Pass GatewayAPIAvailable
Merge Risk: ⚪ Minimal · up to The Gateway API availability change is mergeable after normal checks. It does not address the separately reported TenantControlPlane cleanup stall. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue [ Resolution Implement and test a bounded deletion reconcile for [ Full details: Out of Scope Changes checkExplanation Relative to directly linked issue [
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM. Clean carry of upstream clastix/kamaji#1327 as a build patch on the 26.9.4-edge tag, and it fixes the right thing at the root.
The core change resolves Gateway API availability once at manager startup instead of on every TenantControlPlane reconcile. It holds up on the two questions that matter for a "read a signal once and cache it" change.
A discovery error is no longer swallowed as "not installed". The old utilities.AreGatewayResourcesAvailable returned a bare bool and turned any discovery error into false; the new utilities.GatewayAPIResourcesAvailable returns (bool, error), and cmd/manager/cmd.go now logs and returns that error, which stops the manager. A transient discovery blip crashloops and retries instead of latching "Gateway unavailable" for the life of the process. That is the fix, and it is correct.
A Gateway API CRD installed after startup needs a restart, and the webhook says so: "install the Gateway API CRDs and restart Kamaji". Before this patch the watches were already startup-only while reconcile and the webhook discovered live, so a late install left the controller reconciling Gateway resources it did not watch. After the patch the whole path follows one value and fails closed with a clear message. On this platform the late-install case does not arise: cozystack.kamaji dependsOn cozystack.cert-manager, which dependsOn cozystack.gateway-api-crds, so the CRDs are present before Kamaji starts.
Checked statically and against CI. GatewayAPIResourcesAvailable exists at the 26.9.4-edge tag with the expected signature. The internal/utilities import is dropped only from the two files that stop referencing it and kept where it is still used (cmd.go, and the webhook's TLSRoute check). The removed DiscoveryClient struct fields are read nowhere else. The "Build packages/system/kamaji" job (git apply + go build on the real tag), the in-tree E2E and the unit/controller suites are all green.
Non-blocking:
- The webhook's separate TLSRoute check still runs live discovery on every admission request and can still hit the apiserver's 10s admission timeout. This one is pre-existing, unchanged here, and already listed as an upstream follow-up, so it is not something this PR introduced.
- A discovery failure at startup now crashloops the manager instead of silently disabling Gateway support. That is the intended fail-loud behavior, worth knowing about operationally.
- This is a verbatim carry of an unmerged upstream PR. It needs a re-sync if upstream changes before it merges, and should be dropped once Kamaji ships the change. The description already commits to both.
2 participants
What this PR does
Carries the fix proposed upstream in clastix/kamaji#1327 (not yet merged there; issue clastix/kamaji#1326) as a build patch on the 26.9.4-edge tag that cozystack builds Kamaji from.
In Kamaji 26.9.4-edge, every TenantControlPlane reconcile checks whether the Gateway API is available. The discovery call ignores the reconcile context, so it is bounded only by the client's 32s default timeout, which is longer than the 30s reconcile budget. The watches, however, read availability only once, at startup.
With the patch, Kamaji resolves availability once when the manager starts. The watches, the indexer, the reconciled resources and the admission webhook's Gateway API check all use that one value. A discovery error at startup now stops the manager, so it is no longer mistaken for "Gateway API not installed". A Gateway API installed after Kamaji starts needs a Kamaji restart. On this platform that case does not come up: the kamaji package depends on cert-manager, and cert-manager depends on gateway-api-crds.
The webhook's separate TLSRoute check still runs discovery on each request, as before, and is listed as a follow-up in clastix/kamaji#1327. Kamaji's chart sets no
timeoutSecondson the webhook, so the apiserver's 10s default applies, and a discovery call that can take up to the client's 32s can make that admission time out.The patch is the upstream commit applied unchanged except for the dropped docs hunk, which is not part of the image. It applies cleanly to 26.9.4-edge with no offsets. It modifies different files than the existing
fix-kubelet-config-compat.diff, so the two apply in either order. The upstream tests stay in the patch so that it matches the upstream commit; the image build does not run them. On the patched tree,go build ./...passes, andgo test -racepasses for./controllers/...,./internal/webhook/...,./internal/utilities/...,./internal/kubeadm/...and./cmd/.... I also checked that the new tests can fail: they go red when the availability check is inverted and when the old per-reconcile lookup is put back.If clastix/kamaji#1327 changes before it merges, this patch should be updated to match. It is dropped once Kamaji ships the change.
Refs #4489. This does not close #4489, because this change does not explain the long stall reported there.
Screenshots
No UI change.
Downstream repositories
Release note
Summary by CodeRabbit