Support opt-in managing of CRDs by helm chart

[kyrofa]

nginx continues to be the only chart I've used where its CRDs are not managed by Helm. Some charts use a dual-chart method (e.g. rook), others defy Helm's guidance and update the CRDs as part of the chart (e.g. metallb, cert-manager).

I understand all of the reasoning behind why things work this way. However, this still bit me pretty hard in the past (#4856), and appears to be continuing to bite other people (e.g. #4931). In other words, this is not intuitive behavior, and I think you would agree that ideally this would be solved, at least for simple cases. I wonder if following cert-manager's lead here would be worthwhile. By default, cert-manager's helm chart does not install or update cert-manager's CRDs. However, they support the installCRDs boolean option, where if set to true, the chart will both install the CRDs and keep them up to date.

This only works in some specific scenarios, of course. Because CRDs are cluster-wide, it pre-supposes that you only have one cert-manager, or only one ingress controller. In my case (and I suspect a lot of smaller clusters), this is absolutely true. I would very much like to give the nginx ingress controller helm chart permission to manage its own CRDs.

How would folks feel about supporting this as an opt-in option?

[github-actions[bot]]

Hi @kyrofa thanks for reporting!

Be sure to check out the docs and the Contributing Guidelines while you wait for a human to take a look at this :slightly_smiling_face:

Cheers!

[j1m-ryan]

Hi @kyrofa, thanks for the proposal. I think that an installCRDs flag like cert-manager has, is something we should have a discussion about. I have started a discussion for this here.

I do agree with you that this is a pain point. One small way that we have addressed this recently, which has not been released yet is by updating our helm text to give the user some next steps.

On an upgrade the user will see this:

NOTES:
The NGINX Ingress Controller 3.4.0 has been installed.

For release notes for this version please see: https://docs.nginx.com/nginx-ingress-controller/releases/

Installation and upgrade instructions: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/

If you are upgrading from a version of the chart that uses older Custom Resource Definitions (CRD) it is necessary to manually upgrade the CRDs as this is not managed by Helm.
To update to the latest version of the CRDs:
  $ kubectl apply -f https://raw.githubusercontent.com/nginxinc/kubernetes-ingress/v3.4.0/deploy/crds.yaml

More details on upgrading the CRDs: https://docs.nginx.com/nginx-ingress-controller/installation/installing-nic/installation-with-helm/#upgrading-the-crds

Before we make a larger change here, it will be interesting to see how this helm text change is receieved, and whether we get less issues related to helm upgrades and CRD management.

[vepatel]

just wanted to clarify a bit here, we do install CRDs by default based on the value of enable-custom-resources resource (currently defaults to true), what we don't do(manage) is upgrade or uninstall them.

[kyrofa]

Yes, thank you for the clarification @vepatel.

@j1m-ryan I noticed the PR adding that feature. That can only be helpful, of course, and I appreciate it! That said, as a personal anecdote, I manage my infrastructure with a combination of ansible and terraform, both of which will hide that kind of helpful output. Also, documenting a pain point doesn't always make it not a pain point, you know what I mean?

Anyway, I will hop on over to the discussion you started.

[shaun-nx]

Closing the issue as it was converted to a discussion here: https://github.com/nginxinc/kubernetes-ingress/discussions/5069