search

l7mp / stunner-gateway-operator

STUNner Kubernetes Gateway Operator
Apache License 2.0
15 stars 6 forks source link

Bug: Gateway listener names are not used properly #19

Closed davidkornel closed 1 year ago

davidkornel commented 1 year ago

The gateway-operator does not use the configured names of the listeners in the Gateway resource. In case of applying the following manifest:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: udp-gateway
  namespace: stunner
spec:
  gatewayClassName: stunner-gatewayclass
  listeners:
    - name: udp-listener
      port: 3478
      protocol: UDP
    - name: another-udp-listener
      port: 3479
      protocol: UDP

The following error shows up in the gateway-operator's logs:

2023-03-02T16:51:31.076679312Z    ERROR    updater    cannot update service    {"operation": "unchanged", "error": "cannot upsert service \"stunner/udp-gateway\": Service \"udp-gateway\" is invalid: spec.ports[1].name: Duplicate value: \"udp-gateway-udp\""}
github.com/l7mp/stunner-gateway-operator/internal/updater.(*Updater).Start.func1
    /workspace/internal/updater/updater.go:63

It seems the Services spec.ports.name field is filled with the gateway's name + the port's protocol type, instead of the name configured in the Gateway's spec.listeners.name.

This does not cause any problems if only one listener is configured. As soon as two or more listeners are configured their names will bump into each other causing an error.

joaogbcravo commented 1 year ago

I'm having potentially related problems as well:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: my-gateway
  annotations:
      stunner.l7mp.io/enable-mixed-protocol-lb: "true"
      external-dns.alpha.kubernetes.io/hostname: somehostname.com
      cert-manager.io/cluster-issuer: cluster-issue
      service.beta.kubernetes.io/aws-load-balancer-cross-zone-load-balancing-enabled: "true"
      service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: ip
      service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
      service.beta.kubernetes.io/aws-load-balancer-type: external
      service.beta.kubernetes.io/aws-load-balancer-healthcheck-port: "443"
  spec:
    gatewayClassName: stunner-gateway
    listeners:
    - name: stunner-tls
      hostname: somehostname.com
      port: 443
      protocol: TLS
      tls:
        mode: Terminate
        certificateRefs:
          - name: turn-tls
    - name: stunner-udp
      port: 3478
      protocol: UDP
      tls:
        mode: Terminate
        certificateRefs:
          - name: turn-tls
 2023-09-13T12:21:17.37349359Z   ERROR   updater cannot update service   {"operation": "unchanged", "error": "cannot upsert service \"stunner/stunner-gateway\": Service \"stunner-gateway\" is invalid: spec.loadBalancerClass: Invalid value: \"null\": may not change once set"}
    github.com/l7mp/stunner-gateway-operator/internal/updater.(*Updater).ProcessUpdate
            /workspace/internal/updater/updater.go:113
    github.com/l7mp/stunner-gateway-operator/internal/updater.(*Updater).Start.func1
            /workspace/internal/updater/updater.go:63

Happy to open 4 different issues, if these are really issues. Maybe I'm doing something wrong.

rg0now commented 1 year ago

Huh, that's a lot of issues in one go. Can you please reorg this dump (or at least the ones that we can confirm) to separate issues and close this one?

Let see:

It seems the Services spec.ports.name field is filled with the gateway's name + the port's protocol type, instead of the name configured in the Gateway's spec.listeners.name.

This is confirmed. Here is the offending line: indeed, we should include the listener name (or use only the listener name, it is supposed to be unique), something like the bwlow:

...
Name:     fmt.Sprintf("%s-%s-%s", gw.GetName(), l.Name, strings.ToLower(serviceProto))
...

Do you feel like sending a PR? Don't forget to add a test pls.

When I set up 3 listeners (like UDP, TCP and TLS), one of them was completely ignored on the service. The service doesn't allocate three ports, although cmd/stunnerctl/stunnerctl nows about it).

Does this have something to do with mixed-protocol LB support? This is only a bug if mixed-protocol LB support is enabled. You should know better than me, the PR came from you proper...:-D

When I set up just TCP and TLS (with different ports), it doesn't seem to like it. It complains about the node port being already used.

This one seems particularly nasty: I've seen this issue a couple of times in the tests but I could never quite reproduce it. The problem is that every time we update the service we have to explicitly specify the nodePort otherwise Kubernetes will choose one randomly and we end up rewriting the nodeport every time we run the render cycle. So we have to make sure during service updates that if a service-port already contains a valid nodeport then we leave that unchanged, and only add a new nodeport when we add a new service-port. The only place we have access to both the existing Service and the one we are about to install is inside CreateOrUpdate here: the DeepCopy will need to be made somewhat smarter.

With the following Gateway configuration, I'm constantly getting the error below from the operator logs: spec.loadBalancerClass: Invalid value: \"null\": may not change once set"}

Hmmm, we don't even touch the loadBalancerClass. It seems that at a certain point it gets set to something (by whom?) and then we try to nullify it (because we never set it, so I guess nil is the default), which leads to this issue. I guess the solution is as above: special-case this in the service updater to make sure it never changes once set. A PR maybe?...:-)

Also as you can see in the above configuration I'm adding a useless TLS config on the UDP listener. Without it, it also complains, but not sure if that is actually your problem.

UDP + TLS = DTLS. Still, I don't get this, can you please elaborate?