Skip to content
This repository has been archived by the owner on Jun 2, 2022. It is now read-only.

Add ability to use exec on directories #769

Open
sbrow opened this issue Apr 13, 2020 · 8 comments
Open

Add ability to use exec on directories #769

sbrow opened this issue Apr 13, 2020 · 8 comments
Labels
enhancement New feature or request

Comments

@sbrow
Copy link

sbrow commented Apr 13, 2020

Use Case

When using the docker plugin, in order to run commands in a particular directory, I have to do this:

wexec docker/containers/container_name sh -c 'cd /var/ && ls'

When it would be, IMO, much more convenient and idiomatic to do this:

wexec docker/containers/container_name/fs/var ls

Describe the Solution You Would Like

Add the "exec" action to directories in the docker fs folder, where exec is equivilant to docker exec -it -w $directory_name $container_name $cmd.

For example:

docker exec -it -w /shared/httpd/ devilbox_php_1 pwd

would be equivilant to:

wexec docker/containers/devilbox_php_1/fs/shared/httpd pwd

Thanks! I really love the project.

@sbrow sbrow added the enhancement New feature or request label Apr 13, 2020
@welcome
Copy link

welcome bot commented Apr 13, 2020

Thanks for opening your first issue here! We will follow up as soon as we can.

@MikaelSmith
Copy link
Contributor

That's a great idea, I like it. I'm going to mull it over for a bit, but I don't see any way that contradicts how exec might evolve in the future.

One thing it does break is a habit I have of equating executable things with directly accessible resources, but that's a kludge I use to limit queries anyway. I think a better solution would be to have a separate property (maybe an attribute) that reflects whether the entry is directly addressable or requires indirection to access.

@sbrow
Copy link
Author

sbrow commented Apr 13, 2020

Thanks for the blazing fast response! 😄 If you need any help implementing this on the docker plugin, just point me in the right direction and I can start a PR.

@MikaelSmith
Copy link
Contributor

MikaelSmith commented Apr 13, 2020

I definitely won't get to it immediately, so that would be great.

I'd suggest starting by taking a look at https://github.com/puppetlabs/wash/blob/master/CORE_PLUGIN_DEVELOPMENT.md. The Docker plugin includes its view of the filesystem at https://github.com/puppetlabs/wash/blob/master/plugin/docker/container.go#L92-L94; all plugins actually use a shared implementation to view filesystems: https://pkg.go.dev/github.com/puppetlabs/wash/volume?tab=doc via NewFS.

The volume.FS type implements volume.Interface. Calling List on volume.FS lists the "root" of the filesystem as an array of volume.dir types. I think these are what we want to make executable; when you call wexec docker/containers/container_name/fs/var ls, you're running ls in the /var directory; the same wouldn't make sense for a file.

volume.FS is used to represent a filesystem that can be explored via Exec, so it already has something that we can exec against. volume.dir is also used to represent other things like persistent volumes that don't have a directly relationship to something that can be executed. So I think we'd want to create a new type like execableDir that's used just with volume.FS and wraps volume.Dir. It would look something like

type execableDir struct {
  dir
  impl execableInterface
}

func (v *execableDir) Exec(ctx context.Context, cmd string, args []string, opts plugin.ExecOptions) (plugin.ExecCommand, error) {
  return v.impl.VolumeExec(ctx, v.path, cmd, args, opts)
}

type execableInterface interface {
  Interface
  VolumeExec(ctx context.Context, path string, cmd string, args []string, opts plugin.ExecOptions) (ExecCommand, error)
}

volume.FS should be updated to implement VolumeExec. The purpose of VolumeExec is to update the command to change to the specified path before executing it via the plugin.Execable that volume.FS has a reference to. By implementing the Execable interface, execableDir communicates to Wash that it can execute a command triggered by wexec.

You'll also need to override List in execableDir to create new execableDirs instead of normal dirs, and possibly also change what ChildSchemas returns for FS and execableDir.

My usual process for implementing this would be:

  1. Create execableDir wrapping dir and use it in volume.FS in place of dir. Also update ChildSchemas calls. Then go run wash.go and look at stree output to confirm the schema change.
  2. Implement a dummy Exec on execableDir, run Wash again, and look at ls output to confirm that actions now include exec. You can also add var _ = plugin.Execable(&execableDir{}) for a static assertion that you have the correct Exec signature.
  3. Finish implementing Exec on execableDir and VolumeExec on FS. Try it out. Work on writing some unit tests around them.

One extra factor is that FS has the concept of a login shell. So ideally you'd write your VolumeExec to handle both POSIX-compatible shell and PowerShell syntax (look at other Volume* commands for how they select implementation based on login shell).

@sbrow
Copy link
Author

sbrow commented Apr 14, 2020

Thanks for the in-depth information! Sorry to keep bugging you, but how should I handle the VolumeExec function? I'm assuming that I should return d.executor.Exec from it, but how do I handle exec'ing in the directories? Ideally I'd think that would be implemented in the plugins themselves, right?

@MikaelSmith
Copy link
Contributor

I was thinking it'd be something like prepending cd <dir> && <cmd>, but maybe that wouldn't be sufficient. It does seem likely that plugins might have different ways to determine the working directory they execute in.

I'd add that as a new option to plugin.ExecOptions. Sounds like enabling this would also need to be opt-in when calling volume.NewFS, which the plugin should only do if it handles the new "working directory" option in Exec.

@sbrow
Copy link
Author

sbrow commented Apr 14, 2020

Adding WorkingDir to plugin.ExecOptions does the trick! Got it working with this:

// volume/fs.go
func (d *FS) VolumeExec(ctx context.Context, path string, cmd string, args []string, opts plugin.ExecOptions) (plugin.ExecCommand, error) {
	opts.WorkingDir = path
	return d.executor.Exec(ctx, cmd, args, opts)
}
//docker/container.go
func (c *container) Exec(ctx context.Context, cmd string, args []string, opts plugin.ExecOptions) (plugin.ExecCommand, error) {
        // ...
	cfg := types.ExecConfig{Cmd: command, AttachStdout: true, AttachStderr: true, Tty: opts.Tty, WorkingDir: opts.WorkingDir}
        // ...
}

And with that, docker is working. Now I just need to implement that on the other plugins, which I'm not terribly familiar with.

It's worth noting that although it is working, I'm not seeing [execDir] when I call stree.

@MikaelSmith
Copy link
Contributor

Nice work! The volume plugin can also be used by external plugins. That's why I suggest having an option to enable this mode in volume.NewFS, so we can incrementally add it to other plugins, and external plugins can choose whether or not to support it.

Oh, the name used comes from the entry schema, so you probably need to override https://github.com/puppetlabs/wash/blob/master/volume/dir.go#L32-L34 on execDir.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants