ListenerSet APIs + Generated Clients #3588

dprotaso · 2025-02-03T22:15:53Z

Part of #1713

Merge #3589 beforehand (to make this diff easier)

Note: this required two different client sets because if you try to generate a client set containing the stable group (gateway.networking.k8s.io) and experimental (gateway.networking.k8s-x.io) client-gen uses gateway part from both api groups for naming in the client set and it creates a conflict

refactor codegen scripts to make it easier to generate two clients (to be removed)
add listenerset types
adjust GEP to match go type
run deepcopy-gen & register-gen
perform code generation

Release Note

Introduces ListenerSet API & Generate Clients (in the group gateway.networking.k8s-x.io)

dprotaso · 2025-02-03T22:16:26Z

I'm going to pull

refactor codegen scripts to make it easier to generate two clients

Into a separate PR

dprotaso · 2025-02-03T22:17:53Z

/hold for #3589

dprotaso · 2025-02-03T23:26:30Z

apisx/v1alpha1/listenerset_types.go

+	// +kubebuilder:validation:XValidation:message="Listener name must be unique within the Gateway",rule="self.all(l1, self.exists_one(l2, l1.name == l2.name))"
+	//
+	// # TODO: Figure out how to allow empty port here
+	// #+kubebuilder:validation:XValidation:message="Combination of port, protocol and hostname must be unique for each listener",rule="self.all(l1, self.exists_one(l2, l1.port == l2.port && l1.protocol == l2.protocol && (has(l1.hostname) && has(l2.hostname) ? l1.hostname == l2.hostname : !has(l1.hostname) && !has(l2.hostname))))"


note this TODO

Ok this should become

