CNTRLPLANE-2544: Add ExternalOIDCWithUpstreamParity e2e tests#30906
CNTRLPLANE-2544: Add ExternalOIDCWithUpstreamParity e2e tests#30906xingxingxia wants to merge 2 commits 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. |
WalkthroughUpdated a Go module dependency; extended Keycloak test client to support creating users with explicit email and treat 409 on group creation as success; added a gated OIDC E2E test suite covering claim- and expression-based mappings, multiple validation rules, config validation, and a rollout helper. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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
|
[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 |
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.
|
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
| // invalidExprUsername: email will be split to produce username that's too short (fails userValidationRules) | ||
| invalidExprUsername = "abc" | ||
| invalidExprUsernamePassword = fmt.Sprintf("password-invalid-expr-username-%s", testID) | ||
| invalidExprUsernameEmail := fmt.Sprintf("%s@example.com", invalidExprUsername) | ||
| o.Expect(keycloakCli.CreateUserWithEmail(invalidExprUsername, invalidExprUsernameEmail, invalidExprUsernamePassword, group)).To(o.Succeed(), "should be able to create invalidExprUsername") |
There was a problem hiding this comment.
Make the short-username fixture unique.
Line 578 hard-codes invalidExprUsername to "abc" while the rest of this block is randomized. Any retry or repeated setup pass against the same Keycloak realm will hit a duplicate-user 409 and fail before the validation assertion runs.
🧪 Suggested fix
- invalidExprUsername = "abc"
+ invalidExprUsername = fmt.Sprintf("a%s", testID[:2])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // invalidExprUsername: email will be split to produce username that's too short (fails userValidationRules) | |
| invalidExprUsername = "abc" | |
| invalidExprUsernamePassword = fmt.Sprintf("password-invalid-expr-username-%s", testID) | |
| invalidExprUsernameEmail := fmt.Sprintf("%s@example.com", invalidExprUsername) | |
| o.Expect(keycloakCli.CreateUserWithEmail(invalidExprUsername, invalidExprUsernameEmail, invalidExprUsernamePassword, group)).To(o.Succeed(), "should be able to create invalidExprUsername") | |
| // invalidExprUsername: email will be split to produce username that's too short (fails userValidationRules) | |
| invalidExprUsername = fmt.Sprintf("a%s", testID[:2]) | |
| invalidExprUsernamePassword = fmt.Sprintf("password-invalid-expr-username-%s", testID) | |
| invalidExprUsernameEmail := fmt.Sprintf("%s@example.com", invalidExprUsername) | |
| o.Expect(keycloakCli.CreateUserWithEmail(invalidExprUsername, invalidExprUsernameEmail, invalidExprUsernamePassword, group)).To(o.Succeed(), "should be able to create invalidExprUsername") |
🤖 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 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.
| 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: 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. |
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