feat(installation): add helm support

[Xunzhuo]

Resolves: #650
Resolves: #479

xref: #1072

Signed-off-by: bitliu bitliu@tencent.com

[codecov-commenter]

Codecov Report

Merging #651 (6290f3b) into main (afe646b) will decrease coverage by 0.69%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
- Coverage   64.32%   63.63%   -0.69%     
==========================================
  Files          70       71       +1     
  Lines        9134     9220      +86     
==========================================
- Hits         5875     5867       -8     
- Misses       2855     2948      +93     
- Partials      404      405       +1     

Impacted Files	Coverage Δ
internal/provider/kubernetes/helpers.go	83.10% <0.00%> (-0.68%)	⬇️
internal/cmd/egctl/translate.go	67.57% <0.00%> (ø)
internal/cmd/egctl/config.go	10.52% <0.00%> (ø)
internal/provider/kubernetes/controller.go	49.88% <0.00%> (+0.33%)	⬆️
...rnal/infrastructure/kubernetes/proxy_deployment.go	91.07% <0.00%> (+1.78%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stevehipwell]

@Xunzhuo what release automation are you planning on using?

[Xunzhuo]

@Xunzhuo what release automation are you planning on using?

@stevehipwell I think we can use https://github.com/helm/chart-releaser.

[stevehipwell]

@Xunzhuo were you thinking of using this via GH actions? If so I'd suggest a pattern like is used by ExternalDNS. Basically chart changes can be added and tested at any point in the lifecycle, with the chart being released based on a change to Chart.yaml on the main branch.

[Xunzhuo]

Thank you @stevehipwell! I will take a look:)

[danehans]

cc: @bmetzdorf

[bmetzdorf]

Should this be templated?

[stevehipwell]

I'd suggest templating it from the chart app version field.

[bmetzdorf]

@Xunzhuo This was fast, wow!

I'm very new to this effort here. So, please accept my apologies for my ignorance.

My understanding is that we generate the current install.yaml via a combination of the "official" gatewayapi-crds.yaml and running kustomize in various directories under /internal.

My question is: How can we keep that existing mechanism and this helm chart code in sync? (It is not obvious to me how you created this helm chart). Can we use the existing mechanism to create the helm chart (e.g. convert install.yaml to a helm chart) or can we retire the old mechanism and generate install.yaml from the helm chart? But maybe I'm just missing something here..

[stevehipwell]

@bmetzdorf Helm charts tend to be hand crafted to some degree as they're effectively another application with a dependency on the primary application. You could generate your manifests with helm template and yq to remove Helm only content, but you'd likely still want to commit it into the repo.

[bmetzdorf]

@stevehipwell Committing into this repo here makes total sense. My question is: How can we make sure that the already existing yaml and the helm chart don't drift? Manual diffing seems cumbersome and error prone. Is there any tooling that can help?

[stevehipwell]

@bmetzdorf you could add an automated check as part of the chart change PR workflow (or maybe release workflow to allow parallel work) to compare the chart output with fixed values to the manifests. Logically the Helm chart would be based on the manifests, but able to support a wider target (e.g. K8s version detection for API versions) and extended functionality (e.g. ServiceMonitor).

[Xunzhuo]

Thanks for your comments @stevehipwell @bmetzdorf! Since this PR is still a WIP state, I will address these comments soon.

[stevehipwell]

@Xunzhuo if you let me know when you're done I can do a full review?

[Xunzhuo]

Sure, thanks @stevehipwell

[danehans]

We were considering dropping Kustomize. See #479 for additional details.

[Xunzhuo]

Main part of #1072, except refactoring github workflows/automation.

cc @arkodg @stevehipwell PTAL, thanks.

[arkodg]

thanks for rebasing @Xunzhuo ! I'm not a helm expert, @haq204 @AliceProxy can you help review this PR

[stevehipwell]

I haven't had a chance to review properly, I will have a detailed review on Monday, but a couple of things jumped out at me. Firstly CRDs should be in a crd folder in the root of the chart and should probably be manually curated to remove generation artifacts which shouldn't be included. Secondly why is there a generated folder for RBAC in the templates? And finally a question, is there any chart release automation or is this still pending?

[Alice-Lilith]

Overall I think it looks good, but I have a nit about the installation instructions namespace.

[Alice-Lilith]

Firstly CRDs should be in a crd folder in the root of the chart

@stevehipwell doing it this way with the CRDs is actually preferable since Helm will not update CRDs on a helm install if you let helm handle them in that way. I can confirm that putting them in another directory will enable upgrading of the CRDs on each helm upgrade which is likely going to be very important for us to do with new releases of the Gateway API otherwise helm will ignore any CRD diffs when upgrading if they already exist. This is a problem that Emissary ran into in the past so we decided just to separate our CRDs from the helm charts.

