CNTRLPLANE-2544: Add ExternalOIDCWithUpstreamParity e2e tests#30906
CNTRLPLANE-2544: Add ExternalOIDCWithUpstreamParity e2e tests#30906xingxingxia wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@xingxingxia: This pull request explicitly references no jira issue. 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:
WalkthroughThis pull request extends Keycloak test client functionality and introduces a comprehensive E2E test suite for OpenShift's external OIDC authentication. The Keycloak client is refactored to handle user creation with email parameters and improved HTTP response handling for conflict scenarios. A feature-gated test suite validates OIDC behavior using claim-based mappings, CEL expression mappings, and admission validation constraints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
xingxingxia
changed the title
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
2 times, most recently
from
40ec653 to
7b35701
Compare
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
7b35701 to
1bcae59
Compare
xingxingxia
changed the title
|
@xingxingxia: This pull request references CNTRLPLANE-2544 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 story 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@xingxingxia: This pull request references CNTRLPLANE-2544 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 story 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. |
|
@xingxingxia: This pull request references CNTRLPLANE-2544 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 story 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. |
|
@ShazaAldawamneh , @everettraven , could you please take a look for review? |
|
@xingxingxia: This pull request references CNTRLPLANE-2544 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 story 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. |
| // TODO: remove this "if" when console bug https://redhat.atlassian.net/browse/OCPBUGS-78477 is fixed | ||
| if co.Name == "console" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
We should fix this in console before merging this. This is shared logic used by more than our tests so this ends up making console operator settling ignored for every test that calls this.
There was a problem hiding this comment.
@everettraven , yes I knew. Now I defined a temporary function waitForRollout2 (only concerned about CO kube-apiserver and authentication, not checking co/console for now) as a workaround to be used for g.Describe("[OCPFeatureGate:ExternalOIDCWithUpstreamParity]" tests. So that the tests can pass to be merged. Once console bug is fixed, it'll easy to be removed and replaced with waitForRollout.
|
Thanks for the comprehensive test coverage! The tests look great overall and However, I noticed we're missing some important validation tests for the Could you please add the following test cases to ensure the validation rules
The expected behavior should match what we have in the integration tests at Let me know if you need any clarification on these scenarios! |
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
1bcae59 to
21be253
Compare
openshift-ci
bot
added
the
needs-rebase
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/extended/authentication/keycloak_client.go (1)
138-173: Consider extracting common user creation logic.
CreateUserWithEmailduplicates most ofCreateUser(lines 101-136). A helper could reduce duplication:func (kc *keycloakClient) createUserInternal(user user) error { ... }This is a minor maintainability improvement and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/keycloak_client.go` around lines 138 - 173, CreateUserWithEmail duplicates most of CreateUser; extract the shared construction + POST logic into a private helper (e.g., createUserInternal(user user) error) that performs json.Marshal, kc.DoRequest and status checking, then have CreateUser and CreateUserWithEmail call createUserInternal after assembling their respective user structs; update references to use the helper and remove duplicated error/response handling in CreateUserWithEmail and CreateUser to keep behavior identical.test/extended/authentication/oidc.go (1)
1022-1022: Consider usingwait.PollUntilContextTimeoutinstead of deprecatedwait.PollImmediate.
wait.PollImmediateis deprecated. Since this is a temporary workaround (per TODO), this is acceptable for now, but when removing the console bug workaround, consider switching to the context-aware variant:wait.PollUntilContextTimeout(ctx, 10*time.Second, 50*time.Minute, true, func(ctx context.Context) (bool, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/oidc.go` at line 1022, Replace the deprecated wait.PollImmediate call with the context-aware wait.PollUntilContextTimeout: pass the test's context variable (ctx) into wait.PollUntilContextTimeout, change the closure signature from func() (bool, error) to func(ctx context.Context) (bool, error), keep the same interval (10*time.Second) and timeout (50*time.Minute), and set the immediate boolean to true so the first poll happens immediately; update the surrounding code that calls the closure accordingly (the call originally invoking wait.PollImmediate in the test/extended/authentication/oidc.go 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/extended/authentication/keycloak_client.go`:
- Around line 138-173: CreateUserWithEmail duplicates most of CreateUser;
extract the shared construction + POST logic into a private helper (e.g.,
createUserInternal(user user) error) that performs json.Marshal, kc.DoRequest
and status checking, then have CreateUser and CreateUserWithEmail call
createUserInternal after assembling their respective user structs; update
references to use the helper and remove duplicated error/response handling in
CreateUserWithEmail and CreateUser to keep behavior identical.
In `@test/extended/authentication/oidc.go`:
- Line 1022: Replace the deprecated wait.PollImmediate call with the
context-aware wait.PollUntilContextTimeout: pass the test's context variable
(ctx) into wait.PollUntilContextTimeout, change the closure signature from
func() (bool, error) to func(ctx context.Context) (bool, error), keep the same
interval (10*time.Second) and timeout (50*time.Minute), and set the immediate
boolean to true so the first poll happens immediately; update the surrounding
code that calls the closure accordingly (the call originally invoking
wait.PollImmediate in the test/extended/authentication/oidc.go test).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 95544b70-7fc7-4555-b619-3d59cd47aba0
⛔ Files ignored due to path filters (58)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/.golangci.go-validated.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_backup.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_crio_credential_provider_config.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha2/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha2/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/types_console_sample.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/Makefileis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/types_pacemakercluster.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/etcd/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/legacyfeaturegates.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_awsprovider.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machine.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_machineconfiguration.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/types_clusterapi.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operatoringress/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operatoringress/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
go.modtest/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
21be253 to
77be87f
Compare
openshift-ci
bot
removed
the
needs-rebase
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/authentication/keycloak_client.go (1)
138-173: Consider reducing duplication withCreateUser.This function duplicates
CreateUser(lines 101-136), differing only in how the email is set. A small refactor could haveCreateUserdelegate toCreateUserWithEmail:func (kc *keycloakClient) CreateUser(username, password string, groups ...string) error { return kc.CreateUserWithEmail(username, fmt.Sprintf("%s@payload.openshift.io", username), password, groups...) }This is a minor maintainability suggestion—the current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/keycloak_client.go` around lines 138 - 173, Refactor CreateUser to delegate to CreateUserWithEmail: update the method kc.CreateUser(username, password, groups...) to call kc.CreateUserWithEmail and supply the generated default email (e.g. fmt.Sprintf("%s@payload.openshift.io", username)) instead of duplicating the user construction and HTTP logic; keep CreateUserWithEmail as-is and ensure fmt is available for formatting the email.
🤖 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/extended/authentication/keycloak_client.go`:
- Around line 138-173: Refactor CreateUser to delegate to CreateUserWithEmail:
update the method kc.CreateUser(username, password, groups...) to call
kc.CreateUserWithEmail and supply the generated default email (e.g.
fmt.Sprintf("%s@payload.openshift.io", username)) instead of duplicating the
user construction and HTTP logic; keep CreateUserWithEmail as-is and ensure fmt
is available for formatting the email.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 693f78bc-5c2c-42ae-99f0-340dbd568037
⛔ Files ignored due to path filters (11)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (3)
go.modtest/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/authentication/oidc.go
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
77be87f to
aac8473
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/authentication/keycloak_client.go (1)
123-136:⚠️ Potential issue | 🟠 MajorStop formatting the full
userpayload into errors.Lines 125, 130, and 136 format
userdirectly, which includesCredentials[].Valueand the explicit email. A transient Keycloak failure will leak test passwords into CI artifacts.🔐 Suggested fix
userBytes, err := json.Marshal(user) if err != nil { - return fmt.Errorf("marshalling user configuration %v", user) + return fmt.Errorf("marshalling user configuration for %q: %w", username, err) } resp, err := kc.DoRequest(http.MethodPost, userURL.String(), runtime.ContentTypeJSON, true, bytes.NewBuffer(userBytes)) if err != nil { - return fmt.Errorf("sending POST request to %q to create user %v", userURL.String(), user) + return fmt.Errorf("sending POST request to %q to create user %q: %w", userURL.String(), username, err) } defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { respBytes, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed creating user %v: %s - %s", user, resp.Status, respBytes) + return fmt.Errorf("failed creating user %q: %s - %s", username, resp.Status, respBytes) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/authentication/keycloak_client.go` around lines 123 - 136, The error messages currently interpolate the full user struct (variable "user") in the json.Marshal error and both fmt.Errorf calls around DoRequest and the non-201 response which can leak Credentials[].Value and email; update the three error returns to avoid dumping the whole user: for the json.Marshal error (after json.Marshal(user)), return a concise error that references the user's ID/username or a redacted placeholder instead of the full struct; for the DoRequest error (after kc.DoRequest), include only the target URL and the user's safe identifier (e.g., user.Username or user.ID) or a constant "<redacted>" for sensitive fields; and for the non-201 response, log response status and body but replace the user interpolation with the safe identifier or "<redacted>" so Credentials[].Value and email are never printed. Use the existing variables user, userURL, resp and the surrounding code paths (json.Marshal block, DoRequest error block, and resp.StatusCode != http.StatusCreated block) to make these substitutions.
🤖 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/extended/authentication/oidc.go`:
- Around line 577-581: The short-username fixture invalidExprUsername is
hard-coded to "abc" and can collide across retries; change the assignment of
invalidExprUsername to include the test-specific suffix (e.g., append testID or
a short random token) so it becomes unique while still producing a short
username for validation, then use that variable when calling
keycloakCli.CreateUserWithEmail(invalidExprUsername, invalidExprUsernameEmail,
invalidExprUsernamePassword, group) to avoid 409 duplicate-user errors on
repeated runs.
- Around line 720-757: The test is performing real Authentication updates;
change the calls that exercise validation to perform a dry-run update instead
and assert validation-specific errors: invoke configureOIDCAuthentication (or
add a dryRun flag/option to it) so it performs the update with
metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}; for the rejection
cases assert apierrors.IsInvalid(err) and check the returned field/message for
the specific claim mapping (username.expression with Prefix and
groups.expression with non-empty Prefix) rather than just HaveOccurred(), and
for the acceptance cases perform the same dry-run update and assert no error
(err == nil). Ensure you reference configureOIDCAuthentication,
metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}, and
apierrors.IsInvalid when making these changes.
---
Outside diff comments:
In `@test/extended/authentication/keycloak_client.go`:
- Around line 123-136: The error messages currently interpolate the full user
struct (variable "user") in the json.Marshal error and both fmt.Errorf calls
around DoRequest and the non-201 response which can leak Credentials[].Value and
email; update the three error returns to avoid dumping the whole user: for the
json.Marshal error (after json.Marshal(user)), return a concise error that
references the user's ID/username or a redacted placeholder instead of the full
struct; for the DoRequest error (after kc.DoRequest), include only the target
URL and the user's safe identifier (e.g., user.Username or user.ID) or a
constant "<redacted>" for sensitive fields; and for the non-201 response, log
response status and body but replace the user interpolation with the safe
identifier or "<redacted>" so Credentials[].Value and email are never printed.
Use the existing variables user, userURL, resp and the surrounding code paths
(json.Marshal block, DoRequest error block, and resp.StatusCode !=
http.StatusCreated block) to make these substitutions.
🪄 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: 9b4488a4-b9de-42f8-b9b4-9361b8ce107b
📒 Files selected for processing (2)
test/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
| g.It("should correctly validate prefix configurations with expression-based mappings", func() { | ||
| // Test rejections: prefixPolicy Prefix with username.expression, non-empty prefix with groups.expression | ||
| _, _, err := configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Username = configv1.UsernameClaimMapping{ | ||
| Expression: "claims.email.split('@')[0]", | ||
| PrefixPolicy: configv1.Prefix, | ||
| } | ||
| }) | ||
| o.Expect(err).To(o.HaveOccurred(), "should reject prefixPolicy Prefix when username.expression is set") | ||
|
|
||
| _, _, err = configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Groups = configv1.PrefixedClaimMapping{ | ||
| TokenClaimMapping: configv1.TokenClaimMapping{ | ||
| Expression: "claims.groups", | ||
| }, | ||
| Prefix: "group-", | ||
| } | ||
| }) | ||
| o.Expect(err).To(o.HaveOccurred(), "should reject non-empty prefix when groups.expression is set") | ||
|
|
||
| // Test acceptances: prefixPolicy NoPrefix with username.expression, empty prefix with groups.expression | ||
| _, _, err = configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Username = configv1.UsernameClaimMapping{ | ||
| Expression: "claims.email.split('@')[0]", | ||
| PrefixPolicy: configv1.NoPrefix, | ||
| } | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "should accept prefixPolicy NoPrefix when username.expression is set") | ||
|
|
||
| _, _, err = configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Groups = configv1.PrefixedClaimMapping{ | ||
| TokenClaimMapping: configv1.TokenClaimMapping{ | ||
| Expression: "claims.groups", | ||
| }, | ||
| Prefix: "", | ||
| } | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "should accept empty prefix when groups.expression is set") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PrefixedClaimMapping definition and JSON tag =="
rg -n -A6 -B2 --type=go 'type PrefixedClaimMapping struct'
rg -n --type=go 'json:"prefix'
echo
echo "== Prefix validation test block =="
rg -n -A45 -B5 --type=go 'should correctly validate prefix configurations with expression-based mappings' test/extended/authentication/oidc.go
echo
echo "== configureOIDCAuthentication helper =="
rg -n -A20 -B5 --type=go '^func configureOIDCAuthentication\(' test/extended/authentication/oidc.goRepository: openshift/origin
Length of output: 14916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Full configureOIDCAuthentication function =="
rg -n -A50 '^func configureOIDCAuthentication\(' test/extended/authentication/oidc.go | head -80
echo
echo "== Check for dry-run usage in the test file =="
rg -n 'DryRun\|dry.run\|dryRun' test/extended/authentication/oidc.go
echo
echo "== Check if Update is called =="
rg -n -B2 -A2 'Update\|Create' test/extended/authentication/oidc.go | grep -A2 -B2 'configureOIDCAuthentication'Repository: openshift/origin
Length of output: 2139
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== metav1.UpdateOptions to see if DryRun is supported =="
rg -n 'type UpdateOptions' vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go -A10
echo
echo "== Check for apierrors.IsInvalid usage patterns in tests =="
rg -n 'IsInvalid\|IsConflict' test/extended/ --type=go | head -20
echo
echo "== Check error handling in similar validation tests =="
rg -n -B3 -A3 'HaveOccurred.*should reject' test/extended/authentication/ --type=go | head -40
echo
echo "== Verify Prefix field JSON marshaling =="
rg -n -B5 -A5 'type PrefixedClaimMapping' vendor/github.com/openshift/api/config/v1/types_authentication.goRepository: openshift/origin
Length of output: 659
🏁 Script executed:
#!/bin/bash
set -eulo pipefail
echo "== Full PrefixedClaimMapping type definition =="
rg -n -A10 'type PrefixedClaimMapping struct' vendor/github.com/openshift/api/config/v1/types_authentication.go
echo
echo "== Check what happens when Prefix is empty string - test via Go =="
python3 << 'PYTHON_EOF'
import json
# Simulate the struct with json:"prefix" tag (no omitempty)
data_with_empty = {"Prefix": ""}
data_without_prefix = {}
json_with_empty = json.dumps(data_with_empty)
json_without = json.dumps(data_without_prefix)
print("Serialized with empty string:", json_with_empty)
print("Serialized with omitted field:", json_without)
print("Are they equal?", json_with_empty == json_without)
PYTHON_EOF
echo
echo "== Search for validation error checks in test files =="
rg -n 'IsInvalid|InvalidError|field.*required' test/extended/authentication/ --type=go
echo
echo "== How other tests check validation failures =="
rg -n -B2 -A4 'fielderrors\|invalid.*field' test/extended/authentication/ --type=go | head -30Repository: openshift/origin
Length of output: 1155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for dry-run usage in config tests =="
rg -n 'DryRun.*All' test/extended/authentication/ --type=go
echo
echo "== Check for field validation error patterns =="
rg -n -B2 -A5 'apierrors.*Invalid\|field.*invalid' test/extended/ --type=go | head -50
echo
echo "== Check error messages from configureOIDCAuthentication calls =="
rg -n -B1 'HaveOccurred.*should reject\|NotTo.*HaveOccurred' test/extended/authentication/oidc.go | grep -A1 -B1 'accept\|reject'
echo
echo "== Look for admission webhook validation tests =="
rg -n 'admission\|webhook.*valid' test/extended/authentication/oidc.go | head -20Repository: openshift/origin
Length of output: 106
Use a validation-only path for these admission checks instead of real updates.
The test currently performs full Authentication updates (line 866) for both rejection and acceptance cases, which can trigger disruptive rollouts and may fail due to transient API issues unrelated to validation. For rejection cases at lines 728 and 738, the plain HaveOccurred() assertion catches any error, making it impossible to distinguish validation failures from other failures. For acceptance cases at lines 747 and 757, real updates are unnecessary.
Prefer a dry-run validation approach with proper error assertions: use metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}} and assert on apierrors.IsInvalid() plus the specific field and message for rejection paths. Note that the Prefix: "" field is correctly serialized distinctly from omission (the JSON tag lacks omitempty), so the empty-prefix case is already properly distinct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/extended/authentication/oidc.go` around lines 720 - 757, The test is
performing real Authentication updates; change the calls that exercise
validation to perform a dry-run update instead and assert validation-specific
errors: invoke configureOIDCAuthentication (or add a dryRun flag/option to it)
so it performs the update with metav1.UpdateOptions{DryRun:
[]string{metav1.DryRunAll}}; for the rejection cases assert
apierrors.IsInvalid(err) and check the returned field/message for the specific
claim mapping (username.expression with Prefix and groups.expression with
non-empty Prefix) rather than just HaveOccurred(), and for the acceptance cases
perform the same dry-run update and assert no error (err == nil). Ensure you
reference configureOIDCAuthentication, metav1.UpdateOptions{DryRun:
[]string{metav1.DryRunAll}}, and apierrors.IsInvalid when making these changes.
|
Scheduling required tests: |
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
aac8473 to
6f8bcdd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/extended/authentication/oidc.go`:
- Around line 440-441: The test is not actually validating
Provider.Issuer.DiscoveryURL because you assign it to the default well‑known
path derived from Issuer.URL; update the test in oidc.go so the discovery URL is
a value that cannot be inferred from Issuer.URL (e.g. a different host/path) or
add an explicit assertion that the rendered auth config contains the exact
DiscoveryURL you set (refer to provider.Issuer.DiscoveryURL and the code that
renders/returns the auth config) — also apply the same change to the other
occurrences noted around lines referenced (the ones at the other commented
sites).
- Around line 599-608: The test currently constructs
provider.ClaimMappings.Groups using configv1.PrefixedClaimMapping and sets
TokenClaimMapping.Expression but relies on the non-pointer string Prefix
(json:"prefix") being "omitted" — which it isn't; both the literal that omits
Prefix and the one that sets Prefix:"" serialize to `"prefix": ""`. Fix by
removing the duplicate payload or, to truly test omission of the prefix field,
build the test payload as unstructured JSON/map (e.g., map[string]interface{} or
a raw JSON fixture) for ClaimMappings (referencing configv1.PrefixedClaimMapping
and TokenClaimMapping) so you can omit the "prefix" key entirely when
constructing the payload sent in the test.
🪄 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: c24a9ccc-93f4-4e57-9c9a-8140c8e20cfc
📒 Files selected for processing (2)
test/extended/authentication/keycloak_client.gotest/extended/authentication/oidc.go
|
Scheduling required tests: |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 6f8bcdd
|
|
@ShazaAldawamneh , now added prefixPolicy and prefix tests. To avoid test number too big, I added one test
And implicitly used the test
|
|
@ShazaAldawamneh , @everettraven , updated per your review, and adoption of some points (which I agree to) of coderabbit. Local rerun of all ExternalOIDCWithUpstreamParity tests passed. Pls take another look |
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
6f8bcdd to
a54690c
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xingxingxia 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 |
|
Scheduling required tests: |
|
While the PR waits for review, others already vendor the openshift/api thus cause this PR needs to rebase. Now rebased and removed the vendor commit from the PR. |
|
this lgtm but I will leave it to @everettraven to give it a look and put the label |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Handful of comments.
Other than these, this looks pretty good to me.
| g.It("should correctly validate prefix configurations with expression-based mappings", func() { | ||
| // Test rejections: prefixPolicy Prefix with username.expression, non-empty prefix with groups.expression | ||
| _, _, err := configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Username = configv1.UsernameClaimMapping{ | ||
| Expression: "claims.email.split('@')[0]", | ||
| PrefixPolicy: configv1.Prefix, | ||
| } | ||
| }) | ||
| o.Expect(err).To(o.HaveOccurred(), "should reject prefixPolicy Prefix when username.expression is set") | ||
|
|
||
| _, _, err = configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Groups = configv1.PrefixedClaimMapping{ | ||
| TokenClaimMapping: configv1.TokenClaimMapping{ | ||
| Expression: "claims.groups", | ||
| }, | ||
| Prefix: "group-", | ||
| } | ||
| }) | ||
| o.Expect(err).To(o.HaveOccurred(), "should reject non-empty prefix when groups.expression is set") | ||
|
|
||
| // Test acceptances: prefixPolicy NoPrefix with username.expression, empty prefix with groups.expression | ||
| _, _, err = configureOIDCAuthentication(ctx, oc, keycloakNamespace, oidcClientSecret, func(provider *configv1.OIDCProvider) { | ||
| provider.ClaimMappings.Username = configv1.UsernameClaimMapping{ | ||
| Expression: "claims.email.split('@')[0]", | ||
| PrefixPolicy: configv1.NoPrefix, | ||
| } | ||
| provider.ClaimMappings.Groups = configv1.PrefixedClaimMapping{ | ||
| TokenClaimMapping: configv1.TokenClaimMapping{ | ||
| Expression: "claims.groups", | ||
| }, | ||
| Prefix: "", | ||
| } | ||
| }) | ||
| o.Expect(err).NotTo(o.HaveOccurred(), "should accept prefixPolicy NoPrefix when username.expression is set and empty prefix when groups.expression is set") | ||
| }) |
There was a problem hiding this comment.
I'm not sure we need to test these cases here.
Because these tests are disruptive (today) and require a revision rollout of the KAS to even execute, it is pretty expensive to run these tests just to validate this behavior.
We already have integration tests in https://github.com/openshift/api/blob/master/config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml that exercise this behavior and they are significantly cheaper tests to run.
There was a problem hiding this comment.
@everettraven , reasonable, now I removed these. I originally added these per @ShazaAldawamneh 's #30906 (comment) . @ShazaAldawamneh , pls let me know if it isn't OK not adding these.
xingxingxia
force-pushed
the
add-external-oidc-parity-tests
branch
from
a54690c to
5fb6604
Compare
|
Scheduling required tests: |
| }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed()) | ||
| }) | ||
|
|
||
| g.It("should reject tokens when userValidationRules or CEL claimValidationRules evaluate to false", func() { |
There was a problem hiding this comment.
@everettraven , @ShazaAldawamneh , to avoid more times of rollout, FYI, I merged original two negative tests
g.It("should reject tokens when userValidationRules evaluates to false"
g.It("should reject tokens when CEL claimValidationRules evaluate to false"
to a single test:
g.It("should reject tokens when userValidationRules or CEL claimValidationRules evaluate to false"
I think this merge is reasonable.
There was a problem hiding this comment.
Thanks for the heads up. I agree that it is a reasonable merge.
| }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed()) | ||
| }) | ||
|
|
||
| g.It("should reject when username or groups expression produces value failing userValidationRules", func() { |
There was a problem hiding this comment.
Same here, to avoid more times of rollout, FYI, I merged original two negative tests to this single one.
There was a problem hiding this comment.
Thanks, I have a similar comment here regarding the immediately invoked functions.
|
@xingxingxia: The following test 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. |
|
Job Failure Risk Analysis for sha: 5fb6604
|
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall this LGTM. I'd prefer we don't use the immediately invoked functions in favor of a more linear and readable approach.
| testAPassed := false | ||
| testBPassed := false | ||
| var testAFailure, testBFailure string | ||
|
|
||
| // Test A: userValidationRules false evaluation | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| testAFailure = fmt.Sprintf("%v", r) | ||
| } | ||
| }() | ||
|
|
||
| o.Eventually(func(gomega o.Gomega) { | ||
| err := keycloakCli.Authenticate("admin-cli", invalidUserValidation, invalidUserValidationPassword) | ||
| gomega.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error authenticating as invalidUserValidation") | ||
|
|
||
| copiedOC := *oc | ||
| tokenOC := copiedOC.WithToken(keycloakCli.AccessToken()) | ||
| _, err = tokenOC.KubeClient().AuthenticationV1().SelfSubjectReviews().Create(ctx, &authnv1.SelfSubjectReview{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-info", invalidUserValidation), | ||
| }, | ||
| }, metav1.CreateOptions{}) | ||
| gomega.Expect(err).To(o.HaveOccurred(), "should not be able to create a SelfSubjectReview") | ||
| gomega.Expect(apierrors.IsUnauthorized(err)).To(o.BeTrue(), "should receive an unauthorized error") | ||
| }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed()) | ||
|
|
||
| testAPassed = true | ||
| }() | ||
|
|
||
| // Test B: CEL claimValidationRules false evaluation | ||
| func() { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| testBFailure = fmt.Sprintf("%v", r) | ||
| } | ||
| }() | ||
|
|
||
| o.Eventually(func(gomega o.Gomega) { | ||
| err := keycloakCli.Authenticate("admin-cli", invalidClaimValidation, invalidClaimValidationPassword) | ||
| gomega.Expect(err).NotTo(o.HaveOccurred(), "should not encounter an error authenticating as invalidClaimValidation") | ||
|
|
||
| copiedOC := *oc | ||
| tokenOC := copiedOC.WithToken(keycloakCli.AccessToken()) | ||
| _, err = tokenOC.KubeClient().AuthenticationV1().SelfSubjectReviews().Create(ctx, &authnv1.SelfSubjectReview{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: fmt.Sprintf("%s-info", invalidClaimValidation), | ||
| }, | ||
| }, metav1.CreateOptions{}) | ||
| gomega.Expect(err).To(o.HaveOccurred(), "should not be able to create a SelfSubjectReview") | ||
| gomega.Expect(apierrors.IsUnauthorized(err)).To(o.BeTrue(), "should receive an unauthorized error") | ||
| }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed()) | ||
|
|
||
| testBPassed = true | ||
| }() | ||
|
|
||
| // Final assertion: both tests should pass | ||
| failureMsg := fmt.Sprintf("userValidationRules false evaluation test failed: %t; reason: %s\nCEL claimValidationRules false evaluation test failed: %t; reason: %s", | ||
| !testAPassed, testAFailure, !testBPassed, testBFailure) | ||
|
|
||
| o.Expect(testAPassed && testBPassed).To(o.BeTrue(), failureMsg) |
There was a problem hiding this comment.
Do we need to wrap each of the merged tests in their own immediately invoked functions?
Could we instead just run each of the eventually statements sequentially with additional context provided via a g.By(...) statement (https://onsi.github.io/ginkgo/#documenting-complex-specs-by)?
I find the immediately invoked functions much more difficult to follow as a reader than something like:
g.It("...", func() {
g.By("checking that userValidationRule evaluating to 'false' causes authorization failure")
o.Eventually(...).WithTimeout(...).WithPolling(...).Should(o.Succeed())
g.By("checking that claimValidationRule evaluating to 'false' causes authorization failure")
o.Eventually(...).WithTimeout(...).WithPolling(...).Should(o.Succeed())
})| }).WithTimeout(5 * time.Minute).WithPolling(10 * time.Second).Should(o.Succeed()) | ||
| }) | ||
|
|
||
| g.It("should reject when username or groups expression produces value failing userValidationRules", func() { |
There was a problem hiding this comment.
Thanks, I have a similar comment here regarding the immediately invoked functions.
4 participants
Tests passed:
Prow CI jobs will also be added.
I noticed the BeforeAll and Ordered don't take effect. This is true for even the existing other external oidc tests. I'll investigate how to improve them.
CC @ShazaAldawamneh