NotificationsYou must be signed in to change notification settings
Fork924
Star10.1k

feat(agent/agentcontainers): add ContainerEnvInfoer#16623

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

johnstcn merged 9 commits intomainfromcj/agent-docker-envinfo

Feb 24, 2025

Merged

feat(agent/agentcontainers): add ContainerEnvInfoer#16623

johnstcn merged 9 commits intomainfromcj/agent-docker-envinfo

Feb 24, 2025

Conversation

Copy link

Member

johnstcn commentedFeb 19, 2025•
edited
Loading

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.

feat(agent/agentcontainers): add ContainerEnvInfoer

8c357b4

johnstcn self-assigned this

Feb 19, 2025

ctud

e76af04

johnstcn requested review frommafredri,dannykopping andSasSwart

February 19, 2025 16:44

mafredri reviewed

Feb 19, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Only real issue I spotted is the env parsing. Both the premise (which I question) and also multiline handling. Otherwise looks solid.

agent/agentcontainers/containers_dockercli.go OutdatedShow resolvedHide resolved

agent/agentcontainers/containers_dockercli.goShow resolvedHide resolved

agent/agentcontainers/containers_dockercli.go OutdatedShow resolvedHide resolved

johnstcnand others added5 commits

February 19, 2025 17:57

Apply suggestions from code review

7fd24be

Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>

address mafcode review comments

5e57035

fix issue with docker homedir and env clobbering real env for cmd

d52d0c1

env from devcontainer metadata

467a6e8

re-add comment

06b9b7e

johnstcn mentioned this pull request

Feb 19, 2025

feat(agent): wire up agentssh server to allow exec into container#16638

Merged

dannykopping approved these changes

Feb 20, 2025

View reviewed changes

agent/agentcontainers/containers_dockercli.go Outdated

		}

		// EnvInfo returns information about the environment of a container.
		func EnvInfo(ctx context.Context, execer agentexec.Execer, container, containerUser string) (*ContainerEnvInfoer, error) {

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Where will this be called? Right now it's only used in tests.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

See here:#16638

agent/agentcontainers/containers_dockercli.go OutdatedShow resolvedHide resolved

agent/agentcontainers/containers_dockercli.go Outdated


		func (cei ContainerEnvInfoer) CurrentUser() (user.User, error) {
		// Clone the user so that the caller can't modify it
		u := *cei.user

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Nit: this is technically just a shallow clone, but it's OK becauseuser.User does not have any reference type fields:https://pkg.go.dev/os/user#User

agent/agentcontainers/containers_dockercli.go OutdatedShow resolvedHide resolved

agent/agentcontainers/containers_internal_test.go

		@@ -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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Tasty 👍

PR comments again

bf101d3

mafredri approved these changes

Feb 24, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Noticed one potential issue? Otherwise LGTM once those are fixed 👍🏻

agent/agentcontainers/containers_dockercli.go

Comment on lines +131 to +142

		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()
		}

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Are we intentionally usingos here now? IIRC these were accessingdei.user before. I'm guessing the env and home dir may be different between host and container.

Copy link

MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah I found that we still need to useos 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.

Copy link

Member

mafredriFeb 24, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 fordocker exec commands. We need to serialize the container env asdocker 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?

Copy link

MemberAuthor

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I suppose a better implementation would be to haveDockerEnvInfoer embedsystemEnvInfoer and just override the required methods. That way you keep the abstraction but it's clear then that this implementation does nothing different.

Copy link

MemberAuthor

johnstcnFeb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is going to need a little refactoring so I'll merge as-is and address this in a follow-up.

agent/agentcontainers/containers_dockercli.goShow resolvedHide resolved

strings.Builder

f06105a

johnstcn merged commit304007b intomain

Feb 24, 2025

30 checks passed

johnstcn deleted the cj/agent-docker-envinfo branch

February 24, 2025 15:05

github-actionsbot locked and limited conversation to collaborators

Feb 24, 2025

Labels

None yet

3 participants

Movatterモバイル変換

feat(agent/agentcontainers): add ContainerEnvInfoer#16623

feat(agent/agentcontainers): add ContainerEnvInfoer#16623

Uh oh!

Conversation

johnstcn commentedFeb 19, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredriFeb 24, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnstcn commentedFeb 19, 2025•
edited
Loading

mafredriFeb 24, 2025•
edited
Loading