I understand helm's concerns for why they handle things this way, but from my perspective, we should manage this on our side and help provide users with a migration guide should Gateway API release breaking changes rather than let users upgrade assuming their CRDs are being kept up-to-date.

ref: https://helm.sh/docs/chart_best_practices/custom_resource_definitions/#method-1-let-helm-do-it-for-you

[stevehipwell]

@AliceProxy going to have to strongly disagree with you on this, "works" in this case is only if you squint and look to the side. If you want to manage CRDs with Helm (I have no idea why you would) a separate chart is idiomatic, either basic templates or job based.

I'm not going to rehash all of the arguments for this here, the Helm docs and a simple Google search should suffice. But as a case in point if the CRDs in are the chart (or directly linked to the chart) you will lose all resources if you uninstall, which for a stateless component makes zero sense.

[Xunzhuo]

Firstly CRDs should be in a crd folder in the root of the chart and should probably be manually curated to remove generation artifacts which shouldn't be included.

I think putting gwapi crds into crds folder in the root makes senses, because uninstalling eg may unexpectedly uninstall other existing gwapi CRs. It is done. But some generated CRDs from EG, and they are used in inner EG, I think we should manage them in charts and manage their life cycle along with EG itself. So I put them into generated folder.

Secondly why is there a generated folder for RBAC in the templates?

Rbac and crds are both generated by https://github.com/envoyproxy/gateway/blob/main/tools/make/kube.mk#L28

And finally a question, is there any chart release automation or is this still pending?

It will add after this PR.

[Alice-Lilith]

if the CRDs in are the chart (or directly linked to the chart) you will lose all resources if you uninstall, which for a stateless component makes zero sense.

Yes I think you're definitely right here. I didn't consider how the CRDs being removed on an uninstall would be very undesirable for use-cases such as multitenant environments or setups where you may be managing more than one instance of Envoy Gateway per-cluster and would absolutely not want all your CRDs getting deleted when you helm uninstall one instance. I agree that having a separate chart for the CRDs might be the way to handle it after realizing that.

If you want to manage CRDs with Helm (I have no idea why you would)

I don't personally mind the method I mentioned where the CRDs are not part of any chart and users are required to apply them directly from yaml, but over the course of the various Emissary versions we've received many requests from users to add helm support for the CRDs so that they can manage all their cluster config with tools like helm. The main driver for this request is that many users seem to be working in an environment where they are unable to apply yaml to the cluster from manifests either because of business restrictions or the tooling they use to manage cluster config.

While I think it seems strange that users would be unable to find a suitable workaround to that problem, it has been a point of friction that we frequently get nagged about by some of our larger enterprise users. As for putting the CRDs in helm's crds directory I still don't think that's a great idea. I've personally handled at least a dozen support tickets where a user has failed to upgrade their CRDs when doing a helm upgrade thinking that helm would also upgrade their CRDs since it installed them, so I'd just like to see EG avoid those two friction points if possible. IMO helm's behaviour with the crds directory leads to a lot of user confusion and people who aren't aware of how helm handles them ending up with out-of-date CRDs after a while and being confused. If you require users to get the CRDs from a yaml link then at least they learn on the first-install that they need to manually upgrade CRDs and should expect to do so when upgrading the application.

I think using the crds directory is better than deleting user config when they wouldn't want us to but I'd favour either making a new chart for the CRDs or making them only available via yaml although I don't think these are serious enough problems right now to prevent landing this PR if that's the way ya'll want to manage them.

[Xunzhuo]

Hey @AliceProxy, thanks for comments! I think putting them into crds/ is better to manage rather than a new chart.

For the generated EG CRDs, what do you think? Also putting them into crds/ or manage them along with EG itself?

[Xunzhuo]

For multi-tenant or multi-instance of EG, I think we should also put them into crds/.

I have put all crds into crds/.

[stevehipwell]

It's always best to follow idioms, especially with something as ubiquitous as Helm; I'd suspect that your many support requests are a vocal minority of actual users.

Chart CRDs should be in the crds folder but the chart should have visible docs about always managing CRDs externally to the chart and using --skip-crds unless for a quick POC. Remember that the crds dir is likely the place people will source the CRDs from to get the correct versions for the chart being installed (my automation uses this pattern). If you have end users who can't directly install YAML then you can publish a separate chart just to contain just the CRDs. External CRDs should be in their own separate CRDs chart if they also need supporting as a non-YAML installation.

[haq204]

lgtm