-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add k8s name related formats #384
base: master
Are you sure you want to change the base?
Conversation
are non-standard format strings supposed to be namespaced or not? what are the implications for CRDs created using these format strings prior to this, allowing in data that ignores those format requirements, then auto-tightening validation when upgrading to a control plane with this support added? |
I have not found any mention of namespacing in the specs. xref https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-validation-01#name-custom-format-attributes I am open to naming suggestions for these formats though. I had briefly considered something like "kubernetes-name" but couldn't think of anything that aided rather than harmed clarity on what the format actually is.
Yes, exactly. Today, supported formats is used strip out unsupported |
/assign @apelisse Would you be willing to review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good thanks
Thanks, looks better to me :-) |
pkg/validation/strfmt/default.go
Outdated
@@ -124,6 +124,48 @@ func IsEmail(str string) bool { | |||
return e == nil && addr.Address != "" | |||
} | |||
|
|||
const dns1123LabelFmt string = "[a-z0-9]([-a-z0-9]*[a-z0-9])?" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we're copying/pasting from apimachinery validation, are we inheriting these issues:
- This requires things to be lowercase, which is more strict than the RFC
kubernetes/kubernetes#79351 (comment)
- This tolerates individual labels exceeding 63 chars, which is not as strict as the RFC
Should we fix those issues before adopting those formats here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We intend to publish OpenAPI the in-tree types using these formats. So I think we're stuck inheriting the it all.
That said, we shouldn't using the RFC identifiers for formats that are not exactly compliant with the RFC. I'll propose some alternative names.
pkg/validation/strfmt/default.go
Outdated
dns1123subdomain := DNS1123Subdomain("") | ||
Default.Add("dns1123subdomain", &dns1123subdomain, IsDNS1123Subdomain) | ||
|
||
dns1123label := DNS1123Label("") | ||
Default.Add("dns1123label", &dns1123label, IsDNS1123Label) | ||
|
||
dns1035label := DNS1035Label("") | ||
Default.Add("dns1035label", &dns1035label, IsDNS1035Label) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if these get added to the default set of recognized formats here, how do we roll them out in a controlled way in kubernetes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was held until CRD Ratcheting was promoted to beta in 1.30, we'll leverage that capability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update this code so we can manage the newly added formats separate from the pre-existing ones.
I'm not sure I understand how that makes existing persisted CR data safe. However long we take to roll out support for the format, at some point, would we start treating existing persisted data that didn't match the specified format as invalid? |
We could solve this together with the more general problem of CRD validation ratcheting. I.e. the case where a CRD author adds a format to a CRD, and there already persisted CR data which then immediately becomes invalid. The case we are discussing is similar except that instead of the CRD author adding a format, the CRD author already had a non-validating format in the CRD and then Kubernetes changed the format to be validating. Ratcheting rule would be: if any CR data fails validation but is unchanged from the persisted value, we ignore the validation failure and allow the update. EDIT: This is also what we need for rollback. Note that because any format is allowed on any CRD, this would allow for rollback to arbitrary old versions. |
I agree a solution to that would resolve the issue with this. Shouldn't we get that solution in place before adding this? This would be the first example I can think of where we made a change that would reinterpret existing persisted CRDs in ways that would invalidate existing data |
SGTM. I'll poke around and see who might be able to implement. |
/close |
@jpbetz: Closed this PR. In 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/test-infra repository. |
Just updating progress on this front was made with CRD Ratcheting Alpha added in 1.28: https://kep.k8s.io/4008 Will need to wait for it to mature before using it for name formats, but in the interim we can use inline OpenAPI schemas to describe the regex/cel expressions used for most formats. |
/reopen CRD Ratcheting was promoted to beta in 1.30, freeing us up to resume progress on this. |
@jpbetz: Reopened this PR. In 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jpbetz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Organize k8s names formats into their own registry for ratcheted rollout, prefix with k8s-
This adds the three most heavily used name formats in k8s:
"k8s-dns1123subdmain"
"k8s-dns1123label"
"k8s-dns1035label"
The
k8s-
prefix is added because these formats:Incorrect max length check on metadata.name kubernetes#79351)
Benefits:
xref: https://json-schema.org/draft/2020-12/json-schema-validation#name-custom-format-attributes, https://spec.openapis.org/registry/format/