Skip to content
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

Fix relabeling local volume source dir #37739

Merged
merged 1 commit into from
Aug 31, 2018

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Aug 30, 2018

- What I did / - How I did it / - How to verify it
In case a volume is specified via Mounts API, and SELinux is enabled,
the following error happens on container start:

$ docker volume create testvol
$ docker run --rm --mount source=testvol,target=/tmp busybox true
docker: Error response from daemon: error setting label on mount
source '': no such file or directory.

The functionality to relabel the source of a local mount specified via
Mounts API was introduced in commit 5bbf5cc (#34684) and later broken by commit
e4b6adc (#36688), which removed setting mp.Source field.

With the current data structures, the host dir is already available in
v.Mountpoint, so let's just use it.

Fixes: e4b6adc

- Description for the changelog

Fix relabeling of local volumes specified via Mounts API on selinux-enabled systems

In case a volume is specified via Mounts API, and SELinux is enabled,
the following error happens on container start:

> $ docker volume create testvol
> $ docker run --rm --mount source=testvol,target=/tmp busybox true
> docker: Error response from daemon: error setting label on mount
> source '': no such file or directory.

The functionality to relabel the source of a local mount specified via
Mounts API was introduced in commit 5bbf5cc and later broken by commit
e4b6adc, which removed setting mp.Source field.

With the current data structures, the host dir is already available in
v.Mountpoint, so let's just use it.

Fixes: e4b6adc
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@thaJeztah
Copy link
Member

Some quick thoughts / questions (didn't dig into the whole code flow 😅);

  • IIRC, when using the mount API, relabelling is disabled for bind-mounts (i.e., it's only accepted for volumes); is that still the case with this change?
  • Apparently, our tests didn't catch this regression; is it possible to write a test for this, to verify that labelling works?

@thaJeztah
Copy link
Member

also; ping @cpuguy83 PTAL

@mavenugo
Copy link
Contributor

The janky failure : DockerSwarmSuite.TestSwarmLockUnlockCluster is a known flaky failure.

Copy link
Contributor

@mavenugo mavenugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kolyshkin
Copy link
Contributor Author

IIRC, when using the mount API, relabelling is disabled for bind-mounts
(i.e., it's only accepted for volumes); is that still the case with this change?

I am not touching any non-volume code here so there should be no change.

is it possible to write a test

I thought about that and can't think of any good unit test since the code and data are spread among the different subsystems. I can try adding an integration test though.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's work on (e2e) tests in a follow up

LGTM

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

Successfully merging this pull request may close these issues.

None yet

5 participants