CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR#810
CNTRLPLANE-312: Add generation logic for new API fields in the Authentication CR#810ShazaAldawamneh wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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.21.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:
WalkthroughAdds upstream-parity support to External OIDC: wires user validation rules into JWT generation (behind FeatureGateExternalOIDCWithUpstreamParity), adds CEL claim validation handling, forwards issuer DiscoveryURL, extends operator gating, updates Changes
Sequence Diagram(s)sequenceDiagram
participant Operator
participant Controller
participant Issuer
participant APIServer
Operator->>Controller: enable feature gates (ExternalOIDC, AdditionalClaimMappings, UpstreamParity)
Controller->>Issuer: read issuer configuration (issuer, mappings, discoveryURL, validation rules)
Issuer-->>Controller: return issuer config
Controller->>Controller: validate claim rules (including CEL) and user validation rules
Controller->>APIServer: generate/apply JWT authenticator (Issuer, ClaimValidationRules, UserValidationRules, DiscoveryURL)
APIServer-->>Controller: confirm apply/store
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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
the
do-not-merge/work-in-progress
|
[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 |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Overall, the structure looks pretty good.
I have a handful of comments.
|
/retest-required |
|
/retest-required |
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
82a054d to
4cb42b2
Compare
openshift-merge-robot
added
the
needs-rebase
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
4cb42b2 to
ffbc0c5
Compare
openshift-merge-robot
removed
the
needs-rebase
ShazaAldawamneh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
A few minor things. Other than that, this LGTM
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
407-418:⚠️ Potential issue | 🟠 MajorEnforce UpstreamParity gate for CEL claim validation rules.
Line 407 ignores
featureGates, soTokenValidationRuleTypeCELrules can be emitted even whenFeatureGateExternalOIDCWithUpstreamParityis off. This bypasses feature gating for new fields.🛠️ Suggested fix
func generateClaimValidationRules(featureGates featuregates.FeatureGate, claimValidationRules ...configv1.TokenClaimValidationRule) ([]apiserverv1beta1.ClaimValidationRule, error) { out := []apiserverv1beta1.ClaimValidationRule{} errs := []error{} for _, claimValidationRule := range claimValidationRules { + if claimValidationRule.Type == configv1.TokenValidationRuleTypeCEL && + !featureGates.Enabled(features.FeatureGateExternalOIDCWithUpstreamParity) { + errs = append(errs, fmt.Errorf("claimValidationRule.type %q requires %s feature gate", configv1.TokenValidationRuleTypeCEL, features.FeatureGateExternalOIDCWithUpstreamParity)) + continue + } rule, err := generateClaimValidationRule(claimValidationRule) if err != nil { errs = append(errs, fmt.Errorf("generating claimValidationRule: %v", err)) continue }🤖 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 407 - 418, The loop in generateClaimValidationRules ignores the featureGates parameter and may emit CEL-based rules regardless of the FeatureGateExternalOIDCWithUpstreamParity flag; update generateClaimValidationRules to check featureGates.Has(featuregates.FeatureGateExternalOIDCWithUpstreamParity) (or equivalent) before appending any rule whose type is TokenValidationRuleTypeCEL (as determined from generateClaimValidationRule or the returned rule.Type), and if the gate is off either skip that rule or return a descriptive error; reference generateClaimValidationRules, generateClaimValidationRule, TokenValidationRuleTypeCEL, and FeatureGateExternalOIDCWithUpstreamParity to locate and implement the gate check.
🤖 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 226-228: The code currently sets out.DiscoveryURL when
issuer.DiscoveryURL is non-empty; instead validate the URL scheme (parse
issuer.DiscoveryURL and ensure scheme == "https") before assigning to
out.DiscoveryURL and return the appropriate error for non-HTTPS or invalid URLs;
update the branch around issuer.DiscoveryURL and out.DiscoveryURL so it parses
the value (using net/url.Parse), checks u.Scheme == "https", and only then sets
out.DiscoveryURL = &issuer.DiscoveryURL, otherwise return the “invalid discovery
URL (http)” style error.
---
Outside diff comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 407-418: The loop in generateClaimValidationRules ignores the
featureGates parameter and may emit CEL-based rules regardless of the
FeatureGateExternalOIDCWithUpstreamParity flag; update
generateClaimValidationRules to check
featureGates.Has(featuregates.FeatureGateExternalOIDCWithUpstreamParity) (or
equivalent) before appending any rule whose type is TokenValidationRuleTypeCEL
(as determined from generateClaimValidationRule or the returned rule.Type), and
if the gate is off either skip that rule or return a descriptive error;
reference generateClaimValidationRules, generateClaimValidationRule,
TokenValidationRuleTypeCEL, and FeatureGateExternalOIDCWithUpstreamParity to
locate and implement the gate check.
|
/retest |
|
/retest-required |
|
/retest |
|
Looks like the failing non-required jobs are related to in-development tests. I'm not sure we need to be concerned about breaking them at the moment, especially because all of the other jobs are passing fine. |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from this still piping through unused feature gate parameters, the changes LGTM.
openshift-merge-robot
added
the
needs-rebase
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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: 2
♻️ Duplicate comments (1)
pkg/controllers/externaloidc/externaloidc_controller.go (1)
226-228:⚠️ Potential issue | 🟠 MajorValidate DiscoveryURL scheme before setting it.
Line 226 currently accepts any non-empty value; this allows insecure or malformed endpoints and conflicts with the “invalid discovery URL (http)” expectation.
🔒 Proposed fix
- if len(issuer.DiscoveryURL) > 0 { - out.DiscoveryURL = &issuer.DiscoveryURL - } + if len(issuer.DiscoveryURL) > 0 { + parsed, err := url.Parse(issuer.DiscoveryURL) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return apiserverv1beta1.Issuer{}, fmt.Errorf("discoveryURL must be a valid https URL: %q", issuer.DiscoveryURL) + } + out.DiscoveryURL = &issuer.DiscoveryURL + }🤖 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 226 - 228, Validate the issuer.DiscoveryURL before assigning it to out.DiscoveryURL: parse issuer.DiscoveryURL (e.g., using url.Parse), ensure it is a well-formed URL and that its scheme is "https" (reject "http" or other schemes), and only set out.DiscoveryURL = &issuer.DiscoveryURL when the parse succeeds and scheme == "https"; if parsing fails or the scheme is not "https", return or surface an appropriate validation error instead of assigning the value (refer to issuer.DiscoveryURL and out.DiscoveryURL in externaloidc_controller.go).
🤖 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 463-492: The generateUserValidationRule functions currently don't
validate CEL expressions; update generateUserValidationRule to compile the
rule.Expression using the authenticationcel.UserValidationCondition and the
compiler. Use the compiler.CompileUserExpression(...,
authenticationcel.UserValidationCondition) call (matching the claim validation
pattern) and return a descriptive error if compilation fails; keep
generateUserValidationRules behavior but propagate compilation errors so invalid
expressions are rejected at config time.
- Around line 441-454: The code is using
authenticationcel.ClaimMappingExpression (meant for claim mappings returning
AnyType) when validating claimValidationRule.CEL.Expression; change the
validation to use authenticationcel.ClaimValidationCondition (which returns
BoolType) instead and pass the value (not a pointer) from
claimValidationRule.CEL into validateCELExpression. Specifically, update the
call to validateCELExpression to construct an
authenticationcel.ClaimValidationCondition with Expression set from
claimValidationRule.CEL.Expression (and Message if needed), keep the existing
error handling, and leave out.Expression and out.Message assignments as they
are.
---
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 226-228: Validate the issuer.DiscoveryURL before assigning it to
out.DiscoveryURL: parse issuer.DiscoveryURL (e.g., using url.Parse), ensure it
is a well-formed URL and that its scheme is "https" (reject "http" or other
schemes), and only set out.DiscoveryURL = &issuer.DiscoveryURL when the parse
succeeds and scheme == "https"; if parsing fails or the scheme is not "https",
return or surface an appropriate validation error instead of assigning the value
(refer to issuer.DiscoveryURL and out.DiscoveryURL in
externaloidc_controller.go).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
pkg/controllers/externaloidc/externaloidc_controller.go
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
22caac5 to
70daaf2
Compare
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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.
♻️ Duplicate comments (3)
pkg/controllers/externaloidc/externaloidc_controller.go (3)
463-471:⚠️ Potential issue | 🟠 Major
generateUserValidationRuleperforms no CEL expression validation.Only a non-empty check is present; invalid CEL will only surface at runtime during token evaluation. This was flagged in a previous review and remains unaddressed.
An important additional constraint: the existing
validateCELExpressionhelper (line 607) callsCompileClaimsExpression, which compiles in the JWT-claims CEL environment. User validation rules evaluate against theuser(authentication.k8s.io/v1 UserInfo) object, so a separate helper callingcompiler.CompileUserExpressionwithauthenticationcel.UserValidationConditionis required — reusingvalidateCELExpressiondirectly would produce incorrect compilation semantics.🛠️ Suggested approach
Add a dedicated helper and call it from
generateUserValidationRule:+func validateUserValidationCELExpression(expressionAccessor authenticationcel.ExpressionAccessor) error { + _, err := authenticationcel.NewDefaultCompiler().CompileUserExpression(expressionAccessor) + return err +}func generateUserValidationRule(rule configv1.TokenUserValidationRule) (apiserverv1beta1.UserValidationRule, error) { if len(rule.Expression) == 0 { return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("userValidationRule expression must be non-empty") } + if err := validateUserValidationCELExpression(&authenticationcel.UserValidationCondition{ + Expression: rule.Expression, + }); err != nil { + return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err) + } return apiserverv1beta1.UserValidationRule{ Expression: rule.Expression, Message: rule.Message, }, nil }Run the following to confirm that
CompileUserExpressionandUserValidationConditionare available in the vendoredauthenticationcelpackage:#!/bin/bash # Description: Verify CompileUserExpression and UserValidationCondition exist in the vendored authenticationcel package. rg -n "CompileUserExpression\|UserValidationCondition" vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go🤖 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 463 - 471, generateUserValidationRule currently only checks for non-empty Expression and doesn't validate the CEL against the UserInfo environment; add a new helper (e.g., validateUserCELExpression) that calls compiler.CompileUserExpression with authenticationcel.UserValidationCondition to compile the rule expression, and invoke this helper from generateUserValidationRule to return any compilation error instead of deferring failures to runtime; do not reuse validateCELExpression (it uses CompileClaimsExpression), and ensure generateUserValidationRule propagates the compile error and only returns the apiserverv1beta1.UserValidationRule when compilation succeeds.
447-451:⚠️ Potential issue | 🟠 Major
ClaimMappingExpressionis the wrong CEL accessor type for claim validation rules.
ClaimMappingExpressioncompiles the expression expectingAnyTypereturn (it is designed for claim-to-value mappings). A claim validation rule must evaluate to a boolean; the correct accessor isauthenticationcel.ClaimValidationCondition, which instructs the compiler to enforceBoolType. Using the mapping type here means a CEL expression returning a non-boolean (e.g., a string) will pass compile-time validation but fail at runtime token evaluation.🛠️ Suggested fix
- if err := validateCELExpression(&authenticationcel.ClaimMappingExpression{ - Expression: claimValidationRule.CEL.Expression, - }); err != nil { + if err := validateCELExpression(&authenticationcel.ClaimValidationCondition{ + Expression: claimValidationRule.CEL.Expression, + }); err != 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 447 - 451, The code uses authenticationcel.ClaimMappingExpression when calling validateCELExpression for claimValidationRule, which allows non-boolean results; change the CEL accessor to authenticationcel.ClaimValidationCondition so the compiler enforces BoolType. Concretely, update the validateCELExpression call that currently constructs &authenticationcel.ClaimMappingExpression{Expression: claimValidationRule.CEL.Expression} to instead construct &authenticationcel.ClaimValidationCondition{Expression: claimValidationRule.CEL.Expression} (leave validateCELExpression and claimValidationRule references unchanged).
226-228:⚠️ Potential issue | 🟠 Major
DiscoveryURLis propagated without HTTPS scheme validation.Any non-empty string passes — including
http://URLs — which allows insecure metadata endpoints to be accepted silently. This was flagged in a prior review and remains unresolved.🛠️ Suggested fix
if len(issuer.DiscoveryURL) > 0 { - out.DiscoveryURL = &issuer.DiscoveryURL + parsed, err := url.Parse(issuer.DiscoveryURL) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return apiserverv1beta1.Issuer{}, fmt.Errorf("discoveryURL must be a valid https URL: %q", issuer.DiscoveryURL) + } + out.DiscoveryURL = &issuer.DiscoveryURL }🤖 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 226 - 228, The DiscoveryURL is assigned without validating its scheme; update the logic around issuer.DiscoveryURL → out.DiscoveryURL in externaloidc_controller.go to only propagate non-empty values that use the HTTPS scheme (e.g., validate that issuer.DiscoveryURL starts with "https://" or parse with url.Parse and ensure Scheme == "https"); if the URL is not HTTPS, do not set out.DiscoveryURL and optionally surface/return an error or log a warning from the surrounding reconcile/validation function so insecure HTTP endpoints are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 463-471: generateUserValidationRule currently only checks for
non-empty Expression and doesn't validate the CEL against the UserInfo
environment; add a new helper (e.g., validateUserCELExpression) that calls
compiler.CompileUserExpression with authenticationcel.UserValidationCondition to
compile the rule expression, and invoke this helper from
generateUserValidationRule to return any compilation error instead of deferring
failures to runtime; do not reuse validateCELExpression (it uses
CompileClaimsExpression), and ensure generateUserValidationRule propagates the
compile error and only returns the apiserverv1beta1.UserValidationRule when
compilation succeeds.
- Around line 447-451: The code uses authenticationcel.ClaimMappingExpression
when calling validateCELExpression for claimValidationRule, which allows
non-boolean results; change the CEL accessor to
authenticationcel.ClaimValidationCondition so the compiler enforces BoolType.
Concretely, update the validateCELExpression call that currently constructs
&authenticationcel.ClaimMappingExpression{Expression:
claimValidationRule.CEL.Expression} to instead construct
&authenticationcel.ClaimValidationCondition{Expression:
claimValidationRule.CEL.Expression} (leave validateCELExpression and
claimValidationRule references unchanged).
- Around line 226-228: The DiscoveryURL is assigned without validating its
scheme; update the logic around issuer.DiscoveryURL → out.DiscoveryURL in
externaloidc_controller.go to only propagate non-empty values that use the HTTPS
scheme (e.g., validate that issuer.DiscoveryURL starts with "https://" or parse
with url.Parse and ensure Scheme == "https"); if the URL is not HTTPS, do not
set out.DiscoveryURL and optionally surface/return an error or log a warning
from the surrounding reconcile/validation function so insecure HTTP endpoints
are rejected.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (32)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.coderabbit.yamlis 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/**,!**/zz_generated*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/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/register.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/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*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/**,!**/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/features/legacyfeaturegates.gois 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/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_network.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_70_network_01_networks.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfigurations-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/library-go/pkg/crypto/crypto.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- pkg/operator/starter.go
- pkg/controllers/externaloidc/externaloidc_controller_test.go
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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. |
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
ShazaAldawamneh
force-pushed
the
CNTRLPLANE-312-L
branch
from
6871f41 to
8b5fbf8
Compare
openshift-merge-robot
removed
the
needs-rebase
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-312 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: 3
♻️ Duplicate comments (2)
pkg/controllers/externaloidc/externaloidc_controller_test.go (1)
1124-1141:⚠️ Potential issue | 🟠 MajorTest never reaches user-validation-rule logic due to missing CA bundle.
generateJWTForProvidercallsgenerateIssuerfirst; withcaBundleConfigMapabsent the CA lookup foroidc-ca-bundlefails and the function returns beforegenerateUserValidationRules(lines 196-202 of the controller) is ever invoked. The test'sexpectError: trueis satisfied by the wrong error.Add
caBundleConfigMap: &baseCABundleConfigMap(and setIssuer.URLto skip the live-URL validation) so the test actually exercises the empty-expression guard ingenerateUserValidationRule.🛠️ Suggested fix
{ name: "user validation rule invalid expression", + caBundleConfigMap: &baseCABundleConfigMap, auth: *authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){ func(auth *configv1.Authentication) { + auth.Spec.OIDCProviders[0].Issuer.URL = "https://example.com" auth.Spec.OIDCProviders[0].UserValidationRules = []configv1.TokenUserValidationRule{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/externaloidc/externaloidc_controller_test.go` around lines 1124 - 1141, The test case for "user validation rule invalid expression" is returning early due to a missing CA bundle so generateIssuer exits before generateUserValidationRules runs; update the test's test vector to include caBundleConfigMap: &baseCABundleConfigMap and set the auth fixture's Issuer.URL to a non-empty/safe value so live-URL validation is skipped, ensuring generateJWTForProvider calls generateUserValidationRules and the empty Expression guard in generateUserValidationRule (functions generateJWTForProvider, generateIssuer, generateUserValidationRules) is exercised and the expectError reflects that validation.pkg/controllers/externaloidc/externaloidc_controller.go (1)
226-228:⚠️ Potential issue | 🟠 MajorDiscoveryURL assigned without HTTPS scheme validation.
Any non-empty string (including
http://or malformed URLs) is forwarded toout.DiscoveryURL. This was flagged in the prior review cycle and agreed upon, but remains unimplemented. The new test"invalid discovery URL (http instead of https)"is predicated on this check existing.
net/urlis already imported; the fix is straightforward:🛠️ Proposed fix
- if len(issuer.DiscoveryURL) > 0 { - out.DiscoveryURL = &issuer.DiscoveryURL - } + if len(issuer.DiscoveryURL) > 0 { + parsed, err := url.Parse(issuer.DiscoveryURL) + if err != nil || parsed.Scheme != "https" || parsed.Host == "" { + return apiserverv1beta1.Issuer{}, fmt.Errorf("discoveryURL must be a valid https URL: %q", issuer.DiscoveryURL) + } + out.DiscoveryURL = &issuer.DiscoveryURL + }🤖 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 226 - 228, The assignment to out.DiscoveryURL must validate issuer.DiscoveryURL is a well-formed HTTPS URL; update the block that currently checks len(issuer.DiscoveryURL) and sets out.DiscoveryURL to instead parse issuer.DiscoveryURL with net/url.Parse, ensure url.Scheme == "https" and url.Host is non-empty, and only then set out.DiscoveryURL = &issuer.DiscoveryURL; keep the value unset for non-HTTPS or malformed URLs (this will satisfy the "invalid discovery URL (http instead of https)" test).
🤖 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 1092-1104: The test case is failing for the wrong reason because
no CA bundle is provided and the controller lacks DiscoveryURL scheme
validation; update the test case entry (the table row with name "invalid
discovery URL (http instead of https)") to include caBundleConfigMap:
&baseCABundleConfigMap and set a valid Issuer.URL so generateIssuer runs past CA
lookup, and then implement a scheme check in the controller where DiscoveryURL
is assigned (the code path around generateIssuer and the assignment to
auth.Spec.OIDCProviders[0].Issuer.DiscoveryURL — see generateIssuer and the
controller lines that set DiscoveryURL) to reject or error on non-HTTPS schemes
(i.e., require URL.Scheme == "https") so the test actually triggers the intended
validation error.
- Around line 1105-1123: This test is a duplicate that fails for the wrong
reason; either remove the test case named "claim validation rule missing
required claim" or modify its auth fixture so it actually exercises
claim-validation (not CA lookup) by adding caBundleConfigMap:
&baseCABundleConfigMap to the auth produced by authWithUpdates (or otherwise
ensure a valid CA bundle) so generateIssuer succeeds and
generateClaimValidationRules runs; keep the RequiredClaim: nil to preserve the
intended assertion if you choose to keep and rename the case to avoid
duplication with the existing "auth config with nil claim in validation rule"
test.
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 463-479: The helper validateCELExpression currently always calls
compiler.CompileClaimsExpression which is incorrect for user rules; update
validation to call compiler.CompileUserExpression when given an
authenticationcel.UserValidationCondition (and keep CompileClaimsExpression for
claim conditions), or create a dedicated validateCELUserExpression that calls
CompileUserExpression for UserValidationCondition and use that from
generateUserValidationRule; reference validateCELExpression,
authenticationcel.UserValidationCondition, compiler.CompileUserExpression and
compiler.CompileClaimsExpression to locate and change the call routing so user
expressions are compiled in the user.* CEL environment.
---
Duplicate comments:
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go`:
- Around line 1124-1141: The test case for "user validation rule invalid
expression" is returning early due to a missing CA bundle so generateIssuer
exits before generateUserValidationRules runs; update the test's test vector to
include caBundleConfigMap: &baseCABundleConfigMap and set the auth fixture's
Issuer.URL to a non-empty/safe value so live-URL validation is skipped, ensuring
generateJWTForProvider calls generateUserValidationRules and the empty
Expression guard in generateUserValidationRule (functions
generateJWTForProvider, generateIssuer, generateUserValidationRules) is
exercised and the expectError reflects that validation.
In `@pkg/controllers/externaloidc/externaloidc_controller.go`:
- Around line 226-228: The assignment to out.DiscoveryURL must validate
issuer.DiscoveryURL is a well-formed HTTPS URL; update the block that currently
checks len(issuer.DiscoveryURL) and sets out.DiscoveryURL to instead parse
issuer.DiscoveryURL with net/url.Parse, ensure url.Scheme == "https" and
url.Host is non-empty, and only then set out.DiscoveryURL =
&issuer.DiscoveryURL; keep the value unset for non-HTTPS or malformed URLs (this
will satisfy the "invalid discovery URL (http instead of https)" test).
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (4)
go.modpkg/controllers/externaloidc/externaloidc_controller.gopkg/controllers/externaloidc/externaloidc_controller_test.gopkg/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/starter.go
- go.mod
| { | ||
| name: "invalid discovery URL (http instead of https)", | ||
| auth: *authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){ | ||
| func(auth *configv1.Authentication) { | ||
| auth.Spec.OIDCProviders[0].Issuer.DiscoveryURL = "http://insecure-url.com" | ||
| }, | ||
| }), | ||
| expectError: true, | ||
| featureGates: featuregates.NewFeatureGate( | ||
| []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, | ||
| []configv1.FeatureGateName{}, | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Test triggers the wrong error and controller lacks DiscoveryURL scheme validation.
Two compounding issues:
-
No
caBundleConfigMapis provided, sogenerateIssuerwill fail on the CA bundle lookup (oidc-ca-bundleabsent from indexer) before reaching the DiscoveryURL assignment at line 226 of the controller. The test getsexpectError: truesatisfied, but for the wrong reason. -
The controller code (lines 226-228) unconditionally assigns any non-empty
DiscoveryURLwithout an HTTPS scheme check, so even with a proper CA bundle thehttp://insecure-url.comvalue would be accepted silently — the test would then become a false negative.
To make this test meaningful: add caBundleConfigMap: &baseCABundleConfigMap to the test case, set a valid Issuer.URL, and ensure the scheme validation is implemented in the controller (see comment on controller lines 226-228).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go` around lines
1092 - 1104, The test case is failing for the wrong reason because no CA bundle
is provided and the controller lacks DiscoveryURL scheme validation; update the
test case entry (the table row with name "invalid discovery URL (http instead of
https)") to include caBundleConfigMap: &baseCABundleConfigMap and set a valid
Issuer.URL so generateIssuer runs past CA lookup, and then implement a scheme
check in the controller where DiscoveryURL is assigned (the code path around
generateIssuer and the assignment to
auth.Spec.OIDCProviders[0].Issuer.DiscoveryURL — see generateIssuer and the
controller lines that set DiscoveryURL) to reject or error on non-HTTPS schemes
(i.e., require URL.Scheme == "https") so the test actually triggers the intended
validation error.
| { | ||
| name: "claim validation rule missing required claim", | ||
| auth: *authWithUpdates(baseAuthResource, []func(auth *configv1.Authentication){ | ||
| func(auth *configv1.Authentication) { | ||
| auth.Spec.OIDCProviders[0].ClaimValidationRules = append( | ||
| auth.Spec.OIDCProviders[0].ClaimValidationRules, | ||
| configv1.TokenClaimValidationRule{ | ||
| Type: configv1.TokenValidationRuleTypeRequiredClaim, | ||
| RequiredClaim: nil, | ||
| }, | ||
| ) | ||
| }, | ||
| }), | ||
| expectError: true, | ||
| featureGates: featuregates.NewFeatureGate( | ||
| []configv1.FeatureGateName{features.FeatureGateExternalOIDCWithUpstreamParity}, | ||
| []configv1.FeatureGateName{}, | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Duplicate test that also fires the wrong error.
This test case replicates the existing "auth config with nil claim in validation rule" scenario (lines 701–727), which already covers RequiredClaim: nil with a properly configured CA bundle. Additionally, the missing caBundleConfigMap field means generateIssuer returns a CA-lookup error before generateClaimValidationRules is ever reached, so the test verifies the wrong failure path.
Either remove it (it adds no new coverage) or differentiate it and add caBundleConfigMap: &baseCABundleConfigMap so it actually exercises claim validation rule processing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controllers/externaloidc/externaloidc_controller_test.go` around lines
1105 - 1123, This test is a duplicate that fails for the wrong reason; either
remove the test case named "claim validation rule missing required claim" or
modify its auth fixture so it actually exercises claim-validation (not CA
lookup) by adding caBundleConfigMap: &baseCABundleConfigMap to the auth produced
by authWithUpdates (or otherwise ensure a valid CA bundle) so generateIssuer
succeeds and generateClaimValidationRules runs; keep the RequiredClaim: nil to
preserve the intended assertion if you choose to keep and rename the case to
avoid duplication with the existing "auth config with nil claim in validation
rule" test.
| func generateUserValidationRule(rule configv1.TokenUserValidationRule) (apiserverv1beta1.UserValidationRule, error) { | ||
| if len(rule.Expression) == 0 { | ||
| return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("userValidationRule expression must be non-empty") | ||
| } | ||
|
|
||
| // validate CEL expression | ||
| if err := validateCELExpression(&authenticationcel.UserValidationCondition{ | ||
| Expression: rule.Expression, | ||
| }); err != nil { | ||
| return apiserverv1beta1.UserValidationRule{}, fmt.Errorf("invalid CEL expression: %v", err) | ||
| } | ||
|
|
||
| return apiserverv1beta1.UserValidationRule{ | ||
| Expression: rule.Expression, | ||
| Message: rule.Message, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, verify the file structure and check lines around 463-479
echo "=== Checking externaloidc_controller.go lines 463-500 ==="
head -n 500 pkg/controllers/externaloidc/externaloidc_controller.go | tail -n 50 | cat -nRepository: openshift/cluster-authentication-operator
Length of output: 1872
🏁 Script executed:
#!/bin/bash
# Check if CompileUserExpression exists in authenticationcel compiler
echo "=== Searching for CompileUserExpression and CompileClaimsExpression ==="
rg -n "CompileUserExpression|CompileClaimsExpression" \
vendor/k8s.io/apiserver/pkg/authentication/cel/ --type go -A 3Repository: openshift/cluster-authentication-operator
Length of output: 2135
🏁 Script executed:
#!/bin/bash
# Check the validateCELExpression function implementation
echo "=== Checking validateCELExpression implementation ==="
rg -n "func validateCELExpression" pkg/controllers/externaloidc/ --type go -A 10Repository: openshift/cluster-authentication-operator
Length of output: 578
validateCELExpression uses CompileClaimsExpression — wrong for user-validation expressions.
validateCELExpression (line 614) unconditionally calls compiler.CompileClaimsExpression(expressionAccessor). User-validation rules evaluate against a user.* environment (user.username, user.groups, etc.), whereas claim-validation rules evaluate against claims.*. The authenticationcel compiler provides both CompileUserExpression() and CompileClaimsExpression() for exactly this purpose.
Compiling a UserValidationCondition through the claims-expression compiler means validation occurs in the wrong CEL variable scope, potentially accepting invalid expressions that reference nonexistent user.* variables or rejecting valid ones.
Use compiler.CompileUserExpression(expressionAccessor) when validating UserValidationCondition, and route based on expression type if validateCELExpression is reused across both claim and user validation.
🤖 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 463 -
479, The helper validateCELExpression currently always calls
compiler.CompileClaimsExpression which is incorrect for user rules; update
validation to call compiler.CompileUserExpression when given an
authenticationcel.UserValidationCondition (and keep CompileClaimsExpression for
claim conditions), or create a dedicated validateCELUserExpression that calls
CompileUserExpression for UserValidationCondition and use that from
generateUserValidationRule; reference validateCELExpression,
authenticationcel.UserValidationCondition, compiler.CompileUserExpression and
compiler.CompileClaimsExpression to locate and change the call routing so user
expressions are compiled in the user.* CEL environment.
|
@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. |
4 participants
Summary by CodeRabbit