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

Pod level SELinux context and volumes #14192

Merged
merged 1 commit into from Oct 5, 2015

Conversation

pmorie
Copy link
Member

@pmorie pmorie commented Sep 18, 2015

Splitting from #12944; this proposal covers just SELinux and generic label management for volumes.

Next step is discussion of API backward compatibility issues.

@pweil- @swagiaal @smarterclayton @childsb @wattsteve

Review on Reviewable

@k8s-bot
Copy link

k8s-bot commented Sep 18, 2015

GCE e2e build/test passed for commit 73cb59af1d516e0111e1036906ed88e38c06d35f.

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 18, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/L

@pmorie
Copy link
Member Author

pmorie commented Sep 19, 2015

@rootfs
Copy link
Contributor

rootfs commented Sep 21, 2015

I think hostPath is in the bag. If that is done, and we are able to transform the remote storage to hostPath via loopback (assuming we are able to solve the performance problem), would that help slim the matrix? Some remote volume are not able to read SELinux labels.

@pmorie
Copy link
Member Author

pmorie commented Sep 21, 2015

@rootfs Is what you're proposing only for shared storage? Remote block devices should support writing SELinux xattrs just fine.

@smarterclayton
Copy link
Contributor

Needs backcompat statement filled out.

@pmorie
Copy link
Member Author

pmorie commented Sep 21, 2015

@smarterclayton Oh yes, I'm aware :) That's next.

On Mon, Sep 21, 2015 at 12:14 PM, Clayton Coleman notifications@github.com
wrote:

Needs backcompat statement filled out.


Reply to this email directly or view it on GitHub
#14192 (comment)
.

@brendandburns
Copy link
Contributor

switching review to @smarterclayton since he has SELinux context that I lack.

@smarterclayton
Copy link
Contributor

I agree with all of this, but I would like @thockin to once over because of general volume design and @bgrant0607 to ack the API change that I believe he was ok with in the parent issue.


#### API backward compatibility

TODO
Copy link
Member

Choose a reason for hiding this comment

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

???

Copy link
Member Author

Choose a reason for hiding this comment

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

@bgrant0607 Also on my list for today, this depended on getting the story right with #12823 before it could be done without wasting time. The story I'm going to write today is much simpler than the one I would have written when the TODO went in.

@k8s-bot
Copy link

k8s-bot commented Sep 25, 2015

GCE e2e build/test passed for commit 92466181f93adc582df2ed77848eb091cd93aae7.

@pmorie
Copy link
Member Author

pmorie commented Sep 25, 2015

@thockin Most of your feedback should be addressed mod block devices in read-only mode.

@pmorie
Copy link
Member Author

pmorie commented Sep 25, 2015

@swagiaal @pweil- @rootfs @childsb @wattsteve

@k8s-bot
Copy link

k8s-bot commented Sep 25, 2015

Unit, integration and GCE e2e test build/test passed for commit b2e967e.

@thockin
Copy link
Member

thockin commented Sep 25, 2015

Travis says: run hack/update-generated-docs.sh


Kubernetes volumes can be divided into two broad categories:

1. Unshared storage:
Copy link
Member

Choose a reason for hiding this comment

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

This taxonomy is really the same as the PV taxonomy: RWO, ROX, RWX

Specifically your 1.2 is a lie. Most block devices support single-writer or multi-reader modes

Copy link
Member Author

Choose a reason for hiding this comment

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

ack; I'll rewrite.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thockin So, is it really that 1.2 needs to be split into:

  1. Use of network block devices without using PVs
  2. Use of network block devices with PVs in ReadWriteOnce mode
  3. Use of network block devices with PVs in *Many modes

I think (1) and (2) go where 1.2 is now, and (3) should be another bullet in 'shared storage'

@markturansky @wattsteve @childsb @swagiaal

Copy link

Choose a reason for hiding this comment

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

I think the use of "Unshared" vs "Shared" is confusing here. The real dividing line is "Volumes which support client side chcon" and "volumes which do not"

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree, @swagiaal. For example, in the use-case where you have a network block device being used via a PV in ReadOnlyMany mode, I don't think you want to do the chcon, even though the block device supports it. I think that falls into the same category as NFS, where you can't really do the chcon from the client side (due to the security risk you take on) even if you're in ReadWriteOnce PV mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

To expand a little bit more, if you're using a block device-based volume that supports chcon in one of the Many PV modes, I think you have problems similar to 'shared' FSs like NFS, etc:

  1. Which context is the right one to use?
  2. How should manage the context?

I'm inclined to say the right point at which to solve the above questions is when such a volume is provisioned rather than when it is mounted into a pod.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm leaning towards rewriting this section when I make a pass back through this doc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wattsteve @childsb Opinions welcome, since this recasts the framework we've been talking about volumes in terms of slightly. PTAL

Copy link
Member Author

Choose a reason for hiding this comment

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

@swagiaal For the record, I didn't disagree about being confusing, only on how you were partitioning the space of volumes/FSs.

Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should lump the 1 write multiple read use case into the shared storage category and fully address it in the next iteration.

@k8s-bot
Copy link

k8s-bot commented Sep 26, 2015

Unit, integration and GCE e2e build/test failed for commit dd2a3bfcd35f3bf060cafad6d651c2b31a4ad0f5.

@pmorie
Copy link
Member Author

pmorie commented Sep 30, 2015

@thockin I think this should be ready for review now. How about I owe you one beer per PR?*

*terms and conditions apply

@k8s-bot
Copy link

k8s-bot commented Oct 1, 2015

Unit, integration and GCE e2e build/test failed for commit f37423e4690e4c46e6addf77c64ffef53e4960ca.

@k8s-bot
Copy link

k8s-bot commented Oct 1, 2015

Unit, integration and GCE e2e test build/test passed for commit 7f049b7.

@sdminonne
Copy link
Contributor

cc @EricMountain-1A

### Kubernetes


There is a [proposed change](https://github.com/GoogleCloudPlatform/kubernetes/pull/9844) to the
Copy link
Member

Choose a reason for hiding this comment

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

does this mean we close #9844?

Copy link
Member Author

Choose a reason for hiding this comment

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

Already closed!

@thockin thockin added lgtm "Looks good to me", indicates that a PR is ready to be merged. e2e-not-required labels Oct 2, 2015
@thockin
Copy link
Member

thockin commented Oct 2, 2015

L
G
T
M

@pmorie
Copy link
Member Author

pmorie commented Oct 2, 2015

Thanks a lot @thockin

@smarterclayton
Copy link
Contributor

+1 for that

On Oct 2, 2015, at 2:36 PM, Paul Morie notifications@github.com wrote:

Thanks a lot @thockin https://github.com/thockin


Reply to this email directly or view it on GitHub
#14192 (comment)
.

@pmorie pmorie changed the title WIP: pod level SELinux context and volumes Pod level SELinux context and volumes Oct 5, 2015
@pmorie
Copy link
Member Author

pmorie commented Oct 5, 2015

@k8s-oncall

a-robinson added a commit that referenced this pull request Oct 5, 2015
Pod level SELinux context and volumes
@a-robinson a-robinson merged commit a81545d into kubernetes:master Oct 5, 2015
@pmorie pmorie mentioned this pull request Oct 19, 2015
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Pod level SELinux context and volumes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet