CNTRLPLANE-2552: Add support for CEL expression claim mappings for username and groups#850
CNTRLPLANE-2552: Add support for CEL expression claim mappings for username and groups#850ShazaAldawamneh wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds feature-gate aware CEL expression support and upstream-parity validations to External OIDC claim mapping generation. CEL expressions for username/groups and claim validation rules are compiled and inspected (email_verified checks); compilation results are propagated and errors can block JWT authenticator construction. Tests expanded for expression and error cases. Changes
Sequence DiagramsequenceDiagram
rect rgba(220,220,255,0.5)
participant Controller as External OIDC Controller
end
rect rgba(220,255,220,0.5)
participant FG as Feature Gate System
participant Validator as CEL Validator
participant Generator as Mapping Generators
participant Auth as JWT Authenticator Builder
end
Controller->>FG: Check feature gates (UpstreamParity, AdditionalClaimMappings)
alt UpstreamParity enabled
Controller->>Generator: generateUsernameClaimMapping(spec, featureGates)
Generator->>Validator: compile/validate CEL expr (if provided)
Validator-->>Generator: compilation result / error
Generator-->>Controller: PrefixedClaimOrExpression + compilation result or error
Controller->>Generator: generateGroupsClaimMapping(spec, featureGates)
Generator->>Validator: compile/validate CEL expr (if provided)
Validator-->>Generator: compilation result / error
Generator-->>Controller: PrefixedClaimOrExpression + compilation result or error
Controller->>Generator: generateClaimValidationRules(...)
Generator->>Validator: compile/validate rule expressions
Validator-->>Generator: compilation results / errors
Generator-->>Controller: rules + compilation results
else UpstreamParity disabled
Controller->>Generator: generate legacy claim mappings
Generator-->>Controller: mappings
end
alt AdditionalClaimMappings enabled
Controller->>Generator: generate UID/Extra mappings
Generator-->>Controller: additional mappings
end
Controller->>Auth: Build JWTAuthenticator with mappings + CEL compilation results
Auth-->>Controller: success or propagated error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ShazaAldawamneh 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 |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@pkg/controllers/externaloidc/externaloidc_controller_test.go`:
- Around line 1219-1326: The feature-gate slices are inverted in the test cases:
NewFeatureGate(enabled, disabled) is being called with the gates in the disabled
slice, so the features are actually turned off. Fix the four cases ("auth config
with username expression mapping", "auth config with groups expression mapping",
"auth config with username claim and expression both set, error", "auth config
with groups claim and expression both set, error") by passing the gates
FeatureGateExternalOIDCWithAdditionalClaimMappings and
FeatureGateExternalOIDCWithUpstreamParity in the first (enabled) slice to
NewFeatureGate (and the second slice should be empty), so the expression-mapping
and error paths exercise the intended feature gates.
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 310-315: The early return in generateUsernameClaimMapping skips
CEL compilation and the claim/expression exclusivity checks, allowing invalid
claim+expression combos and empty Claim values; update
generateUsernameClaimMapping (and the similar block around the groups handling)
to mirror generateUIDClaimMapping: when the feature gate allows Expression and
usernameClaimMapping.Expression is set, compile/validate the CEL expression
(handle and return compilation errors) and run the same exclusivity logic that
ensures Claim and Expression are not both set and Claim is not left empty;
ensure any path that returns an apiserverv1beta1.PrefixedClaimOrExpression
validates the expression and enforces the mutual-exclusivity rules before
returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10e30e31-927a-452f-9da1-31bb2b1f4057
⛔ Files ignored due to path filters (14)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis 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/**,!**/zz_generated*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/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/**,!**/zz_generated*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/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (3)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.go
|
@ShazaAldawamneh - looks like those failing tests are broken. You'll probably need to re-run those once you know the tests are stable again. |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
374-379: Groups mapping lacks the explicit claim validation that username mapping has; consider adding defense-in-depth validation.Unlike
generateUsernameClaimMappingwhich rejects an empty claim (lines 327-329),generateGroupsClaimMappingsilently accepts whatever is in the claim field, including empty string. When the feature gate is disabled and a configuration contains only an expression (with empty claim), the expression is ignored without warning.While the CRD validation layer should prevent such configurations from being created (line 362 of types_authentication.go requires claim when the feature gate is disabled), adding explicit validation in the controller—similar to username mapping—would provide consistency and defense-in-depth. This is particularly relevant given that groups mappings can be configured with expressions when the feature gate is enabled.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 374 - 379, The groups mapping handler (generateGroupsClaimMapping) currently assigns out.Claim = groupsMapping.Claim without validating it's non-empty; add the same defense-in-depth check used by generateUsernameClaimMapping to reject an empty claim when the expression feature gate is disabled. Detect the feature gate state and if disabled and groupsMapping.Claim is empty return an error (or the same validation error path used by generateUsernameClaimMapping) instead of silently accepting it, keeping the existing assignment of out.Prefix and return behavior otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 374-379: The groups mapping handler (generateGroupsClaimMapping)
currently assigns out.Claim = groupsMapping.Claim without validating it's
non-empty; add the same defense-in-depth check used by
generateUsernameClaimMapping to reject an empty claim when the expression
feature gate is disabled. Detect the feature gate state and if disabled and
groupsMapping.Claim is empty return an error (or the same validation error path
used by generateUsernameClaimMapping) instead of silently accepting it, keeping
the existing assignment of out.Prefix and return behavior otherwise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d13552e-d253-4056-b6c4-5a1b11494c65
📒 Files selected for processing (2)
pkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.go
ehearne-redhat
left a comment
ehearne-redhat
left a comment
There was a problem hiding this comment.
Looks good to me. I'll leave @everettraven give the lgtm . :)
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/controllers/externaloidc/externaloidc_controller.go (2)
364-385:⚠️ Potential issue | 🟠 Major
generateGroupsClaimMappingstill accepts disabled or empty configs.If
Expressionis set while upstream parity is off, or if bothClaimandExpressionare empty, this helper falls through and returnsClaim: "". That makes the gate non-strict and can serialize an invalid groups mapping; mirror the username path by rejecting gated-off expressions and requiring a non-empty claim on the non-expression path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 364 - 385, In generateGroupsClaimMapping, reject cases where Expression is set while the FeatureGate features.FeatureGateExternalOIDCWithUpstreamParity is disabled and also require a non-empty Claim when not using an Expression (i.e., do not fall through to return Claim:""). Specifically, in function generateGroupsClaimMapping validate that if feature gate is disabled and groupsMapping.Expression != "" you return an error (similar message to the username path), and when choosing the non-expression path ensure groupsMapping.Claim is non-empty (return a clear error if both Claim and Expression are empty); continue to validate CEL expressions via validateCELExpression(authenticationcel.ClaimMappingExpression{...}) when the gate allows expressions.
318-330:⚠️ Potential issue | 🟠 MajorReject prefix configuration on expression mappings instead of dropping it.
Both expression branches return before any prefix validation runs, so
username.prefix*/groups.prefixcan be set in the CR and then silently disappear from the renderedJWTAuthenticator. Please fail fast on those combinations rather than accepting and rewriting the input.Also applies to: 338-358, 366-378
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 318 - 330, When a ClaimMapping uses an Expression we must reject any prefix settings instead of returning early and dropping them; update the blocks that handle usernameClaimMapping.Expression (and the analogous groups mapping branches) to check for a non-empty Prefix field (e.g. usernameClaimMapping.Prefix and groupsClaimMapping.Prefix) before validating the CEL expression and return an error like "prefix must not be set when expression is used" if present; keep the CEL validation via validateCELExpression and the existing out.Expression assignment but only after this prefix check, and apply the same change to the other expression branches referenced (the ones around the handling of groups mapping and the additional blocks at the other ranges).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 198-201: validateEmailVerifiedUsage currently inspects
claimMappings.Extra unconditionally which allows gated-out Extra mappings to
satisfy the email_verified requirement; update the check so Extra is only
considered when the additional-claim-mappings feature is actually enabled
(FeatureGateExternalOIDCWithAdditionalClaimMappings) or when ClaimMappings was
generated with Extra present. Concretely, modify validateEmailVerifiedUsage to
accept a flag or check the feature gate and only scan claimMappings.Extra (and
any Extra-derived expressions) when
FeatureGateExternalOIDCWithAdditionalClaimMappings is true (or when
generateClaimMappings produced out.Extra); keep existing checks for
claimMappings.Username/Email/ClaimValidationRules intact. Reference:
validateEmailVerifiedUsage, generateClaimMappings,
FeatureGateExternalOIDCWithAdditionalClaimMappings, provider.ClaimMappings,
claimMappings.Extra.
---
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 364-385: In generateGroupsClaimMapping, reject cases where
Expression is set while the FeatureGate
features.FeatureGateExternalOIDCWithUpstreamParity is disabled and also require
a non-empty Claim when not using an Expression (i.e., do not fall through to
return Claim:""). Specifically, in function generateGroupsClaimMapping validate
that if feature gate is disabled and groupsMapping.Expression != "" you return
an error (similar message to the username path), and when choosing the
non-expression path ensure groupsMapping.Claim is non-empty (return a clear
error if both Claim and Expression are empty); continue to validate CEL
expressions via
validateCELExpression(authenticationcel.ClaimMappingExpression{...}) when the
gate allows expressions.
- Around line 318-330: When a ClaimMapping uses an Expression we must reject any
prefix settings instead of returning early and dropping them; update the blocks
that handle usernameClaimMapping.Expression (and the analogous groups mapping
branches) to check for a non-empty Prefix field (e.g.
usernameClaimMapping.Prefix and groupsClaimMapping.Prefix) before validating the
CEL expression and return an error like "prefix must not be set when expression
is used" if present; keep the CEL validation via validateCELExpression and the
existing out.Expression assignment but only after this prefix check, and apply
the same change to the other expression branches referenced (the ones around the
handling of groups mapping and the additional blocks at the other ranges).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ca5658f7-01a3-4bff-aecf-a37dd1b753f1
📒 Files selected for processing (1)
pkg/controllers/externaloidc/externaloidc_controller.go
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
680-688: PreferusernameResultas the single source of truth invalidateEmailVerifiedUsage.Passing both the raw expression string and compilation result is redundant. This can be simplified to reduce future drift risk.
♻️ Proposed refactor
func validateEmailVerifiedUsage( - usernameExpression string, usernameResult *authenticationcel.CompilationResult, extraResults []authenticationcel.CompilationResult, claimValidationResults []authenticationcel.CompilationResult, ) error { - if usernameExpression == "" || usernameResult == nil { + if usernameResult == nil { return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller.go` around lines 680 - 688, The function validateEmailVerifiedUsage currently accepts both usernameExpression and usernameResult which is redundant and risks drift; change validateEmailVerifiedUsage to take only usernameResult (remove the usernameExpression parameter), derive the original expression string from usernameResult (use the field/method on authenticationcel.CompilationResult that holds the source/expr) and update all callers to pass the compilation result only; ensure any logic that checks for empty expression uses the value obtained from usernameResult and keep the rest of validation unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 680-688: The function validateEmailVerifiedUsage currently accepts
both usernameExpression and usernameResult which is redundant and risks drift;
change validateEmailVerifiedUsage to take only usernameResult (remove the
usernameExpression parameter), derive the original expression string from
usernameResult (use the field/method on authenticationcel.CompilationResult that
holds the source/expr) and update all callers to pass the compilation result
only; ensure any logic that checks for empty expression uses the value obtained
from usernameResult and keep the rest of validation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8433bee6-cfb0-4b39-bcc2-ba25bb0b7d93
📒 Files selected for processing (2)
pkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controllers/externaloidc/externaloidc_controller_test.go
|
As far as I can tell, the changes look good to me. However, you may need to run < google.golang.org/genproto/googleapis/api v0.0.0-20250303144028-a0af3efb3deb // indirect
`go.mod` content is incorrect - did you run `go mod tidy`?It still looks like the tests are broken. :) |
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-2552 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. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Another handful of comments. Overall I think this is pretty good.
A couple of the comments are more nit-like suggestions that I'll leave it up to you whether or not you want to take. I'm not strongly opinionated enough on the nits to require the changes be made to merge this PR.
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2552
branch
from
9abd662 to
5ddf230
Compare
| // TODO: update the API-side validations to ensure that prefixPolicy is set to | ||
| // NoPrefix or omitted when expression is set, for stronger admission time | ||
| // validation prior to GA. |
There was a problem hiding this comment.
We don't need a TODO comment here for this. Instead, can you create a Jira ticket to track that we need to do this and remove this comment?
|
@ShazaAldawamneh: 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. |
…l_verified enforcement and prefix validation Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-2552
branch
from
5ddf230 to
925fab2
Compare
4 participants
This PR updates to support the newly introduced feature-gated fields in the Authentication configuration API added by openshift/api#2719. It bumps the openshift/api dependency to consume the updated API types and ensures the new fields are correctly pulled into the CAO APIs.
The implementation ensures that the new behaviour is strictly gated and only activated when the TechPreview feature gate is explicitly enabled.
Summary by CodeRabbit
New Features
Improvements
Tests
Chores