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

translate env vars if found within file string #168

Closed
wants to merge 1 commit into from

Conversation

jmclean-starburst
Copy link

@jmclean-starburst jmclean-starburst commented Nov 22, 2021

What this PR does / why we need it:
ArgoCD will pass in a values file that is escaped; this means that any env variables that may exist within the argocd-repo-server cannot be used. In our case, rather than adding the token in clear text to the values.yaml file (remote fetch), we can leverage an environment variable (such as $GITHUB_TOKEN) if it is already existing within that env

Example:
if you supply the string secrets://https://$GITHUB_TOKEN@raw.githubusercontent.com/org/repo/ref/pathtofile.yml to the Argo values files array and the secret GITHUB_TOKEN exists within the argocd-repo-server, it will be injected into the URL.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@jmclean-starburst
Copy link
Author

Not sure why linting is erroring, would love some feedback

@jkroepke
Copy link
Owner

jkroepke commented Nov 22, 2021

Not sure why linting is erroring

The diff explains me that shfmt expects just one space between code and comment.

In general, I don't know if I like to use eval there. Something like https://$(command)/ would also evaluate and executed. This may lead to unwanted code executions. -> https://mywiki.wooledge.org/BashFAQ/048

Normally, I would prefer to use envsubst here. It just substitute the variables and ignore sub shell syntax like $().

On top, it looks like, eval is not compatible with windows paths and other wired path names.

@jmclean-starburst
Copy link
Author

one "gotcha" here that may break folks, is I had to add gettext-base to the installer for the ArgoCD custom image. updated with your recommendation to use envsubst

@jkroepke
Copy link
Owner

I agree. This is breaky.

I looked for alternatives and found this one: https://stackoverflow.com/a/40167919 It still based on shell builtin eval, but it does some sanity checks.

It still feels a little bit risky for me. I'll add this behind a toggle flag. User that want this feature can enable this. Checkout #169 for the code. It's not done yet and tests are missing for that case.

@jkroepke
Copy link
Owner

Closer in favor of #169

@jkroepke jkroepke closed this Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants