allow setting different annotations for headless service in the distributor

[KyriosGN0]

What this PR does

this PR allows users to specify annotation to the distributor headless service alone (meaning without also setting them on the actual clusterIP service)

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

• Tests updated.
• Documentation added.
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• about-versioning.md updated with experimental features.

[narqo]

Appreciate the PR. As I mentioned in the comment below, the current changes seem to break the backwards compatibility.

It's clear what the changes here are doing, but I don't understand the problem that is being solved. That is, what is the use-cases where one needs a special set of annotations only in one distributor's service?

[narqo]

This is a breaking change. The setups that are currently in production, will stop working after they update to this one.

[KyriosGN0]

@narqo for example we want to set the distributor service as load balancer with external-dns annotations, because both the headless service loads the same annotations (i.e. external-dns.alpha.kubernetes.io/hostname), external dns gives that record 2 IPs, one which point to the load balancer service and the other to the headless service (which is cluster ip) this causes us to sometimes get connection error when hitting that ip through dns

i understand this a breaking change, is there a special procedure i need to follow in order to properly announce it (assuming it is ok with the maintainers)

[narqo]

So the issue is that you need to segregate the annotations of the clusterIP and the headless services, to be able to expose the former to an external LB? The gateway component is what the chart comes with for this purpose. This deploys an API gateway (the Mimir open-source uses nginx), whose service you can expose to the LB. This may solve it for you without needing for a breaking change in the chart.

In future major version of the chart we should revisit the layout of the components and their values. For now, though, we're restricting ourselves to avoid any breaking changes. If you can think of a way to solve what you were trying to archive w/o breaking the existing code, we can iterate on that.

[KyriosGN0]

@narqo i can wrap it with another boolean value to opt in to this behavior, is that acceptable ? if not i'll open an issue and will wait until the next major version

[dimitarvdimitrov]

I'd opt for doing it in a non-breaking manner because the next major release is still not planned. Can we for example make the headlessAnnotations be in addition to annotations instead of a replacement?

[KyriosGN0]

@dimitarvdimitrov this doesn't solve the use case that i have (and we don't want to use nginx, we want to use the native services and ingress)
i can wrap it in a flag to opt in to the behavior, if not i'll wait until the next major release

[dimitarvdimitrov]

I'm thinking of users who are already setting annotations. In your case you will set annotations: {}. headlessAnnotations will still be added to the headless service only. So the rendered manifests will be the same as the ones that the current version of this PR would create. Am I missing something? 👀

[KyriosGN0]

@dimitarvdimitrov from what i understand it might be problemtic to users who relay on the both the headless service and clusterIP service having the same annotations, even with the my current changes they can achive this behavior, they can just use the same annotations in headlessAnnotations

hoping im making sense 🙏