OCPBUGS-77056: Revert #825 "Make external cert validation asynchronous (v2 — race condition fixes)" - #829
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. |
📝 WalkthroughWalkthroughThe pull request updates external-certificate secret handling and validation, removes SAR completion status recording, serializes route status admission, and changes conflict retry behavior. Writer leases now use a single worker loop and revised constructors. Debug image builds no longer force architecture settings. Certificate persistence, Kubernetes client configuration, endpoint-slice tests, fake HAProxy startup, and related tests are also updated. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
|
/jira refresh |
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. |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
110-139: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffProvide a caller-owned context for listener startup.
Line 112 violates the configured
noctxrule. Thread a context into the fake-server startup path and usenet.ListenConfig.Listen; pair cancellation with listener closure if it must stop the accept loop. Verify allfakeHAProxy.Startcallers before changing its API.As per path instructions, “context.Context for cancellation and timeouts.”
🤖 Prompt for 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. In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go` around lines 110 - 139, Update the fakeHAProxy.Start startup path and all callers to accept and propagate a caller-owned context, then use net.ListenConfig.Listen with that context instead of net.Listen. Ensure cancellation closes the listener when needed so the accept loop exits cleanly, while preserving the existing startup error reporting and verifying every fakeHAProxy.Start call is updated consistently.Sources: Path instructions, Linters/SAST tools
pkg/router/controller/route_secret_manager_test.go (1)
1264-1291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInformers never stop and
doneChwaits have no timeout.
context.TODO().Done()isnil, soinformer.Runnever returns and each subtest leaks a reflector goroutine. The bare<-recorder.doneChalso turns any regression into a silent CI hang rather than a test failure.♻️ Suggested test scaffolding
+ ctx, cancel := context.WithCancel(context.Background()) + defer cancel() informer := fakeSecretInformer(kubeClient, "sandbox", "tls-secret") - go informer.Run(context.TODO().Done()) + go informer.Run(ctx.Done()) // wait for informer to start - if !cache.WaitForCacheSync(context.TODO().Done(), informer.HasSynced) { + if !cache.WaitForCacheSync(ctx.Done(), informer.HasSynced) { t.Fatal("cache not synced yet") } @@ - // wait until route's status is updated - <-recorder.doneCh + // wait until route's status is updated + select { + case <-recorder.doneCh: + case <-time.After(30 * time.Second): + t.Fatal("timed out waiting for route status to be recorded") + }Same pattern applies in
TestSecretDeleteandTestSecretRecreation.🤖 Prompt for 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. In `@pkg/router/controller/route_secret_manager_test.go` around lines 1264 - 1291, Update the informer-based tests, including TestSecretDelete and TestSecretRecreation, to use cancellable contexts instead of context.TODO(), defer cancellation, and ensure each informer stops after the test. Replace bare recorder.doneCh waits with bounded timeout-based waits that fail the test when the route status is not updated promptly, including cleanup-safe handling for informer shutdown.
🤖 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/factory/factory_endpointslices_test.go`:
- Around line 80-88: Update newEndpointSliceTestSetup to accept a *testing.T
parameter and replace os.Setenv with t.Setenv for scoped feature-gate
configuration; update both callers to pass their testing.T instance and remove
the process-wide environment mutation.
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 311-315: Update the DeleteFunc callback to safely handle informer
tombstones by extracting the *kapi.Secret from either the direct object or
cache.DeletedFinalStateUnknown payload before accessing secret.Name. Avoid
unchecked type assertions and return without processing when the tombstone
payload is invalid, while preserving the existing key generation and deletion
log behavior for valid secrets.
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 526-563: Update ValidateTLSExternalCertificate and its callers to
accept and propagate a context.Context, then pass that context to all three
authorizationutil.Authorize calls and the secretsGetter Get operation instead of
context.TODO(). Preserve the existing validation and error-handling behavior
while allowing route event processing to cancel or time out these API requests.
- Around line 520-532: The ValidateTLSExternalCertificate helper must guard its
TLS inputs before dereferencing tls.ExternalCertificate.Name. Add an early
validation return for nil TLS or nil ExternalCertificate, or explicitly document
and enforce the required caller precondition using the existing
hasExternalCertificate flow; preserve current validation behavior for valid
external certificates.
In `@pkg/router/template/certmanager.go`:
- Around line 181-187: Update simpleCertificateWriter.WriteCertificate to
preserve atomic replacement: write the certificate to a temporary file in the
same directory, close it and apply the required permissions, then commit it with
os.Rename to the final .pem path. Ensure failures clean up the temporary file
and leave the existing certificate untouched.
In `@pkg/router/writerlease/writerlease.go`:
- Around line 253-255: Update the follower lease handling around the writer
lease queue logic to replace the blocking time.Sleep(remaining) with cancellable
delayed queueing. Use the existing stopCh or context cancellation mechanism so
shutdown interrupts the delay promptly, and ensure the worker is released while
waiting before re-adding key to l.queue.
- Around line 142-151: Update WriterLease.Run to wait for its worker goroutine
to exit after the queue is shut down, using synchronization around the single
goroutine started there. Preserve the existing crash handling and worker
cleanup, and return only after the worker has completed.
---
Nitpick comments:
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 1264-1291: Update the informer-based tests, including
TestSecretDelete and TestSecretRecreation, to use cancellable contexts instead
of context.TODO(), defer cancellation, and ensure each informer stops after the
test. Replace bare recorder.doneCh waits with bounded timeout-based waits that
fail the test when the route status is not updated promptly, including
cleanup-safe handling for informer shutdown.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 110-139: Update the fakeHAProxy.Start startup path and all callers
to accept and propagate a caller-owned context, then use net.ListenConfig.Listen
with that context instead of net.Listen. Ensure cancellation closes the listener
when needed so the accept loop exits cleanly, while preserving the existing
startup error reporting and verifying every fakeHAProxy.Start call is updated
consistently.
🪄 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: 0d16cea7-c3ff-4afc-b414-b2c8c70a460d
⛔ Files ignored due to path filters (3)
vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/subject.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/authorization/authorizationutil/util.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (19)
hack/Makefile.debugpkg/cmd/infra/router/clientcmd.gopkg/cmd/infra/router/template.gopkg/router/controller/contention.gopkg/router/controller/factory/factory_endpointslices_test.gopkg/router/controller/route_secret_manager.gopkg/router/controller/route_secret_manager_test.gopkg/router/controller/shared_secret_manager.gopkg/router/controller/shared_secret_manager_test.gopkg/router/controller/status.gopkg/router/controller/status_test.gopkg/router/routeapihelpers/validation.gopkg/router/routeapihelpers/validation_test.gopkg/router/router_test.gopkg/router/template/certmanager.gopkg/router/template/certmanager_test.gopkg/router/template/configmanager/haproxy/testing/haproxy.gopkg/router/writerlease/writerlease.gopkg/router/writerlease/writerlease_test.go
💤 Files with no reviewable changes (5)
- pkg/router/controller/shared_secret_manager_test.go
- pkg/cmd/infra/router/clientcmd.go
- pkg/router/controller/shared_secret_manager.go
- pkg/router/template/certmanager_test.go
- pkg/router/controller/status_test.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/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/d58bfa20-8b6e-11f1-9542-03214933e98d-0 |
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/payload-aggregate periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-rt-upgrade 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/b3134aa0-8b70-11f1-9977-4d41ff45fa85-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv4 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
@bentito: 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/be81ed60-8b70-11f1-825c-80b1bfcbccc7-0 |
|
/retest |
|
/hold cancel We are fairly certain this is why 5.0.0-0.nightly-2026-07-29-133253 was rejected |
openshift-ci
Bot
added
the
approved
openshift-ci-robot
added
the
verified
|
@stbenjam: 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. |
|
@bentito: Jira Issue OCPBUGS-77056: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-77056 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
5 participants
Reverts #825 ; tracked by OCPBUGS-77056
Per OpenShift policy, we are reverting this change to restore CI signal.
CC: @bentito
This reverts commit c73bada, reversing changes made to 682319a.