feat: Enable custom label selectors for Services

[lindhe]

Is your feature request related to a problem? Please describe.

I'm working to replace an old instance of RabbitMQ with an instance managed by this Operator. So in my namespace, I already had a RabbitMQ pod with label app.kubernetes.io/name: rabbitmq. Unfortunately, it seems like this breaks the Operator's assumptions, since the Services deployed use a selector that is hard-coded to the app.kubernetes.io/name label with a value equal to the name of the cluster:

cluster-operator/internal/resource/service.go

Line 69 in fa050de

service.Spec.Selector = metadata.LabelSelector(builder.Instance.Name)

Since I named the RabbitmqCluster instance rabbitmq, this caused a collision. Not a big deal, but it made my migration work a bit more awkward than ideal.

Describe the solution you'd like

I think it would be useful if (a) the default selectors were more specific and (b) it was possible to configure the labels and selectors used.

Describe alternatives you've considered

I can work around this by changing the name of my RabbitmqCluster to something more unique. That works, but does not looks as pretty as I would wish.

[Zerpet]

We follow Kubernetes recommended labels [1]. The service created by the operator is part of the "application", and therefore, setting the CR name as "application" name is correct.

I think it would be useful if (a) the default selectors were more specific

I guess you are suggesting a private label like com.rabbitmq/name. It would work from a technical PoV, but we never intended to support DIY rabbit-on-k8s + Operator rabbits in the same namespace. Plus we are using recommended Kubernetes labels 🤷‍♂️

(b) it was possible to configure the labels and selectors used

I'm not keen to do this. You are suggesting that we should let the user interfere with the automatic wiring performed by the operator to make RabbitMQ on k8s work. Perhaps it's possible to do this via the service override [2]. If it's not possible via override, we won't accept a change to this aspect. It simply goes against the operator mission: deploy and manage the lifecycle of RMQ on k8s.

[1] https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/
[2] https://www.rabbitmq.com/kubernetes/operator/using-operator#override

[lindhe]

We follow Kubernetes recommended labels [1]. The service created by the operator is part of the "application", and therefore, setting the CR name as "application" name is correct.

Great that you bring up Kubernetes' recommended labels! I think those are excellent to try and adhere to.
That documentation is actually highly relevant for my point here:

In order to take full advantage of using these labels, they should be applied on every resource object.

Note "they should be applied on every resource object", meaning all of them. But the operator only applies a single one of them (the app.kubernetes.io/name label). All the other recommended labels are left out and thus it does not adhere well to the recommendation.

What I meant with having more specific selectors was mostly that more labels should be used to select the resources, not necessarily that the particular value should be highly unique. By having multiple labels (and assuming they are reasonably distinctive in scope, as per the recommendation), resource selection in Kubernetes can be very flexible. One might want to select quite broadly in some scenarios (for example when applying a search query in Grafana) or one might want to select a very precise instance of a resource (like with a label selector in a Deployment or Service). This is common practice, as indicated by the recommended labels.

I think that it is a reasonable default by the operator to set the app.kubernetes.io/name label of the Pods to the name of the RabbitmqCluster resource! But without the other recommended labels, resource selection becomes rather limited. And that is not your fault – you could not reasonably know what the values should be for app.kubernetes.io/instance or app.kubernetes.io/part-of should be where the RabbitmqCluster resource gets deployed! That must be up to the end user to specify, which is why I'm suggesting to add optional custom labels (which is optionally also selected on).

[Zerpet]

I see your point, and I can be convinced of adding app.kubernetes.io/component as an additional selector to the Service. I could also be convinced that app.kubernetes.io/part-of and app.kubernetes.io/instance could be configurable by the user. Those two changes are quite simple and they are not risky.

I forgot to mention in my first response: note that you can apply any labels (and annotations) you want to all resources created by the operator [1]. Simply label/annotate the RabbitmqCluster object, and they will be pushed down to all objects created by the operator. Note that Pods won't inherit custom labels/annotations, because that broke a valid git-ops use case (can't remember the issue number).

I'm still against making the selectors configurable, because that can interfere with the wiring that the operator does to connect resources together. I must add that I feel quite strong about this point.

Note "they should be applied on every resource object", meaning all of them. But the operator only applies a single one of them (the app.kubernetes.io/name label). All the other recommended labels are left out and thus it does not adhere well to the recommendation.

That's not precise. The Operator applies the labels to all resources it creates. You don't have to take my word, you can see the code for yourself [2]. It is true that the operator does not create all recommended labels, because some of them, as you pointed, can't be known at deploy time, or known at all by the operator. At the moment, the operator filters them, which is not ideal. That's also an easy change to make that would be accepted.

