implement helm get values with sysdump

[liyihuang]

include the helm get values in the sysdump collecting, and it will try to grab the user provide value and write to a file. when users provides a sysdump file, we don't have to ask the user to submit a separate helm value file.

in this implementation, we just grab the latest value and user provide values for cilium. in the future, there can be more enhancement for this.

1. we can provide more options to collect previous version or the rendered helm values for everything.

btw, this is my first golang/OSS PR. feel free to let me know what I can do to make this better

[maintainer-s-little-helper]

Commit bfc2d79 does not contain "Signed-off-by".

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

[asauber]

This looks great overall. I have a few comments regarding names and error messages.

Please re-request my review if you end up waiting on me!

[mtt0]

If we want hornor --cilium-namespace sysdump flag ranther than the global flag -n, workaround:

Add:

helmDriver := ""
logger := func(format string, v ...interface{}) {}
k8sClient.RESTClientGetter.Namespace = sysdumpOptions.CiliumNamespace
if err := k8sClient.HelmActionConfig.Init(&k8sClient.RESTClientGetter, sysdumpOptions.CiliumNamespace, helmDriver, logger); err != nil {
  return fmt.Errorf("failed to init helm action config: %w", err)
}

below

cilium-cli/internal/cli/cmd/sysdump.go

Lines 32 to 35 in ccc2fcb

	// Honor --namespace global flag in case it is set and --cilium-namespace is not set
	if sysdumpOptions.CiliumNamespace == "" && cmd.Flags().Changed("namespace") {
	sysdumpOptions.CiliumNamespace = namespace
	}

• If -n present and --cilium-namespace not, then -n tack effect
• If --cilium-namespace present, then --cilium-namespace tack effect and override -n （k8sClient.RESTClientGetter.Namespace and k8sClient.HelmActionConfig changed)

Not sure if there are other side effects, whether it will affect other commands, because I know nothing about cilium-cli, sorry😣

[liyihuang]

If we want hornor --cilium-namespace sysdump flag ranther than the global flag -n, workaround:

Add:

helmDriver := ""
logger := func(format string, v ...interface{}) {}
k8sClient.RESTClientGetter.Namespace = sysdumpOptions.CiliumNamespace
if err := k8sClient.HelmActionConfig.Init(&k8sClient.RESTClientGetter, sysdumpOptions.CiliumNamespace, helmDriver, logger); err != nil {
  return fmt.Errorf("failed to init helm action config: %w", err)
}

below

cilium-cli/internal/cli/cmd/sysdump.go

Lines 32 to 35 in ccc2fcb

	// Honor --namespace global flag in case it is set and --cilium-namespace is not set
	if sysdumpOptions.CiliumNamespace == "" && cmd.Flags().Changed("namespace") {
	sysdumpOptions.CiliumNamespace = namespace
	}

• If -n present and --cilium-namespace not, then -n tack effect
• If --cilium-namespace present, then --cilium-namespace tack effect and override -n （k8sClient.RESTClientGetter.Namespace and k8sClient.HelmActionConfig changed)

Not sure if there are other side effects, whether it will affect other commands, because I know nothing about cilium-cli, sorry😣

thanks for the reply.

I don't quite understand why we need init the NS twice here and I see we dont need to worry about NS in the RESTClientGetter here

cilium-cli/k8s/client.go

Lines 125 to 130 in b98ada8

	helmDriver := ""
	actionConfig := action.Configuration{}
	logger := func(format string, v ...interface{}) {}
	if err := actionConfig.Init(&restClientGetter, ciliumNamespace, helmDriver, logger); err != nil {
	return nil, err
	}

By the way, RESTClientGetter is the interface, and I don't think I can access the underlying struct directly. It seems like I may need to initialize another Kubernetes client specifically for sysdump to handle the --cilium-namespace flag. However, if we need to support --tetragon-namespace in the future, this approach could become problematic.

[mtt0]

Oh...this project is using the vendor directory, so RESTClientGetter is indeed an interface...

We need switch kubeClient namespace as well. Because in action.Init(), kubeClient comes from getter:

https://github.com/helm/helm/blob/37cc2fa5cefb7f5bb97905b09a2a19b8c05c989f/pkg/action/action.go#L370

See:

• Wrong namespace in action.Install helm/helm#9256
• Helm v3 go library can't reuse same history, install, and upgrade clients for multiple namespaces helm/helm#8269 (comment)
  for details @liyihuang

[asauber]

Looks great for the most part.

Can you squash the commits down to 1 or 2 commits? Maybe with supporting the additional namespace flag as the second commit.

It's also important that the CLI is fully usable after each commit, which might be one reason to squash down to 1.

[asauber]

One style comment, otherwise LGTM

[sayboras]

LGTM ✅