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

Allow adding rules to cgroup devices.allow on container create/run #22563

Merged
merged 3 commits into from Feb 1, 2017

Conversation

@mlaventure
Contributor

mlaventure commented May 6, 2016

- What I Did

This introduce a new --device-cgroup-rule flag that allow a user to add
one or more entry to the container cgroup device devices.allow

This should hopefully give a solution to issue like the one referred in the #22206 proposal:

  • add a rule to the container cgroup (e.g. 'c 13:* rwm')
  • via udev, execute a script anytime such a device is added.
  • the script should check whether the device should or should not be added to a given container
  • if the device is to be added, the script can run a docker exec to mknod the device into the container

- Note

I will make the required vendoring/engin-api PR once this has been accepted.

- Description for the changelog

  • Add the ability to specify extra rules for a container device cgroup devices.allow mechanism

- A picture of a cute animal (not mandatory but encouraged)


Signed-off-by: Kenfe-Mickael Laventure mickael.laventure@gmail.com

@mlaventure mlaventure changed the title from [WIP] Allow adding rules to cgroup devices.allow on container create/run to Allow adding rules to cgroup devices.allow on container create/run May 12, 2016

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 May 23, 2016

Contributor

I agree with a feature, but naming is confusing I think. Maybe something like device-cgroup-rule? I dunno

Contributor

LK4D4 commented May 23, 2016

I agree with a feature, but naming is confusing I think. Maybe something like device-cgroup-rule? I dunno

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure May 23, 2016

Contributor

@LK4D4 I like the name, thanks! Will update shortly

Contributor

mlaventure commented May 23, 2016

@LK4D4 I like the name, thanks! Will update shortly

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah May 26, 2016

Member

ping @LK4D4 ptal

Member

thaJeztah commented May 26, 2016

ping @LK4D4 ptal

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Jun 3, 2016

Contributor

Added a test

Contributor

mlaventure commented Jun 3, 2016

Added a test

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 4, 2016

Contributor

This seems super low-level (much more so than any other flag we have, I think).

I don't understand the limitations on the devices cgroup, but... it seems like we should be able to address the use case with docker update to deal with adding the device to the whitelist and performing the mknod for the user.

This could also be looked at along with security profiles... some profile option to enable host device update access.

Contributor

cpuguy83 commented Jun 4, 2016

This seems super low-level (much more so than any other flag we have, I think).

I don't understand the limitations on the devices cgroup, but... it seems like we should be able to address the use case with docker update to deal with adding the device to the whitelist and performing the mknod for the user.

This could also be looked at along with security profiles... some profile option to enable host device update access.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Jun 4, 2016

Contributor

ping @tonistiigi , if I'm not wrong he was against a docker update solution. I'll let him explain his view :)

Contributor

mlaventure commented Jun 4, 2016

ping @tonistiigi , if I'm not wrong he was against a docker update solution. I'll let him explain his view :)

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Sep 15, 2016

Member

Design LGTM

Member

tonistiigi commented Sep 15, 2016

Design LGTM

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Sep 15, 2016

Member

@cpuguy83 I'm not strictly against the update solution but it would be much complicated. Then we also need to do an exec to make the mknod. Then someone will ask why we can do devices but not volumes and it may become complicated soon. Anyway, either way looks fine by me.

Member

tonistiigi commented Sep 15, 2016

@cpuguy83 I'm not strictly against the update solution but it would be much complicated. Then we also need to do an exec to make the mknod. Then someone will ask why we can do devices but not volumes and it may become complicated soon. Anyway, either way looks fine by me.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 21, 2016

Member

ping @mlaventure what's the status here? also needs a rebase

Member

thaJeztah commented Oct 21, 2016

ping @mlaventure what's the status here? also needs a rebase

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Oct 21, 2016

Contributor

Rebased.

Status is that it's waiting on a decision whether we want this in this form or not :)

Contributor

mlaventure commented Oct 21, 2016

Rebased.

Status is that it's waiting on a decision whether we want this in this form or not :)

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Oct 22, 2016

Member

ping @cpuguy83 @tonistiigi PTAL - too low level for me, haha

Member

thaJeztah commented Oct 22, 2016

ping @cpuguy83 @tonistiigi PTAL - too low level for me, haha

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 2, 2016

Contributor

Maybe the low-level is ok, but I think we should have a higher level abstraction as well that the user can say "let me hotplug stuff into my usb ports and propagate to the container".

Contributor

cpuguy83 commented Nov 2, 2016

Maybe the low-level is ok, but I think we should have a higher level abstraction as well that the user can say "let me hotplug stuff into my usb ports and propagate to the container".

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Nov 2, 2016

Contributor

But I don't think many humans will actually use this.

Contributor

cpuguy83 commented Nov 2, 2016

But I don't think many humans will actually use this.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Nov 2, 2016

Contributor

@cpuguy83 at least on linux this high level interface is not that simple. A usb device can end up creating any sort of MAJOR:MINOR combination.

So this would involve playing around with udev, /sys/dev and cgroups (to add them to the allow list).

Also, you may want to prohibit some category of devices to be propagated (or vice-versa).

I'd rather leave this complexity to the user (at least for now).

Contributor

mlaventure commented Nov 2, 2016

@cpuguy83 at least on linux this high level interface is not that simple. A usb device can end up creating any sort of MAJOR:MINOR combination.

So this would involve playing around with udev, /sys/dev and cgroups (to add them to the allow list).