[1] https://www.rabbitmq.com/kubernetes/operator/using-operator#labels-annotations
[2] https://github.com/search?q=repo%3Arabbitmq/cluster-operator%20GetLabels&type=code

[lindhe]

I'm glad to hear that. 👍 And thanks for mentioning the custom labels for the resources directly created by the operator – I think it is useful that it works like that.

Note that Pods won't inherit custom labels/annotations, because that broke a valid git-ops use case (can't remember the issue number).
[…]
I'm still against making the selectors configurable, because that can interfere with the wiring that the operator does to connect resources together. I must add that I feel quite strong about this point.

Are you referring to #442?

It seems like the issue in that case was that labels from .metadata.labels on the RabbitmqCluster resource (or perhaps .metadata.labels on the StatefulSet, even) got propagated into the StatefulSet's selectors and the .metadata.labels field of the Pod resources. I agree that this can be problematic and I did not mean to suggest we do that.

To be clear: what I wanted so suggest was a mechanism to set custom labels on the resulting Pods created by the StatefulSet and use them as selectors in the StatefulSet and Service resources upon initial creation. I don't think the Pods should automatically inherit their parent resource's labels.

Setting new Pod labels is clearly a benign change. I'm guessing this is what you refer to by "At the moment, the operator filters them, which is not ideal. That's also an easy change to make that would be accepted."? The only potential danger lies with implementing the selectors, not setting the labels.

Regarding the selectors: it might be harmful to change selectors mid-operation. If custom labels were allowed to be propagated into the selectors of StatefulSet and Service resources, it might be wise to set that field as immutable to prevent users from modifying it after first creation. I think Kustomize has phrased it well in their documentation:

Selectors for resources such as Deployments and Services shouldn’t be changed once the resource has been applied to a cluster.

Changing includeSelectors to true or changing labels when includeSelectors is true in live resources is equivalent to changing commonLabels and could result in failures.

[Zerpet]

Are you referring to #442?

It seems like the issue in that case was that labels from .metadata.labels on the RabbitmqCluster resource (or perhaps .metadata.labels on the StatefulSet, even) got propagated into the StatefulSet's selectors and the .metadata.labels field of the Pod resources. I agree that this can be problematic and I did not mean to suggest we do that.

Yes, that's the issue I had in mind 🙂 Setting labels in the STS selectors was part of the problem. The other part of the problem was that we set the CR labels as Pod labels, discarding any additional labels present in the Pod. In other words, if you ran kubectl label rabbit-pod foo=bar, the operator would remove your foo=bar label and set the labels in the CR as Pod labels.

To be clear: what I wanted so suggest was a mechanism to set custom labels on the resulting Pods created by the StatefulSet and use them as selectors in the StatefulSet and Service resources upon initial creation. I don't think the Pods should automatically inherit their parent resource's labels.

Setting new Pod labels is clearly a benign change. I'm guessing this is what you refer to by "At the moment, the operator filters them, which is not ideal. That's also an easy change to make that would be accepted."? The only potential danger lies with implementing the selectors, not setting the labels.

Regarding the selectors: it might be harmful to change selectors mid-operation. If custom labels were allowed to be propagated into the selectors of StatefulSet and Service resources, it might be wise to set that field as immutable to prevent users from modifying it after first creation. I think Kustomize has phrased it well in their documentation:

Selectors for resources such as Deployments and Services shouldn’t be changed once the resource has been applied to a cluster.
Changing includeSelectors to true or changing labels when includeSelectors is true in live resources is equivalent to changing commonLabels and could result in failures.

The key here is upon initial creation, because updates to STS selectors are rejected by the STS controller. The more I think about it, the less I like allowing configurability of the selectors. I believe that you can tweak the selectors to your liking using the override feature [1]. Did you try that by any chance?

Regarding Pod labels, I quite like the current experience of setting CR labels and those being propagated down. We could include Pods as propagation targets. We should not break the Flux use case if we simply ignore existing labels on the Pods.

[1] https://www.rabbitmq.com/kubernetes/operator/using-operator#override

[lindhe]

I believe that you can tweak the selectors to your liking using the override feature [1]. Did you try that by any chance?

I must admit I don't recall if I did try that. Thanks for pointing it out, let me get back to you on that.

[github-actions]

This issue has been marked as stale due to 60 days of inactivity. Stale issues will be closed after a further 30 days of inactivity; please remove the stale label in order to prevent this occurring.