Add distinct concept to Listener definition #2436

youngnick · 2023-09-27T07:34:14Z

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?:

Updated rules about Listener uniqueness to use the term `distinct`

k8s-ci-robot · 2023-09-27T07:34:24Z

[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:

~~OWNERS~~ [youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

youngnick · 2023-09-27T07:34:32Z

/hold

for multiple review

youngnick · 2023-09-27T07:37:29Z

apis/v1beta1/gateway_types.go

-	// The .listener.hostname field is used to route traffic that has already
-	// arrived at the Gateway to the correct in-cluster destination.
-	//


I removed this because it's just confusing to talk about hostname here - it's not relevant at all.

apis/v1beta1/gateway_types.go

robscott

Thanks @youngnick! Lots of great improvements here.

apis/v1beta1/gateway_types.go

robscott · 2023-09-28T00:55:38Z

apis/v1beta1/gateway_types.go

+    // 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"`.


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.

Yeah, I thought it was better to err on the side of over-specific since we've been bitten here before.

apis/v1beta1/gateway_types.go

robscott

Thanks @youngnick! Lots of great improvements here.

apis/v1beta1/gateway_types.go

robscott · 2023-10-05T05:54:51Z

apis/v1beta1/gateway_types.go

+	// 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


Do we want this to happen? Maybe this should only be recommended if a Gateway has already been accepted?

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.

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).

Suggested change

// 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

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.

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.

apis/v1beta1/gateway_types.go

robscott

Thanks @youngnick!

apis/v1beta1/gateway_types.go

robscott · 2023-10-10T00:25:19Z

apis/v1beta1/gateway_types.go

+	// 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


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).

Suggested change

// 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

Signed-off-by: Nick Young <nick@isovalent.com>

robscott

Thanks @youngnick!

/lgtm

robscott · 2023-10-11T00:04:15Z

apis/v1beta1/gateway_types.go

+	// 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.


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>

robscott · 2023-10-11T21:55:13Z

/lgtm

youngnick · 2023-10-12T00:32:30Z

/unhold

k8s-ci-robot requested review from bowei and shaneutt September 27, 2023 07:34

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023

youngnick commented Sep 27, 2023

View reviewed changes

shaneutt requested a review from robscott September 27, 2023 13:07

shaneutt reviewed Sep 27, 2023

View reviewed changes

apis/v1beta1/gateway_types.go Outdated Show resolved Hide resolved

apis/v1beta1/gateway_types.go Outdated Show resolved Hide resolved

apis/v1beta1/gateway_types.go Outdated Show resolved Hide resolved

apis/v1beta1/gateway_types.go Outdated Show resolved Hide resolved

robscott reviewed Sep 28, 2023

View reviewed changes

youngnick force-pushed the distinct-listeners branch from 9a9e0c1 to d1362db Compare September 28, 2023 04:18

shaneutt self-requested a review September 28, 2023 13:13

robscott reviewed Oct 5, 2023

View reviewed changes

youngnick force-pushed the distinct-listeners branch 2 times, most recently from d34d42d to 8e68d61 Compare October 9, 2023 11:20

shaneutt added this to the v1.0.0 milestone Oct 9, 2023

youngnick force-pushed the distinct-listeners branch from 8e68d61 to 26fcc50 Compare October 9, 2023 21:17

robscott reviewed Oct 10, 2023

View reviewed changes

youngnick force-pushed the distinct-listeners branch from 26fcc50 to e8d7e0d Compare October 10, 2023 23:56

youngnick added 2 commits October 10, 2023 23:57

Add distinct concept to Listener definition

97e9ac1

Signed-off-by: Nick Young <nick@isovalent.com>

Fix PR comments

16ea07f

Signed-off-by: Nick Young <nick@isovalent.com>

youngnick force-pushed the distinct-listeners branch from e8d7e0d to c35711d Compare October 10, 2023 23:57

robscott reviewed Oct 11, 2023

View reviewed changes

k8s-ci-robot assigned robscott Oct 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023

Fix further PR comments

3abcca7

Signed-off-by: Nick Young <nick@isovalent.com>

youngnick force-pushed the distinct-listeners branch from c35711d to 3abcca7 Compare October 11, 2023 04:58

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023

k8s-ci-robot merged commit 85a9236 into kubernetes-sigs:main Oct 12, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add distinct concept to Listener definition #2436

Add distinct concept to Listener definition #2436

youngnick commented Sep 27, 2023

k8s-ci-robot commented Sep 27, 2023

youngnick commented Sep 27, 2023

youngnick Sep 27, 2023

robscott left a comment

robscott Sep 28, 2023

youngnick Sep 28, 2023

robscott left a comment

robscott Oct 5, 2023

youngnick Oct 9, 2023

robscott Oct 10, 2023

youngnick Oct 10, 2023

robscott Oct 10, 2023

robscott left a comment

robscott Oct 10, 2023

robscott left a comment

robscott Oct 11, 2023

robscott commented Oct 11, 2023

youngnick commented Oct 12, 2023

	// 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`.

Add distinct concept to Listener definition #2436

Add distinct concept to Listener definition #2436

Conversation

youngnick commented Sep 27, 2023

k8s-ci-robot commented Sep 27, 2023

youngnick commented Sep 27, 2023

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Oct 11, 2023

youngnick commented Oct 12, 2023