CNTRLPLANE-2589: Migrate e2e-* tests to Ginkgo v2#859
Open
ropatil010 wants to merge 3 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 and registers encryption and OIDC e2e suites, refactors many test entrypoints to delegate to shared library implementations, introduces encryption perf helpers and Keycloak admin utilities, standardizes test helper interfaces, removes unsafe TestMain randomization, lazily reads KAS service port, and bumps klog in go.mod. ChangesE2E Test Infrastructure Expansion and Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ 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
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
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
|
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 |
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.
|
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
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
✅ 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
ropatil010
added a commit
to ropatil010/cluster-authentication-operator
that referenced
this pull request
May 28, 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>
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
a96c6eb to
d64f805
Compare
Contributor
|
[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 |
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
73eb701 to
ccfe637
Compare
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
test/e2e-oidc/external_oidc.go (3)
75-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the suite on
FeatureGateExternalOIDConly.
CheckFeatureGatesOrSkipis OR-based, so Line 75 still lets the whole suite run when onlyFeatureGateExternalOIDCWithAdditionalClaimMappingsis enabled. Most of this test depends on the base ExternalOIDC path, so the additional-claim gate should stay local to the CEL-specific cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-oidc/external_oidc.go` at line 75, Change the suite-level gate to require only FeatureGateExternalOIDC by calling CheckFeatureGatesOrSkip with FeatureGateExternalOIDC alone (remove FeatureGateExternalOIDWithAdditionalClaimMappings from the top-level call to CheckFeatureGatesOrSkip in external_oidc.go), and instead perform a local CheckFeatureGatesOrSkip(…, FeatureGateExternalOIDCWithAdditionalClaimMappings) inside the CEL-specific test cases (the CEL handler/tests) so the additional-claim gate only gates those CEL-specific paths; reference symbols: CheckFeatureGatesOrSkip, FeatureGateExternalOIDC, FeatureGateExternalOIDCWithAdditionalClaimMappings.
111-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSkip the KAS rollout wait when rollback made no auth change.
authResourceRollbackat Lines 1070-1088 is a no-op when the current spec already matchesorigAuthSpec, but cleanup still always waits for a new kube-apiserver revision on Line 117. That turns early-failure cleanup into a long timeout with no rollout in flight.🤖 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-oidc/external_oidc.go` around lines 111 - 118, The cleanup always waits for a KAS rollout even when authResourceRollback did nothing; change the flow so you detect whether rollback actually modified the auth spec and only wait for a new kube-apiserver revision if it did. Concretely, update authResourceRollback to indicate whether it performed a change (e.g., return (changed bool, err error) or provide a helper that compares the current spec to origAuthSpec) and in the cleanup use that boolean to decide whether to call testClient.kasLatestAvailableRevision and test.WaitForNewKASRollout; if changed is false, skip the revision read and the WaitForNewKASRollout call.
556-570:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the shared informers during teardown.
newTestClientstartsdynamicInformerswith the uncancelled test context, and the caller currently passescontext.Background()at Line 71. Those watch goroutines stay alive until process exit, so repeated specs accumulate background informers and cache state. As per coding guidelines,**/*.go: context.Context for cancellation and timeouts.🤖 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-oidc/external_oidc.go` around lines 556 - 570, The dynamic informer factory is started with an uncancelled context so its goroutines never stop; change newTestClient to create a cancelable context (ctx, cancel := context.WithCancel(parentCtx)) and store the cancel on the returned test client (e.g., tc.cancelInformers), call dynamicInformers.Start(ctx.Done()) and dynamicInformers.WaitForCacheSync(ctx.Done()) as you already do, and ensure teardown calls tc.cancelInformers() (or implement tc.Close that calls cancel) so the informers are stopped during test teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 34: The klog version bump to k8s.io/klog/v2 v2.140.0 should be justified
because the API you need (klog.SetOutput(io.Writer)) already exists in v2.130.1
and OSV reports no vulnerabilities; either revert the dependency back to
v2.130.1 (or the previous version) if no behavioral changelog is required, or
keep v2.140.0 but add a clear changelog/PR note explaining the specific
behavior/fix in k8s.io/klog/v2 that you rely on (mentioning klog.SetOutput and
any relevant klog behavior changes) so reviewers understand why the bump is
necessary.
In `@test/library/encryption/perf_helpers.go`:
- Around line 114-115: The call to runWorkFunctions currently discards its
returned error; change the worker to capture the returned err (e.g., err :=
runWorkFunctions(kubeClient, "", r.collectError, r.collectStat, workFunc...))
and handle it instead of ignoring it — propagate it, log it via the existing
errorCollector or processLogger, and/or return it from the surrounding function
so failures are not lost; update any surrounding control flow to check err and
act accordingly (using the same error handling approach as
r.collectError/r.errorStore).
In `@test/library/encryption/scenarios.go`:
- Around line 333-343: The calls to libClientSet.ApiServerConfig.Get and Update
use context.TODO() and can block inside retry.RetryOnConflict; replace those
with a cancellable timeout context: create ctx, cancel :=
context.WithTimeout(context.Background(), <reasonable duration e.g. 30s>) before
the retry loop (or before the Get/Update pair), defer cancel(), and pass ctx to
libClientSet.ApiServerConfig.Get and Update (and any retry.RetryOnConflict
wrapper) so the apiServer retrieval/update (apiServer variable) will time out
instead of stalling indefinitely.
---
Duplicate comments:
In `@test/e2e-oidc/external_oidc.go`:
- Line 75: Change the suite-level gate to require only FeatureGateExternalOIDC
by calling CheckFeatureGatesOrSkip with FeatureGateExternalOIDC alone (remove
FeatureGateExternalOIDWithAdditionalClaimMappings from the top-level call to
CheckFeatureGatesOrSkip in external_oidc.go), and instead perform a local
CheckFeatureGatesOrSkip(…, FeatureGateExternalOIDCWithAdditionalClaimMappings)
inside the CEL-specific test cases (the CEL handler/tests) so the
additional-claim gate only gates those CEL-specific paths; reference symbols:
CheckFeatureGatesOrSkip, FeatureGateExternalOIDC,
FeatureGateExternalOIDCWithAdditionalClaimMappings.
- Around line 111-118: The cleanup always waits for a KAS rollout even when
authResourceRollback did nothing; change the flow so you detect whether rollback
actually modified the auth spec and only wait for a new kube-apiserver revision
if it did. Concretely, update authResourceRollback to indicate whether it
performed a change (e.g., return (changed bool, err error) or provide a helper
that compares the current spec to origAuthSpec) and in the cleanup use that
boolean to decide whether to call testClient.kasLatestAvailableRevision and
test.WaitForNewKASRollout; if changed is false, skip the revision read and the
WaitForNewKASRollout call.
- Around line 556-570: The dynamic informer factory is started with an
uncancelled context so its goroutines never stop; change newTestClient to create
a cancelable context (ctx, cancel := context.WithCancel(parentCtx)) and store
the cancel on the returned test client (e.g., tc.cancelInformers), call
dynamicInformers.Start(ctx.Done()) and
dynamicInformers.WaitForCacheSync(ctx.Done()) as you already do, and ensure
teardown calls tc.cancelInformers() (or implement tc.Close that calls cancel) so
the informers are stopped during test teardown.
🪄 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: f879b4eb-e2fa-4fb1-baab-ec1f6b023a58
⛔ 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 (21)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.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-kms/main_test.go
- test/e2e-encryption/main_test.go
…nsion framework This PR migrates five e2e test suites to Ginkgo v2 using the OpenShift Tests Extension (OTE) framework: - e2e-encryption - e2e-encryption-kms - e2e-encryption-perf - e2e-encryption-rotation - e2e-oidc Key Changes: - Migrate all encryption test suites to use Ginkgo v2 with OTE framework - Add test suite qualifiers and proper serial execution for tests that modify cluster-wide encryption configuration - Create shared test helper functions that accept testing.TB for compatibility with both standard Go tests and Ginkgo tests - Extract test logic from *_test.go files into separate files to enable reuse between standard tests and Ginkgo tests - Add feature gate checks for KMS encryption tests - Fix klog output corruption in test listing by suppressing klog during init phase - Update library-go dependencies to use EncryptionProvider struct type - Add proper timeout handling and conflict retry logic - Improve migration timestamp validation to ensure fresh condition transitions Test Organization: - operator-encryption/serial: Basic encryption tests - operator-encryption-rotation/serial: Key rotation tests - operator-encryption-perf/serial: Performance tests - operator-encryption-kms/serial: KMS encryption tests Technical Details: - Updated code to work with library-go's EncryptionProvider struct type instead of configv1.EncryptionType string type - All test scenarios now use library.EncryptionProvider with proper APIServerEncryption wrapper - Fixed format string arguments in logging statements - Improved test robustness based on code review feedback Dependencies: - Updated klog to v2.140.0 - Updated to standard OTE version Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
force-pushed
the
migrate-e2e-encryption-final
branch
from
ccfe637 to
ca23a0b
Compare
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/library/waits.go (1)
86-101:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
WaitForClusterOperatorStatusStablecurrently returns success when status becomes unstable.When
CheckClusterOperatorStatusreturnsdone == falseandcheckErr == nil, Line 91 returns(true, nil), which makes polling exit successfully and the function returnnil.✅ Minimal fix
err := wait.PollUntilContextTimeout(ctx, 10*time.Second, 10*time.Minute, true, func(_ context.Context) (bool, error) { done, conditions, checkErr := CheckClusterOperatorStatus(t, ctx, client, name, requiredConditions...) if len(conditions) > 0 { endConditions = conditions } - return !done, checkErr + if checkErr != nil { + return false, checkErr + } + if !done { + return false, fmt.Errorf("clusteroperators.config.openshift.io/%s status diverged", name) + } + return false, nil })🤖 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/waits.go` around lines 86 - 101, The polling callback currently negates the CheckClusterOperatorStatus result causing the poll to stop when the operator is unstable; update the closure in WaitForClusterOperatorStatusStable to return the actual done value from CheckClusterOperatorStatus (i.e., return done, checkErr) instead of negating it, keeping the existing logic that updates endConditions when conditions are present so the final logs remain correct.
♻️ Duplicate comments (2)
test/e2e-oidc/external_oidc.go (2)
569-570:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInformer lifecycle is not cancelable from test teardown.
dynamicInformers.Start(ctx.Done())/WaitForCacheSync(ctx.Done())usectxthat originates fromcontext.Background()in this test path, so informer goroutines can outlive the test. Use a cancelable context owned bytestClientand cancel it in cleanup.As per coding guidelines, "
**/*.go: ... context.Context for cancellation and timeouts".🤖 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-oidc/external_oidc.go` around lines 569 - 570, The informer Start/WaitForCacheSync calls are using a non-cancelable ctx (the ctx from context.Background()), so informer goroutines may outlive the test; change the code to use a cancelable context owned by the test client (e.g. create ctx, cancel := context.WithCancel(testClient.Context() or a test-owned context) and pass that ctx to dynamicInformers.Start and dynamicInformers.WaitForCacheSync), then register cancel() in the test cleanup so the informers are stopped when the test finishes; update references to dynamicInformers.Start(ctx.Done()) and dynamicInformers.WaitForCacheSync(ctx.Done()) to use the cancelable ctx.Done() instead.
111-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCleanup may hang waiting for a rollout that never starts.
authResourceRollbackcan be a no-op, but cleanup always waits for a new KAS rollout at Line 117. Gate the rollout wait on an actual persisted auth spec change to avoid long cleanup timeouts.🤖 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-oidc/external_oidc.go` around lines 111 - 118, The cleanup always waits for WaitForNewKASRollout even when authResourceRollback does nothing; modify the cleanup to only wait for a KAS rollout when the persisted auth spec actually changed. After calling testClient.authResourceRollback(testCtx, origAuthSpec) (or by inspecting the current persisted spec via the client), compare the resulting persisted auth spec to origAuthSpec (or have authResourceRollback return a boolean/changed flag). Only call test.WaitForNewKASRollout(t, testCtx, testClient.operatorConfigClient.OperatorV1().KubeAPIServers(), kasOriginalRevision) when a real persisted change occurred; otherwise skip the rollout wait to avoid hanging.
🤖 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 36-37: Replace the unbounded context created with context.TODO()
by creating a cancellable, time-bounded context (e.g. using context.WithTimeout)
and ensure the cancel function is registered with t.Cleanup(cancel); pass the
new ctx into getPerfClients and any downstream calls/DB loader wrappers so
clientSet and subsequent operations use the bounded context to avoid indefinite
hangs on stalled API operations.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Line 31: Replace the unbounded context created with context.TODO() by creating
a cancellable, timed context: use context.WithTimeout(context.Background(),
<reasonableDuration>) to create ctx and obtain cancel(), defer cancel()
immediately after creation, and pass that ctx to all cluster-scoped operations
in encryption_rotation.go so the test will time out and stop retries instead of
hanging indefinitely; ensure you choose an appropriate timeout constant (e.g.,
testTimeout or 2*time.Minute) and update any uses of the previous ctx variable
accordingly.
In `@test/e2e-encryption/encryption.go`:
- Line 66: Use a bounded context instead of context.TODO() when calling
operatorencryption.CreateAndStoreTokenOfLife; create a context with timeout
(e.g., via context.WithTimeout(context.Background(), <reasonable duration>)),
defer the cancel() and pass that ctx into CreateAndStoreTokenOfLife along with
operatorencryption.GetClients(t), and add the time import if needed to ensure
the test callback can be cancelled and won't block indefinitely.
In `@test/e2e-oidc/external_oidc.go`:
- Around line 388-391: The TLSClientConfig in the OIDC client setup currently
sets InsecureSkipVerify: true (in the TLSClientConfig block) which disables
certificate validation; instead load and use a trusted CA pool (e.g., create an
x509.CertPool, AppendCertsFromPEM with the IdP/Keycloak CA bundle) and assign it
to TLSClientConfig.RootCAs so verification remains enabled; update the code that
constructs TLSClientConfig (the struct literal used for JWKS/OpenID fetches in
external_oidc.go) to remove InsecureSkipVerify and set RootCAs to the cert pool
sourced from test-provided CA data or environment so the e2e test trusts the IdP
without skipping verification.
In `@test/library/client.go`:
- Line 39: Remove or redact the cluster API hostname logged by the test: replace
or delete the t.Logf call that prints config.Host in test/library/client.go (the
t.Logf("Found configuration for host %v", config.Host) invocation) so test
output no longer exposes internal hostnames; if preserving a hint is required,
log a non-identifying token or masked value (e.g., "host redacted" or mask
config.Host) instead of the raw hostname.
In `@test/library/encryption/helpers.go`:
- Line 56: Remove or redact the kube API host from the test log: replace the
t.Logf call that prints config.Host so it does not output config.Host (either
remove the log entirely or log a redacted placeholder like "<REDACTED_HOST>" or
only log that a host was found). Locate the usage of t.Logf(...) referencing
config.Host in test/library/encryption/helpers.go (the t.Logf call that mentions
config.Host) and update it to avoid exposing config.Host in logs.
In `@test/library/encryption/perf_helpers.go`:
- Around line 37-38: The watcher currently calls context.Background() inside
watchForMigrationControllerProgressingCondition which allows its
PollUntilContextTimeout goroutine to outlive tests; change
watchForMigrationControllerProgressingCondition to accept a caller-provided
context (e.g., add a ctx context.Context parameter) and use that context in the
wait.PollUntilContextTimeout call instead of context.Background(); update all
callers to pass in their test-scoped cancelable context (created with
context.WithCancel or t.Cleanup) and ensure they cancel/cleanup on test teardown
so polling stops promptly.
In `@test/library/keycloakidp.go`:
- Around line 393-435: Update the Keycloak client APIs to accept and propagate a
request-scoped context: add a context.Context parameter to ListGroups,
DeleteGroup, DeleteUser, and ListUsers and pass that context into kc.do; update
kc.do to create requests with http.NewRequestWithContext instead of
http.NewRequest so the context is honored; ensure callers (and KeycloakClientFor
if it constructs the client) are updated accordingly; also set a sensible
Timeout on the http.Client constructed in KeycloakClientFor to avoid indefinite
hangs during pagination/cleanup loops.
---
Outside diff comments:
In `@test/library/waits.go`:
- Around line 86-101: The polling callback currently negates the
CheckClusterOperatorStatus result causing the poll to stop when the operator is
unstable; update the closure in WaitForClusterOperatorStatusStable to return the
actual done value from CheckClusterOperatorStatus (i.e., return done, checkErr)
instead of negating it, keeping the existing logic that updates endConditions
when conditions are present so the final logs remain correct.
---
Duplicate comments:
In `@test/e2e-oidc/external_oidc.go`:
- Around line 569-570: The informer Start/WaitForCacheSync calls are using a
non-cancelable ctx (the ctx from context.Background()), so informer goroutines
may outlive the test; change the code to use a cancelable context owned by the
test client (e.g. create ctx, cancel := context.WithCancel(testClient.Context()
or a test-owned context) and pass that ctx to dynamicInformers.Start and
dynamicInformers.WaitForCacheSync), then register cancel() in the test cleanup
so the informers are stopped when the test finishes; update references to
dynamicInformers.Start(ctx.Done()) and
dynamicInformers.WaitForCacheSync(ctx.Done()) to use the cancelable ctx.Done()
instead.
- Around line 111-118: The cleanup always waits for WaitForNewKASRollout even
when authResourceRollback does nothing; modify the cleanup to only wait for a
KAS rollout when the persisted auth spec actually changed. After calling
testClient.authResourceRollback(testCtx, origAuthSpec) (or by inspecting the
current persisted spec via the client), compare the resulting persisted auth
spec to origAuthSpec (or have authResourceRollback return a boolean/changed
flag). Only call test.WaitForNewKASRollout(t, testCtx,
testClient.operatorConfigClient.OperatorV1().KubeAPIServers(),
kasOriginalRevision) when a real persisted change occurred; otherwise skip the
rollout wait to avoid hanging.
🪄 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: c84d816d-879d-4a14-bfa0-0e7ba1213b7e
⛔ 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 (21)
cmd/cluster-authentication-operator-tests-ext/main.gogo.modpkg/controllers/readiness/wellknown_ready_controller.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
ropatil010
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Tip For best results, initiate chat on the files or code changes.
In short: the core review findings are consistent (as seen in the detailed suggestions already provided), but the exact set of items surfaced in any given run may vary due to LLM non-determinism. The open review comments remain the authoritative list of items to address. |
This commit addresses multiple security vulnerabilities and improves context management across the test codebase following code review feedback. Security Fixes: - Fixed TLS certificate validation bypass in OIDC tests (CRITICAL) * Removed InsecureSkipVerify=true from fetchIssuerJWKS * Added proper x509.CertPool with trusted CA certificates * Prevents MITM attacks on JWKS/OpenID endpoint fetches - Removed internal hostname disclosure from test logs (MEDIUM) * Redacted config.Host from log output in client.go and helpers.go * Prevents leaking internal Kubernetes API hostnames Context Management Improvements: - Replaced context.TODO() with proper time-bounded contexts (7 locations) * e2e-encryption-perf: 30-minute timeout for performance tests * e2e-encryption-rotation: 3-hour timeout matching test annotation * e2e-encryption: 2-minute timeout for token creation - Fixed goroutine lifecycle management in performance helpers * Watcher goroutines now stop promptly with test completion * Previously could run for up to 79 minutes after test finished * Added cancelable context with t.Cleanup(cancel) Keycloak Client Refactor (18 methods): - Updated all KeycloakClient methods to accept context.Context - Modified do() to use http.NewRequestWithContext - All HTTP requests now support cancellation and timeout - Prevents indefinite hangs on network stalls during pagination/cleanup - Updated all callers in test files to pass appropriate context Methods updated: CreateClientGroupMapper, CreateGroup, ListGroups, DeleteGroup, CreateUser, DeleteUser, ListUsers, UpdateUser, GetUser, ListUserGroups, DeleteUserFromGroups, UpdateClientAccessTokenTimeout, UpdateClientDirectAccessGrantsEnabled, UpdateClient, GetClient, GetClientByClientID, ListClients, RegenerateClientSecret Test Coverage: - All unit tests pass (44 packages) - No race conditions detected - All test packages compile successfully - Binaries build cleanly Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Contributor
Author
|
/test e2e-agnostic-ipv6 |
Contributor
Author
|
/test e2e-aws-operator-encryption-serial-ote-1of2 |
- Restore test/e2e-encryption/main_test.go and test/e2e-encryption-kms/main_test.go to support both standard Go tests and Ginkgo/OTE execution during transition - Add explanatory comments to test wrapper functions referencing the new OTE jobs: * e2e-aws-operator-encryption-serial-ote * e2e-aws-operator-encryption-perf-serial-ote * e2e-aws-operator-encryption-rotation-serial-ote - Follow the same transitional pattern used in e2e-encryption-kms tests - main_test.go files will be removed once OTE jobs are verified stable
Contributor
Author
|
/test e2e-aws-operator-encryption-serial-ote-1of2 |
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
|
Hi @liouk PTAL on the PR. Failures profiles are wrt Monitor cases Currently added for these jobs: Wrt OIDC cases PASS in e2e-aws-operator-serial-ote |
ropatil010
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
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 adds security and context management improvements to the recently migrated e2e test suites, addressing code review feedback.
Changes from PR #839:
Migrate e2e-encryption, e2e-encryption-kms, e2e-encryption-perf, e2e-encryption-rotation, and e2e-oidc test suites to Ginkgo v2
Add testing.TB compatibility layer for library-go integration
Create new test implementation files with Ginkgo v2 specs
Configure appropriate test suite parallelism and qualifiers
Remove obsolete main_test.go files
Security Fixes
Context Management Improvements