Allow podman to run in an environment with keys containing spaces #15434
Conversation
openshift-ci
bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@edsantiago I don't know how we can easily test this. Could I use python to test? Or do you have a better idea? $ python -c 'import os; os.environ["this is a test"]="foo"; os.execl("/bin/podman", "podman", "run", "alpine", "echo", "hi")' |
|
What's wrong with |
pkg/specgen/generate/container.go
Outdated
| @@ -140,7 +140,7 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat | |||
| } | |||
| // First transform the os env into a map. We need it for the labels later in | |||
| // any case. | |||
| osEnv, err := envLib.ParseSlice(os.Environ()) | |||
| osEnv, err := envLib.ParseSliceIgnoreErrors(os.Environ(), []error{envLib.ErrWhiteSpace}) | |||
There was a problem hiding this comment.
Why is this doing the parseEnv on os.Environ() at all?
os.Environ() is pretty much by definition correct:
- So we shouldn’t trim any whitespace (from neither names nor values) like
parseEnvdoes - So I don’t see why we should treat trailing
*specially.
Is there some later consumer that imposes such restrictions?
There was a problem hiding this comment.
Ok just translating from a slice of strings to a map[string]string.
There was a problem hiding this comment.
ACK, that looks reasonable.
(I didn’t read all of the code and other users of pkg/env.)
rhatdan
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
Fixes: containers#15251 Signed-off-by: Daniel J Walsh <[email protected]>
| @@ -58,6 +58,13 @@ var _ = Describe("Podman run", func() { | |||
| Expect(session).Should(Exit(0)) | |||
| Expect(session.OutputToString()).To(ContainSubstring("/bin")) | |||
|
|
|||
| // Verify environ keys with spaces do not blow up podman command | |||
| os.Setenv("FOO BAR", "BAZ") | |||
| session = podmanTest.Podman([]string{"run", "--rm", ALPINE, "true"}) | |||
There was a problem hiding this comment.
Why not go for a full-on all-the-way test?
...{"run", "--rm", "-e", "FOO BAR", ALPINE, "printenv", "FOO BAR"})
...output-should-equal("BAZ")(for extra credit, s/BAZ/BAZ OOKA or something with spaces/)
There was a problem hiding this comment.
I could do that but that would be a separate test. What I was looking at was a failure to run if any environment variable contained a space, even if the variable was not being passed to the container.
There was a problem hiding this comment.
I also did not see an easy way to do this in a system test.
env "foo bar"="foo baz" run_podman ...
Blew up.
There was a problem hiding this comment.
I don't see a sane way to test this in system tests either. Bash won't allow it. It would be possible to build a test binary that uses prctl() to inject FOO BAR into the parent process's environ, but let's just not go there.
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Signed-off-by: Daniel J Walsh [email protected]
Fixes: #15251
Does this PR introduce a user-facing change?