CNTRLPLANE-2589: Migrate e2e-* tests to Ginkgo v2#859
Open
ropatil010 wants to merge 4 commits into
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic to target the "4.22.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. |
|
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:
WalkthroughAdds new Ginkgo e2e suites and extracted reusable encryption/rotation/perf/KMS and OIDC test libraries; replaces many in-file tests with wrapper delegates; adds Keycloak admin helpers; introduces DB-population and migration-watcher helpers; lazy-inits KAS HTTPS port; removes custom TestMain randomization; bumps klog. ChangesE2E Tests & Library
Infrastructure & Tooling
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant Keycloak as Keycloak IdP
participant CAO as Auth Operator
participant KAS as Kube API Server
participant K8s as Kubernetes API
Test->>Keycloak: provision realm, user, claims
Test->>CAO: apply AuthenticationSpec (OIDC config, CA, claim mappings)
CAO->>K8s: create/update auth-config ConfigMaps and operator resources
CAO->>KAS: trigger KAS rollout / ensure generated auth-config.json present
KAS->>Keycloak: fetch JWKS (OIDC discovery) for token validation
Test->>K8s: call SelfSubjectReview with ID token
K8s-->>Test: return authentication result (Allowed / Unauthorized)
Note over CAO,KAS: operator reconciles and overwrites manual auth-config changes when reconciling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
debcb9a to
9541074
Compare
Contributor
Author
|
Before fix in CI logs we were hitting the same error which was mentioned in the Reverting PR: #857 After fix the CI logs got pass /assign @liouk @stbenjam |
ropatil010
changed the title
liouk
reviewed
Apr 2, 2026
liouk
suggested changes
Apr 2, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ropatil010 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 |
Member
|
@ropatil010 please correct me if I'm wrong, but this issue seems that it's caused by OTE itself, is there a plan on fixing that on the OTE side? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
test/e2e-oidc/external_oidc.go (1)
441-470: Consider extending algorithm support for future-proofing.The
extractRSAPubKeyFunconly handles RS256. While this matches current Keycloak defaults, if the IDP configuration changes to use RS384/RS512, tests would fail with an unhelpful error. This is acceptable for now but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc.go` around lines 441 - 470, extractRSAPubKeyFunc currently only accepts "RS256" and returns an error for other RSA algorithms; update it to accept other RSA signing algorithms (e.g., "RS384", "RS512") or any algorithm that starts with "RS" so the JWKS parsing is future-proof: modify the switch on key.Alg in extractRSAPubKeyFunc (and the related issuerJWKS/Key usage) to handle "RS384" and "RS512" (or use a strings.HasPrefix(key.Alg, "RS") check) and keep constructing the rsa.PublicKey the same way, while keeping the unexpected-algorithm error for non-RS algorithms. Ensure the error messages still include key.KID and key.Alg for clarity.test/e2e-encryption-perf/encryption_perf.go (1)
55-66: Simplify error handling with directt.Fatalcalls.The pattern of creating an error and immediately passing it to
require.NoErroris confusing. Since the error is always non-nil when created,require.NoErrorwill always fail, making this equivalent tot.Fatalbut harder to read.♻️ Proposed simplification
AssertDBPopulatedFunc: func(t testing.TB, errorStore map[string]int, statStore map[string]int) { tokenCount, ok := statStore[tokenStatsKey] if !ok { - err := errors.New("missing oauth access tokens count stats, can't continue the test") - require.NoError(t, err) + t.Fatal("missing oauth access tokens count stats, can't continue the test") } if tokenCount < 14000 { - err := fmt.Errorf("expected to create at least 14000 tokens but %d were created", tokenCount) - require.NoError(t, err) + t.Fatalf("expected to create at least 14000 tokens but %d were created", tokenCount) } t.Logf("Created %d access tokens", tokenCount) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-perf/encryption_perf.go` around lines 55 - 66, In AssertDBPopulatedFunc simplify error handling by replacing the pattern that constructs an error and calls require.NoError with direct test failures: when statStore[tokenStatsKey] is missing call t.Fatalf (or require.FailNow) with a clear message about the missing oauth access tokens count, and when tokenCount < 14000 call t.Fatalf (or require.FailNow) with a message showing the expected vs actual tokenCount; update references around AssertDBPopulatedFunc and tokenStatsKey accordingly.test/e2e-encryption-rotation/encryption_rotation.go (1)
47-75: Consider adding retry logic for the operator update.The
UnsupportedConfigFuncperforms a read-modify-write operation without handling potential conflict errors. If the operator object is modified between theGet(line 49) andUpdate(line 73), the update will fail with a conflict. For a 3-hour test, this could cause intermittent failures.If conflicts are expected to be rare in this test context, the current implementation is acceptable, but retry-on-conflict would improve reliability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-rotation/encryption_rotation.go` around lines 47 - 75, Wrap the read-modify-write in UnsupportedConfigFunc with retry-on-conflict logic (e.g., use k8s.io/client-go/util/retry.RetryOnConflict) so that if cs.OperatorClient.Update returns a conflict you re-get the Operator (cs.OperatorClient.Get), reapply the unsupportedConfig merge steps and retry the Update; ensure the loop returns the final error if retries are exhausted and keep references to UnsupportedConfigFunc, cs.OperatorClient.Get, and cs.OperatorClient.Update to locate the change.test/e2e-encryption/encryption.go (1)
54-74: Consider reusing the providedClientSetparameter.The
CreateResourceFunccallback receives alibrary.ClientSetparameter but ignores it, creating a new client viaoperatorencryption.GetClients(t). If this is intentional (e.g., needing operator-specific client configuration), adding a brief comment would clarify the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption/encryption.go` around lines 54 - 74, The CreateResourceFunc inside testEncryptionTurnOnAndOff ignores the provided library.ClientSet parameter and calls operatorencryption.GetClients(t); either update CreateResourceFunc to use the passed-in client (the second parameter) when calling operatorencryption.CreateAndStoreTokenOfLife (or adapt/convert the library.ClientSet to the expected client type), or if using GetClients(t) is intentional, add a brief comment above CreateResourceFunc explaining why operatorencryption.GetClients(t) is required instead of the supplied client; reference CreateResourceFunc, operatorencryption.CreateAndStoreTokenOfLife, and operatorencryption.GetClients for locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 136-137: The go.mod replace pointing to the forked module
(github.com/ropatil010/openshift-tests-extension
v0.0.0-20260406095036-3a490698dab7) introduces supply-chain and reproducibility
risk; update the repository by adding a clear TODO comment in go.mod and create
a tracking issue linking upstream PR `#60` ("feat: configure klog to write to
stderr") so we revert this replace as soon as that PR is merged, then remove the
replace and restore github.com/openshift-eng/openshift-tests-extension; also add
a CI/release gate (or note in the release checklist) to block merging release
candidates until the follow-up removal is completed.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Around line 27-28: Fix the typo in the comment above testEncryptionRotation:
change "encyrption.Reason" to "encryption.Reason" in the comment that describes
forcing a key rotation so the term is spelled correctly and matches the
configuration field name.
In `@test/library/encryption/perf_helpers.go`:
- Around line 35-46: The predicate passed to wait.PollUntilContextTimeout
currently sends any EncryptionMigrationControllerProgressing=True timestamp even
if it predates the test and it never checks the cond.Reason; update the
predicate used by wait.PollUntilContextTimeout in perf_helpers.go (the anonymous
func that calls getOperatorConditionsFn and writes to migrationStartedCh) to
also require cond.Reason == "Migrating" and that cond.LastTransitionTime is
strictly after a test start time captured before the poll (e.g., record start :=
time.Now() or start := time.Now().UTC() before calling
wait.PollUntilContextTimeout, then only send cond.LastTransitionTime.Time if
cond.Type == "EncryptionMigrationControllerProgressing" && cond.Status ==
operatorv1.ConditionTrue && cond.Reason == "Migrating" &&
cond.LastTransitionTime.Time.After(start)). Ensure the log message remains
meaningful when the newer transition is observed.
In `@test/library/encryption/scenarios.go`:
- Around line 189-210: The test uses a non-blocking select to read
migrationStartedCh which can fall through before the watcher goroutine sends;
change this to wait for the signal with a bounded timeout instead of using the
default case: replace the select block reading migrationStartedCh with a
blocking receive combined with a timeout (e.g., select { case migrationStarted
:= <-migrationStartedCh: ... case <-time.After(timeout): tb.Error(...) }) or use
a context with deadline that you cancel when runTestEncryptionPerf returns;
ensure you reference the watcher by keeping migrationStartedCh passed to
watchForMigrationControllerProgressingConditionAsync and use that same channel
for the bounded wait so the watcher is not left running indefinitely.
- Around line 72-83: The switch in TestEncryptionType currently routes the
empty-string provider ("") to TestEncryptionTypeIdentity which causes
APIServer.Spec.Encryption.Type to be set to "identity"; change the dispatch so
that case configv1.EncryptionTypeIdentity handles the explicit identity value
and the empty-string case ("") dispatches to TestEncryptionTypeUnset instead.
Update the switch in the TestEncryptionType function (the cases for
configv1.EncryptionTypeIdentity and "") so that "" calls
TestEncryptionTypeUnset(tb, scenario) while leaving the explicit Identity case
calling TestEncryptionTypeIdentity(tb, scenario).
- Around line 340-349: The APIServer Get+Update can fail with resourceVersion
conflicts; wrap the read-modify-update of apiServer (performed via
libClientSet.ApiServerConfig.Get and Update) inside a retry.RetryOnConflict
loop: import "k8s.io/client-go/util/retry", use retry.RetryOnConflict to
repeatedly Get the APIServer, set apiServer.Spec.Encryption.Type =
encryptionType when changed, and call Update until it succeeds or the retry
gives up; ensure errors from Update are returned/checked with
require.NoError(tb, err) as before.
---
Nitpick comments:
In `@test/e2e-encryption-perf/encryption_perf.go`:
- Around line 55-66: In AssertDBPopulatedFunc simplify error handling by
replacing the pattern that constructs an error and calls require.NoError with
direct test failures: when statStore[tokenStatsKey] is missing call t.Fatalf (or
require.FailNow) with a clear message about the missing oauth access tokens
count, and when tokenCount < 14000 call t.Fatalf (or require.FailNow) with a
message showing the expected vs actual tokenCount; update references around
AssertDBPopulatedFunc and tokenStatsKey accordingly.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Around line 47-75: Wrap the read-modify-write in UnsupportedConfigFunc with
retry-on-conflict logic (e.g., use k8s.io/client-go/util/retry.RetryOnConflict)
so that if cs.OperatorClient.Update returns a conflict you re-get the Operator
(cs.OperatorClient.Get), reapply the unsupportedConfig merge steps and retry the
Update; ensure the loop returns the final error if retries are exhausted and
keep references to UnsupportedConfigFunc, cs.OperatorClient.Get, and
cs.OperatorClient.Update to locate the change.
In `@test/e2e-encryption/encryption.go`:
- Around line 54-74: The CreateResourceFunc inside testEncryptionTurnOnAndOff
ignores the provided library.ClientSet parameter and calls
operatorencryption.GetClients(t); either update CreateResourceFunc to use the
passed-in client (the second parameter) when calling
operatorencryption.CreateAndStoreTokenOfLife (or adapt/convert the
library.ClientSet to the expected client type), or if using GetClients(t) is
intentional, add a brief comment above CreateResourceFunc explaining why
operatorencryption.GetClients(t) is required instead of the supplied client;
reference CreateResourceFunc, operatorencryption.CreateAndStoreTokenOfLife, and
operatorencryption.GetClients for locating the change.
In `@test/e2e-oidc/external_oidc.go`:
- Around line 441-470: extractRSAPubKeyFunc currently only accepts "RS256" and
returns an error for other RSA algorithms; update it to accept other RSA signing
algorithms (e.g., "RS384", "RS512") or any algorithm that starts with "RS" so
the JWKS parsing is future-proof: modify the switch on key.Alg in
extractRSAPubKeyFunc (and the related issuerJWKS/Key usage) to handle "RS384"
and "RS512" (or use a strings.HasPrefix(key.Alg, "RS") check) and keep
constructing the rsa.PublicKey the same way, while keeping the
unexpected-algorithm error for non-RS algorithms. Ensure the error messages
still include key.KID and key.Alg for clarity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a512169-a204-4df2-92e6-760ec43345c5
⛔ Files ignored due to path filters (17)
go.sumis excluded by!**/*.sumvendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/cmd/cmdrun/runsuite.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/result_writer.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/extension/extensiontests/viewer.htmlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift-eng/openshift-tests-extension/pkg/ginkgo/logging.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/README.mdis excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/textlogger/options.gois excluded by!vendor/**,!**/vendor/**vendor/k8s.io/klog/v2/textlogger/textlogger.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (22)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_test.gotest/e2e-encryption-kms/main_test.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption-rotation/encryption_rotation.gotest/e2e-encryption/encryption.gotest/e2e-encryption/encryption_test.gotest/e2e-encryption/main_test.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/client.gotest/library/encryption/helpers.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.gotest/library/featuregates.gotest/library/waits.go
💤 Files with no reviewable changes (2)
- test/e2e-encryption/main_test.go
- test/e2e-encryption-kms/main_test.go
ropatil010
changed the title
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
4342519 to
8f115d7
Compare
ropatil010
added a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Apr 7, 2026
This commit migrates five test suites (e2e-encryption, e2e-encryption-kms, e2e-encryption-perf, e2e-encryption-rotation, e2e-oidc) from standalone test binaries to the OpenShift Tests Extension (OTE) framework using Ginkgo v2. Key changes: - Migrate tests to Ginkgo v2 with OTE integration - Extract test logic into dedicated helper files for better organization - Remove obsolete main_test.go files in favor of Ginkgo suite setup - Configure appropriate test suite parallelism and qualifiers - Add klog stderr redirection to fix JSON output corruption Critical fix for klog output corruption: - Add klog.SetOutput(os.Stderr) in test binary's main() to prevent klog warnings from corrupting JSON output during 'openshift-tests list' commands - This fixes "invalid character 'W'" parsing errors in CI that caused PR openshift#839 to be reverted in PR openshift#857 - Ensures stdout contains only clean JSON for CI consumption while klog warnings go to stderr Test organization: - operator/parallel: Non-serial operator, templates, and tokens tests (parallelism: 4) - operator/serial: Serial operator, OIDC, templates, and tokens tests - operator/disruptive: Tests marked as [Disruptive] - operator-encryption/serial: Basic encryption tests - operator-encryption-rotation/serial: Encryption key rotation tests - operator-encryption-perf/serial: Encryption performance tests - operator-encryption-kms/serial: KMS encryption tests Dependencies: - Use standard OTE version v0.0.0-20251205182537-ff5553e56f33 - Updated klog dependency to v2.140.0 - No fork dependencies See: openshift#857 (revert) openshift#859 (this PR) Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
8f115d7 to
6c90e52
Compare
ropatil010
added a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
Apr 8, 2026
Restore the init package approach to fix 'invalid character W looking for beginning of value' errors in CI when listing tests. The previous fix using klog.SetOutput(os.Stderr) in main() was too late - klog warnings were already generated during Go's init phase, corrupting the JSON output before main() could run. This commit implements a two-phase approach: 1. Init phase (pkg/test/init): Suppress all klog output using io.Discard - Runs during Go initialization, before any dependency can trigger warnings - Prevents warnings from appearing in stdout during 'openshift-tests list' 2. Main phase: Reconfigure klog to stderr for normal test execution - Enables proper logging after initialization completes - Keeps stdout clean for JSON output This is the same approach that made commit 9541074 pass in CI, but was inadvertently removed in commit 6c90e52, causing test failures. Fixes the root cause of CI failures in PR openshift#859 See: openshift#857 Co-Authored-By: Rohit Patil <ropatil@redhat.com>
liouk
suggested changes
Apr 9, 2026
Contributor
Author
|
Waiting for PR: openshift/origin#31009 to gets merged which fixes the issue "error: encountered errors while listing tests: invalid character 'W' looking for beginning of value". The above mentioned issue has been fixed in this PR using init things but looks like it is not acceptable. |
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
753ca4a to
aa529ed
Compare
Contributor
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
test/library/encryption/scenarios.go (3)
340-349:⚠️ Potential issue | 🟠 MajorRetry the APIServer update on conflict.
This is still a single read-modify-write against
apiserver/cluster. If anything updates the object betweenGetandUpdate, the test fails on a transientresourceVersionconflict even though retrying would succeed. Wrap the mutation inretry.RetryOnConflicthere.Suggested fix
+ "k8s.io/client-go/util/retry"- apiServer, err := libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{}) - require.NoError(tb, err) - - // Update encryption type if needed - needsUpdate := apiServer.Spec.Encryption.Type != encryptionType - if needsUpdate { - tb.Logf("Updating encryption type in the config file for APIServer to %q", encryptionType) - apiServer.Spec.Encryption.Type = encryptionType - _, err = libClientSet.ApiServerConfig.Update(context.TODO(), apiServer, metav1.UpdateOptions{}) - require.NoError(tb, err) + apiServer, err := libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{}) + require.NoError(tb, err) + + needsUpdate := apiServer.Spec.Encryption.Type != encryptionType + if needsUpdate { + tb.Logf("Updating encryption type in the config file for APIServer to %q", encryptionType) + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + apiServer, err = libClientSet.ApiServerConfig.Get(context.TODO(), "cluster", metav1.GetOptions{}) + if err != nil { + return err + } + apiServer.Spec.Encryption.Type = encryptionType + _, err = libClientSet.ApiServerConfig.Update(context.TODO(), apiServer, metav1.UpdateOptions{}) + return err + }) + require.NoError(tb, err) } else { tb.Logf("APIServer is already configured to use %q mode", encryptionType) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/scenarios.go` around lines 340 - 349, The update to the APIServer object does a single read-modify-write and can fail on resourceVersion conflicts; wrap the mutation+Update call in a retry.RetryOnConflict loop so it re-reads and retries on conflict. Locate the block that calls libClientSet.ApiServerConfig.Get and then sets apiServer.Spec.Encryption.Type and calling libClientSet.ApiServerConfig.Update, and replace that update path with a retry.RetryOnConflict that fetches the latest ApiServer, applies the same encryptionType mutation to apiServer.Spec.Encryption.Type, and retries the Update until it succeeds or a non-conflict error occurs.
204-210:⚠️ Potential issue | 🟠 MajorDon't use a non-blocking receive for
migrationStartedCh.If the watcher goroutine sends a moment later, this
defaultpath reports a false failure and never computes the migration duration. Wait on the same channel with a bounded timeout or cancelable context instead of falling through immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/scenarios.go` around lines 204 - 210, The non-blocking select on migrationStartedCh can cause false failures; replace the select/default with a blocking receive that waits with a bounded timeout or a cancelable context so the test actually observes the watcher send. Specifically, wait for migrationStartedCh (instead of the default path) and compute migrationTime = endTimeStamp.Sub(migrationStarted) before calling tb.Logf and scenario.AssertMigrationTime; if the timeout/context expires, call tb.Error with a clear timeout message. Use the existing symbols migrationStartedCh, endTimeStamp, tb.Logf, and scenario.AssertMigrationTime to locate and update the code.
73-81:⚠️ Potential issue | 🟠 MajorDispatch
""toTestEncryptionTypeUnset.The empty-string provider still falls through to
TestEncryptionTypeIdentity, which writes"identity"intoAPIServer.Spec.Encryption.Typeinstead of exercising the unset/defaulting path that this helper already models separately.Suggested fix
switch provider { case configv1.EncryptionTypeAESCBC: TestEncryptionTypeAESCBC(tb, scenario) case configv1.EncryptionTypeAESGCM: TestEncryptionTypeAESGCM(tb, scenario) case configv1.EncryptionTypeKMS: TestEncryptionTypeKMS(tb, scenario) - case configv1.EncryptionTypeIdentity, "": + case configv1.EncryptionTypeIdentity: TestEncryptionTypeIdentity(tb, scenario) + case "": + TestEncryptionTypeUnset(tb, scenario) default: tb.Fatalf("Unknown encryption type: %s", provider) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/scenarios.go` around lines 73 - 81, The switch in the dispatcher incorrectly routes the empty-string provider to TestEncryptionTypeIdentity; update the case handling so that configv1.EncryptionTypeIdentity maps only to TestEncryptionTypeIdentity and the empty string ("") maps to TestEncryptionTypeUnset. Locate the switch in TestEncryption scenarios (the switch on provider in scenarios.go) and replace the combined case `case configv1.EncryptionTypeIdentity, "": TestEncryptionTypeIdentity(tb, scenario)` with two separate cases: one calling TestEncryptionTypeIdentity(tb, scenario) for configv1.EncryptionTypeIdentity and one calling TestEncryptionTypeUnset(tb, scenario) for the "" provider; leave the other cases (TestEncryptionTypeAESCBC, TestEncryptionTypeAESGCM, TestEncryptionTypeKMS) unchanged.test/library/encryption/perf_helpers.go (1)
35-46:⚠️ Potential issue | 🟠 MajorOnly capture a fresh
"Migrating"transition.This still records any
EncryptionMigrationControllerProgressing=Truecondition, even if it predates the current test or has a different reason. That can inflate the reported migration duration or use a stale start time. Capture astart := time.Now()before polling and only sendLastTransitionTimewhen the condition isTrue,Reason == "Migrating", and the transition is newer thanstart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/perf_helpers.go` around lines 35 - 46, The poll currently sends any EncryptionMigrationControllerProgressing condition with Status True, possibly using a stale LastTransitionTime; fix by recording a start := time.Now() immediately before calling wait.PollUntilContextTimeout and inside the poll only send cond.LastTransitionTime to migrationStartedCh when cond.Type == "EncryptionMigrationControllerProgressing", cond.Status == operatorv1.ConditionTrue, cond.Reason == "Migrating" and cond.LastTransitionTime.Time.After(start); use getOperatorConditionsFn to fetch conditions and ensure you return true,nil only when the transition is newer than start.
🧹 Nitpick comments (3)
pkg/controllers/readiness/wellknown_ready_controller.go (1)
355-358: Optional: cache KAS service port once per function for readability.Line 357 is correct, but pulling
getKASServicePort()out of the loop makes intent clearer and avoids repeated calls in the condition.♻️ Proposed refactor
func getKASTargetPortFromService(service *corev1.Service) (int, bool) { + kasPort := getKASServicePort() for _, port := range service.Spec.Ports { - if targetPort := port.TargetPort.IntValue(); targetPort != 0 && port.Protocol == corev1.ProtocolTCP && int(port.Port) == getKASServicePort() { + if targetPort := port.TargetPort.IntValue(); targetPort != 0 && port.Protocol == corev1.ProtocolTCP && int(port.Port) == kasPort { return targetPort, true } } return 0, false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/readiness/wellknown_ready_controller.go` around lines 355 - 358, The loop in getKASTargetPortFromService repeatedly calls getKASServicePort() inside the condition which obscures intent; cache the result once before iterating by assigning kasPort := getKASServicePort(), then use kasPort in the loop condition (referencing getKASTargetPortFromService and getKASServicePort) so the code is clearer and avoids repeated calls.test/e2e-encryption-perf/encryption_perf.go (2)
55-65: Consider usingt.Fatalfdirectly instead ofrequire.NoErrorwith fabricated errors.The current pattern creates an error just to pass it to
require.NoError, which is indirect. Usingt.Fatalfwould be more idiomatic:♻️ Suggested simplification
AssertDBPopulatedFunc: func(t testing.TB, errorStore map[string]int, statStore map[string]int) { tokenCount, ok := statStore[tokenStatsKey] if !ok { - err := errors.New("missing oauth access tokens count stats, can't continue the test") - require.NoError(t, err) + t.Fatalf("missing oauth access tokens count stats, can't continue the test") } if tokenCount < 14000 { - err := fmt.Errorf("expected to create at least 14000 tokens but %d were created", tokenCount) - require.NoError(t, err) + t.Fatalf("expected to create at least 14000 tokens but %d were created", tokenCount) } t.Logf("Created %d access tokens", tokenCount) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-perf/encryption_perf.go` around lines 55 - 65, The AssertDBPopulatedFunc currently fabricates errors and calls require.NoError; change those to direct test failures using t.Fatalf: when tokenStatsKey is missing, call t.Fatalf("missing oauth access tokens count stats, can't continue the test"); when tokenCount < 14000, call t.Fatalf("expected to create at least 14000 tokens but %d were created", tokenCount). Update the block in AssertDBPopulatedFunc (which reads tokenCount, ok := statStore[tokenStatsKey]) to remove the temporary err variables and use t.Fatalf with the appropriate messages.
81-97: Unusednamespaceparameter in the returned function.The
namespaceparameter on line 82 is unused. If this is intentional (OAuth access tokens are cluster-scoped), consider using_to make it explicit that it's intentionally ignored for clarity, consistent with the other unused parameters.♻️ Suggested clarification
func createAccessTokenWrapper(ctx context.Context, tokenClient oauthclient.OAuthAccessTokensGetter) library.DBLoaderFuncType { - return func(_ kubernetes.Interface, namespace string, errorCollector func(error), statsCollector func(string)) error { + return func(_ kubernetes.Interface, _ string, errorCollector func(error), statsCollector func(string)) error {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-perf/encryption_perf.go` around lines 81 - 97, The returned anonymous function in createAccessTokenWrapper currently declares a parameter named namespace that is never used; update the signature of the returned func (in createAccessTokenWrapper) to replace the unused namespace parameter with _ (e.g., func(_ kubernetes.Interface, _ string, errorCollector func(error), statsCollector func(string)) error) so it's explicit the namespace is intentionally ignored while keeping the rest of the behavior the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e-oidc/external_oidc.go`:
- Line 75: The suite is being gated incorrectly: change the
CheckFeatureGatesOrSkip call so it only requires FeatureGateExternalOIDC (i.e.,
call CheckFeatureGatesOrSkip(t, testCtx, testClient.configClient,
features.FeatureGateExternalOIDC) instead of passing both gates), and remove
FeatureGateExternalOIDCWithAdditionalClaimMappings from the top-level gate; for
tests that need the additional-claim behavior, add local calls to
CheckFeatureGatesOrSkip (or explicit checks) inside those specific subtests
before exercising code paths that depend on
FeatureGateExternalOIDCWithAdditionalClaimMappings; update references around
CheckFeatureGatesOrSkip and any subtest bodies that currently assume the extra
gate is globally enabled.
- Around line 111-118: The cleanup currently always calls
test.WaitForNewKASRollout after testClient.authResourceRollback even when
authResourceRollback was a no-op; change the cleanup closure (around
kasOriginalRevision, testClient.authResourceRollback, and
test.WaitForNewKASRollout) to detect whether the rollback actually persisted a
change and only call WaitForNewKASRollout when it did. Implement this by either:
(A) updating authResourceRollback to return an indicator (e.g., bool changed) in
addition to error and branch on that, or (B) after authResourceRollback, fetch
the current authentication spec (same shape as origAuthSpec) and compare it to
origAuthSpec and only invoke test.WaitForNewKASRollout if they differ; ensure
require.NoError checks still run for the rollback error but skip the rollout
wait when no spec change occurred.
- Around line 554-568: The informers are started with a non-cancelable context,
leaving goroutines running; change the Start/WaitForCacheSync usage to use a
cancelable context derived via context.WithCancel (derive cancelCtx, cancel :=
context.WithCancel(ctx) or similar) before calling
dynamicInformers.Start(cancelCtx.Done()) and
dynamicInformers.WaitForCacheSync(cancelCtx.Done()), and register cleanup to
stop them by either calling cancel() during test teardown or storing
dynamicInformers on the testClient and calling
dynamicInformers.Shutdown()/Shutdown in the teardown; update the code paths
around NewClusterScopedOperatorClient, dynamicInformers, Start,
WaitForCacheSync, and testClient to ensure the cancel function or Shutdown is
invoked in cleanup.
---
Duplicate comments:
In `@test/library/encryption/perf_helpers.go`:
- Around line 35-46: The poll currently sends any
EncryptionMigrationControllerProgressing condition with Status True, possibly
using a stale LastTransitionTime; fix by recording a start := time.Now()
immediately before calling wait.PollUntilContextTimeout and inside the poll only
send cond.LastTransitionTime to migrationStartedCh when cond.Type ==
"EncryptionMigrationControllerProgressing", cond.Status ==
operatorv1.ConditionTrue, cond.Reason == "Migrating" and
cond.LastTransitionTime.Time.After(start); use getOperatorConditionsFn to fetch
conditions and ensure you return true,nil only when the transition is newer than
start.
In `@test/library/encryption/scenarios.go`:
- Around line 340-349: The update to the APIServer object does a single
read-modify-write and can fail on resourceVersion conflicts; wrap the
mutation+Update call in a retry.RetryOnConflict loop so it re-reads and retries
on conflict. Locate the block that calls libClientSet.ApiServerConfig.Get and
then sets apiServer.Spec.Encryption.Type and calling
libClientSet.ApiServerConfig.Update, and replace that update path with a
retry.RetryOnConflict that fetches the latest ApiServer, applies the same
encryptionType mutation to apiServer.Spec.Encryption.Type, and retries the
Update until it succeeds or a non-conflict error occurs.
- Around line 204-210: The non-blocking select on migrationStartedCh can cause
false failures; replace the select/default with a blocking receive that waits
with a bounded timeout or a cancelable context so the test actually observes the
watcher send. Specifically, wait for migrationStartedCh (instead of the default
path) and compute migrationTime = endTimeStamp.Sub(migrationStarted) before
calling tb.Logf and scenario.AssertMigrationTime; if the timeout/context
expires, call tb.Error with a clear timeout message. Use the existing symbols
migrationStartedCh, endTimeStamp, tb.Logf, and scenario.AssertMigrationTime to
locate and update the code.
- Around line 73-81: The switch in the dispatcher incorrectly routes the
empty-string provider to TestEncryptionTypeIdentity; update the case handling so
that configv1.EncryptionTypeIdentity maps only to TestEncryptionTypeIdentity and
the empty string ("") maps to TestEncryptionTypeUnset. Locate the switch in
TestEncryption scenarios (the switch on provider in scenarios.go) and replace
the combined case `case configv1.EncryptionTypeIdentity, "":
TestEncryptionTypeIdentity(tb, scenario)` with two separate cases: one calling
TestEncryptionTypeIdentity(tb, scenario) for configv1.EncryptionTypeIdentity and
one calling TestEncryptionTypeUnset(tb, scenario) for the "" provider; leave the
other cases (TestEncryptionTypeAESCBC, TestEncryptionTypeAESGCM,
TestEncryptionTypeKMS) unchanged.
---
Nitpick comments:
In `@pkg/controllers/readiness/wellknown_ready_controller.go`:
- Around line 355-358: The loop in getKASTargetPortFromService repeatedly calls
getKASServicePort() inside the condition which obscures intent; cache the result
once before iterating by assigning kasPort := getKASServicePort(), then use
kasPort in the loop condition (referencing getKASTargetPortFromService and
getKASServicePort) so the code is clearer and avoids repeated calls.
In `@test/e2e-encryption-perf/encryption_perf.go`:
- Around line 55-65: The AssertDBPopulatedFunc currently fabricates errors and
calls require.NoError; change those to direct test failures using t.Fatalf: when
tokenStatsKey is missing, call t.Fatalf("missing oauth access tokens count
stats, can't continue the test"); when tokenCount < 14000, call
t.Fatalf("expected to create at least 14000 tokens but %d were created",
tokenCount). Update the block in AssertDBPopulatedFunc (which reads tokenCount,
ok := statStore[tokenStatsKey]) to remove the temporary err variables and use
t.Fatalf with the appropriate messages.
- Around line 81-97: The returned anonymous function in createAccessTokenWrapper
currently declares a parameter named namespace that is never used; update the
signature of the returned func (in createAccessTokenWrapper) to replace the
unused namespace parameter with _ (e.g., func(_ kubernetes.Interface, _ string,
errorCollector func(error), statsCollector func(string)) error) so it's explicit
the namespace is intentionally ignored while keeping the rest of the behavior
the same.
🪄 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: be01b227-071d-425e-b47e-7ae44ef825ce
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/k8s.io/klog/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/textlogger.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (22)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_test.gotest/e2e-encryption-kms/main_test.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption-rotation/encryption_rotation.gotest/e2e-encryption/encryption.gotest/e2e-encryption/encryption_test.gotest/e2e-encryption/main_test.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/client.gotest/library/encryption/helpers.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.gotest/library/featuregates.gotest/library/waits.go
💤 Files with no reviewable changes (2)
- test/e2e-encryption-kms/main_test.go
- test/e2e-encryption/main_test.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (6)
- test/library/encryption_wrappers.go
- test/library/featuregates.go
- cmd/cluster-authentication-operator-tests-ext/main.go
- test/e2e-encryption-kms/encryption_kms_test.go
- test/e2e-encryption/encryption.go
- test/library/waits.go
…nsion framework This commit migrates five test suites (e2e-encryption, e2e-encryption-kms, e2e-encryption-perf, e2e-encryption-rotation, e2e-oidc) from standalone test binaries to the OpenShift Tests Extension (OTE) framework using Ginkgo v2. Migration Changes: - Migrate tests to Ginkgo v2 with OTE integration - Extract test logic into dedicated helper files for better organization - Remove obsolete main_test.go files in favor of Ginkgo suite setup - Configure appropriate test suite parallelism and qualifiers - Add wrapper functions for Ginkgo v2 testing.TB compatibility Test Organization: - operator/parallel: Non-serial operator, templates, and tokens tests (parallelism: 4) - operator/serial: Serial operator, OIDC, templates, and tokens tests - operator/disruptive: Tests marked as [Disruptive] - operator-encryption/serial: Basic encryption tests - operator-encryption-rotation/serial: Encryption key rotation tests - operator-encryption-perf/serial: Encryption performance tests - operator-encryption-kms/serial: KMS encryption tests Dependencies: - Use standard OTE version v0.0.0-20251205182537-ff5553e56f33 - Updated klog dependency to v2.140.0 - Removed fork dependency (was blocking supply chain review) Test Reliability Improvements (addresses coderabbitai feedback): 1. Migration timestamp validation: - Capture test start time before starting migration watcher - Validate cond.Reason == "Migrating" - Verify condition transition is fresh (after test start) - Prevents accepting stale conditions from previous tests 2. Empty string provider routing: - Separate empty string case from Identity case - Empty string now correctly calls TestEncryptionTypeUnset - Ensures proper encryption defaulting path is exercised 3. Non-blocking select timeout: - Replace default case with 30-second timeout - Prevents immediate test failure if watcher hasn't sent yet - Provides bounded wait instead of instant failure 4. APIServer Get+Update conflict handling: - Wrap Get+Update pattern in retry.RetryOnConflict - Handle ResourceVersion conflicts from concurrent writes - Prevents flaky test failures from transient conflicts Fixes: - Fixed KMS plugin deployment calls (added missing count parameter) - Fixed typo in encryption rotation comment (encyrption -> encryption) Testing: - All unit tests pass - Binary builds successfully (111 MB) - make verify passes - No compilation errors Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
aa529ed to
805bcb9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (7)
test/e2e-encryption-kms/encryption_kms.go (2)
48-53: Typo: "techincal" should be "technical".Line 52 contains a spelling error in the comment.
📝 Fix typo
// TODO: Remove this skip once the authentication operator fully supports KMS encryption. // Currently, while the API accepts encryption.type: "KMS" and the operator mounts the KMS // plugin socket, it does not generate the EncryptionConfiguration with KMS provider stanza. // This causes tests to timeout waiting for encryption keys to be created and migration to complete. - // See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under techincal preview + // See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under technical preview t.Skip("Skipping KMS encryption test: operator implementation is incomplete")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-kms/encryption_kms.go` around lines 48 - 53, Fix the spelling mistake in the comment above the t.Skip call: change "techincal" to "technical" in the comment that references the operator being under technical preview (the comment block immediately preceding the t.Skip("Skipping KMS encryption test: operator implementation is incomplete") line).
95-100: Same typo: "techincal" should be "technical".📝 Fix typo
// TODO: Remove this skip once the authentication operator fully supports KMS encryption. // Currently, while the API accepts encryption.type: "KMS" and the operator mounts the KMS // plugin socket, it does not generate the EncryptionConfiguration with KMS provider stanza. // This causes tests to timeout waiting for encryption keys to be created and migration to complete. - // See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under techincal preview + // See: https://redhat.atlassian.net/browse/OCPSTRAT-108 it is still under technical preview t.Skip("Skipping KMS encryption test: operator implementation is incomplete")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-kms/encryption_kms.go` around lines 95 - 100, Fix the typo in the block comment above the t.Skip call: change "techincal" to "technical" in the sentence referencing the technical preview and the Jira ticket so the comment reads correctly; the change is in the comment surrounding the t.Skip("Skipping KMS encryption test: operator implementation is incomplete") statement in encryption_kms.go.test/library/encryption/perf_helpers.go (1)
118-120: Misleading log message when workers encounter errors.The log message "All workers completed successfully" is printed regardless of whether workers encountered errors. Since errors are collected in
r.errorStorerather than causing the function to fail, this message can be misleading during debugging.📝 Suggested improvement
r.wg.Wait() - t.Log("All workers completed successfully") + if len(r.errorStore) > 0 { + t.Logf("All workers completed with %d unique error types", len(r.errorStore)) + } else { + t.Log("All workers completed successfully") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/perf_helpers.go` around lines 118 - 120, The current code always logs "All workers completed successfully" after r.wg.Wait() which is misleading because worker errors are stored in r.errorStore; update the post-wait logic to inspect r.errorStore (e.g., check its error count or equivalent method on the runner) and only emit the success message when it is empty, otherwise log or fail the test with the collected errors (use t.Logf/t.Fatalf and include the contents of r.errorStore) so the message accurately reflects worker outcomes.test/e2e-encryption-rotation/encryption_rotation.go (1)
76-76: Use the predefined constantconfigv1.EncryptionTypeAESCBCinstead of string cast.Line 76 uses
configv1.EncryptionType("aescbc"), which works but should use the constantconfigv1.EncryptionTypeAESCBCdefined in the API types. This is consistent with how the constant is used throughout the encryption test library and aligns with the codebase pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-encryption-rotation/encryption_rotation.go` at line 76, Replace the string-cast usage configv1.EncryptionType("aescbc") with the predefined constant configv1.EncryptionTypeAESCBC in the EncryptionProvider field; locate the assignment in encryption_rotation.go (the EncryptionProvider assignment) and swap the value to configv1.EncryptionTypeAESCBC so the test uses the API constant consistently with other tests.test/e2e-oidc/external_oidc.go (3)
950-969: Consider cleaning up test-created Keycloak resources.The test creates a group and user in Keycloak but doesn't explicitly clean them up. If the Keycloak instance persists across test runs, these resources accumulate. If the IDP cleanup (
test.AddKeycloakIDPcleanup handlers at line 127) removes the entire Keycloak deployment, this is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc.go` around lines 950 - 969, The test creates Keycloak resources (group and user via kcClient.CreateGroup and kcClient.CreateUser assigned to variables group and user) but never explicitly deletes them; add cleanup logic that calls the appropriate kcClient.DeleteGroup(group) and kcClient.DeleteUser(user) (or equivalent deletion methods) in a deferred or test cleanup handler after creation, or ensure these deletions are registered with the existing test.AddKeycloakIDP cleanup handlers so the group and user are removed when the Keycloak IDP is torn down.
390-439: Use context-aware HTTP requests for cancellation support.The
client.Get()calls don't support context cancellation. Consider accepting a context parameter and usinghttp.NewRequestWithContextwithclient.Do()instead. This allows the requests to be cancelled if the test context times out.♻️ Proposed fix
-func fetchIssuerJWKS(issuerURL string) (*jwks, error) { +func fetchIssuerJWKS(ctx context.Context, issuerURL string) (*jwks, error) { client := &http.Client{ Timeout: 30 * time.Second, Transport: &http.Transport{ TLSClientConfig: &tls.Config{ InsecureSkipVerify: true, MinVersion: tls.VersionTLS12, }, }, } // grab openid-configuration JSON which contains the URL of the provider's JWKS - resp, err := client.Get(issuerURL + "/.well-known/openid-configuration") + req, err := http.NewRequestWithContext(ctx, http.MethodGet, issuerURL+"/.well-known/openid-configuration", nil) + if err != nil { + return nil, fmt.Errorf("could not create request for well-known config: %v", err) + } + resp, err := client.Do(req) if err != nil { return nil, fmt.Errorf("could not get issuer OpenID well-known configuration: %v", err) } defer resp.Body.Close() // ... similar change for the second request at line 422🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc.go` around lines 390 - 439, fetchIssuerJWKS currently uses client.Get which doesn't support cancellation; change its signature to accept ctx context.Context (e.g., fetchIssuerJWKS(ctx context.Context, issuerURL string) (*jwks, error)) and replace both client.Get calls with context-aware requests by creating req := http.NewRequestWithContext(ctx, "GET", issuerURL+"/.well-known/openid-configuration", nil) and req = http.NewRequestWithContext(ctx, "GET", oidcConfig.JwksURL, nil) and use client.Do(req) for each, preserving existing error handling and defer resp.Body.Close().
690-716: String-based JSON comparison is fragile.The exact string comparison of
auth-config.jsonwill break if field ordering changes in the operator's JSON marshaling (though Go'sjson.Marshalis currently deterministic by struct field order). Consider unmarshaling both and using semantic comparison viaequality.Semantic.DeepEqualfor more robust assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-oidc/external_oidc.go` around lines 690 - 716, The test currently compares auth-config.json via exact string equality (expectedAuthConfigJSON vs actualCM.Data["auth-config.json"]) which is fragile; instead unmarshal both the expectedAuthConfigJSON and actualCM.Data["auth-config.json"] into generic JSON objects (e.g., map[string]interface{} or interface{}) and use k8s.io/apimachinery/pkg/api/equality.Semantic.DeepEqual to assert semantic equality; update the assertion after fetching actualCM (in the loop that calls tc.kubeClient.CoreV1().ConfigMaps(...).Get) to unmarshal both JSON strings, check errors with require.NoError, and require.Truef(t, equality.Semantic.DeepEqual(expectedObj, actualObj), "unexpected auth-config.json contents in %s/%s: diff=%v", actualCM.Namespace, actualCM.Name, cmp.Diff(expectedObj, actualObj)) so field ordering changes won’t break the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 48-53: Fix the spelling mistake in the comment above the t.Skip
call: change "techincal" to "technical" in the comment that references the
operator being under technical preview (the comment block immediately preceding
the t.Skip("Skipping KMS encryption test: operator implementation is
incomplete") line).
- Around line 95-100: Fix the typo in the block comment above the t.Skip call:
change "techincal" to "technical" in the sentence referencing the technical
preview and the Jira ticket so the comment reads correctly; the change is in the
comment surrounding the t.Skip("Skipping KMS encryption test: operator
implementation is incomplete") statement in encryption_kms.go.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Line 76: Replace the string-cast usage configv1.EncryptionType("aescbc") with
the predefined constant configv1.EncryptionTypeAESCBC in the EncryptionProvider
field; locate the assignment in encryption_rotation.go (the EncryptionProvider
assignment) and swap the value to configv1.EncryptionTypeAESCBC so the test uses
the API constant consistently with other tests.
In `@test/e2e-oidc/external_oidc.go`:
- Around line 950-969: The test creates Keycloak resources (group and user via
kcClient.CreateGroup and kcClient.CreateUser assigned to variables group and
user) but never explicitly deletes them; add cleanup logic that calls the
appropriate kcClient.DeleteGroup(group) and kcClient.DeleteUser(user) (or
equivalent deletion methods) in a deferred or test cleanup handler after
creation, or ensure these deletions are registered with the existing
test.AddKeycloakIDP cleanup handlers so the group and user are removed when the
Keycloak IDP is torn down.
- Around line 390-439: fetchIssuerJWKS currently uses client.Get which doesn't
support cancellation; change its signature to accept ctx context.Context (e.g.,
fetchIssuerJWKS(ctx context.Context, issuerURL string) (*jwks, error)) and
replace both client.Get calls with context-aware requests by creating req :=
http.NewRequestWithContext(ctx, "GET",
issuerURL+"/.well-known/openid-configuration", nil) and req =
http.NewRequestWithContext(ctx, "GET", oidcConfig.JwksURL, nil) and use
client.Do(req) for each, preserving existing error handling and defer
resp.Body.Close().
- Around line 690-716: The test currently compares auth-config.json via exact
string equality (expectedAuthConfigJSON vs actualCM.Data["auth-config.json"])
which is fragile; instead unmarshal both the expectedAuthConfigJSON and
actualCM.Data["auth-config.json"] into generic JSON objects (e.g.,
map[string]interface{} or interface{}) and use
k8s.io/apimachinery/pkg/api/equality.Semantic.DeepEqual to assert semantic
equality; update the assertion after fetching actualCM (in the loop that calls
tc.kubeClient.CoreV1().ConfigMaps(...).Get) to unmarshal both JSON strings,
check errors with require.NoError, and require.Truef(t,
equality.Semantic.DeepEqual(expectedObj, actualObj), "unexpected
auth-config.json contents in %s/%s: diff=%v", actualCM.Namespace, actualCM.Name,
cmp.Diff(expectedObj, actualObj)) so field ordering changes won’t break the
test.
In `@test/library/encryption/perf_helpers.go`:
- Around line 118-120: The current code always logs "All workers completed
successfully" after r.wg.Wait() which is misleading because worker errors are
stored in r.errorStore; update the post-wait logic to inspect r.errorStore
(e.g., check its error count or equivalent method on the runner) and only emit
the success message when it is empty, otherwise log or fail the test with the
collected errors (use t.Logf/t.Fatalf and include the contents of r.errorStore)
so the message accurately reflects worker outcomes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b29dbfbd-1513-45ba-a96f-a0e9944a0f1f
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/k8s.io/klog/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/textlogger.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (22)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_test.gotest/e2e-encryption-kms/main_test.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption-rotation/encryption_rotation.gotest/e2e-encryption/encryption.gotest/e2e-encryption/encryption_test.gotest/e2e-encryption/main_test.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/client.gotest/library/encryption/helpers.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.gotest/library/featuregates.gotest/library/waits.go
💤 Files with no reviewable changes (2)
- test/e2e-encryption/main_test.go
- test/e2e-encryption-kms/main_test.go
✅ Files skipped from review due to trivial changes (2)
- go.mod
- test/library/encryption/helpers.go
🚧 Files skipped from review as they are similar to previous changes (9)
- test/library/featuregates.go
- test/library/client.go
- test/e2e-encryption/encryption.go
- pkg/controllers/readiness/wellknown_ready_controller.go
- test/e2e-encryption/encryption_test.go
- test/library/waits.go
- test/e2e-encryption-perf/encryption_perf.go
- test/library/encryption_wrappers.go
- test/library/encryption/scenarios.go
Contributor
Author
|
/test e2e-agnostic-ipv6 |
Contributor
Author
|
@coderabbitai can you look at failing jobs and see if this PR is the cause of those errors? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Unrecoverable error: Failed to clone repository 🏁 Script executed: Length of output: 10050 Let me fetch the logs from the failing jobs to understand the root cause. 🏁 Script executed: Length of output: 3332 🏁 Script executed: Length of output: 50284
1.
|
| Job | Root Cause | Related to PR? |
|---|---|---|
e2e-agnostic-upgrade |
MonitorTest failure (3/10) | ❌ No |
e2e-aws-operator-encryption-perf-serial-ote-1of2 |
Post-phase job timeout (test itself passed!) | ❌ No |
e2e-aws-operator-encryption-serial-ote-2of2 |
MonitorTest failure (3/10) | ❌ No |
e2e-aws-operator-parallel-ote |
MonitorTest failure (2/10) | ❌ No |
e2e-aws-operator-serial-ote |
Mass MonitorTest failure (12/10) | ❌ No |
All failures are infrastructure-level MonitorTest violations or a CI job timeout during artifact collection — not caused by the code changes in this PR. Notably, all GCP-based encryption jobs passed, the actual test steps in failing jobs either passed or weren't reached, and the original "invalid character 'W'" JSON parsing issue fixed by this PR is not present in any of the failures.
You can safely request a re-run of the failing jobs with /retest.
Contributor
Author
|
Failure logs are not wrt the PR. Here the tc execution list PASS logs for profile: e2e-agnostic-ipv6 The dependent PR is merged: openshift/origin#31009 /verified by CI results |
openshift-ci-robot
added
the
verified
Contributor
|
@ropatil010: 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. |
ropatil010
changed the title
ropatil010
changed the title
liouk
suggested changes
Apr 29, 2026
Remove LocalClientSet type and toLibraryClientSet() converter function that duplicated library.ClientSet from openshift/library-go. Update SetAndWaitForEncryptionType to return library.ClientSet directly, eliminating unnecessary wrapper conversions. This addresses reviewer feedback on PR openshift#859 to simplify the code and remove duplication. Co-Authored-By: Rohit Patil <ropatil@redhat.com>
openshift-ci-robot
removed
the
verified
Contributor
|
@ropatil010: This pull request references CNTRLPLANE-2589 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 epic 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. |
Member
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Member
|
@ropatil010 could you please also address the following comments from @coderabbitai:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7642
1.
|
This commit addresses all CodeRabbit AI review suggestions: - Fix misleading success log in perf_helpers to check error store - Use named constant EncryptionTypeAESCBC instead of string literal - Add cleanup handlers for Keycloak groups and users in OIDC tests - Make fetchIssuerJWKS context-aware with proper cancellation support - Implement missing DeleteGroup and DeleteUser methods in KeycloakClient Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Contributor
Author
|
/test e2e-agnostic-ipv6 |
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 `@test/e2e-oidc/external_oidc.go`:
- Around line 171-181: The polling callback masks real API errors by setting
cmErr and returning (false, nil); change the callback used in
wait.PollUntilContextTimeout so that when the GET returns a non-NotFound error
you return (false, err) immediately (use the result of
testClient.kubeClient.CoreV1().ConfigMaps(managedNS).Get(...) directly), and
remove the separate cmErr handling; also fix the final require.NoError assertion
to pass the actual error variable (waitErr) as the formatting argument rather
than the undefined err so failures surface immediately from
wait.PollUntilContextTimeout and the ConfigMap GET.
In `@test/library/encryption/perf_helpers.go`:
- Around line 55-58: The polling branch that handles an error from
wait.PollUntilContextTimeout currently logs and returns without notifying
migrationStartedCh, which leaves callers blocked; modify the error path in the
function containing the wait.PollUntilContextTimeout call so that before
returning it either closes migrationStartedCh or sends a zero time value on
migrationStartedCh to signal failure, and update callers to handle a closed
channel/zero time appropriately; reference the wait.PollUntilContextTimeout
invocation and the migrationStartedCh channel when making the change.
In `@test/library/keycloakidp.go`:
- Around line 393-457: DeleteGroup currently uses ListGroups which only fetches
the first page, so targets on later pages are missed and DeleteGroup returns nil
(silent failure); update the logic to either (A) make ListGroups support
pagination (accept first/max or iterate internally) and fetch all pages using
the Keycloak admin query params "first" and "max" until an empty page, or (B)
change DeleteGroup to page through results itself by calling the admin /groups
endpoint with increasing first offsets (e.g., first=0, first+=max) and scanning
each page for the matching name until found. After exhausting pages, return a
clear error (not nil) when the group isn't found. Apply the same pattern for
DeleteUser/ListUsers and use the functions ListGroups, DeleteGroup, ListUsers,
DeleteUser as entry points for the change.
🪄 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: 4568ba91-241f-4c97-b2a4-df197d85e4dd
📒 Files selected for processing (6)
test/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-rotation/encryption_rotation.gotest/e2e-encryption/encryption.gotest/e2e-oidc/external_oidc.gotest/library/encryption/perf_helpers.gotest/library/keycloakidp.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e-encryption/encryption.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-encryption-perf/encryption_perf.go
Contributor
Author
Fix polling error handling and resource pagination issues: - Make ConfigMap polling fail fast on API errors instead of timing out - Signal migration watcher failures via channel close to prevent indefinite hangs - Add pagination support to Keycloak ListGroups/ListUsers to handle >100 entries - Handle zero-time values from closed channels in migration time calculations These changes ensure test failures surface immediately and cleanup operations work correctly with large datasets. Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
Contributor
|
@ropatil010: The following tests 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. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/library/encryption/perf_helpers.go (1)
25-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake the migration watcher cancelable.
This helper hard-wires the poll loop to
context.Background(). If the caller times out waiting onmigrationStartedCh, the goroutine keeps polling for up towaitPollTimeoutand can keep logging throughtafter the spec has already returned. Please take a caller-ownedcontext.Contexthere and cancel it inTestPerfEncryptionon both the success and timeout paths.Suggested direction
-func watchForMigrationControllerProgressingConditionAsync(t testing.TB, getOperatorCondFn library.GetOperatorConditionsFuncType, migrationStartedCh chan time.Time, testStartTime time.Time) { +func watchForMigrationControllerProgressingConditionAsync(ctx context.Context, t testing.TB, getOperatorCondFn library.GetOperatorConditionsFuncType, migrationStartedCh chan time.Time, testStartTime time.Time) { t.Helper() - go watchForMigrationControllerProgressingCondition(t, getOperatorCondFn, migrationStartedCh, testStartTime) + go watchForMigrationControllerProgressingCondition(ctx, t, getOperatorCondFn, migrationStartedCh, testStartTime) } -func watchForMigrationControllerProgressingCondition(t testing.TB, getOperatorConditionsFn library.GetOperatorConditionsFuncType, migrationStartedCh chan time.Time, testStartTime time.Time) { +func watchForMigrationControllerProgressingCondition(ctx context.Context, t testing.TB, getOperatorConditionsFn library.GetOperatorConditionsFuncType, migrationStartedCh chan time.Time, testStartTime time.Time) { t.Helper() t.Logf("Waiting up to %s for the condition %q with the reason %q to be set to true (after %v)", waitPollTimeout.String(), "EncryptionMigrationControllerProgressing", "Migrating", testStartTime) - ctx := context.Background() err := wait.PollUntilContextTimeout(ctx, waitPollInterval, waitPollTimeout, true, func(ctx context.Context) (bool, error) {🤖 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 `@test/library/encryption/perf_helpers.go` around lines 25 - 38, The watcher uses context.Background() so its poll can't be canceled by the caller; change watchForMigrationControllerProgressingCondition to accept a caller-owned context.Context parameter and use that context in wait.PollUntilContextTimeout instead of context.Background(), update the helper wrapper watchForMigrationControllerProgressingConditionAsync to take and forward the same context, and in TestPerfEncryption create a cancellable context and cancel it on both the success and timeout paths to ensure the goroutine stops and stops logging to t.
🧹 Nitpick comments (1)
test/e2e-encryption/encryption.go (1)
30-39: ⚡ Quick winExtract the shared oauth-apiserver
BasicScenariodefaults.The same
Namespace/LabelSelector/secret/operator tuple is repeated in all three helpers here, and the same literal shows up again in the KMS/perf suites. A tiny constructor keeps those suites from drifting on the next config rename.♻️ Refactor sketch
+func oauthEncryptionBasicScenario() library.BasicScenario { + return library.BasicScenario{ + Namespace: "openshift-config-managed", + LabelSelector: "encryption.apiserver.operator.openshift.io/component=openshift-oauth-apiserver", + EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", + EncryptionConfigSecretNamespace: "openshift-config-managed", + OperatorNamespace: "openshift-authentication-operator", + TargetGRs: operatorencryption.DefaultTargetGRs, + AssertFunc: operatorencryption.AssertTokens, + } +} + func testEncryptionTypeIdentity(t testing.TB) { - testlibrary.TestEncryptionTypeIdentity(t, library.BasicScenario{ - Namespace: "openshift-config-managed", - LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", - EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", - EncryptionConfigSecretNamespace: "openshift-config-managed", - OperatorNamespace: "openshift-authentication-operator", - TargetGRs: operatorencryption.DefaultTargetGRs, - AssertFunc: operatorencryption.AssertTokens, - }) + testlibrary.TestEncryptionTypeIdentity(t, oauthEncryptionBasicScenario()) }Also applies to: 42-51, 54-73
🤖 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 `@test/e2e-encryption/encryption.go` around lines 30 - 39, Extract the repeated oauth-apiserver BasicScenario defaults into a small constructor function (e.g., newOAuthAPIServerScenario or oauthAPIServerDefaults) that returns library.BasicScenario prefilled with Namespace="openshift-config-managed", LabelSelector="encryption.apiserver.operator.openshift.io/component=openshift-oauth-apiserver", EncryptionConfigSecretName="encryption-config-openshift-oauth-apiserver", EncryptionConfigSecretNamespace="openshift-config-managed", and OperatorNamespace="openshift-authentication-operator"; then update testEncryptionTypeIdentity (and the other helpers that repeat the same literals) to call that constructor and only set/override the remaining fields (TargetGRs, AssertFunc, etc.) so all oauth-apiserver consumers (including KMS/perf suites) share the single source of truth.
🤖 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 `@test/e2e-encryption-perf/encryption_perf.go`:
- Around line 55-66: The callback AssertDBPopulatedFunc must fail fast if any
worker errors occurred: inspect errorStore (the map[string]int passed in) before
trusting statStore and require it to be empty (or sum/count > 0 should cause a
test failure) and include details of the non-zero errors in the failure message;
after asserting errorStore is empty, proceed to read tokenStatsKey from
statStore (tokenCount) and assert tokenCount >= 14000 as before. Ensure you
reference AssertDBPopulatedFunc, errorStore, statStore, and tokenStatsKey when
adding the pre-check and error message.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Around line 47-74: The Get→mutate→Update block inside UnsupportedConfigFunc
should be wrapped in retry.RetryOnConflict: call retry.RetryOnConflict with a
closure that re-fetches the operator (cs.OperatorClient.Get(ctx, "cluster",
metav1.GetOptions{})), applies the same json.Unmarshal/SetNestedMap/Marshal
logic to set authOperator.Spec.UnsupportedConfigOverrides.Raw, and then calls
cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{}), returning
the update error from the closure; propagate the final error returned by
RetryOnConflict. Keep use of ctx, operatorencryption.GetClients,
cs.OperatorClient.Get/Update, and
oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix as in the original
code.
---
Duplicate comments:
In `@test/library/encryption/perf_helpers.go`:
- Around line 25-38: The watcher uses context.Background() so its poll can't be
canceled by the caller; change watchForMigrationControllerProgressingCondition
to accept a caller-owned context.Context parameter and use that context in
wait.PollUntilContextTimeout instead of context.Background(), update the helper
wrapper watchForMigrationControllerProgressingConditionAsync to take and forward
the same context, and in TestPerfEncryption create a cancellable context and
cancel it on both the success and timeout paths to ensure the goroutine stops
and stops logging to t.
---
Nitpick comments:
In `@test/e2e-encryption/encryption.go`:
- Around line 30-39: Extract the repeated oauth-apiserver BasicScenario defaults
into a small constructor function (e.g., newOAuthAPIServerScenario or
oauthAPIServerDefaults) that returns library.BasicScenario prefilled with
Namespace="openshift-config-managed",
LabelSelector="encryption.apiserver.operator.openshift.io/component=openshift-oauth-apiserver",
EncryptionConfigSecretName="encryption-config-openshift-oauth-apiserver",
EncryptionConfigSecretNamespace="openshift-config-managed", and
OperatorNamespace="openshift-authentication-operator"; then update
testEncryptionTypeIdentity (and the other helpers that repeat the same literals)
to call that constructor and only set/override the remaining fields (TargetGRs,
AssertFunc, etc.) so all oauth-apiserver consumers (including KMS/perf suites)
share the single source of truth.
🪄 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: e8222167-d1ad-493c-aa5d-1881a44b8a74
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/k8s.io/klog/v2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_no_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/internal/serialize/keyvalues_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/klogr_slog.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/options.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/klog/v2/textlogger/textlogger.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (23)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.gotest/e2e-encryption-kms/encryption_kms.gotest/e2e-encryption-kms/encryption_kms_test.gotest/e2e-encryption-kms/main_test.gotest/e2e-encryption-perf/encryption_perf.gotest/e2e-encryption-perf/encryption_perf_test.gotest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption-rotation/encryption_rotation.gotest/e2e-encryption/encryption.gotest/e2e-encryption/encryption_test.gotest/e2e-encryption/main_test.gotest/e2e-oidc/external_oidc.gotest/e2e-oidc/external_oidc_test.gotest/library/client.gotest/library/encryption/helpers.gotest/library/encryption/perf_helpers.gotest/library/encryption/scenarios.gotest/library/encryption_wrappers.gotest/library/featuregates.gotest/library/keycloakidp.gotest/library/waits.go
💤 Files with no reviewable changes (2)
- test/e2e-encryption/main_test.go
- test/e2e-encryption-kms/main_test.go
Comment on lines
+47
to
+74
| UnsupportedConfigFunc: func(rawUnsupportedEncryptionCfg []byte) error { | ||
| cs := operatorencryption.GetClients(t) | ||
| authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| unsupportedConfigAsMap := map[string]interface{}{} | ||
| if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 { | ||
| if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| unsupportedEncryptionConfigAsMap := map[string]interface{}{} | ||
| if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil { | ||
| return err | ||
| } | ||
| if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil { | ||
| return err | ||
| } | ||
| rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg | ||
|
|
||
| _, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{}) | ||
| return err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and read the relevant section
fd -t f "encryption_rotation.go" test/Repository: openshift/cluster-authentication-operator
Length of output: 137
🏁 Script executed:
# Read the file to see the actual code
wc -l test/e2e-encryption-rotation/encryption_rotation.goRepository: openshift/cluster-authentication-operator
Length of output: 140
🏁 Script executed:
# Read lines around 47-74 to verify the code
sed -n '40,80p' test/e2e-encryption-rotation/encryption_rotation.goRepository: openshift/cluster-authentication-operator
Length of output: 1658
🏁 Script executed:
# Check for retry.RetryOnConflict usage in the codebase
rg "retry\.RetryOnConflict" --type goRepository: openshift/cluster-authentication-operator
Length of output: 1541
🏁 Script executed:
# Check for similar Get-Update patterns
rg "OperatorClient\.Get\(" --type go -A 5 | head -100Repository: openshift/cluster-authentication-operator
Length of output: 1036
🏁 Script executed:
# Check if retry package is imported anywhere in similar test files
rg '"k8s\.io/client-go/util/retry"' --type goRepository: openshift/cluster-authentication-operator
Length of output: 874
Wrap the operator update in retry.RetryOnConflict to handle concurrent writes.
This Get → mutate → Update path runs against a live operator resource, so a concurrent write can invalidate the fetched resourceVersion and cause the test to fail spuriously. Please wrap the mutation in retry.RetryOnConflict and re-fetch inside the retry closure.
Suggested fix
+import "k8s.io/client-go/util/retry"
+
UnsupportedConfigFunc: func(rawUnsupportedEncryptionCfg []byte) error {
cs := operatorencryption.GetClients(t)
- authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{})
- if err != nil {
- return err
- }
-
- unsupportedConfigAsMap := map[string]interface{}{}
- if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 {
- if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil {
- return err
- }
- }
- unsupportedEncryptionConfigAsMap := map[string]interface{}{}
- if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil {
- return err
- }
- if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil {
- return err
- }
- rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap)
- if err != nil {
- return err
- }
- authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg
-
- _, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{})
- return err
+ return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{})
+ if err != nil {
+ return err
+ }
+
+ unsupportedConfigAsMap := map[string]interface{}{}
+ if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 {
+ if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil {
+ return err
+ }
+ }
+ unsupportedEncryptionConfigAsMap := map[string]interface{}{}
+ if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil {
+ return err
+ }
+ if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil {
+ return err
+ }
+ rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap)
+ if err != nil {
+ return err
+ }
+ authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg
+
+ _, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{})
+ return err
+ })
},📝 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.
Suggested change
| UnsupportedConfigFunc: func(rawUnsupportedEncryptionCfg []byte) error { | |
| cs := operatorencryption.GetClients(t) | |
| authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| unsupportedConfigAsMap := map[string]interface{}{} | |
| if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 { | |
| if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil { | |
| return err | |
| } | |
| } | |
| unsupportedEncryptionConfigAsMap := map[string]interface{}{} | |
| if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil { | |
| return err | |
| } | |
| if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil { | |
| return err | |
| } | |
| rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap) | |
| if err != nil { | |
| return err | |
| } | |
| authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg | |
| _, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{}) | |
| return err | |
| UnsupportedConfigFunc: func(rawUnsupportedEncryptionCfg []byte) error { | |
| cs := operatorencryption.GetClients(t) | |
| return retry.RetryOnConflict(retry.DefaultRetry, func() error { | |
| authOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{}) | |
| if err != nil { | |
| return err | |
| } | |
| unsupportedConfigAsMap := map[string]interface{}{} | |
| if len(authOperator.Spec.UnsupportedConfigOverrides.Raw) > 0 { | |
| if err := json.Unmarshal(authOperator.Spec.UnsupportedConfigOverrides.Raw, &unsupportedConfigAsMap); err != nil { | |
| return err | |
| } | |
| } | |
| unsupportedEncryptionConfigAsMap := map[string]interface{}{} | |
| if err := json.Unmarshal(rawUnsupportedEncryptionCfg, &unsupportedEncryptionConfigAsMap); err != nil { | |
| return err | |
| } | |
| if err := unstructured.SetNestedMap(unsupportedConfigAsMap, unsupportedEncryptionConfigAsMap, oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix); err != nil { | |
| return err | |
| } | |
| rawUnsupportedCfg, err := json.Marshal(unsupportedConfigAsMap) | |
| if err != nil { | |
| return err | |
| } | |
| authOperator.Spec.UnsupportedConfigOverrides.Raw = rawUnsupportedCfg | |
| _, err = cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{}) | |
| return err | |
| }) | |
| }, |
🤖 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 `@test/e2e-encryption-rotation/encryption_rotation.go` around lines 47 - 74,
The Get→mutate→Update block inside UnsupportedConfigFunc should be wrapped in
retry.RetryOnConflict: call retry.RetryOnConflict with a closure that re-fetches
the operator (cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{})),
applies the same json.Unmarshal/SetNestedMap/Marshal logic to set
authOperator.Spec.UnsupportedConfigOverrides.Raw, and then calls
cs.OperatorClient.Update(ctx, authOperator, metav1.UpdateOptions{}), returning
the update error from the closure; propagate the final error returned by
RetryOnConflict. Keep use of ctx, operatorencryption.GetClients,
cs.OperatorClient.Get/Update, and
oauthapiconfigobservercontroller.OAuthAPIServerConfigPrefix as in the original
code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hi Team,
This PR restores the Ginkgo v2 migration from PR #839 with a critical fix for the stdout corruption issue that caused it to be reverted in PR #857.
Changes from PR #839:
Critical fix (new):
The fix adds klog.SetOutput(os.Stderr) in main() to ensure:
Fixes the issue from PR #857.
Summary by CodeRabbit
Tests
Chores