Do not relabel if user did not request it for non local volumes #20834

Merged
merged 1 commit into from Mar 4, 2016

Conversation

Projects
None yet
9 participants
@rhatdan
Contributor

rhatdan commented Mar 1, 2016

fixes #18005

Signed-off-by: Dan Walsh dwalsh@redhat.com

Do not relabel if user did not request it for non local volumes
Signed-off-by: Dan Walsh <dwalsh@redhat.com>
@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 1, 2016

Contributor

I think this is a better fix then #20829

Since it will fail if the user requests a relabel. Should fix the rexray problem.

We are automatically labeling local volumes, but if the user uses other different volumes, he should request relabeling. Then we can fail if the volume does not support the labeling.

Contributor

rhatdan commented Mar 1, 2016

I think this is a better fix then #20829

Since it will fail if the user requests a relabel. Should fix the rexray problem.

We are automatically labeling local volumes, but if the user uses other different volumes, he should request relabeling. Then we can fail if the volume does not support the labeling.

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 2, 2016

Contributor

also ping @clintonskitson

Contributor

HackToday commented Mar 2, 2016

also ping @clintonskitson

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 2, 2016

Contributor

I tried above patch above, seems it could not work. Failed with following error

$ sudo ./docker-1.11.0-dev run -ti --volume-driver=rexray -v testtwo:/test busybox
permission denied
./docker-1.11.0-dev: Error response from daemon: Container command could not be invoked..

And docker logs are

Mar 02 09:24:17 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:17.166285982Z" level=debug msg="Closing buffered stdin pipe"
Mar 02 09:24:16 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:16.971304507Z" level=error msg="Handler for POST /v1.23/containers/b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802/start returned error: Container command could not be invoked."
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.150251640Z" level=error msg="Error unmounting container b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802: not mounted"
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.148944616Z" level=warning msg="failed to cleanup ipc mounts:\nfailed to umount /var/lib/docker/containers/b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802/shm: invalid argument"
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.134551045Z" level=error msg="error locating sandbox id 7f3371375e5711701ad91e76eafd0ad82fb30c42ee2574d36b41b4bf5356155e: sandbox 7f3371375e5711701ad91e76eafd0ad82fb30c42ee2574d36b41b4bf5356155e not found"
Mar 02 09:24:08 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:08.140166342Z" level=debug msg="devmapper: UnmountDevice(hash=5a0cb8fafadc7a8861896e510f5132fde57bcd73b7766eebf60b3e173fda249a) END"
Mar 02 09:24:08 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker
Contributor

HackToday commented Mar 2, 2016

I tried above patch above, seems it could not work. Failed with following error

$ sudo ./docker-1.11.0-dev run -ti --volume-driver=rexray -v testtwo:/test busybox
permission denied
./docker-1.11.0-dev: Error response from daemon: Container command could not be invoked..

And docker logs are

Mar 02 09:24:17 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:17.166285982Z" level=debug msg="Closing buffered stdin pipe"
Mar 02 09:24:16 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:16.971304507Z" level=error msg="Handler for POST /v1.23/containers/b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802/start returned error: Container command could not be invoked."
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.150251640Z" level=error msg="Error unmounting container b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802: not mounted"
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.148944616Z" level=warning msg="failed to cleanup ipc mounts:\nfailed to umount /var/lib/docker/containers/b63e68846a4dc663bbcfa0e5efb8f336afc7d5c0e491c75c782decf108e0a802/shm: invalid argument"
Mar 02 09:24:15 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:15.134551045Z" level=error msg="error locating sandbox id 7f3371375e5711701ad91e76eafd0ad82fb30c42ee2574d36b41b4bf5356155e: sandbox 7f3371375e5711701ad91e76eafd0ad82fb30c42ee2574d36b41b4bf5356155e not found"
Mar 02 09:24:08 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker-1.11.0-dev[2005]: time="2016-03-02T09:24:08.140166342Z" level=debug msg="devmapper: UnmountDevice(hash=5a0cb8fafadc7a8861896e510f5132fde57bcd73b7766eebf60b3e173fda249a) END"
Mar 02 09:24:08 sw-s5lcdlw5ktj-0-jserl73ppvan-swarm-node-lscjkrmyn6rx.novalocal docker
@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 2, 2016

Contributor

@HackToday Could you print out the name of the bind.Driver and the bind.Mode before the Requires Relabel check?

Contributor

rhatdan commented Mar 2, 2016

@HackToday Could you print out the name of the bind.Driver and the bind.Mode before the Requires Relabel check?

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 2, 2016

Contributor

