carvel-dev / vendir

Easy way to vendor portions of git repos, github releases, helm charts, docker image contents, etc. declaratively
https://carvel.dev/vendir
Apache License 2.0
279 stars 50 forks source link

vendir does not respect existing environment variables #164

Closed ggillies closed 2 years ago

ggillies commented 2 years ago

What steps did you take: Two scenarios 1) Setup environment/use asdf-vm as a package manager for tools used by vendir (helm, git, etc).

2) Setup specific git authentication setup inside CI job (e.g. export GIT_SSH_COMMAND="ssh -i $SSH_PRIVATE_KEY -o IdentitiesOnly=yes -o GlobalKnownHostsFile=$SSH_KNOWN_HOSTS") in which vendir will run, hoping it will use it This works if you make sure to specify no ssh auth options at all in vendir

What happened: 1) vendir is unable to use tools installed by asdf-vm 2) vendir ignores the GIT_SSH_COMMAND environment variable

What did you expect: As vendir is just calling specific tools underneath it, I would expect it to work with and respect the existing environment setup including all environment vairables

Anything else you would like to add: I believe this is due to vendir explicitly setting the environment for the exec.Command calls but not sure

Environment:


Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible" 👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

ggillies commented 2 years ago

I understand we can specify some Git auth options using a secret object and refencing it in vendir.yml, but this isn't the best solution for me as we have differing git auth setups for local workstatiosn VS ci. I would really like to just setup ssh/git outside of vendir and have it just work transparently with that.

cppforlife commented 2 years ago

@ggillies interesting, are you setting auth options which makes this code [0] set GIT_SSH_COMMAND itself.

https://github.com/vmware-tanzu/carvel-vendir/blob/develop/pkg/vendir/fetch/git/git.go#L91-L117

(if not, we do just propagate env variables to git.)

ggillies commented 2 years ago

@cppforlife ah thanks for that link, looking at the code I see if I remove all secret related stuff from vendir.yml it works as expected for the ssh auth options.

cppforlife commented 2 years ago

@ggillies is that an acceptable configuration or you were looking for some kind of combination of providing secret related stuff in vendir.yml and specifying additional configuration via env var?

ggillies commented 2 years ago

@cppforlife I think this is acceptable for my use case, however I have updated the issue description to make it clear there is an outstanding issue when using asdf-vm which I believe might also be related. However I am happy to change this issue if we feel this is wrong.

cppforlife commented 2 years ago

vendir is unable to use tools installed by asdf-vm

hmm, can you provide any additional detail. are you seeing an error, or something else?

ggillies commented 2 years ago

Apologies for not replying sooner.

Steps to reproduce

1) Install and setup asdf-vm 2) create a new empty directory 3) Run

asdf plugin add helm
asdf plugin add vendir
cat <<EOF >.tool-versions
vendir 0.27.0
helm 3.9.0
EOF
asdf install
cat <<EOF >vendir.yml
---
apiVersion: vendir.k14s.io/v1alpha1
kind: Config
directories:
  - path: vendor/charts/gitlab
    contents:
      - path: pre
        helmChart:
          name: gitlab
          version: "6.1.0"
          repository:
            url: https://charts.gitlab.io/
EOF
vendir sync

4) Note the following error

$ vendir sync
Fetching: vendor/charts/gitlab + pre (helm chart from https://charts.gitlab.io/@gitlab:6.1.0)

vendir: Error: Syncing directory 'vendor/charts/gitlab':
  Syncing directory 'pre' with helm chart contents:
    Add helm chart repository: exit status 1 (stderr: unknown command: helm. Perhaps you have to reshim?
)

The message about reshim comes from asdf, and typically indicates the shell environment (which asdf relies on heavily) has changed in some way to break it

ggillies commented 2 years ago

If I am reading https://github.com/vmware-tanzu/carvel-vendir/blob/aa2ac34a1f46b8664b1d9b4da05b969322f09cb1/pkg/vendir/fetch/helmchart/http_source.go#L46-L63 correctly, it looks like almost all environment variables are not preserved in the helm execution, including PATH? Is there a specific reason vendir does this?

cppforlife commented 2 years ago

correctly, it looks like almost all environment variables are not preserved in the helm execution, including PATH? Is there a specific reason vendir does this?

specifically for helm, yes, since we wanted to have a somewhat reproducible execution of helm binary. we may want to relax it to include HELM_* env vars though.

Add helm chart repository: exit status 1 (stderr: unknown command: helm. Perhaps you have to reshim?

this is an interesting error since go os.Exec uses vendir's PATH env var (not the env vars passed to the command) to look up comands on path, so im not sure why helm would not be found. how does your PATH env var look like?

cppforlife commented 2 years ago

(trying to repro...)

cppforlife commented 2 years ago

yup, looks like asdf relies on other env vars (which is somewhat surprising). let me relax the constraints here.

cppforlife commented 2 years ago

https://github.com/vmware-tanzu/carvel-vendir/releases/tag/v0.28.0 should have the fix. @ggillies if you dont mind double checking.

ggillies commented 2 years ago

@cppforlife thanks so much for taking a look at this! Unfortunately trying v0.28.0 still seems to have the same issue :(

$ cat .tool-versions 
helm 3.9.0
vendir 0.28.0

$ vendir version
vendir version 0.28.0

Succeeded

$ vendir sync
Fetching: vendor/charts/gitlab + pre (helm chart from https://charts.gitlab.io/@gitlab:6.1.0)

vendir: Error: Syncing directory 'vendor/charts/gitlab':
  Syncing directory 'pre' with helm chart contents:
    Add helm chart repository: exit status 1 (stderr: unknown command: helm. Perhaps you have to reshim?
)
cppforlife commented 2 years ago

hmm, im not sure how it was working yesterday when i tried it out with the fix... boiled it down to the problem with asdf not liking when HOME directory is changed (from where it was installed?).

$ HOME=/ helm version
unknown command: helm. Perhaps you have to reshim?

taking another shot... https://github.com/vmware-tanzu/carvel-vendir/pull/170

cppforlife commented 2 years ago

another run at this: https://github.com/vmware-tanzu/carvel-vendir/releases/tag/v0.28.1

cppforlife commented 2 years ago

@ggillies im going to assume it worked. feel free to reopen otherwise.

ggillies commented 2 years ago

@cppforlife sorry for not replying sooner, I was away from my computer for a few weeks. https://github.com/vmware-tanzu/carvel-vendir/pull/170 fixed the issue for me, thanks again!