Fix secrets in kubelet running in a container #13791

fgrzadkowski · 2015-09-10T12:32:47Z

Introduce writer interface that could be used by volume plugins. This PR provides two implementations:

standard implementation that uses golang standard libraries
implementation that uses nsenter to write data in host mount namespace.

@smarterclayton (who wrote nsenter_mounter.go which is similar)
@dchen1107 (as this touches kubelet and volume plugins)
@dalanlan @resouer (who are interested in running kubernetes in containers)
@brendandburns (who is interested in cluster set up UX)

Ref #4869

k8s-bot · 2015-09-10T12:33:49Z

GCE e2e build/test failed for commit 84d36268ea2a4d13b7152cf969c76bc1c9055c40.

dalanlan · 2015-09-10T12:42:09Z

Quick question.
Currently nsenter mounter runs based on the content of "/rootfs", which is the mount path of root filesystem of host. Do you think it's a good idea to make it configurable? It might touch the whole mounter interface though, i'm not sure:)

ncdc · 2015-09-10T13:12:22Z

@pmorie @kubernetes/rh-cluster-infra @kubernetes/rh-storage

k8s-bot · 2015-09-10T13:13:41Z

GCE e2e build/test failed for commit e20ddaeea138ef415da366b70c548de0d000a4ea.

fgrzadkowski · 2015-09-10T13:19:22Z

sorry for few iterations with failing tests but I couldn't merge it properly with head

k8s-bot · 2015-09-10T13:19:57Z

GCE e2e build/test passed for commit 1d3a2f557fb9c9565f3ef85d67837b2d52532e50.

markturansky · 2015-09-10T13:20:16Z

Any change to volumes often explodes into every plugin being changed. It makes this PR huge, though.

Any way to peel off smaller PRs for easier review?

fgrzadkowski · 2015-09-10T13:21:32Z

I think it'd be hard, because the part that spans all plugins is changing the API that they implement.

markturansky · 2015-09-10T13:23:18Z

I copied the labels from the linked issue to this PR.

k8s-bot · 2015-09-10T13:44:52Z

GCE e2e build/test passed for commit 51f192d830f3020f97fa49ca3252241e870f477d.

pmorie · 2015-09-11T04:33:00Z

pkg/volume/plugins.go

@@ -42,6 +42,13 @@ type VolumeOptions struct {
 	RootContext string
 }

