move kube-dns to a separate service account #38816

deads2k · 2016-12-15T14:56:26Z

Switches the kubedns addon to run as a separate service account so that we can subdivide RBAC permission for it. The RBAC permissions will need a little more refinement which I'm expecting to find in #38626 .

@cjcullen @kubernetes/sig-auth since this is directly related to enabling RBAC with subdivided permissions
@thockin @kubernetes/sig-network since this directly affects now kubedns is added.

`kube-dns` now runs using a separate `system:serviceaccount:kube-system:kube-dns` service account which is automatically bound to the correct RBAC permissions.

k8s-reviewable · 2016-12-15T14:56:33Z

This change is

MrHohn · 2016-12-15T19:44:53Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

+			// a role to use for the kube-dns pod
+			ObjectMeta: api.ObjectMeta{Name: "system:kube-dns"},
+			Rules: []rbac.PolicyRule{
+				rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("endpoints", "services").RuleOrDie(),


Would kube-dns have the permission to fetch ConfigMap in general with this setup? Fetching ConfigMap might not be a right permission for kube-dns but we introduced it (#36775) as a short term fix for the federation issue.

Would kube-dns have the permission to fetch ConfigMap in general with this setup? Fetching ConfigMap might not be a right permission for kube-dns but we introduced it (#36775) as a short term fix for the federation issue.

Like I've set it up here, no. I saw that access and assumed it was a bug. This role is a good starting point and we can have a separate pull to add questionable permissions.

If we get the optional config maps in 1.6, we won't need that permission, right?

If we get the optional config maps in 1.6, we won't need that permission, right?

The configmap permission could be done as a secondary, optional role attached to your service account. A cluster-admin could add it if he needed the feature. We've done that with a few permissions in openshift. There's a base role that works 90% of the time and if you need more power, you grant more power with a second role: opt-in.

davidopp · 2016-12-15T20:41:49Z

@deads2k Just for my edification, can you explain what RBAC with subdivided permissions means? (And how it related to this PR.) Or if there is something written up, can you point me to it?

deads2k · 2016-12-15T20:51:20Z

@deads2k Just for my edification, can you explain what RBAC with subdivided permissions means? (And how it related to this PR.) Or if there is something written up, can you point me to it?

RBAC is an authorizer described here: http://kubernetes.io/docs/admin/authorization/ . It allows you to control permissions based on roles (examples in the doc). By more tightly controlling roles for controllers and powerful components, we can prevent bugs or exploits from causing excessive damage. One of our requirements for beta here: https://docs.google.com/document/d/1woLGRoONE3EBVx-wTb4pvp4CI7tmLZ6lS26VTbosLKM/edit?ts=570fa5c5#heading=h.9kaxh998i32c is to tighten permissions on our components. I found this one by inspecting which subject was making requests without RBAC permissions.

In order to create a role and bind it to a proper subject, the subject running the kube-dns needs to separated from other components. This attempts to start that separation.

davidopp · 2016-12-15T20:53:54Z

So this can be used to reduce the power that stuff running in the context of the DNS pod has, in terms of operations on the Kubernetes API objects?

deads2k · 2016-12-15T21:17:15Z

So this can be used to reduce the power that stuff running in the context of the DNS pod has, in terms of operations on the Kubernetes API objects?

Correct.

thockin · 2016-12-15T21:27:18Z

I don't know how to do that, but I now know who to ask. For the time being DNS needs to be able to access configmaps.

…

On Thu, Dec 15, 2016 at 1:22 PM, David Eads ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go <#38816>: > @@ -229,6 +229,13 @@ func ClusterRoles() []rbac.ClusterRole { rbac.NewRule("create").Groups(authorizationGroup).Resources("subjectaccessreviews").RuleOrDie(), }, }, + { + // a role to use for the kube-dns pod + ObjectMeta: api.ObjectMeta{Name: "system:kube-dns"}, + Rules: []rbac.PolicyRule{ + rbac.NewRule("list", "watch").Groups(legacyGroup).Resources("endpoints", "services").RuleOrDie(), If we get the optional config maps in 1.6, we won't need that permission, right? The configmap permission could be done as a secondary, optional role attached to your service account. A cluster-admin could add it if he needed the feature. We've done that with a few permissions in openshift. There's a base role that works 90% of the time and if you need more power, you grant more power with a second role: opt-in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38816>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVHJwKmjFFY9fwlxySICHWzDsrOBuks5rIa-dgaJpZM4LOLx6> .

bowei · 2016-12-17T01:06:59Z

change looks ok to me modulo the configmap issue

liggitt · 2016-12-19T13:53:11Z

For the time being DNS needs to be able to access configmaps.

Rather than expanding the role to include permissions that don't make sense, I'd rather see the work done to support optional configmaps and/or addon tolerating existing objects. That is landing in 1.6, right?

deads2k · 2016-12-19T14:06:53Z

Rather than expanding the role to include permissions that don't make sense, I'd rather see the work done to support optional configmaps and/or addon tolerating existing objects. That is landing in 1.6, right?

The permissions are for 1.6. Knowing now what this is for, I agree that adding the permission is the wrong approach.

thockin · 2016-12-22T02:29:37Z

In the interim, DNS doesn't work on HEAD?

…

On Mon, Dec 19, 2016 at 6:07 AM, David Eads ***@***.***> wrote: Rather than expanding the role to include permissions that don't make sense, I'd rather see the work done to support optional configmaps and/or addon tolerating existing objects. That is landing in 1.6, right? The permissions are for 1.6. Knowing now what this is for, I agree that adding the permission is the wrong approach. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38816 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVE5HrfAzLguZUH8PLo6rqe-Byrw0ks5rJo-PgaJpZM4LOLx6> .

deads2k · 2016-12-22T13:26:32Z

In the interim, DNS doesn't work on HEAD?

I think so. We'd create a P1 bug and perhaps provide a manual workaround (adding a new role and binding), but expanding SA rights against the recommended way to use the platform isn't something I want to propagate in our bootstrap authorization rules.

bowei · 2016-12-22T18:30:06Z

This seems to be the wrong order of commits. I would prefer not to break HEAD due to a forward dependency on two features (optional configmaps + kube-dns support). Can this PR not wait until those features have been committed?

I am concerned about confusion involving kube-dns in the interim period. Anyone debugging issues would have to know about the breakage + fix.

thockin · 2016-12-22T18:31:24Z

+1. This PR can wait until the rest are done, then. We don't (knowingly) break head like this.

liggitt · 2016-12-22T18:34:02Z

I'd rather hold this PR then set up an overly broad role for kube DNS

liggitt · 2016-12-22T18:39:49Z

actually, isn't kube-dns broken in HEAD today with authorization enabled if it is assuming it has access to all configmaps? seems like the instructions to make kube-dns use a configmap could include granting authorization to read that configmap

deads2k · 2016-12-22T19:42:15Z

actually, isn't kube-dns broken in HEAD today with authorization enabled if it is assuming it has access to all configmaps? seems like the instructions to make kube-dns use a configmap could include granting authorization to read that configmap

It breaks as soon as you stop granting all permissions to service accounts, which is a permission that should definitely be removed. API security that equates the power to create a pod with root power against the API isn't very effective.

luxas · 2017-01-16T22:00:02Z

Any chance we could give kube-dns the configmap permission for now with a P1 issue that explains why and when we should do it the right way when we can? (I know, I know, long-term it's the wrong thing to do, but anyway?)

I think it was suggested already, but that's what I think should do, in order to get this merged.

liggitt · 2017-01-16T22:02:30Z

you can give it the configmap permission with a second namespaced role grant already:

kubectl create rolebinding kube-dns-configmap-binding \
  --clusterrole=view \
  --namespace=my-configmap-namespace \
  --serviceaccount=kube-system:kube-dns

I wouldn't want to add a global configmap permission to this role

luxas · 2017-01-16T22:05:19Z

@liggitt Yes, so why don't we do that for now along with an issue to completely remove that dep?

k8s-github-robot · 2017-01-24T00:17:20Z

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

We suggest the following people:
cc @zmerlynn @erictune
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k · 2017-02-16T20:50:04Z

These scripts are a nightmare. Somehow this pull is breaking either the authn or authz setup.

deads2k · 2017-02-16T20:58:37Z

cluster/gce/gci/configure-helper.sh

@@ -1195,8 +1195,10 @@ function start-kube-addons {
    setup-addon-manifests "addons" "dns"
    local -r dns_controller_file="${dst_dir}/dns/kubedns-controller.yaml"
    local -r dns_svc_file="${dst_dir}/dns/kubedns-svc.yaml"
+    local -r dns_sa_file="${dst_dir}/dns/kubedns-sa.yaml"


Alright, I'm betting I borked the add-on manager somehow. @cjcullen @ncdc @mikedanese anything obvious?

I think you don't need this line local -r dns_sa_file="${dst_dir}/dns/kubedns-sa.yaml", because setup-addon-manifests "addons" "dns" already copy all the files.

And it seems like "kube-addon-manager.yaml" is not copied on to the master, which means start_kube_addons() met some errors in the middle.

MrHohn · 2017-02-16T23:29:42Z

cluster/gce/gci/configure-helper.sh

    mv "${dst_dir}/dns/kubedns-controller.yaml.in" "${dns_controller_file}"
    mv "${dst_dir}/dns/kubedns-svc.yaml.in" "${dns_svc_file}"
+    mv "${dst_dir}/dns/kubedns-sa.yaml" "${dns_sa_file}"


I think you are moving "${dst_dir}/dns/kubedns-sa.yaml" into "${dst_dir}/dns/kubedns-sa.yaml", which are the same file, so it broke.

I think you are moving "${dst_dir}/dns/kubedns-sa.yaml" into "${dst_dir}/dns/kubedns-sa.yaml", which are the same file, so it broke.

Thanks!

luxas · 2017-02-17T14:50:13Z

@k8s-bot kops aws e2e test this

luxas · 2017-02-17T14:52:20Z

@deads2k Super cool to have this PR green!
Please fill in a relnote and I think @erictune and @bowei will merge this

liggitt · 2017-02-17T14:52:34Z

@deads2k I've seen some of the other addons include the service account in the same yaml file as a separate record... would remove the need for any of the script changes, and make it so anyone else who was using the add-on would get the change automatically.

bowei

/approve

luxas · 2017-02-19T10:09:58Z

@bowei approved this, added the label (/approve doesn't work in a review comment I think)

@liggitt @bowei @erictune Can we get this /lgtm'd as well?
It would be super-cool to get this into v1.6.0-alpha.4 which is cut on Monday

luxas · 2017-02-19T10:10:11Z

@deads2k please squash though

deads2k · 2017-02-20T12:37:43Z

Squashed. Didn't add the SA to the RC file. Is that a thing we want to do? Seems a little weird.

liggitt · 2017-02-20T16:20:29Z

Squashed. Didn't add the SA to the RC file. Is that a thing we want to do? Seems a little weird.

I don't feel strongly either way

liggitt · 2017-02-22T20:42:06Z

tagging per #38816 (review) #38816 (review)

k8s-github-robot · 2017-02-23T20:06:12Z

Automatic merge from submit-queue

@bowei

Automatic merge from submit-queue Moves dns-horizontal-autoscaler to a separate service account Similar to #38816. As one of the cluster add-ons, dns-horizontal-autoscaler is now using the default service account in kube-system namespace, which is introduced by https://github.com/kubernetes/kubernetes/blob/master/cluster/addons/e2e-rbac-bindings/random-addon-grabbag.yaml for the ease of transition. This default service account will be removed in the future. This PR subdivides dns-horizontal-autoscaler to a separate service account and setup the necessary permissions. @bowei **Release note**: ```release-note NONE ```

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 15, 2016

k8s-github-robot assigned erictune Dec 15, 2016

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 15, 2016

MrHohn reviewed Dec 15, 2016

View reviewed changes

thockin assigned bowei Dec 15, 2016

liggitt mentioned this pull request Jan 15, 2017

Add authorization mode to kubeadm #39846

Merged

This was referenced Jan 24, 2017

Add support for pulling config from a directory kubernetes/dns#39

Merged

Make kube-dns mount optional configmap #40382

Merged

deads2k force-pushed the rbac-23-switch-kubedns-sa branch from bfbdcf2 to 3a5b8fc Compare February 16, 2017 19:25

deads2k commented Feb 16, 2017

View reviewed changes

MrHohn reviewed Feb 16, 2017

View reviewed changes

luxas added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Feb 17, 2017

bowei approved these changes Feb 17, 2017

View reviewed changes

luxas added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2017

liggitt added this to the v1.6 milestone Feb 19, 2017

move kube-dns to a separate service account

36b586d

deads2k force-pushed the rbac-23-switch-kubedns-sa branch from 4de64df to 36b586d Compare February 20, 2017 12:37

liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2017

k8s-github-robot merged commit b799bbf into kubernetes:master Feb 23, 2017

This was referenced Mar 4, 2017

token flag shouldn't had been removed kubernetes/kubeadm#158

Closed

On node join, validate that the minimum server version is v1.6.0-alpha.1 kubernetes/kubeadm#152

Closed

MrHohn mentioned this pull request Mar 7, 2017

Moves dns-horizontal-autoscaler to a separate service account #42617

Merged

deads2k deleted the rbac-23-switch-kubedns-sa branch March 7, 2017 20:05

This was referenced Nov 8, 2017

RBAC is broken kubernetes/minikube#1734

Closed

kube-dns and RBAC on Minikube still broken?? jetstack/navigator#107

Closed

liggitt mentioned this pull request Mar 16, 2018

Add missing configmaps resources for kube-dns #61291

Closed

move kube-dns to a separate service account #38816

move kube-dns to a separate service account #38816

Conversation

deads2k commented Dec 15, 2016 • edited

k8s-reviewable commented Dec 15, 2016

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

davidopp commented Dec 15, 2016

deads2k commented Dec 15, 2016

davidopp commented Dec 15, 2016

deads2k commented Dec 15, 2016

thockin commented Dec 15, 2016 via email

bowei commented Dec 17, 2016 • edited

liggitt commented Dec 19, 2016

deads2k commented Dec 19, 2016

thockin commented Dec 22, 2016 via email

deads2k commented Dec 22, 2016

bowei commented Dec 22, 2016

thockin commented Dec 22, 2016

liggitt commented Dec 22, 2016

liggitt commented Dec 22, 2016

deads2k commented Dec 22, 2016

luxas commented Jan 16, 2017

liggitt commented Jan 16, 2017 • edited

luxas commented Jan 16, 2017

k8s-github-robot commented Jan 24, 2017

deads2k commented Feb 16, 2017

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

luxas commented Feb 17, 2017

luxas commented Feb 17, 2017

liggitt commented Feb 17, 2017 • edited

bowei left a comment

Choose a reason for hiding this comment

luxas commented Feb 19, 2017

luxas commented Feb 19, 2017

deads2k commented Feb 20, 2017

liggitt commented Feb 20, 2017

liggitt commented Feb 22, 2017

k8s-github-robot commented Feb 23, 2017

deads2k commented Dec 15, 2016 •

edited

bowei commented Dec 17, 2016 •

edited

liggitt commented Jan 16, 2017 •

edited

liggitt commented Feb 17, 2017 •

edited