gateway-api icon indicating copy to clipboard operation
gateway-api copied to clipboard

Webhook: Validate unique port, protocol, and hostname per listener

Open robscott opened this issue 3 years ago • 12 comments

What would you like to be added: The validating webhook should ensure that port, protocol, and hostname are unique among Gateway listeners. For example, 2 listeners for foo.com would be valid if they were on different ports or protocols, but not if port and protocol were also identical.

Why this is needed: To provide a better user experience.

robscott avatar Sep 01 '21 23:09 robscott

/assign

bishtsaurabh5 avatar Sep 05 '21 18:09 bishtsaurabh5

@robscott I am thinking of generating a hash based on the (Hostname + Port +Protocol) and then store them in a map where key is hash of the listener array and value is the index and if the key is already present then log all the indexes in the errors . Do you have views ?

bishtsaurabh5 avatar Sep 07 '21 15:09 bishtsaurabh5

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Dec 06 '21 15:12 k8s-triage-robot

/lifecycle frozen

hbagdi avatar Dec 06 '21 18:12 hbagdi

@robscott @bishtsaurabh5 May I know if this feature is still required based on the current design? If so, I'd like to continue to implement this.

gyohuangxin avatar Sep 15 '22 07:09 gyohuangxin

Hey @gyohuangxin, thanks for checking! This is still unimplemented and would be a nice addition to https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/validation/gateway.go. Let me know if you're still interested in implementing this.

robscott avatar Sep 26 '22 16:09 robscott

Hey i would like to contribute if no one is working on it.

akankshakumari393 avatar Sep 26 '22 17:09 akankshakumari393

@robscott Thanks! I'd like to contribute on this. @akankshakumari393 Sorry! I'm working on this, but any reviews and comments are welcome :)

gyohuangxin avatar Sep 27 '22 01:09 gyohuangxin

/assign

gyohuangxin avatar Sep 27 '22 01:09 gyohuangxin

I meet an issue when implementing this feature. My method is to add a function in https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1beta1/validation/gateway.go, which will create a Kubernetes client via "sigs.k8s.io/controller-runtime/pkg/client" to get gateways resource list at cluster scope, here is the example code:

func listGateway() (*v1beta1.GatewayList, error) {

	cfg, err := config.GetConfig()
	if err != nil {
		klog.Errorf("Error loading Kubernetes config: %v", err)
	}
	cl, err := client.New(cfg, client.Options{})
	if err != nil {
		klog.Errorf("Failed to create new Kubernetes client: %v", err)
	}

	v1beta1.AddToScheme(cl.Scheme())

	gatewayList := &v1beta1.GatewayList{}
	err = cl.List(context.Background(), gatewayList, &client.ListOptions{})
	if err != nil {
		return nil, err
	}

	return gatewayList, nil
}

func validateUniquePort(listeners []gatewayv1b1.Listener, path *field.Path) field.ErrorList {
	var errs field.ErrorList

	gatewayList, err := listGateway()
	if err != nil {
		klog.Errorf("Failed to list gateways: %v", err)
	}
	klog.Infof("Gateways resources: %v", gatewayList)

	return errs
}

However, it doesn't work because the user "system:serviceaccount:gateway-system:default" used by webhook server is forbidden to list "gateway" resource at cluster scope.

I1012 14:51:51.693874       1 request.go:1073] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"gateways.gateway.networking.k8s.io is forbidden: User \"system:serviceaccount:gateway-system:default\" cannot list resource \"gateways\" in API group \"gateway.networking.k8s.io\" at the cluster scope","reason":"Forbidden","details":{"group":"gateway.networking.k8s.io","kind":"gateways"},"code":403}
E1012 15:44:42.933257       1 gateway.go:132] Failed to list gateways: gateways.gateway.networking.k8s.io is forbidden: User "system:serviceaccount:gateway-system:default" cannot list resource "gateways" in API group "gateway.networking.k8s.io" at the cluster scope
I1012 15:44:42.933278       1 gateway.go:134] Listing gateways resources: <nil>

@robscott Do you have any ideas?

gyohuangxin avatar Oct 12 '22 08:10 gyohuangxin

Hey @gyohuangxin, sorry for the confusion here! This was actually intended to be smaller in scope than that. Instead of ensuring that a Listener's port, protocol, and hostname are unique across all Gateways, we want to make sure that they are unique within each individual Gateway. A quick example:

What we're trying to prevent with this validation:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
  name: example-gateway
spec:
  gatewayClassName: example-gateway-class
  listeners:
  - name: http
    protocol: HTTP
    port: 80
  - name: other-http
    protocol: HTTP
    port: 80

What we don't care about:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
  name: example-gateway
spec:
  gatewayClassName: example-gateway-class
  listeners:
  - name: http
    protocol: HTTP
    port: 80
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
  name: example-gateway2
spec:
  gatewayClassName: example-gateway-class
  listeners:
  - name: http
    protocol: HTTP
    port: 80

robscott avatar Oct 13 '22 03:10 robscott

@robscott Understand, thanks for clarifying!

gyohuangxin avatar Oct 13 '22 05:10 gyohuangxin

we want to make sure that they are unique within each individual Gateway. A quick example:

This seems fairly limited in my opinion. What about other properties in the listeners?

Looking at the discussion (https://github.com/kubernetes-sigs/gateway-api/discussions/1248) we could have many certificates (HTTP01) for services in a cluster. If there are multiple controllers updating the gateway listener resource that'll lead to conflicts and churn.

In theory a way to coordinate this could be by adding multiple listeners but with the same protocol/port and hostname but with a different name and tls configuration

ie.

apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
  name: example-gateway
spec:
  gatewayClassName: example-gateway-class
  listeners:
  - name: https
    protocol: HTTPS
    port: 443
    tls: 
      certificateRefs: [some-cert]
  - name: https-2
    protocol: HTTPS
    port: 443
    tls: 
      certificateRefs: [another-cert]

Alternatively maybe we should ensure merging of distinct Gateways resources is part of the core functionality.

dprotaso avatar Feb 07 '23 21:02 dprotaso

Actually I realize the listeners would have unique hostname's when setting tls - so my comment isn't applicable.

dprotaso avatar Feb 08 '23 19:02 dprotaso