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

Enable privileged containers for apiserver and controller #57561

Merged
merged 2 commits into from Jan 18, 2018

Conversation

@dims
Member

dims commented Dec 22, 2017

What this PR does / why we need it:

In OpenStack environment, when there is no metadata service, we
look at the config drive to figure out the metadata. Since we need
to run commands like blkid, we need to ensure that api server and
kube controller are running in the privileged mode.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #47392
Fixes kubernetes/kubeadm#588

Special notes for your reviewer:

Release note:

Fix issue when using OpenStack config drive for node metadata
@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

/assign @luxas

@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

/retest

@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

/open

@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

/reopen

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 22, 2017

@dims: failed to re-open PR: state cannot be changed. There are no new commits on the dims:enable-privileged-container-for-apiserver-and-controller branch.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

/reopen

@kad

This comment has been minimized.

Member

kad commented Dec 22, 2017

Two questions: 1. unit tests. 2. can it be done with allowing few needed caps instead of fully privileged ?

@dims

This comment has been minimized.

Member

dims commented Dec 22, 2017

@kad we need CAP_SYS_ADMIN as we are mounting/unmounting file systems (similar to http://ceph.com/planet/no-more-privileged-containers-for-ceph-osds/). Any suggestions on the best way to do this?

Thanks for the quick review!

@kad

This comment has been minimized.

Member

kad commented Dec 22, 2017

In case of mounts, probably no suggestions from me at the moment. If CAP_SYS_ADMIN is enough, maybe that would be the best way.

@dims

This comment has been minimized.

Member

dims commented Dec 23, 2017

@kad so leave the current implementation as-is?

@kad

This comment has been minimized.

Member

kad commented Dec 27, 2017

@dims test case still will be good. so if someone will do refactoring on that code later, we will not loose it.
@liggitt what do you think about security/privileged pod for api manager and controller manager for that usage scenario ?

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Dec 27, 2017

@kad

This comment has been minimized.

Member

kad commented Dec 28, 2017

lgtm, however it will be great if @liggit or somebody else from k8s security specialists comment on privileged mode.

@@ -104,6 +104,16 @@ func GetStaticPodSpecs(cfg *kubeadmapi.MasterConfiguration, k8sVersion *version.
}, mounts.GetVolumes(kubeadmconstants.KubeScheduler)),
}
if cfg.CloudProvider == "openstack" {

This comment has been minimized.

@liggitt

liggitt Dec 28, 2017

Member

Is this the first cloud provider specific things kubeadm is doing? I would like to see the plan for this going forward… this is just moving compiled-in logic up a level, when what we really want is to enable external integration of cloud provider specific modifications.

This comment has been minimized.

This comment has been minimized.

@liggitt

liggitt Dec 28, 2017

Member

Interesting. The linked example seems like a bug… admission plugins don't depend solely on cloud provider… seems like specifying cloud provider aws with kube 1.8 would not work with that code

if cfg.CloudProvider == "openstack" {
True := true
staticPodSpecs[kubeadmconstants.KubeAPIServer].Spec.Containers[0].SecurityContext = &v1.SecurityContext{
Privileged: &True,

This comment has been minimized.

This comment has been minimized.

@dims

dims Jan 2, 2018

Member

Done!

@dims

This comment has been minimized.

Member

dims commented Jan 5, 2018

/test pull-kubernetes-node-e2e

@mikedanese mikedanese removed their assignment Jan 9, 2018

@dims

This comment has been minimized.

Member

dims commented Jan 10, 2018

@jamiehannaford

I'd also like if we could reduce the permission scope if possible. If it's only possible with privileged, fine with me

@@ -104,6 +105,15 @@ func GetStaticPodSpecs(cfg *kubeadmapi.MasterConfiguration, k8sVersion *version.
}, mounts.GetVolumes(kubeadmconstants.KubeScheduler)),
}
if cfg.CloudProvider == "openstack" {

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 12, 2018

Member

might be worth making openstack a constant and also referencing different providers in the unit tests

This comment has been minimized.

@dims

dims Jan 12, 2018

Member

@jamiehannaford i see "aws" and "gce" as well. i'll do it in another PR after this one merges

This comment has been minimized.

@jamiehannaford

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

I'd rather this be more generic enablement, b/c this isn't openstack specific. This is a specific os-deployment issue.

This comment has been minimized.

@dims

dims Jan 17, 2018

Member

Should i remove the if condition for openstack?

This comment has been minimized.

@jamiehannaford

jamiehannaford Jan 17, 2018

Member

Do AWS and GCE also use the config drive for node metadata?

This comment has been minimized.

@dims

dims Jan 17, 2018

Member

I don't know for sure @jamiehannaford

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

@dims I'd prefer an opt-in knob to enable privileged (default false), b/c again, this is very specific and not necessarily the common case.

This comment has been minimized.

@jbeda

jbeda Jan 17, 2018

Contributor

Agree with a specific opt-in knob. I'd consider it acceptable to have documentation for that knob state that often times cloud providers need it to be set and name openstack specifically.

This comment has been minimized.

@jbeda

jbeda Jan 17, 2018

Contributor

Agree with a specific opt-in knob. I'd consider it acceptable to have documentation for that knob state that often times cloud providers need it to be set and name openstack specifically.

Privileged: utilpointer.BoolPtr(true),
}
staticPodSpecs[kubeadmconstants.KubeControllerManager].Spec.Containers[0].SecurityContext = &v1.SecurityContext{
Privileged: utilpointer.BoolPtr(true),

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

Why does the controller manager need to talk with the meta-data service?

This comment has been minimized.

@dims

dims Jan 17, 2018

Member

when the cloud provider is looking for "which node am i" info, in this specific scenario there is no openstack metadata service and the code looks in the config drive, to be able to mount the config drive it needs privs. we have a search-order (https://kubernetes.io/docs/concepts/cluster-administration/cloud-providers/) but in this case there is simply no metadata service and the only choice is the config service where it fails because the kube controller manager does not have enough super powers

This comment has been minimized.

@kargakis

kargakis Jan 17, 2018

Member

It would be nice to put this down into a code comment.

This comment has been minimized.

@dims

dims Jan 17, 2018

Member

@kargakis ack, if this looks ok otherwise, will happily do that in a little bit

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

Once the cloud-controller code is excised from the controller manager, is there any reason that we would still need this?

This comment has been minimized.

@dims

dims Jan 17, 2018

Member

@timothysc correct, when we excise code that calls out to openstack cloud provider from these 2 containers we can yank the privilege flag.

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

When is the ETA on that excise? 1.10, 1.11? "Hand-wavy" future?

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

Also, would #58080 work for this use case?

This comment has been minimized.

@dims

dims Jan 18, 2018

Member

@timothysc we con't currently expose --cloud-provider from kubeadm command line, so that won't help. i've updated the PR with a new field in MasterConfiguration and updated the unit test. hopefully that combination will work for everyone.

This comment has been minimized.

@dims

dims Jan 18, 2018

Member

@timothysc For 1.10, i do not expect to move at least the openstack cloud provider out of tree. at best we will have an external openstack controller manager that will work for some scenarios with the in-tree one still being the primary one. 1.11 is a distinct possibility.

@@ -104,6 +105,15 @@ func GetStaticPodSpecs(cfg *kubeadmapi.MasterConfiguration, k8sVersion *version.
}, mounts.GetVolumes(kubeadmconstants.KubeScheduler)),
}
if cfg.CloudProvider == "openstack" {

This comment has been minimized.

@timothysc

timothysc Jan 17, 2018

Member

I'd rather this be more generic enablement, b/c this isn't openstack specific. This is a specific os-deployment issue.

@dims

This comment has been minimized.

Member

dims commented Jan 18, 2018

/test pull-kubernetes-unit

@dims

This comment has been minimized.

Member

dims commented Jan 18, 2018

/retest

@timothysc

minor nit on extraneous shufflings, then lgtm

NodeName string
AuthorizationModes []string
// Name of the cloud provider
CloudProvider string

This comment has been minimized.

@timothysc

timothysc Jan 18, 2018

Member

The CloudProvider strings don't really need to be shuffled around, it makes it look like you're changing it for some reason.

This comment has been minimized.

@dims

dims Jan 18, 2018

Member

Fixed. thanks!

@timothysc

/lgtm
/approve

dims added some commits Dec 22, 2017

Enable privileged containers for apiserver and controller
In OpenStack environment, when there is no metadata service, we
look at the config drive to figure out the metadata. Since we need
to run commands like blkid, we need to ensure that api server and
kube controller are running in the privileged mode.

So add a new field in MasterConfiguration for specifying that the
api server and controller manager (s) need extra privileges. Added
a TODO to remove this code when we fully yank out cloud provider
specific calls from these processes.

@k8s-merge-robot k8s-merge-robot removed the lgtm label Jan 18, 2018

@dims

This comment has been minimized.

Member

dims commented Jan 18, 2018

@timothysc apologies, looks like i left one test broken, fixed it now. will ping when everything goes green.

@timothysc

This comment has been minimized.

Member

timothysc commented Jan 18, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 18, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 18, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, timothysc

Associated issue: #47392

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 18, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Jan 18, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit afd01c0 into kubernetes:master Jan 18, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce
Details
cla/linuxfoundation dims authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke-gci Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@bputt

This comment has been minimized.

bputt commented Jan 19, 2018

Will this be in a 1.9.x release?

@errordeveloper

This comment has been minimized.

Member

errordeveloper commented Jan 22, 2018

@bputt I think this will land in 1.10 and won't get cherry-picked into 1.9.

@dims

This comment has been minimized.

Member

dims commented Jan 22, 2018

@errordeveloper @bputt Ilya is correct, this will fall into the new feature category (not bug fix)

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