CNTRLPLANE-3306: add ExternalOIDCWithUpstreamParity e2e tests#8287
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@ehearne-redhat: This pull request references CNTRLPLANE-3306 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 "5.0.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. |
|
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:
📝 WalkthroughWalkthroughAdded unit and e2e tests and updated a test utility to exercise the ExternalOIDCWithUpstreamParity feature gate. Tests and the utility now emit and verify CEL-based claim mappings for username and groups, add CEL claim validation rules (email presence/non-empty and email_verified == true), and add a CEL user validation rule preventing usernames with the Sequence Diagram(s)sequenceDiagram
actor TestRunner
participant HostedClusterSpec as "HostedCluster Spec"
participant Controller as "Control Plane Operator"
participant KAS as "Kube API Server (KAS)"
participant SSR as "SelfSubjectReview"
participant OIDC as "External OIDC Provider"
TestRunner->>HostedClusterSpec: create spec with ExternalOIDCWithUpstreamParity enabled
HostedClusterSpec->>Controller: reconcile spec
Controller->>KAS: produce AuthenticationConfiguration (DiscoveryURL, CEL claim mappings, claim/user validation rules)
KAS->>OIDC: use DiscoveryURL to fetch metadata / validate tokens
OIDC-->>KAS: return token and user claims
KAS->>SSR: evaluate ClaimMappings and UserValidationRules (CEL)
SSR-->>TestRunner: return UserInfo (username, groups) and validation result
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ehearne-redhat 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 |
openshift-ci
Bot
added
area/testing
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
423d933 to
67f1e85
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go (1)
1964-1996: Tighten this negative case by asserting error reason.This case currently checks only that an error occurs. Please assert an expected error substring (e.g., empty CEL expression) so unrelated failures can’t satisfy the test.
🔧 Suggested pattern
type testCase struct { name string client crclient.Reader expectedAuthenticationConfiguration *AuthenticationConfiguration hcpAuthenticationSpec *configv1.AuthenticationSpec shouldError bool + expectedErrContains string featureGates []featuregate.Feature } // in negative test case: shouldError: true, +expectedErrContains: "expression is not set", // in assertion block: if tc.shouldError { if err == nil { t.Fatal("expected an error to have occurred but got none") } + if tc.expectedErrContains != "" && !strings.Contains(err.Error(), tc.expectedErrContains) { + t.Fatalf("expected error containing %q, got: %v", tc.expectedErrContains, err) + } return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go` around lines 1964 - 1996, The test case "claimValidationRule with CEL - empty expression, error" currently only sets shouldError = true; tighten it by adding an expected error substring (e.g., expectedErrorSubstring: "empty" or "CEL expression") and changing the test assertions in the test loop to assert the returned error string contains that substring rather than merely checking shouldError; update the test harness code that reads shouldError (the test loop in auth_test.go) to look for expectedErrorSubstring when an error is returned (and fail if no substring match) so unrelated failures cannot satisfy the 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go`:
- Around line 1964-1996: The test case "claimValidationRule with CEL - empty
expression, error" currently only sets shouldError = true; tighten it by adding
an expected error substring (e.g., expectedErrorSubstring: "empty" or "CEL
expression") and changing the test assertions in the test loop to assert the
returned error string contains that substring rather than merely checking
shouldError; update the test harness code that reads shouldError (the test loop
in auth_test.go) to look for expectedErrorSubstring when an error is returned
(and fail if no substring match) so unrelated failures cannot satisfy the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 829cd042-7de5-4a8b-9700-9504081ff9a9
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.gotest/e2e/external_oidc_test.gotest/e2e/util/external_oidc.go
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
67f1e85 to
92c4197
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8287 +/- ##
==========================================
+ Coverage 39.95% 40.42% +0.47%
==========================================
Files 751 755 +4
Lines 92733 93238 +505
==========================================
+ Hits 37048 37689 +641
+ Misses 52998 52850 -148
- Partials 2687 2699 +12
... and 38 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ehearne-redhat
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(BeEmpty()) | ||
| // Username should be the email prefix (before @) since we configured expression: claims.email.split('@')[0] | ||
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(ContainSubstring("@")) | ||
| t.Logf("CEL username expression successfully mapped to: %s", selfSubjectReview.Status.UserInfo.Username) |
There was a problem hiding this comment.
Should also make sure the actual username matches what we expect?
There was a problem hiding this comment.
Yes, that makes sense. I have looked into how to do that. We can use userInfo.extra["claims.email"] and string parsing to ensure that the username matches what we expect i.e. if email is foo@bar.net then username should be foo .
There was a problem hiding this comment.
Is there a more deterministic way for us to have a set of expected output data based on the input data?
Relying on a separate feature to test the output of this feature feels like a bit of an unnecessary dependency if we have a way to be more deterministic.
There was a problem hiding this comment.
I did try to ponder on how we could do this. I wanted a different way to this but this was the one that jumped out to me. It was the only way I could see to ensure an absolute verification of the username.
I will investigate further and see.
| g := NewWithT(t) | ||
| t.Logf("begin to test CEL groups expression mapping") | ||
| // Groups expression uses: has(claims.groups) && type(claims.groups) == list ? claims.groups : [] | ||
| // If the token has groups, they should be present without prefix (no prefix in CEL expression) |
There was a problem hiding this comment.
For this test case, do we guarantee that the user will have groups configured?
There was a problem hiding this comment.
I believe so - I'm looking at this script here.
I'm thinking it wouldn't be a bad idea to have more reference to this script since there is this line that references a variable from this script.
https://github.com/openshift/hypershift/blob/main/test/e2e/util/external_oidc.go#L233-L234
That way we know exactly where this is all coming from. WDYT?
There was a problem hiding this comment.
I wonder if we should do away with the automatic setup of users and groups within Keycloak via that script?
Or at the very least have a deterministic set of users/groups that get created so that we can have more deterministic test behaviors?
Ideally, our test cases are self-sufficient and fully encompass the scope of what needs to be done.
Maybe this is better suited as a follow up though to prevent the scope of this work from getting blown up.
There was a problem hiding this comment.
We would need to look into it, definitely. From what I could tell, the users/groups had to be deployed in this way because of the nature of HyperShift itself. We can't do CRUD for the users/groups here because Keycloak is run externally here - we rely on it being set up alongside HyperShift, and it would be different to us having access to a cluster to just spin up keycloak/modify on the fly in our origin test suites.
I think realistically we would need to modify that script so the environment can match our expectations. However, it would also need to match the other test expectations too since these changes would impact those tests too.
I think we should definitely aim to figure that out sooner rather than later before we have too many tests that "commit" or conform to the script expectations.
There was a problem hiding this comment.
We can't do CRUD for the users/groups here because Keycloak is run externally here - we rely on it being set up alongside HyperShift, and it would be different to us having access to a cluster to just spin up keycloak/modify on the fly in our origin test suites
The running Keycloak instance has to be accessible to the HostedCluster that is created. Makes me think it would inherently be accessible to us as well. We would just need to make sure we know where we need to make the requests to configure it on the fly.
I wonder if we could even go so far as deploying Keycloak on the fly to have full control in the test setup like we do in the standalone openshift tests.
There was a problem hiding this comment.
I think you're right - we have access to admin creds from the script in keycloak ns.
With these creds we could control the users/groups by making api calls via ExtOIDCConfig.IssuerURL .
And on that same thread, we could actually deploy Keycloak here. We probably have everything we need. I could convert the script deployment into go code here.
This way we would have full control over testing username concretely.
I'll work on a potential way of doing this and re-ping when ready. Thanks for thinking this one out! :)
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test claim validation rules", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to test claim validation rules") | ||
| g.Expect(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules).NotTo(BeEmpty()) | ||
| // Authentication succeeded, so claim validation rules passed | ||
| claimRules := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules | ||
| g.Expect(claimRules).Should(HaveLen(1)) | ||
| g.Expect(claimRules[0].Type).Should(Equal(configv1.TokenValidationRuleTypeCEL)) | ||
| g.Expect(claimRules[0].CEL.Expression).Should(Equal("has(claims.email) && claims.email != ''")) | ||
| t.Logf("Claim validation rules successfully validated token") | ||
| }) | ||
|
|
||
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test user validation rules", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to test user validation rules") | ||
| g.Expect(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].UserValidationRules).NotTo(BeEmpty()) | ||
| // Authentication succeeded, so user validation rules passed | ||
| userRules := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].UserValidationRules | ||
| g.Expect(userRules).Should(HaveLen(1)) | ||
| g.Expect(userRules[0].Expression).Should(Equal("!user.username.startsWith('system:')")) | ||
| // Verify the username doesn't start with 'system:' (which is what the rule checks) | ||
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(HavePrefix("system:")) | ||
| t.Logf("User validation rules successfully validated user: %s", selfSubjectReview.Status.UserInfo.Username) | ||
| }) | ||
|
|
||
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test authentication config is properly set", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to verify authentication configuration") | ||
| oidcProvider := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0] | ||
|
|
||
| // Verify CEL expressions are set for username and groups | ||
| g.Expect(oidcProvider.ClaimMappings.Username.Expression).Should(Equal("claims.email.split('@')[0]")) | ||
| g.Expect(oidcProvider.ClaimMappings.Groups.Expression).Should(Equal("has(claims.groups) && type(claims.groups) == list ? claims.groups : []")) | ||
|
|
||
| // Verify claim validation rules are set | ||
| g.Expect(oidcProvider.ClaimValidationRules).Should(HaveLen(1)) | ||
| g.Expect(oidcProvider.ClaimValidationRules[0].Type).Should(Equal(configv1.TokenValidationRuleTypeCEL)) | ||
|
|
||
| // Verify user validation rules are set | ||
| g.Expect(oidcProvider.UserValidationRules).Should(HaveLen(1)) | ||
| g.Expect(oidcProvider.UserValidationRules[0].Expression).Should(Equal("!user.username.startsWith('system:')")) | ||
|
|
||
| t.Logf("All ExternalOIDCWithUpstreamParity configurations properly set") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Do we actually need these to be separate test cases? It seems like these are all inherently tested by the other two tests that were added here so it doesn't seem like these being explicit cases adds much value other than quantity of test cases.
Is there a way that we could add some negative test cases here similar to that of our proposed standalone openshift test cases like:
- token that fails claim validation results in unauthorized response from the selfsubjectaccessreview request
- token that fails user validation results in unauthorized response from the selfsubjectaccessreview request
There was a problem hiding this comment.
OK, I see what you mean. You're saying the last test referenced here combined the first two tests referenced. Yeah, that makes sense. I'll remove the last one. If you want them both combined, let me know. :)
There probably is a way to add negative test cases. However, I'm unsure how possible it is from the hypershift side because it all depends on what this script adds in terms of valid/invalid users.
As it currently stands, it adds/configures just valid users/groups. From what I understand, we would need to either add a new test or modify this script to configure these invalid users/groups so we can properly test extensively.
I'll push up what I have done currently, and look to go through new review comments tomorrow if any. :)
There was a problem hiding this comment.
OK, I see what you mean. You're saying the last test referenced here combined the first two tests referenced. Yeah, that makes sense. I'll remove the last one. If you want them both combined, let me know. :)
What I was meaning is that the first 2 test cases:
- "[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL username expression mapping"
- "[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL groups expression mapping"
Already inherently test what these last 2 tests are testing because you configure the claim validation and user validation rules for all the test cases.
I'm not sure it makes sense for us to retest this behavior.
There probably is a way to add negative test cases. However, I'm unsure how possible it is from the hypershift side because it all depends on what this script adds in terms of valid/invalid users.
As it currently stands, it adds/configures just valid users/groups. From what I understand, we would need to either add a new test or modify this script to configure these invalid users/groups so we can properly test extensively.
Okay, I've gone a bit back and forth on this throughout this review a touch already but now I'm thinking that if that script is presenting a limitation in what we can or cannot test I think modifying it as necessary is important to ensure we are getting the right level of test coverage here.
Because of this, we should include any modifications that need to happen there within scope of the testing work for HyperShift.
There was a problem hiding this comment.
Oh OK - noted. :)
I'll look into how exactly this would work so that our tests and the other tests using this script wouldn't exactly need to be changed.
It could be the case where we have an additional script but then we set the expectation of needing new script every time we test hypershift which would add burden. It might make sense to make the necessary changes to both tests if needed now so the burden isn't heavy later (we would hope :D) .
I'll look into it shortly.
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
92c4197 to
cfa62cb
Compare
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(ContainSubstring("@")) | ||
| // equals the actual email prefix | ||
| // e2eutil.ExternalOIDCExtraKeyFoo --> claims.email expression | ||
| emailValues := selfSubjectReview.Status.UserInfo.Extra[e2eutil.ExternalOIDCExtraKeyFoo] |
There was a problem hiding this comment.
@everettraven do we know if this field is feature gate dependent?
So if ExternalOIDCWithUIDAndExtraClaimMappings feature gate was disabled in the future what impact does it have on accessing this field for email verification?
There was a problem hiding this comment.
Since I have ensured the field can accessible on either feature gate enablement this shouldn't be an issue. :)
There was a problem hiding this comment.
That feature gate should already be enabled by default for a couple releases and therefore should never be disabled in the future. We actually need to remove it in the near future.
I don't think we need to check the UserInfo.Extra field as part of these tests though because that is functionality from an entirely different feature.
There was a problem hiding this comment.
Ah OK - thanks for clarifying! In that case we would need to use another mechanism as you rightfully pointed out in #8287 (comment) .
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go (1)
2220-2221:⚠️ Potential issue | 🟠 MajorUse optional claim access before calling
orValuehere.These two cases still use
claims.groups.orValue([])..., butorValue([])only works on an optional value. The earlier parity case already uses the correct form:claims.?groups.orValue([]). As written, these"shouldError: false"cases are asserting invalid CEL as a supported mapping.Suggested fix
- Expression: "claims.groups.orValue([]).filter(g, g.startsWith('ocp-'))", + Expression: "claims.?groups.orValue([]).filter(g, g.startsWith('ocp-'))",- Expression: "claims.groups.orValue([]).filter(g, g.startsWith('ocp-'))", + Expression: "claims.?groups.orValue([]).filter(g, g.startsWith('ocp-'))",Also applies to: 2258-2260, 2311-2312, 2356-2358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go` around lines 2220 - 2221, The test cases are using non-optional access `claims.groups.orValue([])...`, which is invalid because `orValue` requires an optional; update each offending expression to use optional access `claims.?groups.orValue([]).filter(g, g.startsWith('ocp-'))` (i.e., change `claims.groups.orValue([])` to `claims.?groups.orValue([])` in the test vectors). Locate and fix the same pattern occurrences referenced in the file (the similar expressions around the other test cases that currently use `claims.groups.orValue([])`).
🤖 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/e2e/external_oidc_test.go`:
- Around line 124-128: The username assertion currently reads the expected value
from the extra claim key e2eutil.ExternalOIDCExtraKeyFoo which only exists when
the ExternalOIDCWithUIDAndExtraClaimMappings feature is enabled; update the test
in external_oidc_test.go so it first checks whether
ExternalOIDCWithUIDAndExtraClaimMappings is enabled and only then asserts the
username from
selfSubjectReview.Status.UserInfo.Extra[e2eutil.ExternalOIDCExtraKeyFoo],
otherwise derive the expectedUserName from the test’s chosen upstream user (use
the variable that holds the selected test user/email used to create the upstream
identity) and assert that selfSubjectReview.Status.UserInfo.Username equals that
derived username.
---
Duplicate comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go`:
- Around line 2220-2221: The test cases are using non-optional access
`claims.groups.orValue([])...`, which is invalid because `orValue` requires an
optional; update each offending expression to use optional access
`claims.?groups.orValue([]).filter(g, g.startsWith('ocp-'))` (i.e., change
`claims.groups.orValue([])` to `claims.?groups.orValue([])` in the test
vectors). Locate and fix the same pattern occurrences referenced in the file
(the similar expressions around the other test cases that currently use
`claims.groups.orValue([])`).
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0971fdb6-6682-4fe9-b2fd-40b3d5169afd
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.gotest/e2e/external_oidc_test.gotest/e2e/util/external_oidc.go
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
cfa62cb to
ac11bf0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/external_oidc_test.go (1)
132-139: Consider verifying actual groups mapping, not just configuration.This subtest only validates that the groups expression is configured but doesn't verify the actual
selfSubjectReview.Status.UserInfo.Groupscontains expected values. If the Keycloak test users have groups configured, asserting their presence would strengthen this test.💡 Suggested enhancement
t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL groups expression mapping", func(t *testing.T) { g := NewWithT(t) t.Logf("begin to test CEL groups expression mapping") // Groups expression uses: has(claims.groups) && type(claims.groups) == list ? claims.groups : [] // If the token has groups, they should be present without prefix (no prefix in CEL expression) g.Expect(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression).NotTo(BeEmpty()) + // Verify groups are actually present in the response if user has groups configured + g.Expect(selfSubjectReview.Status.UserInfo.Groups).NotTo(BeEmpty()) t.Logf("CEL groups expression configured: %s", hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/external_oidc_test.go` around lines 132 - 139, The test currently only asserts the CEL groups expression is configured; instead, after confirming hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression is not empty, call the same code path that performs a self-subject review (inspect the SelfSubjectReview object used in this test) and assert that selfSubjectReview.Status.UserInfo.Groups contains the expected group names for the Keycloak test user(s). Locate the subtest block (the t.Run with label "Test CEL groups expression mapping") and add an assertion using the existing test helper that retrieves the SelfSubjectReview (or create one via the API client used elsewhere in the test suite) to verify the groups slice is non-empty and includes the known group(s) for the test account.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go`:
- Around line 2219-2221: The CEL expressions in test fixtures using
PrefixedClaimOrExpression.Expression wrongly call orValue() on possibly-missing
fields (e.g. "claims.groups.orValue([])") which requires optional access; update
each Expression string to use the optional operator before the field (e.g.
"claims.?groups.orValue([])") and do the same for roles (use
"claims.?roles.orValue([])"); apply this change to all occurrences of
PrefixedClaimOrExpression where Expression references claims.groups or
claims.roles (the instances noted around the test cases at the four groups lines
and the two roles lines).
---
Nitpick comments:
In `@test/e2e/external_oidc_test.go`:
- Around line 132-139: The test currently only asserts the CEL groups expression
is configured; instead, after confirming
hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression
is not empty, call the same code path that performs a self-subject review
(inspect the SelfSubjectReview object used in this test) and assert that
selfSubjectReview.Status.UserInfo.Groups contains the expected group names for
the Keycloak test user(s). Locate the subtest block (the t.Run with label "Test
CEL groups expression mapping") and add an assertion using the existing test
helper that retrieves the SelfSubjectReview (or create one via the API client
used elsewhere in the test suite) to verify the groups slice is non-empty and
includes the known group(s) for the test account.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 66e94f4c-86ef-48ad-9645-5acece62f594
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.gotest/e2e/external_oidc_test.gotest/e2e/util/external_oidc.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/util/external_oidc.go
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
ac11bf0 to
842e4e3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/external_oidc_test.go (2)
132-139: Outdated comment contradicts actual implementation.The comment on line 135 states:
// Groups expression uses: has(claims.groups) && type(claims.groups) == list ? claims.groups : []However, the actual expression set in
test/e2e/util/external_oidc.goline 172 isclaims.?groups.orValue([]). Please update the comment to reflect the correct expression.Additionally, this test only verifies configuration was applied (checking
Expressionis non-empty), not actual behavior. Consider strengthening the test by verifyingselfSubjectReview.Status.UserInfo.Groupsmatches expected values, similar to how username is verified.📝 Proposed fix for the comment
- // Groups expression uses: has(claims.groups) && type(claims.groups) == list ? claims.groups : [] - // If the token has groups, they should be present without prefix (no prefix in CEL expression) + // Groups expression uses: claims.?groups.orValue([]) + // Groups are returned as-is when present, or empty list when absent🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/external_oidc_test.go` around lines 132 - 139, The comment inside the Test CEL groups expression mapping test is stale and should be updated to match the actual CEL expression used (claims.?groups.orValue([])) — locate the test (t.Run "[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL groups expression mapping") and replace the old comment with the correct expression string referencing hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression; additionally, strengthen the test by fetching the SelfSubjectReview and asserting selfSubjectReview.Status.UserInfo.Groups contains the expected group values (similar to the existing username assertion) to verify behavior, not just configuration presence.
141-156: Test assertions are tightly coupled to implementation details.The tests hard-code the exact CEL expressions being verified. If the implementation changes these expressions, the tests will fail even if the new expressions are functionally equivalent.
Consider extracting these to constants shared between
external_oidc.goand this test file, or using more behavioral assertions where possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/external_oidc_test.go` around lines 141 - 156, The test hard-codes exact CEL strings from ClaimValidationRules (hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules) which couples the test to implementation; extract the two CEL expressions into exported constants (e.g., ClaimExprEmailExists and ClaimExprEmailVerified) in the package that defines external_oidc.go and reference those constants in this test, or replace the exact-string assertions with behavioral checks (e.g., assert the rule Type is TokenValidationRuleTypeCEL and the CEL expression contains/semantically matches the expected predicate) so the test verifies intent not exact syntax.
🤖 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/e2e/external_oidc_test.go`:
- Around line 132-139: The comment inside the Test CEL groups expression mapping
test is stale and should be updated to match the actual CEL expression used
(claims.?groups.orValue([])) — locate the test (t.Run
"[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL groups expression
mapping") and replace the old comment with the correct expression string
referencing
hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimMappings.Groups.Expression;
additionally, strengthen the test by fetching the SelfSubjectReview and
asserting selfSubjectReview.Status.UserInfo.Groups contains the expected group
values (similar to the existing username assertion) to verify behavior, not just
configuration presence.
- Around line 141-156: The test hard-codes exact CEL strings from
ClaimValidationRules
(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules)
which couples the test to implementation; extract the two CEL expressions into
exported constants (e.g., ClaimExprEmailExists and ClaimExprEmailVerified) in
the package that defines external_oidc.go and reference those constants in this
test, or replace the exact-string assertions with behavioral checks (e.g.,
assert the rule Type is TokenValidationRuleTypeCEL and the CEL expression
contains/semantically matches the expected predicate) so the test verifies
intent not exact syntax.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 98f452a1-8817-4b7c-898f-55d646f9551c
📒 Files selected for processing (3)
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.gotest/e2e/external_oidc_test.gotest/e2e/util/external_oidc.go
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
842e4e3 to
7870823
Compare
| // Check if ExternalOIDCWithUIDAndExtraClaimMappings feature gate is enabled. | ||
| // If not, we will need to add extra mapping to access email for username | ||
| // verification later. | ||
| if !featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUIDAndExtraClaimMappings) { | ||
| authnSpec.OIDCProviders[0].ClaimMappings.Extra = append(authnSpec.OIDCProviders[0].ClaimMappings.Extra, | ||
| configv1.ExtraMapping{ | ||
| Key: ExternalOIDCExtraKeyFoo, | ||
| ValueExpression: ExternalOIDCExtraKeyFooValueExpression, | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I'm not sure I'm following why this step here is necessary? They are two separate features and this looks like it already happens above if the gate is enabled?
There was a problem hiding this comment.
Yep - makes sense. I just needed that clarity from #8287 (comment) . I'll work on removing that.
| g.Expect(actualAuth.OIDCProviders).NotTo(BeEmpty()) | ||
|
|
||
| if featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUIDAndExtraClaimMappings) { | ||
| if featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUpstreamParity) || featuregates.Gate().Enabled(featuregates.ExternalOIDCWithUIDAndExtraClaimMappings) { |
There was a problem hiding this comment.
This also doesn't seem necessary? The feature we are adding tests for doesn't have any reliance on the extra claim mapping.
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(ContainSubstring("@")) | ||
| // equals the actual email prefix | ||
| // e2eutil.ExternalOIDCExtraKeyFoo --> claims.email expression | ||
| emailValues := selfSubjectReview.Status.UserInfo.Extra[e2eutil.ExternalOIDCExtraKeyFoo] |
There was a problem hiding this comment.
That feature gate should already be enabled by default for a couple releases and therefore should never be disabled in the future. We actually need to remove it in the near future.
I don't think we need to check the UserInfo.Extra field as part of these tests though because that is functionality from an entirely different feature.
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(BeEmpty()) | ||
| // Username should be the email prefix (before @) since we configured expression: claims.email.split('@')[0] | ||
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(ContainSubstring("@")) | ||
| t.Logf("CEL username expression successfully mapped to: %s", selfSubjectReview.Status.UserInfo.Username) |
There was a problem hiding this comment.
Is there a more deterministic way for us to have a set of expected output data based on the input data?
Relying on a separate feature to test the output of this feature feels like a bit of an unnecessary dependency if we have a way to be more deterministic.
| g := NewWithT(t) | ||
| t.Logf("begin to test CEL groups expression mapping") | ||
| // Groups expression uses: has(claims.groups) && type(claims.groups) == list ? claims.groups : [] | ||
| // If the token has groups, they should be present without prefix (no prefix in CEL expression) |
There was a problem hiding this comment.
I wonder if we should do away with the automatic setup of users and groups within Keycloak via that script?
Or at the very least have a deterministic set of users/groups that get created so that we can have more deterministic test behaviors?
Ideally, our test cases are self-sufficient and fully encompass the scope of what needs to be done.
Maybe this is better suited as a follow up though to prevent the scope of this work from getting blown up.
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test claim validation rules", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to test claim validation rules") | ||
| g.Expect(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules).NotTo(BeEmpty()) | ||
| // Authentication succeeded, so claim validation rules passed | ||
| claimRules := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].ClaimValidationRules | ||
| g.Expect(claimRules).Should(HaveLen(1)) | ||
| g.Expect(claimRules[0].Type).Should(Equal(configv1.TokenValidationRuleTypeCEL)) | ||
| g.Expect(claimRules[0].CEL.Expression).Should(Equal("has(claims.email) && claims.email != ''")) | ||
| t.Logf("Claim validation rules successfully validated token") | ||
| }) | ||
|
|
||
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test user validation rules", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to test user validation rules") | ||
| g.Expect(hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].UserValidationRules).NotTo(BeEmpty()) | ||
| // Authentication succeeded, so user validation rules passed | ||
| userRules := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0].UserValidationRules | ||
| g.Expect(userRules).Should(HaveLen(1)) | ||
| g.Expect(userRules[0].Expression).Should(Equal("!user.username.startsWith('system:')")) | ||
| // Verify the username doesn't start with 'system:' (which is what the rule checks) | ||
| g.Expect(selfSubjectReview.Status.UserInfo.Username).NotTo(HavePrefix("system:")) | ||
| t.Logf("User validation rules successfully validated user: %s", selfSubjectReview.Status.UserInfo.Username) | ||
| }) | ||
|
|
||
| t.Run("[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test authentication config is properly set", func(t *testing.T) { | ||
| g := NewWithT(t) | ||
| t.Logf("begin to verify authentication configuration") | ||
| oidcProvider := hostedCluster.Spec.Configuration.Authentication.OIDCProviders[0] | ||
|
|
||
| // Verify CEL expressions are set for username and groups | ||
| g.Expect(oidcProvider.ClaimMappings.Username.Expression).Should(Equal("claims.email.split('@')[0]")) | ||
| g.Expect(oidcProvider.ClaimMappings.Groups.Expression).Should(Equal("has(claims.groups) && type(claims.groups) == list ? claims.groups : []")) | ||
|
|
||
| // Verify claim validation rules are set | ||
| g.Expect(oidcProvider.ClaimValidationRules).Should(HaveLen(1)) | ||
| g.Expect(oidcProvider.ClaimValidationRules[0].Type).Should(Equal(configv1.TokenValidationRuleTypeCEL)) | ||
|
|
||
| // Verify user validation rules are set | ||
| g.Expect(oidcProvider.UserValidationRules).Should(HaveLen(1)) | ||
| g.Expect(oidcProvider.UserValidationRules[0].Expression).Should(Equal("!user.username.startsWith('system:')")) | ||
|
|
||
| t.Logf("All ExternalOIDCWithUpstreamParity configurations properly set") | ||
| }) | ||
| } |
There was a problem hiding this comment.
OK, I see what you mean. You're saying the last test referenced here combined the first two tests referenced. Yeah, that makes sense. I'll remove the last one. If you want them both combined, let me know. :)
What I was meaning is that the first 2 test cases:
- "[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL username expression mapping"
- "[OCPFeatureGate:ExternalOIDCWithUpstreamParity] Test CEL groups expression mapping"
Already inherently test what these last 2 tests are testing because you configure the claim validation and user validation rules for all the test cases.
I'm not sure it makes sense for us to retest this behavior.
There probably is a way to add negative test cases. However, I'm unsure how possible it is from the hypershift side because it all depends on what this script adds in terms of valid/invalid users.
As it currently stands, it adds/configures just valid users/groups. From what I understand, we would need to either add a new test or modify this script to configure these invalid users/groups so we can properly test extensively.
Okay, I've gone a bit back and forth on this throughout this review a touch already but now I'm thinking that if that script is presenting a limitation in what we can or cannot test I think modifying it as necessary is important to ensure we are getting the right level of test coverage here.
Because of this, we should include any modifications that need to happen there within scope of the testing work for HyperShift.
|
/uncc |
|
/test e2e-azure-aks-external-oidc-techpreview |
2 similar comments
|
/test e2e-azure-aks-external-oidc-techpreview |
|
/test e2e-azure-aks-external-oidc-techpreview |
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
cab0dd0 to
195f156
Compare
|
/test e2e-azure-aks-external-oidc-techpreview |
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
195f156 to
7f66a0d
Compare
|
/test e2e-azure-aks-external-oidc-techpreview |
| HTTPClient *http.Client | ||
| AdminUser string | ||
| AdminPass string | ||
| CACertFile string |
There was a problem hiding this comment.
Check if we actually need this ca cert because we don't use TLS when we specify via the API (we skip the check).
| @@ -347,3 +415,917 @@ func GetClientConfigForKeycloakOIDCUser(clientCfg *rest.Config, authConfig *ExtO | |||
|
|
|||
| return userClientConfig | |||
| } | |||
|
|
|||
| // KeycloakAdminClient provides methods to interact with Keycloak Admin REST API | |||
| type KeycloakAdminClient struct { | |||
There was a problem hiding this comment.
move the keycloak api related code to a separate file to keep things clean
| } | ||
|
|
||
| // NewTestResources creates a new TestResources tracker | ||
| func NewTestResources(adminClient *KeycloakAdminClient) *TestResources { |
There was a problem hiding this comment.
add a check to see if admin token expires and if so, refresh?
| "username should not contain @ symbol") | ||
| t.Logf("CEL username expression correctly mapped '%s' to '%s'", testUser.Email, testUser.SelfSubjectReview.Status.UserInfo.Username) | ||
|
|
||
| // Edge case test: preferred_username vs email-derived username mismatch |
There was a problem hiding this comment.
This part of the test - we can remove because it doesn't test our feature gate configurations. We are mostly concerned with happy mappings here and negative testing can be done in validations testing.
There was a problem hiding this comment.
actually fine to keep
| "forbidden user cannot authenticate as it violates user validation rule") | ||
| t.Logf("✓ User with 'forbidden' in username correctly rejected with error: %v", err) | ||
|
|
||
| // NOTE: We cannot test the negative case for the system: prefix rule via Keycloak |
There was a problem hiding this comment.
We can modify the config in external_oidc.go to have a rule that CAN map to system: for username regardless of email e.g. for a specific email keyword.
There was a problem hiding this comment.
Tie this in with #8287 (comment) as - if we can use bespoke config per test mentioned in #8287 (comment), we should be able to make this happen that way .
|
|
||
| // CEL expressions for user validation rules | ||
| UserValidationExprNoSystemPrefix = "!user.username.startsWith('system:')" | ||
| UserValidationExprNoForbiddenWord = "!user.username.contains('forbidden')" | ||
| ) | ||
|
|
||
| type ExtOIDCConfig struct { |
There was a problem hiding this comment.
We need to look at having a bespoke config per test so this won't fail when the feature gate graduates to default.
|
I think we will also need to do something similar to what we did in openshift/origin related to having a unique authentication configuration to be used for each test case so that we can control what the inputs are to every test case. Here is a link to the function we implemented to do this in o/origin: https://github.com/openshift/origin/blob/428b9a0e36ff2c43b2a956b12d22141a1c88b950/test/extended/authentication/oidc.go#L771 This might end up being a case where we will have to spin up multiple hypershift clusters, one for each configuration we would like to test. It looks like there is a way to get that done via modification of the cluster options passed into the |
…instance This commit adds the necessary functions to interact with the externally deployed Keycloak instance. The Keycloak instance is deployed at the link below. https://github.com/openshift/release/blob/main/ci-operator/step-registry/idp /external-oidc/keycloak/server/idp-external-oidc-keycloak-server-commands.sh Also adds user/group CRUD functionality to tests.
This commit configures the control plane operator feature sets in hypershift-operator. This change is required as hypershift-operator re-uses control-plane-operator auth config validation code, which checks specifically for control-plane-operator feature set enablement. Without this change, if we try to enable feature set such as TechPreviewNoUpgrade, validation will fail as hypershift-operator does not configure control-plane-operator feature sets. This change will allow us to test feature gates behind TechPreviewNoUpgrade and others that are present in control-plane-operator, throughout HyperShift.
This commit fixes failing tests for ExternalOIDCWithUpstreamParity and ExternalOIDCWithUIDAndExtraClaimMappings feature gates by wrapping feature gate specific test expectations behind if-statements. Also reworks ExternalOIDCWithUpstreamParity tests with new user/group CRUD framework. It also adds negative testing to existing tests. Additionally, AWS and AKS e2e support is added. Keycloak is deployed differently on both clusters and credentials are also stored differently. TryAuthenticateUser() now uses AnonymousClientConfig() instead of using clientcfg on its own, which effectively copied over admin token that was able to authenticate with kube-apiserver whenever it was used. This caused KAS logs to show selfsubjectreviews being successful for system:admin, because these weren't cleared. Now, only the specified user token should be used to identify the user for validation rule testing. It also adds check for `Unauthorized` to be present in error message, rather than relying on a more detailed error message to be thrown.
ehearne-redhat
force-pushed
the
external-oidc-test-featuregate-externaloidcwithupstreamparity
branch
from
7f66a0d to
aeda561
Compare
This change moves keycloak api code from `external_oidc.go` to its own file, `keycloak.go` . Also adds check for admin token expiry + refreshes if needed.
|
/test e2e-azure-aks-external-oidc-techpreview |
|
/test e2e-azure-aks-external-oidc-techpreview |
This change adds the ability to specify custom auth config for testing ExternalOIDC functionality, which will prove useful for when features behind feature gates graduate to default feature set.
This change experiments with custom auth config chage from previous commit by adding new NHT.Execute() with custom auth config specified.
|
/test e2e-azure-aks-external-oidc-techpreview |
|
/test e2e-aws-external-oidc-techpreview |
|
@ehearne-redhat: 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. |
| @@ -483,7 +483,11 @@ func (h *hypershiftTest) createHostedCluster(opts *PlatformAgnosticOptions, plat | |||
| if v.Spec.Configuration == nil { | |||
| v.Spec.Configuration = &hyperv1.ClusterConfiguration{} | |||
| } | |||
| v.Spec.Configuration.Authentication = opts.ExtOIDCConfig.GetAuthenticationConfig() | |||
There was a problem hiding this comment.
Could you just have the GetAuthenticationConfig() method return the right one?
| // custom field for adding custom auth configuration | ||
| CustomAuthSpec *configv1.AuthenticationSpec |
There was a problem hiding this comment.
We probably don't want to have a full override here where we have to define the entire spec including issuers, clients, etc. We just want the ability to modify a given baseline configuration for each test case.
|
Now I have all the evidence. Let me produce the final report. Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe second HostedCluster created by Root CauseThe root cause is a test name that is too long for the AWS IAM role name constraint. The chain is:
The Recommendations
Evidence
|
4 participants
What this PR does / why we need it:
This change:
Adds extensive testing for the ExternalOIDCWithUpstreamParity feature gate in
control-plane-operator/controllers/hostedcontrolplane/v2/kas/auth_test.go.Tests the feature gate in a HyperShift cluster in
test/e2e/external_oidc_test.go.Provides basic auth config testing of the feature gate in
test/e2e/util/external_oidc.go.Configures the control plane operator feature sets in hypershift-operator.
This change is required as hypershift-operator re-uses control-plane-operator
auth config validation code, which checks specifically for control-plane-operator
feature set enablement.
Without this change, if we try to enable feature set such as TechPreviewNoUpgrade,
validation will fail as hypershift-operator does not configure control-plane-operator
feature sets.
This change will allow us to test feature gates behind TechPreviewNoUpgrade and
others that are present in control-plane-operator, throughout HyperShift.
This should allow us to progress in promotion of the feature from TechPreview to GA.
Which issue(s) this PR fixes:
https://redhat.atlassian.net/browse/CNTRLPLANE-3306.
Allows us to progress in promotion of the ExternalOIDCWithUpstreamParity feature from TechPreview to GA.
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
New Features
Tests