+// System is an interface that abstracts certain operations on a host operating
+// system that a volume might want to execute.
+type System interface {


@markturansky This is really the thing that makes this PR so big.

@fgrzadkowski
I think this is a good abstraction to introduce, but I'm still deciding whether I like this name or not.

Any thoughts about naming here, @thockin ?

I think you misspelled Host.GetMounter() and Host.WriteFile()

In all seriousness, the VolumeHost is provided to plugins as THE way to interface with the app that hosts the plugin. I do not think we should be injecting more objects from the host's plane into the plugin's plane.

To be honest I just followed the same path as mount interface. When reading Host interface I was under the impression that it's supposed to be used only to talk to Kubelet not the host machine itself. Do you suggest moving both interfaces to Host?

Host is your gateway to the rest of the app (think of it as if the plugin was a .so file). The host application provides an implmentation of Mounter and a function to write files in the filesystem

clearer: yes, I think you don't need a system object, you need to add either one method WriteFile or to remove the mount.Interface that is passed around and add two methods WriteFile and GetMounter. The latter is probably cleaner overall, but more work.

Done. Added GetMounter() andGetWritertoVolumeHost. I'm usingGetWriter()instead ofWriteFile()``` so that it will be easier to extend it to other operations (like symbolic links which are used in downward API plugin).

I'm fine with that, though I bet Writer() becomes ReaderWriter() eventually

pmorie · 2015-09-11T04:43:59Z

@fgrzadkowski

For context, we carry a patch in the Red Hat distributions for docker that makes the container's mount namespace propagation mode to SLAVE so that the container 'sees' the mountpoints that the kubelet makes and makes it so that when the kubelet writes secret data into the volume directory, it's written to the mointpoint that the kubelet created in the host's mount namespace, which is why I didn't need to implement something like this when I wrote nsenter_mount.go.

Ultimately you should be able to just set the propagation mode correctly for the container the kubelet runs in instead of having to do this. I think the idea behind this workaround is good. One concern that I have is that it's a lot of work that solves only a single facet of the problem. For example, this won't make git repo volumes work or the downward API volume work. Have you thought about those cases at all? I have no problem changing the signature of the plugins as you have in this PR, but I would like to ensure that we have the other related aspects of this problem solved out or at least considered. Any thoughts about that?

thockin · 2015-09-11T04:44:37Z

pkg/util/writer.go

+type DefaultWriter struct {
+}
+
+func NewDefaultWriter() Writer {


why a constructor that does nothing?

To be consistent with mounter.

fgrzadkowski · 2015-09-11T07:39:03Z

@pmorie I'm aware of the RH patch to docker. There is a docker PR (moby/moby#15648) that should solve it properly. It should be shipped in docker 1.9. However I don't think we can wait for it as we will have to support older docker version for quite some time. At the same time we agreed that docker based setup is supposed to be the default solution for new platforms/OSes so the priority is rather high.

Regarding git_repo plugin I think it should work because it doesn't use mounts - it just creates regular directory that will be populated.
I agree we will still have problem with downward API plugin as it uses tmpfs based empty dir plugin, which means it uses mounts. I didn't look carefully, but my understanding is that we will be able to fix it similarly to secrets (in the end it also writes files to tmpfs). Also I think that fixing secrets is more important as it's a pre-requisite for service accounts.

pmorie · 2015-09-11T19:50:55Z

@fgrzadkowski Good about re: the other plugins, it was too late and I was wrong 👍

pmorie · 2015-09-11T19:51:03Z

@fgrzadkowski rebase needed

k8s-bot · 2015-09-17T14:37:45Z

GCE e2e build/test failed for commit f78e6f59a402fb90daa1df8fd1bdf354fadd601b.

fgrzadkowski · 2015-09-17T14:40:20Z

@pmorie Sorry for the noise. It seems I didn't commit the changes locally.

k8s-bot · 2015-09-17T15:01:15Z

GCE e2e build/test passed for commit 7fe34f2.

pmorie · 2015-09-17T15:32:49Z

@fgrzadkowski np, it happens

k8s-github-robot · 2015-09-17T15:36:36Z

LGTM was before last commit, removing LGTM

fgrzadkowski · 2015-09-17T17:20:29Z

@pmorie It seems it will need LGTM again, not that it's rebased.

fgrzadkowski · 2015-09-17T20:26:28Z

@pmorie I'm sorry for nagging you, but I'm worried I'll have to rebase again if we don't merge it soon. Can you please re-add LGTM label?

pmorie · 2015-09-17T20:29:49Z

@fgrzadkowski NP, added.

k8s-github-robot · 2015-09-17T21:55:43Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2015-09-17T22:13:22Z

GCE e2e build/test passed for commit 7fe34f2.

k8s-github-robot · 2015-09-17T22:13:29Z

Automatic merge from SubmitQueue

Auto commit by PR queue bot

pmorie · 2015-09-18T02:53:43Z

@fgrzadkowski Thanks again for the fix.

sttts · 2015-09-18T11:55:22Z

This broke Kubernetes on Mesos. The fix is in #14169

…docker Auto commit by PR queue bot

googlebot added the cla: yes label Sep 10, 2015

fgrzadkowski force-pushed the fix_secrets_in_docker branch 2 times, most recently from 1d3a2f5 to e20ddae Compare September 10, 2015 13:12

fgrzadkowski force-pushed the fix_secrets_in_docker branch from e20ddae to 51f192d Compare September 10, 2015 13:16

markturansky added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 10, 2015

markturansky added area/platform/local priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 10, 2015

mikedanese assigned pmorie Sep 10, 2015

pmorie reviewed Sep 11, 2015
View reviewed changes

thockin reviewed Sep 11, 2015
View reviewed changes

Introduce nsenter writer for volume plugins

7fe34f2

fgrzadkowski force-pushed the fix_secrets_in_docker branch from f78e6f5 to 7fe34f2 Compare September 17, 2015 14:40

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2015

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2015

pmorie added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2015

k8s-github-robot pushed a commit that referenced this pull request Sep 17, 2015

Merge pull request #13791 from fgrzadkowski/fix_secrets_in_docker

c1eb1a1

Auto commit by PR queue bot

k8s-github-robot merged commit c1eb1a1 into kubernetes:master Sep 17, 2015

sttts mentioned this pull request Sep 18, 2015

MESOS: Fix missing volume writer in executor kubelet instantiation #14169

Merged

dalanlan mentioned this pull request Oct 31, 2015

Volumes Not Mounting in Dockerized K8S #16055

Closed

guybrush mentioned this pull request Nov 7, 2015

[Proposal] kubernetes cluster management using docker containers #13424

Closed

5 tasks

dalanlan mentioned this pull request Nov 7, 2015

containerized controller manager doesn't generate secrets w/ default configuration #16965

Closed

luxas mentioned this pull request Nov 13, 2015

Enable serviceaccounts for the docker-multinode deployment #17213

Merged

brendandburns mentioned this pull request Nov 14, 2015

Introduce nsenter writer for volume plugins #17273

Closed

luxas mentioned this pull request Nov 19, 2015

mounting secrets when running containerized kubelet only works with dash as /bin/sh #17522

Closed

mkantor mentioned this pull request Dec 2, 2015

Does not support kubernetes secrets vyshane/docker-compose-kubernetes#9

Closed

sebgoa mentioned this pull request Dec 22, 2015

Containerized cluster cannot mount default secrets #18239

Closed

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

Merge pull request kubernetes#13791 from fgrzadkowski/fix_secrets_in_…

d67bb46

…docker Auto commit by PR queue bot

Fix secrets in kubelet running in a container #13791

Fix secrets in kubelet running in a container #13791

Conversation

fgrzadkowski commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

dalanlan commented Sep 10, 2015

ncdc commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

fgrzadkowski commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

markturansky commented Sep 10, 2015

fgrzadkowski commented Sep 10, 2015

markturansky commented Sep 10, 2015

k8s-bot commented Sep 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Sep 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgrzadkowski commented Sep 11, 2015

pmorie commented Sep 11, 2015

pmorie commented Sep 11, 2015

k8s-bot commented Sep 17, 2015

fgrzadkowski commented Sep 17, 2015

k8s-bot commented Sep 17, 2015

pmorie commented Sep 17, 2015

k8s-github-robot commented Sep 17, 2015

fgrzadkowski commented Sep 17, 2015

fgrzadkowski commented Sep 17, 2015

pmorie commented Sep 17, 2015

k8s-github-robot commented Sep 17, 2015

k8s-bot commented Sep 17, 2015

k8s-github-robot commented Sep 17, 2015

pmorie commented Sep 18, 2015

sttts commented Sep 18, 2015