self.all(l1, !has(l1.port) || // Port is missing no conflict can ever occur self.exists_one(l2, has(l2.port) && // If L2 has no port than there can't be a conflict // Regular check since both listeners have a port l1.port == l2.port && l1.protocol == l2.protocol && (has(l1.hostname) && has(l2.hostname) ? l1.hostname == l2.hostname : !has(l1.hostname) && !has(l2.hostname)) ) )

dprotaso · 2025-02-04T00:21:06Z

/hold need to add the Gateway changes

dprotaso · 2025-02-04T00:47:08Z

/hold cancel

dprotaso · 2025-02-06T15:55:10Z

Note: this required two different client sets because if you try to generate a client set containing the stable group (gateway.networking.k8s.io) and experimental (gateway.networking.k8s-x.io) client-gen uses gateway part from both api groups for naming in the client set and it creates a conflict

Ok - I figured out there's a marker

+groupGoName=SomeUniqueShortName

So we could have a single clientset that has stable and experimental apis - let me know what people prefer

shaneutt · 2025-02-07T15:41:05Z

geps/gep-1713/index.md

@@ -74,7 +74,7 @@ type ListenerNamespaces struct {
 	// * None: Only listeners defined in the Gateway's spec are allowed
 	//
 	// +optional
-	// +kubebuilder:default=Same


This PR seems to jump us all the way from provisional to experimental without hitting implementable. If we have consensus then there might not actually be anything wrong with that, but bare minimum we should update the GEP status accordingly here.

Want me to split out the APIs in to a separate PR?

No that's OK! I just think we should flag the status change here and then please feel free to consider this comment resolved.

I just realized another opportunity to update the GEP, but I'll create a separate comment.

k8s-ci-robot · 2025-02-07T15:41:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dprotaso
Once this PR has been reviewed and has the lgtm label, please ask for approval from shaneutt. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

shaneutt · 2025-02-07T17:11:56Z

apis/v1/gateway_types.go

+type AllowedListeners struct {
+	// Namespaces defines which namespaces ListenerSets can be attached to this Gateway.
+	// While this feature is experimental, the default value is to allow no ListenerSets.
+	//
+	// +optional
+	// +kubebuilder:default={from: None}
+	Namespaces *ListenerNamespaces `json:"namespaces,omitempty"`
+}


I love this part of the API, because I think it captures a very important use case for cluster admins: when they want a Gateway to operate more like a classic ingress-controller, in that there's a single LB for routes across many namespaces.

What occurs to me is that we should update the GEP, as it currently states cross-namespace as a "later goal" but it seems like we're already going for it, and I'm very much in favor of this being an immediate goal. Anything I'm missing? (if not, please just feel free to update the GEP to reflect the goal and then consider my comment resolved at your discretion)

Eventually I personally would like to take this a step further: I would like our documentation to be even more clear about a strong intention for this kind of use case as I anticipate it being a very common ask over time.

+1. I think that cross-namespace is very important for this GEP and I'd love to see it in the first iteration of the API.

shaneutt

I think overall we're heading in a good direction here, but I think we need to reiterate what feels like a more obvious question as I read more: "What is a Gateway after this, really?"

Personally I'll have to think on this quite a bit and come back around to it 🤔

… in kubernetes-sigs#3588

dprotaso · 2025-02-14T20:56:38Z

I pushed a change to combine everything into a single clientset. It requires me to have a single applyconfiguration package that contains apis and apisx. So folks will have to change their package names if they're using the types there directly. (We could add type aliases)

It should be easy to revert the single client changes so let me know what you prefer

robscott

Thanks @dprotaso!

robscott · 2025-02-15T01:25:01Z

apisx/doc.go

+limitations under the License.
+*/
+
+package apix


Suggested change

package apix

package apisx

robscott · 2025-02-15T01:25:18Z

apisx/v1alpha1/doc.go

+//
+// +k8s:openapi-gen=true
+// +kubebuilder:object:generate=true
+// +groupName=gateway.networking.k8s-x.io


Suggested change

// +groupName=gateway.networking.k8s-x.io

// +groupName=gateway.networking.x-k8s.io

robscott · 2025-02-15T01:26:58Z

apisx/v1alpha1/listenerset_types.go

+	//
+	// 1. "parent" Gateway
+	// 2. ListenerSet ordered by creation time (oldest first)
+	// 3. ListenerSet ordered alphabetically by “{namespace}/{name}”.


What do we do if there are conflicting Listeners within a ListenerSet?

robscott · 2025-02-15T01:28:44Z

apisx/v1alpha1/listenerset_types.go

+
+type ListenerEntry struct {
+	// Name is the name of the Listener. This name MUST be unique within a
+	// Gateway.


🤔 how is this supposed to work in the wild world of ListenerSets? If I have listener foo and attach it to Gateway bar, can a route target the foo listener in the bar Gateway and expect to be attached to the listener in this ListenerSet? What about name conflicts?

robscott · 2025-02-15T01:30:25Z

apisx/v1alpha1/listenerset_types.go

+	// Listeners in a `Gateway` and their attached `ListenerSets` are concatenated
+	// as a list when programming the underlying infrastructure.
+	//
+	// Listeners should be merged using the following precedence:


When there are conflicts, how are implementations supposed to handle them? IE what if multiple Listeners across different ListenerSets claim the same hostname? Do we reject the entire ListenerSet? Just the Listener? How do we communicate this to users?

robscott · 2025-02-15T01:35:27Z

apisx/v1alpha1/listenerset_types.go

+	AllowedRoutes *AllowedRoutes `json:"allowedRoutes,omitempty"`
+}
+
+type ListenerSetStatus struct {


The structure of this status really locks us into sticking within a single ParentRef. I think this is OK, just want to call out that the lack of namespacing by parent here will make it very difficult to every support multiple ParentRefs. With that said, trying to namespace all of these conditions by parent would be hard to scale, so this feels like a strong case in favor of a single parent.

robscott · 2025-02-15T01:36:27Z

apisx/v1alpha1/listenerset_types.go

+	// Controllers may raise this condition with other reasons,
+	// but should prefer to use the reasons listed above to improve
+	// interoperability.
+	ListenerSetConditionProgrammed ListenerSetConditionType = "Programmed"


In what cases would we expect this status to be different than the parent status? Could a Gateway be "Programmed" but not a ListenerSet? What about the other way around?

I would expect that a ListenerSet cannot be programmed in a non-programmed Gateway, while the other way around would be possible.

mlavacca · 2025-02-18T14:04:41Z

apis/v1/gateway_types.go

+type AllowedListeners struct {
+	// Namespaces defines which namespaces ListenerSets can be attached to this Gateway.
+	// While this feature is experimental, the default value is to allow no ListenerSets.
+	//
+	// +optional
+	// +kubebuilder:default={from: None}
+	Namespaces *ListenerNamespaces `json:"namespaces,omitempty"`
+}


+1. I think that cross-namespace is very important for this GEP and I'd love to see it in the first iteration of the API.

mlavacca · 2025-02-18T14:08:17Z

apisx/v1alpha1/shared_types.go

+
+// ParentGatewayReference identifies an API object including its namespace,
+// defaulting to Gateway.
+type ParentGatewayReference struct {


Since this is a generic reference, I would consider naming it differently than ParentGatewayReference, as the reference can of any group/kind

mlavacca · 2025-02-18T14:12:21Z

apisx/v1alpha1/listenerset_types.go

+// ListenerSetSpec defines the desired state of a ListenerSet.
+type ListenerSetSpec struct {
+	// ParentRef references the Gateway that the listeners are attached to.
+	ParentRef ParentGatewayReference `json:"parentRef,omitempty"`


I think we should consider to make this field Required. Is there any specific reason to accept a ListenerSet with an empty ParentRef?

mlavacca · 2025-02-18T14:17:07Z

apisx/v1alpha1/listenerset_types.go

+type ListenerSetStatus struct {
+	// Conditions describe the current conditions of the ListenerSet.
+	//
+	// Implementations should prefer to express ListenerSet conditions


Why should here? I think we may want to have a MUST here, as it'll likely be part of the conditions checked by conformance tests.

mlavacca · 2025-02-18T14:19:33Z

apisx/v1alpha1/listenerset_types.go

+	// Controllers may raise this condition with other reasons,
+	// but should prefer to use the reasons listed above to improve
+	// interoperability.
+	ListenerSetConditionProgrammed ListenerSetConditionType = "Programmed"


I would expect that a ListenerSet cannot be programmed in a non-programmed Gateway, while the other way around would be possible.

k8s-ci-robot · 2025-02-21T19:55:14Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Feb 3, 2025

k8s-ci-robot requested review from kflynn and mlavacca February 3, 2025 22:16

dprotaso mentioned this pull request Feb 3, 2025

refactor codegen scripts to make it easier to generate two clients #3589

Merged

dprotaso force-pushed the listenerset branch from d1c440a to 389a354 Compare February 3, 2025 22:46

dprotaso changed the title ~~ListenerSet APIs + Generated Clients~~ [wip] ListenerSet APIs + Generated Clients Feb 3, 2025

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2025

dprotaso changed the title ~~[wip] ListenerSet APIs + Generated Clients~~ ListenerSet APIs + Generated Clients Feb 3, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 3, 2025

dprotaso commented Feb 3, 2025

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 4, 2025

shaneutt self-assigned this Feb 6, 2025

shaneutt self-requested a review February 6, 2025 15:52

shaneutt requested review from robscott and youngnick February 6, 2025 15:57

shaneutt requested changes Feb 7, 2025

View reviewed changes

shaneutt reviewed Feb 7, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2025

ericdbishop added a commit to ericdbishop/gateway-api that referenced this pull request Feb 13, 2025

Attempting to match the experimental API structure that dprotaso made…

e1dcdad

… in kubernetes-sigs#3588

ericdbishop added a commit to ericdbishop/gateway-api that referenced this pull request Feb 13, 2025

Attempting to match the experimental API structure that dprotaso made…

6f04b8b

… in kubernetes-sigs#3588

ericdbishop mentioned this pull request Feb 13, 2025

GEP 3388 Retry Budget API Implementation #3607

Open

dprotaso added 8 commits February 14, 2025 15:24

add listenerset types

d59ff79

adjust GEP to match go type

6c2213c

run deepcopy-gen & register-gen

09080a2

perform code generation

8ae33c6

Add ListenerSetList type

5168808

remove TODO for cel validation that allows an optional port

45f1491

add Gateway allowedListeners property

c7694e1

Add 'None' as an allowable option for FromNamespaces

547053f

dprotaso force-pushed the listenerset branch from 9385dd0 to 547053f Compare February 14, 2025 20:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2025

combine into a single client

e320cfe

robscott reviewed Feb 15, 2025

View reviewed changes

mlavacca reviewed Feb 18, 2025

View reviewed changes

DuncanDoyle mentioned this pull request Feb 19, 2025

Add GEP-1713: ListenerSets - Standard Mechanism to Merge Multiple Gateways support kgateway-dev/kgateway#10662

Open

5 tasks

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ListenerSet APIs + Generated Clients #3588

ListenerSet APIs + Generated Clients #3588

dprotaso commented Feb 3, 2025 •

edited

Loading

dprotaso commented Feb 3, 2025

dprotaso commented Feb 3, 2025

dprotaso Feb 3, 2025

dprotaso Feb 4, 2025

dprotaso commented Feb 4, 2025

dprotaso commented Feb 4, 2025

dprotaso commented Feb 6, 2025

shaneutt Feb 7, 2025

dprotaso Feb 7, 2025

shaneutt Feb 7, 2025

k8s-ci-robot commented Feb 7, 2025

shaneutt Feb 7, 2025 •

edited

Loading

mlavacca Feb 18, 2025

shaneutt left a comment

dprotaso commented Feb 14, 2025 •

edited

Loading

robscott left a comment

robscott Feb 15, 2025

robscott Feb 15, 2025

robscott Feb 15, 2025

robscott Feb 15, 2025

robscott Feb 15, 2025

robscott Feb 15, 2025

robscott Feb 15, 2025

mlavacca Feb 18, 2025

mlavacca Feb 18, 2025

mlavacca Feb 18, 2025

mlavacca Feb 18, 2025

mlavacca Feb 18, 2025

mlavacca Feb 18, 2025

k8s-ci-robot commented Feb 21, 2025

	// +groupName=gateway.networking.k8s-x.io
	// +groupName=gateway.networking.x-k8s.io

ListenerSet APIs + Generated Clients #3588

Are you sure you want to change the base?

ListenerSet APIs + Generated Clients #3588

Conversation

dprotaso commented Feb 3, 2025 • edited Loading

Release Note

dprotaso commented Feb 3, 2025

dprotaso commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Feb 4, 2025

dprotaso commented Feb 4, 2025

dprotaso commented Feb 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 7, 2025

shaneutt Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaneutt left a comment

Choose a reason for hiding this comment

dprotaso commented Feb 14, 2025 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 21, 2025

dprotaso commented Feb 3, 2025 •

edited

Loading

shaneutt Feb 7, 2025 •

edited

Loading

dprotaso commented Feb 14, 2025 •

edited

Loading