feat: Make Cilium CLI work with Cilium installed through helm with a non-default name. #2430

matthewhembree · 2024-03-19T06:39:11Z

This update introduces a new parameter across several functions and actions to allow the user to specify a Helm release name for the Cilium installation. This enables referencing installations installed via Helm directly. The default Helm release name remains "cilium" when no custom release name is provided.

The new global flag proposed here is: --helm-release-name.

Fixes: #2274
Signed-off-by: Matthew Hembree 47449406+matthewhembree@users.noreply.github.com

install/install.go

sysdump/sysdump.go

matthewhembree · 2024-03-19T06:56:58Z

cli/clustermesh.go

note: No changes were made to clustermesh connect and clustermesh disconnect to support a Helm release name override for the remote cluster that would be different than the local cluster.

I felt that it would be best to take an opinionated stance that both clusters should be similarly configured. (i.e. use the same Helm release name) The constraint already exists for the namespace name. Deviation would probably create difficulty for the community that supports this project.

matthewhembree · 2024-03-19T07:11:41Z

Tested with a default installation and with a custom Helm release name installation.

The associated CLI sub-commands were tested across the two installations with and without the new flag.

Additionally, combination testing was done with sysdump specific flag --cilium-helm-release-name and the global flag --helm-release-name to validate the override priority. (i.e. more specific, if provided, overrides the global)

maintainer-s-little-helper · 2024-03-19T07:24:29Z

Commit 7ec19cb does not match "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

gandro

This is great, thank you so much!

I've left some minor inline comments.

A note on the git commits themselves:
Please do not use merge commits, use rebase instead.
Also, please line break your git commit description at ~80-100 character limit per line.

Otherwise this looks excellent!

gandro · 2024-03-19T09:28:14Z

cli/sysdump.go