@rhatdan I would have access environment tomorrow, right now, at home, I could not access that. :(

Contributor

HackToday commented Mar 2, 2016

@rhatdan I would have access environment tomorrow, right now, at home, I could not access that. :(

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 2, 2016

Contributor

@HackToday Ok, this patch is supposed to prevent all non local volumes from doing the relabel, unless the user requests it. The code above adds 'z" to the bind.Mode if the Driver is local which forces the relabel. In the current code, the relabel is forced on all volume drivers which I believe is causing the issue with rexray, and other volumes that do not support XATTR (SELinux) labels.

I like this change better since we will still get failures if a user tries to mount a volume like rexray and specifies :Z or :z

Contributor

rhatdan commented Mar 2, 2016

@HackToday Ok, this patch is supposed to prevent all non local volumes from doing the relabel, unless the user requests it. The code above adds 'z" to the bind.Mode if the Driver is local which forces the relabel. In the current code, the relabel is forced on all volume drivers which I believe is causing the issue with rexray, and other volumes that do not support XATTR (SELinux) labels.

I like this change better since we will still get failures if a user tries to mount a volume like rexray and specifies :Z or :z

@clintkitson

This comment has been minimized.

Show comment
Hide comment
@clintkitson

clintkitson Mar 2, 2016

Contributor

@rhatdan Confirmed, this patch works as well.

Contributor

clintkitson commented Mar 2, 2016

@rhatdan Confirmed, this patch works as well.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 2, 2016

Contributor

Excellent, I think this is the patch that should be merged into docker.

Contributor

rhatdan commented Mar 2, 2016

Excellent, I think this is the patch that should be merged into docker.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 2, 2016

Member

This fixes #18005 ?

Member

thaJeztah commented Mar 2, 2016

This fixes #18005 ?

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 2, 2016

Contributor

Yes it should.

Contributor

rhatdan commented Mar 2, 2016

Yes it should.

@HackToday

This comment has been minimized.

Show comment
Hide comment
@HackToday

HackToday Mar 3, 2016

Contributor

hi @rhatdan as another issue #20855 which prevent me from verify your fix here, But I think if coreos works, it should work with atomic, Right?

Also, for our cases, since this fix can only be merged in 1.11.0 or 1.10.*, But atomic not integrated such new version, Is it possible to have some work-around for atomic ? (means, still enable selinux, but need user do something when run containers with volume driver)

Contributor

HackToday commented Mar 3, 2016

hi @rhatdan as another issue #20855 which prevent me from verify your fix here, But I think if coreos works, it should work with atomic, Right?

Also, for our cases, since this fix can only be merged in 1.11.0 or 1.10.*, But atomic not integrated such new version, Is it possible to have some work-around for atomic ? (means, still enable selinux, but need user do something when run containers with volume driver)

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 3, 2016

Contributor

If this gets merged into 1.11.0 we will cherrypick it into our docker-1.10 for Fedora and RHEL, or if it ends up in 1.10.3 we would upgrade to this.

Contributor

rhatdan commented Mar 3, 2016

If this gets merged into 1.11.0 we will cherrypick it into our docker-1.10 for Fedora and RHEL, or if it ends up in 1.10.3 we would upgrade to this.

@runcom

This comment has been minimized.

Show comment
Hide comment
Member

runcom commented Mar 3, 2016

@calavera

This comment has been minimized.

Show comment
Hide comment
@calavera

calavera Mar 3, 2016

Contributor

LGTM

Contributor

calavera commented Mar 3, 2016

LGTM

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Mar 4, 2016

Member

LGTM

Member

runcom commented Mar 4, 2016

LGTM

runcom added a commit that referenced this pull request Mar 4, 2016

Merge pull request #20834 from rhatdan/relabelvolume
Do not relabel if user did not request it for non local volumes

@runcom runcom merged commit 8142ebb into moby:master Mar 4, 2016

8 checks passed

docker/dco-signed All commits signed
Details
documentation success 1 tests run, 1 skipped, 0 failed.
Details
experimental Jenkins build Docker-PRs-experimental 15681 has succeeded
Details
gccgo Jenkins build Docker-PRs-gccgo 2578 has succeeded
Details
janky Jenkins build Docker-PRs 24471 has succeeded
Details
userns Jenkins build Docker-PRs-userns 6821 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 22589 has succeeded
Details
windowsTP4 Jenkins build Docker-PRs-WoW-TP4 1880 has succeeded
Details

@thaJeztah thaJeztah added this to the 1.10.3 milestone Mar 4, 2016

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Mar 4, 2016

Contributor

:+1 looks good.

Contributor

cpuguy83 commented Mar 4, 2016

:+1 looks good.

@rhatdan

This comment has been minimized.

Show comment
Hide comment
@rhatdan

rhatdan Mar 4, 2016

Contributor

Nice.

Contributor

rhatdan commented Mar 4, 2016

Nice.

@tiborvass tiborvass referenced this pull request Mar 7, 2016

Merged

1.10.3 cherrypicks #21011

@miry

This comment has been minimized.

Show comment
Hide comment
@miry

miry Aug 17, 2016

@rhatdan is this patch merged in 1.10.3?

miry commented Aug 17, 2016

@rhatdan is this patch merged in 1.10.3?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 17, 2016

Member

@miry yes; just follow the link above: #21011

Member

thaJeztah commented Aug 17, 2016

@miry yes; just follow the link above: #21011

@miry

This comment has been minimized.

Show comment
Hide comment
@miry

miry Aug 17, 2016

For some reason I need to run in privileged to access the folder.

$ docker run -ti --volume-driver=rexray -v testtwo:/test:rw busybox              
/ # touch /test/1
touch: /test/1: Permission denied 
$ docker run --privileged -ti --volume-driver=rexray -v testtwo:/test:rw busybox
/ # touch /test/1
/ #

permissions of mounted volume:

drwx------ 2 root root 4096 Aug 17 08:54 test

miry commented Aug 17, 2016

For some reason I need to run in privileged to access the folder.

$ docker run -ti --volume-driver=rexray -v testtwo:/test:rw busybox              
/ # touch /test/1
touch: /test/1: Permission denied 
$ docker run --privileged -ti --volume-driver=rexray -v testtwo:/test:rw busybox
/ # touch /test/1
/ #

permissions of mounted volume:

drwx------ 2 root root 4096 Aug 17 08:54 test

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 17, 2016

Member

@miry could you open an issue for that? Better than discussing it on a merged PR; make sure to provide info about your setup (at least docker version and docker info)

Member

thaJeztah commented Aug 17, 2016

@miry could you open an issue for that? Better than discussing it on a merged PR; make sure to provide info about your setup (at least docker version and docker info)

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