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

Linux upstream kernel Overlay file systems support SELinux #25616

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Aug 11, 2016

Remove checks that prevent overlay and SELinux from working together.
Fixes are arriving in the 4.9 kernel.

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

} else {
logrus.Warn("Docker could not enable SELinux on the host system")
}
logrus.Warn("Docker could not enable SELinux on the host system")
Copy link
Member

Choose a reason for hiding this comment

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

should this be

if !selinuxEnabled() {
    logrus.Warn("Docker could not enable SELinux on the host system")
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, if this depends on kernel 4.9, should we use the old behavior if an older kernel is in use? (or is there another way to detect if it's supported?)

Remove checks that prevent overlay and SELinux from working together.
Fixes are arriving in the 4.9 kernel.

Signed-off-by: Dan Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Contributor Author

rhatdan commented Aug 11, 2016

@thaJeztah Yes your right, I got a little too aggressive with my deleting.

I am not sure docker should do anything about handling kernel version. This is a tough problem and will get worse going forward, with what the kernel supports and does not. For example if new sysctls get namespaced, how do we handle this. How does docker handle users setting up storage back ends that the kernel does not support?

I would not put this fix into newer dockers until 1.13, and hopefully by then people will start to see kernel 4.9 showing up. We plan on back porting this feature to RHEL kernels.

@thaJeztah
Copy link
Member

I agree that kernel version tracking is tricky (feature detection would be preferable of course), on the other hand; not warning people that it may not be supported may also result in issues; perhaps a kernel check and a flag to override this check (like #24518)?

@justincormack
Copy link
Contributor

This seems reasonable, but yes we are in a difficult situation with a lot of issues. There are lots of combinations of versions that do not work, but cannot easily be detected. Kernel version checks are generally unreliable too, because of ports.

LGTM as stopping valid use of overlay seems problematic.

@cpuguy83
Copy link
Member

Maybe we can print a warning? We can say something like "Overlay storage driver selected with selinux enabled: only some newer kernels support this combination -- Ensure your kernel is up to date and supports selinux on overlay."

@rhatdan
Copy link
Contributor Author

rhatdan commented Aug 12, 2016

Thats fine, but seems more like a man page thing then a warning. Hopefully within a year this message will become mute.

@crosbymichael
Copy link
Contributor

Is there a way to test for functionality or does doing an overlay mount with labels not return an error when the kernel does not support it?

@rhatdan
Copy link
Contributor Author

rhatdan commented Aug 12, 2016

I don't think the mount command fails, but the files are not labeled correctly.

@crosbymichael
Copy link
Contributor

LGTM

@thaJeztah
Copy link
Member

@cpuguy83 PTAL if you agree; but don't merge yet, because this needs docs updates (reference docs, and at least have a note/warning in a couple of places); I'll point out the places that needs to be updated when we're in docs review

@thaJeztah thaJeztah added this to the 1.13.0 milestone Aug 12, 2016
@cpuguy83
Copy link
Member

LGTM, moving to docs.

@sfsmithcha
Copy link
Contributor

Waiting for @thaJeztah to identify docs updates.

@SvenDowideit
Copy link
Contributor

looks to me like the non-man page docs never mentioned the original restrictions, so

LGTM

@crosbymichael crosbymichael merged commit b42ab41 into moby:master Aug 31, 2016
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.

8 participants