Skip to content
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

Put CRD's into a separate Helm Chart #659

Closed
xonstone opened this issue Jul 30, 2024 · 7 comments
Closed

Put CRD's into a separate Helm Chart #659

xonstone opened this issue Jul 30, 2024 · 7 comments

Comments

@xonstone
Copy link

Currently the Helm Chart only installs/upgraded CRD's on the initial install of the Helm Chart. This is a well documented issue.

It would be nice to have a separate Helm Chart for the CRD's as described here so we can upgrade these easily along with the controller and avoid issues like #658

@zijun726911
Copy link
Contributor

By the v1.0.6 controller design, even if the TLSRoute CRD in a separate Helm Chart, but you don't install that separate Helm Chart, the controller won't start successfully. The controller full functionality rely on TLSRoute and some other CRDs installed in the cluster.

What do you think the approach mentioned in this issue: #658 (comment)

@xonstone
Copy link
Author

xonstone commented Jul 30, 2024

The separate Helm Chart will help to install the necessary CRD's since if you keep your current Helm Chart (with the CRD's in the crd folder) the CRD's only get installed at the initial helm install.

Once the Helm Chart is installed and you use helm upgrade there won't be any additional CRD's installed and that may cause issues like #658.

This is a well-documented behaviour of Helm and they suggest managing the CRD's manually or creating a separate Helm Chart with the CRD's as regular templates not in the CRD folder (which is what I am suggesting to do)

I would opt for the (additional) Helm Chart since you can then automate upgrades and use semver on the Helm Chart to depict breaking changes.

If not, it should at least be documented that you need to upgrade the CRD's manually yourself and have the requirements mentioned when the controller needs a certain additional CRD.

@xonstone xonstone changed the title Put CRD's into a seperate Helm Chart Put CRD's into a separate Helm Chart Jul 30, 2024
@graehren
Copy link
Collaborator

@xonstone We appreciate your feedback and have mentioned in the v1.0.6 release notes that customers need to install the Helm chart again or add the new TLSRoute CRD manually. For future EKS Controller enhancements we can add CRD's as an optional upgrade using a separate Helm chart, but we do not plan to move the TLSRoute CRD to make it optional as the code for TLS Passthrough support is coupled with the support for target groups.

We will update our development guidelines to use an optional Helm chart for upcoming feature enhancements or use Semver (1.x) for feature releases. Does this address your issue?

@xonstone
Copy link
Author

That seems like a good approach to me! Typically both Helm Charts would have the same version so it's easy to track what version should be installed (like eg Karpenter does it as well).

In addition I would also look into #660. That ticket mentions to error out sooner on startup so mistakes are caught during deployment instead of after it...

@graehren
Copy link
Collaborator

Thank you, #660 is a great suggestion for the controller.

@xonstone
Copy link
Author

Let’s keep this issue open until the separate Helm Chart for CRD’s is implemented.

@DanielCastronovo
Copy link

Any news ? the 1.0.6 has been released, so it should be interesting to have 2 helm charts :

  • gateway controller
  • crds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants