TRT-2841: Revert "OCPBUGS-77056: Make external cert validation asynchronous (Resurrection)"#824
Conversation
…surrection)" This reverts commit 13f62cb. Reverting router#822 which reintroduced RouteExternalCertificate test failures in the 5.0 nightly stream. Tracked in TRT-2841.
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
ready-for-human-review
|
@redhat-chai-bot: This pull request references TRT-2841 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. 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. |
WalkthroughThe PR updates external-certificate validation, replaces the shared secret manager, changes secret-event status handling, serializes route status admission, simplifies writer leases, and adjusts build defaults and test infrastructure. ChangesRouter validation and runtime behavior
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Route
participant authorizationutil
participant SecretsGetter
Route->>authorizationutil: Authorize secret access
Route->>SecretsGetter: Retrieve referenced TLS Secret
SecretsGetter-->>Route: Return Secret or validation error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[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 |
|
/payload-aggregate periodic-ci-openshift-hypershift-release-5.0-periodics-e2e-aws-ovn-conformance 10 |
|
@neisw: 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/99d2eb40-8864-11f1-822b-6a1747442f40-0 |
|
/payload-aggregate periodic-ci-openshift-release-main-ci-5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-rt-upgrade 10 |
|
@neisw: 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/b5f22b10-8864-11f1-855d-aaa086abd364-0 |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-metal-ipi-ovn-ipv4 periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn-techpreview |
|
@neisw: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/0a333ac0-8865-11f1-955f-076351bfea4c-0 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/router/controller/contention.go (1)
296-303: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
conditionsEqualfaithful for status updates.
isIngressConditionUpdateRequiredalso callsconditionsEqualto decide whether to write a condition. With the current ignore-set shortcut, changes likeAdmitted=True→Admitted=Falsecan be treated as unchanged when one condition reason is in the ignore set, skipping a required status write. Use this ignore behavior only in the contention/path where it is needed.🤖 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 296 - 303, Update conditionsEqual to always compare Type, Status, Reason, and Message, removing the ignoreIngressConditionReason shortcut so status changes such as Admitted=True to Admitted=False are detected. Preserve the ignore-set behavior in the specific contention/path that requires it, rather than applying it through the shared equality helper.pkg/router/template/configmanager/haproxy/testing/haproxy.go (1)
110-132: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle post-start listener errors.
go func() errordiscards the goroutine return: startup listener failures are already sent onlistenErr, but laterlistener.Accept()failures also return from the goroutine and are thrown away, silently stopping the accept loop. Send those errors onlistenError otherwise report them. Also use acontext.Contextfor the listener call instead ofnet.Listenfor any noctx lint requirement.🤖 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 - 132, Update the listener goroutine to report errors from both startup and post-start `listener.Accept()` failures through `listenErr` instead of discarding its return value. Replace `net.Listen` with the context-aware listener API, using the surrounding test context, while preserving normal shutdown behavior without reporting it as an error.Sources: Path instructions, Linters/SAST tools
🧹 Nitpick comments (2)
pkg/router/controller/route_secret_manager_test.go (1)
53-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest helper mutates process env and starts an informer that never stops.
os.Setenvhas no cleanup (uset.Setenvby passingt), andcontext.TODO().Done()returns nil, so everyinformer.Rungoroutine lives for the whole test binary.♻️ Suggested change
-func fakeSecretInformer(fakeKubeClient *testclient.Clientset, namespace, name string) cache.SharedInformer { +func fakeSecretInformer(t *testing.T, fakeKubeClient *testclient.Clientset, namespace, name string) cache.SharedInformer { // WatchListClient featuregate is enabled by default since v0.35. Fake client does not support // initializing its cache from Watch, so falling back to use List instead. The envvar below // configures the featuregate state. - os.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") + t.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False")and drive
Runwith a per-test cancellable context:ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) go informer.Run(ctx.Done())🤖 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 53 - 75, Update fakeSecretInformer to accept *testing.T, replace os.Setenv with t.Setenv so the feature gate is restored automatically, and use a per-test cancellable context for informer.Run. Register cancel with t.Cleanup and pass ctx.Done() to ensure the informer goroutine stops when the test completes.pkg/router/routeapihelpers/validation.go (1)
293-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment doesn't match the implementation.
The comment claims only the first block is checked, but the loop validates every decoded block. Also, "cannot be null" is misleading for a byte slice — "empty" is accurate.
♻️ Suggested wording fix
-// validatePEMContent takes content and pemType, PEM decodes content and -// validates that the first block matches the expected pemType. This validation -// 1. Ensures that the required pemType is first in the order +// validatePEMContent takes content and pemType, PEM decodes content and +// validates that every decoded block matches the expected pemType. This validation +// 1. Ensures that the required pemType is the only type present (and therefore first in order) // 2. Blocks attempts of passing certificates where keys should be used, and // 3. Blocks attempts of passing keys where certificates should be used. @@ if len(content) == 0 { - return fmt.Errorf("the PEM content cannot be null") + return fmt.Errorf("the PEM content cannot be empty") }🤖 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 293 - 317, Update the validatePEMContent doc comment to state that validation checks every decoded PEM block, not only the first block, and replace the “cannot be null” wording with “cannot be empty” to accurately describe the []byte input. Keep the implementation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/controller/factory/factory_endpointslices_test.go`:
- Around line 80-88: Update newEndpointSliceTestSetup to accept *testing.T and
replace the global os.Setenv call for the WatchListClient feature gate with
t.Setenv, preserving the existing disabled value while ensuring automatic
test-scoped cleanup.
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 115-133: The statusRecorder completion signalling and state access
are not concurrency-safe. In
pkg/router/controller/route_secret_manager_test.go:115-133, add mutex protection
for rejections and updates, restore guarded getters, and use sync.Once when
closing doneCh. In pkg/router/controller/route_secret_manager_test.go:1409-1417,
stop reassigning recorder.doneCh; instead use a buffered event channel sent by
statusRecorder so each phase can be awaited without mutating shared state.
In `@pkg/router/controller/route_secret_manager.go`:
- Around line 311-314: Update the DeleteFunc callback to safely handle informer
tombstones before accessing the deleted secret. Unwrap
cache.DeletedFinalStateUnknown and extract its underlying *kapi.Secret, while
preserving direct *kapi.Secret handling and avoiding a panic for unexpected
object types; then continue generating the existing deletion message.
In `@pkg/router/writerlease/writerlease.go`:
- Around line 253-255: Replace the blocking time.Sleep(remaining) in the
follower lease-expiration path with the delaying queue mechanism, reusing
l.queue to schedule key after the remaining duration. Preserve the existing log
and avoid directly blocking the worker so Run shutdown can interrupt it.
---
Outside diff comments:
In `@pkg/router/controller/contention.go`:
- Around line 296-303: Update conditionsEqual to always compare Type, Status,
Reason, and Message, removing the ignoreIngressConditionReason shortcut so
status changes such as Admitted=True to Admitted=False are detected. Preserve
the ignore-set behavior in the specific contention/path that requires it, rather
than applying it through the shared equality helper.
In `@pkg/router/template/configmanager/haproxy/testing/haproxy.go`:
- Around line 110-132: Update the listener goroutine to report errors from both
startup and post-start `listener.Accept()` failures through `listenErr` instead
of discarding its return value. Replace `net.Listen` with the context-aware
listener API, using the surrounding test context, while preserving normal
shutdown behavior without reporting it as an error.
---
Nitpick comments:
In `@pkg/router/controller/route_secret_manager_test.go`:
- Around line 53-75: Update fakeSecretInformer to accept *testing.T, replace
os.Setenv with t.Setenv so the feature gate is restored automatically, and use a
per-test cancellable context for informer.Run. Register cancel with t.Cleanup
and pass ctx.Done() to ensure the informer goroutine stops when the test
completes.
In `@pkg/router/routeapihelpers/validation.go`:
- Around line 293-317: Update the validatePEMContent doc comment to state that
validation checks every decoded PEM block, not only the first block, and replace
the “cannot be null” wording with “cannot be empty” to accurately describe the
[]byte input. Keep the implementation unchanged.
🪄 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: 3ea1c099-020a-4a3c-b967-4403f1e85ad8
⛔ 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
💤 Files with no reviewable changes (4)
- pkg/router/controller/shared_secret_manager_test.go
- pkg/cmd/infra/router/clientcmd.go
- pkg/router/controller/shared_secret_manager.go
- pkg/router/controller/status_test.go
| func newEndpointSliceTestSetup(plugin router.Plugin, initialObjects ...runtime.Object) (*fakekubeclient.Clientset, chan struct{}) { | ||
| stopCh := make(chan struct{}) | ||
| client := fakekubeclient.NewSimpleClientset(initialObjects...) | ||
| fakeProject := &fakeproject.FakeProjectV1{} | ||
|
|
||
| // WatchListClient featuregate is enabled by default since v0.35. Fake client does not support | ||
| // initializing its cache from Watch, so falling back to use List instead. The envvar below | ||
| // configures the featuregate state. | ||
| t.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") | ||
| os.Setenv("KUBE_FEATURE_"+string(features.WatchListClient), "False") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching factory_endpointslices_test.go:\n'
fd -a 'factory_endpointslices_test.go' . || true
file="$(fd 'factory_endpointslices_test.go' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
printf '\nFile: %s\n' "$file"
wc -l "$file"
printf '\nLines 1-140:\n'
sed -n '1,140p' "$file" | nl -ba
fi
printf '\nSearch for newEndpointSliceTestSetup and related env usage:\n'
rg -n "newEndpointSliceTestSetup|WatchListClient|KUBE_FEATURE_WatchListClient|Setenv" . --glob '*.go'Repository: openshift/router
Length of output: 473
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./pkg/router/controller/factory/factory_endpointslices_test.go"
printf 'File: %s\n' "$file"
wc -l "$file"
printf '\nLines 1-140:\n'
sed -n '1,140p' "$file" | awk '{printf "%6d %s\n", NR, $0}'
printf '\nSearch for watcher helper and env usage:\n'
rg -n "newEndpointSliceTestSetup|WatchListClient|KUBE_FEATURE_WatchListClient|Setenv|CheckError" "$file" ./pkg/router/controller/factory --glob '*.go' || true
printf '\nDiff/stat context if available:\n'
git diff --stat HEAD^..HEAD 2>/dev/null || true
git diff -- pkg/router/controller/factory/factory_endpointslices_test.go 2>/dev/null | sed -n '1,220p' || trueRepository: openshift/router
Length of output: 8208
🌐 Web query:
Go testing.T.Setenv environment variable set automatically restored after test error no ignore os.Setenv documentation
💡 Result:
Yes, the environment variable changes made using testing.T.Setenv (as well as B.Setenv and F.Setenv) are automatically restored after the test finishes, regardless of whether the test passes or errors out [1][2][3]. When you call t.Setenv(key, value), the testing package internally performs the following steps: 1. It records the current state of the environment variable (if it exists) [2]. 2. It calls os.Setenv to update the variable [1][2]. 3. It registers a cleanup function using the t.Cleanup method to restore the variable to its original state (or unset it if it did not exist) once the test completes [1][2]. Because t.Cleanup is used, the restoration logic is guaranteed to run after the test function returns, even if the test fails or calls T.Fatal/T.Fatalf [1][2]. Important considerations regarding t.Setenv: - Process-Global Impact: Since environment variables are process-wide, t.Setenv cannot be used in parallel tests [1][4]. If you attempt to call t.Setenv in a test that has called T.Parallel (or if any of its ancestors are parallel), the testing framework will panic [5][2][4]. This safeguard prevents cross-test interference and potential race conditions associated with changing environment variables in a concurrent environment [6][4]. - No Manual Cleanup Required: You do not need to manually call os.Unsetenv or attempt to restore the variable yourself when using t.Setenv, as the testing framework handles this automatically and reliably [6][4][3].
Citations:
- 1: https://pkg.go.dev/testing
- 2: https://go.googlesource.com/go/+/refs/changes/69/356669/2/src/testing/testing.go
- 3: https://deepsource.com/directory/go/issues/GO-W1032
- 4: https://env.dev/guides/go-env-variables
- 5: testing: T.Setenv can be called after T.Parallel golang/go#55128
- 6: testing: add TB.Setenv() golang/go#41260
Keep feature-gate configuration test-scoped.
newEndpointSliceTestSetup sets WatchListClient=False globally with os.Setenv, so later endpoint-slice tests can inherit this state, and the error return is ignored. Change the helper to accept *testing.T and use t.Setenv(...).
🤖 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/factory/factory_endpointslices_test.go` around lines 80
- 88, Update newEndpointSliceTestSetup to accept *testing.T and replace the
global os.Setenv call for the WatchListClient feature gate with t.Setenv,
preserving the existing disabled value while ensuring automatic test-scoped
cleanup.
Source: Path instructions
| type statusRecorder struct { | ||
| sync.Mutex | ||
| rejections []string | ||
| updates []string | ||
| unservableInFutureVersions map[string]string | ||
| doneCh chan struct{} | ||
| } | ||
|
|
||
| func (r *statusRecorder) routeKey(route *routev1.Route) string { | ||
| return route.Namespace + "-" + route.Name | ||
| } | ||
| func (r *statusRecorder) RecordRouteRejection(route *routev1.Route, reason, message string) { | ||
| r.Lock() | ||
| defer r.Unlock() | ||
| defer close(r.doneCh) | ||
| r.rejections = append(r.rejections, fmt.Sprintf("%s:%s", r.routeKey(route), reason)) | ||
| } | ||
|
|
||
| func (r *statusRecorder) RecordRouteUpdate(route *routev1.Route, reason, message string) { | ||
| r.Lock() | ||
| defer r.Unlock() | ||
| defer close(r.doneCh) | ||
| r.updates = append(r.updates, fmt.Sprintf("%s:%s", r.routeKey(route), reason)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
statusRecorder completion signalling and state are not concurrency-safe. Both sites stem from one root cause: the informer goroutine mutates recorder state and closes doneCh while the test goroutine reads that state and swaps the channel.
pkg/router/controller/route_secret_manager_test.go#L115-L133: guardrejections/updateswith a mutex (restore guarded getters) and closedoneChviasync.Once.pkg/router/controller/route_secret_manager_test.go#L1409-L1417: stop reassigningrecorder.doneCh; use a buffered event channel the recorder sends on so each phase can be awaited without mutating shared state.
📍 Affects 1 file
pkg/router/controller/route_secret_manager_test.go#L115-L133(this comment)pkg/router/controller/route_secret_manager_test.go#L1409-L1417
🤖 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 115 - 133,
The statusRecorder completion signalling and state access are not
concurrency-safe. In pkg/router/controller/route_secret_manager_test.go:115-133,
add mutex protection for rejections and updates, restore guarded getters, and
use sync.Once when closing doneCh. In
pkg/router/controller/route_secret_manager_test.go:1409-1417, stop reassigning
recorder.doneCh; instead use a buffered event channel sent by statusRecorder so
each phase can be awaited without mutating shared state.
| DeleteFunc: func(obj interface{}) { | ||
| secret, ok := obj.(*kapi.Secret) | ||
| if !ok { | ||
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) | ||
| if !ok { | ||
| log.Error(nil, "Couldn't get object from tombstone", "type", fmt.Sprintf("%T", obj)) | ||
| return | ||
| } | ||
| secret, ok = tombstone.Obj.(*kapi.Secret) | ||
| if !ok { | ||
| log.Error(nil, "Tombstone contained object that is not a secret", "type", fmt.Sprintf("%T", tombstone.Obj)) | ||
| return | ||
| } | ||
| } | ||
| secret := obj.(*kapi.Secret) | ||
| key := generateKey(namespace, routeName) | ||
| msg := fmt.Sprintf("external certificate validation failed: secret %q deleted for route %q", secret.Name, key) | ||
| msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Unchecked type assertion in DeleteFunc can panic on a tombstone.
Shared informers deliver cache.DeletedFinalStateUnknown when a delete is missed (watch gap / resync), so obj.(*kapi.Secret) will panic and take down the router process. The tombstone unwrapping that previously guarded this was dropped here.
🛡️ Proposed fix
DeleteFunc: func(obj interface{}) {
- secret := obj.(*kapi.Secret)
+ secret, ok := obj.(*kapi.Secret)
+ if !ok {
+ tombstone, ok := obj.(cache.DeletedFinalStateUnknown)
+ if !ok {
+ log.V(4).Info("unexpected object type in DeleteFunc", "namespace", namespace, "route", routeName)
+ return
+ }
+ secret, ok = tombstone.Obj.(*kapi.Secret)
+ if !ok {
+ log.V(4).Info("unexpected tombstone object type in DeleteFunc", "namespace", namespace, "route", routeName)
+ return
+ }
+ }
key := generateKey(namespace, routeName)
msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DeleteFunc: func(obj interface{}) { | |
| secret, ok := obj.(*kapi.Secret) | |
| if !ok { | |
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) | |
| if !ok { | |
| log.Error(nil, "Couldn't get object from tombstone", "type", fmt.Sprintf("%T", obj)) | |
| return | |
| } | |
| secret, ok = tombstone.Obj.(*kapi.Secret) | |
| if !ok { | |
| log.Error(nil, "Tombstone contained object that is not a secret", "type", fmt.Sprintf("%T", tombstone.Obj)) | |
| return | |
| } | |
| } | |
| secret := obj.(*kapi.Secret) | |
| key := generateKey(namespace, routeName) | |
| msg := fmt.Sprintf("external certificate validation failed: secret %q deleted for route %q", secret.Name, key) | |
| msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key) | |
| DeleteFunc: func(obj interface{}) { | |
| secret, ok := obj.(*kapi.Secret) | |
| if !ok { | |
| tombstone, ok := obj.(cache.DeletedFinalStateUnknown) | |
| if !ok { | |
| log.V(4).Info("unexpected object type in DeleteFunc", "namespace", namespace, "route", routeName) | |
| return | |
| } | |
| secret, ok = tombstone.Obj.(*kapi.Secret) | |
| if !ok { | |
| log.V(4).Info("unexpected tombstone object type in DeleteFunc", "namespace", namespace, "route", routeName) | |
| return | |
| } | |
| } | |
| key := generateKey(namespace, routeName) | |
| msg := fmt.Sprintf("secret %q deleted for route %q", secret.Name, key) |
🤖 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 311 - 314, Update
the DeleteFunc callback to safely handle informer tombstones before accessing
the deleted secret. Unwrap cache.DeletedFinalStateUnknown and extract its
underlying *kapi.Secret, while preserving direct *kapi.Secret handling and
avoiding a panic for unexpected object types; then continue generating the
existing deletion message.
| log.V(4).Info("follower awaiting lease expiration", "worker", l.name, "key", key, "leaseTimeRemaining", remaining) | ||
| time.Sleep(remaining) | ||
| l.queue.Add(key) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)writerlease\.go$|pkg/router/writerlease'
echo "== file outline =="
ast-grep outline pkg/router/writerlease/writerlease.go --view expanded || true
echo "== relevant lines 220-275 =="
sed -n '220,275p' pkg/router/writerlease/writerlease.go | nl -ba -v220
echo "== search worker/run fields and queue usage =="
rg -n "type .*Queue|struct Worker|func .*Worker|l\.queue\.AddAfter|time\.Sleep|Run\(\)|stop|quit|context|Done\(key\)" pkg/router/writerlease/writerlease.goRepository: openshift/router
Length of output: 2411
🌐 Web query:
kubernetes client-go workqueue Type.DelayingInterface AddAfter Sleep context stopable example
💡 Result:
In Kubernetes client-go, the workqueue's DelayingInterface (and its Typed equivalent) provides an AddAfter method to enqueue items after a specified delay [1][2][3]. Crucially, the workqueue implementation is designed to handle stop signals gracefully via its ShutDown method rather than through a context.Context passed to individual AddAfter calls [1][2][4]. When you call AddAfter(item, duration), the implementation internally uses a select block with a stop channel (stopCh) [1][2]: select { case <-q.stopCh: // Stops the addition if the queue has been shut down case q.waitingForAddCh <- &waitFor[T]{data: item, readyAt: q.clock.Now.Add(duration)}: // Adds the item to the delayed processing queue } Key points regarding your requirements: 1. Stopability: The queue becomes "stopable" by calling its ShutDown method [4]. This closes the stopCh, which allows pending AddAfter operations to unblock and prevents new items from being added [1][2][4]. There is no native support for passing a context.Context to AddAfter to interrupt it externally. 2. Avoiding Blocking: AddAfter is designed to be non-blocking under normal operation; it sends the item to a channel (waitingForAddCh) consumed by a dedicated "waitingLoop" goroutine [1][2]. It only blocks if the channel is full or if the system is shutting down, at which point the stopCh case takes precedence [1][2]. 3. Best Practice for Workers: To stop worker goroutines that are blocked on the standard queue.Get call, the standard pattern is to call ShutDown on the queue [4][5]. This causes Get to return a shutdown flag (true), signaling your worker loop to terminate gracefully [4][5]. If you need to implement a complex shutdown involving external contexts, you must manage this outside the workqueue by checking your context before calling AddAfter or by triggering the queue's ShutDown when your context is cancelled. Note that attempts to add custom context-awareness to the queue itself (such as making Get interruptible) have been discussed in the community, but are not currently supported features [6].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/master/util/workqueue/delaying_queue.go
- 2: https://github.com/kubernetes/client-go/blob/v0.35.3/util/workqueue/delaying_queue.go
- 3: https://godocs.io/k8s.io/client-go/util/workqueue
- 4: https://github.com/kubernetes/client-go/blob/master/util/workqueue/queue.go
- 5: https://oneuptime.com/blog/post/2026-02-09-client-go-work-queues-rate-limited/view
- 6: Add an interruptible work queue kubernetes/kubernetes#131185
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== writerlease lines 136-265 =="
sed -n '136,265p' pkg/router/writerlease/writerlease.go
echo "== test shutdown references =="
sed -n '1,260p' pkg/router/writerlease/writerlease_test.goRepository: openshift/router
Length of output: 6213
Do not block the sole worker while deferring follower work.
time.Sleep(remaining) cannot be interrupted by Run’s shutdown path, so the worker remains stuck until the remaining lease time elapses. Requeue with the delaying queue so shutdown can stop the worker.
Proposed fix
- time.Sleep(remaining)
- l.queue.Add(key)
+ l.queue.AddAfter(key, remaining)
l.queue.Done(key)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.V(4).Info("follower awaiting lease expiration", "worker", l.name, "key", key, "leaseTimeRemaining", remaining) | |
| time.Sleep(remaining) | |
| l.queue.Add(key) | |
| log.V(4).Info("follower awaiting lease expiration", "worker", l.name, "key", key, "leaseTimeRemaining", remaining) | |
| l.queue.AddAfter(key, remaining) | |
| l.queue.Done(key) |
🤖 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 253 - 255, Replace the
blocking time.Sleep(remaining) in the follower lease-expiration path with the
delaying queue mechanism, reusing l.queue to schedule key after the remaining
duration. Preserve the existing log and avoid directly blocking the worker so
Run shutdown can interrupt it.
|
/retest-required |
|
/lgtm |
openshift-ci
Bot
added
approved
openshift-ci-robot
added
the
verified
|
@neisw: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
3 participants
Summary
This reverts merge commit 13f62cb (PR #822),
which resurrected the asynchronous external certificate validation feature
originally introduced in PR #745.
Why
PR #745 was reverted via PR #817 after it caused
RouteExternalCertificatehypershift conformance test failures in the 5.0 nightly stream, tracked as
TRT-2813.
PR #822 re-introduced the same feature with bug fixes. However, after it
merged (2026-07-24 18:58 UTC), the 5.0 nightly stream began experiencing
x509: ECDSA verification failureerrors inRouteExternalCertificatetests— the router intermittently serves a broken certificate chain after secret
updates, causing TLS handshake failures. The historical pass rate for
aggregated-hypershift-ovn-conformance-5.0degraded from 89–91% to 50–70%.Investigation of the payload immediately before #822 merged
(
5.0.0-0.nightly-2026-07-24-144712) confirmed that its hypershiftconformance failures were caused by infrastructure issues (pod scheduling
failures, insufficient aggregated runs), not by
RouteExternalCertificatetest assertion failures. The actual x509 cert chain failures began in
subsequent payloads, aligning with #822's merge time.
As of
5.0.0-0.nightly-2026-07-25-093347, 5.0 has seen 5 consecutivepayload rejections with no accepted nightly in ~147.5 hours.
Tracking
@neisw requested in Slack thread
Summary by CodeRabbit
Bug Fixes
Improvements