Setup environment for processes with os.Environ()

[dqminh]

• genericError constructor should not panic when we pass in nil error, but we should not pass nil error actually. This fixes it.
• Instead of taking the raw config.Env, we use os.Environ() when execve which works correctly because the environment has been setup properly with newContainerInit

[LK4D4]

@dqminh Some error on jenkins. Looks like easy to fix.

[LK4D4]

@dqminh Btw very good catch about genericError - panics all over code :)

[LK4D4]

ping @jfrazelle
Pls take a look when you'll have time :)

[jessfraz]

I rebuilt ;)

[LK4D4]

@jfrazelle Thank you very much!

[LK4D4]

LGTM

[rjnagal]

LGTM

[rjnagal]

@dqminh Can you squash some of the commits?

[dqminh]

@rjnagal which commits do you want me to squash. The two commits that set os.Environ() for both setns and init process ?

[rjnagal]

yes, the last two.

[crosbymichael]

Why do we need to do this? why is config.Env not enough?

[LK4D4]

@crosbymichael There is no PATH and HOME. Tests in docker run and exec failing without this. Do you think that tests are wrong?

[crosbymichael]

why is there no PATH and home? docker prepopulates both of these either in the image or there is a hard coded default for PATH

[LK4D4]

I don't know actually :/
https://github.com/docker/docker/blob/master/integration-cli/docker_cli_run_test.go#L843

[dqminh]

@crosbymichael @LK4D4 i think the reason there's no PATH or HOME is because we set HOME / PATH with Setenv, but exec the process with config.Env which doesnt include everything. Also using os.Environ is better because it also removes the duplicated environment variables, you can see this in the test.

[crosbymichael]

I really don't think we should do this because it will cause env vars to leak into the container. The config.Env should have everything that the container's process needs, if it does not, I think it's a bug.

I'm just scared by doing this we will get a custom PATH that the user specified overwritten.

[LK4D4]

I'm not sure about PATH, but "HOME" we set indeed before Execv in setupUser by os.Setenv, should we pass append(l.config.Env, os.Getenv("HOME")?

[mrunalp]

I agree with @crosbymichael . We should be able to modify config.Env ourselves. @LK4D4 I think that should work.

[LK4D4]

At least we need fix for genericError :)

[mrunalp]

@LK4D4 agree :) @dqminh Maybe split that off into a separate PR?

[dqminh]

agreed. I will remove the os.Environ() commit and change this into genericError patch.

[dqminh]

@LK4D4 @mrunalp the genericError patch is here #424