GatewaySpec Listener CEL block empty Hostname if similar Listeners also present

[rainest]

What happened:

0.8.0 does not allow a Gateway with two same Port, same Protocol Listeners where one Listener has Hostname set and the other does not. This violates a CEL rule that ensures Listeners have a unique Port/Protocol/Hostname combination.

What you expected to happen:

The Gateway passes validation. Per the above spec comment:

one Listener within a group may omit Hostname, in which case this Listener matches when no other Listener matches.

How to reproduce it (as minimally and precisely as possible):

With 0.8.0 installed:

$ echo "apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway                                                             
metadata:    
  name: prod-web
spec:           
  gatewayClassName: acme-lb
  listeners:               
  - protocol: HTTP
    port: 80      
    name: http
  - protocol: HTTP
    port: 80      
    name: httphostname
    hostname: http.example" | kubectl create -f -
The Gateway "prod-web" is invalid: spec.listeners: Invalid value: "array": Combination of port, protocol and hostname must be unique for each listener

Anything else we need to know?:

It looks like

(has(l1.hostname) && has(l2.hostname) ? l1.hostname == l2.hostname : true

requires a set value to compare if Hostname is equal.

Setting Hostname to "" or "*" does not work around this, as these values violate Hostname's '^(\*\.)?[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' validation regex.

[robscott]

Edit: I was wrong here, the spec clearly states that an empty hostname can be combined with a specified hostname. Recommend skipping this and my next comment.

In my opinion this example should be invalid. The spec currently states that the combination of Hostname, Protocol, and Port must be unique:

gateway-api/apis/v1beta1/gateway_types.go

Lines 72 to 73 in c21d0e4

	// Each listener in a Gateway must have a unique combination of Hostname,
	// Port, and Protocol.

Unfortunately the webhook validation did allow this to happen:

gateway-api/apis/v1beta1/validation/gateway.go

Lines 145 to 165 in ab03a59

	// validateHostnameProtocolPort validates that the combination of port, protocol, and hostname are
	// unique for each listener.
	func validateHostnameProtocolPort(listeners []gatewayv1b1.Listener, path *field.Path) field.ErrorList {
	var errs field.ErrorList
	hostnameProtocolPortSets := sets.Set[string]{}
	for i, listener := range listeners {
	hostname := new(gatewayv1b1.Hostname)
	if listener.Hostname != nil {
	hostname = listener.Hostname
	}
	protocol := listener.Protocol
	port := listener.Port
	hostnameProtocolPort := fmt.Sprintf("%s:%s:%d", *hostname, protocol, port)
	if hostnameProtocolPortSets.Has(hostnameProtocolPort) {
	errs = append(errs, field.Duplicate(path.Index(i), "combination of port, protocol, and hostname must be unique for each listener"))
	} else {
	hostnameProtocolPortSets.Insert(hostnameProtocolPort)
	}
	}
	return errs
	}

For completeness, here's the corresponding CEL validation:

gateway-api/apis/v1beta1/gateway_types.go

Line 131 in c21d0e4

// +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 : true)))"

To me, this is/was a bug in validation. I recognize that if a bug exists long enough, it becomes a feature. I'm not sure if we've already crossed that line here. Would be interested in what others think.

/cc @shaneutt @youngnick

[robscott]

This feels conceptually similar to ParentRefs where we're also saying that for ParentRefs to be distinct, either SectionName or Port needs to be set to non-empty + distinct values, x-ref #2350. To avoid confusion here, I think we should do what we can to make sure the definitions of distinct here are as consistent as possible.

[rainest]

Considering this invalid would severely limit the ability to use a catch-all wildcard hostname, since you couldn't use it in combination with any other Listeners on the same Port+Protocol. I don't think we'd want that, and the prose indicates that we do want the 0.7 behavior--it's "one Listener within a group [port and protocol combination] may omit Hostname" rather than "a Listener may omit Hostname and match any Hostname if it is the only Listener in the group)".

We could require an explicit non-nil value for the catch-all (either "*" or ""), but it'd be a breaking change (the webhook used the same regex validation previously, and the spec said to omit it).

IMO empty is fine so long as we can check it and confirm that there are no two Listeners both using it on the same Protocol+Port. Rather, for Hostname alone, the list ["foo.example", "*.example", nil] has no duplicates, but ["foo.example", "*.example", nil, nil] does.

Testing that in CEL may be another story, but I assume you can somehow work in a failure condition for !has(l1.hostname) && !has(l2.hostname) (from brief testing, you cannot directly compare l1.hostname == l2.hostname if one is nil).

[frankbu]

This seems like a valid configuration to me. The missing hostname is unique, it essentially behaves like hostname: *, right? So http.example will match the second listener, everything else will go to the first. It seems no different than if the first listener had hostname: *.example, which would be considered a valid unique hostname combination.

[robscott]

Yep, I think you're all right. I missed @rainest's reference to the spec in the original issue that very much settles this:

gateway-api/apis/v1beta1/gateway_types.go

Lines 105 to 107 in d031c16

	// 3. As a special case, one Listener within a group may omit Hostname,
	// in which case this Listener matches when no other Listener
	// matches.

Sorry for the noise here, this is indeed a bug in the CEL validation and we should fix it ASAP.

/triage accepted
/help

[k8s-ci-robot]

@robscott:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Yep, I think you're all right. I missed @rainest's reference to the spec in the original issue that very much settles this:

gateway-api/apis/v1beta1/gateway_types.go

Lines 105 to 107 in d031c16

	// 3. As a special case, one Listener within a group may omit Hostname,
	// in which case this Listener matches when no other Listener
	// matches.

Sorry for the noise here, this is indeed a bug in the CEL validation and we should fix it ASAP.

/triage accepted
/help

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/test-infra repository.

[frankbu]

I'm no CEL expert, but it seems that the rule just needs to remove the has() part, so it will just check for duplicate hostnames, nil or not.

 // +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 && l1.hostname == l2.hostname))"