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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds feature-gated ExternalOIDCExternalClaimsSourcing to the Authentication API. Introduces new exported types and constants for external claim sources, authentication modes, OAuth2 client-credential and TLS config, URL hostname/path inputs, predicates, and claim mappings. Extends OIDCProvider with an atomic externalClaimsSources list (min 1, max 5) and cross-source uniqueness validation for mapping names. Updates CRD schemas (DevPreview and CustomNoUpgrade) with OpenAPI and x-kubernetes-validations enforcing conditional fields, formats, sizing, and uniqueness rules. Adds an apiextensions.k8s.io/v1 test manifest covering positive and negative validation cases. 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
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
everettraven
force-pushed
the
feature/external-claims-api
branch
from
6498c6e to
a9da673
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml`:
- Around line 786-790: The schema for url.hostname currently allows an optional
":port" but enforces maxLength: 253 which will reject a valid 253-character
hostname with a port appended; update the constraint for the url.hostname field
(the minLength/maxLength entries) to allow the colon and up to 5 port digits by
increasing maxLength from 253 to 259, or alternatively split host and port into
separate fields (e.g., hostname and port) and validate each independently; make
the change where url.hostname's maxLength/minLength are defined so the optional
":65535" can be accommodated.
- Around line 605-611: Replace the raw-string check for user info (the rule
"self.find('@') == ''" under the "tokenEndpoint must not have user info"
message) with a parsed-URL userinfo check so paths containing '@' are not
rejected; change the rule to use the URL parser like "url(self).getUserInfo() ==
''" (or the equivalent parsed userinfo accessor in this policy language) so only
authority userinfo is disallowed while allowing '@' in the path.
🪄 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: 49bac58b-6fe1-4b05-b238-bdca3f196925
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml
- config/v1/types_authentication.go
everettraven
force-pushed
the
feature/external-claims-api
branch
from
a9da673 to
ee1517b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
config/v1/types_authentication.go (1)
8-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThread
ExternalOIDCExternalClaimsSourcingthrough the rest of the OIDC API surface.This updates the top-level OIDC validation and
spec.oidcProviders, butAuthenticationTypestill does not allowOIDCfor this gate andstatus.oidcClientsis still guarded by the older gate set. With onlyExternalOIDCExternalClaimsSourcingenabled, the new manifests in this PR can still be rejected or lose the existingoidcClientsinvariant.Also applies to: 94-94
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_authentication.go` at line 8, The top-level validation and spec.oidcProviders were updated for ExternalOIDCExternalClaimsSourcing, but you must also propagate that feature gate through AuthenticationType checks and the status.oidcClients guard so enabling only ExternalOIDCExternalClaimsSourcing doesn't leave AuthenticationType disallowing OIDC or keep status.oidcClients behind the old gate; update the feature-gate annotations and validation conditions that reference AuthenticationType and status.oidcClients to include ExternalOIDCExternalClaimsSourcing (in the same style as the long openshift:validation tag and inside any code paths that validate AuthenticationType == OIDC or access status.oidcClients) so the new gate is threaded consistently across AuthenticationType, spec.oidcProviders, and status.oidcClients.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1/types_authentication.go`:
- Around line 881-886: The godoc comment for the mappings field contains a stale
TODO; remove the line "TODO: Should this hold true? If so, validate it." from
the comment block that documents mappings in config/v1/types_authentication.go
so the API contract reads as definitive (keep the rest of the text about
required entries, min/max and uniqueness) and do not change any validation
logic—references to mappings and externalClaimsSources should remain intact.
---
Duplicate comments:
In `@config/v1/types_authentication.go`:
- Line 8: The top-level validation and spec.oidcProviders were updated for
ExternalOIDCExternalClaimsSourcing, but you must also propagate that feature
gate through AuthenticationType checks and the status.oidcClients guard so
enabling only ExternalOIDCExternalClaimsSourcing doesn't leave
AuthenticationType disallowing OIDC or keep status.oidcClients behind the old
gate; update the feature-gate annotations and validation conditions that
reference AuthenticationType and status.oidcClients to include
ExternalOIDCExternalClaimsSourcing (in the same style as the long
openshift:validation tag and inside any code paths that validate
AuthenticationType == OIDC or access status.oidcClients) so the new gate is
threaded consistently across AuthenticationType, spec.oidcProviders, and
status.oidcClients.
🪄 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: 72fbd2c4-e141-4c12-b8ef-54ee46817177
⛔ 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
✅ Files skipped from review due to trivial changes (2)
- payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yaml
everettraven
force-pushed
the
feature/external-claims-api
branch
from
ee1517b to
4affa2a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/v1/types_authentication.go (1)
94-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
ExternalOIDCExternalClaimsSourcingacross the rest of the OIDC surface.
spec.oidcProvidersis enabled for the new gate here, butAuthenticationTypestill does not allowOIDCunder that gate andstatus.oidcClientsis still missing it as well. With onlyExternalOIDCExternalClaimsSourcingenabled, the new create cases can still fail beforeexternalClaimsSourcesis even validated.Suggested follow-up
-// +openshift:enable:FeatureGate=ExternalOIDC -// +openshift:enable:FeatureGate=ExternalOIDCWithUIDAndExtraClaimMappings +// +openshift:enable:FeatureGate=ExternalOIDC +// +openshift:enable:FeatureGate=ExternalOIDCWithUIDAndExtraClaimMappings +// +openshift:enable:FeatureGate=ExternalOIDCWithUpstreamParity +// +openshift:enable:FeatureGate=ExternalOIDCExternalClaimsSourcing // +optional OIDCClients []OIDCClientStatus `json:"oidcClients"` -// +openshift:validation:FeatureGateAwareEnum:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings,enum="";None;IntegratedOAuth;OIDC +// +openshift:validation:FeatureGateAwareEnum:featureGate=ExternalOIDC;ExternalOIDCWithUIDAndExtraClaimMappings;ExternalOIDCWithUpstreamParity;ExternalOIDCExternalClaimsSourcing,enum="";None;IntegratedOAuth;OIDC type AuthenticationType string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_authentication.go` around lines 94 - 96, The new feature gate ExternalOIDCExternalClaimsSourcing is enabled for spec.oidcProviders but not propagated elsewhere; update AuthenticationType to allow the OIDC enum value when ExternalOIDCExternalClaimsSourcing is enabled (modify the validation/enum logic around AuthenticationType), and ensure status.oidcClients includes OIDC-based entries when the gate is present (add gating checks where status is constructed/updated to append OIDC client info tied to spec.oidcProviders and externalClaimsSources). Also audit any create/validation paths that currently reject OIDC when the gate is set and modify them to consult ExternalOIDCExternalClaimsSourcing so new create flows don’t fail before externalClaimsSources are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@config/v1/types_authentication.go`:
- Around line 94-96: The new feature gate ExternalOIDCExternalClaimsSourcing is
enabled for spec.oidcProviders but not propagated elsewhere; update
AuthenticationType to allow the OIDC enum value when
ExternalOIDCExternalClaimsSourcing is enabled (modify the validation/enum logic
around AuthenticationType), and ensure status.oidcClients includes OIDC-based
entries when the gate is present (add gating checks where status is
constructed/updated to append OIDC client info tied to spec.oidcProviders and
externalClaimsSources). Also audit any create/validation paths that currently
reject OIDC when the gate is set and modify them to consult
ExternalOIDCExternalClaimsSourcing so new create flows don’t fail before
externalClaimsSources are validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: aa5bf0e5-8bfc-4dbf-97be-88d9725d1175
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (1)
- payload-manifests/crds/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yaml
|
Holding until openshift/oauth-apiserver#197 makes it in. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
everettraven
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Overall structure lgtm, although I think I may not have all the nuances of the details. Some questions/minor suggestions inline
| // 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. |
There was a problem hiding this comment.
To understand, this is enforced here via the listType=map, and cross objects via the validation on the ExternalClaimsSources field. Should this godoc live in the higher level object instead?
There was a problem hiding this comment.
It certainly doesn't hurt to include it in both locations, so I will update that.
| // ExternalClaimsSource provides the configuration for a single external claim source. | ||
| type ExternalClaimsSource struct { | ||
| // authentication is an optional field that configures how the apiserver authenticates with an external claims source. | ||
| // When not specified, anonymous authentication is used. |
There was a problem hiding this comment.
Is there an expectation that the user should never be able to explicitly set anonymous authentication given the options for this field are RequestProvidedToken;ClientCredential ? What does anonymous authentication mean in this context? (just for my own understanding)
There was a problem hiding this comment.
Is there an expectation that the user should never be able to explicitly set anonymous authentication given the options for this field are RequestProvidedToken;ClientCredential ?
Correct. If you are specifying this field, you should provide the mode of authentication to be used. If you want no authentication to be used (i.e anonymous authentication) then you omit the field altogether.
What does anonymous authentication mean in this context? (just for my own understanding)
In this context it means that the Authorization header is not set in the HTTP request.
| Expression string `json:"expression,omitempty"` | ||
| } | ||
|
|
||
| // CertificateAuthorityConfigMapReference is a reference to a ConfigMap in the openshift-config |
There was a problem hiding this comment.
There's a mismatch between the godoc and type name here.
| // | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[!#-[\\\\]-~]+$')",message="scopes must only contain printable ASCII characters excluding spaces, double quotes and backslashes" | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=256 |
There was a problem hiding this comment.
(minor) wondering if the godoc for length should be here instead of the Scopes[] object
There was a problem hiding this comment.
It should be in both, updating.
| // namespace that contains the CA certificate to use to validate TLS connections with the external claims source. | ||
| // | ||
| // +required | ||
| CertificateAuthority ExternalSourceCertificateAuthorityConfigMapReference `json:"certificateAuthority,omitzero"` |
There was a problem hiding this comment.
Is there an expectation on what the key should be in the configmap, like you have below for the ClientSecret?
There was a problem hiding this comment.
Yes, good catch.
| // | ||
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=259 |
There was a problem hiding this comment.
minor: I guess this accounts for the 6 characters in :xxxxx in the port? Not sure if it's worth trying to be more explicit in the godoc (Something like The total length including the optional port must not exceed 259 characters instead?)
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| PathExpression string `json:"pathExpression,omitempty"` |
There was a problem hiding this comment.
Curious why this is PathExpression and not just Expression like the other fields.
Also wondering if it's worth making this a type since there's 3 references to it, but I guess since we can't really validate whether the CEL expression is correct or not, it doesn't really save us that much duplication?
There was a problem hiding this comment.
I landed on pathExpression because this is explicitly a CEL expression that returns a list of strings used to build the path segment of the URL.
It could be the case that we have to expand this in the future to support query parameters and expressions associated with that so I wanted to make sure we don't run into any future naming collisions and have these fields be as descriptive as possible to what they are used for.
There was a problem hiding this comment.
Also wondering if it's worth making this a type since there's 3 references to it, but I guess since we can't really validate whether the CEL expression is correct or not, it doesn't really save us that much duplication?
It wouldn't save us much duplication and there may be a future where certain expression string constraints need to be relaxed to allow for more complex expressions based on the use cases. Especially this one because it is supposed to be a list of strings that are used to create a URL path.
everettraven
force-pushed
the
feature/external-claims-api
branch
from
4affa2a to
6c725de
Compare
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
| // When omitted, claims are always attempted to be fetched | ||
| // from this external source. |
There was a problem hiding this comment.
attempted to be fetched feels like an awkward phrasing, maybe drop the attempted part?
| // When omitted, claims are always attempted to be fetched | |
| // from this external source. | |
| // When omitted, claims are always fetched from this external source. |
Is there anywhere in this API where it would feel appropriate to explain what happens if the request to fetch the extra claims fails?
There was a problem hiding this comment.
I'll explain that at the top level.
| // +kubebuilder:validation:XValidation:rule="self.matches('^[[:print:]]+$')",message="clientID must only contain printable ASCII characters" | ||
| ClientID string `json:"clientID,omitempty"` | ||
|
|
||
| // clientSecret is a required reference to a Secret in the openshift-config namespace to be used |
There was a problem hiding this comment.
Out of interest, why are the secret here and configmap in certificateAuthority limited to the openshift-config namespace?
There was a problem hiding this comment.
Because this is how existing fields in this API, that are used for similar purposes, are configured. The cluster operator controllers are designed such that configuration values within configmaps/secrets for the components being managed should exist in the openshift-config namespace and get synced over to the appropriate operand namespace where necessary.
End users should already be familiar with this configuration pattern as it exists across many of our APIs for control plane level operand configuration.
| // OAuth2Scope is a string alias that represents an OAuth2 Scope as defined by https://datatracker.ietf.org/doc/html/rfc6749#appendix-A.4 | ||
| // Must be at least 1 character in length, must not exceed 256 characters in length and must only contain printable ASCII characters, excluding spaces, double quotes and backslashes. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[!#-[\\\\]-~]+$')",message="scopes must only contain printable ASCII characters excluding spaces, double quotes and backslashes" |
There was a problem hiding this comment.
This doesn't have the :print: pattern from the other printable validation, does it need it?
There was a problem hiding this comment.
This expression is equivalent to the :print: pattern with the exception of the excluded characters (spaces, double quotes, and backslashes).
I couldn't find a reasonable way to include the use of that :print: pattern while also negating any of the excluded characters.
| // +required | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=259 | ||
| // +kubebuilder:validation:XValidation:rule="isURL('https://'+self)",message="hostname must be a valid hostname" |
There was a problem hiding this comment.
What does this cover that the format validation below doesn't?
There was a problem hiding this comment.
It ensures that the entire hostname string, when prepended with the HTTPS scheme at least parses as a valid URL.
It isn't a super strong validation, but because the underlying logic that performs the request prepends the HTTPS scheme before making a request, I figured it is an easy enough check to make sure we have in place to prevent invalid URL strings.
| // expression must be at least 1 character in length and must not exceed 1024 characters in length. | ||
| // | ||
| // Values in the returned list will be joined with the hostname using a forward slash | ||
| // (`/`) as a separator. Values in the returned list do not need to include the forward slash. |
There was a problem hiding this comment.
What happens if they do include the forward slash? They get a double slash?
There was a problem hiding this comment.
It will be encoded as %2F. I can add a note about this here.
| // +kubebuilder:validation:XValidation:rule="self.split(':').size() > 1 ? int(self.split(':')[1]) <= 65535 : true",message="port must not exceed 65535" | ||
| Hostname string `json:"hostname,omitempty"` | ||
|
|
||
| // pathExpression is a required CEL expression that returns a list |
There was a problem hiding this comment.
I think this is a sufficiently complex field that it might be worth including an example of a valid expression in the godoc
| // will produce a value to be assigned to the claim. | ||
| // The full response body from the request to the | ||
| // external claim source is provided via the | ||
| // `response` variable. |
There was a problem hiding this comment.
How do I know what values are available in this response variable? What are its properties?
There was a problem hiding this comment.
I recently updated this so that the response will always have a body subfield that contains the key-value pairs from the JSON response returned by the external source. I'll make sure this is documented.
Because it is arbitrary, we don't know what response.body will actually contain. It is up to the end user configuring this to understand what the endpoint they are telling us to interact with will return and how to process it via this CEL expression. I'll make sure to make this more clear in the doc here.
| // and false means that it should not. | ||
| // | ||
| // Claims from the token used for the request to the kube-apiserver | ||
| // are made available via the `claims` variable. |
There was a problem hiding this comment.
How would I know the properties available on this variable?
There was a problem hiding this comment.
Similar to the other comment, this is dependent on the claims that are present in the JWT being processed. We don't control that so we cannot concretely define what the properties here are. It is up to the end-user configuring this to understand what information they have configured their identity provider(s) to include in the token. I'll make sure this is clarified in the doc.
external sources Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
everettraven
force-pushed
the
feature/external-claims-api
branch
from
6c725de to
b2c3178
Compare
|
@everettraven: 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. |
|
Thanks for the updates, generally lgtm from my side - marking as shadow review complete |
5 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.