-
Notifications
You must be signed in to change notification settings - Fork 582
Improve some ListenetSet gep descriptions #3978
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -59,7 +59,7 @@ type GatewaySpec struct { | |||||||||
} | ||||||||||
|
||||||||||
type AllowedListeners struct { | ||||||||||
// +kubebuilder:default={from:Same} | ||||||||||
// +kubebuilder:default={from: None} | ||||||||||
Namespaces *ListenerNamespaces `json:"namespaces,omitempty"` | ||||||||||
} | ||||||||||
|
||||||||||
|
@@ -69,7 +69,7 @@ type ListenerNamespaces struct { | |||||||||
// values are: | ||||||||||
// | ||||||||||
// * Same: Only ListenerSets in the same namespace may be attached to this Gateway. | ||||||||||
// * Selector: ListenerSets in namespaces selected by the selector may be attached to this Gateway.:w | ||||||||||
// * Selector: ListenerSets in namespaces selected by the selector may be attached to this Gateway. | ||||||||||
// * All: ListenerSets in all namespaces may be attached to this Gateway. | ||||||||||
// * None: Only listeners defined in the Gateway's spec are allowed | ||||||||||
// | ||||||||||
|
@@ -115,6 +115,16 @@ type ListenerSetSpec struct { | |||||||||
// 2. ListenerSet ordered by creation time (oldest first) | ||||||||||
// 3. ListenerSet ordered alphabetically by “{namespace}/{name}”. | ||||||||||
// | ||||||||||
// Regarding Conflict Management, Listeners in a ListenerSet follow the same | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add this new clause to the API types godoc? Also we might want to call out sectionName behaves a bit differently where sibling ListenerSets can have the same sectionName. |
||||||||||
// rules of Listeners on a Gateway resource. | ||||||||||
// | ||||||||||
// This validation should happen within all of the ListenerSets attached to a | ||||||||||
// Gateway, and the precedence of "parent Gateway" -> "oldest first" -> | ||||||||||
// "alphabetically ordered" should be respected. | ||||||||||
// | ||||||||||
// ListenerSets containing conflicting Listeners MUST set the Conflicted | ||||||||||
// Condition to true and clearly indicate which Listeners are conflicted. | ||||||||||
// | ||||||||||
// +listType=map | ||||||||||
// +listMapKey=name | ||||||||||
// +kubebuilder:validation:MinItems=1 | ||||||||||
|
@@ -332,13 +342,17 @@ type ParentGatewayReference struct { | |||||||||
|
||||||||||
The following example shows a `Gateway` with an HTTP listener and two child HTTPS `ListenerSets` with unique hostnames and certificates. | ||||||||||
|
||||||||||
Only `ListenerSets` from the same namespace of the `Gateway` will be accepted: | ||||||||||
Comment on lines
343
to
+345
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.k8s.io/v1 | ||||||||||
kind: Gateway | ||||||||||
metadata: | ||||||||||
name: parent-gateway | ||||||||||
spec: | ||||||||||
gatewayClassName: example | ||||||||||
allowedListeners: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. per the api spec, not having an allowedListener means "None" which would make this invalid, so I am fixing the example for at least same namespace |
||||||||||
namespaces: | ||||||||||
from: Same | ||||||||||
listeners: | ||||||||||
- name: foo | ||||||||||
hostname: foo.com | ||||||||||
|
@@ -417,7 +431,8 @@ metadata: | |||||||||
name: parent-gateway | ||||||||||
spec: | ||||||||||
allowedListeners: | ||||||||||
- from: Same | ||||||||||
namespaces: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. allowedListeners is not an array, so changing here to reflect the current implemented API |
||||||||||
from: Same | ||||||||||
``` | ||||||||||
|
||||||||||
### Route Attachment | ||||||||||
|
@@ -438,35 +453,38 @@ spec: | |||||||||
sectionName: second | ||||||||||
``` | ||||||||||
|
||||||||||
For instance, the following `HTTPRoute` attempts to attach to a listener defined in the parent `Gateway` using the sectionName `foo`. This is not valid and the route's status `Accepted` condition should be set to `False` | ||||||||||
|
||||||||||
To attach to listeners in both a `Gateway` and `ListenerSet` the route MUST have two `parentRefs`: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have changed the order of examples here to make it clear what means using two parentRefs, before the next example which is an invalid example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reading this now it is a duplicate of I saw we replace the above yaml apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
name: httproute-example
spec:
parentRefs:
- name: second-workload-listeners
kind: ListenerSet
sectionName: second with the yaml containing two parent refs |
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.k8s.io/v1 | ||||||||||
kind: HTTPRoute | ||||||||||
metadata: | ||||||||||
name: httproute-example | ||||||||||
spec: | ||||||||||
parentRefs: | ||||||||||
- name: some-workload-listeners | ||||||||||
- name: second-workload-listeners | ||||||||||
kind: ListenerSet | ||||||||||
sectionName: second | ||||||||||
- name: parent-gateway | ||||||||||
kind: Gateway | ||||||||||
sectionName: foo | ||||||||||
``` | ||||||||||
|
||||||||||
To attach to listeners in both a `Gateway` and `ListenerSet` the route MUST have two `parentRefs`: | ||||||||||
For instance, the following `HTTPRoute` attempts to attach to a listener defined in the parent `Gateway` using the sectionName `foo`. This is not valid and the route's status `Accepted` condition should be set to `False` | ||||||||||
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.k8s.io/v1 | ||||||||||
kind: HTTPRoute | ||||||||||
metadata: | ||||||||||
name: httproute-example | ||||||||||
spec: | ||||||||||
parentRefs: | ||||||||||
- name: second-workload-listeners | ||||||||||
kind: ListenerSet | ||||||||||
sectionName: second | ||||||||||
- name: parent-gateway | ||||||||||
- name: some-workload-listeners | ||||||||||
Comment on lines
-442
to
+481
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess no real change here, just order, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, I wrote on my self review why I did the change. The example of the attachment of Gateway AND ListenerSet is more clear on this order, then we can go to the invalid examples :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To make this invalid the |
||||||||||
kind: Gateway | ||||||||||
sectionName: foo | ||||||||||
``` | ||||||||||
>--- Ricardo - above is a bit confusing, maybe ask Dave some clarification | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. /cc @dprotaso There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dprotaso the example above is a bit confusing for me, probably because the API changed from when it was implemented. I was wondering what it means for an HTTPRoute to try to attach a listener defined on a Gateway, using a section name
So how can the situation above be invalid? Maybe we can get the full yaml definition, with Gateway, ListenerSet and HTTPRoute for clarification here? Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
|
||||||||||
|
||||||||||
|
||||||||||
#### Optional Section Name | ||||||||||
|
||||||||||
|
@@ -545,7 +563,195 @@ Listeners should be merged using the following precedence: | |||||||||
2. ListenerSet ordered by creation time (oldest first) | ||||||||||
3. ListenerSet ordered alphabetically by “{namespace}/{name}”. | ||||||||||
|
||||||||||
Conflicts are covered in the section 'ListenerConditions within a ListenerSet' | ||||||||||
Conflicts are covered in the section [Listener and ListenerSet conflicts](#listener-and-listenerset-conflicts) | ||||||||||
|
||||||||||
### Listener and ListenerSet conflicts | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should combine this section into |
||||||||||
|
||||||||||
ListenerSet conflicts should be managed similarly to [Gateway resource conflict](https://github.com/kubernetes-sigs/gateway-api/blob/372a5b06624cff12117f41dcd26c08cb1def22e7/apis/v1/gateway_types.go#L76) | ||||||||||
management. | ||||||||||
|
||||||||||
With ListenerSet this validation should happen within the same ListenerSet resource, | ||||||||||
but MUST be validated also within a Gateway scope and all of the attached Listeners/ListenerSets. | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this language needs to exclude sectionName when comparing it across sibling Listeners There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or refer to 'Optional Section Name' |
||||||||||
|
||||||||||
This means that the validation should happen now between distinct ListenerSets | ||||||||||
attached to the same Gateway, and in case of a conflict, the [Listener Precedence](#listener-precedence) | ||||||||||
should be respected, and the conflicting listener MUST have a `Conflicted` condition | ||||||||||
set to True and with an explicit reason on its message. | ||||||||||
|
||||||||||
Following are some examples of a conflict situation: | ||||||||||
|
||||||||||
#### Conflict between ListenerSet and parent Gateway | ||||||||||
|
||||||||||
Given the following resource definitions: | ||||||||||
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.k8s.io/v1 | ||||||||||
kind: Gateway | ||||||||||
metadata: | ||||||||||
name: parent-gateway | ||||||||||
namespace: infra | ||||||||||
spec: | ||||||||||
allowedListeners: | ||||||||||
namespaces: | ||||||||||
from: All | ||||||||||
listeners: | ||||||||||
- name: foo | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
tls: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||||||
mode: Terminate | ||||||||||
certificateRefs: | ||||||||||
- kind: Secret | ||||||||||
group: "" | ||||||||||
name: default-cert | ||||||||||
--- | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
name: user-listenerset | ||||||||||
namespace: user01 | ||||||||||
spec: | ||||||||||
parentRef: | ||||||||||
name: parent-gateway | ||||||||||
kind: Gateway | ||||||||||
group: gateway.networking.k8s.io | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
tls: | ||||||||||
mode: Terminate | ||||||||||
certificateRefs: | ||||||||||
- kind: Secret | ||||||||||
group: "" | ||||||||||
name: app-cert | ||||||||||
``` | ||||||||||
|
||||||||||
The ListenerSet `user-listenerset` should be marked as Conflicted, as the `parent-gateway` | ||||||||||
have a listener definition called `foo` that conflicts with the ListenetSet definition | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Can we also update this (the paragraph not the condition message) to highlight that it is a conflict because the host name is the same but they use different termination TLS certificates |
||||||||||
called `myapp`, as the following: | ||||||||||
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
name: user-listenerset | ||||||||||
namespace: user01 | ||||||||||
.... | ||||||||||
status: | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
conditions: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using a tab here instead of spaces There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a bunch of these places - just view the GitHub rendering of the GEP and you'll see broken syntax formatting |
||||||||||
- message: ListenerSet has conflicts with Gateway 'infra/parent-gateway' | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would we want to highlight the listener name in the error message? |
||||||||||
reason: Conflicted | ||||||||||
status: "True" | ||||||||||
type: Conflicted | ||||||||||
``` | ||||||||||
|
||||||||||
#### Conflict between two ListenerSets | ||||||||||
|
||||||||||
The following example represents a conflict between two ListenerSets on distinct | ||||||||||
namespaces. The controller should avoid setting any Condition that exposes information | ||||||||||
from other users, but still provide meaningful information of why a ListenerSet | ||||||||||
was not accepted | ||||||||||
|
||||||||||
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
creationTimestamp: "2025-08-11T15:44:05Z" | ||||||||||
name: listenerset1 | ||||||||||
namespace: user01 | ||||||||||
spec: | ||||||||||
parentRef: | ||||||||||
name: parent-gateway | ||||||||||
kind: Gateway | ||||||||||
group: gateway.networking.k8s.io | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
tls: | ||||||||||
mode: Terminate | ||||||||||
certificateRefs: | ||||||||||
- kind: Secret | ||||||||||
group: "" | ||||||||||
name: app-cert | ||||||||||
--- | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
creationTimestamp: "2025-08-11T13:44:05Z" | ||||||||||
name: listenerset2 | ||||||||||
namespace: user02 | ||||||||||
spec: | ||||||||||
parentRef: | ||||||||||
name: parent-gateway | ||||||||||
kind: Gateway | ||||||||||
group: gateway.networking.k8s.io | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
tls: | ||||||||||
mode: Terminate | ||||||||||
certificateRefs: | ||||||||||
- kind: Secret | ||||||||||
group: "" | ||||||||||
name: other-app-cert | ||||||||||
``` | ||||||||||
|
||||||||||
In this case, there's a conflict as both users are setting the same hostname and | ||||||||||
port on distinct Listeners. In this case, because the ListenerSet `user02/listenerset2` | ||||||||||
is older, it will be accepted while `user01/listenerset1` should not be accepted, | ||||||||||
and receive a `Conflicted=True` condition. | ||||||||||
|
||||||||||
The status of ListenerSets can be defined as the following: | ||||||||||
|
||||||||||
```yaml | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
creationTimestamp: "2025-08-11T15:44:05Z" | ||||||||||
name: listenerset1 | ||||||||||
namespace: user01 | ||||||||||
status: | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
conditions: | ||||||||||
- message: ListenerSet has conflicts with other listeners attached to the same Gateway | ||||||||||
reason: Conflicted | ||||||||||
status: "True" | ||||||||||
type: Conflicted | ||||||||||
--- | ||||||||||
apiVersion: gateway.networking.x-k8s.io/v1alpha1 | ||||||||||
kind: ListenerSet | ||||||||||
metadata: | ||||||||||
creationTimestamp: "2025-08-11T13:44:05Z" | ||||||||||
name: listenerset2 | ||||||||||
namespace: user02 | ||||||||||
status: | ||||||||||
listeners: | ||||||||||
- name: myapp | ||||||||||
hostname: www.something.tld | ||||||||||
protocol: HTTPS | ||||||||||
port: 443 | ||||||||||
conditions: | ||||||||||
- reason: Accepted | ||||||||||
status: "True" | ||||||||||
type: Accepted | ||||||||||
``` | ||||||||||
|
||||||||||
### Gateway Conditions | ||||||||||
|
||||||||||
|
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 was made to reflect the current implementation
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 may be out of context, wdym here? are we moving away from the Same as default?
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 the API definition Same is not the default, None is:
gateway-api/apis/v1/gateway_types.go
Line 306 in fb75360