OCPBUGS-77056: Make external cert validation asynchronous (Resurrection) - #822
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. |
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (17)
WalkthroughThe PR adds shared Secret informer management, throttled and cached external-certificate SAR validation, SAR completion status reporting, concurrent writer lease workers, and related controller, build, and test-harness updates. ChangesExternal certificate processing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RouteSecretManager
participant SharedSecretManager
participant ValidateTLSExternalCertificate
participant KubernetesAPI
RouteSecretManager->>SharedSecretManager: Register route and referenced Secret
SharedSecretManager->>KubernetesAPI: Probe permissions and watch Secrets
RouteSecretManager->>ValidateTLSExternalCertificate: Validate external certificate
ValidateTLSExternalCertificate->>KubernetesAPI: Create SARs and fetch TLS Secret
KubernetesAPI-->>ValidateTLSExternalCertificate: Authorization and Secret results
ValidateTLSExternalCertificate-->>RouteSecretManager: Validation result
RouteSecretManager->>RouteSecretManager: Record SAR completion status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
|
/test verify-deps |
|
/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/d2e30930-85e7-11f1-9f54-ba31857f6125-0 |
|
/test e2e-agnostic |
openshift-ci
Bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/router/writerlease/writerlease_test.go (1)
9-9: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd coverage for multiple workers.
All updated tests use
workers=1; add a boundedworkers=2test that verifies two distinct keys execute concurrently and shutdown waits for both workers.Also applies to: 33-33, 53-53, 75-75, 115-115
🤖 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_test.go` at line 9, Add a bounded test alongside the existing New calls in the writerlease tests using workers=2, with two distinct keys whose work blocks until both have started to prove concurrent execution. Release the work, shut down the writer lease, and assert shutdown does not return until both workers finish.pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
110-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the
noctxlint finding.
golangci-lintreports directnet.Listenusage here; usenet.ListenConfig.Listenwith an appropriate context, or add a narrow documented exception if this synchronous test-only bind is intentionally exempt.🤖 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` at line 110, Replace the direct net.Listen call in the test setup with net.ListenConfig.Listen using an appropriate context, preserving the existing network and socket behavior; only add a narrow documented noctx exception if this synchronous test-only bind is intentionally exempt.Sources: Path instructions, Linters/SAST tools
pkg/cmd/infra/router/template.go (1)
818-818: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDecouple writer-lease worker count from the SAR throttle constant, and review write amplification.
Passing
routeapihelpers.MaxConcurrentSARChecksas the writer-leaseworkersargument ties status-write parallelism to an unrelated SAR-concurrency limit—future tuning of one silently changes the other. It also raises status-write concurrency to 50 workers, which combined with the client-sideQPS=50/Burst=100could saturate the client rate limiter and increase status-update conflicts under scale. Consider a dedicated named constant and validating the API write load.🤖 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/template.go` at line 818, Update the writer-lease initialization in the route template generation flow to use a dedicated named worker-count constant instead of routeapihelpers.MaxConcurrentSARChecks. Choose a conservative value for status writes, define it alongside the related configuration constants, and keep SAR concurrency tuning independent from writer-lease parallelism.pkg/router/controller/shared_secret_manager.go (1)
111-118: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffFast-path caches all namespace secrets in router memory.
Using
fields.Everything()makes the shared informer list/watch and cache every Secret (includingData) in each namespace that has an external-cert route. In namespaces with many/large secrets this increases the router's memory footprint and the amount of secret material held in-process. Consider bounding memory (or documenting expected footprint) for high-secret-count namespaces.🤖 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 - 118, Update the unrestricted selector logic in the shared-secret manager’s informer setup so it does not use fields.Everything() to cache every namespace Secret. Bound the list/watch to only the secrets required by external-cert routes, or otherwise apply an explicit memory-limiting strategy while preserving the restricted secretName 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 `@pkg/router/controller/shared_secret_manager.go`:
- Around line 108-109: The map lookup in the informer-management flow assigns
infState before it is used, then overwrites it in the !exists branch. Remove the
unused infState assignment from the initial lookup while preserving the exists
check and subsequent informer-state initialization behavior.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 109-112: Update fake HAProxy Start’s listener goroutine to report
net.Listen and unexpected listener.Accept failures through a test-visible error
mechanism instead of panicking or silently exiting; ensure startup failure still
signals completion appropriately, and treat the expected
shutdown/closed-listener error as non-fatal while propagating other accept
errors.
---
Nitpick comments:
In `@pkg/cmd/infra/router/template.go`:
- Line 818: Update the writer-lease initialization in the route template
generation flow to use a dedicated named worker-count constant instead of
routeapihelpers.MaxConcurrentSARChecks. Choose a conservative value for status
writes, define it alongside the related configuration constants, and keep SAR
concurrency tuning independent from writer-lease parallelism.
In `@pkg/router/controller/shared_secret_manager.go`:
- Around line 111-118: Update the unrestricted selector logic in the
shared-secret manager’s informer setup so it does not use fields.Everything() to
cache every namespace Secret. Bound the list/watch to only the secrets required
by external-cert routes, or otherwise apply an explicit memory-limiting strategy
while preserving the restricted secretName behavior.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Line 110: Replace the direct net.Listen call in the test setup with
net.ListenConfig.Listen using an appropriate context, preserving the existing
network and socket behavior; only add a narrow documented noctx exception if
this synchronous test-only bind is intentionally exempt.
In `@pkg/router/writerlease/writerlease_test.go`:
- Line 9: Add a bounded test alongside the existing New calls in the writerlease
tests using workers=2, with two distinct keys whose work blocks until both have
started to prove concurrent execution. Release the work, shut down the writer
lease, and assert shutdown does not return until both workers finish.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dee1794-61fa-4494-ba5e-5ede45df1b01
⛔ 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 (17)
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/configmanager/haproxy/testing/haproxy.gopkg/router/writerlease/writerlease.gopkg/router/writerlease/writerlease_test.go
…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.
357b83b to
da1c969
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
|
/payload-aggregate-with-prs periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 10 #822 |
| conn, err := listener.Accept() | ||
| if err != nil { | ||
| return err | ||
| return |
There was a problem hiding this comment.
minor: maybe this should be a panic as well?
There was a problem hiding this comment.
yeah let's not wait on a cycle of testing for this. I'll make a note for small follow up PR. thanks
|
/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
|
@bentito I compared this PR with the former one, changes sounds good. Just found a hidden error message, but it was being hidden before this change as well, so it doesn't worth to run all the CI because of that, and future updates should remove that test as well. Approved, lgtm-ed. |
All 10 runs of the HyperShift AWS OVN Conformance suite built with our PR #822 payload completed successfully. Previously had a flaky 40% failure rate (4/10 runs) on PR #745 |
|
the functionality is already verified with #745 (comment), but seems one more check can be done, hence will verify again after the merge.. |
openshift-ci-robot
added
verified-later
verified
|
@melvinjoseph86: This PR has been marked to be verified later 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@melvinjoseph86: 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. |
|
all tests were passed before /override ci/prow/e2e-agnostic |
|
@bentito: Overrode contexts on behalf of bentito: ci/prow/e2e-agnostic, ci/prow/e2e-aws-fips, ci/prow/e2e-aws-serial-1of2, ci/prow/e2e-aws-serial-2of2, ci/prow/e2e-upgrade, ci/prow/unit 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 kubernetes-sigs/prow repository. |
|
@bentito: Jira Issue OCPBUGS-77056: All pull requests linked via external trackers have merged: This pull request has the 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: 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. |
|
Fix included in release 5.0.0-0.nightly-2026-07-25-032712 |
…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>
…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>
6 participants
This PR is a resurrection of PR #745 ("Make external cert validation asynchronous") which was reverted in PR #817 due to a regression in the
RouteExternalCertificatehypershift conformance tests.What PR #745 Did
It implemented semaphore-throttled, cached, asynchronous checks using
SubjectAccessReviewfor routespec.tls.externalCertificateduring router startup. This maintains full RBAC security checks while executing them asynchronously in the background, avoiding theO(N * API_latency)startup Probe delays causingCrashLoopBackOffwhen scaling to thousands of routes.Reversion Root Cause
When a referenced certificate secret was deleted, the route's ingress condition stayed in the successful
ExternalCertificateSARCompleted/Truestate instead of transitioning toExternalCertificateValidationFailed/False. The e2e conformance tests checked for this transition and timed out. This was due to:conditionsEqualin master ignoring status transitions when one of the reasons belonged to the ignored set.DeleteFuncof the secret handler recording a transientExternalCertificateSecretDeleted/Falsestatus and relying on a separate watch-triggered re-evaluation cycle to hitvalidate()and record the finalExternalCertificateValidationFailedstate. This multi-step process was race-prone and didn't reliably complete before the test polled.What This Resurrection Adds (Robustness & Bug Fixes)
DeleteFuncinroute_secret_manager.goto directly and immediately recordExternalCertificateValidationFailed/Falsestatus upon secret deletion. This satisfies the E2E test assertions instantly, avoids a secondary status update, and eliminates any race conditions.Groupsfield on all SAR requests with standard service account groups (system:serviceaccounts,system:serviceaccounts:openshift-ingress, andsystem:authenticated). This ensures correct RBAC evaluation on clusters where permissions are granted via group-level roles, preventing false-negative denials.os.Setenvmutations witht.Setenvinfactory_endpointslices_test.goto avoid process-wide test environment pollution.conditionsEqual()to ensure it correctly comparesTypeandStatusbefore ignoring reasons."external certificate validation failed: secret ... deleted"to keep it consistent with the rejection reason.All unit tests compiled, executed, and passed cleanly with Go 1.26 and Kubernetes 1.36.2.
Summary by CodeRabbit
New Features
Bug Fixes