CNTRLPLANE-311: adding auth config missing fields to API - #2487
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @ShazaAldawamneh! Some important instructions when contributing to openshift/api: |
ShazaAldawamneh
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@ShazaAldawamneh: This pull request references CNTRLPLANE-311 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. |
openshift-ci
Bot
added
the
size/XXL
ShazaAldawamneh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/assign |
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
For all of the CEL expression fields, can we also get a PR up against openshift/kubernetes similar to openshift/kubernetes#2353 to show that we are going to be adding admission time validation for the newly added CEL expression fields?
openshift-merge-robot
added
the
needs-rebase
582db23 to
f854a3d
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/retest-required |
WalkthroughAdds a feature-gated ExternalOIDCWithUpstreamParity: new onCreate test YAML, feature gate registration, Authentication API additions (discoveryURL, CEL-based claim/user validation, new types), regenerated deepcopy/OpenAPI/Swagger, and multiple CRD and payload manifest variants exposing updated schema and validations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/v1/zz_generated.swagger_doc_generated.go (1)
446-453: ClarifyuserValidationRulesdocs to match TokenUserValidationRule semanticsThe description mentions validating "claims" and supporting rules that "require specific claims and values", but
userValidationRulesis backed by[]TokenUserValidationRule, which only exposes CEL expressions over user information (expression/message). This misleads API consumers about supported rule types.Apply this diff to make the description accurate and focused on CEL-based user validation:
- "userValidationRules": "userValidationRules defines the set of rules used to validate claims in a user's token. Each rule is evaluated independently to determine whether the token subject is considered valid. Rules can either require specific claims and values to be present, or define CEL expressions that must evaluate to true for the token to be accepted. All rules must evaluate to true for the token to be accepted. If any rule evaluates to false, the token is rejected. A maximum of 64 rules can be specified. This field is optional.\n\nSee https://kubernetes.io/docs/reference/using-api/cel/ for CEL syntax.", + "userValidationRules": "userValidationRules defines the set of CEL-based rules used to validate a user's token. Each rule defines a CEL expression evaluated against the token's user information (for example, username and groups). All rules must evaluate to true for the token to be accepted. If any rule evaluates to false, the token is rejected. A maximum of 64 rules can be specified. This field is optional.\n\nSee https://kubernetes.io/docs/reference/using-api/cel/ for CEL syntax.",openapi/openapi.json (1)
11243-11259: Enforce schema constraints for discoveryURL and audienceMatchPolicy, but use correct format for URL validation.The
discoveryURLdescription specifies multiple constraints (HTTPS, absolute URL, no query params, no user info, no fragments) but the schema lacks validation. Similarly,audienceMatchPolicydescribes "MatchAny" as an allowed value but doesn't declare an enum constraint.For
discoveryURL, the correct approach requires both format and pattern. Useformat: "uri"(noturi-reference, which allows relative URLs) combined with a pattern to enforce HTTPS:"discoveryURL": { "description": "discoveryURL is an optional field that, if specified, overrides the default discovery endpoint used to retrieve OIDC configuration metadata. By default, the discovery URL is derived from `issuerURL` as \"{url}/.well-known/openid-configuration\".\n\nThe discoveryURL must:\n - Be a valid absolute URL.\n - Use the HTTPS scheme.\n - Not contain query parameters, user info, or fragments.\n - Be different from the value of `url` (ignoring trailing slashes)", "type": "string", + "format": "uri", + "pattern": "^https://" }For
audienceMatchPolicy, if "MatchAny" is the only allowed value, declare an enum:"audienceMatchPolicy": { "description": "AudienceMatchPolicy specifies how token audiences are matched. Allowed values are `MatchAny`. When set to `MatchAny`, the token is accepted if any of its audiences match any of the configured audiences. When omitted, the system applies a default policy. Currently, the default is `MatchAny`.", "type": "string", + "enum": ["MatchAny"] }Note: Complex constraints like "different from
issuerURLignoring trailing slashes" and preventing query parameters/fragments cannot be fully expressed in JSON Schema and require runtime validation via CEL rules in a CRD or API server validation.
♻️ Duplicate comments (4)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml (1)
295-316: Drop length-only test (per prior guidance).This test only validates max length and adds noise. Please remove it.
- - name: Expression too long - initial: | - ... - expectedError: "expression must be set when type is 'Expression'" + # dropped: length-only validation caseconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDC.yaml (1)
192-247: [Duplicate] Missingexpressionfield in schema; documentation references non-existent field.Same issue as flagged in
ExternalOIDCWithUIDAndExtraClaimMappings.yaml: the schema claims to support "Expression" mode (lines 239–240) but lacks the correspondingexpressionfield in properties. Both feature-gated CRD variants have this inconsistency.See the prior review comment in
ExternalOIDCWithUIDAndExtraClaimMappings.yamlfor the detailed analysis and recommended fix. Both files must be updated consistently.config/v1/types_authentication.go (2)
317-319: Fix field reference in documentation.The documentation references
urlbut should referenceissuerURLfor consistency with the actual field name. This was previously flagged in review.- // parameters, user information, or fragments. Additionally, it must differ from - // the value of `url` (ignoring trailing slashes). + // parameters, user information, or fragments. Additionally, it must differ from + // the value of `issuerURL` (ignoring trailing slashes).
792-798: Remove invalid spacing between comment blocks.There's an empty line between the field documentation and the
+optionalmarker. Comment blocks must be continuous with//on each line. This was previously flagged in review.// requiredClaim allows configuring a required claim name and its expected value. // This field is required when `type` is set to RequiredClaim, and must be omitted // when `type` is set to any other value. The Kubernetes API server uses this field // to validate if an incoming JWT is valid for this identity provider. - + // // +optional RequiredClaim *TokenRequiredClaim `json:"requiredClaim,omitempty"`
🟡 Minor comments (9)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml-448-449 (1)
448-449: Rename test title to match payload (message is present).The case includes both expression and message; title says “expression only.”
- - name: Valid TokenUserValidationRule with expression only + - name: Valid TokenUserValidationRule with expression and messageAlso applies to: 466-468, 469-486
payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml-465-468 (1)
465-468: Doc bug: referenceissuerURL, noturl.“The value of
url” is misleading.- the value of `url` (ignoring trailing slashes). + the value of `issuerURL` (ignoring trailing slashes).payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml-465-468 (1)
465-468: Doc bug:issuerURLwording.Fix reference.
- the value of `url` (ignoring trailing slashes). + the value of `issuerURL` (ignoring trailing slashes).payload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml-465-468 (1)
465-468: Doc bug:issuerURLreference.Update wording.
- the value of `url` (ignoring trailing slashes). + the value of `issuerURL` (ignoring trailing slashes).config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml-465-468 (1)
465-468: Doc bug: referenceissuerURL, noturl.Same as other variants.
- the value of `url` (ignoring trailing slashes). + the value of `issuerURL` (ignoring trailing slashes).config/v1/zz_generated.swagger_doc_generated.go-537-545 (1)
537-545: Align TokenUserValidationRule summary with struct comment (claims vs user info)The type-level description says expressions are evaluated against the token's claims, while the
expressionfield docs and the struct inconfig/v1/types_authentication.gostate they are evaluated against user information (username, groups). This inconsistency can confuse CEL authors.Adjust the summary string to match the field/struct semantics:
var map_TokenUserValidationRule = map[string]string{ - "": "TokenUserValidationRule provides a CEL-based rule used to validate a token subject. Each rule contains a CEL expression that is evaluated against the token’s claims.", + "": "TokenUserValidationRule provides a CEL-based rule used to validate a token's user information (for example, username and groups). Each rule contains a CEL expression that is evaluated against this user context.", "expression": "expression is a CEL expression that must evaluate to true for the token to be accepted. The expression is evaluated against the token's user information (e.g., username, groups). This field must be non-empty and may not exceed 1024 characters.", "message": "message allows configuring a human-readable message that is logged by the Kubernetes API server when a token fails validation based on the CEL expression defined in 'Expression'. This field is optional. If provided, the message must be at least 1 character long and cannot exceed 256 characters. This message is logged and not returned to the caller.", }config/v1/zz_generated.swagger_doc_generated.go-498-502 (1)
498-502: Restore documentation forrequiredClaimon TokenClaimValidationRule
TokenClaimValidationRulestill has aRequiredClaim *TokenRequiredClaimfield, butmap_TokenClaimValidationRuleno longer documents therequiredClaimproperty, which regresses API clarity for RequiredClaim-type rules.Add a
requiredClaimentry so the swagger docs describe both rule variants:var map_TokenClaimValidationRule = map[string]string{ "": "TokenClaimValidationRule represents a validation rule based on token claims. If type is RequiredClaim, requiredClaim must be set. If type is Expression, expression must be set.", "type": "type is an optional field that configures the type of the validation rule.\n\nAllowed values are \"RequiredClaim\" and \"Expression\".\n\nWhen set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value.\n\nWhen set to 'Expression', the Kubernetes API server will be configured to validate the incoming JWT against the configured CEL expression.", + "requiredClaim": "requiredClaim allows configuring a required claim name and its expected value. This field is required when `type` is set to RequiredClaim, and must be omitted when `type` is set to any other value. The Kubernetes API server uses this field to validate if an incoming JWT is valid for this identity provider.", "expression": "expression configures a CEL expression that will be used by the Kubernetes API server to validate if an incoming JWT is valid for this identity provider. The CEL expression must return a boolean value where 'true' signals a valid state. Expression must be set when 'type' is 'Expression', and is forbidden otherwise.", }config/v1/types_authentication.go-845-852 (1)
845-852: Inconsistent documentation: godoc says "optional" but marker says+required.The godoc states "This field is optional" but the
+requiredmarker indicates the field is mandatory. The CRD manifests also showmessageas required. Update the documentation to reflect that this field is required.// message allows configuring a human-readable message that is logged by the Kubernetes API server // when a token fails validation based on the CEL expression defined in 'Expression'. - // This field is optional. If provided, the message must be at least 1 character long - // and cannot exceed 256 characters. This message is logged and not returned to the caller. + // This field is required. The message must be at least 1 character long + // and must not exceed 256 characters. This message is logged and not returned to the caller. // +requiredconfig/v1/types_authentication.go-867-874 (1)
867-874: Inconsistent documentation: godoc says "optional" but marker says+required.Same issue as
TokenExpressionRule.Message- the godoc states "This field is optional" but the+requiredmarker indicates the field is mandatory. Update the documentation for consistency.// message allows configuring a human-readable message that is logged by the Kubernetes API server // when a token fails validation based on the CEL expression defined in 'Expression'. - // This field is optional. If provided, the message must be at least 1 character long - // and cannot exceed 256 characters. This message is logged and not returned to the caller. + // This field is required. The message must be at least 1 character long + // and must not exceed 256 characters. This message is logged and not returned to the caller. // +required
🧹 Nitpick comments (7)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml (3)
403-424: Remove commented-out test block.Dead/commented code adds churn to future edits.
- # - name: Expression too long in TokenUserValidationRule - # initial: | - # ... - # expectedError: "expression: Too long" + # (removed)
145-164: Nit: clarify title (“issuerURL”).Rename “URL” → “issuerURL” for precision.
- - name: discoveryURL must be different from URL + - name: discoveryURL must be different from issuerURL
76-83: False-positive secret scan: basic auth in test URL.Static analysis flags
https://user:pass@.... It’s intentional to test “no userinfo”, but may trip CI. Consider obfuscatingpassor adding repo-level suppression for this test path.payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml (1)
480-481: Prefer URL parsing over regex for userinfo.If available in this CEL environment, prefer
url(self).getUserInfo().size() == 0to avoid pattern gaps.- - message: discoveryURL must not contain user info - rule: '!self.matches(''^https://.+:.+@.+/.*$'')' + - message: discoveryURL must not contain user info + rule: url(self).getUserInfo().size() == 0Please confirm
getUserInfo()support in your apiserver CEL library.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml (1)
480-481: Optional: switch to URL userinfo check.Use
url(self).getUserInfo().size() == 0if supported.payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml (1)
480-481: Optional: robust userinfo check using URL parts.Prefer CEL URL accessors if available.
payload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml (1)
480-481: Optional: use URL userinfo accessor.Prefer
url(self).getUserInfo().size() == 0if supported.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (26)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml(1 hunks)config/v1/types_authentication.go(6 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yaml(3 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml(1 hunks)config/v1/zz_generated.deepcopy.go(4 hunks)config/v1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDC.yaml(2 hunks)config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml(2 hunks)config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml(1 hunks)config/v1/zz_generated.swagger_doc_generated.go(3 hunks)features.md(1 hunks)features/features.go(1 hunks)openapi/generated_openapi/zz_generated.openapi.go(7 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml(3 hunks)payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml(1 hunks)payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlconfig/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlfeatures/features.gofeatures.mdconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlconfig/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yamlpayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlconfig/v1/types_authentication.goconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDC.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.swagger_doc_generated.goopenapi/openapi.jsonopenapi/generated_openapi/zz_generated.openapi.go
🧬 Code graph analysis (4)
features/features.go (1)
config/v1/types_feature.go (2)
DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
config/v1/types_authentication.go (1)
config/v1/zz_generated.swagger_doc_generated.go (3)
TokenUserValidationRule(543-545)TokenRequiredClaim(533-535)TokenExpressionRule(513-515)
config/v1/zz_generated.deepcopy.go (2)
config/v1/types_authentication.go (2)
TokenUserValidationRule(857-875)TokenExpressionRule(834-853)config/v1/zz_generated.swagger_doc_generated.go (2)
TokenUserValidationRule(543-545)TokenExpressionRule(513-515)
config/v1/zz_generated.swagger_doc_generated.go (1)
config/v1/types_authentication.go (3)
TokenClaimValidationRule(779-810)TokenExpressionRule(834-853)TokenUserValidationRule(857-875)
🪛 Checkov (3.2.334)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
[medium] 76-77: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (35)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yaml (1)
10-10: Approve metadata annotation for feature-set tracking.The addition of the feature-set annotation is appropriate for the Default variant and aligns with the standard OpenShift CRD annotation pattern.
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml (5)
351-399: Dual-mode token claim validation with proper mutual exclusivity enforcement.The new
expressionfield and type enum introduce CEL-based validation alongside the existing RequiredClaim mode. The x-kubernetes-validations rules (lines 428–436) correctly enforce that exactly one ofexpressionorrequiredClaimis set based on the type value.
428-436: Verify mutual exclusivity validation rules are exhaustive.The x-kubernetes-validations at lines 428–436 enforce two conditions:
- If type is 'Expression', expression must be set (line 429–432)
- If type is 'RequiredClaim', requiredClaim must be set (line 433–436)
However, if type is omitted or set to an invalid value, both rules would require their respective fields to be absent. Ensure this defensive logic aligns with the Go implementation in
types_authentication.go.
459-481: discoveryURL field with strict HTTPS and format validation.The new
discoveryURLfield enables overriding OIDC discovery endpoints. Validation rules enforce:
- Valid absolute HTTPS URL (lines 472–475)
- No query parameters (line 476–477)
- No fragments (line 478–479)
- No user info (line 480–481)
This is appropriately strict for security.
528-532: Validate the CEL logic for discoveryURL vs issuerURL differentiation.Line 530–532 enforces that discoveryURL must differ from issuerURL (after stripping trailing slashes). The CEL rule uses
.find('^.+[^/]')to strip trailing slashes and compares both URLs. Verify this regex logic handles edge cases (e.g., root paths, query parameters that should have been blocked earlier, empty strings).
632-675: New userValidationRules field with CEL-based per-user validation.The
userValidationRulesfield (max 64, min 1 when present) enables CEL-based validation at the user level. The use ofexpressionas the x-kubernetes-list-map-key ensures uniqueness per rule. Verify that theminItems: 1constraint is intentional—it means if the field is present, at least one rule must be configured.payload-manifests/featuregates/featureGate-Hypershift-Default.yaml (1)
102-104: Feature gate correctly added to disabled list in Default variant.The
ExternalOIDCWithUpstreamParitygate is appropriately placed in the disabled list for the Default feature set and maintains alphabetical ordering.payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
160-162: Feature gate correctly enabled in DevPreviewNoUpgrade variant.The
ExternalOIDCWithUpstreamParityentry is placed in the enabled list for DevPreviewNoUpgrade and maintains alphabetical order.features.md (1)
44-44: Feature gate documentation added with correct state matrix.The
ExternalOIDCWithUpstreamParityentry in features.md correctly reflects the gate's enabled state across preview variants and disabled state in Default.payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml (1)
169-171: Feature gate correctly enabled in TechPreviewNoUpgrade variant.The entry is properly placed in the enabled list with correct alphabetical positioning.
payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml (1)
102-104: Feature gate correctly added to disabled list in SelfManagedHA Default variant.Consistent with the Hypershift Default manifest; the gate is disabled and alphabetically ordered.
payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml (1)
142-144: Feature gate correctly enabled in SelfManagedHA DevPreviewNoUpgrade variant.Entry is properly placed and alphabetically ordered, consistent with other platform variants.
config/v1/zz_generated.featuregated-crd-manifests.yaml (1)
31-36: Authentication CRD now correctly advertises the new feature gateAdding
ExternalOIDCWithUpstreamParityto theFeatureGateslist forauthentications.config.openshift.iois consistent with the new feature gate and keeps the CRD metadata in sync withfeatures.goand payload manifests.payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml (1)
146-155: SelfManagedHA TechPreview manifest wiring for ExternalOIDCWithUpstreamParity looks correctThe new enabled entry for
ExternalOIDCWithUpstreamParityis placed alongside the other ExternalOIDC feature gates and matches the TechPreview scope defined infeatures/features.go.features/features.go (1)
460-466: New ExternalOIDCWithUpstreamParity feature gate definition is consistentThe gate metadata (name, Jira component, contact, enhancement PR) and
enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade)pattern match adjacent ExternalOIDC feature gates and avoid unnecessary cluster‑profile specialization, as requested in earlier review feedback.config/v1/zz_generated.deepcopy.go (4)
4738-4760: OIDCProvider deepcopy correctly extended for UserValidationRulesThe added block copying
UserValidationRulesallocates a new slice and value‑copies eachTokenUserValidationRule, which is sufficient since the rule struct only contains strings. This avoids aliasing and keepsOIDCProviderdeepcopy behavior consistent with other slice fields.
6435-6443: TokenClaimValidationRule now deep‑copies Expression as wellAssigning
out.Expression = in.Expressioncorrectly copies the newTokenExpressionRulefield by value, which is appropriate given it only contains string fields (no nested reference types).
6477-6491: DeepCopy for TokenExpressionRule matches its simple value semanticsBoth
DeepCopyIntoandDeepCopyuse struct assignment, which is sufficient forTokenExpressionRulesince it only has string members.
6531-6545: DeepCopy for TokenUserValidationRule is correctly implementedThe autogenerated deepcopy functions simply assign the struct, which fully copies the
ExpressionandMessagestring fields and is consistent with how other simple structs are handled.config/v1/zz_generated.swagger_doc_generated.go (1)
508-515: TokenExpressionRule swagger docs align with the type definitionThe new
TokenExpressionRulemap andSwaggerDoccorrectly exposeexpressionandmessageand reflect the validation semantics fromconfig/v1/types_authentication.go(required non-empty CEL expression, optional log message with length bounds).config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
343-399: Missingexpressionfield in schema; documentation references non-existent field.The
claimValidationRulesdocumentation mentions support for "Expression" mode validation (lines 390–391), but the schema properties (lines 356–393) only definerequiredClaimandtypefields—noexpressionfield is present. If Expression mode is supported, the schema must include anexpressionfield and the validation rule must be updated to enforce its presence when type is "Expression".Verify that the
expressionfield is defined inconfig/v1/types_authentication.goand ensure it's included in this generated CRD schema.openapi/generated_openapi/zz_generated.openapi.go (6)
414-423: LGTM!The new type registrations follow the existing pattern and maintain alphabetical ordering.
20628-20639: LGTM!The updated description clearly documents the two validation rule types and their semantics.
20642-20661: LGTM!The expression field correctly references TokenExpressionRule and maintains proper dependencies.
20767-20773: LGTM!The discoveryURL field is well-documented with clear validation constraints.
20699-20724: Fix inconsistency:messagefield marked both optional and required.Line 20714 describes
messageas optional ("This field is optional"), but line 20720 includes it in theRequiredarray. This contradiction will cause API validation to reject requests without themessagefield, conflicting with the documentation.Resolve by either:
- Removing
messagefrom theRequiredarray (line 20720) if it should be optional, or- Updating the description (line 20714) to state the field is required.
Since this is generated code, verify the source type definition to determine the correct state, then regenerate if necessary.
17473-17500: Verify maxItems constraint is enforced at the CRD level.The description states "A maximum of 64 rules can be specified," but the generated OpenAPI schema lacks a
maxItemsconstraint. This validation may be enforced via CEL validation rules or CRD constraints in the source type definition. Confirm the constraint is present in the TokenUserValidationRule type definition or CRD validation rules.config/v1/types_authentication.go (3)
247-263: LGTM!The
userValidationRulesfield is well-defined with proper constraints (MaxItems=64, MinItems=1), feature gating, and uniqueness enforcement vialistType=mapwithexpressionas the key.
757-771: LGTM!The
TokenValidationRuleTypeenum is properly defined with feature-gate-aware enum validation, correctly gatingExpressionbehindExternalOIDCWithUpstreamParitywhile allowingRequiredClaimby default.
809-809: Verifyomitzerojson tag compatibility with Go version requirements.The
omitzerotag on theexpressionfield requires Go 1.24+. Confirm this aligns with the project's minimum Go version specified ingo.mod.config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml (2)
1-11: LGTM!The CRD manifest correctly includes the
ExternalOIDCWithUpstreamParityfeature gate annotation and follows the expected structure for OpenShift API CRDs.
481-524: LGTM!The
userValidationRulesschema correctly implements the constraints from the Go types: max 64 items, min 1 item, map-based list keyed onexpressionfor uniqueness, and properly nestedTokenUserValidationRulestructure.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml (2)
1-11: LGTM!The CRD manifest is correctly annotated for the DevPreviewNoUpgrade feature set with appropriate release annotations.
697-707: LGTM!The
typeenum correctly includesOIDCalongside the standard values, as expected for the DevPreviewNoUpgrade feature set which enables ExternalOIDC capabilities.openapi/openapi.json (1)
11187-11198: Verify conditional validation constraints for TokenClaimValidationRule.The schema allows both
expressionRuleandrequiredClaimto be absent or present simultaneously, but the description implies a conditional constraint: iftypeis "Expression",expressionRulemust be set; iftypeis "RequiredClaim",requiredClaimmust be set. This requires checking:
- Go struct validation tags in
config/v1/types_authentication.gofor TokenClaimValidationRule- CEL validation rules in generated CRD manifests to enforce this conditional logic
Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
bebeeb2 to
5470873
Compare
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @config/v1/types_authentication.go:
- Around line 311-328: Update the comment for DiscoveryURL to reference the
correct serialized field name `issuerURL` instead of `url` (e.g., change "differ
from the value of `url`" to "differ from the value of `issuerURL`") so the
documentation matches the actual field name and serialization; ensure all
occurrences in the block around the DiscoveryURL field and its validation
comments use `issuerURL`.
In
@config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml:
- Around line 460-468: The discoveryURL field description incorrectly references
`url`; update the docstring for `discoveryURL` to say it must differ from
`issuerURL` (ignoring trailing slashes) so it matches the schema/validation;
change the sentence "it must differ from the value of `url` (ignoring trailing
slashes)" to "it must differ from the value of `issuerURL` (ignoring trailing
slashes)" in the `discoveryURL` description.
- Around line 503-521: Update the issuerURL JSONSchema/x-kubernetes-validation
in the Go type for Authentication so its query-empty check matches discoveryURL:
replace the rule that uses getQuery() == {} with getQuery().size() == 0; locate
the issuerURL validation in types_authentication.go (the Authentication struct /
issuerURL field) and modify the validation tag/expression accordingly so
regenerated CRDs use url(self).getQuery().size() == 0.
In
@config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yaml:
- Around line 351-407: The CRD for TokenClaimValidationRule is inconsistent: the
description mentions "CEL" but the schema's enum only contains "RequiredClaim",
there is no `cel` property or CEL validation, and `type` is documented as
optional but marked required. Fix by updating the Go API type that generates
TokenClaimValidationRule to either (A) restrict the description to only
"RequiredClaim" and ensure `Type` is required in the Go struct/tags, or (B) add
CEL support by adding a `Cel`/`cel` field to the Go type, include "CEL" in the
Type enum/tag, and add the corresponding validation logic (mirror the
TechPreview/Custom variant and x-kubernetes-validations for CEL). Regenerate the
CRD so TokenClaimValidationRule, Type enum, `cel` property, and
x-kubernetes-validations are consistent.
In
@payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml:
- Around line 460-483: Update the discoveryURL docs to say it must differ from
issuerURL (not `url`) so description matches validation, and tighten the "must
not contain user info" validation for discoveryURL to reject any user-info (both
user:pass and user@ forms) — replace the current regex check on discoveryURL
with a stricter CEL condition that detects an '@' in the authority portion (same
approach used for issuerURL) so configurations with embedded credentials are
consistently rejected; target the discoveryURL field and its
x-kubernetes-validations entry to make these edits.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yaml (1)
449-471: UsegetQuery().size() == 0for issuerURL instead ofgetQuery() == {}The issuerURL validation currently checks for an empty query with
url(self).getQuery() == {}. Comparing a map to an untyped{}literal is brittle in CEL and inconsistent with other Authentication CRD variants that already usegetQuery().size() == 0. This pattern is also inconsistently applied across multiple generated CRD files (8 files still use the old pattern; others have been partially updated to the new pattern). Prefer the size-based emptiness check:- message: must not have a query rule: isURL(self) && url(self).getQuery().size() == 0This should be fixed in the generator / Go source so all generated CRDs are consistent.
payload-manifests/crds/0000_10_config-operator_01_authentications-OKD.crd.yaml (1)
351-407: Schema lacks CEL support mentioned in documentation.The description at lines 351-354 states "If Type is CEL, CEL must be set" and lines 389-396 describe CEL validation behavior, but the schema provides no
celfield and thetypeenum at lines 398-399 only includesRequiredClaim. This creates a misleading contract.Either remove all CEL references from the documentation to match the OKD feature set's actual capabilities, or add the missing
celfield andCELenum value if CEL support is intended.🔎 Proposed fix to remove CEL references
description: |- TokenClaimValidationRule represents a validation rule based on token claims. - If type is RequiredClaim, requiredClaim must be set. - If Type is CEL, CEL must be set and RequiredClaim must be omitted. + The type must be RequiredClaim, and requiredClaim must be set. properties: requiredClaim: description: |- requiredClaim allows configuring a required claim name and its expected value. - This field is required when `type` is set to RequiredClaim, and must be omitted - when `type` is set to any other value. The Kubernetes API server uses this field - to validate if an incoming JWT is valid for this identity provider. + This field is required. The Kubernetes API server uses this field to validate + if an incoming JWT is valid for this identity provider. ... type: description: |- type is an optional field that configures the type of the validation rule. - Allowed values are "RequiredClaim" and "CEL". + Allowed value is "RequiredClaim". When set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value. - - When set to 'CEL', the Kubernetes API server will be configured - to validate the incoming JWT against the configured CEL expression.
♻️ Duplicate comments (23)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yaml (1)
351-407: AlignTokenClaimValidationRule.typedocs with OKD schema (no CEL supported here)The description advertises
typevalues"RequiredClaim"and"CEL"and describes CEL behavior, but:
- The
enumonly allows"RequiredClaim".- There is no
celproperty or CEL‑related validation.typeis listed as required while the description still calls it “optional”.This makes the OKD CRD misleading and suggests configurations that will be rejected by validation. For the OKD feature‑set variant, either:
- Remove/adjust CEL references and describe only
"RequiredClaim", and update the wording sotypeis documented as required, or- If CEL is actually intended to be usable on OKD, extend the schema here (add
cel, include"CEL"in the enum, and mirror the TechPreview/Custom validations).Given the separate feature‑gated manifests, the first option (document only
RequiredClaimin this file) seems correct; please update the Go type comments / generator accordingly and regenerate the CRDs.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml (1)
460-468: Doc fix: refer toissuerURL, noturl, indiscoveryURLdescriptionThe
discoveryURLdescription says:“…Additionally, it must differ from the value of
url(ignoring trailing slashes).”There is no
urlfield here; the cross‑field validation compares againstissuerURL. Update the prose to referenceissuerURLinstead so the docs match the actual schema and validation behavior.config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yaml (1)
449-471: Switch issuerURL query check togetQuery().size() == 0(matches other variants)The issuerURL validation still uses:
- message: must not have a query rule: isURL(self) && url(self).getQuery() == {}Other Authentication CRD variants now use
url(self).getQuery().size() == 0, which is the safer CEL idiom and avoids comparing maps to an untyped{}literal. Please update the generator / source so this rule becomes:- - message: must not have a query - rule: isURL(self) && url(self).getQuery() == {} + - message: must not have a query + rule: isURL(self) && url(self).getQuery().size() == 0openapi/openapi.json (7)
9150-9161: Enforce maximum rule count constraint in schema.The description states "no more than 64 rules may be specified," but the schema still lacks a
maxItems: 64constraint. This issue was previously flagged but remains unaddressed.
10968-10971: Add schema constraint for expression field length limits.The description specifies "must be at least 1 character in length and must not exceed 1024 characters," but the schema still lacks
minLength: 1andmaxLength: 1024constraints. This issue was previously flagged but remains unaddressed.
10972-10975: Missing string length constraints on message field.The description requires "must be at least 1 character in length and must not exceed 256 characters," but the schema still lacks
minLength: 1andmaxLength: 256constraints. This issue was previously flagged but remains unaddressed.
10994-10998: Inconsistency: type field marked required but described as optional.The description states "type is an optional field," but the field is listed in the
requiredarray at lines 10981-10983. This inconsistency was previously flagged but remains unaddressed.
11037-11040: Add URL validation and length constraints to discoveryURL field.The description requires "a valid absolute HTTPS URL" without "query parameters, user information, or fragments" and specifies "at least 1 character long and no longer than 2048 characters," but the schema still lacks
format: uri, pattern constraint,minLength: 1, andmaxLength: 2048. This issue was previously flagged but remains unaddressed.
11080-11083: Add schema constraint for expression field length limit.The description states "must be at least 1 character in length and must not exceed 1024 characters," but the schema still lacks
minLength: 1andmaxLength: 1024constraints. This issue was previously flagged but remains unaddressed.
11084-11087: Missing string length constraints on message field.The description requires "must be at least 1 character in length and must not exceed 256 characters," but the schema still lacks
minLength: 1andmaxLength: 256constraints. This issue was previously flagged but remains unaddressed.features/features.go (1)
444-450: Verify the intended feature gate enablement scope.The feature gate is currently enabled only in
DevPreviewNoUpgradeandTechPreviewNoUpgrade. Based on the past review comment and the fact that related feature gates likeExternalOIDC(Line 433) are enabled in all profiles includingDefaultandOKD, please confirm whether this is intentional or if this feature should also be enabled more broadly.payload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yaml (1)
351-399: Remove references to unsupported "Expression" type in descriptions.The
typeenum correctly restricts values toRequiredClaimonly (Line 398), but the descriptions at Lines 353-354 and 389-396 still reference the "Expression" type which is not supported in the Default feature set. Update the documentation to match the actual schema constraints.🔎 Proposed documentation fix
description: |- TokenClaimValidationRule represents a validation rule based on token claims. If type is RequiredClaim, requiredClaim must be set. - If Type is CEL, CEL must be set and RequiredClaim must be omitted. properties:type: description: |- type is an optional field that configures the type of the validation rule. - Allowed values are "RequiredClaim" and "CEL". + Allowed value is "RequiredClaim". When set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value. - - When set to 'CEL', the Kubernetes API server will be configured - to validate the incoming JWT against the configured CEL expression.payload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yaml (3)
377-380: Makemessageoptional in TokenClaimValidationCELRule.The description at Line 371 states that
messageis required, and it's listed in therequiredarray at Lines 377-380. However, per past review feedback,messageshould be optional to allow flexibility in CEL-based validation rules.🔎 Proposed fix
message: description: |- - message is a required human-readable message to be logged by the Kubernetes API server + message is an optional human-readable message to be logged by the Kubernetes API server if the CEL expression defined in 'expression' fails. message must be at least 1 character in length and must not exceed 256 characters. maxLength: 256 minLength: 1 type: string required: - expression - - message
466-467: Fix field name reference in discoveryURL description.Line 467 incorrectly references
urlinstead ofissuerURL. Update the documentation to use the correct field name.🔎 Proposed fix
The discoveryURL must be a valid absolute HTTPS URL. It must not contain query - parameters, user information, or fragments. Additionally, it must differ from the value of `url` (ignoring trailing slashes). + parameters, user information, or fragments. Additionally, it must differ from the value of `issuerURL` (ignoring trailing slashes).
633-674: Fix userValidationRules list configuration and make message optional.Multiple issues need to be addressed:
- The
x-kubernetes-list-type: mapwithexpressionas a key is incorrect sinceexpressionis not a top-level field- The
messagefield should be optional (Lines 665-668)- Consider using
atomiclist type instead🔎 Proposed fix
message: description: |- - message is a required human-readable message to be logged by the Kubernetes API server + message is an optional human-readable message to be logged by the Kubernetes API server if the CEL expression defined in 'expression' fails. message must be at least 1 character in length and must not exceed 256 characters. maxLength: 256 minLength: 1 type: string required: - expression - - message type: object maxItems: 64 minItems: 1 type: array - x-kubernetes-list-map-keys: - - expression - x-kubernetes-list-type: map + x-kubernetes-list-type: atomicconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yaml (1)
351-407: Remove CEL references from documentation or add CEL support to schema.The description at lines 353-354 and 389-396 mentions CEL type and validation, but the schema lacks a
celfield and thetypeenum at lines 397-399 only includesRequiredClaim. This feature gate does not support CEL-based validation.Remove all CEL references from the documentation to accurately reflect the feature gate's capabilities, or add the missing
celfield andCELenum value if CEL support is intended for this feature gate.🔎 Proposed documentation fix
description: |- TokenClaimValidationRule represents a validation rule based on token claims. - If type is RequiredClaim, requiredClaim must be set. - If Type is CEL, CEL must be set and RequiredClaim must be omitted. + The type must be RequiredClaim, and requiredClaim must be set. properties: requiredClaim: description: |- requiredClaim allows configuring a required claim name and its expected value. - This field is required when `type` is set to RequiredClaim, and must be omitted - when `type` is set to any other value. The Kubernetes API server uses this field + This field is required. The Kubernetes API server uses this field to validate if an incoming JWT is valid for this identity provider. ... type: description: |- type is an optional field that configures the type of the validation rule. - Allowed values are "RequiredClaim" and "CEL". + Allowed value is "RequiredClaim". When set to 'RequiredClaim', the Kubernetes API server will be configured to validate that the incoming JWT contains the required claim and that its value matches the required value. - - When set to 'CEL', the Kubernetes API server will be configured - to validate the incoming JWT against the configured CEL expression. enum: - RequiredClaimpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml (1)
514-524: Use type-safe map emptiness check for issuerURL query validation.Line 520 uses
url(self).getQuery() == {}which is type-unsafe in CEL. The discoveryURL validation at line 478 correctly usesurl(self).getQuery().size() == 0. Apply the same pattern here for consistency and to avoid CEL type dispatch ambiguities.🔎 Proposed fix
- message: must not have a query - rule: isURL(self) && url(self).getQuery() == {} + rule: isURL(self) && url(self).getQuery().size() == 0This is a generated CRD that originates from
config/v1/types_authentication.goline 281, so the fix should be applied at the source and CRDs regenerated.config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml (1)
363-373: Apply type-safe map emptiness check for issuerURL query validation.Line 369 uses
url(self).getQuery() == {}, which is type-unsafe in CEL. The discoveryURL validation at line 327 correctly usesurl(self).getQuery().size() == 0. For consistency and type safety, apply the same pattern to issuerURL.🔎 Proposed fix
- message: must not have a query - rule: isURL(self) && url(self).getQuery() == {} + rule: isURL(self) && url(self).getQuery().size() == 0Since this is a generated CRD from source definitions, update the source validation in
config/v1/types_authentication.goand regenerate all CRD manifests.config/v1/zz_generated.swagger_doc_generated.go (2)
522-522: FixdiscoveryURLdescription to referenceissuerURLinstead of nonexistenturlThe last sentence says
discoveryURLmust differ from the value ofurl, butTokenIssuerhas nourlfield—onlyissuerURLanddiscoveryURL. This is confusing in the public API docs and should be corrected to referenceissuerURL.Proposed doc fix
- "discoveryURL": "discoveryURL is an optional field that, if specified, overrides the default discovery endpoint used to retrieve OIDC configuration metadata. By default, the discovery URL is derived from `issuerURL` as \"{issuerURL}/.well-known/openid-configuration\".\n\nThe discoveryURL must be a valid absolute HTTPS URL. It must not contain query parameters, user information, or fragments. Additionally, it must differ from the value of `url` (ignoring trailing slashes). The discoveryURL value must be at least 1 character long and no longer than 2048 characters.", + "discoveryURL": "discoveryURL is an optional field that, if specified, overrides the default discovery endpoint used to retrieve OIDC configuration metadata. By default, the discovery URL is derived from `issuerURL` as \"{issuerURL}/.well-known/openid-configuration\".\n\nThe discoveryURL must be a valid absolute HTTPS URL. It must not contain query parameters, user information, or fragments. Additionally, it must differ from the value of `issuerURL` (ignoring trailing slashes). The discoveryURL value must be at least 1 character long and no longer than 2048 characters.",
538-546: AlignTokenUserValidationRuletop‑level doc with identity‑based evaluationThe empty‑key description still says the CEL expression is evaluated against “a token subject” and “the token’s claims”, but the
expressionfield (and the Go type) clearly describe evaluation over the constructed cluster user identity (username, groups, etc.). This mismatch can mislead users writing CEL.Proposed doc tweak
-var map_TokenUserValidationRule = map[string]string{ - "": "TokenUserValidationRule provides a CEL-based rule used to validate a token subject. Each rule contains a CEL expression that is evaluated against the token’s claims.", +var map_TokenUserValidationRule = map[string]string{ + "": "TokenUserValidationRule provides a CEL-based rule used to validate a cluster user identity that was constructed from token claims. Each rule contains a CEL expression that is evaluated against cluster user identity attributes (for example, username and groups).", "expression": "expression is a required CEL expression that performs a validation on cluster user identity attributes like username, groups, etc. The expression must evaluate to a boolean value. When the expression evaluates to 'true', the cluster user identity is considered valid. When the expression evaluates to 'false', the cluster user identity is not considered valid. expression must be at least 1 character in length and must not exceed 1024 characters.", "message": "message is a required human-readable message to be logged by the Kubernetes API server if the CEL expression defined in 'expression' fails. message must be at least 1 character in length and must not exceed 256 characters.", }openapi/generated_openapi/zz_generated.openapi.go (2)
20254-20266: Critical: Unresolved inconsistency in type enum value references.Line 20263 documents allowed values as "RequiredClaim" and "CEL", but per the previously flagged critical issue, this should reference "Expression" instead of "CEL" to match the intended API semantics. This inconsistency persists from the previous review and remains unaddressed.
🤖 Prompt for AI Agents
In config/v1/types_authentication.go, locate the TokenClaimValidationRule type field documentation and update all references from "CEL" to "Expression" as the enum value (keeping "RequiredClaim" as-is); ensure the description at line ~20263 reads 'Allowed values are "RequiredClaim" and "Expression"', then regenerate the OpenAPI spec by running 'make update-generated-openapi'.
20276-20282: Critical: Inconsistent enum value reference and capitalization.Two issues persist at line 20278:
- The description states 'Must be set when Type is "CEL"', but should reference "Expression" instead of "CEL" to align with the corrected enum values (same critical issue as line 20263).
- "Type" should be lowercase "type" for consistency with line 20272.
🤖 Prompt for AI Agents
In config/v1/types_authentication.go, locate the TokenClaimValidationRule.CEL field documentation and update the description to: "cel holds the CEL expression and message for validation. Must be set when type is \"Expression\", and forbidden otherwise." (note lowercase "type" and "Expression" instead of "CEL"), then regenerate by running 'make update-generated-openapi'.config/v1/types_authentication.go (1)
848-869: Update documentation to match past review resolution.The struct-level comment at line 849 still states the expression is "evaluated against the token's claims," but a past review (marked as addressed in commit 91c0fcd) clarified this should reference the "constructed cluster user identity attributes" instead. User validation rules evaluate the mapped identity (username, groups, uid, extra) after claim-to-identity mapping, not the raw JWT token claims.
This is an important distinction that differentiates user validation from claim validation.
-// TokenUserValidationRule provides a CEL-based rule used to validate a token subject. -// Each rule contains a CEL expression that is evaluated against the token's claims. +// TokenUserValidationRule provides a CEL-based rule used to validate a token subject. +// Each rule contains a CEL expression that is evaluated against the cluster user identity +// attributes (username, groups, uid, extra) constructed from the token claims.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (30)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yamlconfig/v1/types_authentication.goconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDC.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yamlconfig/v1/zz_generated.swagger_doc_generated.gofeatures.mdfeatures/features.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (8)
- payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
- config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
- features.md
- payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-Hypershift-OKD.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUIDAndExtraClaimMappings.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-TechPreviewNoUpgrade.crd.yamlconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-Default.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-OKD.crd.yamlconfig/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlconfig/v1/zz_generated.swagger_doc_generated.goconfig/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDC.yamlconfig/v1/types_authentication.goopenapi/openapi.jsonconfig/v1/zz_generated.deepcopy.goopenapi/generated_openapi/zz_generated.openapi.goconfig/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
🧬 Code graph analysis (4)
features/features.go (1)
config/v1/types_feature.go (2)
DevPreviewNoUpgrade(49-49)TechPreviewNoUpgrade(45-45)
config/v1/zz_generated.swagger_doc_generated.go (1)
config/v1/types_authentication.go (2)
TokenClaimValidationCELRule(829-846)TokenUserValidationRule(850-869)
config/v1/types_authentication.go (1)
config/v1/zz_generated.swagger_doc_generated.go (3)
TokenUserValidationRule(544-546)TokenRequiredClaim(534-536)TokenClaimValidationCELRule(503-505)
config/v1/zz_generated.deepcopy.go (2)
config/v1/types_authentication.go (2)
TokenUserValidationRule(850-869)TokenClaimValidationCELRule(829-846)config/v1/zz_generated.swagger_doc_generated.go (2)
TokenUserValidationRule(544-546)TokenClaimValidationCELRule(503-505)
🪛 Checkov (3.2.334)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml
[medium] 112-113: Basic Auth Credentials
(CKV_SECRET_4)
🔇 Additional comments (20)
payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml (1)
166-168: ExternalOIDCWithUpstreamParity gate wiring looks consistentNew gate is enabled alongside the other ExternalOIDC* gates with the expected shape; no further changes needed here.
config/v1/zz_generated.featuregated-crd-manifests.yaml (1)
31-35: Authentication CRD correctly annotated with new feature gateIncluding
ExternalOIDCWithUpstreamParityin the Authentication FeatureGates registry aligns this CRD with the new OIDC parity feature and the other manifests in the PR.config/v1/zz_generated.deepcopy.go (1)
4563-4585: DeepCopy coverage for new OIDC validation types looks correct
UserValidationRulesis deep-copied as a fresh slice, and bothTokenClaimValidationCELRuleandTokenUserValidationRuleget value-wise DeepCopy helpers, which is sufficient given their string-only fields. No pointer or map fields are missed.Also applies to: 6209-6223, 6306-6320
payload-manifests/featuregates/featureGate-SelfManagedHA-OKD.yaml (1)
107-109: LGTM!The new feature gate
ExternalOIDCWithUpstreamParityis correctly added to the disabled list, following the established pattern and maintaining alphabetical ordering.config/v1/tests/authentications.config.openshift.io/ExternalOIDCWithUpstreamParity.yaml (1)
1-458: LGTM!The test suite comprehensively covers the new
ExternalOIDCWithUpstreamParityfeature functionality, including:
- Valid and invalid discoveryURL configurations
- RequiredClaim and CEL-based validation rules
- TokenUserValidationRule scenarios
- Cross-field validations between issuerURL and discoveryURL
The static analysis warning about basic auth credentials on Line 112-113 is a false positive—this is intentionally invalid test data used to verify proper validation of userinfo in URLs.
config/v1/zz_generated.swagger_doc_generated.go (3)
452-452:userValidationRulesSwagger doc looks accurateThe description correctly matches the intent of validating the constructed cluster user identity with CEL-based rules and clearly states cardinality limits. No changes needed here.
498-505: CEL rule Swagger mapping is consistent with the Go typeThe
TokenClaimValidationCELRuleSwagger map cleanly documentsexpressionandmessagein line with the struct-level comments (required, length bounds, boolean CEL semantics). This looks correct.
508-511: No action needed—Swagger enum values already match the Go type.The Swagger documentation correctly declares allowed values
"RequiredClaim"and"CEL"forTokenClaimValidationRule.type, matching theTokenValidationRuleTypeenum constants defined inconfig/v1/types_authentication.go(lines 766, 769). Both the Go type comment and validation rules consistently use these exact string literals.openapi/generated_openapi/zz_generated.openapi.go (5)
401-412: LGTM!The new OpenAPI definition mappings for
TokenClaimValidationCELRuleandTokenUserValidationRuleare correctly registered.
17147-17179: LGTM!The
userValidationRulesfield is correctly defined with proper list-map configuration using "expression" as the key, matching the field inTokenUserValidationRule.
20227-20252: LGTM!The
TokenClaimValidationCELRuleschema is well-defined with appropriate field descriptions and validation constraints. Bothexpressionandmessagefields are correctly marked as required, matching their descriptions.
20365-20377: LGTM!The
discoveryURLfield is well-documented with comprehensive validation constraints covering security requirements (HTTPS-only), format restrictions, and length limits.
20410-20439: LGTM!The
TokenUserValidationRuleschema is correctly defined with bothexpressionandmessagefields properly marked as required. The field descriptions are clear and consistent with the schema requirements.config/v1/types_authentication.go (7)
8-8: LGTM!The feature gate aware validation rule correctly ensures that all OIDC clients in the spec match their componentName and componentNamespace to either a previously configured client or one that exists in the status.
248-261: LGTM!The userValidationRules field is well-documented and correctly configured:
- Clear documentation of CEL-based validation semantics (all rules must pass)
- Appropriate constraints (1-64 rules)
- Correct listMapKey targeting the expression field for uniqueness
- Properly feature-gated
267-267: LGTM!The CEL validation correctly ensures that discoveryURL differs from issuerURL when both are present, using optional chaining to safely handle the optional discoveryURL field.
755-770: LGTM!The TokenValidationRuleType enum is properly defined with:
- Correct feature gate progression (CEL only available with ExternalOIDCWithUpstreamParity)
- Appropriate const naming convention
- Clear documentation for each enum value
772-805: LGTM!The TokenClaimValidationRule struct properly implements a discriminated union:
- Type field is required with clear documentation
- Mutual exclusivity enforced via CEL validation
- CEL field correctly uses omitzero for empty struct handling
- Feature-gated CEL support is appropriate
807-827: LGTM!The TokenRequiredClaim struct is straightforward and correctly defined with appropriate constraints preventing empty strings for both required fields.
829-846: LGTM!The TokenClaimValidationCELRule struct correctly combines
+requiredmarkers withomitemptyJSON tags, which is appropriate since the zero value (empty string) is invalid per the MinLength=1 constraints. This prevents invalid zero values from being serialized.
|
/retest-required |
1 similar comment
|
/retest-required |
|
Known issue with okd job. /override ci/prow/okd-scos-image |
|
@everettraven: /override requires failed status contexts, check run or a prowjob name to operate on.
Only the following failed contexts/checkruns were expected:
If you are trying to override a checkrun that has a space in it, you must put a double quote on the context. 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 kubernetes-sigs/prow repository. |
|
/verified by @everettraven /override ci/prow/okd-scos-images |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/okd-scos-images 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 kubernetes-sigs/prow repository. |
openshift-ci-robot
added
the
verified
|
@everettraven: This PR has been marked as verified by 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. |
|
/test e2e-aws-serial |
|
@everettraven: The specified target(s) for The following commands are available to trigger optional jobs: Use 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 kubernetes-sigs/prow repository. |
|
These tests were sharded and the sharded instances are passing /override ci/prow/e2e-aws-serial |
|
@everettraven: Overrode contexts on behalf of everettraven: ci/prow/e2e-aws-serial, ci/prow/e2e-aws-serial-techpreview 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 kubernetes-sigs/prow repository. |
|
@ShazaAldawamneh: all tests passed! 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. |
Updates are made to support the newly introduced feature-gated fields in the Authentication configuration API added by openshift/api#2487 and 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 HyperShift APIs. To prevent the feature from automatically going GA, this PR introduces a new TechPreviewNoUpgrade (TPNU) feature gate in HyperShift. This includes updating the feature-gate manifests used for API generation and wiring the new feature gate through to the Control Plane Operator so that the additional Kubernetes API Server deployment configuration logic is conditionally enabled. The implementation ensures that the new behavior is strictly gated and only activated when the TechPreview feature gate is explicitly enabled. Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
…ups claim mapping Bumps openshift/api to consume the new feature-gated fields from openshift/api#2487 and openshift/api#2719 and introduces a new TechPreviewNoUpgrade feature gate to keep the feature from going GA. The new gate is wired through to the Control Plane Operator so the additional KAS configuration is only activated when TechPreview is explicitly enabled. Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
…ups claim mapping Bumps openshift/api to consume the new feature-gated fields from openshift/api#2487 and openshift/api#2719 and introduces a new TechPreviewNoUpgrade feature gate to keep the feature from going GA. The new gate is wired through to the Control Plane Operator so the additional KAS configuration is only activated when TechPreview is explicitly enabled. Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
…ups claim mapping Bumps openshift/api to consume the new feature-gated fields from openshift/api#2487 and openshift/api#2719 and introduces a new TechPreviewNoUpgrade feature gate to keep the feature from going GA. The new gate is wired through to the Control Plane Operator so the additional KAS configuration is only activated when TechPreview is explicitly enabled. Signed-off-by: Shaza Aldawamneh <shaza.aldawamneh@hotmail.com>
5 participants
No description provided.