-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plugins: container-rootfs-relative paths #26398
Conversation
de699b3
to
895ef95
Compare
@@ -58,7 +58,7 @@ func Init(root string, remote libcontainerd.Remote, rs registry.Service, liveRes | |||
root = filepath.Join(root, "plugins") | |||
manager = &Manager{ | |||
libRoot: root, | |||
runRoot: "/run/docker", | |||
runRoot: "/run/docker/plugins", |
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.
@anusha-ragunathan @vieux I changed this to accommodate legacy plugins that have hardcoded /run/docker/plugins
already. I had initially chosen /run/docker
because it was shorter. To be clear: this is the path inside the container where we expect the socket to be.
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.
Works for me. It reduces the code change required by the legacy plugins to move to pluginv2.
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.
@tiborvass : you dont need to change this for plugins. This is the host directory that gets bind mounted to the plugin. The plugin run time path is defaultPluginRuntimeDestination
which is already set to /run/docker/plugins
https://github.com/docker/docker/blob/fa7c1a68a68ffb7c6851c59d2c0b41091b9d6e5e/plugin/v2/plugin.go#L29
We could change runRoot back to /run/docker/plugins
to indicate that the directories that get created under there are plugins related.
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.
For a separate PR, but this should not be hard-coded to /run/docker
but instead use the daemon exec root
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.
The reason to hard code is that, this holds the plugin socket. There's a socket path limitation on Linux to 108 bytes. The daemon exec root doesnt have any socket and is not bound by this limitation. So we have to choose a path that always works.
895ef95
to
cc0760a
Compare
@@ -205,18 +213,53 @@ func (p *Plugin) GetTypes() []types.PluginInterfaceType { | |||
|
|||
// InitSpec creates an OCI spec from the plugin's config. | |||
func (p *Plugin) InitSpec(s specs.Spec, libRoot string) (*specs.Spec, error) { | |||
rootfs := filepath.Join(libRoot, p.PluginObj.ID, "rootfs") | |||
p.Rootfs = filepath.Join(libRoot, p.PluginObj.ID, "rootfs") |
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.
p.Rootfs can also be used in https://github.com/docker/docker/blob/master/plugin/backend.go#L132
Lets keep this PR specifically for container-rootfs-relative path fix and open new ones for the other bug fixes. Keeps a cleaner history. |
Readonly: false, // TODO: all plugins should be readonly? settable in manifest? | ||
} | ||
|
||
// TODO: the following should be only if granting device permissions |
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.
Lets fix this todo by adding to Manifest's privilege section. Something like "REQUEST_DEVICE_CREATION".
cc0760a
to
3cce395
Compare
@@ -113,7 +113,7 @@ func (ps *PluginStore) getAllByCap(capability string) []CompatPlugin { | |||
|
|||
result := make([]CompatPlugin, 0, 1) | |||
for _, p := range ps.plugins { | |||
if _, err := p.FilterByCap(capability); err == nil { | |||
if _, err := p.FilterByCap(capability); err == nil && p.IsEnabled() { |
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.
@anusha-ragunathan I could not change this in the caller function because I have only access to CompatPlugin interface values, and not *Plugin objects.
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.
Fair enough.
3eb7294
to
1362b42
Compare
|
Please let me know if I can assist. Looking forward to seeing this get merged for 1.13. |
Thanks, I'm planning to update this soon. |
ping @tiborvass what's the status here? |
1362b42
to
74bb539
Compare
if typ.Capability == "volumedriver" && typ.Prefix == "docker" && strings.HasPrefix(typ.Version, "1.") { | ||
if p.PluginObj.Config.PropagatedMount != "" { | ||
// TODO: sanitize PropagatedMount and prevent breakout | ||
p.PropagatedMount = filepath.Join(p.Rootfs, p.PluginObj.Config.PropagatedMount) |
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.
Please see #26398 (comment)
c.Log("plugin disabled") | ||
mounts, err := mount.GetMounts() | ||
c.Assert(err, checker.IsNil) | ||
for _, mnt := range mounts { |
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.
We are simply printing out the mounts, which doesnt test much. Can we use findmnt
to see if the mount propagated?
Beyond the already commented items, code seems ok. |
@@ -32,7 +32,7 @@ func (s *DockerSuite) TestPluginBasicOps(c *check.C) { | |||
id = strings.TrimSpace(id) | |||
c.Assert(err, checker.IsNil) | |||
|
|||
out, _, err = dockerCmdWithError("plugin", "remove", pNameWithTag) | |||
out, _, _ = dockerCmdWithError("plugin", "remove", pNameWithTag) |
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.
Shouldn't this be dockerCmd
instead ? Doesn't seem right to not care about error at all 😓
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.
@vdemeester I kept err
and asserted it non-nil. Realized that dockerCmd
will assume non-nil error...
2e89b78
to
46c343f
Compare
for _, typ := range p.PluginObj.Config.Interface.Types { | ||
if typ.Capability == "volumedriver" && typ.Prefix == "docker" && strings.HasPrefix(typ.Version, "1.") { | ||
if p.PluginObj.Config.PropagatedMount != "" { | ||
// TODO: sanitize PropagatedMount and prevent breakout |
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 applicable? When does the mount leak?
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.
../../../../etc
@@ -21,11 +22,13 @@ type Plugin struct { | |||
PluginObj types.Plugin `json:"plugin"` | |||
PClient *plugins.Client `json:"-"` | |||
RuntimeSourcePath string `json:"-"` | |||
Rootfs string `json:"-"` |
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.
Lets not introduce Public fields in this struct. Refer to #29191 is making changes to make this all private and move necessary fields to a controller
struct in the manager. Please review that PR and follow similar design on the new fields.
@@ -71,6 +71,10 @@ type PluginConfig struct { | |||
// Required: true | |||
Network PluginConfigNetwork `json:"Network"` | |||
|
|||
// propagated mount | |||
// Required: true | |||
PropagatedMount string `json:"PropagatedMount"` |
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.
https://github.com/docker/docker/blob/master/docs/extend/config.md needs to be updated. Please do that alongside.
46c343f
to
a0b59db
Compare
LGTM (if tests pass) |
if typ.Capability == "volumedriver" && typ.Prefix == "docker" && strings.HasPrefix(typ.Version, "1.") { | ||
if p.PluginObj.Config.PropagatedMount != "" { | ||
// TODO: sanitize PropagatedMount and prevent breakout | ||
p.PropagatedMount = filepath.Join(p.LibRoot, p.PluginObj.Config.PropagatedMount) |
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.
filepath.Base(p.PluginObj.Config.PropagatedMount)
?
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.
Making sure I understand this... this is for mounting the propagated path to the plugin root (outside rootfs)?
Maybe even just use mnt
instead of the mount path.
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.
@tiborvass : You should be using filepath.Join(p.LibRoot, p.PluginObj.ID, p.PluginObj.Config.PropagatedMount)
. Else mounts from different plugins with the same name will collide. Another option is to fix p.LibRoot
to include the pluginID (It should have been this way in the first place)
We dont want to use hard coded mnt
. We want plugin devs to specify the mount that needs propagation.
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.
@cpuguy83 : PropagatedMount
is strictly for those plugins (typically volume and graph drivers) that need to setup mounts that need to propagated to the host.
We want plugin devs to specify which mounts need to be propagated, rather than hard coding it to a specific path.
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.
Right, but isn't this a singleton that's the "host" side of the mount?
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.
Yes. Having the same dir as the mount in the config could be less confusing. On the other hand, having a standard dir such as mnt
is also reasonable. I'll let @tiborvass decide. Also, we dont have to do this in this PR.
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.
For start up reasons, this field is also set in plugin/manager.go
. And I just realized it's set to something else. So I'll go with p.Rootfs for now.
Legacy plugins expect host-relative paths (such as for Volume.Mount). However, a containerized plugin cannot respond with a host-relative path. Therefore, this commit modifies new volume plugins' paths in Mount and List to prepend the container's rootfs path. This introduces a new PropagatedMount field in the Plugin Config. When it is set for volume plugins, RootfsPropagation is set to rshared and the path specified by PropagatedMount is bind-mounted with rshared prior to launching the container. This is so that the daemon code can access the paths returned by the plugin from the host mount namespace. Signed-off-by: Tibor Vass <tibor@docker.com>
a0b59db
to
c54b717
Compare
Legacy plugins expect host-relative paths (such as for Volume.Mount).
However, a containerized plugin cannot respond with a host-relative
path. Therefore, this commit modifies new volume plugins' paths in Mount
and List to prepend the container's rootfs path.
Volume plugins that do mounts and want it to propagate to the host namespace, need to mount inside
/mnt
.Signed-off-by: Tibor Vass tibor@docker.com