OCPBUGS-77056: Lock DeleteFunc to stop SARCompleted overwriting rejection - #840
OCPBUGS-77056: Lock DeleteFunc to stop SARCompleted overwriting rejection#840bentito wants to merge 1 commit into
Conversation
…tion On secret deletion, DeleteFunc marks the route in deletedSecrets and records an ExternalCertificateValidationFailed rejection. HandleRoute's tail guards its SARCompleted write with a deletedSecrets.Load() check, but that check and the RecordRouteUpdate(SARCompleted) write are not atomic relative to DeleteFunc's Store + rejection. An in-flight registration that loaded deletedSecrets before DeleteFunc stored it can finish afterward and overwrite the rejection, leaving the route stuck at [ExternalCertificateSARCompleted, True] with its backing secret already gone. Every registered path in HandleRoute already holds the per-route lock through its tail (via validateAndRegister's deferred unlock), so having DeleteFunc acquire that same lock around its deletedSecrets.Store + RecordRouteRejection serializes the two paths. Whichever side runs second observes the other's write, making the rejection authoritative in both orderings. Adds TestDeleteFuncSerializesWithRegistrationLock, which asserts DeleteFunc blocks while the per-route lock is held (fails before this change, passes after), and corrects the overstated comment on the SARCompleted guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
openshift-ci-robot
added
jira/severity-critical
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Secret deletion now serializes with route registration so deletion rejection status is retained rather than overwritten by a stale registration completion. The focused test covers the lock ordering and resulting rejection behavior; no merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
openshift-ci
Bot
added
the
ready-for-human-review
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/pipeline required |
|
Scheduling tests matching the |
|
@melvinjoseph86: it appears that you have attempted to use some version of the payload command, but your comment was incorrectly formatted and cannot be acted upon. See the docs for usage info. |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.1-e2e-vsphere-ovn-rhcos10-techpreview periodic-ci-openshift-release-main-nightly-5.1-e2e-vsphere-ovn-techpreview |
|
@melvinjoseph86: trigger 2 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/1fd0aed0-a886-11f1-9087-6fdd89bb05a9-0 |
|
one more time |
|
@melvinjoseph86: trigger 2 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/5700df00-a8c3-11f1-886d-470f4a308805-0 |
|
/retest-required |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-rhcos10-techpreview periodic-ci-openshift-release-main-nightly-5.0-e2e-vsphere-ovn-techpreview |
|
@melvinjoseph86: trigger 2 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/fb7abc90-a8c3-11f1-8313-a8eedcd57e02-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. |
3 participants
What
Fixes a residual race in the merged external-certificate fix (#828) that the Component Readiness / QSE triage bot flagged on OCPBUGS-77056 (re-regression of
OCPFeatureGate:RouteExternalCertificate, techpreview variants across 4.22/4.23/5.0/5.1).The race
On secret deletion,
DeleteFuncmarks the route indeletedSecretsand records anExternalCertificateValidationFailedrejection.HandleRoute's tail guards itsSARCompletedwrite with adeletedSecrets.Load()check — but that check and theRecordRouteUpdate(SARCompleted)write are not atomic relative toDeleteFunc'sStore+ rejection. Losing interleaving:HandleRoute(Added/registered) loadsdeletedSecrets→ not deletedDeleteFuncstoresdeletedSecrets, records rejection[ValidationFailed, False]HandleRoutewrites[SARCompleted, True], overwriting the rejectionResult: after the backing secret is deleted, the route's ingress condition is stuck at
[ExternalCertificateSARCompleted, True]instead of flipping to[ExternalCertificateValidationFailed, False], which is what the e2e test (test/extended/router/external_certificate.go, "and the secret is deleted → then routes are not reachable") asserts. It surfaces as a race under the parallel conformance suite's concurrency/API load, not in an isolated single-spec run.The fix
Every
registeredpath inHandleRoutealready holds the per-route lock (routeLocks) through its tail viavalidateAndRegister's deferred unlock. HavingDeleteFuncacquire that same per-route lock around itsdeletedSecrets.Store+RecordRouteRejectionserializes the two paths, so whichever runs second observes the other's write and the rejection stays authoritative in both orderings.Test
Adds
TestDeleteFuncSerializesWithRegistrationLock, which assertsDeleteFuncblocks while the per-route lock is held (fails before this change, passes after). The existingTestInFlightRegistrationDoesNotReAdmitDeletedSecretRouteonly covered the fully-sequential ordering thedeletedSecretsguard already handled; this covers the interleaving that actually regressed. Fullpkg/router/controllersuite passes under-race.🤖 Generated with Claude Code