NO-JIRA: Update OIDC e2e test to expect admission-time validation error of invalid CEL expression #797
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@liouk: This pull request explicitly references no jira issue. In 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. |
WalkthroughChange updates test helper signature to return (*configv1.Authentication, error) and propagates explicit error handling to all call sites in the external OIDC tests; also replaces context initialization in TestExternalOIDCWithKeycloak to use t.Context() and adjusts test flows to use the returned auth objects. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
approved
![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
🧹 Nitpick comments (2)
test/e2e-oidc/external_oidc_test.go (2)
189-191: Strengthen admission error assertion (and fix message grammar)Also assert the error is an Invalid admission error.
- _, err := testClient.updateAuthResource(t, testCtx, testSpec, tt.specUpdate) - require.Error(t, err, "uncompilable CEL expression should return in admission error") + _, err := testClient.updateAuthResource(t, testCtx, testSpec, tt.specUpdate) + require.Error(t, err, "uncompilable CEL expression should return an admission error") + require.True(t, errors.IsInvalid(err), "expected Invalid admission error, got: %v", err)
586-604: Helper polish and conflict-resilient update
- Add t.Helper() for better failure locations.
- Consider retrying on 409 conflicts; status writes can race with spec updates.
- Docstring says “persists them”; when updateAuthSpec == nil you currently do not persist baseSpec. Either persist baseSpec or clarify the comment.
Apply this diff to improve the helper:
func (tc *testClient) updateAuthResource(t *testing.T, ctx context.Context, baseSpec *configv1.AuthenticationSpec, updateAuthSpec func(baseSpec *configv1.AuthenticationSpec)) (*configv1.Authentication, error) { + t.Helper() - auth := tc.getAuth(t, ctx) - if updateAuthSpec == nil { - return auth, nil - } - - spec := baseSpec.DeepCopy() - updateAuthSpec(spec) - - auth.Spec = *spec - auth, err := tc.configClient.ConfigV1().Authentications().Update(ctx, auth, metav1.UpdateOptions{}) - if err != nil { - return nil, err - } - require.True(t, equality.Semantic.DeepEqual(auth.Spec, *spec)) - return auth, nil + // retry on resourceVersion conflicts + var updated *configv1.Authentication + err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + curr := tc.getAuth(t, ctx) + spec := baseSpec.DeepCopy() + if updateAuthSpec != nil { + updateAuthSpec(spec) + } + curr.Spec = *spec + var err error + updated, err = tc.configClient.ConfigV1().Authentications().Update(ctx, curr, metav1.UpdateOptions{}) + return err + }) + if err != nil { + return nil, err + } + require.True(t, equality.Semantic.DeepEqual(updated.Spec, *baseSpec.DeepCopy()), "persisted spec differs from expected") + return updated, nil }Add import:
// at top imports "k8s.io/client-go/util/retry"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
test/e2e-oidc/external_oidc_test.go(6 hunks)
🔇 Additional comments (3)
test/e2e-oidc/external_oidc_test.go (3)
195-236: Degrade-path test looks correctGood split: invalid issuer CA/URL should pass admission, then degrade operator. No issues spotted.
261-270: Call-site update LGTMHandling (auth, err) and asserting no error is correct here.
314-322: Call-site update LGTMPattern consistent with other updates; no issues.
| func TestExternalOIDCWithKeycloak(t *testing.T) { | ||
| testCtx := t.Context() |
There was a problem hiding this comment.
🧩 Analysis chain
Duplicate test function: redeclaration of TestExternalOIDCWithKeycloak
Second declaration will cause a compile error. Keep a single test function. Remove the newly introduced header and context line.
Apply this diff:
-func TestExternalOIDCWithKeycloak(t *testing.T) {
- testCtx := t.Context()
+// removed duplicate function header; use the original test function and its context🏁 Script executed:
#!/bin/bash
# Verify duplicate test function definitions
rg -nP '^\s*func\s+TestExternalOIDCWithKeycloak\s*\(' -C2Length of output: 601
Remove duplicate TestExternalOIDCWithKeycloak declaration
The second TestExternalOIDCWithKeycloak at line 64 duplicates the one at line 60 and will not compile. Delete the duplicate function starting at line 64.
🤖 Prompt for AI Agents
In test/e2e-oidc/external_oidc_test.go around lines 64 to 65 there is a
duplicate declaration of TestExternalOIDCWithKeycloak (the same function already
declared at line 60) which causes a compile error; remove the entire duplicate
function starting at line 64 (and any immediately associated closing brace) so
only the original TestExternalOIDCWithKeycloak remains, then run `go test` to
verify compilation.
There was a problem hiding this comment.
This looks incorrect based on the diff.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there's only one TestExternalOIDCWithKeycloak function
rg -n "^func TestExternalOIDCWithKeycloak" test/e2e-oidc/external_oidc_test.go
# Show the actual lines around the function declaration
sed -n '58,68p' test/e2e-oidc/external_oidc_test.go
Length of output: 412
@everettraven You're absolutely right, and I apologize for the confusion.
I've verified the actual file, and there is only one TestExternalOIDCWithKeycloak function declaration at line 60. There is no duplicate function and no compilation error.
The diff context provided appears to have been misleading or incorrectly formatted. The actual change in this PR is simply replacing the old context initialization:
testCtx, cancel := context.WithCancel(context.Background())
defer cancel()with the new approach:
testCtx := t.Context()This is a valid change that modernizes the test to use Go's built-in test context handling. My previous comment about duplicate declarations was incorrect. Thank you for catching this!
🧬 Code Graph Analysis Results
- File: test/e2e-oidc/external_oidc_test.go
-
Summary: fetchIssuerJWKS(issuerURL string) (*jwks, error)
- Purpose: Retrieve the OpenID Connect provider's JWKS (JSON Web Key Set) by discovering the JWKS URL from the provider's well-known configuration.
- Parameters:
- issuerURL: base URL of the OIDC provider.
- Returns:
- *jwks: parsed JWKS containing RSA public keys used to verify tokens.
- error: if any network, parsing, or JSON errors occur.
- Notes:
- Uses an HTTP client that skips TLS verification.
- Performs two steps: fetch /.well-known/openid-configuration to get jwks_uri, then fetch the JWKS itself.
- Potential relevance: Used to verify tokens signed by the external IdP in tests; if a diff indicates token verification changes, this code is central.
-
Summary: extractRSAPubKeyFunc(issuerJWKS *jwks) func(*jwt.Token) (any, error)
- Purpose: Provide a key function to jwt.ParseWithClaims to validate JWT signatures using the RSA public keys from the provider JWKS.
- Parameters:
- issuerJWKS: the JWKS containing keys with kid, alg, n, e.
- Returns:
- A function suitable for jwt.ParseWithClaims(token, claims, keyFunc) that selects the matching RSA public key by kid and validates the algorithm.
- Behavior:
- Iterates issuerJWKS.Keys to find a matching kid (from token.Header["kid"]).
- Decodes base64url-encoded N and E to construct rsa.PublicKey.
- Supports RS256; returns error if key not found or algorithm unsupported.
- Notes:
- Errors out if decoding fails or no matching key is found.
- Potential relevance: Critical for JWT verification; a diff affecting token verification logic would touch this function.
-
Summary: type oidcAuthResponse struct
- Purpose: Model for OAuth token response used in tests.
- Fields:
- AccessToken, ExpiresIn, RefreshToken, RefreshExpiresIn, TokenType, IdToken, NotBeforePolicy, SessionState, Scope.
- Return/Use: Unmarshaled from token endpoint responses during tests.
-
Summary: type expectedClaims struct
- Purpose: JWT claims structure used to assert contents of tokens in tests.
- Fields:
- jwt.RegisteredClaims (embedded)
- Email, Type, Name, GivenName, FamilyName, PreferredUsername
- Return/Use: Used to decode/validate claims from access_token and id_token.
-
Summary: type jwks struct
- Purpose: Local representation of JWKS for tests.
- Fields:
- Keys: slice of keys with KID, Use, KTY, Alg, N, E
- Return/Use: Used by fetchIssuerJWKS and extractRSAPubKeyFunc.
-
Summary: func validateAuthConfigJSON(t *testing.T, ctx context.Context, authSpec *configv1.AuthenticationSpec, usernamePrefix, groupsClaim, groupsPrefix string, kasRevision int32)
- Purpose: Validate that the CAO-produced auth-config.json contents in the relevant ConfigMaps match expected values.
- Parameters:
- t, ctx: testing and context
- authSpec: current AuthenticationSpec to be reflected in the expected JSON
- usernamePrefix, groupsClaim, groupsPrefix: claim mapping prefixes to verify in the JSON
- kasRevision: KAS revision affecting configMap name suffix
- Returns: none (asserts via require)
- Behavior:
- Builds expected JSON template (with dynamic handling for feature gate ExternalOIDCWithAdditionalClaimMappings).
- Reads ConfigMaps from openshift-config-managed and openshift-kube-apiserver (and a per-revision one) and asserts Content equals expectedAuthConfigJSON.
- Important details:
- CA bundle fetch pulls from openshift-config/auth CA bundle if configured.
- Handles two possible claim mappings for username (email with Prefix or sub, depending on configuration and feature gates).
-
Summary: func (tc *testClient) testOIDCAuthentication(t *testing.T, ctx context.Context, kcClient *test.KeycloakClient, usernameClaim, usernamePrefix string, expectAuthSuccess bool)
- Purpose: Exercise OIDC authentication flow against Keycloak IdP using a test user, then attempt to authenticate to the Kubernetes API with the obtained OIDC tokens.
- Parameters:
- t, ctx: testing and context
- kcClient: Keycloak test client
- usernameClaim: which claim to use for username (e.g., "email" or "sub")
- usernamePrefix: prefix to apply to the username for Kubernetes authentication
- expectAuthSuccess: whether to expect successful authentication via SelfSubjectAccessReview
- Behavior:
- Creates a user/group in Keycloak, authenticates to obtain tokens, validates tokens by parsing with JWKS, then creates a Kubernetes client authenticated with the id_token and performs a SelfSubjectReview, asserting success or unauthorized based on expectAuthSuccess and usernameClaim.
- Notes:
- Validates token contents against expected claims (issuer, audience, givenName, familyName, etc.).
- Breaks authentication if username claim cannot be resolved when expectAuthSuccess is true/false accordingly.
-
Summary: func (tc *testClient) requireKASRolloutSuccessful(t *testing.T, testCtx context.Context, authSpec *configv1.AuthenticationSpec, kasOriginalRevision int32, expectedUsernamePrefix string)
- Purpose: Wait for KAS rollout to complete and validate authentication config JSON against expected state after changes.
- Parameters:
- t, testCtx: testing and context
- authSpec: AuthenticationSpec to reflect in subsequent validation
- kasOriginalRevision: baseline KAS revision to compare against
- expectedUsernamePrefix: expected username prefix in the validated config
- Behavior:
- Waits for new KAS rollout, then validates KAS config and authentication config JSON using validateKASConfig and validateAuthConfigJSON.
-
Summary: func featureGateEnabled(ctx context.Context, configClient *configclient.Clientset, feature configv1.FeatureGateName) bool
- Purpose: Helper to check if a feature gate is enabled.
- Parameters:
- ctx, configClient, feature
- Returns: true if the named feature gate is enabled; false otherwise.
- Behavior: Reads cluster FeatureGates status and checks for the given feature name.
-
These snippets are relevant for understanding how the test builds, verifies, and validates OIDC-related configuration and token processing (including how diffs in expected behavior might be surfaced). If the diff pertains to token verification flow, auth-config.json contents, or OIDC claim mappings, these sections are particularly relevant.
There was a problem hiding this comment.
@everettraven initially I pushed a commit that had this issue indeed but removed it shortly after 😅
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…valid CEL expression
liouk
force-pushed
the
e2e-oidc-cel-fix
branch
from
41f570e to
90ef251
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/e2e-oidc/external_oidc_test.go (1)
582-600: Consider refactoring the equality assertion for error scenarios.The signature change correctly propagates errors. However, line 597 performs a
require.True()assertion that will fail the test before returning the auth object. This is appropriate for successful updates but creates an inconsistency: callers that expect errors (like the CEL validation test) will never see them because the test will fail at the assertion first if the spec somehow doesn't match.Consider moving the equality check to a separate validation helper that callers invoke only when they expect success:
func (tc *testClient) updateAuthResource(t *testing.T, ctx context.Context, baseSpec *configv1.AuthenticationSpec, updateAuthSpec func(baseSpec *configv1.AuthenticationSpec)) (*configv1.Authentication, error) { auth := tc.getAuth(t, ctx) if updateAuthSpec == nil { return auth, nil } spec := baseSpec.DeepCopy() updateAuthSpec(spec) auth.Spec = *spec auth, err := tc.configClient.ConfigV1().Authentications().Update(ctx, auth, metav1.UpdateOptions{}) if err != nil { return nil, err } - - require.True(t, equality.Semantic.DeepEqual(auth.Spec, *spec)) - return auth, nil }Then add a helper for callers that expect success:
func (tc *testClient) requireAuthUpdate(t *testing.T, ctx context.Context, baseSpec *configv1.AuthenticationSpec, updateAuthSpec func(baseSpec *configv1.AuthenticationSpec)) *configv1.Authentication { auth, err := tc.updateAuthResource(t, ctx, baseSpec, updateAuthSpec) require.NoError(t, err) require.True(t, equality.Semantic.DeepEqual(auth.Spec, *baseSpec)) return auth }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
test/e2e-oidc/external_oidc_test.go(6 hunks)
🔇 Additional comments (4)
test/e2e-oidc/external_oidc_test.go (4)
61-61: LGTM: Idiomatic test context initialization.Using
t.Context()is the recommended approach for test contexts in Go 1.18+, providing automatic cancellation when the test completes.
151-189: LGTM: Properly validates admission-time CEL validation errors.This test correctly validates the PR objective by expecting admission-time errors when uncompilable CEL expressions are provided. The test covers both uid and extras claim mapping scenarios with appropriate feature gate checks.
191-232: LGTM: Correctly distinguishes admission success from runtime degradation.This test properly validates that invalid OIDC configurations (CA bundle, issuer URL) pass admission but cause the authentication operator to become degraded at runtime. The error handling correctly expects the update to succeed (line 225) while later validating operator degradation (line 227).
224-225: LGTM: Call sites correctly handle the new error return.All call sites properly capture both the auth object and error return values, assert no error where updates are expected to succeed, and correctly use the returned auth objects for subsequent validation steps.
Also applies to: 258-265, 310-317
|
@liouk: The following tests failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/retest-required |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, liouk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/verified by e2e-oidc-techpreview |
openshift-ci-robot
added
the
verified
|
@liouk: This PR has been marked as verified by In 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. |
openshift-merge-bot
bot
merged commit e31142a
into
openshift:master
3 participants
openshift/kubernetes#2353 introduced admission-time CEL expression validations for the auth CR; this PR updates the OIDC e2e test to expect an admission-time error when uncompilable CEL expressions are used.