WIP: CNTRLPLANE-2521: feat: configv1/authentication: add fields for sourcing claims from external sources#2827
Conversation
|
@everettraven: This pull request references CNTRLPLANE-2521 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
size/XXL
📝 WalkthroughWalkthroughAdds feature-gated support 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml (1)
280-943: ⚡ Quick winAdd tests for two contract edges to prevent drift.
Please add cases for:
- duplicate
mappings[].nameacross twoexternalClaimsSourcesentries, and- explicit
predicates: []behavior (accept or reject, based on intended contract).These two edges are currently where schema-vs-contract drift is most likely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml` around lines 280 - 943, Add two test cases to the existing list: (1) "Cannot duplicate mappings.name across externalClaimsSources" that defines two externalClaimsSources each with a mappings entry using the same name (e.g., name: email) and sets expectedError to assert duplication (e.g., "Duplicate value: \"email\"") to catch cross-source duplicate mapping names; (2) "Explicit predicates empty array behavior" that adds an externalClaimsSources entry with predicates: [] and an initial config, and set either expected (if contract allows an explicit empty predicates array) or expectedError (if contract forbids empty predicates) to reflect the intended contract—use the same structures and fields (externalClaimsSources, mappings, predicates) as the surrounding tests so the new entries align with the suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_authentication.go`:
- Around line 893-911: The struct field Predicates is documented to allow
omitted or empty lists but the validation tag +kubebuilder:validation:MinItems=1
prevents an explicit empty slice; remove or change that validation so empty
lists are accepted (e.g., delete the +kubebuilder:validation:MinItems=1 tag or
set it to 0) on the Predicates []ExternalSourcePredicate field and keep the
existing +kubebuilder:validation:MaxItems=16 and list/map tags intact so the
documented semantics match the schema.
---
Nitpick comments:
In
`@config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml`:
- Around line 280-943: Add two test cases to the existing list: (1) "Cannot
duplicate mappings.name across externalClaimsSources" that defines two
externalClaimsSources each with a mappings entry using the same name (e.g.,
name: email) and sets expectedError to assert duplication (e.g., "Duplicate
value: \"email\"") to catch cross-source duplicate mapping names; (2) "Explicit
predicates empty array behavior" that adds an externalClaimsSources entry with
predicates: [] and an initial config, and set either expected (if contract
allows an explicit empty predicates array) or expectedError (if contract forbids
empty predicates) to reflect the intended contract—use the same structures and
fields (externalClaimsSources, mappings, predicates) as the surrounding tests so
the new entries align with the suite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 1e34b0e9-5d75-41d8-a767-d23db5e399c1
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (4)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yamlconfig/v1/types_authentication.gopayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
| // mappings is a required list of the claim | ||
| // and response handling expression pairs | ||
| // that produces the claims from the external source. | ||
| // mappings must have at least 1 entry and must not exceed 16 entries. | ||
| // Entries must have a unique name across all external claim sources. | ||
| // | ||
| // +required | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Mappings []SourcedClaimMapping `json:"mappings,omitempty"` |
There was a problem hiding this comment.
Enforce (or relax) cross-source claim-name uniqueness contract.
Line 884 states mapping names must be unique across all external claim sources, but Lines 887-891 only enforce uniqueness within one source (listMapKey=name on mappings). This allows duplicate produced claim names across different sources, which can create ambiguous/last-write-wins claim behavior.
Please either add cross-source validation (preferred) or narrow the contract text to per-source uniqueness.
| // predicates is an optional list of constraints in | ||
| // which claims should attempt to be fetched from this | ||
| // external source. | ||
| // | ||
| // When omitted or empty, claims are always attempted to be fetched | ||
| // from this external source. | ||
| // | ||
| // When specified, all predicates must evaluate to 'true' | ||
| // before claims are attempted to be fetched from this external source. | ||
| // | ||
| // predicates must not exceed 16 entries. | ||
| // Entries must have unique expressions. | ||
| // | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=expression | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| Predicates []ExternalSourcePredicate `json:"predicates,omitempty"` |
There was a problem hiding this comment.
predicates: [] is documented as valid but currently rejected.
Line 897 says omitted or empty predicates should behave the same, but Line 909 (MinItems=1) rejects an explicit empty list. Please align docs and validation semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_authentication.go` around lines 893 - 911, The struct field
Predicates is documented to allow omitted or empty lists but the validation tag
+kubebuilder:validation:MinItems=1 prevents an explicit empty slice; remove or
change that validation so empty lists are accepted (e.g., delete the
+kubebuilder:validation:MinItems=1 tag or set it to 0) on the Predicates
[]ExternalSourcePredicate field and keep the existing
+kubebuilder:validation:MaxItems=16 and list/map tags intact so the documented
semantics match the schema.
external sources Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/external-claims-api
branch
from
09e520a to
6498c6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml (1)
918-943: ⚡ Quick winAdd the matching invalid-name case for
authentication.clientCredential.tls.certificateAuthority.This manifest only exercises the DNS-name validation for the top-level
externalClaimsSources[].tls.certificateAuthority, but the same validation is duplicated underexternalClaimsSources[].authentication.clientCredential.tls.certificateAuthority. A broken schema generation in that nested branch would currently go uncaught.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml` around lines 918 - 943, Add a second invalid-name test case that mirrors the existing externalClaimsSources TLS CA check but for the nested authentication.clientCredential.tls.certificateAuthority path: update the YAML in ExternalOIDCExternalClaimsSourcing.yaml to include an authentication: clientCredential: tls: certificateAuthority: name: "INVALID_NAME!" entry under the same externalClaimsSources item and assert the same expectedError string ("name must start and end with a lowercase alphanumeric character, and must only contain lowercase alphanumeric characters, '-' or '.'") so the schema validation for externalClaimsSources[].authentication.clientCredential.tls.certificateAuthority is exercised just like externalClaimsSources[].tls.certificateAuthority.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_authentication.go`:
- Around line 94-96: The new FeatureGate annotation
ExternalOIDCExternalClaimsSourcing was added to OIDCProviders but not propagated
across the OIDC-related API surface; update the API types and annotations so the
gate consistently enables OIDC everywhere: add the same
+openshift:enable:FeatureGate=ExternalOIDCExternalClaimsSourcing (and +optional
where appropriate) to the AuthenticationType enum entry that allows "OIDC", to
the OIDCClients (oidcClients) type/status/validation annotations, and any
xValidation annotations tied to OIDC so that oidcProviders, oidcClients, and
AuthenticationType check the same gate; locate symbols AuthenticationType,
OIDCProviders, oidcClients, and related xValidation annotations in
types_authentication.go and the OIDC client/status definitions and add the gate
annotation to each corresponding declaration to maintain consistent gating.
---
Nitpick comments:
In
`@config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yaml`:
- Around line 918-943: Add a second invalid-name test case that mirrors the
existing externalClaimsSources TLS CA check but for the nested
authentication.clientCredential.tls.certificateAuthority path: update the YAML
in ExternalOIDCExternalClaimsSourcing.yaml to include an authentication:
clientCredential: tls: certificateAuthority: name: "INVALID_NAME!" entry under
the same externalClaimsSources item and assert the same expectedError string
("name must start and end with a lowercase alphanumeric character, and must only
contain lowercase alphanumeric characters, '-' or '.'") so the schema validation
for
externalClaimsSources[].authentication.clientCredential.tls.certificateAuthority
is exercised just like externalClaimsSources[].tls.certificateAuthority.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f0339821-9928-47c2-b13d-2bb067849bbb
⛔ Files ignored due to path filters (8)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (4)
config/v1/tests/authentications.config.openshift.io/ExternalOIDCExternalClaimsSourcing.yamlconfig/v1/types_authentication.gopayload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
| // +openshift:enable:FeatureGate=ExternalOIDCExternalClaimsSourcing | ||
| // +optional | ||
| OIDCProviders []OIDCProvider `json:"oidcProviders,omitempty"` |
There was a problem hiding this comment.
Propagate ExternalOIDCExternalClaimsSourcing across the rest of the OIDC API gates.
Adding the new gate only to spec.oidcProviders leaves the API internally inconsistent: AuthenticationType still does not allow OIDC for this gate, and the existing OIDC oidcClients status/XValidation annotations are still tied to the older gates. The new validation manifest enables only ExternalOIDCExternalClaimsSourcing, so these create cases can still fail or lose existing OIDC invariants unless the gate is threaded through the rest of the OIDC surface too.
Suggested annotation updates
-// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings;ExternalOIDCWithUpstreamParity,rule="!has(self.spec.oidcProviders) || self.spec.oidcProviders.all(p, !has(p.oidcClients) || p.oidcClients.all(specC, self.status.oidcClients.exists(statusC, statusC.componentNamespace == specC.componentNamespace && statusC.componentName == specC.componentName) || (has(oldSelf.spec.oidcProviders) && oldSelf.spec.oidcProviders.exists(oldP, oldP.name == p.name && has(oldP.oidcClients) && oldP.oidcClients.exists(oldC, oldC.componentNamespace == specC.componentNamespace && oldC.componentName == specC.componentName)))))",message="all oidcClients in the oidcProviders must match their componentName and componentNamespace to either a previously configured oidcClient or they must exist in the status.oidcClients"
+// +openshift:validation:FeatureGateAwareXValidation:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings;ExternalOIDCWithUpstreamParity;ExternalOIDCExternalClaimsSourcing,rule="!has(self.spec.oidcProviders) || self.spec.oidcProviders.all(p, !has(p.oidcClients) || p.oidcClients.all(specC, self.status.oidcClients.exists(statusC, statusC.componentNamespace == specC.componentNamespace && statusC.componentName == specC.componentName) || (has(oldSelf.spec.oidcProviders) && oldSelf.spec.oidcProviders.exists(oldP, oldP.name == p.name && has(oldP.oidcClients) && oldP.oidcClients.exists(oldC, oldC.componentNamespace == specC.componentNamespace && oldC.componentName == specC.componentName)))))",message="all oidcClients in the oidcProviders must match their componentName and componentNamespace to either a previously configured oidcClient or they must exist in the status.oidcClients"
-// +openshift:validation:FeatureGateAwareEnum:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings,enum="";None;IntegratedOAuth;OIDC
+// +openshift:validation:FeatureGateAwareEnum:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings;ExternalOIDCWithUpstreamParity;ExternalOIDCExternalClaimsSourcing,enum="";None;IntegratedOAuth;OIDC
-// +openshift:enable:FeatureGate=ExternalOIDC
-// +openshift:enable:FeatureGate=ExternalOIDCWithUIDAndExtraClaimMappings
+// +openshift:enable:FeatureGate=ExternalOIDC
+// +openshift:enable:FeatureGate=ExternalOIDCWithUIDAndExtraClaimMappings
+// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity
+// +openshift:enable:FeatureGate=ExternalOIDCExternalClaimsSourcing🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_authentication.go` around lines 94 - 96, The new FeatureGate
annotation ExternalOIDCExternalClaimsSourcing was added to OIDCProviders but not
propagated across the OIDC-related API surface; update the API types and
annotations so the gate consistently enables OIDC everywhere: add the same
+openshift:enable:FeatureGate=ExternalOIDCExternalClaimsSourcing (and +optional
where appropriate) to the AuthenticationType enum entry that allows "OIDC", to
the OIDCClients (oidcClients) type/status/validation annotations, and any
xValidation annotations tied to OIDC so that oidcProviders, oidcClients, and
AuthenticationType check the same gate; locate symbols AuthenticationType,
OIDCProviders, oidcClients, and related xValidation annotations in
types_authentication.go and the OIDC client/status definitions and add the gate
annotation to each corresponding declaration to maintain consistent gating.
2 participants
Description
Updates the
authentications.config.openshift.io/v1API to add new fields for sourcing claims from external sources (i.e not from the token being authenticated).This PR is based on the changes in openshift/oauth-apiserver#197 which adds all the fields to the configuration file that this API is used to generate.