Additional clusters for envoy configuration

[wilmer05]

What?

A LoadBalancer could have endpoints belonging to different envoy clusters, so it is possible in the future to configure Server Name Indication (SNI) and health checking.

Why?

If a Load Balancer need to be configured with SNI you can specify different domains to different group of endpoints. The same happens for health checking. A customer may want to configure different health checking for each of the endpoints. Grouping endpoints is a way to achieve this. Health checking is configured at the cluster level in the envoy configuration and grouping endpoints avoids having to add a health check configuration per endpoint.

How?

Currently there is no notion of EndpointGroups at in the LoadBalancer type. One way to introduce this, is by extending this type:

// LoadBalancerSpec defines the desired state of LoadBalancer
type LoadBalancerSpec struct {
	...
	// ADDED: EndpointGroups defines the Endpoints Groups for the LoadBalancer.	
	// +optional
	EndpointGroups []LoadBalancerEndpointGroup `json:"endpointGroups,omitempty"`

	// Endpoints defines the Endpoints for the LoadBalancer.
	Endpoints []LoadBalancerEndpoint `json:"endpoints,omitempty"`
	...
}

Also we would need to extend the LoadBalancerEndpoint type:

// LoadBalancerEndpoint defines a Endpoint for the LoadBalancer
type LoadBalancerEndpoint struct {
	// Name defines a name for the Endpoint (example: node name).
	Name string `json:"name"`
	// Addresses is a list of addresses for the endpoint, they can contain IPv4 and IPv6 addresses.
	Addresses []string `json:"addresses,omitempty"`	
	
	// ADDED: Groups is a list of references to the groups the endpoint belongs to 
	// +optional
	Groups: []string `json:"groups,omitempty"`
}

Where the groups are references to the endpoint group the endpoint is belonging to.

// ADDED: LoadBalancerEndpointGroup defines an EndpointGroup for the LoadBalancer
type LoadBalancerEndpointGroup struct {
	// Name defines a name for this Endpoint Group (example: node name).
	Name string `json:"name"`
	
	// FOR LATER: DomainName defines the domain name for this Endpoint Group (example: group.domain.de).	
	// +optional
	DomainName string `json:"domainName"`
	
	// FOR LATER: HealthCheck defines a health check for this Endpoint Group	
	// +optional
	HealthCheck HealthCheck `json:"healthCheck"`
}

From this, the yawollet logic could include now to add multiple clusters to envoy and we can in the future extend the endpoint group type with a protocol to be used or add a domain name per cluster to support SNI.

Migration

1. If an endpoint is not part of any endpoint group then it will belong to the implicit default endpoint group since it keeps the API backwards compatible and it helps on the migration of old instances of load balancers.

Questions

1. Should we deprecate the use of endpoints with no endpoint groups?

[hikhvar]

I would move the linking between a group and an endpoint from the endpoint to the endpoint group and use the following datastructure:

// ADDED: LoadBalancerEndpointGroup defines an EndpointGroup for the LoadBalancer
type LoadBalancerEndpointGroup struct {
	// Name defines a name for this Endpoint Group (example: node name).
	Name string `json:"name"`
	
	// FOR LATER: DomainName defines the domain name for this Endpoint Group (example: group.domain.de).	
	// +optional
	DomainName string `json:"domainName"`
	
	// FOR LATER: HealthCheck defines a health check for this Endpoint Group	
	// +optional
	HealthCheck HealthCheck `json:"healthCheck"`
	
	// ADDED: List of the endpoint names
	Endpoints: []string `json:"groups,omitempty"`
}

This enables the user to more easily construct a new endpoint group from a already computed set of endpoints. The other way around, we must modify all the existing endpoints to add or remove a new endpoint group.

To ease the API, would vote for removing the endpoints slice and move to endpoint groups only like this:

// LoadBalancerSpec defines the desired state of LoadBalancer
type LoadBalancerSpec struct {
	...
	// ADDED: EndpointGroups defines the Endpoints Groups for the LoadBalancer.	
	// +optional
	EndpointGroups []LoadBalancerEndpointGroup `json:"endpointGroups,omitempty"`
	
	// REMOVED: 
	// Endpoints []LoadBalancerEndpoint `json:"endpoints,omitempty"`
	...
}

