-
Notifications
You must be signed in to change notification settings - Fork 468
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 distinct concept to Listener definition #2436
Add distinct concept to Listener definition #2436
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
/hold for multiple review |
// The .listener.hostname field is used to route traffic that has already | ||
// arrived at the Gateway to the correct in-cluster destination. | ||
// |
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 removed this because it's just confusing to talk about hostname
here - it's not relevant at all.
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.
Thanks @youngnick! Lots of great improvements here.
apis/v1beta1/gateway_types.go
Outdated
// The wildcard character will match any number of characters _and dots_ to | ||
// the left, however, so `"*.example.com"` will match both | ||
// `"foo.bar.example.com"` _and_ `"bar.example.com"`. |
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 thought we'd already had guidance for this on Listener, but I can't find it. We do already have guidance on HTTPRoute that matches this:
gateway-api/apis/v1beta1/httproute_types.go
Lines 86 to 88 in 789326b
// Hostnames that are prefixed with a wildcard label (`*.`) are interpreted | |
// as a suffix match. That means that a match for `*.example.com` would match | |
// both `test.example.com`, and `foo.test.example.com`, but not `example.com`. |
I think we're ok since this was already specified on HTTPRoute, but want to call this out in case it surprises anyone.
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.
Yeah, I thought it was better to err on the side of over-specific since we've been bitten here before.
9a9e0c1
to
d1362db
Compare
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.
Thanks @youngnick! Lots of great improvements here.
apis/v1beta1/gateway_types.go
Outdated
// Implementations MAY choose to accept a Gateway with Conflicted | ||
// Listeners if they accept a partial Listener set that contains no | ||
// Conflicted Listeners. They MUST set a "ListenersNotValid" condition |
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.
Do we want this to happen? Maybe this should only be recommended if a Gateway has already been accepted?
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 allows breaking changes to only affect the things that are broken rather than the entire Gateway - I thought that this would be desirable because of 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.
I think we're saying the same thing here. Basically I'm just trying to make it clear that implementations should only accept conflicted listeners if the Gateway has already been accepted. (I think you're basically saying the same thing in your follow up, but let me know if I'm misunderstanding).
// Implementations MAY choose to accept a Gateway with Conflicted | |
// Listeners if they accept a partial Listener set that contains no | |
// Conflicted Listeners. They MUST set a "ListenersNotValid" condition | |
// If a Gateway has already been Accepted and a Listener becomes invalid | |
// or conflicted, implementations MAY choose to support the subset of Listeners | |
// that are still valid and distinct. They MUST set a "ListenersNotValid" condition |
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 don't like the idea of the spec making statements that involve the implementation holding state. Practically, the implementation will hold state, but the more we can have the decisions about Accepted not require referencing the state, the better off we'll be.
To put this another way, why should the "Already accepted" state be treated differently than the "new Gateway" state? All this is saying is, if you accept any part of a set of Listeners, it has to have no conflicts. Rereading what I have, I can see that I haven't said that very clearly though. Let me update and see what you think.
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.
To clarify, I was actually meaning if the Gateway status already had "Accepted" set to true. Agree that we should not require implementations to hold state.
d34d42d
to
8e68d61
Compare
8e68d61
to
26fcc50
Compare
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.
Thanks @youngnick!
apis/v1beta1/gateway_types.go
Outdated
// Implementations MAY choose to accept a Gateway with Conflicted | ||
// Listeners if they accept a partial Listener set that contains no | ||
// Conflicted Listeners. They MUST set a "ListenersNotValid" condition |
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 think we're saying the same thing here. Basically I'm just trying to make it clear that implementations should only accept conflicted listeners if the Gateway has already been accepted. (I think you're basically saying the same thing in your follow up, but let me know if I'm misunderstanding).
// Implementations MAY choose to accept a Gateway with Conflicted | |
// Listeners if they accept a partial Listener set that contains no | |
// Conflicted Listeners. They MUST set a "ListenersNotValid" condition | |
// If a Gateway has already been Accepted and a Listener becomes invalid | |
// or conflicted, implementations MAY choose to support the subset of Listeners | |
// that are still valid and distinct. They MUST set a "ListenersNotValid" condition |
26fcc50
to
e8d7e0d
Compare
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
e8d7e0d
to
c35711d
Compare
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.
Thanks @youngnick!
/lgtm
// Implementations MAY choose to accept a Gateway with some Conflicted | ||
// Listeners only if they only accept the partial Listener set that contains | ||
// no Conflicted Listeners. To put this another way, implementations may | ||
// accept a partial Listener set only if they throw out *all* the conflicting | ||
// Listeners. No picking one of the conflicting listeners as the winner. | ||
// This also means that the Gateway must have at least one non-conflicting | ||
// Listener in this case, otherwise it violates the requirement that at | ||
// least one Listener must be present. |
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.
On first read I thought this guidance might be problematic for any implementations that revert to the last known good state. Thinking about it more, this just guards if you can accept a Gateway, but I don't think it prevents implementations from falling back to the last known good state instead of dropping Listeners. No action required here, just writing this thought process out for posterity in case this comes up again.
Signed-off-by: Nick Young <nick@isovalent.com>
c35711d
to
3abcca7
Compare
/lgtm |
/unhold |
What type of PR is this?
/kind cleanup
/area conformance
What this PR does / why we need it:
This updates the Listener language to add the concept of
distinct
. Why add this when it's not a huge change? Well, we actually need to consider if things are distinct in many places in the API, and I think that standardizing on this term will help.I've built on @rainest's great work in #2288, and attempted to incorporate some items from the discussions there and on #2416.
There's still an outstanding item here - to introduce the
GatewayListenerIsolation
concept, but I have specifically left that out of this PR, to make it easier to review. @robscott is on the hook for a follow-up to address that one.Which issue(s) this PR fixes:
Updates #2416
Does this PR introduce a user-facing change?: