OCPBUGS-77056: Make external cert validation asynchronous#745
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-critical
|
@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: 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-merge-robot
added
the
needs-rebase
bentito
changed the title
bentito
force-pushed
the
OCPBUGS-77056-async-sar
branch
from
325f313 to
214b603
Compare
openshift-merge-robot
removed
the
needs-rebase
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSemaphore-throttled, cached SAR validation for external TLS; new RBAC-aware SharedSecretManager with hybrid informer strategy; SAR-completed route status events and condition equality fixes; WriterLease gains worker concurrency; synchronous test refactors and small build/client wiring tweaks. ChangesExternal Certificate Validation, Secret Management, and Route Status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Important 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 288-298: The current Add handler unconditionally calls
RecordRouteRejection when a secret is replayed; change it to mirror the
UpdateFunc/SAR-completion logic: check the route's admission (via
populateRouteTLSFromSecret result or route.Status.Conditions) and if the route
is already admitted emit a normal update event instead of calling
RecordRouteRejection, otherwise record the rejection; extract the string
"ExternalCertificateSecretLoaded" into a shared constant (e.g.,
ExtCrtStatusReasonSecretLoaded) and add that constant to the
ignoreIngressConditionReason set in contention.go so the admission-ignoring
logic treats this replay reason as benign. Ensure you update references to
RecordRouteRejection, populateRouteTLSFromSecret, UpdateFunc,
ExternalCertificateSecretLoaded, and ignoreIngressConditionReason accordingly.
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 503-556: The async SAR flow stores only a single pending error
(pendingErr) in asyncSARCache for a cacheKey, so subsequent calls (in validate
flow) that hit the cache drop their onComplete callback and never get
re-enqueued; change the cache value to include a list of waiting callbacks (or a
small struct with errs + []onComplete callbacks) keyed by asyncSARCache so that
when you detect a pending entry you append the current onComplete for
route.Namespace/secretName, and in the goroutine after
asyncSARCache.Store(cacheKey, errs) iterate and invoke all stored callbacks (not
just the first) and then replace the cache entry with errs only; update code
paths that read the cache (the cache-hit branch and the goroutine completion) to
handle this new struct and ensure onComplete is invoked for every waiting route.
- Around line 485-490: The global asyncSARCache currently stores completed
validation results forever; change it to avoid permanent sticky entries by (a)
replacing the raw sync.Map value with a small struct that includes the cached
field.ErrorList plus an expiration timestamp or a source type flag, (b) only
writing non-transient successful validation results (or results with a short
TTL) into asyncSARCache, and (c) adding explicit invalidation helpers (e.g.,
InvalidateAsyncSARCache(namespace, secretName) and
InvalidateAllAsyncSARCacheForSecret(namespace, secretName)) and call them from
the secret add/update/delete/recreate paths in the secret manager
(route_secret_manager.go) so secret changes force revalidation; also update
ClearAsyncSARCacheForTest to clear the new structure. Ensure references to
asyncSARCache and ClearAsyncSARCacheForTest (and the code paths mentioned around
the 503-556 region) are updated to use the new value type and invalidation APIs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e07f375c-945a-4e58-b5ec-3347eaff109c
📒 Files selected for processing (5)
pkg/router/controller/contention.gopkg/router/controller/route_secret_manager.gopkg/router/controller/route_secret_manager_test.gopkg/router/routeapihelpers/validation.gopkg/router/routeapihelpers/validation_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/router/routeapihelpers/validation.go (1)
485-497:⚠️ Potential issue | 🟠 MajorCompleted SAR entries are still permanent.
Only secret add/update/delete clears this cache. A finished allow/deny result for
namespace/secretNamesurvives route deletion, later RBAC grants/revocations, and transient SAR/API failures, so a new route that reuses the same secret name can inherit a stale decision without performing a fresh check. Please put completed entries behind a TTL/generation and avoid caching transient internal errors indefinitely.Also applies to: 593-605
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/routeapihelpers/validation.go` around lines 485 - 497, The cache currently stores completed asyncSARResult entries forever; update asyncSARResult (and uses of asyncSARCache) to include a timestamp or generation counter and enforce a TTL/generation check before returning cached results so that completed allow/deny decisions expire after a short interval or when generation changes. Ensure only stable allow/deny outcomes are cached long-term; if asyncSARResult.errs contains transient/internal errors, set a much shorter TTL (or do not mark done) so they trigger fresh SARs. Update the cache read path to validate timestamp/generation and the write path to record it, and keep InvalidateAsyncSARCache(namespace, secretName) as-is to support manual invalidation.pkg/router/controller/route_secret_manager.go (1)
262-304:⚠️ Potential issue | 🟠 MajorDon't clear the fresh SAR result on the initial
SecretLoadedreplay.
RegisterRoute()is only reached aftervalidate()has already completed successfully, so this non-deletedAddFuncpath is replaying an already-validated secret. ClearingasyncSARCachehere throws away that fresh result, defeats the per-secret cache for shared secrets, and can bounce the route back into a second"authorization check pending"/ExternalCertificateValidationFailedcycle on theSecretLoadedrequeue. Keep invalidation on actual secret changes (recreated/updated/deleted), but not on the initial load replay.Suggested shape
AddFunc: func(obj interface{}) { secret := obj.(*kapi.Secret) log.V(4).Info("Secret added for route", "namespace", namespace, "secret", secret.Name, "route", routeName) - routeapihelpers.InvalidateAsyncSARCache(namespace, secret.Name) // Secret re-creation scenario // Check if the route key exists in the deletedSecrets map, indicating that the secret was previously deleted for this route. // If it exists, it means the secret is being recreated. Remove the key from the map and proceed with handling the route. @@ key := generateKey(namespace, routeName) if _, deleted := p.deletedSecrets.LoadAndDelete(key); deleted { + routeapihelpers.InvalidateAsyncSARCache(namespace, secret.Name) log.V(4).Info("Secret recreated for route", "namespace", namespace, "secret", secret.Name, "route", routeName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/route_secret_manager.go` around lines 262 - 304, The AddFunc for secret events currently calls routeapihelpers.InvalidateAsyncSARCache unconditionally, which discards fresh per-secret async SAR results during the initial cache-sync replay; to fix, remove the unconditional call to routeapihelpers.InvalidateAsyncSARCache from the top of the AddFunc in RegisterRoute's secret handler and only invoke routeapihelpers.InvalidateAsyncSARCache when the secret is actually changed/recreated/deleted (e.g., inside the p.deletedSecrets.LoadAndDelete "recreated" branch and in the Update/Delete handlers), using generateKey(namespace, routeName) and p.deletedSecrets to determine real recreation so you don't invalidate the SAR cache on the initial load.
🧹 Nitpick comments (1)
pkg/router/controller/route_secret_manager_test.go (1)
1355-1372: Put a timeout around these async waits.These bare channel receives will hang the whole suite on any missed informer callback. Now that the tests intentionally depend on async completion, a tiny helper with
select+time.Afterwill turn those deadlocks into normal test failures.Suggested helper
+func waitForRecorderEvent(t *testing.T, ch <-chan struct{}) { + t.Helper() + select { + case <-ch: + case <-time.After(2 * time.Second): + t.Fatal("timeout waiting for recorder event") + } +}Then replace each bare receive with:
-<-recorder.doneCh +waitForRecorderEvent(t, recorder.doneCh)Also applies to: 1434-1444, 1496-1516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/controller/route_secret_manager_test.go` around lines 1355 - 1372, Replace bare channel receives on recorder.doneCh with a timed wait helper to avoid test hangs: add a small helper (e.g., waitForDone(ctxTimeout, ch) or waitWithTimeout(t, ch, duration)) that uses select with the channel and time.After to fail the test on timeout, then update every occurrence of "<-recorder.doneCh" in route_secret_manager_test.go (including the occurrences around the Secret Update flow that use kubeClient.CoreV1().Secrets(...).Update and the initial Add wait) to call that helper instead so missed informer callbacks produce a deterministic test failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 262-304: The AddFunc for secret events currently calls
routeapihelpers.InvalidateAsyncSARCache unconditionally, which discards fresh
per-secret async SAR results during the initial cache-sync replay; to fix,
remove the unconditional call to routeapihelpers.InvalidateAsyncSARCache from
the top of the AddFunc in RegisterRoute's secret handler and only invoke
routeapihelpers.InvalidateAsyncSARCache when the secret is actually
changed/recreated/deleted (e.g., inside the p.deletedSecrets.LoadAndDelete
"recreated" branch and in the Update/Delete handlers), using
generateKey(namespace, routeName) and p.deletedSecrets to determine real
recreation so you don't invalidate the SAR cache on the initial load.
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 485-497: The cache currently stores completed asyncSARResult
entries forever; update asyncSARResult (and uses of asyncSARCache) to include a
timestamp or generation counter and enforce a TTL/generation check before
returning cached results so that completed allow/deny decisions expire after a
short interval or when generation changes. Ensure only stable allow/deny
outcomes are cached long-term; if asyncSARResult.errs contains
transient/internal errors, set a much shorter TTL (or do not mark done) so they
trigger fresh SARs. Update the cache read path to validate timestamp/generation
and the write path to record it, and keep InvalidateAsyncSARCache(namespace,
secretName) as-is to support manual invalidation.
---
Nitpick comments:
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 1355-1372: Replace bare channel receives on recorder.doneCh with a
timed wait helper to avoid test hangs: add a small helper (e.g.,
waitForDone(ctxTimeout, ch) or waitWithTimeout(t, ch, duration)) that uses
select with the channel and time.After to fail the test on timeout, then update
every occurrence of "<-recorder.doneCh" in route_secret_manager_test.go
(including the occurrences around the Secret Update flow that use
kubeClient.CoreV1().Secrets(...).Update and the initial Add wait) to call that
helper instead so missed informer callbacks produce a deterministic test
failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c800f527-b5e1-4386-b6cf-e20eb4726a31
📒 Files selected for processing (4)
pkg/router/controller/contention.gopkg/router/controller/route_secret_manager.gopkg/router/controller/route_secret_manager_test.gopkg/router/routeapihelpers/validation.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/router/controller/contention.go
|
/retest-required |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 561-568: The semaphore (asyncSARSemaphore) is being acquired on
the caller path before launching the goroutine, causing the caller to block when
the limit is reached; move the acquire into the goroutine so the caller can
spawn all per-secret goroutines without blocking, then release the token inside
that goroutine (use defer to ensure release). Concretely: start the goroutine
immediately, perform asyncSARSemaphore <- struct{}{} at the top of the goroutine
body, and keep the existing defer func() { <-asyncSARSemaphore }() to guarantee
release; ensure any early returns in the goroutine still release the token.
- Around line 532-535: The current guard that returns nil when sarc or
secretsGetter is nil hides wiring bugs and skips RBAC/secret validation;
instead, change the conditional in validation.go so that if sarc == nil ||
secretsGetter == nil you return an internal error (e.g., fmt.Errorf or the
package's internal/server error type) describing "missing validation clients"
rather than nil, or alternatively gate this behind a test-only seam/flag so
production never silently succeeds; update callers to propagate/handle the
returned error as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31462f9b-48bb-466f-8d90-5c399b5fab8b
📒 Files selected for processing (1)
pkg/router/routeapihelpers/validation.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 526-529: The code currently returns a synthetic pending error on
cache miss without registering a callback if onComplete is nil; change the logic
in the validation path (the block that manipulates cached.callbacks and returns
cached.errs) so that onComplete must never be nil: either (A) make onComplete a
required parameter at the API boundary, or (B) when onComplete == nil execute
the async validation synchronously (drive the same validation routine on the
current goroutine, wait for completion, populate cached.errs, and then return
the real errors) instead of just returning the pending placeholder; if you
choose (B) ensure you reuse the same validation function that enqueues callbacks
(so cached state is updated) and that cached.callbacks is appended only when
onComplete is non-nil or after synchronous completion you return the finalized
errors.
- Around line 572-599: Wrap each SAR and secret GET with a per-request timeout
context (use context.WithTimeout and defer cancel) instead of context.TODO();
for the SAR checks stop using authorizationutil.Authorize and call the SAR
client Create() directly (e.g., sarClient.Create) with that timeout context so
you can inspect both (response, err) separately, treat API/transport errors as
retryable/internal (append field.InternalError or a distinct cached error) and
only append field.Forbidden when the SAR response explicitly denies, and call
secretsGetter.Secrets(route.Namespace).Get with the same bounded context and map
Get errors to NotFound vs InternalError accordingly. Ensure you still reference
routerServiceAccount, secretName, fldPath and preserve existing
field.Forbidden/field.InternalError/field.NotFound usages when classifying the
outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbebc97e-5cff-40e0-80d7-bff7bf16a68c
📒 Files selected for processing (1)
pkg/router/routeapihelpers/validation.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/router/routeapihelpers/validation.go (1)
637-639: Consider recovering from callback panics to ensure all callbacks are invoked.If a callback panics, the remaining callbacks in the slice won't be invoked. This is unlikely but could leave some routes stuck in a pending state if one route's callback handler has a bug.
🛡️ Proposed defensive callback invocation
for _, cb := range callbacks { - cb(route.Namespace, secretName) + func() { + defer func() { + if r := recover(); r != nil { + // Log panic but continue invoking remaining callbacks + } + }() + cb(route.Namespace, secretName) + }() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/router/routeapihelpers/validation.go` around lines 637 - 639, Wrap each callback invocation in a deferred recover to prevent a single panic from aborting the loop: inside the loop over callbacks (callbacks, cb) call each cb via a small closure that uses defer + recover to catch any panic, log or report the panic along with the callback context (e.g., route.Namespace and secretName) and continue to the next cb; ensure the recovered panic does not re-panic so all callbacks are invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 637-639: Wrap each callback invocation in a deferred recover to
prevent a single panic from aborting the loop: inside the loop over callbacks
(callbacks, cb) call each cb via a small closure that uses defer + recover to
catch any panic, log or report the panic along with the callback context (e.g.,
route.Namespace and secretName) and continue to the next cb; ensure the
recovered panic does not re-panic so all callbacks are invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9c78ea5-dba5-483e-8d75-13dc6f9905d7
📒 Files selected for processing (1)
pkg/router/routeapihelpers/validation.go
openshift-merge-robot
added
the
needs-rebase
bentito
force-pushed
the
OCPBUGS-77056-async-sar
branch
from
716e042 to
0a99ee0
Compare
openshift-merge-robot
removed
the
needs-rebase
bentito
force-pushed
the
OCPBUGS-77056-async-sar
branch
from
0a99ee0 to
4e1eae4
Compare
|
Depends-On: openshift/library-go#2132 |
|
/retest |
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.
|
/retest |
| new-openshift-router-image: | ||
| GO111MODULE=on CGO_ENABLED=0 GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router | ||
| $(IMAGEBUILDER) build -t $(LOCAL_IMAGE) -f hack/Dockerfile.debug . | ||
| GO111MODULE=on CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) GOFLAGS=-mod=vendor go build -o openshift-router -gcflags=all="-N -l" ./cmd/openshift-router |
There was a problem hiding this comment.
same comment as on the GOARCH definition
|
/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. |
|
@coderabbitai can you clean up your old comments and re-review this PR comprehensively? |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# List all review comments made by coderabbitai on PR `#745`
gh api repos/openshift/router/pulls/745/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:80])}' 2>/dev/null
echo "---REVIEW COMMENTS ABOVE---"
# Also check issue comments (top-level PR comments)
gh api repos/openshift/router/issues/745/comments \
--jq '.[] | select(.user.login == "coderabbitai[bot]") | {id: .id, body: (.body | .[0:80])}' 2>/dev/nullLength of output: 3352 🏁 Script executed: #!/bin/bash
# Get all review threads for PR `#745` and resolve the ones from coderabbitai
THREADS=$(gh api graphql -f query='
{
repository(owner: "openshift", name: "router") {
pullRequest(number: 745) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 1) {
nodes {
author { login }
body
}
}
}
}
}
}
}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false) | select(.comments.nodes[0].author.login == "coderabbitai[bot]") | .id')
echo "Unresolved coderabbitai threads:"
echo "$THREADS"
echo "---"
# Resolve each thread
for thread_id in $THREADS; do
echo "Resolving thread: $thread_id"
gh api graphql -f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread { id isResolved }
}
}' -f threadId="$thread_id" --jq '.data.resolveReviewThread.thread' 2>/dev/null || echo "Failed to resolve $thread_id"
doneLength of output: 1128
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
110-127:⚠️ Potential issue | 🟠 MajorPropagate fake HAProxy listener failures explicitly instead of panicking/silently exiting.
Line 113 panics inside a goroutine on bind failure, which crashes the test process abruptly and prevents proper error handling. Line 126 returns on any
Accepterror without surfacing the cause, making test failures flaky and hard to diagnose. TheStart()method returns void, so callers cannot detect failures at all.Convert
Start()to returnerror, replace the panic with explicit error propagation through an error channel, and handleAcceptfailures by differentiating between shutdown and unexpected errors. Usecontext.Background()withnet.ListenConfigfor consistency with Go best practices.🤖 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 - 127, The fakeHAProxy Start() method needs to be refactored to return an error instead of void, and the goroutine it spawns should propagate errors explicitly rather than panicking or silently exiting. Replace the panic call on the net.Listen failure with sending the error through an error channel that Start() can read from before returning. Update the listener.Accept() error handling to differentiate between expected errors during shutdown (when p.shutdown is true) and unexpected errors, propagating unexpected ones through the error channel or logging mechanism. Additionally, modernize the net.Listen call to use net.ListenConfig with context.Background() for consistency with current Go best practices. Ensure the error channel is created, read from in Start() to detect initialization failures, and properly closed to prevent goroutine leaks.Sources: Coding guidelines, Linters/SAST tools
pkg/router/routeapihelpers/validation_test.go (1)
2512-2523:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep this case as a rejection unless the API contract intentionally changed.
The test still says a key field containing certificate/CA-chain material “should be rejected”, but
expectedErrors: 0now accepts that malformed key field and masks the validation regression. Either restore the rejection expectation and fix the validator, or rename/comment this case with an explicit contract-change rationale.🧪 Proposed test expectation if rejection remains the contract
- expectedErrors: 0, + expectedErrors: 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/routeapihelpers/validation_test.go` around lines 2512 - 2523, The test case in the validation_test.go file has an inconsistency where the test comment states that a key field containing Public Key/certificate attributes "should be rejected", but expectedErrors is set to 0, which means the test is accepting the malformed input rather than rejecting it. Either update the expectedErrors value to a positive number (likely 1) to match the stated rejection behavior and ensure the validator in the routeapihelpers package actually rejects keys containing CA chain material, or update the test comment and add explicit documentation explaining that the API contract has intentionally changed to allow this case. Choose the appropriate fix based on the actual intended API contract.
🧹 Nitpick comments (3)
pkg/router/writerlease/writerlease_test.go (1)
1-148: 💤 Low valueConsider adding multi-worker concurrency tests.
All tests pass
workers=1, which maintains deterministic single-worker behavior. Since production usage passesMaxConcurrentSARChecks=50workers, consider adding a test that exercises concurrent worker behavior—for example, verifying that multiple queued items are processed in parallel and that shutdown cleanly waits for all workers.🤖 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` around lines 1 - 148, Add new test functions to verify concurrent worker behavior with multiple workers, since all existing tests use a single worker (third parameter set to 1 in the New constructor calls). Create tests that instantiate a writerlease with multiple workers (e.g., 5 or 10 for testing purposes) instead of 1, and verify that multiple queued work items are processed in parallel and that shutdown gracefully waits for all workers to complete their tasks. This will ensure the code behaves correctly under production conditions where MaxConcurrentSARChecks is 50 workers.pkg/router/controller/status_test.go (1)
1502-1538: ⚡ Quick winAssert message preservation too.
This case verifies the ignored
Reasonis preserved, but it leavesMessageempty on both sides. Add a non-empty message to catch regressions that keepReasonwhile clearing the user-facing detail.🧪 Proposed test tightening
Conditions: []routev1.RouteIngressCondition{{ Type: routev1.RouteAdmitted, Status: corev1.ConditionTrue, Reason: ExtCrtStatusReasonSARCompleted, + Message: "SAR check and secret load completed for secret \"tls-secret\"", }}}, }}, }, @@ Conditions: []routev1.RouteIngressCondition{{ Type: routev1.RouteAdmitted, Status: corev1.ConditionTrue, Reason: ExtCrtStatusReasonSARCompleted, + Message: "SAR check and secret load completed for secret \"tls-secret\"", }}}, }}, },🤖 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_test.go` around lines 1502 - 1538, In the test case "do not overwrite existing ignored reason with empty reason", add a non-empty Message field to both the existing condition (in the route.Status.Ingress[0].Conditions[0]) and the expected condition (in expectedRoute.Status.Ingress[0].Conditions[0]) with the same value. This ensures the test verifies that the Message field is also preserved alongside the Reason field, preventing regressions where the Reason is kept but the Message is inadvertently cleared.pkg/router/controller/route_secret_manager_test.go (1)
112-123: ⚡ Quick winAdd a regression case where the downstream plugin rejects the route.
The production fix depends on
RecordRouteUpdate(...SARCompleted...)not firing whenplugin.HandleRoutereturns an error, but this table always uses a successfulfakePlugin. Add a scenario withp.err != niland assertexpectedUpdates == nil.Also applies to: 1114-1118, 1129-1134
🤖 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 112 - 123, Add a new test scenario to the scenarios table in the route secret manager test that validates the regression case where the downstream plugin rejects the route. Create a scenario where the fakePlugin is configured with a non-nil error value (p.err != nil) to simulate HandleRoute returning an error, and set expectedUpdates to nil to assert that RecordRouteUpdate is not invoked when the plugin rejects the route. This ensures the production fix where RecordRouteUpdate does not fire on plugin errors is properly covered by regression testing.
🤖 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 `@go.mod`:
- Line 121: Add a comment in go.mod above the replace directive for
github.com/openshift/library-go (the line replacing it with
github.com/bentito/library-go) to clarify the intent and provenance of this
fork. The comment should explicitly document whether this fork is temporary
(pending upstream acceptance of changes) or permanent (intentional architectural
split), and explain the justification such as the HAProxy API improvements (DCM
add/del functionality) that necessitate this divergence from upstream. This
visibility helps future maintainers understand the fork's lifecycle and when it
can potentially be reconciled with upstream openshift/library-go.
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 327-339: In the DeleteFunc handler for the route secret manager,
the error logs are logging entire objects which could expose sensitive data from
Secret Data fields. Replace the logging of whole objects in both error cases:
for the first log.Error call, instead of logging the full obj parameter, log
only the dynamic type or safe metadata; for the second log.Error call, instead
of logging the full tombstone.Obj, log only safe metadata such as the type or
name, not the entire object that may contain sensitive Data fields.
In `@pkg/router/controller/shared_secret_manager_test.go`:
- Around line 42-55: The test creates a SharedSecretManager that uses informers
internally but never verifies cache synchronization or tests the actual
GetSecret behavior that depends on HasSynced(). Either modify the
SharedSecretManager to accept an injectable informer factory so tests can
provide a mock source instead of the real REST client, or extend the test to
call WaitForCacheSync after setting up the manager and then verify informer
functionality by calling GetSecret to ensure the cache lookups work correctly.
In `@pkg/router/controller/shared_secret_manager.go`:
- Around line 107-109: The variable infState is assigned from the map lookup
m.informers[infKey] on the same line as the exists check, but this value is
never used before being potentially overwritten. Remove the infState assignment
from the initial map lookup statement, and only assign infState after confirming
the key exists (after the if !exists block completes), or restructure the code
to avoid the ineffectual assignment by directly using the map lookup result only
when needed.
- Around line 131-141: The AddEventHandler call is ignoring the error return
value, which in k8s.io/client-go v0.35.0 now returns both a
ResourceEventHandlerRegistration and an error. Capture the error return value
from the AddEventHandler method call and check if it is non-nil. If the error
occurs, the route that was previously registered (referenced in the comment as
being registered at lines 101-105) must be rolled back to maintain consistency,
since the handler registration failure means the route cannot process events
properly.
---
Outside diff comments:
In `@pkg/router/routeapihelpers/validation_test.go`:
- Around line 2512-2523: The test case in the validation_test.go file has an
inconsistency where the test comment states that a key field containing Public
Key/certificate attributes "should be rejected", but expectedErrors is set to 0,
which means the test is accepting the malformed input rather than rejecting it.
Either update the expectedErrors value to a positive number (likely 1) to match
the stated rejection behavior and ensure the validator in the routeapihelpers
package actually rejects keys containing CA chain material, or update the test
comment and add explicit documentation explaining that the API contract has
intentionally changed to allow this case. Choose the appropriate fix based on
the actual intended API contract.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 110-127: The fakeHAProxy Start() method needs to be refactored to
return an error instead of void, and the goroutine it spawns should propagate
errors explicitly rather than panicking or silently exiting. Replace the panic
call on the net.Listen failure with sending the error through an error channel
that Start() can read from before returning. Update the listener.Accept() error
handling to differentiate between expected errors during shutdown (when
p.shutdown is true) and unexpected errors, propagating unexpected ones through
the error channel or logging mechanism. Additionally, modernize the net.Listen
call to use net.ListenConfig with context.Background() for consistency with
current Go best practices. Ensure the error channel is created, read from in
Start() to detect initialization failures, and properly closed to prevent
goroutine leaks.
---
Nitpick comments:
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 112-123: Add a new test scenario to the scenarios table in the
route secret manager test that validates the regression case where the
downstream plugin rejects the route. Create a scenario where the fakePlugin is
configured with a non-nil error value (p.err != nil) to simulate HandleRoute
returning an error, and set expectedUpdates to nil to assert that
RecordRouteUpdate is not invoked when the plugin rejects the route. This ensures
the production fix where RecordRouteUpdate does not fire on plugin errors is
properly covered by regression testing.
In `@pkg/router/controller/status_test.go`:
- Around line 1502-1538: In the test case "do not overwrite existing ignored
reason with empty reason", add a non-empty Message field to both the existing
condition (in the route.Status.Ingress[0].Conditions[0]) and the expected
condition (in expectedRoute.Status.Ingress[0].Conditions[0]) with the same
value. This ensures the test verifies that the Message field is also preserved
alongside the Reason field, preventing regressions where the Reason is kept but
the Message is inadvertently cleared.
In `@pkg/router/writerlease/writerlease_test.go`:
- Around line 1-148: Add new test functions to verify concurrent worker behavior
with multiple workers, since all existing tests use a single worker (third
parameter set to 1 in the New constructor calls). Create tests that instantiate
a writerlease with multiple workers (e.g., 5 or 10 for testing purposes) instead
of 1, and verify that multiple queued work items are processed in parallel and
that shutdown gracefully waits for all workers to complete their tasks. This
will ensure the code behaves correctly under production conditions where
MaxConcurrentSARChecks is 50 workers.
🪄 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: Enterprise
Run ID: eebcedd7-463a-43a4-832b-5884254e1cc0
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sumvendor/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/github.com/openshift/library-go/pkg/route/secretmanager/manager.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/pkg/secret/secret_monitor.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modhack/Makefile.debugpkg/cmd/infra/router/clientcmd.gopkg/cmd/infra/router/template.gopkg/router/controller/contention.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
|
/assign |
jcmoraisjr
left a comment
jcmoraisjr
left a comment
There was a problem hiding this comment.
Good job. A few suggestions and some nitpicking comments, just ignore the ones that does not apply.
| // Release lock during API probe to avoid blocking other registrations. | ||
| m.lock.Unlock() | ||
| // We use a metadata-only List check to see if we have namespace-wide permissions. | ||
| _, err := m.kubeClient.CoreV1().Secrets(namespace).List(ctx, metav1.ListOptions{Limit: 1}) | ||
| m.lock.Lock() |
There was a problem hiding this comment.
It seems this Unlock() is the only reason a single Unlock() is not as a defer in the start of the method. From the comments this is not to block other registrations. Is there a fair decrease of the time implementing this way? Unlocking here is giving a chance for other concurrent calls to grab the lock and create the same registeredRoute as this goroutine is already creating. Also, it seems that this List would only be called once per namespace since it's under the !known branch.
There was a problem hiding this comment.
The List check only happens once per namespace. However, in high-scale environments (similar to the load testing done by the Performance and Scale team), we might see hundreds of routes created simultaneously in a newly provisioned namespace. Because m.lock is a global lock across the entire SharedSecretManager, if we hold it during the kubeClient.CoreV1().Secrets().List() network call, we would block all other route registrations across all namespaces while waiting for the API server to respond.
By dropping the lock during the network call, we allow routes in other (already known) namespaces to continue registering without being blocked by this API latency. When we re-acquire the lock, we re-evaluate if restricted, known = m.restrictedNamespaces[namespace]; !known to handle the case where another concurrent goroutine completed the API call for this namespace while we were unlocked. Felt this complexity was worth it to prevent a single slow API call from stalling the entire secret management pipeline.
| // Only check routes in the same namespace | ||
| if !strings.HasPrefix(k, namespace+"/") { |
There was a problem hiding this comment.
nit: using types.NamespacedName as the key gives the advantage of not need to concat string when building the key, and a direct comparison here with the Namespace field of the struct.
There was a problem hiding this comment.
agreed, changing in upcoming commit
| func (a *StatusAdmitter) HandleRoute(eventType watch.EventType, route *routev1.Route) error { | ||
| a.lock.Lock() | ||
| defer a.lock.Unlock() |
There was a problem hiding this comment.
I can see the Lock() being removed here but not removed from other methods. On a quick look, this method in fact seems thread safe, but wondering if all the other methods should follow the same approach and the lock field being removed altogether. If it is in fact safe here, it should be safe on the others.
There was a problem hiding this comment.
yeah, we should remove the lock field from StatusAdmitter, remove its initialization in NewStatusAdmitter, and remove the a.lock.Lock() and defer a.lock.Unlock() calls from all RecordRoute* methods. Doing in upcoming commit
| // sarCache stores successful SAR validation results keyed by "namespace/secretName". | ||
| // Only successful validations are cached; failures always trigger fresh checks. | ||
| var sarCache sync.Map |
There was a problem hiding this comment.
nit: It sounds like this code is claiming for a struct to organize the fields related with the ValidateTLSExternalCertificate() func. This should also allow the removal of the ClearAsyncSARCacheForTest().
Just thinking out loud. The test related method aside, all the rest seems ok in the global area as this is a small package.
There was a problem hiding this comment.
I agree it would be slightly cleaner in a struct, but as you mentioned, given the small footprint here, we opted to keep it at the package level for simplicity and to minimize structural churn on this critical path
| // Acquire semaphore slot — blocks if all slots are in use. | ||
| // This throttles concurrent SAR API calls to MaxConcurrentSARChecks. | ||
| sarSemaphore <- struct{}{} | ||
| defer func() { <-sarSemaphore }() |
There was a problem hiding this comment.
Never had that idea before, nice trick! Wondering if you know and considered Acquire() from "golang.org/x/sync/semaphore". Just curious, no action needed.
There was a problem hiding this comment.
eh, the channel approach here is pretty standard, dependency-free pattern that covers our exact use case without needing the extra features from x/sync
| timeoutCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | ||
| defer cancel() |
There was a problem hiding this comment.
This is the timeout for the API calls, I thought about 10s or so, wondering if the 30s timeout consider some scenario or it is the result of your performance tests.
There was a problem hiding this comment.
Yes, the 30s timeout was specifically chosen based on our results during the P&S scale testing. Under high load and concurrency, we observed that API calls could occasionally queue up and take longer than 10s to complete. Setting it to 30s provided enough buffer to prevent spurious timeouts during these peak load events without compromising the overall responsiveness of the router.
|
[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 |
|
/assign @Miciah |
|
/retest |
1 similar comment
|
/retest |
|
/retest-required |
|
/retest |
|
@bentito: The following test failed, say
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. |
|
/retest |
8 participants
This PR implements asynchronous and cached
SubjectAccessReviewchecks forspec.tls.externalCertificateduring router startup.Previously, each external certificate triggered synchronous API validations that blocked the main thread. When scaling to thousands of external cert routes, the router hit
O(N * API_latency)delays and would hitCrashLoopBackOfffrom failing liveness probes.This fix maintains full RBAC security checks but executes them asynchronously in the background. A global
sync.Mapacts as anasyncSARCacheto drastically speed up repeated checks for the same underlying secret.Fixes: OCPBUGS-77056
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests