Skip to content

Support opt-in managing of CRDs by helm chart #5063

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kyrofa opened this issue Feb 9, 2024 · 5 comments
Closed

Support opt-in managing of CRDs by helm chart #5063

kyrofa opened this issue Feb 9, 2024 · 5 comments
Labels
proposal An issue that proposes a feature request

Comments

@kyrofa
Copy link

kyrofa commented Feb 9, 2024

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?

@kyrofa kyrofa added the proposal An issue that proposes a feature request label Feb 9, 2024
Copy link

github-actions bot commented Feb 9, 2024

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 🙂

Cheers!

@j1m-ryan
Copy link

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
Copy link
Contributor

vepatel commented Feb 12, 2024

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.

@shaun-nx shaun-nx added the waiting for response Waiting for author's response label Feb 12, 2024
@kyrofa
Copy link
Author

kyrofa commented Feb 12, 2024

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.

@j1m-ryan j1m-ryan removed the waiting for response Waiting for author's response label Feb 13, 2024
@shaun-nx
Copy link
Contributor

Closing the issue as it was converted to a discussion here: #5069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal An issue that proposes a feature request
Projects
None yet
Development

No branches or pull requests

4 participants