gateway: create interface for reading from container filesystem#6262
Conversation
0965d22 to
64096ea
Compare
64096ea to
5cd1eac
Compare
tonistiigi
left a comment
There was a problem hiding this comment.
Needs integration test.
I'm not sure if the mount selection really is feasible this way.
|
|
||
| func (gwCtr *gatewayContainer) findMount(ctx context.Context, fullpath string) (m executor.Mount, path string) { | ||
| m = gwCtr.rootFS | ||
| path, _ = filepath.Rel("/", fullpath) |
There was a problem hiding this comment.
Not sure what this is supposed to do. Also error ignore.
There was a problem hiding this comment.
It's mostly to remove the leading slash. The fs.FS interface requires relative paths and doesn't work with absolute paths.
| } | ||
|
|
||
| for _, mount := range gwCtr.mounts { | ||
| if strings.HasPrefix(fullpath, mount.Dest) { |
There was a problem hiding this comment.
There has been no cleanup of fullpath afaics.
There was a problem hiding this comment.
This doesn't seem to check directory boundaries as well iiuc.
|
|
||
| for _, mount := range gwCtr.mounts { | ||
| if strings.HasPrefix(fullpath, mount.Dest) { | ||
| remainder, err := filepath.Rel(mount.Dest, fullpath) |
There was a problem hiding this comment.
What if this is a symlink. Or contains a symlink somewhere in the path.
|
Talked with @tonistiigi about this and we're going to change the path parsing logic to instead include the index of the mount as a client parameter. This puts the onus on the client for determining which mount they're accessing instead of trying to get that logic correct on the server where there might be more security concerns regarding symlinks. This will also be a little better from the DAP perspective because we can put an entry for each mount instead of putting everything into the filesystem tree. That will result in the mount roots being more easily visible in DAP. |
| // Check if this mount has already been mounted. | ||
| if f, ok := gwCtr.localMounts[mount]; ok { | ||
| return f, path, nil | ||
| } | ||
|
|
||
| ref, err := mount.Src.Mount(ctx, true) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } |
There was a problem hiding this comment.
Was discussing with @tonistiigi, potentially this logic is worth extending to the existing ReadFile/StatFile/etc, not just for containers?
Don't want to step on your toes here @jsternberg, would it be reasonable if I cherry-picked that change out into a separate PR if this one is in the v0.future milestone?
There was a problem hiding this comment.
It's fine if you want to cherry pick it. This PR is going to likely undergo some changes though so I can't promise it'll be compatible. I had to do some other work that was a bit of a prerequisite to this one and I'm likely going to pick this up again either now or soon.
5cd1eac to
7186b8a
Compare
7186b8a to
b10815a
Compare
ac6e7ea to
d47b53f
Compare
f1ea495 to
511424f
Compare
This creates an interface that can be used to read the filesystem of a new container created through the gateway API. These filesystem reading methods are tied to a specific container that has been created, but aren't tied to the container itself. Due to being run inside of buildkit, these containers have access to the same mounts that a container request would have. This is useful for features like the file explorer in `buildx dap` because it can access container filesystem state from stages that error along with ones that have completed successfully. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
511424f to
2999fdc
Compare
| localMounts[i].Src = p.Root.Src | ||
| continue | ||
| } | ||
| localMounts[i].Src = mountableByDest[m.Dest] |
There was a problem hiding this comment.
Maybe error case could be added in here in follow-up? To catch if anything goes wrong between the mnts and PreparedMounts constency.
This creates an interface that can be used to read the filesystem of a
new container created through the gateway API. These filesystem reading
methods are tied to a specific container that has been created, but
aren't tied to the container itself.
Due to being run inside of buildkit, these containers have access to the
same mounts that a container request would have. This is useful for
features like the file explorer in
buildx dapbecause it can accesscontainer filesystem state from stages that error along with ones that
have completed successfully.
Offers a more robust solution to docker/buildx#3410.