@@ -34,6 +34,10 @@ func newCmdSysdump(hooks sysdump.Hooks) *cobra.Command {
 			if sysdumpOptions.CiliumNamespace == "" && cmd.Flags().Changed("namespace") {
 				sysdumpOptions.CiliumNamespace = namespace
 			}
+			// Honor --helm-release-name global flag in case it is set and --cilium-helm-release-name is not set
+			if sysdumpOptions.CiliumHelmReleaseName == "" && cmd.Flags().Changed("helm-release-name") {


Could we deprecate cilium-helm-release-name?

I was wondering where a change in this would break things. IMO, for a manual diagnostic tool I'd have a lower level of caution. But sysdump can be called by connectivity test, so there's three places to set the helmReleaseName.

Would the process be:

Open an issue noting the deprecation.

Put a // TODO: Deprecated by #nnnn on these lines.

install/install.go

sysdump/sysdump.go

hubble/hubble.go

matthewhembree · 2024-03-20T06:46:04Z

The EKS (tunnel) workflow works fine when run from my account/cluster. It could be failing due to spot interruptions; even though those instance types report <5% interruption rate. The logs show that a pod can't be found.

defaults/defaults.go

gandro · 2024-03-20T10:13:21Z

Adding @tklauser (one of the maintainers) as a reviewer to also get his eyes on this change, to make sure we're not missing anything

install/install.go

tklauser

LGTM, thanks!

tklauser · 2024-03-26T21:47:32Z

@matthewhembree it looks like this PR picked up a merge conflict. Could you please rebase your branch on latest main?

This update introduces a new parameter across several functions and actions to allow the user to specify a Helm release name for the Cilium installation. This enables referencing installations installed via Helm directly. The default Helm release name remains "cilium" when no custom release name is provided. Fixes: cilium#2274 Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

In this update, several log messages in the sysdump.go file have been refactored to provide a clearer output. This was achieved by introducing colons for namespace detections and consistently formatting namespace outputs across messages. A small change radius was maintained for this commit. There were other log messages that could have been updated, but left in place. Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

This is to allow `cilium hubble disable --helm-release-name foo` as missing from a previous/complimentary commit. Fixes: cilium#2274 Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

…ninstall. This is to allow `cilium uninstall --helm-release-name foo` to correctly uninstall Hubble before uninstalling the core Cilium. This includes a fix to specify the namespace. Fixes: cilium#2274 Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

…ium install|uninstall --wait. This is to correct a case where `cilium uninstall --wait` would be configured to wait for Hubble to uninstall(disable), but have an implied duration of 0. Causing a quasi-false error log message. The exit code is still 0, but it was misleading to see the error log in CI. Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

The code in the install.go file has been rearranged. Helm properties such as 'HelmRepository' and 'HelmReleaseName' have been moved to the top for better visibility, while 'ListVersions' and 'NodesWithoutCilium' have been moved to the bottom. The aim was to enhance code readability and organization. Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

The default Helm release name for Cilium, previously in "defaults .go", has been moved to "sysdump.go". This change was made to ensure that the default value is readily available in the same file where it's solely used. Now, the sysdump file locally hosts the private 'helmReleaseName' constant, which is directly assigned to 'CiliumHelmReleaseName' (cilium sysdump --cilium-helm-release-name) if it's not set by the user. Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

Removed unused Helm-related definitions in defaults.go file. Two constants, HelmValuesSecretKeyName and HelmChartVersionSecretKeyName, were unused and hence deleted. Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

tklauser · 2024-03-26T23:06:10Z

AKS workflow failed because of #2446 (comment), fixed in #2452. All other workflows passed. Merging.

matthewhembree temporarily deployed to ci March 19, 2024 06:39 — with GitHub Actions Inactive

matthewhembree commented Mar 19, 2024

View reviewed changes

install/install.go Outdated Show resolved Hide resolved

matthewhembree commented Mar 19, 2024

View reviewed changes

sysdump/sysdump.go Show resolved Hide resolved

matthewhembree commented Mar 19, 2024

View reviewed changes

matthewhembree marked this pull request as ready for review March 19, 2024 07:15

matthewhembree requested review from a team as code owners March 19, 2024 07:15

matthewhembree requested review from joamaki, asauber, gandro and YutaroHayakawa March 19, 2024 07:15

maintainer-s-little-helper bot added the dont-merge/needs-sign-off label Mar 19, 2024

matthewhembree temporarily deployed to ci March 19, 2024 07:24 — with GitHub Actions Inactive

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from 7ec19cb to 487c64b Compare March 19, 2024 07:29

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off label Mar 19, 2024

matthewhembree temporarily deployed to ci March 19, 2024 07:29 — with GitHub Actions Inactive

YutaroHayakawa approved these changes Mar 19, 2024

View reviewed changes

gandro requested changes Mar 19, 2024

View reviewed changes

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from 487c64b to c2238b9 Compare March 19, 2024 10:36

matthewhembree temporarily deployed to ci March 19, 2024 10:36 — with GitHub Actions Inactive

gandro approved these changes Mar 19, 2024

View reviewed changes

matthewhembree temporarily deployed to ci March 19, 2024 11:56 — with GitHub Actions Inactive

matthewhembree commented Mar 19, 2024

View reviewed changes

hubble/hubble.go Show resolved Hide resolved

gandro approved these changes Mar 19, 2024

View reviewed changes

matthewhembree temporarily deployed to ci March 20, 2024 07:08 — with GitHub Actions Inactive

matthewhembree commented Mar 20, 2024

View reviewed changes

defaults/defaults.go Show resolved Hide resolved

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from e6530c7 to 334a8ef Compare March 20, 2024 07:31

matthewhembree temporarily deployed to ci March 20, 2024 07:31 — with GitHub Actions Inactive

gandro requested a review from tklauser March 20, 2024 10:12

tklauser requested changes Mar 20, 2024

View reviewed changes

install/install.go Outdated Show resolved Hide resolved

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from 334a8ef to e6530c7 Compare March 20, 2024 21:12

matthewhembree temporarily deployed to ci March 20, 2024 21:12 — with GitHub Actions Inactive

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from e6530c7 to c720a89 Compare March 20, 2024 21:17

matthewhembree temporarily deployed to ci March 20, 2024 21:17 — with GitHub Actions Inactive

matthewhembree requested a review from tklauser March 20, 2024 21:27

tklauser approved these changes Mar 22, 2024

View reviewed changes

asauber approved these changes Mar 22, 2024

View reviewed changes

matthewhembree added 8 commits March 26, 2024 16:51

feat: Add helmReleaseName to hubble disable parameters.

0aead5c

This is to allow `cilium hubble disable --helm-release-name foo` as missing from a previous/complimentary commit. Fixes: cilium#2274 Signed-off-by: Matthew Hembree <47449406+matthewhembree@users.noreply.github.com>

matthewhembree force-pushed the pr/matthewhembree/add-helm-release-name-flag branch from c720a89 to b7f1c50 Compare March 26, 2024 21:52

matthewhembree temporarily deployed to ci March 26, 2024 21:52 — with GitHub Actions Inactive

tklauser merged commit fff4fce into cilium:main Mar 26, 2024
12 of 13 checks passed

BrewTestBot mentioned this pull request Mar 28, 2024

cilium-cli 0.16.4 Homebrew/homebrew-core#167416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make Cilium CLI work with Cilium installed through helm with a non-default name. #2430

feat: Make Cilium CLI work with Cilium installed through helm with a non-default name. #2430

matthewhembree commented Mar 19, 2024 •

edited

matthewhembree Mar 19, 2024

matthewhembree commented Mar 19, 2024

maintainer-s-little-helper bot commented Mar 19, 2024

gandro left a comment

gandro Mar 19, 2024

matthewhembree Mar 19, 2024 •

edited

matthewhembree commented Mar 20, 2024

gandro commented Mar 20, 2024

tklauser left a comment

tklauser commented Mar 26, 2024

tklauser commented Mar 26, 2024

feat: Make Cilium CLI work with Cilium installed through helm with a non-default name. #2430

feat: Make Cilium CLI work with Cilium installed through helm with a non-default name. #2430

Conversation

matthewhembree commented Mar 19, 2024 • edited

matthewhembree Mar 19, 2024

Choose a reason for hiding this comment

matthewhembree commented Mar 19, 2024

maintainer-s-little-helper bot commented Mar 19, 2024

gandro left a comment

Choose a reason for hiding this comment

gandro Mar 19, 2024

Choose a reason for hiding this comment

matthewhembree Mar 19, 2024 • edited

Choose a reason for hiding this comment

matthewhembree commented Mar 20, 2024

gandro commented Mar 20, 2024

tklauser left a comment

Choose a reason for hiding this comment

tklauser commented Mar 26, 2024

tklauser commented Mar 26, 2024

matthewhembree commented Mar 19, 2024 •

edited

matthewhembree Mar 19, 2024 •

edited