aws / aws-application-networking-k8s

A Kubernetes controller for Amazon VPC Lattice
https://www.gateway-api-controller.eks.aws.dev/
Apache License 2.0
175 stars 50 forks source link

Put CRD's into a separate Helm Chart #659

Closed xonstone closed 3 months ago

xonstone commented 3 months ago

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 https://github.com/aws/aws-application-networking-k8s/issues/658

zijun726911 commented 3 months ago

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: https://github.com/aws/aws-application-networking-k8s/issues/658#issuecomment-2258936391

xonstone commented 3 months ago

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 https://github.com/aws/aws-application-networking-k8s/issues/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.

graehren commented 3 months ago

@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 commented 3 months ago

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 https://github.com/aws/aws-application-networking-k8s/issues/660. That ticket mentions to error out sooner on startup so mistakes are caught during deployment instead of after it...

graehren commented 3 months ago

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

xonstone commented 3 months ago

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

DanielCastronovo commented 2 months ago

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