Skip to content
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

GEP-1713: ListenerSets - Standard Mechanism to Merge Gateway Listeners (rev 2) #3213

Open
wants to merge 40 commits into
base: main
Choose a base branch
from

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Jul 23, 2024

This replaces #1863
What type of PR is this?

/kind gep

What this PR does / why we need it:

Outlines a mechanism to merge Gateway Listeners

Which issue(s) this PR fixes:

Fixes #1713

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 23, 2024
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite a few comments here, thanks for keeping working on pushing this forward.

geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work on this @dprotaso!

geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
Copy link
Contributor

@youngnick youngnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM for Provisional (or maybe even Implementable, since we'll need to implement it and try it out) now.

@robscott
Copy link
Member

robscott commented Aug 2, 2024

Status update: We're at the end of the 2-day extension for this GEP to make it into this release, so unfortunately I think this is going to have to switch to targeting v1.3. Fortunately, this new shorter release cycle means that's actually not that far away.

Ultimately, I think you're right. For this to make sense, we'd need some structural changes to Gateway, particularly making Listeners optional. That's a huge change to a GA API (even if it would initially be limited to experimental channel, breaking changes to something labeled as "v1"can be confusing). Combined with the huge scope of the PR, and most of the feedback just coming from Nick and I, I think this particular GEP would benefit from some more soak time for additional reviewers + to smooth out any areas we're uncertain. Unfortunately, I think it's just too big to try to squeeze in today.

With that said, I would like to see this GEP make it in early in the next release cycle (similar to how named Route rules just barely missed the previous cycle but was merged at the beginning of this one).

I think it would be useful to do the following between now and then:

  1. Continue to iterate on this GEP, discussing in meetings as necessary (this can include merging this as provisional with some unresolved sections and iterating in smaller PRs)
  2. Build out a clearer vision for how this can be expanded to cover more use cases in the future (particularly the ones we discussed in the hierarchy doc) - that will help make a stronger case for why this is important
  3. Present these ideas at the SIG-Network review session for v1.2 so we can get early feedback on this overall idea and adjust the proposal accordingly
  4. Warn all controller authors that it's possible that Listeners may become optional in a future release of Gateway API and they should make sure they can handle that case

Thanks again for all the work you've been putting in to push this forward @dprotaso! I really do appreciate it.

type ListenerSetSpec struct {
// ParentRefs references the Gateway that the listeners are attached to.
//
// +kubebuilder:validation:MaxItems=2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why limit ParentRefs to 2?

Copy link
Contributor

@mikemorris mikemorris Sep 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is to allow "moving" a set of listeners from one Gateway to another without downtime, but limiting the ability to attach the same set of listeners to many different Gateways - if we find sufficient demand for that pattern loosening this constraint in the future could be possible, but it feels like a pattern that likely should be avoided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to Mike's answer. @dprotaso do you mind capturing this rationale somewhere in the GEP?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circling back on this I want to check the assumption - we sorta added multiple parents for 'future' proofing without a use case. See: #3213 (comment)

@mikemorris mentioned the migration as one use case but I don't particularly see that being any simpler than creating a copy of the ListenerSet.

I'd be willing to hear other use cases but so far I'm not convinced a list is the right thing here - especially if we're limiting it to 2 parents. It makes the status more complicated when we know the 80% of uses will be a single parent Gateway.

Again if we didn't have multiple parents a user could simply create two ListenerSets.

@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Sep 18, 2024
@shaneutt shaneutt added this to the v1.3.0 milestone Sep 18, 2024
@dprotaso dprotaso changed the title GEP-1713: Standard Mechanism to Gateway Merge Listeners (rev 2) GEP-1713: Standard Mechanism to Merge Gateway Listeners (rev 2) Nov 12, 2024
@dprotaso dprotaso changed the title GEP-1713: Standard Mechanism to Merge Gateway Listeners (rev 2) GEP-1713: ListenerSets - Standard Mechanism to Merge Gateway Listeners (rev 2) Nov 25, 2024
@dprotaso
Copy link
Contributor Author

dprotaso commented Dec 9, 2024

/retest

Copy link
Member

@robscott robscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work on this @dprotaso! Took a fresh pass and I think this largely makes sense.

geps/gep-1713/index.md Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Outdated Show resolved Hide resolved
type ListenerSetSpec struct {
// ParentRefs references the Gateway that the listeners are attached to.
//
// +kubebuilder:validation:MaxItems=2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to Mike's answer. @dprotaso do you mind capturing this rationale somewhere in the GEP?

geps/gep-1713/index.md Outdated Show resolved Hide resolved
Comment on lines +462 to +464
> 1. Should we have made it easier to leave port empty or do port ranges?
> 2. Should we support multiple hostnames?
> 3. Are there any validations that we wish we'd tightened up?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for capturing this!

+1 to past me on all points 🙃. I'd argue that we should probably make attempts at resolving both 1 and 2 here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For multiple hostnames, I can see the utility, but it risks making the rules about Listener -> Route hostname matching even more complicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional port would be nice - then let the implementation choose it. Though it's a rare use case for me.

geps/gep-1713/index.md Outdated Show resolved Hide resolved
geps/gep-1713/index.md Show resolved Hide resolved
protocol: HTTP
port: 80
---
apiVersion: gateway.networking.k8s.io/v1alpha1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like to get some resolution on #3497 before we release this API, as I'd prefer to switch this group to gateway.networking.x-k8s.io

}
// ListenerEntry embodies the concept of a logical endpoint where a Gateway accepts
// network connections.
type ListenerEntry struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's been some renewed interest in pushing L4 + ClusterIP Gateways forward, and one thing that could help with that is if Listeners could point directly to backends. I'd personally like to explore that in this GEP as a follow up to the similar idea in the doc. (No need to take action here until we get some more comments/feedback on this idea).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/gep PRs related to Gateway Enhancement Proposal(GEP) priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

GEP: Standard Mechanism to Merge Multiple Gateways
10 participants