Feature proposal: Gateway Annotation to include all kind of Service.spec fields

[davidkornel]

Currently, we do not support many Service fields, thus limiting the possible configurations that might be needed in not well-documented/known use cases. The service that is generated by the operator based on Gateway resource has limited functionality without any extra fields. A Discord issue stated that service.spec.externalIPs field is needed in order to set up an external DNS for the service in order to have control of the publicly accessible address of the service. JSON formatted Gateway annotation would look like the following:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: complex-gateway
  namespace: stunner
  annotations:
    stunner.l7mp.io/extra-spec-fields: {\r\n  \"spec\": {\r\n    \"externalIPs\": [\r\n      \"198.51.100.32\",\r\n      \"1.2.3.4\"\r\n    ]\r\n  }\r\n}
spec:
  gatewayClassName: stunner-gatewayclass
  listeners:
    - name: udp-listener
      port: 3478
      protocol: UDP
      allowedRoutes:
        namespaces:
          from: All

I can think of three different approaches on how to set the annotation field:

#without special characters
{\"spec\":{\"externalIPs\":[\"198.51.100.32\",\"1.2.3.4\"]}}
#without spec explicit top-level spec key
{\"externalIPs\":[\"198.51.100.32\",\"1.2.3.4\"]}
#with the indenting special characters (probably not the best idea)
{\r\n  \"spec\": {\r\n    \"externalIPs\": [\r\n      \"198.51.100.32\",\r\n      \"1.2.3.4\"\r\n    ]\r\n  }\r\n}

What do you think? What would be a good annotation key instead of stunner.l7mp.io/extra-spec-fields?

[rg0now]

Thanks, @davidkornel, for picking this up. I'd definitely vote for the second option, "without spec, explicit top-level spec key", but I'd simplify the design specifically for supporting external-IPs. Oh, and always without control chars. So my pick would be the following:

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: complex-gateway
  namespace: stunner
  annotations:
    stunner.l7mp.io/externalIPs: "[\"198.51.100.32\",\"1.2.3.4\"]"
spec:
  gatewayClassName: stunner-gatewayclass
  listeners:
    - name: udp-listener
      port: 3478
      protocol: UDP

This will come with the usual ugliness of marshaling/unmarshaling JSON to/from annotation values.

I'm not sure we have to go very far with this though. We already provide too many annotations and discoverability of specific features is getting worse with every new annotation we add. What about simplifying this to a single external IP?

apiVersion: gateway.networking.k8s.io/v1alpha2
kind: Gateway
metadata:
  name: complex-gateway
  annotations:
    stunner.l7mp.io/externalIP: 198.51.100.32
...

[davidkornel]

Two things @rg0now We can simplify the design just to support external ips, it would definitely ease the implementation and get rid of many many corner cases. However as you mentioned we already support way too many annotations that's why I was thinking about handling all the other potential fields in one go. All in all I agree to simplify just to support the externalIPs field.

The other thing about the number of addresses. The first element in the spec externalIPs[xyz] list will be taken and put in the stunnerd.conf config file, right? Could/can we support multiple public addresses per stunner? If yes then we should let them users define multiple addresses in the annotation list, if not then we must limit to a single address in order to prevent confusion and bugs.

[rg0now]

The other thing about the number of addresses. The first element in the spec externalIPs[xyz] list will be taken and put in the stunnerd.conf config file, right? Could/can we support multiple public addresses per stunner? If yes then we should let them users define multiple addresses in the annotation list, if not then we must limit to a single address in order to prevent confusion and bugs.

This makes a lot of sense, and frankly, we should have started with a design where listener.public_address is a list in the stunnerd config file, not a single address. At this point, however, changing this will require an API break, because from the operator to the dataplane all the way to the auth-service we rely on the assumption that there is a single external IP.

In summary: yes, we should allow multiple public addresses. However, this will have to wait until after v1 happens and we bump the config file version to v1alpha2. So let's allow only a single external IP for now and think about extensions later, wdyt?

[davidkornel]

Indeed, but who would have thought this many cases will come in?! Agree, I will focus on publishing a single external IP instead of a list. Will update you in a PR.

[davidkornel]

@rg0now another idea came to my mind Since the gateway API has the gw.spec.addresses field there is no reason to not use that field to set any type of addresses. Instead of specifying the externalIPs in the annotation as a list of addresses or an individual address, just use the annotation as a boolean setter. If the annotation key is set to True then the gw.spec.addresses field will be used to set the svc.Spec.ExternalIPs field if it is set to false or unset the specified addresses will set the svc.spec.LoadBalancerIP. Kubernetes API allows both of them to be used at the same time but I don't think there is a valid reason to do that.

kind: Gateway
metadata:
  name: gateway-sample
  namespace: stunner
  annotations:
    stunner.l7mp.io/externalIP: "True"

Since it is up to us to specify what the gw.spec.addresses field set I think we should use it for this as well.

AND another idea on the same topic. Maybe we could use the annotation value to evaluate what to use the gw.spec.addresses field for. See:

kind: Gateway
metadata:
  name: gateway-sample
  namespace: stunner
  annotations:
    stunner.l7mp.io/address-hint-type: "loadbalancerip"

The above would set the svc.spec.loadbalancerIP field. And the following would set the svc.spec.externalIP[0] field stunner.l7mp.io/address-hint-type: "externalip"

And for finally another point for using the gw.spec.addresses to set the svc.spec.externalIPs field is that in this piece of code we take the address hint from the gw and not from the svc as the number one strategy here.

WDYT?

[rg0now]

Since the gateway API has the gw.spec.addresses field there is no reason to not use that field to set any type of addresses.

Absolutely.

Would it be completely stupid to unconditionally set svc.Spec.ExternalIPs from gw.spec.addresses (say, as option 0) and avoid the new annotation all together? I'm pretty confused as to which address is supposed to mean what from all these.

[davidkornel]

Closed with #33