Also, you may want to prohibit some category of devices to be propagated (or vice-versa).

I'd rather leave this complexity to the user (at least for now).

@vdemeester vdemeester merged commit 27f90ac into moby:master Feb 1, 2017

5 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 29997 has succeeded
Details
janky Jenkins build Docker-PRs 38602 has succeeded
Details
vendor Jenkins build Docker-PRs-vendor 2810 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9635 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 1, 2017

## Specify isolation technology for container (--isolation)
This option is useful in situations where you are running Docker containers on

This comment has been minimized.

@thaJeztah

thaJeztah Feb 2, 2017

Member

Why was this section removed?

@thaJeztah

thaJeztah Feb 2, 2017

Member

Why was this section removed?

This comment has been minimized.

@thaJeztah

thaJeztah Feb 2, 2017

Member

oh, I see; moved to the other document

@thaJeztah

thaJeztah Feb 2, 2017

Member

oh, I see; moved to the other document

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 2, 2017

Member

@mlaventure looks like you forgot to update the CLI reference with the example; #22563 (comment)

Member

thaJeztah commented Feb 2, 2017

@mlaventure looks like you forgot to update the CLI reference with the example; #22563 (comment)

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Mar 13, 2017

Contributor

ping @albers I had initially added a basic completion for bash (it's a very old PR), but I think you would do a better job, mind having a second look?

Thanks!

Contributor

mlaventure commented Mar 13, 2017

ping @albers I had initially added a basic completion for bash (it's a very old PR), but I think you would do a better job, mind having a second look?

Thanks!

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Mar 14, 2017

Member

@mlaventure I don't think this can be improved because the argument contains a space. AFAIK, bash completion does not work inside quoted arguments.

Member

albers commented Mar 14, 2017

@mlaventure I don't think this can be improved because the argument contains a space. AFAIK, bash completion does not work inside quoted arguments.

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Mar 14, 2017

Contributor

@albers thanks for checking!

Contributor

mlaventure commented Mar 14, 2017

@albers thanks for checking!

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#22563 from mlaventure/cgroup-devices
Allow adding rules to cgroup devices.allow on container create/run
@hqhq

This comment has been minimized.

Show comment
Hide comment
@hqhq

hqhq Aug 1, 2017

Contributor

Why not improving --device to support something like --device /dev/sdb*:rx than to introduce this option?

Contributor

hqhq commented Aug 1, 2017

Why not improving --device to support something like --device /dev/sdb*:rx than to introduce this option?

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Aug 1, 2017

Contributor

@hqhq what you suggest would require to spawn a goroutine to monitor changes to the filesystem and replicate it within the container. But that would also assume that the container has all the tools necessary for this (e.g. mknod) or force it to be there.

Also, maybe the right of the device thus newly created would need to be changed within the container.

It's easier to let people set up their own policy with the tool provided.

Contributor

mlaventure commented Aug 1, 2017

@hqhq what you suggest would require to spawn a goroutine to monitor changes to the filesystem and replicate it within the container. But that would also assume that the container has all the tools necessary for this (e.g. mknod) or force it to be there.

Also, maybe the right of the device thus newly created would need to be changed within the container.

It's easier to let people set up their own policy with the tool provided.

@th3mis

This comment has been minimized.

Show comment
Hide comment
@th3mis

th3mis Jan 29, 2018

  1. How can I use --device-cgroup-rule option by docker-compose?

  2. Also, I created some POC for working with dynamically created USB devices in Docker, does it correct? It works fine, but it seems very ugly:
    https://github.com/th3mis/docker-usb-sync

th3mis commented Jan 29, 2018

  1. How can I use --device-cgroup-rule option by docker-compose?

  2. Also, I created some POC for working with dynamically created USB devices in Docker, does it correct? It works fine, but it seems very ugly:
    https://github.com/th3mis/docker-usb-sync

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Jan 29, 2018

Contributor

@th3mis

  1. I don't think it's supported, you should ask on the compose repository: https://github.com/docker/compose

  2. That looks correct. I don't see the ugliness in it. I'm not fan of adding a whole udev handler in the Docker daemon when there's already a dedicated architecture for udev.

Contributor

mlaventure commented Jan 29, 2018

@th3mis

  1. I don't think it's supported, you should ask on the compose repository: https://github.com/docker/compose

  2. That looks correct. I don't see the ugliness in it. I'm not fan of adding a whole udev handler in the Docker daemon when there's already a dedicated architecture for udev.

@th3mis

This comment has been minimized.

Show comment
Hide comment
@th3mis

th3mis Jan 29, 2018

@mlaventure
Why also there is no option to initially add с 189:* rwminto devices.list when create new cgroup for container? I suppose it best solution, but I am new in Docker, and maybe I'm wrong here

th3mis commented Jan 29, 2018

@mlaventure
Why also there is no option to initially add с 189:* rwminto devices.list when create new cgroup for container? I suppose it best solution, but I am new in Docker, and maybe I'm wrong here

@mlaventure

This comment has been minimized.

Show comment
Hide comment
@mlaventure

mlaventure Jan 30, 2018

Contributor

@th3mis that what the option you're providing does.

Contributor

mlaventure commented Jan 30, 2018

@th3mis that what the option you're providing does.

@pamelamei

This comment has been minimized.

Show comment
Hide comment
@pamelamei

pamelamei Jun 13, 2018

I think this feature is really helpful.

pamelamei commented Jun 13, 2018

I think this feature is really helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment