Add support for setting additional annotations on the CRDs #1402
Conversation
|
|
| version: 0.71.2 | ||
| version: 0.72.0 |
There was a problem hiding this comment.
you can change this to only bump the patch version
There was a problem hiding this comment.
Rebased on bumped again.
I assume that was part of why the previous CI run failed.
Would you mind approving the workflow? It looks like we won't be able to merge if the workflow doesn't run before a new tag is created.
There was a problem hiding this comment.
Not sure how to fix the Test Operator Charts jobs.
Any suggestion?
This is especially useful since CRDs are now installed through templates. Because of this change that was introduced in 0.57.0, now running `helm uninstall` of the operator chart will delete the CRDs as well, which is a destructive operation. The workaround is to leverage the special helm.sh/resource-policy=keep annotation, see https://helm.sh/docs/howto/charts_tips_and_tricks/#tell-helm-not-to-uninstall-a-resource One currently open issue though, is that the annotation needs to be set by Helm itself, see helm/helm#10890 So running kubectl annotate on the CRDs as a post-install step doesn't work.
|
i believe you need to run |
But for some reason, doing so reverts the very changes I want to bring. |
|
argh, i think this is because of the sed in the makefile... @swiatekm would you be able to help out? |
|
I'm not convinced letting you keep the CRDs via Helm magic is something we should support in this Chart. If this is what you actually want, you should disable CRD installation in this Chart and either use https://github.com/open-telemetry/opentelemetry-helm-charts/tree/main/charts/opentelemetry-kube-stack/charts/crds or manage them on your own. |
Well there is value in leveraging what is already managed by the opentelemetry-operator chart, that is embedding the CRDs at the version that presumably matches the operator's, and handling the webhook configuration. I could indeed reproduce that (I actually already had my own Kubernetes job to handle the CRDs upgrades), but I don't really see the point now that it's something directly supported, especially since I expect https://github.com/open-telemetry/opentelemetry-helm-charts/tree/main/charts/opentelemetry-kube-stack/charts/crds/crds to eventually follow the same pattern to properly configure the webhook. Beside, from this chart's point of view, it's just supporting custom annotations on the CRDs, not advertising a particular use case, we could probably find others where that comes in handy. Let me know what you think. |
The annotations themselves are fine, although adding templating to the CRDs is a pain from a tooling point of view. If you want this to work, see this Makefile macro, and ensure it adds the annotation bits. But more broadly, wanting to do these kinds of adjustments is usually a sign that you should be managing the CRDs yourself. It's unfortunate that the K8s ecosystem in general and Helm more specifically, don't give us good tools to do this automatically, but that won't change in the near future. At the very least, be aware that we don't test for this scenario.
For the record, the opentelemetry-kube-stack chart won't follow the same pattern. One of the reasons it does things differently is so you can pick which method you prefer. I don't think this is clearly documented anywhere, but it's our intent going forward. Managing CRDs with Helm is a bit of a crapshoot in general, and the best we can really do is let users pick their poison. |
|
Got you 👌 |
This is especially useful since CRDs are now installed through templates. Because of this change that was introduced in 0.57.0, now running
helm uninstallof the operator chart will delete the CRDs as well, which is a destructive operation.The workaround is to leverage the special helm.sh/resource-policy=keep annotation, see https://helm.sh/docs/howto/charts_tips_and_tricks/#tell-helm-not-to-uninstall-a-resource
One currently open issue though, is that the annotation needs to be set by Helm itself, see helm/helm#10890 So running kubectl annotate on the CRDs as a post-install step doesn't work.