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
|
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: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 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
openshift-ci
Bot
removed
needs-rebase
|
/payload-aggregate-with-prs periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 20 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (16)
pkg/cmd/infra/router/clientcmd.go (1)
63-69: 🚀 Performance & Scalability | 🔵 TrivialConfirm the aggregate rate-limit budget.
template.goconstructs one clientset throughConfig.Clients()plus route, project, and authorization clients fromKubeConfig(). Since client-go creates a rate limiter per clientset, these settings can allow roughly 200 QPS and 400 burst across that path rather than 50/100 router-wide. Confirm this aggregate load is intentional and covered by the scale test; otherwise share a limiter or divide the budget. (raw.githubusercontent.com)🤖 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/cmd/infra/router/clientcmd.go` around lines 63 - 69, Review the rate-limit configuration around clientConfig.QPS and clientConfig.Burst against all clientsets created by Config.Clients() and KubeConfig(), including route, project, and authorization clients. Ensure the router-wide aggregate remains within the intended 50 QPS/100 burst budget by sharing a limiter or dividing these values across clientsets, and update or verify the scale-test coverage for the chosen behavior.pkg/router/writerlease/writerlease.go (1)
151-173: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for multiple workers and shutdown.
The changed tests and integrations shown here all pass
1, so the new concurrentRunpath andWaitGroupshutdown behavior remain untested. Add a test with at least two workers, overlapping work, stop-channel closure, and confirmation that all workers exit cleanly.🤖 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/writerlease/writerlease.go` around lines 151 - 173, Add coverage for WriterLease.Run using at least two workers, ensuring their work overlaps before closing stopCh. Verify that closing the stop channel shuts down the queue and that Run waits until every worker exits cleanly, covering the WaitGroup-based shutdown path.pkg/router/controller/status.go (1)
108-117: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAdd coverage for concurrent route event callbacks.
RouterController.HandleRouteserializes events for oneRouterControllerinstance withc.lock, andStatusAdmitter.HandleRouteholds no locking now, but the status controller tests only hit single-threaded admission paths. Add a concurrent test/assurance for multiple route watchers or replica-to-replica contentions to keep this wrapper behavior covered before removing the lock.🤖 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/status.go` around lines 108 - 117, Add concurrent test coverage around StatusAdmitter.HandleRoute and RouterController.HandleRoute, exercising simultaneous callbacks from multiple route watchers or replica-contention scenarios. Verify callbacks remain serialized and status updates stay correct, preserving the existing wrapper behavior before any lock removal.pkg/router/template/certmanager.go (2)
194-204: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider
tmpFile.Sync()beforeClose/Rename.Rename is atomic with respect to concurrent readers, but without an fsync the contents may not be durable across a node crash, leaving a zero-length
<id>.pemon recovery — exactly the state this change aims to prevent.🤖 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/certmanager.go` around lines 194 - 204, Update the temporary certificate write flow after tmpFile.Write and before tmpFile.Close to call tmpFile.Sync and handle any error with the same cleanup and logging behavior as the existing write/close failures. Only proceed to close and rename the file after a successful sync.
187-192: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTemp files land in the certificate directory and can leak.
os.CreateTemp(directory, id+"*.pem.tmp")creates<id><rand>.pem.tmpnext to the live certs. If the router is killed between create and rename, the stray file stays forever (DeleteCertificateonly removes<id>.pem), and any HAProxy config that globs the cert directory could pick it up. Consider a fixed.tmpname per id (so a retry overwrites the stale one) or sweeping*.pem.tmpon startup.Also applies to: 210-214
🤖 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/certmanager.go` around lines 187 - 192, Update the temporary certificate handling around os.CreateTemp in the certificate write flow to prevent random .pem.tmp files from accumulating in the certificate directory or being matched by certificate globs. Use a deterministic per-id temporary filename that retries overwrite, or clean up matching stale temporary files during startup, while preserving the existing rename and error handling behavior.pkg/router/controller/shared_secret_manager.go (3)
171-212: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy lift
notifyscans every registered route per secret event.On the fast path the informer watches all secrets in the namespace, so every unrelated secret change fans into a full O(len(registeredRoutes)) scan under
RLock. At the 2,000-external-cert-route scale target this is the hot path. Consider a secondary indexmap[namespace/secretName][]routeKeymaintained inRegisterRoute/UnregisterRouteand looked up directly here.🤖 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/shared_secret_manager.go` around lines 171 - 212, The notify method currently scans all registeredRoutes for each secret event; add a secondary index keyed by namespace/secretName to retrieve only affected route keys. Maintain this index consistently in RegisterRoute and UnregisterRoute, then update notify to look up the matching route keys and resolve their handlers without iterating every registered route.
109-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
infStateis assigned but never read (golangci-lintineffassign).Only
existsis used; the struct is stored directly into the map.🧹 Proposed cleanup
- infState, exists := m.informers[infKey] - if !exists { + if _, exists := m.informers[infKey]; !exists { @@ ctx, cancel := context.WithCancel(context.Background()) - infState = &informerState{ + infState := &informerState{ informer: inf, cancel: cancel, } m.informers[infKey] = infState🤖 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/shared_secret_manager.go` around lines 109 - 157, Remove the unnecessary infState assignment in the informer-creation branch and store the newly constructed informerState directly in m.informers[infKey]. Preserve the existing informer and cancel initialization while retaining exists for the lookup condition.Source: Linters/SAST tools
111-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winLimit the namespace-wide secret informer scope.
fields.Everything()makes the fast path keep every secret in the namespace in the router’s informer cache, including unrelated service-account tokens and credentials. Use a type selector for the TLS cases (for exampletype=kubernetes.io/tls) or the route-specific secret name when possible to reduce memory and blast radius.🤖 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/shared_secret_manager.go` around lines 111 - 130, Update the selector construction in the shared secret informer setup so the unrestricted path does not use fields.Everything(); constrain TLS-related watches with the appropriate Kubernetes secret type selector (kubernetes.io/tls), while preserving the existing metadata.name selector for restricted watches.pkg/router/routeapihelpers/validation.go (3)
578-636: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three near-identical SAR blocks into a loop.
The
get/watch/listblocks differ only by verb; a loop removes ~50 duplicated lines and keeps the group list in one place.♻️ Sketch
for _, verb := range []string{"get", "watch", "list"} { sar := &authorizationv1.SubjectAccessReview{ Spec: authorizationv1.SubjectAccessReviewSpec{ User: routerServiceAccount, Groups: []string{"system:serviceaccounts", "system:serviceaccounts:openshift-ingress", "system:authenticated"}, ResourceAttributes: &authorizationv1.ResourceAttributes{ Namespace: route.Namespace, Verb: verb, Resource: "secrets", Name: secretName, }, }, } resp, err := sarc.Create(timeoutCtx, sar, metav1.CreateOptions{}) switch { case err != nil: errs = append(errs, field.InternalError(fldPath, fmt.Errorf("failed to check %q permission for secret %q: %v", verb, secretName, err))) case !resp.Status.Allowed: errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("router serviceaccount does not have permission to %s this secret", verb))) } }🤖 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/routeapihelpers/validation.go` around lines 578 - 636, Replace the duplicated SAR handling in the validation flow with a loop over the get, watch, and list verbs. Build each SubjectAccessReview using the shared routerServiceAccount, group list, namespace, resource, and secretName fields, then reuse the existing Create call and append verb-specific internal or forbidden errors while preserving the current behavior.
561-576: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSemaphore is held across four serial API calls; consider narrowing or shortening the timeout.
Each admission holds one of 50 slots for up to 30s while performing 3 SARs + 1 secret GET sequentially. Under the stated 2,000-route scale target with a slow API server this becomes the admission bottleneck (worst case ~40 batches × 30s). Consider a per-call timeout instead of one 30s budget for all four, and/or releasing the slot before the secret GET (which is cache-independent and cheap).
Also applies to: 638-657
🤖 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/routeapihelpers/validation.go` around lines 561 - 576, Update the SAR validation flow around the semaphore acquisition and timeoutCtx so a semaphore slot is not held for the full serial SAR-plus-secret sequence: use appropriately scoped per-call timeouts and release the slot before the independent secret GET where safe, while preserving the cache recheck and existing error aggregation behavior.
513-516: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the whole-map reassignment with
sarCache.Clear().
ClearAsyncSARCacheForTest()replaces the package-levelsync.Mapvariable while cache operations use it concurrently (Load/Store/Delete). Switching to the in-place clear API is safe here because this module targets Go 1.26.♻️ Proposed fix
func ClearAsyncSARCacheForTest() { - sarCache = sync.Map{} + sarCache.Clear() }🤖 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/routeapihelpers/validation.go` around lines 513 - 516, Update ClearAsyncSARCacheForTest to clear the existing sarCache in place using sync.Map’s Clear method instead of reassigning the package-level variable, preserving safe concurrent cache operations.pkg/router/template/certmanager_test.go (1)
296-306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the assertion: each read should equal exactly
certAorcertB.The length heuristic is weak (both blobs are the same length, so a mixed/garbled read of equal length passes) and read errors are swallowed by
continue, which could hide the very failure being tested.💚 Proposed fix
data, err := os.ReadFile(pemPath) if err != nil { + readErrors.Add(1) continue } totalReads.Add(1) if len(data) == 0 { emptyReads.Add(1) - } else if len(data) < len(certA) && len(data) < len(certB) { + } else if !bytes.Equal(data, certA) && !bytes.Equal(data, certB) { truncatedReads.Add(1) }🤖 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/certmanager_test.go` around lines 296 - 306, Strengthen the read validation in the test loop around the certmanager read operation: fail the test on any os.ReadFile error instead of continuing, then classify each successful read only when it exactly matches certA or certB. Preserve the existing counters for empty and truncated reads, while ensuring mixed or garbled data is reported as an invalid read rather than accepted by length.pkg/router/controller/route_secret_manager.go (2)
454-475: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ExtCrtStatusReasonSecretRecreatedis unreachable in this switch.The recreation path records via
RecordRouteRejection(Line 314), i.e.Admitted=False, so thecondition.Status == kapi.ConditionTrueguard above can never see that reason. Dropping it (or documenting why it's listed) keeps the intent clear.🤖 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.go` around lines 454 - 475, The ExtCrtStatusReasonSecretRecreated case in hasExtCertAdmittedReason is unreachable because recreation records Admitted=False; remove that reason from the switch and retain only reasons that can accompany Admitted=True.
360-362: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore or remove
ExtCrtStatusReasonSecretDeleted
ExtCrtStatusReasonSecretDeletedis only referenced in the contention ignore set, while the secret-delete path reportsExtCrtStatusReasonValidationFailed, so the dedicated deleted status reason is no longer active. If the deleted condition semantics are needed, record that reason; otherwise remove the unused constant and contention entry.🤖 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.go` around lines 360 - 362, Align the secret-deletion path in the route reconciliation logic with the status-reason definitions: either record ExtCrtStatusReasonSecretDeleted when handling the deleted secret case, or remove that unused constant and its contention-ignore entry if the dedicated condition is not required. Keep the existing ExtCrtStatusReasonValidationFailed behavior only when the deleted-specific reason is intentionally removed.pkg/router/controller/shared_secret_manager_test.go (1)
17-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage gap:
notifyfan-out andGetSecretfallback are untested.The riskiest logic in
shared_secret_manager.go— multi-route handler fan-out, tombstone (cache.DeletedFinalStateUnknown) handling innotify, andGetSecret's cache-miss → API fallback — has no test. Want me to draft those cases?🤖 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/shared_secret_manager_test.go` around lines 17 - 93, Expand TestSharedSecretManagerHybrid coverage for notify and GetSecret: add cases verifying notify fans events out to every registered route handler, handles cache.DeletedFinalStateUnknown tombstones, and preserves route behavior when handlers are registered through shared informers. Add a GetSecret case that exercises a cache miss followed by the API fallback and validates the returned secret, using the existing manager and fake client setup.pkg/router/controller/contention.go (1)
297-307: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify: the
Type/Statuscomparison is evaluated twice.♻️ Proposed refactor
func conditionsEqual(a, b *routev1.RouteIngressCondition) bool { - if a.Type == b.Type && a.Status == b.Status { - if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) { - return true - } - } - return a.Type == b.Type && - a.Status == b.Status && - a.Reason == b.Reason && - a.Message == b.Message + if a.Type != b.Type || a.Status != b.Status { + return false + } + // Reasons in the ignore set are transient ext-cert bookkeeping; treat them as equal + // so they don't trigger repeated status writes. + if ignoreIngressConditionReason.Has(a.Reason) || ignoreIngressConditionReason.Has(b.Reason) { + return true + } + return a.Reason == b.Reason && a.Message == b.Message }🤖 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/contention.go` around lines 297 - 307, Update conditionsEqual to evaluate the shared Type and Status comparison once, then reuse that result for the ignored-reason early return and the full Type/Status/Reason/Message comparison, preserving the existing equality behavior.
🤖 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 `@hack/Makefile.debug`:
- Around line 15-16: Update the image build command in the debug Makefile to use
the builder-portable --platform $(GOOS)/$(GOARCH) option instead of --arch
$(GOARCH), preserving cross-platform builds for both the default Podman builder
and Docker.
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 59-64: Update statusRecorder.RecordRouteUnservableInFutureVersions
to lazily initialize unservableInFutureVersions before assigning entries,
ensuring all existing &statusRecorder{} constructions safely support the
interface method without nil-map panics.
- Around line 1331-1333: Update the comments for
TestDeletedSecretDoesNotGetReadmitted and the corresponding test section around
SARCompleted to describe the regression being prevented rather than stating the
test is expected to fail. Tighten the SAR update assertion from a greater-than
check to exactly one recorded update, ensuring the test detects both duplicate
re-admission and missing emission.
In `@pkg/router/controller/shared_secret_manager.go`:
- Around line 88-99: Update the permission probe in the shared-secret manager’s
namespace restriction check to cache a result only when the List call succeeds
or returns a definitive Forbidden response. Do not memoize transient or
indeterminate errors in restrictedNamespaces; leave the namespace unknown so a
later probe can retry instead of selecting the namespace-wide informer.
- Around line 64-106: Re-check registeredRoutes[key] after reacquiring m.lock
from the namespace permission probe in RegisterRoute, before the unconditional
assignment. If another registration exists, preserve the existing duplicate
semantics: update and succeed when its secretName matches, otherwise return the
route-already-registered error without overwriting it; only create the new
referencedSecret when the key remains absent.
In `@pkg/router/controller/status.go`:
- Line 308: Update the retry log in the route status update flow to remove or
redact the user-controlled route.Namespace and route.Name fields. Keep the
action context and existing retry message, and do not add unflagged
customer-data logging at this site.
In `@pkg/router/template/certmanager.go`:
- Around line 205-209: Update the os.Chmod call in the certificate bundle
handling flow to use a restrictive 0600 or 0640 mode, matching the existing
os.WriteFile permissions for certificate material. Keep the cleanup, logging,
and error return behavior unchanged.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 123-127: Update the listener loop around p.process to capture and
surface its returned error instead of discarding it. Ensure failures from the
goroutine reach the test caller or fail the test helper, while preserving the
existing connection-accept handling.
---
Nitpick comments:
In `@pkg/cmd/infra/router/clientcmd.go`:
- Around line 63-69: Review the rate-limit configuration around clientConfig.QPS
and clientConfig.Burst against all clientsets created by Config.Clients() and
KubeConfig(), including route, project, and authorization clients. Ensure the
router-wide aggregate remains within the intended 50 QPS/100 burst budget by
sharing a limiter or dividing these values across clientsets, and update or
verify the scale-test coverage for the chosen behavior.
In `@pkg/router/controller/contention.go`:
- Around line 297-307: Update conditionsEqual to evaluate the shared Type and
Status comparison once, then reuse that result for the ignored-reason early
return and the full Type/Status/Reason/Message comparison, preserving the
existing equality behavior.
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 454-475: The ExtCrtStatusReasonSecretRecreated case in
hasExtCertAdmittedReason is unreachable because recreation records
Admitted=False; remove that reason from the switch and retain only reasons that
can accompany Admitted=True.
- Around line 360-362: Align the secret-deletion path in the route
reconciliation logic with the status-reason definitions: either record
ExtCrtStatusReasonSecretDeleted when handling the deleted secret case, or remove
that unused constant and its contention-ignore entry if the dedicated condition
is not required. Keep the existing ExtCrtStatusReasonValidationFailed behavior
only when the deleted-specific reason is intentionally removed.
In `@pkg/router/controller/shared_secret_manager_test.go`:
- Around line 17-93: Expand TestSharedSecretManagerHybrid coverage for notify
and GetSecret: add cases verifying notify fans events out to every registered
route handler, handles cache.DeletedFinalStateUnknown tombstones, and preserves
route behavior when handlers are registered through shared informers. Add a
GetSecret case that exercises a cache miss followed by the API fallback and
validates the returned secret, using the existing manager and fake client setup.
In `@pkg/router/controller/shared_secret_manager.go`:
- Around line 171-212: The notify method currently scans all registeredRoutes
for each secret event; add a secondary index keyed by namespace/secretName to
retrieve only affected route keys. Maintain this index consistently in
RegisterRoute and UnregisterRoute, then update notify to look up the matching
route keys and resolve their handlers without iterating every registered route.
- Around line 109-157: Remove the unnecessary infState assignment in the
informer-creation branch and store the newly constructed informerState directly
in m.informers[infKey]. Preserve the existing informer and cancel initialization
while retaining exists for the lookup condition.
- Around line 111-130: Update the selector construction in the shared secret
informer setup so the unrestricted path does not use fields.Everything();
constrain TLS-related watches with the appropriate Kubernetes secret type
selector (kubernetes.io/tls), while preserving the existing metadata.name
selector for restricted watches.
In `@pkg/router/controller/status.go`:
- Around line 108-117: Add concurrent test coverage around
StatusAdmitter.HandleRoute and RouterController.HandleRoute, exercising
simultaneous callbacks from multiple route watchers or replica-contention
scenarios. Verify callbacks remain serialized and status updates stay correct,
preserving the existing wrapper behavior before any lock removal.
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 578-636: Replace the duplicated SAR handling in the validation
flow with a loop over the get, watch, and list verbs. Build each
SubjectAccessReview using the shared routerServiceAccount, group list,
namespace, resource, and secretName fields, then reuse the existing Create call
and append verb-specific internal or forbidden errors while preserving the
current behavior.
- Around line 561-576: Update the SAR validation flow around the semaphore
acquisition and timeoutCtx so a semaphore slot is not held for the full serial
SAR-plus-secret sequence: use appropriately scoped per-call timeouts and release
the slot before the independent secret GET where safe, while preserving the
cache recheck and existing error aggregation behavior.
- Around line 513-516: Update ClearAsyncSARCacheForTest to clear the existing
sarCache in place using sync.Map’s Clear method instead of reassigning the
package-level variable, preserving safe concurrent cache operations.
In `@pkg/router/template/certmanager_test.go`:
- Around line 296-306: Strengthen the read validation in the test loop around
the certmanager read operation: fail the test on any os.ReadFile error instead
of continuing, then classify each successful read only when it exactly matches
certA or certB. Preserve the existing counters for empty and truncated reads,
while ensuring mixed or garbled data is reported as an invalid read rather than
accepted by length.
In `@pkg/router/template/certmanager.go`:
- Around line 194-204: Update the temporary certificate write flow after
tmpFile.Write and before tmpFile.Close to call tmpFile.Sync and handle any error
with the same cleanup and logging behavior as the existing write/close failures.
Only proceed to close and rename the file after a successful sync.
- Around line 187-192: Update the temporary certificate handling around
os.CreateTemp in the certificate write flow to prevent random .pem.tmp files
from accumulating in the certificate directory or being matched by certificate
globs. Use a deterministic per-id temporary filename that retries overwrite, or
clean up matching stale temporary files during startup, while preserving the
existing rename and error handling behavior.
In `@pkg/router/writerlease/writerlease.go`:
- Around line 151-173: Add coverage for WriterLease.Run using at least two
workers, ensuring their work overlaps before closing stopCh. Verify that closing
the stop channel shuts down the queue and that Run waits until every worker
exits cleanly, covering the WaitGroup-based shutdown path.
🪄 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: 12a8c2d9-9e56-458e-b565-7cad06c56990
⛔ 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
…n test Update TestDeletedSecretDoesNotGetReadmitted to describe the regression it guards rather than pre-fix behavior, and tighten the assertion from sarCount > 1 to sarCount != 1 so it also catches the case where zero SARCompleted updates are recorded. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The org-wide CodeRabbit check 'Ote Binary Stdout Contract' prohibits writing to stdout from process-level setup code like TestMain. Our TestMain in router_test.go was calling fmt.Println on flag-set errors. Fix: redirect error output to os.Stderr using fmt.Fprintln.
|
/assign @jcmoraisjr |
|
/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/1bf924a0-8c24-11f1-9be4-93afaa1ca3d4-0 |
|
I manually compared the previous and already approved branch with the current one. It differs only in the deletedSecrets hashmap being checked before emitting /approve |
|
[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 |
3 participants
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