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

Make exec checks consistently not inherit parent environment #127

Closed
benhoyt opened this issue Jul 22, 2022 · 1 comment · Fixed by #131
Closed

Make exec checks consistently not inherit parent environment #127

benhoyt opened this issue Jul 22, 2022 · 1 comment · Fixed by #131

Comments

@benhoyt
Copy link
Contributor

benhoyt commented Jul 22, 2022

Currently exec checks inherit the parent/Pebble environment if the check configuration has no environment variables set, but does not inherit it if there are one or more environment fields. This is inconsistent and unexpected -- an artifact of how we're setting up exec.Command.Env, which inherits os.Environ() if nil.

I'm not sure whether checkers should inherit the parent environment or not.

Services do inherit, and one-shot commands do not. I think both of those are by design. I remember discussing with Gustavo that one-shot commands probably should not inherit. But we need to decide whether or not checkers should inherit the environment. I suspect it'll be less of a breaking change if we have them inherit, because most people probably aren't setting environment variables for checkers. Are there any security or other concerns with doing so?

Either way we should be consistent, and not have inheritance depend on whether or not there are any environment variables in the config.

See also #125. CC @hpidcock @jnsgruk.
@benhoyt
Copy link
Contributor Author

benhoyt commented Jul 29, 2022

Per discussion with @hpidcock, we're going to go the same way as one-shot commands here and not inherit the parent environment (in both of these cases).

@benhoyt benhoyt changed the title Make exec checks consistently (not) inherit parent environment Make exec checks consistently not inherit parent environment Jul 29, 2022
benhoyt added a commit to benhoyt/pebble that referenced this issue Jul 29, 2022
@benhoyt
Make exec checks consistently not inherit parent environment variables
b20bcea 
Currently it inherits them if you don't have any environment variables
set in the config, but does not inherit them if you have one or more
set. This is a quirk of the implementation, and how the
exec.Command.Env field works. Make them not inherited in both cases to
be consistent.

Fixes canonical#127
@benhoyt benhoyt mentioned this issue Jul 29, 2022
Merged
benhoyt added a commit that referenced this issue Jul 29, 2022
@benhoyt
Make exec checks consistently not inherit parent environment variables (
d9b6b8c 
#131)

Currently it inherits them if you don't have any environment variables
set in the config, but does not inherit them if you have one or more
set. This is a quirk of the implementation, and how the
exec.Command.Env field works. Make them not inherited in both cases to
be consistent.

Fixes #127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make exec checks consistently not inherit parent environment variables benhoyt/pebble
1 participant
@benhoyt