[dergeberl]

@nightlyone What do you mean with endpoint group is managed? The yawollet needs to react to all changes of the LoadBalancer-Object. Fore each change the yawollet` creates a new EnvoyConfig and if there are changes, than it configures Envoy.

[nightlyone]

@dergeberl Looks like my comment got truncated. The endpoint group with explicitly managed membership would document when changes are done and what those are, not matter the change. With labeling it needs needs to detect and track that change somewhere. So now you have a 2nd source of changes,

I would switch to that model once autoscaling is implemented for vm. e.g, when some autoscaling vm grouping for openstack is implemented.

[hikhvar]

What you usually do with kubernetes controllers is to subscribe your endpoint group controller to endpoints as well. Than use lookup function to find the endpoint group if a endpoint is changed: https://book.kubebuilder.io/reference/watching-resources/externally-managed.html

[hikhvar]

ok, had another look into the data structures. I somehow assumed that the LoadBalancerEndpoint are already a own custom resource. With that structure, the following would be possible:

apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancer
spec:
  ...
  EndPointGroups:
      - name: http
        HealthCheck:
          foo: bar
        
        EndpointSelector:
        app: http
        
        - name: tcp
          HealthCheck:
            foo: bar
        
          EndpointSelector:
        protocol: tcp
---
apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancerEndpoint
metadata:
  labels:
    app: http
spec:
  name: firstEndpoint
  addresses:
    - ...
---
apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancerEndpoint
metadata:
  labels:
    protocol: tcp
spec:
  name: secondEndpoint
  addresses:
    - ...
---
apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancerEndpoint
metadata:
  labels:
    app: http
    protocol: tcp
spec:
  name: thirdEndpoint
  addresses:
    - ...		

This also works well with autoscaling etc, because in that case the autoscaling must only create a new Endpoint resource.

[breuerfelix]

@hikhvar creating more custom resources the yawollet needs to watch also means, that we need to have a more complex role for the yawollet kubernetes client, since currently it is only able to see its own LBM object. That way, the yawollet of one customer is not able to see other loadbalancermachines from other customers (since with your implementation they share one namespace).
I would not create separate CRDs for endpoints. To me it feels like it would "bloat" yawol a little.

@nightlyone well there is no need to "track" the endpoints with a specific label since all endpoints are inside a list of the LB object. it will just be one if statement that filters out all endpoints with no label matching while iterating over the endpoints to generate the cluster.
Using label selector feels more like "the kubernetes way" although we are talking about an inline list here and not CRDs, thats why i am also fine with just referencing the endpoints as strings in the endpointsgroups.

[dergeberl]

Thanks for the suggestion. I have a few comments/ideas.

I also like the proposal from @hikhvar with the labelselector.

Overall, I think the settings for SNI should not be done in the Endpoint, It is more an Ingress/Listener topic. Currently, we use there the Ports which is a copy from the k8s service object. Maybe we should change here to an own type which supports all our needed features. For us, it was easy because the yawol-cloud-controller can just copy the types. (Currently, you can set UDP and TCP as Protocol for each Port. SNI should only work with TCP.) I think we should change it here too and add later the SNI Topic also there. This is much more configurable and a clearer.

Example:

// LoadBalancerSpec defines the desired state of LoadBalancer
type LoadBalancerSpec struct {
....
	// ADD: Listener defines listerner, ports and protocols for the LoadBalancer.
	Listener []LoadBalancerListener `json:"endpoints,omitempty"`
	// DEPRICATED, REMOVED in future
	// Ports defines the Ports for the LoadBalancer (copy from service)
	Ports []corev1.ServicePort `json:"ports,omitempty"`
....
}

// LoadBalancerListener only one of TCP, UDP, SNI can be set configuered.
type LoadBalancerListener struct {
	// TCP settings ...
	TCP *LoadBalancerTCPUDPListener 	`json:"tcp,omitempty"`
	// UDP settings ...
	UDP *LoadBalancerTCPUDPListener 	`json:"udp,omitempty"`
	// SNI settings ...
	SNI *LoadBalancerSNIListener 		`json:"sni,omitempty"`
	// EndpointGroup settings ...
	Groups []string 	    			`json:"groups,omitempty"`
	// Or LabelSelector
	LabelSelector metav1.LabelSelector	`json:"groups,omitempty"`
}

type LoadBalancerListenerInline struct {
	// Name the port on witch the LoadBalancer listens
	Name string `json:"name,omitempty"`
	// Port the port on witch the LoadBalancer listens
	Port int32 `json:"port,omitempty"`
	// TargetPort the port on which the LoadBalancer forwards to
	TargetPort int32 `json:"targetPort,omitempty"`
}

type LoadBalancerTCPUDPListener struct {
 	LoadBalancerListenerInline `json:",inline"`
}

// FOR LATER
type LoadBalancerSNIListener struct {
  	LoadBalancerListenerInline `json:",inline"`
	// DomainName defines the domain name for this Listener (example: domain.de).
	DomainName string `json:"domainName"`
}

In any case, it is important for the migration:
If there are new object types (like in the example above the LoadBalancerListener) it is important that the old path (in this case Ports) is only deprecated and nor removed. Because if you update to the new version first, the CRDs and yawol-controller updates (the yawol-controller triggers the update for the yawollet with a new LoadBalancerMachine). If the type is removed (for example Ports), the yawollet can not work anymore in the time until it gets also updates.

cc: @breuerfelix

[nightlyone]

A lot of features from the listener then need to be duplicated and can accidentally be specified at udp or tcp (but can be caught by a oneOf then.). Endpoint groups define basically machine pools + ports you can target via SNI.

The design suggested by @wilmer05 allows:

• Seamless migration of existing setup
• Machines can be in multiple endpoint groups at once without specifying them twice and they also get removed from all groups easily at once.
• The group struct allows it to get also features like active health checks so the target pool (aka endpoint group) manages its own service health as well.

SNI can of course also be defined at the listener but then you will need some mapping at a certain point which listener matches which endpoint group, (or match label). Esp, when having multiple listeners at a later point.

[dergeberl]

A lot of features from the listener then need to be duplicated and can accidentally be specified at udp or tcp (but can be caught by a oneOf then.). Endpoint groups define basically machine pools + ports you can target via SNI.
I dont get it here. When you set the SNI Domain on the Endpoint Groups and specify UDP as Protocol, than this can not work. But will be possible to configure.

It would be also possible to add more fields in the SNI Part, so that only one listerner is created for one Port. And you add all Domains to it for in a List. Example:

// LoadBalancerListener only one of TCP, UDP, SNI can be set configuered.
type LoadBalancerListener struct {
	// TCP settings ...
	TCP *LoadBalancerTCPUDPListener 	`json:"tcp,omitempty"`
	// UDP settings ...
	UDP *LoadBalancerTCPUDPListener 	`json:"udp,omitempty"`
	// SNI settings ...
	SNI *LoadBalancerSNIListener 		`json:"sni,omitempty"`
}

type LoadBalancerListenerInline struct {
	// Name the port on witch the LoadBalancer listens
	Name string `json:"name,omitempty"`
	// Port the port on witch the LoadBalancer listens
	Port int32 `json:"port,omitempty"`
	// TargetPort the port on which the LoadBalancer forwards to
	TargetPort int32 `json:"targetPort,omitempty"`
}

type LoadBalancerTCPUDPListener struct {
 	LoadBalancerListenerInline `json:",inline"`
	// EndpointGroup settings ...
	Groups []string 	    			`json:"groups,omitempty"`
	// Or LabelSelector
	LabelSelector metav1.LabelSelector	`json:"groups,omitempty"`
}

// FOR LATER
type LoadBalancerSNIListener struct {
  	LoadBalancerListenerInline `json:",inline"`
	// Domains defines ....
	Domains []LoadBalancerSNIDomains `json:"domains"`
}

// FOR LATER
type LoadBalancerSNIDomains struct {
  	LoadBalancerListenerInline 			`json:",inline"`
	// DomainName defines the domain name for this Listener (example: domain.de).
	DomainName string 					`json:"domainName"`
	// EndpointGroup settings ...
	Groups []string 	    			`json:"groups,omitempty"`
	// Or LabelSelector
	LabelSelector metav1.LabelSelector	`json:"groups,omitempty"`
}

It feels wrong to configure it on the Endpoint side.

You are right, this is a change which includes API changes. Which must be migrated, but then we are much more free.

Machines can be in multiple endpoint groups at once without specifying them twice and they also get removed from all groups easily at once.

This is also possible with this setting. With the LabelSelector and with Groups as a String slice.

The group struct allows it to get also features like active health checks so the target pool (aka endpoint group) manages its own service health as well.

This can also be done on the Listener side. It is just a other place for the configuration.

[breuerfelix]

@dergeberl proposal requires migration, thats true, but it allows more customisability in the long run.
Even though our configuration should not be an "envoy rip-off" and should abstract the complex envoy configuration, having Listeners and Endpoints as an abstraction layer to Envoys Listener and Clusters seems just right to me.

[nightlyone]

@hikhvar / @dergeberl a label selector implies that there is an entity that is currently labeled. Does such an entity already exist in your case? In our use case we would need to watch all openstack vms for that openstack project for label changes for that to work. Explicitly listing them sounds simpler to me and is backward compatible as well. It also allows to decouple updating the list of endpoints from using them. Labeling also requires actions on the admins of those VMs which were not necessary before.

[breuerfelix]

@wilmer05 with label selectors you can even create more complex scenarios than with the one you proposed because endpoints and groups both supports lists.

apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancer
....
spec:
  endpoints:
  - addresses:
    - 10.10.10.10
    name: test1
    labels:
      app: application
  - addresses:
    - 10.10.10.11
    name: test2
    labels:
      app: application2
      foo: bar
...
  listener:
    - tcp:
        name: status-port
        targetPort: 32551
        port: 15021
      groups:
        app: application
        foo: bar
    - sni:
        name: status-port
        targetPort: 32551
        port: 15021
        domainName: example.com
      groups:
        app: application2

In this example the second endpoint belongs to both endpoint groups. Labels basically allow list-list mappings.
Of course this depends on the actual implementation but usually it should be "if one label matches, it matches".
/edit
there was a misunderstanding here on my side with the labels. it is "all group labels have to match in order to match".
in the following case the second endpoint belongs to two groups:

apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancer
....
spec:
  endpoints:
  - addresses:
    - 10.10.10.10
    name: test1
    labels:
      app1: application1
  - addresses:
    - 10.10.10.11
    name: test2
    labels:
      app1: application1
      app2: application2
...
  listener:
    - tcp:
        name: status-port
        targetPort: 32551
        port: 15021
      groups:
        app2: application2
    - sni:
        name: status-port
        targetPort: 32551
        port: 15021
        domainName: example.com
      groups:
        app1: application1

[nightlyone]

Ok @wilmer05 / @breuerfelix / @dergeberl / @hikhvar looks like we have a way to express this: With labels without values, which is a normal mode for k8s labels.
As labels and Selectors can work also with the key only, we can successfully express what we want:

apiVersion: yawol.stackit.cloud/v1beta1
kind: LoadBalancer
....
spec:
  endpoints:
  - addresses:
    - 10.10.10.10
    name: test1
    labels:
      purpose.vendor.tld/application1
  - addresses:
    - 10.10.10.11
    name: test2
    labels:
      purpose.vendor.tld/application1
      purpose.vendor.tld/application2
...
  listener:
    - tcp:
        name: status-port
        targetPort: 32551
        port: 15021
      matchLabels:
        purpose.vendor.tld/application1
    - sni:
        name: status-port
        targetPort: 32551
        port: 15021
        domainName: example.com
      matchLabels:
        purpose.vendor.tld/application2

That would still allow to switch to extra endpoint objects later, if k8s watches are not limiting us anymore one day.

Since currently endpoints have no labels we would specify that the empty set of matchLabels will match

1. all unlabeled endpoints.
2. all endpoints, no matter whether labeled or not.

I would prefer solution 1 over 2 as it enables a migration path and you don't need to pay the labeling effort price for sitioations where you neither need nor want that while still allowing a migration path towards labeling.
What is your opinion on this?

[nightlyone]

@breuerfelix / @dergeberl / @hikhvar does silence mean agreement to solution 1 here?

[hikhvar]

I think solution one is good enough.

[dergeberl]

@nightlyone sorry for the late answer. 1. all unlabeled endpoints. sounds good. 👍🏻