-
Notifications
You must be signed in to change notification settings - Fork 472
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 api changes according to gep-3155 #3304
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -535,6 +535,19 @@ type Hostname string | |
// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` | ||
type PreciseHostname string | ||
|
||
// AbsoluteURI represents a Uniform Resource Identifier (URI) as defined by RFC3986. | ||
|
||
// The AbsoluteURI MUST NOT be a relative URI, and it MUST follow the URI syntax and | ||
// encoding rules specified in RFC3986. The AbsoluteURI MUST include both a | ||
// scheme (e.g., "http" or "spiffe") and a scheme-specific-part. URIs that | ||
// include an authority MUST include a fully qualified domain name or | ||
// IP address as the host. | ||
Comment on lines
+538
to
+544
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how we should approach for case-sensitivity, scheme and host aren't but other parts can be. Let me know if you want to add any language to indicate what we expect There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe "implementations SHOULD treat this string case insensitively" or something? Feels like it's better to have some stance on case sensitivity rather than none. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'd leave case sensitivity out of this as it's potentially different depending on the scheme. Per RFC-3986:
https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.2.1 |
||
|
||
// +kubebuilder:validation:MinLength=1 | ||
// +kubebuilder:validation:MaxLength=253 | ||
// +kubebuilder:validation:Pattern=`^(([^:/?#]+):)(//([^/?#]*))([^?#]*)(\?([^#]*))?(#(.*))?` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking nit: Would be nice to have a way to track the source of this RegEx for future readers. Can you leave a comment in #3306 with the source of this validation? |
||
type AbsoluteURI string | ||
|
||
// Group refers to a Kubernetes Group. It must either be an empty string or a | ||
// RFC 1123 subdomain. | ||
// | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 was kind of confused when I saw this new experimental feature to be set as core. This can be problematic, as when we graduate this feature to GA, this will be a breaking change: all implementation will have to support it, otherwise conformance won't be given. I think this is a bit problematic, but I'd like to ask others' opinion (@robscott @youngnick @shaneutt).
I just found out we already have a precedent, with the infrastructure Gateway feature.
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.
Fair. I think before graduating this to GA we should have at least (3?) implementations implementing it, right?
I am not sure if there are guidelines on whether a feature should have to graduate as extended, I think support level and channel are two different things. I believe if we move any feature to Core support, it would mean some implementations would loose conformance.
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 actually might be sufficiently portable that everyone could implement it. I know we default to "extended" for so many things, that it's rare to add a new core feature, but this particular one seems like it could be worth aiming for. In general, I'd say the support level that gets merged in experimental is more of a target than a reality since it's still locked behind experimental CRDs. So I'm actually in favor of starting with "core" as a signal that we hope and expect this to graduate at that level, so implementations have a lot of time (2 releases) to call out issues with this goal. I think it would be less disruptive to move from "core" to "extended" in the future than the other way around.
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'm in favor of starting in extended and then we can start communicating a desire to move it to core as soon as we like, giving some soak time for implementations to think about that.
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.
My thinking is that nothing in experimental channel can really be "core" anyways, so the support level here is more aspirational than anything. So at this point we're signaling that we think this can be "core", and thus giving the maximum possible notice that that's what it may graduate to standard as. Any alternative seems like it would either result in less notice to implementations or graduating the feature as "extended" and then having to manage a second transition to "core". I'd rather just start with a target of what we think we can achieve and then accept pushback to a lower level before this feature graduates if we think that's necessary.
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.
+1
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'm fine with having this targeted as core, and agree when you say that in case we'll change it in the future, the core->extended transition is less problematic than the extended->core. One thing we should be very careful about is clearly stating to implementations that when an experimental core feature graduates, that's a breaking change (i.e., all the implementations MUST implement it). We need to communicate this ahead of time, to avoid that implementation suddenly and inexpectedly see their conformance broken.