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

Prevent overriding environment variables with --envfile #5802

Closed
evandam opened this issue Sep 5, 2023 · 10 comments · Fixed by #5803
Closed

Prevent overriding environment variables with --envfile #5802

evandam opened this issue Sep 5, 2023 · 10 comments · Fixed by #5803
Labels
bug 🐞 Something isn't working

Comments

@evandam
Copy link
Contributor

evandam commented Sep 5, 2023

Hi folks,

I'm not sure if this is considered a bug or "expected behavior," but --envfile overwrites any existing environment variables, which seems like the opposite of other dotenv libraries such as https://github.com/bkeepers/dotenv.

Similarly, it seems like if godotenv was used instead of a custom parser, this logic would be the same: https://github.com/joho/godotenv#precedence--conventions

For example, I would expect FOO to be system, but instead Caddy reads .env here:

$ cat .env
FOO=.env

$ FOO=system caddy run --envfile .env --environ
...
FOO=.env
...

If this is expected behavior, would it be possible to get an optional flag to prevent this from happening?

For example, a common use case may be to have a Kubernetes ConfigMap mounted with environment-specific environment variables, but you still want to have a .env file to fallback on, if it makes sense.

I opened a PR if it's alright, looking forward to hearing your thoughts 🙌 #5803

Thanks in advance!

@mholt
Copy link
Member

mholt commented Sep 6, 2023

Thanks for opening an issue! (And PR.)

I don't use this flag myself, but if the convention for this file is to not overwrite existing values, then I suppose that's a good fix.

I wonder how many people are relying on the buggy behavior? 😶

@mholt mholt added the bug 🐞 Something isn't working label Sep 6, 2023
@evandam
Copy link
Contributor Author

evandam commented Sep 6, 2023

That was my one hesitation as well 😅

I can't imagine that many people relying on .env to override an env var since they're typically used (in my experience, at least) to supplement existing env vars. Famous last words, anyway.

Let me know if there's anything else I can help out with on my end, I'm not much of a Go developer but I can certainly try 🙌

@evandam
Copy link
Contributor Author

evandam commented Sep 8, 2023

Thanks for help on this one @mholt and @francislavoie! Not rushing at all, but I was just curious if you have a rough timeline for when we can expect the next release with this included?

@mholt
Copy link
Member

mholt commented Sep 8, 2023

Not sure yet, but you can always build from that commit if you'd like! 👍

@francislavoie
Copy link
Member

We don't really have any scheduled releases. We release when we feel we have enough for one or if there's a fix that's important enough to warrant immediate release. That said 2.7.5 should probably not be too far away. Maybe a couple weeks to a month from now 🤷‍♂️

@evandam
Copy link
Contributor Author

evandam commented Sep 8, 2023

Gotcha, looking forward to it! Appreciate all the work that goes into Caddy ❤️

@francislavoie
Copy link
Member

FYI reminder that you can build from master if you need it right away, especially easy to do with xcaddy. You can pin to a specific commit hash for now until there's a tagged release.

@hevisko
Copy link

hevisko commented Jan 24, 2024

okay, so I might've been the one user that RELIED on the --envfile to OVERWRITE existing env variables (like HOME and USER )

Situation/case in point:

using supervisor I start caddy in /etc/caddy and use the HOME (that is different from /etc/passwd for the www-data user) to force caddy's HOME to /etc/caddy so it can read/use /etc/caddy/ssl for reading/writing to the local

I've (elsewhere before) used the --envfile /etc/caddy/CaddyEnv to overwrite the HOME & USER, but now... it seems to NOT do it anymore (2.7.6) I believe I had it "working" somewhere before 2.6.4 using the envfile

So question: the PR loaded for 2.7.5/6 ?

How would/should/could I force Environment variable to be overwritten from --envfile ?

@hevisko
Copy link

hevisko commented Jan 24, 2024

reason for NOT doiing the ENV in supervisor, is that supervisor has quite a horrible syntax to add multiple environment variables for a command

@francislavoie
Copy link
Member

Overriding HOME is the wrong way to do that. See https://caddyserver.com/docs/conventions#data-directory you can set the XDG variables instead, or explicitly configure storage locations with the storage global option https://caddyserver.com/docs/caddyfile/options#storage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants