CNTRLPLANE-2589: Migrate test/e2e-encryption to Ginkgo v2 framework #839
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 Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
|
[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.
|
/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
|
@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.
This commit migrates the e2e test suites for encryption, encryption-rotation, encryption-perf, and OIDC tests from Ginkgo v1 to v2 framework. The migration includes: - Update test suite initialization to use Ginkgo v2 syntax - Replace deprecated test main functions with proper suite setup - Extract test logic into separate package files for better organization - Fix type assertions and error handling in e2e tests - Add HTTP client safety measures and nil pointer checks - Fix gofmt formatting issues - Update test suite definitions to match CI job configuration - Add encryption wrapper utilities for common test operations Co-Authored-By: Rohit Patil <ropatil@redhat.com>
ropatil010
force-pushed
the
migrate-e2e-encryption
branch
from
c8214a4 to
9f9321b
Compare
|
/retest |
test/e2e-oidc/external_oidc.go
Outdated
| ) | ||
|
|
||
| var _ = g.Describe("[sig-auth] authentication operator", func() { | ||
| g.It("[OIDC][Parallel] TestExternalOIDCWithKeycloak", func() { |
There was a problem hiding this comment.
The external OIDC tests shouldn't be run in parallel; they're Serial and also Disruptive, as they're rolling out new kube-apiserver pods. See also: https://github.com/openshift/origin/blob/main/test/extended/authentication/oidc.go#L41
Given the OTE integration, we're planning on merging the e2e tests that currently exist in this repo with the ones in origin, as they have a large overlap.
The TestExternalOIDCWithKeycloak test modifies cluster-wide authentication configuration, which can disrupt cluster operations and interfere with other tests running in parallel. This change: - Updates test tag from [Parallel] to [Serial][Disruptive] - Adds new oidc/serial-disruptive suite with ClusterStability: Disruptive - Sets 120-minute timeout for disruptive operations Co-Authored-By: Rohit Patil <ropatil@redhat.com>
|
@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. |
3 participants
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.