OCPBUGS-56908: validate identity provider names as annotation keys - #3025
OCPBUGS-56908: validate identity provider names as annotation keys#3025agawai wants to merge 1 commit into
Conversation
Reject identity provider names that cannot be used in the oauth.openshift.io/idp.<name> Group annotation so OpenID group sync cannot break login. Existing names are grandfathered until changed. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@agawai: This pull request references Jira Issue OCPBUGS-56908, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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/L
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe OAuth API now validates identity provider names against the Kubernetes annotation name-part pattern. Existing unchanged invalid names remain allowed through the grandfather clause. The generated CRD includes the same validation and documentation. Tests cover valid names, names containing spaces, provider field updates, invalid provider additions, and valid provider renames. Merge Risk: ⚪ Minimal · up to OAuth identity provider names now reject annotation-incompatible new values while allowing unchanged legacy names to be updated. The validation, published schema, and covered update behavior are aligned, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
Hello @agawai! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @agawai. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
| // name is also used as the name-part of the Group annotation | ||
| // oauth.openshift.io/idp.<name> during OpenID group sync, so it must be a valid | ||
| // Kubernetes annotation name (alphanumeric, '-', '_', '.', at most 59 characters). | ||
| // Existing names that predate this restriction are grandfathered until changed. | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="self.all(idp, (idp.name.matches('^[A-Za-z0-9]([-A-Za-z0-9_.]*[A-Za-z0-9])?$') && size(idp.name) <= 59) || (oldSelf.hasValue() && oldSelf.value().exists(old, old.name == idp.name)))",message="identity provider name must consist of alphanumeric characters, '-', '_' or '.', start and end with an alphanumeric character, and be at most 59 characters because it is used as a Kubernetes annotation key during group sync",optionalOldSelf=true |
There was a problem hiding this comment.
Instead of this being a validation on this field, we can probably just add equivalent validation on the actual Name field within the IdentityProvider type.
| apiVersion: config.openshift.io/v1 | ||
| kind: OAuth | ||
| spec: {} | ||
| - name: Should be able to create an identity provider with a valid annotation-key name |
There was a problem hiding this comment.
We also want to test that the change to the validation successfully ratchets. The tests in
api/example/v1/tests/stableconfigtypes.example.openshift.io/AAA_ungated.yaml
Lines 902 to 964 in 8127870
3 participants
Summary
spec.identityProviderssonamematches the annotation name-part used by oauth-server (oauth.openshift.io/idp.<name>): alphanumeric,-,_,., start and end alphanumeric, at most 59 characters. This is the OCPBUGS-56908 failure (Microsoft Entra ID,AIF - Keycloak).optionalOldSelfso existing clusters that already have those names can still update the OAuth object until the name is changed. New names with spaces are rejected at admission.Test plan
config/v1/tests/oauths.config.openshift.io/AAA_ungated.yamlcover valid names, names with spaces, adding a new invalid IdP, and renaming to a valid nameverify/ integration tests foroauths.config.openshift.ioMade with Cursor