- Notifications
You must be signed in to change notification settings - Fork1k
feat(agent): wire up agentssh server to allow exec into container#16638
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
d096ac4 to56cb9e9Compare56cb9e9 todb9df4bComparedb9df4b to36707f9Compare| // EnvInfoer encapsulates external information required by CreateCommand. | ||
| typeEnvInfoerinterface { | ||
| // CurrentUser returns the current user. | ||
| CurrentUser() (*user.User,error) | ||
| // Environ returns the environment variables of the current process. | ||
| Environ() []string | ||
| // UserHomeDir returns the home directory of the current user. | ||
| UserHomeDir() (string,error) | ||
| // UserShell returns the shell of the given user. | ||
| UserShell(usernamestring) (string,error) | ||
| } | ||
| typesystemEnvInfoerstruct{} | ||
| vardefaultEnvInfoerEnvInfoer=&systemEnvInfoer{} | ||
| // DefaultEnvInfoer returns a default implementation of | ||
| // EnvInfoer. This reads information using the default Go | ||
| // implementations. | ||
| funcDefaultEnvInfoer()EnvInfoer { | ||
| returndefaultEnvInfoer | ||
| } | ||
| func (systemEnvInfoer)CurrentUser() (*user.User,error) { | ||
| returnuser.Current() | ||
| } | ||
| func (systemEnvInfoer)Environ() []string { | ||
| returnos.Environ() | ||
| } | ||
| func (systemEnvInfoer)UserHomeDir() (string,error) { | ||
| returnuserHomeDir() | ||
| } | ||
| func (systemEnvInfoer)UserShell(usernamestring) (string,error) { | ||
| returnusershell.Get(username) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: moved tousershell package
| func (*DockerEnvInfoer)Environ() []string { | ||
| // Return a clone of the environment so that the caller can't modify it | ||
| returnos.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. | ||
| returnos.UserHomeDir() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: we now use the embeddedusershell.SystemEnvInfo instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks great 👍🏻, minor nits/suggestions inline.
Uh oh!
There was an error while loading.Please reload this page.
| slog.F("before",append([]string{name},args...)), | ||
| slog.F("after",append([]string{modifiedName},modifiedArgs...)), | ||
| ) | ||
| cmd:=s.Execer.PTYCommandContext(ctx,modifiedName,modifiedArgs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Didn't we also have a docker execer? Did we end up not implementing that change, and are only relying on env infoer now? Or are we modifying commands from multiple angles? If former, all good, if latter, I'm wondering if we could unify the concepts (perhaps infoer can be applied on the execer level or vice-versa).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's all in the envinfoer now.
Uh oh!
There was an error while loading.Please reload this page.
| // HomeDir returns the home directory of the current user, giving | ||
| // priority to the $HOME environment variable. | ||
| funcHomeDir() (string,error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this still useful as a separate function or should we just require usage of envinfoer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd like to unexport it and require going through envinfoer, but trying to keep the scope of refactorings small here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm going to mark it as deprecated for now, we can remove it later.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Mathias Fredriksson <mafredri@gmail.com>Co-authored-by: Danny Kopping <danny@coder.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just one minor suggestion, but otherwise this is great 👍🏻
Uh oh!
There was an error while loading.Please reload this page.
172e523 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Relates to#16419
Builds on top of#16623 and wires up the ReconnectingPTY server. This does nothing to wire up the web terminal yet but the added test demonstrates the functionality working.
Other changes:
SystemEnvInfointerface to theagent/usershellpackage to address follow-up fromfeat(agent/agentcontainers): add ContainerEnvInfoer #16623 (comment)usershellinfo.Getas deprecated. Consumers should use theEnvInfoerinterface instead.