Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ensure kubeclient ns is release ns for helm actions #2652

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ensure kubeclient ns is release ns for helm actions #2652

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions internal/dao/helm_chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ func (h *HelmChart) Delete(_ context.Context, path string, _ *metav1.DeletionPro
func (h *HelmChart) Uninstall(path string, keepHist bool) error {
ns, n := client.Namespaced(path)
flags := h.Client().Config().Flags()
flags.Namespace = &ns
cfg, err := ensureHelmConfig(flags, ns)
if err != nil {
return err
Expand All @@ -145,8 +144,21 @@ func (h *HelmChart) Uninstall(path string, keepHist bool) error {

// ensureHelmConfig return a new configuration.
func ensureHelmConfig(flags *genericclioptions.ConfigFlags, ns string) (*action.Configuration, error) {
settings := &genericclioptions.ConfigFlags{
thorbenbelow marked this conversation as resolved.
Show resolved Hide resolved
Namespace: &ns,
Context: flags.Context,
BearerToken: flags.BearerToken,
APIServer: flags.APIServer,
CAFile: flags.CAFile,
KubeConfig: flags.KubeConfig,
Impersonate: flags.Impersonate,
Insecure: flags.Insecure,
TLSServerName: flags.TLSServerName,
ImpersonateGroup: flags.ImpersonateGroup,
WrapConfigFn: flags.WrapConfigFn,
}
cfg := new(action.Configuration)
Comment on lines 146 to 160
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @derailed, unfortunately I'm not too familiar with k9s' and Helms' codebase, so I've just been trying to orient myself on how helm constructs their client.

Just to be clear: You are suggesting to do this instead?

Suggested change
func ensureHelmConfig(flags *genericclioptions.ConfigFlags, ns string) (*action.Configuration, error) {
settings := &genericclioptions.ConfigFlags{
Namespace: &ns,
Context: flags.Context,
BearerToken: flags.BearerToken,
APIServer: flags.APIServer,
CAFile: flags.CAFile,
KubeConfig: flags.KubeConfig,
Impersonate: flags.Impersonate,
Insecure: flags.Insecure,
TLSServerName: flags.TLSServerName,
ImpersonateGroup: flags.ImpersonateGroup,
WrapConfigFn: flags.WrapConfigFn,
}
cfg := new(action.Configuration)
func ensureHelmConfig(flags *genericclioptions.ConfigFlags, ns string) (*action.Configuration, error) {
settings := &genericclioptions.ConfigFlags{
Namespace: &ns,
}
cfg := new(action.Configuration)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thorbenbelow nw Thank you Thorben! right I think flags.Namespace = &ns should be all we need.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. However, in this case usePersistentConfig must be false. Would you still prefer that over copying the config for helm operations?

There is also a PR in helm that should fix this issue as well. Not sure if that gets merged any time soon though.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thorbenbelow Thank you Thorben! I think the core issue here is helm does not use the passed in ns and create their own api client via a factory. Let's keep your original impl for the time being and make a copy of the flags prior to calling the helm api which I think is the safest course of action.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late reply. I squashed my changes and rebased.
Do we need tests for this or is there anything else needed to make this PR mergeable?

err := cfg.Init(flags, ns, os.Getenv("HELM_DRIVER"), helmLogger)
err := cfg.Init(settings, ns, os.Getenv("HELM_DRIVER"), helmLogger)

return cfg, err
}
Expand Down