add api changes according to gep-3155 #3304
Conversation
k8s-ci-robot
added
release-note
k8s-ci-robot
added
the
size/L
LiorLieberman
force-pushed
the
gep-3155-impl
branch
from
452e98f
to
99fe698
Compare
k8s-ci-robot
added
size/XL
LiorLieberman
force-pushed
the
gep-3155-impl
branch
from
99fe698
to
a1fcab1
Compare
k8s-ci-robot
added
size/L
LiorLieberman
force-pushed
the
gep-3155-impl
branch
from
a1fcab1
to
8288b78
Compare
k8s-ci-robot
added
the
do-not-merge/invalid-commit-message
LiorLieberman
force-pushed
the
gep-3155-impl
branch
3 times, most recently
from
c131972
to
0877cf2
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
|
/retest |
| // 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. |
There was a problem hiding this comment.
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.
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.
I think I'd leave case sensitivity out of this as it's potentially different depending on the scheme. Per RFC-3986:
When a URI uses components of the generic syntax, the component syntax equivalence rules always apply; namely, that the scheme and host are case-insensitive and therefore should be normalized to lowercase. For example, the URI HTTP://www.EXAMPLE.com/ is equivalent to http://www.example.com/. The other generic syntax components are assumed to be case-sensitive unless specifically defined otherwise by the scheme
https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.2.1
| // 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. |
There was a problem hiding this comment.
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.
| // Support: Core | ||
| // | ||
| // +optional | ||
| // <gateway:experimental> |
There was a problem hiding this comment.
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.
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.
🤔 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.
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.
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.
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.
Co-authored-by: mkosieradzki 10385115+mkosieradzki@users.noreply.github.com
LiorLieberman
force-pushed
the
gep-3155-impl
branch
from
0877cf2
to
a8f6a72
Compare
There was a problem hiding this comment.
Thanks @LiorLieberman! Will defer to someone else for final LGTM.
/approve
|
|
||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern=`^(([^:/?#]+):)(//([^/?#]*))([^?#]*)(\?([^#]*))?(#(.*))?` |
There was a problem hiding this comment.
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?
| // Support: Core | ||
| // | ||
| // +optional | ||
| // <gateway:experimental> |
There was a problem hiding this comment.
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.
k8s-ci-robot
added
the
approved
There was a problem hiding this comment.
Thanks @LiorLieberman!
This looks good:
/approve
However, there seems to be some building consensus that we should not be calling this supported at a Core level so just putting a hold until everyone's had a chance to say their piece and we sort that out.
/hold
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: LiorLieberman, robscott, shaneutt The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I saw you commented +1 on the thread to leave it as core, to indicate our aspirational support level. Am i wrong ? |
Maybe just waiting to ensure @mlavacca is on board with this approach? In any case, I'm fine to hold off on merging this until he can take a look. IMO, this is sufficiently complete within the timeline we'd agreed to to make it into v1.2, just holding off on final merge for one last signoff. |
k8s-ci-robot
removed
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
/kind feature
What this PR does / why we need it:
Update CRDs per gep-3155
Does this PR introduce a user-facing change?:
/cc @mkosieradzki