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.

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

helm-template function fails with .kube/config #951

Closed
Liujingfang1 opened this issue Aug 17, 2020 · 13 comments · Fixed by kubernetes-sigs/kustomize#2900
Closed

helm-template function fails with .kube/config #951

Liujingfang1 opened this issue Aug 17, 2020 · 13 comments · Fixed by kubernetes-sigs/kustomize#2900
Assignees
Labels
area/fn-catalog Functions Catalog size/S 1 day triaged Issue has been triaged by adding an `area/` label

Comments

@Liujingfang1
Copy link
Contributor

Running the helm-template function

kpt pkg get https://github.com/GoogleContainerTools/kpt-functions-catalog.git/examples/helm-template .
kpt fn run helm-template/local-configs --mount type=bind,src=$(pwd)/helloworld-chart,dst=/source

with the error

[ERROR] Helm template command results in error: W0817 23:32:40.608720      12 loader.go:223] Config not found: /usr/local/google/home/jingfangliu/.kube/config

Error: exit status 1

Unsetting the environment variable by unset KUBECONFIG solves this error, but we should fix the problem from the helm-template image side.

@Liujingfang1
Copy link
Contributor Author

cc @prachirp @Shell32-Natsu

@prachirp
Copy link
Contributor

@Shell32-Natsu This issue of helm not finding KUBECONFIG is similar to #879 where kustomize could not find TMPDIR

@Shell32-Natsu
Copy link
Contributor

@prachirp You mean we should ignore KUBECONFIG env?

@Shell32-Natsu
Copy link
Contributor

TMPDIR is implicitly propagated by macOS but obviously KUBECONFIG isn't. I think what makes more sense might be check is path specified by KUBECONFIG is valid and unset it if it isn't in the function.

@prachirp
Copy link
Contributor

@Shell32-Natsu kpt has no way of knowing whether any env variable is used in the function so we need more general logic.

How about we expose a --kubeconfig=/x/y/z flag in kpt fn run which takes a path to the kube config as argument, and if no arguments are passed it default uses the local KUBECONFIG env variable. If --kubeconfig flag is not passed then we ignore KUBECONFIG env.

@Shell32-Natsu
Copy link
Contributor

I don't think add a flag for single env is a good idea, otherwise we will have dozens of flags in the future. I am think about using the same way that docker is using. Add a -e, --env flag to ask user to explicitly export env variable instead of blindly export all envs.

@frankfarzan Any input?

@prachirp
Copy link
Contributor

That makes sense to me. Now is a good time to make this change since we haven't released kpt v1.0.0 yet. As the biggest drawback is this is backwards-incompatible; it involves changing from passing all local env variables to docker to passing none by default.

@mortent mortent added area/fn-catalog Functions Catalog triaged Issue has been triaged by adding an `area/` label labels Aug 18, 2020
@mortent mortent added this to Backlog in Config as Data - Milestone 3 via automation Aug 18, 2020
@seans3 seans3 closed this as completed Aug 18, 2020
@seans3
Copy link
Contributor

seans3 commented Aug 18, 2020

That makes sense to me. Now is a good time to make this change since we haven't released kpt v1.0.0 yet. As the biggest drawback is this is backwards-incompatible; it involves changing from passing all local env variables to docker to passing none by default.

I think it might make sense to have this work the same way kubectl works. Users will be familiar with that. Is this possible to always propagate the env var (KUBECONFIG) into kpt fn run (in fact into all kpt commands)?

The ordering for finding kubeconfig in kubectl is:

  1. --kubeconfig flag
  2. KUBECONFIG env var. NOTE: This can have multiple kubeconfig files separated by colon which are merged
  3. Default location for Unix: ~/.kube/config, Windows (forgot)

Also, values in the kubeconfig can be overriden (e.g --server).

See: https://ahmet.im/blog/mastering-kubeconfig/

@seans3 seans3 reopened this Aug 18, 2020
@seans3
Copy link
Contributor

seans3 commented Aug 18, 2020

Sorry: accidentally closed this. I've re-opened.

@Shell32-Natsu
Copy link
Contributor

Thanks @seans3 , the main problem in this issue is KUBECONFIG is exported to the function accidentally and the path/file doesn't exist in the container. If we always export, it's up to user to make sure that the path specified by KUBECONFIG does exist in the container, otherwise they will have issue (like this).

At the same time I still think changing to passing no env by default is better than passing all because currently user has no way to control which env should or should not be passed.

Another thought is not all functions are using kubectl, is it worthwhile to add a flag for this? I prefer the method used in istioctl_analyze function. Flags to istioctl are passed as data and converted to flags by TS wrapper.

@frankfarzan
Copy link
Contributor

I think automatically exporting all host env vars to the container will be problematic in general. Setting env variables should be done explicitly. @Shell32-Natsu This should be tracked in the discussion of revisiting how docker flags are exposed in kpt.

@Shell32-Natsu
Copy link
Contributor

We still haven't had a final decision about the flag name, reopen this.

@Shell32-Natsu
Copy link
Contributor

closed by kubernetes-sigs/kustomize#2988

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fn-catalog Functions Catalog size/S 1 day triaged Issue has been triaged by adding an `area/` label
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants