Cert-manager: Consider re-adding CRDs to helm chart

[calind]

I want to propose to re-include CRD install in helm charts. This way cert-manager can be added as a dependency to another chart without the need of the parent chart to install the CRDs.

In our operator, in order to fix install if the CRDs are already installed, we do a check for .Capabilities.APIVersions.Has. See https://github.com/presslabs/mysql-operator/commit/2a49e6d670495d46a45a7deabce881f4a5b56239.

[munnerz]

Thanks for opening the issue 😄

I'd love to be able to reduce our install step down to a single helm install command, however solutions like you've suggested here still have edge-cases and caveats that can cause a lot more confusion for users and maintenance burden for us. Namely, with your example, it's not possible to perform any upgrades to CRD resources after they've been installed the first time.

This means the CRD objects are 'out of our control' once a user has set up cert-manager for the first time. As we start to make use of more features of CRDs (e.g. conversion webhooks), this is going to become a big issue.

I hear there are some proposals upstream in Helm to try and resolve the CRD handling issues though, so once we see those completed I think we can definitely move back 😄

[calind]

Yup, I share your pain points.

What we want to do for mysql-operator is let the controller reconcile it's CRDs if the need arises, and give users a manual update procedure if they don't want to grant the operator the necessary privileges. This is clearly suboptimal but users can helm install and helm delete --purge safely for the vast majority of use cases.

I don't have a strong opinion on this approach, just wanted to share.

[Nuru]

Cloud Posse has published a fork of the v0.7.0 chart that includes the CRDs with the "helm.sh/hook": crd-install annotation for those who want to use it.

$ helm repo add cloudposse-incubator https://charts.cloudposse.com/incubator/
$ helm install cloudposse-incubator/cert-manager

[MPV]

For reference, one could also look at how the prometheus-operator stable chart handles their CRDs...

...during installs:
https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus-operator/crd-prometheus.yaml#L1-L13

...and removal:
https://github.com/helm/charts/blob/master/stable/prometheus-operator/templates/prometheus-operator/cleanup-crds.yaml#L1-L10

[MPV]

Helm has also fixed their CRD hooks issue upstream:
https://github.com/helm/helm/pull/5112

...which might help unblock this issue?

[kfox1111]

+1 to ensuring crd updates work before readopting helm hooks. I love helm, but its crd support has been quite fragile. I wouldn't assume that because they fixed one more crd bug with it that all the issues are now resolved.

[retest-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

[munnerz]

Support for Helm 2 is on its last legs - we will be considering this as part of our move forward towards 1.0, and we'll also be moving to support Helm 3 with our chart. Once we have testing for this in place, and a better understand of Helm 3's behaviour with CRDs, we'll look whether it is feasible/safe to add the CRDs back to the chart.

In the meantime I'm going to close this as I think it will be considered as part of a larger discussion on our deployment mechanisms.