Introduce installCRDs flag for easier CRD management

[j1m-ryan]

Issue #5063 proposes adding an installCRDs flag, similar to cert-manager, to improve our CRD management, which is a common pain point.

One case against adding this, is that it goes against helms CRD management guidelines, but if it's optional and defaults to false it could strike a balance, being a user friendly option for people with simpler setups.

We made a recent update to our helm NOTES.txt file for guiding users through upgrades. A more integrated solution like a installCRDs flag could further improve user experience.

Maybe it is worth a review of popular projects which use CRDs, to determine whether they use something like installCRDs, or the dual-charts method, or like us, require manual upgrades of CRDs after a helm upgrade if the CRDs changed?

[brianehlert]

Helm has officially avoided this as a core function. Which leaves us to sort out a path for the project.
This is not a new issue, and most customers seem to expect that the project handles it and can handle upgrading the CRDs.
As the popularity of the project has grown, the occurrence of asks around this issue has continued to increase.
We should address this somehow, even though the Helm project has decided to provide no common support or guidance.

[kyrofa]

One case against adding this, is that it goes against helms CRD management guidelines [...]

Helm has very good reasons for not supporting this generically. As I understand it, it ultimately comes down to this:

The core problem is that CRDs (being a globally shared resource) are fragile. Once a CRD is installed, we typically have to assume (all other things being equal) that it is shared across namespaces and groups of users.

For that reason, installing, modifying, and deleting CRDs is a process that has ramifications for all users and systems of that cluster.

I totally get where they're coming from, there, but I'm not sure that should be interpreted as generic guidance for all Helm charts. It really comes down to the individual chart, wouldn't you agree? The fact is, CRDs are a global resource. Changing them at all is a risk, but change is a fact of life: if the user wants to use a new chart, they're going to need the new CRD. I expect individual charts try to be as careful with CRD changes as they are with any other kind of public API. I think one big question, though, is whether nginx actually needs its CRDs to run. In other words: can we actually do this in one chart? Or are two actually required to pull this off, which changes the proposal a bit? Metallb actually makes one the dependency of the other, which is an interesting take on it.

Then it comes down to how nginx is deployed. If there are multiple ingress controllers in a cluster, they are sharing the same CRDs. In that case, we can say that the user should handle the CRD upgrades outside of the ingress controller upgrades, but as far as I can tell that still assumes that the new CRDs were changed in a way that is still compatible with the old version of the controller. I'm not sure how different that really is from having one of the charts handle those upgrades, but that sounds awkward doesn't it. In reality, I'd say the multiple-ingress-controller situation will always be awkward, even if this helm chart took care of that upgrade (either the way we're discussing here, or via a dual-chart pattern like Rook).

But there's one very clear use-case where having the chart handle CRD upgrades is clearly useful: when a cluster has a single ingress controller which alone is consuming its CRDs. The risk of upgrading the CRDs here seems awfully low, as long as the user has opted into it and read the bit of documentation that says not to do it with multiple ingress controllers.

[...] but if it's optional and defaults to false it could strike a balance, being a user friendly option for people with simpler setups.

This outlines my thinking indeed. As long as the user meets the one requirement of not having multiple ingress controllers, if they enable this option, it makes it virtually impossible for them to mess up their CRDs. That sounds like a win.

[jasonwilliams14]

@kyrofa
Helm also has a best practices approach for managing CRDs with a separate chart:
https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

Keeping closely aligned with helms approach and best practices is a preferred approach.
This is something we are looking at.

[kyrofa]

Yes that's the approach used by Rook, and it works perfectly. It's where I was driving with this point:

I think one big question, though, is whether nginx actually needs its CRDs to run. In other words: can we actually do this in one chart? Or are two actually required to pull this off, which changes the proposal a bit?

The maintenance on nginx's end feels like it would be a little more work, but I'm definitely on board as a user. Also, as a user, it seems that it would be trivial to migrate to this method from how nginx is deployed today, because you're really just introducing another chart to manage CRDs which aren't currently managed at all.

[jasonwilliams14]

While cert-manager does have a installcrds flag for their helm chart, cert-manager recommends for production environments to use kubectl:

https://cert-manager.io/docs/installation/helm/#option-1-installing-crds-with-kubectl

That being said, I would be more inclined to research the CRDs in a helm subchart over using installCRDS flag approach.

[sass1997]

I would also vote for the approach to have a seperate crd chart. Like it's done in https://github.com/prometheus-community/helm-charts/tree/main/charts/prometheus-operator-crds as example

[danielnginx]

@oseoin could we get your opinion?