Add default mount options to pass to drivers #176
Conversation
rhatdan
changed the title
|
@nalind PTAL |
|
Looks reasonable, but we'll want to make sure we cover the other drivers, too. |
|
@nalind Added support for devicemapper, and returned failures for aufs, vfs and windows. Not sure if this can be done for btrfs or zfs. |
docs/containers-storage.conf.5.md
Outdated
| Default options to be used to mount container images. Suggested value "nodev". | ||
|
|
||
| **ostree_repo=""** | ||
| Tell storage drivers to use the specified OSTree repository. Some storage drivers, such as overlay, might use |
There was a problem hiding this comment.
Looks like we dropped some text here.
There was a problem hiding this comment.
This shouldn't be in the storage.options.thinpool section.
There was a problem hiding this comment.
Removed from my patch set.
drivers/devmapper/deviceset.go
Outdated
| @@ -2690,8 +2692,6 @@ func NewDeviceSet(root string, doInit bool, options []string, uidMaps, gidMaps [ | |||
| devices.filesystem = val | |||
| case "dm.mkfsarg": | |||
| devices.mkfsArgs = append(devices.mkfsArgs, val) | |||
| case "dm.mountopt": | |||
| devices.mountOptions = joinMountOptions(devices.mountOptions, val) | |||
There was a problem hiding this comment.
I get that the devicemapper.mountopt name comes from the driver name and how the option is parsed from the config file, but removing recognition of dm.mountopt seems unnecessary. Can we treat the two names as values for the same case?
There was a problem hiding this comment.
Wouldn't this just confuse people? I don't see a reason to have two options to do the same thing.
There was a problem hiding this comment.
They do the same thing. Otherwise we're renaming it, and configurations that used one name for the option will start returning an error in anything that uses the newer version of this library.
There was a problem hiding this comment.
Added back with no documentation.
drivers/zfs/zfs.go
Outdated
| @@ -300,6 +303,9 @@ func (d *Driver) create(id, parent string, storageOpt map[string]string) error { | |||
| } | |||
| if parent == "" { | |||
| mountoptions := map[string]string{"mountpoint": "legacy"} | |||
| if d.options.mountOptions != "" { | |||
| mountoptions[d.options.mountOptions] = "" | |||
| } | |||
There was a problem hiding this comment.
This looks like it wouldn't handle options of the key=value form (for example, uid=1000) correctly.
drivers/zfs/zfs.go
Outdated
| @@ -300,6 +303,10 @@ func (d *Driver) create(id, parent string, storageOpt map[string]string) error { | |||
| } | |||
| if parent == "" { | |||
| mountoptions := map[string]string{"mountpoint": "legacy"} | |||
| for _, opt := range strings.Split(d.options.mountOptions, ",") { | |||
| val := strings.SplitN(opt, "=", 2) | |||
| mountoptions[val[0]] = val[1] | |||
There was a problem hiding this comment.
If opt doesn't contain =, wouldn't val[1] be out of bounds?
drivers/zfs/zfs.go
Outdated
| } else { | ||
| mountoptions[val[0]] = "" | ||
| } | ||
| } |
There was a problem hiding this comment.
You're sure this goes here instead of in Get() below?
|
I would say a majority do support it. Not supported |
rhatdan
force-pushed
the
mount-options
branch
3 times, most recently
from
68f5651 to
c3b6d87
Compare
layers_ffjson.go
Outdated
| @@ -1,5 +1,5 @@ | |||
| // Code generated by ffjson <https://github.com/pquerna/ffjson>. DO NOT EDIT. | |||
| // source: layers.go | |||
| // source: ./layers.go | |||
There was a problem hiding this comment.
This PR doesn't need this change.
There was a problem hiding this comment.
I think it does, but I will try without it.
drivers/devmapper/deviceset.go
Outdated
| @@ -2664,6 +2664,8 @@ func NewDeviceSet(root string, doInit bool, options []string, uidMaps, gidMaps [ | |||
| } | |||
| key = strings.ToLower(key) | |||
| switch key { | |||
| case "devicemapper.mountopt": | |||
There was a problem hiding this comment.
Go ahead and merge this with the case "dm.mountopt": below.
I believe we should be running container images mounted with nodev by default. This would eliminate the disk of a device sneaking into the container without being on the approved list. This would give us the same or potentially additional security over the device cgroup. It would be nice if this could be passed in on an image by image basis. So users could also specify if they want nosuid images. Signed-off-by: Daniel J Walsh <[email protected]>
|
LGTM |
I believe we should be running container images mounted with nodev by default.
This would eliminate the disk of a device sneaking into the container without
being on the approved list. This would give us the same or potentially additional
security over the device cgroup.
It would be nice if this could be passed in on an image by image basis. So users
could also specify if they want nosuid images.
Signed-off-by: Daniel J Walsh [email protected]