ListenerSet adjust PortNumber kubebuilder validations #3750
Conversation
f0f9284 to
2797f27
Compare
|
bump |
There was a problem hiding this comment.
Thanks for this PR, @dprotaso! Can we add some validation tests in https://github.com/kubernetes-sigs/gateway-api/blob/main/pkg/test/cel/httproute_standard_test.go, https://github.com/kubernetes-sigs/gateway-api/blob/main/pkg/test/cel/gateway_test.go and add new tests for ListenerSets?
* formatting * clarify optional port semantics * clarify listener name semantics * clarify attachment/grants semantics - Route attachment without sectionName - re-order policy attachment section - include a section about ReferenceGrants * set proper min/max value for listenerentry.port We had to drop the use of the PortNumber type because of limitations with overriding min max using kubebuilder annotations * Update geps/gep-1713/index.md Co-authored-by: Nick Young <inocuo@gmail.com> * Drop validation markers this is handled in another PR See: #3750 * address Nick's feedback * remove stray backtick * address Rob's feedback * incorporate gep changes into godoc --------- Co-authored-by: Nick Young <inocuo@gmail.com>
bbdc171 to
2314651
Compare
controller-tools only allows this to work on object types and not ref types
| // | ||
| // +optional | ||
| // | ||
| // +kubebuilder:default=0 |
There was a problem hiding this comment.
If the default is now 0, is this not a backward compatible change once it goes to standard? Formerly the port was optional, so there could be widespread omitted-port cases that were already properly handled. Now there could be a completely different way that missing ports are handled. Do we need to worry about that?
There was a problem hiding this comment.
If the default is now 0, is this not a backward compatible change once it goes to standard?
I don't think so the ListenerSet is an alpha API so I believe we can make these changes.
Formerly the port was optional
It wasn't actually - maybe that's the confusion here
There was a problem hiding this comment.
This is only non-breaking because we're going from required to optional. But as @dprotaso says, it would be okay to make a breaking change because this is alpha.
|
This LGTM since it's for ListenerSet. Also, I would update the changelog note to remove the word |
@youngnick done |
|
/hold cancel |
|
This LGTM now, so I'll approve. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, youngnick 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 |
|
There are language nits I might be able to pick with this, but it looks good to me overall, so /lgtm I'd really like to see this one followed up quickly with making |
|
/hold cancel |
|
I'd prefer not to make Listeners optional in Gateway until we're confident that this shape for ListenerSet is what we want. Making that change is one we can't walk back once we do it, and it will require a bunch of updates to conformance tests and status expectations. |
|
Right now the GEP states
But we haven't reflected that in any API tags etc. I sorta view this as a implementation detail that we recommend for now. |
|
Yeah, @youngnick, I should've been more clear. I look forward to that day, but I am definitely not advocating that we rush it. 🙂 |
|
I believe this is a bit of an odd change... We previously had This allowed many places to re-use the same validations by just re-using the type This PR changed it to Removing the shared validations. Now having a What's worse, is that it's breaking controller-gen (see slack) and fixing controller gen to support this doesn't look like a very easy fix. IMO we should either remove the |
|
controller-gen or whatever had issues with overwriting markers when doing type aliases. There's an open issue somewhere I believe. The approach taken in the PR was the only way to get this to work in GatewayAPI Adding a new go type for different validations ruins the UX for go authors. Then you could end up with 5 different port numbers types |
|
Honestly the right fix seems to be to introduce the validation back once controller tools has fixed their alias support for overriding markers |
|
We also had to adjust the min/max marker as well because it didn't look at underlying types and just dropped markers See: gateway-api/pkg/generator/markers.go Line 31 in 9098973 |
Nit, there's a difference in Go between a type alias (
What benefit does having to cast all int32s to a PortNumber type provide for Go authors at the moment?
Each with different use cases, specific documentation to those use cases, and the ability to evolve with those use cases without then affecting other use cases inadvertently... That sounds like a good thing to me?
This is to do with the way that controller-gen parses and then flattens the types. I have a fix that would potentially unblock this, but it is perhaps a breaking change, see kubernetes-sigs/controller-tools#1270 |
Yup
I'm assuming it was for the validation markers - until controller-gen didn't work as expected
I'd rather the documentation be on the field use vs. the type (assuming overriding markers works)
Awesome if the change lands we can make the tweaks in this repo. Then in theory it should be a noop once we re-adjust our markers. |
Uh oh!
There was an error while loading. Please reload this page.