Add loadBalancerClass field to ListenerClass.spec

[nightkr]

Description

See stackabletech/listener-operator#285, part of stackabletech/listener-operator#288

This PR adds a two new fields to ListenerClass:

spec:
  loadBalancerClass: foo
  loadBalancerAllocateNodePorts: true

Which are mapped to Service.spec.{loadBalancerClass,allocateLoadBalancerNodePorts}, respectively. Currently these are ignored unless serviceType: LoadBalancer; in v1alpha2, serviceType should be turned into a rich enum instead, and these fields be moved into it, like so:

spec:
  serviceType:
    loadBalancer:
      loadBalancerClass: foo
      allocateNodePorts: true

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

1. Changes are OpenShift compatible
2. CRD changes approved
3. Integration tests passed (for non trivial changes)

Reviewer

Preview Give feedback

1. Code contains useful comments
2. (Integration-)Test cases added
3. Documentation added or updated
4. Changelog updated
5. Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

Preview Give feedback

1. Feature Tracker has been updated
2. Proper release label has been added

[nightkr]

Moving the CRD changes into RFC

[siegfriedweber]

The suggestion looks okay for me. Nevertheless, I would like to present my thoughts that lead me to another suggestion:

• How would the spec look like in v1alpha2 for ClusterIP and NodePort where usually nothing is defined?

  spec:
    serviceType:
      clusterIp: {}

  The default use case will look slightly uglier than now.
• The additional Service properties are sometimes specific to the Service type (e.g. loadBalancerClass) and sometimes they are not (e.g. publishNotReadyAddresses). Kubernetes decided against a deep structure in the Service spec for whatever reason. Why should we introduce one if users already got used to the flat structure?
• The additional properties "are" not a service type, they are part of the service with a specific type, so serviceType: <type identifier> feels more accurate than serviceType: <structure where the next key is the type identifier>.
• We prefixed serviceType with service because it is the type of the Service, not the type of the ListenerClass. It might be consistent, to also prefix the other Service properties, e.g. serviceLoadBalancerClass.
• The Service property allocateLoadBalancerNodePorts is named loadBalancerAllocateNodePorts in the Listener, probably to emphasize that it belongs to the Service type loadBalancer. It is easier for the user, if Kubernetes knowledge can just be applied to our platform without translation.
• In v1alpha2, loadBalancerClass is called loadBalancerClass in the loadBalancer structure, but loadBalancerAllocateNodePorts is renamed to allocateNodePorts. In which cases will the prefix be removed and in which ones not?

The ListenerClass requires Service properties. Why not just add an according structure where the properties can be set in exactly the same way, as they are set in Services:

spec:
  serviceProperties:
    type: LoadBalancer:
    loadBalancerClass: manual
    allocateLoadBalancerNodePorts: false

If we do not want to introduce a breaking change, we could just keep the field serviceType:

spec:
  serviceType: LoadBalancer
  serviceProperties:
    loadBalancerClass: manual
    allocateLoadBalancerNodePorts: false

[nightkr]

How would the spec look like in v1alpha2 for ClusterIP and NodePort where usually nothing is defined?

Yeah. Not super pretty, but also consistent with emptyDir: {} and http01: {}.

The additional Service properties are sometimes specific to the Service type (e.g. loadBalancerClass) and sometimes they are not (e.g. publishNotReadyAddresses). Kubernetes decided against a deep structure in the Service spec for whatever reason. Why should we introduce one if users already got used to the flat structure?

Looking back at v1.0.0 (https://github.com/kubernetes/kubernetes/blob/v1.0.0/api/swagger-spec/v1.json#L13468-L13499), they seem to have made the same mistake as us: made it a string enum before they had any per-type options, and then they were unable to change it since it was already a stable API.

They also do some weird pseudo-enumisms (For example: "This field will be wiped when a service is updated to a non 'LoadBalancer' type." for loadBalancerClass).

We prefixed serviceType with service because it is the type of the Service, not the type of the ListenerClass. It might be consistent, to also prefix the other Service properties, e.g. serviceLoadBalancerClass.

In retrospect, I think this might've been a mistake. Creating a backing Service is one way to implement a Listener, but it's not really core to the API contract at all (for example, I could see future ListenerClass types that use Ingress or Gateway instead, or which bypass K8s networking entirely).

It's a tricky balance between letting people reuse existing K8s knowledge and owning our own API that we can adapt to our own needs.

The additional properties "are" not a service type, they are part of the service with a specific type

They are parameters for instantiating that service type, and is similar to how rich enums are handled elsewhere (Volume). Unless you mean that you object to the word "type", in which case I agree but don't have a better option to offer.

In v1alpha2, loadBalancerClass is called loadBalancerClass in the loadBalancer structure, but loadBalancerAllocateNodePorts is renamed to allocateNodePorts. In which cases will the prefix be removed and in which ones not?

Mentally, I read loadBalancerClass as "load-balancer class" (one concept that implies that it is only active in load-balancer mode), but loadBalancerAllocateNodePorts as "when in load-balancer mode, allocate node ports?" (one concept guarded by a conditional). I would drop the conditional when implied by context, but would not drop half the concept name.

The ListenerClass requires Service properties. Why not just add an according structure where the properties can be set in exactly the same way, as they are set in Services:

My response to this is similar to the "prefix everything with service" comment: I'm skeptical to this because it doesn't really define a clear identity for listeners.

If you want the prefix for "anything that currently relates to Services at all" then the prefix applies to, well, the whole CRD and we gain pretty much nothing.

If you want the prefix for "things that are passed directly through to the Service" then I'm hesitant to apply it to anything because I think we should own the API, and what it means. The exception would be in defining some sort of generic serviceOverride mechanism, perhaps (similar to our existing podOverride). I would also class our existing serviceAnnotations under this bucket of "overrides for pragmatic reasons, even if it doesn't quite line up with our vision for our API".