Use helm charts to generate manifests

[aanm]

Instead of hard-coding all manifests into Cilium-cli, we should use the helm tgz available from the official Cilium chart repository. This will prevent files from being out-of-sync as well as prevent developer's confusion on which installation method it should be used.

Note to reviewers: there is some future work that can be done which is to store the helm "set" options into Cilium's ConfigMap (edit: implemented in this PR). With this, cilium-cli will know the helm settings used to deploy the Cilium which will allow commands such as cilium hubble enable without requiring to pass --set with all the helm options. Currently, commands such as cilium hubble enable are still using hardcoded manifests (edit: Still not done).

Fixes #256

[aditighag]

🚀

Fixes #256

(Had some issues checking out the PR, and browser complains about 1k files changed in the PR. )

For cgroup2 fs mounting, we need to add version checks. You can use this PR as a reference - https://github.com/cilium/cilium-cli/pull/627/files.

[aanm]

rocket

Fixes #256

(Had some issues checking out the PR, and browser complains about 1k files changed in the PR. )

For cgroup2 fs mounting, we need to add version checks. You can use this PR as a reference - https://github.com/cilium/cilium-cli/pull/627/files.

@aditighag why? Isn't that logic already versioned in the helm chart?

[aditighag]

Isn't that logic already versioned in the helm chart?

Version checks are not required in the cilium oss helm charts because we added the flags in the same version as the program that mounts cgroup2 fs.

[aanm]

Isn't that logic already versioned in the helm chart?

Version checks are not required in the cilium oss helm charts because we added the flags in the same version as the program that mounts cgroup2 fs.

@aditighag and the same is enforced in the cilium-cli with this PR. If the user installs version 1.x.y the helm chart that will be used is 1.x.y

[ti-mo]

This change is incredibly large, so I can't really tell for sure, but it feels like this moves the CLI towards a model where it's mostly a 'thin' Helm wrapper. Will this impact our ability to perform 'smart' logic based on certain properties of the target cluster? What about the maintenance cost of making sure the CLI stays compatible with past and future charts? This effectively turns the Helm values file into an API contract, unless the intention was to put that burden on the user by introducing --helm-set.

I'm not sure if the mandatory use of --helm-set is a net positive for user experience. It's great that we're converging on a single manifest format to reduce user confusion (though the initial plan was to deprecate Helm in favor of the CLI..), but now we're introducing the concept of some flags being direct Helm values and some being CLI args, which could be equally confusing.

The nice thing about having flags declared by the CLI itself was that --help was never far away (as opposed to having to browse the chart reference), the UI was simple, and the CLI could perform input validation. What's the reason for deprecating the flags instead of mapping them onto their corresponding Helm values (as stated above, I assume ease of maintenance)? And how did you decide on which ones to deprecate (because not all flags are being deprecated)?

[aanm]

This change is incredibly large, so I can't really tell for sure, but it feels like this moves the CLI towards a model where it's mostly a 'thin' Helm wrapper. Will this impact our ability to perform 'smart' logic based on certain properties of the target cluster?

Not really, that logic still lives inside cilium-cli. This PR is simply removing the duplicated logic and manifests that already existed in helm and use that instead to generate them.

What about the maintenance cost of making sure the CLI stays compatible with past and future charts? This effectively turns the Helm values file into an API contract, unless the intention was to put that burden on the user by introducing --helm-set.

The CLI is still in experimental stage. Whatever compatibility we offer with helm will be used in the CLI

I'm not sure if the mandatory use of --helm-set is a net positive for user experience. It's great that we're converging on a single manifest format to reduce user confusion (though the initial plan was to deprecate Helm in favor of the CLI..), but now we're introducing the concept of some flags being direct Helm values and some being CLI args, which could be equally confusing.

We briefly discussed that in the community meeting and it doesn't makes sense to keep the CLI flags if only half of them exist based on developer demand. It also doesn't make sense to keep a 1:1 mapping of a CLI flag to an existing helm flag. The flags are now marked as deprecated and the order of priority is: --helm-set can overwrite any option set by the CLI which can overwrite any default helm option.

The nice thing about having flags declared by the CLI itself was that --help was never far away (as opposed to having to browse the chart reference), the UI was simple, and the CLI could perform input validation. What's the reason for deprecating the flags instead of mapping them onto their corresponding Helm values (as stated above, I assume ease of maintenance)?

Explained above.

And how did you decide on which ones to deprecate (because not all flags are being deprecated)?

I've deprecated all flags that have a 1:1 mapping to the helm flag. There are certain CLI flags that have a 1 to many helm options. I've left those for now.

[nbusseneau]

I am OK with the PR in its current state. We still have work to do and I largely share the same concerns as already expressed (re: user-side confusion, ease of use) but we can iterate on that.

[chancez]

We briefly discussed that in the community meeting and it doesn't makes sense to keep the CLI flags if only half of them exist based on developer demand. It also doesn't make sense to keep a 1:1 mapping of a CLI flag to an existing helm flag. The flags are now marked as deprecated and the order of priority is: --helm-set can overwrite any option set by the CLI which can overwrite any default helm option.

I don't agree that having a 1:1 mapping of CLI flag to helm flags is bad. Flags in cilium-cli are discoverable, and provide help text, and validation that flags via --helm-set do not.

I also agree with @ti-mo overall pushing users towards --helm-set is making the user experience worse, and the cost of additional flags is probably not large, as we can mostly just pass them through.

[tklauser]

To me it’s still not clear whether all concerns re. deprecating flags have been addressed. Unfortunately, the discussion is split across this PR, the 2022-03-09 community meeting and a private slack thread and I’ve lost track a bit. How about we leave the deprecation of the flags out of this PR for now and open a separate PR for that, so it can be reviewed and discussed more easily?

Regarding the above point: While testing again, I also noticed that we use some of the deprecated flags in our v1.10 and v1.11 getting started guides (e.g. --azure-resource-group-name for the GSG on AKS and --config in the external workloads, IPSec and performance tuning guides). We suggest to install the latest stable cilium-cli version (i.e. https://github.com/cilium/cilium-cli/releases/latest/download/cilium-linux-amd64.tar.gz), so once we release a new cilium-cli version with this PR in, users following the cilium v1.10/v1.11 guides might get deprecation warnings which I think is not optimal in terms of UX.

[aanm]

@tklauser I've dropped the last 2 commits from the PR. I'll create a new PR with them once this is merged. Thanks

[tklauser]

The commits deprecating the existing flags have been moved out of this PR. Several reviewers approved and judging from the review comments and offline discussion we seem to agree on the remaining changes. Thus merging this PR, thanks a lot for your patience @aanm!