feat(agent/agentcontainers): add ContainerEnvInfoer #16623
Conversation
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>
| } | ||
|
|
||
| // EnvInfo returns information about the environment of a container. | ||
| func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) { |
There was a problem hiding this comment.
Where will this be called? Right now it's only used in tests.
|
|
||
| func (cei *ContainerEnvInfoer) CurrentUser() (*user.User, error) { | ||
| // Clone the user so that the caller can't modify it | ||
| u := *cei.user |
There was a problem hiding this comment.
Nit: this is technically just a shallow clone, but it's OK because user.User does not have any reference type fields: https://pkg.go.dev/os/user#User
| @@ -93,12 +105,93 @@ func TestDockerCLIContainerLister(t *testing.T) { | |||
| if assert.Len(t, foundContainer.Volumes, 1) { | |||
| assert.Equal(t, testTempDir, foundContainer.Volumes[testTempDir]) | |||
| } | |||
| // Test that EnvInfo is able to correctly modify a command to be | |||
| func (*DockerEnvInfoer) Environ() []string { | ||
| // Return a clone of the environment so that the caller can't modify it | ||
| return os.Environ() | ||
| } | ||
|
|
||
| func (*DockerEnvInfoer) UserHomeDir() (string, error) { | ||
| // We default the working directory of the command to the user's home | ||
| // directory. Since this came from inside the container, we cannot guarantee | ||
| // that this exists on the host. Return the "real" home directory of the user | ||
| // instead. | ||
| return os.UserHomeDir() | ||
| } |
There was a problem hiding this comment.
Are we intentionally using os here now? IIRC these were accessing dei.user before. I'm guessing the env and home dir may be different between host and container.
There was a problem hiding this comment.
Yeah I found that we still need to use os here otherwise you won't have e.g. $PATH set correctly, which will cause all kinds of issues. Given that both the implementations are doing the same thing now I could take it out of the equation, but it's also nice to have this abstraction in place IMO.
There was a problem hiding this comment.
Hmm, if that's the case, I suppose I don't understand how this is supposed to be used.
Essentially setting cmd.Env is no-op for docker exec commands. We need to serialize the container env as docker exec -e ENV1=val1 ... where this is needed (or an env file or some such).
In general I like the abstraction, but as used it seems confusing to me?
There was a problem hiding this comment.
I suppose a better implementation would be to have DockerEnvInfoer embed systemEnvInfoer and just override the required methods. That way you keep the abstraction but it's clear then that this implementation does nothing different.
There was a problem hiding this comment.
This is going to need a little refactoring so I'll merge as-is and address this in a follow-up.
This PR adds an alternative implementation of EnvInfo (#16603) that reads information from a running container.
NOTE: the new tests aren't run in CI by default; you'll have to just "trust" me that they work or run them yourself.