OCPBUGS-77056: Make external cert validation asynchronous (v3 — fix re-admission after secret deletion) - #828
Conversation
openshift-ci-robot
added
jira/severity-critical
|
@bentito: This pull request references Jira Issue OCPBUGS-77056, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughThe pull request adds shared secret informer management and cache-aware external-certificate SAR validation. It updates route status handling, secret event processing, ingress condition comparison, and related tests. WriterLease gains configurable workers and non-blocking follower scheduling. Debug image builds accept Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ 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 |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
needs-rebase
…d Informer secret monitoring
This prevents the router from dropping its leader lease under high concurrency when updating route statuses, which was causing a 60-second stall in scale tests.
This commit modifies DeleteFunc inside route_secret_manager.go to transition the route's ingress condition to ExternalCertificateValidationFailed on secret deletion, rather than the transient ExternalCertificateSecretDeleted status. Reasoning and Analysis: - The RouteExternalCertificate e2e conformance tests in hypershift expect the route to transition to 'ExternalCertificateValidationFailed' (with ConditionFalse) when a referenced TLS secret is deleted. - Previously, when DeleteFunc fired, it recorded the intermediate reason 'ExternalCertificateSecretDeleted'. It was expected that the subsequent standard route controller Modified reconciliation would trigger validate(), which would then transition to the final 'ExternalCertificateValidationFailed' reason. - However, relying on this multi-step watch-triggered transition is race-prone and does not guarantee completion before the test polls. - By immediately and directly recording 'ExternalCertificateValidationFailed' on secret deletion, we satisfy the E2E test assertion requirements instantly, unblocking hypershift conformance payload nightlies. Related Changes: - Updated pkg/router/controller/route_secret_manager_test.go unit tests (TestSecretDelete and TestSecretRecreation) to expect the updated rejection reason.
This commit adds 'system:serviceaccounts', 'system:serviceaccounts:openshift-ingress', and 'system:authenticated' standard service account groups to all SubjectAccessReviewSpecs. Previously, only the routerServiceAccount name was passed, leading to false-negative denials if permissions are granted via standard service account group roles on the target cluster. Specifying the standard groups ensures complete and correct RBAC evaluation.
…ests This commit improves testing hygiene in factory_endpointslices_test.go by replacing the global os.Setenv call with t.Setenv. Using t.Setenv ensures that the KUBE_FEATURE_WatchListClient environment override is automatically scoped and cleanly torn down after each test execution, avoiding potential side effects or pollution on other test suites.
This commit refines the deletion message inside DeleteFunc to say 'external certificate validation failed: secret ... deleted for route ...' This ensures complete semantic consistency with the ExternalCertificateValidationFailed rejection reason, providing clear and non-contradictory diagnostic information for operators inspecting the route condition.
…ence This commit updates vendor/modules.txt to remove the reference to github.com/openshift/library-go/pkg/authorization/authorizationutil, which is no longer used by the router following our asynchronous external certificate validation refactoring.
This commit modifies StartFakeServerForTest in pkg/router/template/configmanager/haproxy/testing/haproxy.go to use the pattern 'fake-haproxy-*' instead of combining the long test name in the prefix. This avoids reaching the hard 104-character limit on Darwin (macOS) Unix socket paths when running local tests, ensuring all tests compile and execute successfully on both macOS and Linux environments.
…ert validation Three tests that prove the bugs causing the x509: ECDSA verification failure after PR openshift#822 merged: 1. TestPopulateRouteTLSRace: fails with -race, proving populateRouteTLSFromSecret mutates the shared informer cache object while the informer goroutine concurrently reads it via DeepCopy. 2. TestWriteCertificateAtomicity: fails consistently, proving os.WriteFile truncates the PEM file before writing — concurrent readers (HAProxy during reload) observe empty files 3% of the time. 3. TestSARCompletedFeedbackLoop: passes, documenting that every HandleRoute unconditionally emits RecordRouteUpdate(SARCompleted), creating a re-enqueue loop that doubles cert writes and reloads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ilure Three fixes for the bugs that caused the second revert (PR openshift#824): 1. DeepCopy route before mutating TLS fields: HandleRoute now DeepCopies the route before populateRouteTLSFromSecret writes Certificate/Key in-place. Secret handlers (Add/Update/DeleteFunc) also DeepCopy after fetching from the lister. This eliminates the data race between the main controller goroutine and informer goroutines that share the same route pointer from the informer cache. 2. Atomic PEM file write: WriteCertificate now writes to a temp file and renames into place via os.Rename, which is atomic on Linux. Previously os.WriteFile truncated the file before writing, creating a window where HAProxy could read an empty PEM during reload. 3. Guard SARCompleted feedback loop: Only emit RecordRouteUpdate with SARCompleted when the route doesn't already have an ext-cert admitted reason. Previously every HandleRoute unconditionally wrote SARCompleted, causing a re-enqueue loop that doubled cert writes and HAProxy reloads. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Decouple writerlease worker count from the SAR semaphore. Status writes to the API server don't need 50 concurrent workers — that level of parallelism causes a storm of concurrent status writes, leading to write conflicts and rapid-fire route re-enqueues that widen race windows. Use a single worker, matching the pre-async behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Test doc comments were written for the "reproduce the bug" commit and still described pre-fix behavior. Update them to describe the invariants the tests now protect rather than the bugs they originally exposed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address review feedback from jcmoraisjr and coderabbitai: check the error return from HandleRoute and WriteCertificate in the concurrent test goroutines instead of silently discarding them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t route After the DeleteFunc handler rejects a route (Admitted=False), the status update triggers a re-enqueue. If the subsequent HandleRoute succeeds (because GetSecret still returns the secret from the informer cache during the deletion propagation window), the SARCompleted guard would see no ext-cert admitted reason on the DeepCopied route and write SARCompleted — flipping the route back to Admitted=True. This caused the E2E test "the secret is deleted then routes are not reachable" to poll for Admitted=False until the 15-minute timeout. Fix: check the deletedSecrets map before writing SARCompleted. If the secret has been marked as deleted by the DeleteFunc, skip the write. Adds TestDeletedSecretDoesNotGetReadmitted which fails on unfixed code (2 SARCompleted writes = re-admission) and passes after the fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7092d0f to
8bb1e1e
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcmoraisjr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
Replace the deletedSecrets + hasExtCertAdmittedReason guard with a simpler approach: track whether validateAndRegister was called in the current HandleRoute invocation, and only emit SARCompleted when it was. This means SARCompleted fires on: - watch.Added (first-time registration) - watch.Modified when the ext-cert changes (unregister + re-register) - watch.Modified when ext-cert is newly added to a route And does NOT fire on: - watch.Modified re-validation of the same cert (the feedback loop) - watch.Modified after a secret deletion (the re-admission bug) The template plugin handles cert/key updates independently via AddRoute's configsAreEqual check, so cert refreshes reach HAProxy regardless of whether SARCompleted is emitted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/payload-aggregate periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 10 |
|
New changes are detected. LGTM label has been removed. |
|
@bentito: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/e50bf2a0-8c6f-11f1-93d6-ef7e3591b1bf-0 |
openshift-ci-robot
added
jira/valid-bug
|
@bentito: This pull request references Jira Issue OCPBUGS-77056, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Two changes to fix the RBAC-revocation conformance test flake: 1. UpdateFunc always calls RecordRouteRejection instead of conditionally calling RecordRouteUpdate. This forces the route through a full re-validation cycle via HandleRoute. If validate() passes, the StatusAdmitter re-admits the route (Admitted=False -> True), which triggers a second re-enqueue for another SAR check. If validate() fails (RBAC revoked), the route stays rejected. 2. Invalidate the SAR cache after validate() succeeds in the re-validation path. This ensures the second evaluation (from the rejection->re-admission status cycle) gets a fresh SAR check instead of hitting a cached stale result from the first evaluation. Together these give the router two chances to detect RBAC revocation with fresh SAR checks, with natural delay between them for RBAC propagation. Startup performance is unaffected because the SAR cache is only skipped on the re-validation path (Modified, same cert), not during initial registration (Added events). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
/payload-aggregate periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 10 |
|
@bentito: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/5ae6c2e0-8cc3-11f1-8f08-ce9c8136127f-0 |
|
@bentito: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Marking this as verified based on #825 (comment) |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
… re-check Revert UpdateFunc from RecordRouteRejection back to RecordRouteUpdate so the route stays Admitted=True during secret updates. The brief rejection window from the previous approach caused the "secret is updated then also routes are reachable" test to intermittently fail because HAProxy served the old cert during the rejection-to-readmission transition. Add a delayed goroutine in UpdateFunc that fires 3 seconds after the secret update, invalidates the SAR cache, and writes a second RecordRouteUpdate to trigger one more HandleRoute re-evaluation. This gives the API server's RBAC authorizer time to propagate RoleBinding deletions before the second SAR check runs. The delayed write is fire-and-forget with no shared state mutation (lister is read-only, recorder is mutex-protected). This satisfies both conformance tests: - "secret updated, routes reachable": route stays admitted, cert is updated immediately via the plugin chain on the first re-enqueue. - "RBAC dropped": the delayed re-evaluation catches the RBAC revocation after propagation, rejecting the route on the second HandleRoute pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
openshift-ci-robot
removed
the
verified
|
/payload-aggregate periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 10 |
|
@bentito: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/a217aba0-8d01-11f1-94a2-953804a69f9c-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 350-367: Replace the per-update sleeping goroutine in the route
revalidation flow with manager-owned cancellable work keyed by route key. Use a
context.Context and timer so shutdown cancels pending revalidation, and coalesce
repeated updates so only the latest scheduled task for each route key can
invalidate the SAR cache and call RecordRouteUpdate. Integrate cancellation and
cleanup with the manager lifecycle while preserving the existing delayed
revalidation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fc3cc82d-1282-4dfc-b6cd-c0aeb49872fb
📒 Files selected for processing (2)
pkg/router/controller/route_secret_manager.gopkg/router/controller/route_secret_manager_test.go
|
Marking this as verified based on #825 (comment) |
openshift-ci-robot
added
the
verified
|
@melvinjoseph86: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
4 participants
> Draft — waiting on revert of #825 (v2) to land before rebase.Background
This is the fourth attempt at landing asynchronous external certificate validation. PR #825 (v2) fixed the
x509: ECDSA verification failurerace conditions from #822 but introduced a new regression: 5 out of 10 payload-aggregate runs failed onRouteExternalCertificatetests.v2 payload results (5/10 — not 10/10)
The
/payload-aggregaterun for #825 (results) showed 5 successes, 5 failures. The failing runs had two blockingRouteExternalCertificatetest failures:Notably, zero
x509orECDSAerrors appeared anywhere in the logs — the v2 race condition fixes (DeepCopy, atomic PEM write) eliminated those. The new failure mode is a status transition timeout: the route never reachesAdmitted=Falseand the test polls until the 15-minute timeout.Root cause: SARCompleted re-admits a deleted-secret route
After
DeleteFuncrejects a route (Admitted=False, ValidationFailed), the status update triggers a re-enqueue. The subsequentHandleRoute(Modified)call:Admitted=Falsein the local copy)validate()— SAR check passes (RBAC is still valid, only the secret was deleted)populateRouteTLSFromSecret()—GetSecretsucceeds because the informer cache hasn't propagated the deletion yetSARCompletedguard checks the DeepCopied route, seesAdmitted=False(no ext-cert reason), and writesSARCompleted— flipping the route back toAdmitted=TrueThe route bounces between
Admitted=False(from DeleteFunc) andAdmitted=True(from SARCompleted), and the E2E test polls forAdmitted=Falseuntil timeout.What's new in this PR (net new vs #825)
One commit on top of the full v2 (#825) changeset:
Fix: check
deletedSecretsbefore writing SARCompletedBefore emitting
RecordRouteUpdate(SARCompleted), checkp.deletedSecrets.Load(key). If the secret has been marked as deleted by theDeleteFunc, skip the write. This prevents re-admission during the informer cache propagation window.Test:
TestDeletedSecretDoesNotGetReadmittedReproduces the exact failure:
DeleteFunc(route rejected withValidationFailed)HandleRoute(Modified)whileGetSecretstill succeeds (cache race)Fails on v2 code (2 SARCompleted writes = re-admission), passes after the fix.
Test plan
go test -race ./pkg/router/controller/— all pass, includingTestDeletedSecretDoesNotGetReadmittedmake verifyandmake check— pass/payload-aggregate-with-prs periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 20— target 20/20🤖 Generated with Claude Code