CNTRLPLANE-2589: Migrate test/e2e-encryption to Ginkgo v2 framework#839
Open
ropatil010 wants to merge 7 commits intoopenshift:masterfrom
Open
CNTRLPLANE-2589: Migrate test/e2e-encryption to Ginkgo v2 framework#839ropatil010 wants to merge 7 commits intoopenshift:masterfrom
ropatil010 wants to merge 7 commits intoopenshift:masterfrom
Conversation
|
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 multiple new end-to-end test suites and helpers for the cluster-authentication-operator: encryption (core, perf, rotation), external OIDC, test-library TB adapters, and multiple test registry suites; refactors tests to delegate to TB wrappers and removes a custom test-order randomizer. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ropatil010 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e-encryption/encryption.go`:
- Around line 30-40: The type assertion t.(*testing.T) in
testEncryptionTypeIdentity is unsafe for Ginkgo; change the API instead: update
the library function signature of library.TestEncryptionTypeIdentity to accept
testing.TB (not *testing.T) and adapt its implementation accordingly, then call
it directly as library.TestEncryptionTypeIdentity(t, library.BasicScenario{...})
from testEncryptionTypeIdentity; ensure all other callers of
TestEncryptionTypeIdentity are updated to pass testing.TB.
Contributor
Author
|
/retitle CNTRLPLANE-2589: Migrate test/e2e-encryption to Ginkgo v2 framework |
openshift-ci
bot
changed the title
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. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@test/e2e-encryption-perf/encryption_perf.go`:
- Around line 29-79: The test currently casts tt.(*testing.T) in
testPerfEncryptionTypeAESCBC which panics under Ginkgo's *GinkgoTBWrapper;
update the call to library.TestPerfEncryption to pass tt as a testing.TB (i.e.,
call library.TestPerfEncryption(tt, ...)) and change the library-go helper
signature from func TestPerfEncryption(t *testing.T, ...) to func
TestPerfEncryption(t testing.TB, ...), removing any internal *testing.T type
assertions; update any other callers of TestPerfEncryption to accept testing.TB
accordingly.
In `@test/e2e-encryption-rotation/encryption_rotation.go`:
- Line 31: Change the TestEncryptionRotation API to accept the testing.TB
interface instead of forcing a *testing.T cast: update the library function
signature of TestEncryptionRotation to take parameter type testing.TB (e.g.,
func TestEncryptionRotation(t testing.TB, ...)) and adjust its internal uses to
only call methods available on testing.TB (or accept a helper wrapper when
*testing.T-specific behavior is required); then remove the unsafe cast in
encryption_rotation.go and pass t (from g.GinkgoTB() or normal *testing.T)
directly to library.TestEncryptionRotation. Ensure any call sites and helper
functions (TestEncryptionRotation and callers) are updated to compile against
testing.TB.
In `@test/e2e-oidc/external_oidc.go`:
- Around line 311-320: The poll callback is checking the wrong error variable
and the require messages use the stale `err`; update the anonymous func passed
to wait.PollUntilContextTimeout to check `if cmErr != nil { return false, nil }`
after calling testClient.kubeClient.CoreV1().ConfigMaps(...).Get(...), and
update the require.NoError assertions to reference `cmErr` for the Get error and
`waitErr` for the wait error (use their respective variables in the format
strings). This affects the function using wait.PollUntilContextTimeout, the
local variables `cmPtr`/`cmErr`, and the require.NoError calls that currently
interpolate `err`.
- Line 94: Change the WaitForNewKASRollout and
WaitForClusterOperatorStatusAlwaysAvailable functions in test/library/waits.go
to accept testing.TB instead of *testing.T (update their signatures and any
internal param names), and update all callers (e.g., in
test/e2e-oidc/external_oidc.go) to pass t directly without type-asserting
t.(*testing.T); remove the casts at the calls that reference
WaitForNewKASRollout and WaitForClusterOperatorStatusAlwaysAvailable and ensure
any other callers across the repo are updated to use testing.TB-compatible
calls.
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc.go (1)
463-467: Minor: Unreachable return aftert.Fatalf.The
returnstatement on line 467 is unreachable sincet.Fatalfwill terminate the test. This is harmless but could be cleaned up.♻️ Optional cleanup
if len(featureGates.Status.FeatureGates) != 1 { // fail test if there are multiple feature gate versions (i.e. ongoing upgrade) t.Fatalf("multiple feature gate versions detected") - return }
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
from
da9db09 to
c1df3a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/e2e-oidc/external_oidc.go`:
- Around line 1073-1087: In authResourceRollback, avoid nil dereference by
capturing the authentication name (e.g., authName := "cluster" or a variable
holding the resource name used) before calling
tc.configClient.ConfigV1().Authentications().Get, then use that authName in both
error messages instead of auth.Name; after a successful Get you can still use
the returned auth for updates, but do not reference auth when the Get returned
an error.
- Around line 379-426: In fetchIssuerJWKS, set a finite timeout on the
http.Client and enforce a TLS minimum version to avoid hung tests and weak TLS;
update the client construction (in fetchIssuerJWKS) to include e.g. Timeout:
10*time.Second and in the Transport's TLSClientConfig set MinVersion:
tls.VersionTLS12 (keep InsecureSkipVerify as before if required), so the HTTP
calls time out and TLS is at least 1.2.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e-oidc/external_oidc.go`:
- Around line 63-66: Update the Ginkgo test label for the test defined in the
g.Describe block so it runs serially: change the test declaration
g.It("[OIDC][Parallel] TestExternalOIDCWithKeycloak", ...) to use "[Serial]"
instead of "[Parallel]" (i.e., g.It("[OIDC][Serial]
TestExternalOIDCWithKeycloak", func() {
testExternalOIDCWithKeycloak(g.GinkgoTB()) })). Ensure only the label text is
changed and the call to testExternalOIDCWithKeycloak remains unchanged.
c8214a4 to
9f9321b
Compare
liouk
suggested changes
Feb 11, 2026
Contributor
Author
|
/test e2e-aws-operator-encryption-serial-ote-1of2 |
Contributor
|
/pipeline required |
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
from
e4e1822 to
69247d0
Compare
liouk
reviewed
Feb 24, 2026
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
2 times, most recently
from
e511745 to
4ce4710
Compare
Contributor
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Contributor
Author
|
/test e2e-aws-operator-serial-ote |
Contributor
Author
|
/test e2e-operator |
Contributor
Author
|
/test e2e-aws-operator-serial-ote |
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
from
e855094 to
c11012c
Compare
This commit migrates encryption-related and OIDC e2e test suites to the
Ginkgo v2 framework and fixes critical OIDC test failures observed in CI.
## Ginkgo v2 Migration
### Encryption Tests
- Migrate encryption, encryption-rotation, encryption-perf e2e tests to
Ginkgo v2 framework with updated test initialization syntax
- Extract test logic into separate files (encryption.go, encryption_rotation.go,
encryption_perf.go) for better organization and reusability
- Implement encryption test scenarios with testing.TB support for flexible
test execution
- Refactor encryption wrappers to use local implementations
- Add timeout tags to encryption e2e tests for better test management
- Remove obsolete main_test.go files
### OIDC Tests
- Migrate OIDC e2e tests to Ginkgo v2 framework
- Extract TestExternalOIDCWithKeycloak implementation to external_oidc.go
- Convert OIDC e2e tests to Serial and Disruptive execution model
### Test Infrastructure
- Configure test suites in the OTE test registry with appropriate parallelism
and timeout settings for encryption tests
- Add helper modules in test/library/encryption/ for test scenarios and
performance helpers
- Remove unused oidc/serial-disruptive suite that was never invoked by any
Prow job (OIDC tests run via operator/serial suite or direct go test)
- Remove unused time package import from main.go
## OIDC Test Failure Fixes
### Root Cause
TestExternalOIDCWithKeycloak was timing out after 1 hour while executing
subtests that each wait for authentication and kube-apiserver operators to
remain stable for 10 continuous minutes. In CI environments with resource
contention, the test legitimately takes ~3 hours to complete all subtests.
When it timed out, cleanup didn't complete properly, leaving the cluster
in a degraded state. Subsequent OIDC tests (TestGitLabAsOIDCPasswordGrantCheck,
TestKeycloakAsOIDCPasswordGrantCheckAndGroupSync) inherited this degraded
cluster and failed with 'authentication operator never became progressing' errors.
### Fixes Applied
1. **Increase TestExternalOIDCWithKeycloak timeout from 1h to 3h**
Based on e2e-oidc Prow logs (build 2027354543884341248), the test
completes successfully in approximately 3 hours when given enough time.
The test has multiple subtests, each waiting 10 minutes for operator
stability, which is legitimate test behavior for disruptive tests.
2. **Add cluster recovery step in test cleanup**
After cleanup runs (whether test passes, fails, or times out), we now
wait for both authentication and kube-apiserver operators to become
Available/NotProgressing/NotDegraded before exiting. This ensures:
- Subsequent tests start on a healthy cluster
- No cascade of failures from inherited degraded state
- Uses WaitForClusterOperatorAvailableNotProgressingNotDegraded (max 10min)
instead of the stricter WaitForClusterOperatorStatusAlwaysAvailable
- Logs warnings but doesn't fail if recovery times out
### Expected Impact
- TestExternalOIDCWithKeycloak will complete successfully in CI
- Even if it times out, cluster will recover before next test
- TestGitLabAsOIDCPasswordGrantCheck will pass on clean cluster
- TestKeycloakAsOIDCPasswordGrantCheckAndGroupSync will pass on clean cluster
## Testing
- All encryption tests verified with refactored structure
- All three OIDC tests pass on live cluster (4.22.0-nightly)
- Cluster recovery validated on timeout scenarios
- Unit tests pass: make test-unit
- Build successful: make build
- Verification passed: make verify
Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
from
bb4951c to
ac1c999
Compare
Contributor
Author
|
CI Jobs results wrt cluster auth operator: Hi @liouk, can you plz help to review this PR. |
liouk
suggested changes
Mar 3, 2026
| }) | ||
|
|
||
| func testExternalOIDCWithKeycloak(t testing.TB) { | ||
| testCtx := context.Background() |
Member
There was a problem hiding this comment.
Can we use t.Context() instead?
Suggested change
| testCtx := context.Background() | |
| testCtx := t.Context() |
Contributor
Author
There was a problem hiding this comment.
Tried with the suggestion but hit error as mentioned in my 3rd commit
GinkgoTB() doesn't implement Context() method (added in Go 1.21). Calling t.Context() returns nil, causing a nil pointer dereference. Replace with context.Background() to match Ginkgo v2 patterns.
Issue: - "runtime error: invalid memory address or nil pointer dereference"
| } | ||
|
|
||
| // Test: auth-config cm must not exist and gets deleted by the CAO if manually created when type not OIDC | ||
| t.Logf("auth-config cm must not exist and gets deleted by the CAO if manually created when type not OIDC") |
Member
There was a problem hiding this comment.
I've noticed that we're now using t.Logf() for sub-tests; this means individual sub-test failures won't be reported separately in Ginkgo - the entire test is a single g.It(). Is there a way to preserve the sub-tests as separate, while still being compatible with Ginkgo?
Contributor
Author
There was a problem hiding this comment.
- The shared Keycloak setup is too expensive to duplicate
- Some tests explicitly depend on previous test state (Test 5 needs Test 4), Would need complex BeforeEach logic to set up different states
- Cluster rollout times make per-test cleanup prohibitively slow
Contributor
Author
There was a problem hiding this comment.
Tried with the suggestions but hit error as mentioned in 3rd commit.
g.By() can only be called within a Ginkgo spec context (inside g.It(), BeforeEach(), etc.). When test logic is extracted to a separate function called via g.GinkgoTB(), the Ginkgo context is lost and g.By() fails with "calling By outside of a running spec". Replace all g.By() calls with t.Logf() for proper logging.
Issue: "Ginkgo detected an issue with your spec structure"
- Use t.Context() instead of context.Background() for better test lifecycle integration - Replace t.Logf() with g.By() for improved test step visibility in Ginkgo reports - Remove unused oidcAuthResponse struct These changes address PR review feedback to improve test context management and sub-test reporting while maintaining the current test structure. Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Contributor
Author
|
/test e2e-oidc-techpreview |
Contributor
Author
|
/test e2e-console-login |
Fixes two critical bugs introduced during the Ginkgo v2 migration: 1. GinkgoTB() doesn't implement Context() method (added in Go 1.21). Calling t.Context() returns nil, causing a nil pointer dereference. Replace with context.Background() to match Ginkgo v2 patterns. 2. g.By() can only be called within a Ginkgo spec context (inside g.It(), BeforeEach(), etc.). When test logic is extracted to a separate function called via g.GinkgoTB(), the Ginkgo context is lost and g.By() fails with "calling By outside of a running spec". Replace all g.By() calls with t.Logf() for proper logging. These issues caused CI failures with: - "runtime error: invalid memory address or nil pointer dereference" - "Ginkgo detected an issue with your spec structure" Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Completes the Ginkgo v2 migration by converting the KMS encryption tests to use Ginkgo specs while maintaining backward compatibility with traditional go test execution. ## Changes ### Test Structure Migration - Create encryption_kms.go with Ginkgo v2 specs using g.Describe/g.It - Convert encryption_kms_test.go to thin wrapper functions for go test - Remove main_test.go (test randomization now handled by Ginkgo) - Add [Encryption][Serial] tags for proper suite classification - Add [Timeout:2h] tags for long-running KMS tests ### Testing.TB Support - Add TestEncryptionProvidersMigration() to test/library/encryption/scenarios.go that accepts testing.TB instead of *testing.T for Ginkgo compatibility - Add wrapper function in test/library/encryption_wrappers.go - Fix go vet warnings by using proper format strings in tb.Logf calls ### OTE Integration - Enable e2e-encryption-kms import in cmd/cluster-authentication-operator-tests-ext/main.go - Tests are now discoverable by OpenShift Tests Extension framework - Will run in operator-encryption-kms/serial suite with parallelism=1 ## Dual Execution Support The tests now work with both execution methods: 1. **Traditional go test**: `make test-e2e-encryption-kms` - Uses wrapper functions in encryption_kms_test.go - Maintains backward compatibility 2. **Ginkgo/OTE**: `./cluster-authentication-operator-tests-ext` - Uses specs in encryption_kms.go - Integrated with OpenShift CI framework ## Verification - ✅ Both test methods compile and discover tests correctly - ✅ All unit tests pass (make test-unit) - ✅ Code formatting verified (make verify-gofmt) - ✅ Static analysis clean (make verify-govet) - ✅ Binaries build successfully (make build) - ✅ OTE discovers both KMS tests with correct tags Co-Authored-By: Rohit Patil <ropatil@redhat.com>
The e2e-encryption-kms tests were failing with "Unknown encryption type: KMS" because the Ginkgo v2 migration in commit f3eb497 created local implementations of encryption test functions but forgot to add KMS encryption type support. ## Issue When migrating to Ginkgo v2, local implementations of encryption test functions were created in test/library/encryption/scenarios.go to support testing.TB (needed for Ginkgo's GinkgoTB()). However, while the upstream library-go has TestEncryptionTypeKMS, the local implementation was missing: - TestEncryptionTypeKMS() function - Case for configv1.EncryptionTypeKMS in TestEncryptionType() switch This caused tests to fail with: scenarios.go:75: Unknown encryption type: KMS ## Fix Add TestEncryptionTypeKMS() function and KMS case to the switch statement, following the same pattern as AESCBC and AESGCM implementations. ## Testing - All unit tests pass (make test-unit) - All verification checks pass (make verify) - Binaries build successfully (make build) - e2e-encryption-kms tests compile successfully - Verified the fix allows tests to progress beyond the encryption type check Tests now fail at cluster validation level (expected - requires KMSEncryption feature gate enabled on the test cluster). Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Extract duplicate checkFeatureGatesOrSkip function to shared library, add proper feature gate checking before conditional skips in KMS tests, and fix race condition in OIDC authResourceRollback using retry logic. Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Remove unused imports that were leftover from extracting checkFeatureGatesOrSkip to shared library. Co-Authored-By: Rohit Patil <ropatil@redhat.com>
Contributor
Author
|
/test e2e-oidc |
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. |
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,
Changes:
File Structure (matching test/e2e pattern):
Test Implementation:
OTE Integration:
Test Configuration (from Makefile line 59-66):
Tests Migrated:
Verified:
Need to remove the fail cmd/**/main.go file once hte PR: #833 gets merged.