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

fix ingress rbac roles #806

Merged
merged 1 commit into from
Jun 2, 2017
Merged

fix ingress rbac roles #806

merged 1 commit into from
Jun 2, 2017

Conversation

puja108
Copy link
Member

@puja108 puja108 commented Jun 1, 2017

There was 2 things that the current IC (0.9 beta7) needs. (Note that in the pasted api server logs my SA is called nginx-ingress-controller and the IC is running in kube-system)

The ClusterRole was missing get nodes:

RBAC DENY: user "system:serviceaccount:kube-system:nginx-ingress-controller" groups [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] cannot "get" resource "nodes" named "xxx" cluster-wide

The Role was missing update configmaps:

RBAC DENY: user "system:serviceaccount:kube-system:nginx-ingress-controller" groups [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] cannot "update" resource "configmaps" named "ingress-controller-leader-nginx" in namespace "kube-system"

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 1, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c96758a on puja108:patch-1 into ** on kubernetes:master**.

@nevetS
Copy link

nevetS commented Jun 2, 2017

There are two changes here - one for configmaps - that part is obsoleted by PR 798 that was just merged.

The other is get for nodes.

@aledbf, @weitzj @liggitt - correct me if I'm wrong here, but if we just need get for nodes cluster-wide, we should separate out the permissions for nodes.

So instead of adding get to line 26, we'd want a whole new rule just for gets on nodes. We'll also want to update the readme to reflect the new permissions.

Of course, if I'm wrong and we need get for configmaps, endpoints, pods, and secrets cluster-wide this would be good if the configmaps update is removed and a readme update is in place.

@puja108 - thanks!

@@ -69,6 +70,7 @@ rules:
- secrets
verbs:
- get
- update
Copy link
Member

Choose a reason for hiding this comment

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

you only need access to update a single config map, the one used for the leader election, so that should be in a separate rule, with a resourceNames stanza listing the particular name of the config map you need to update

@liggitt
Copy link
Member

liggitt commented Jun 2, 2017

I just swept the code under the nginx package and didn't see any calls to get a node... am I missing that? It's not clear to me why the ingress controller would need that

@aledbf
Copy link
Member

aledbf commented Jun 2, 2017

I just swept the code under the nginx package and didn't see any calls to get a node... am I missing that?

@liggitt here https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/controller.go#L295

@puja108
Copy link
Member Author

puja108 commented Jun 2, 2017

Thanks for the input! As both comments above show (and I was thinking of that, too), we might want to make the roles tighter. I will move the update cm to a separate rule @liggitt thanks for the tip. As for the get nodes, I'll check #798 if it is tight enough and otherwise include the new rule in this PR.

@puja108
Copy link
Member Author

puja108 commented Jun 2, 2017

added the changes, will also update readme now

@puja108
Copy link
Member Author

puja108 commented Jun 2, 2017

Should be fixed now. RFR @nevetS @liggitt

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 46.465% when pulling b136ced on puja108:patch-1 into f148465 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.45% when pulling b136ced on puja108:patch-1 into f148465 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.45% when pulling b136ced on puja108:patch-1 into f148465 on kubernetes:master.

@nevetS
Copy link

nevetS commented Jun 2, 2017

This looks good to me. Please squash your commits so it can get merged.

Using gcr.io/google_containers/nginx-ingress-controller:0.9.0-beta.7
the nginx-controller needs to handle leader-election via configmaps.

To perform the leader-election the nginx-controller needs to have the
appropiate RBAC permissions.

Previously to this fix, the following errors occured:

-  cannot get configmaps in the namespace "NAMESPACE_PLACEHOLDER". (get configmaps ingress-controller-leader-nginx)
- initially creating leader election record: User "system:serviceaccount:NAMESPACE_PLACEHOLDER" cannot create configmaps in the namespace "NAMESPACE_PLACEHOLDER". (post configmaps)

fix ingress rbac roles

There was 2 things that the current IC (0.9 beta7) needs.

The ClusterRole was missing `get nodes`:

```
RBAC DENY: user "system:serviceaccount:kube-system:nginx-ingress-controller" groups [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] cannot "get" resource "nodes" named "xxx" cluster-wide
```

The Role was missing `update configmaps`:

```RBAC DENY: user "system:serviceaccount:kube-system:nginx-ingress-controller" groups [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] cannot "update" resource "configmaps" named "ingress-controller-leader-nginx" in namespace "kube-system"```

removed update configmap because of kubernetes#798

rebased on master, moved get nodes to own rule

added get nodes to cluster permissions
@puja108
Copy link
Member Author

puja108 commented Jun 2, 2017

sqashed

@aledbf
Copy link
Member

aledbf commented Jun 2, 2017

@puja108 thanks!

@aledbf aledbf merged commit 4c868cf into kubernetes:master Jun 2, 2017
@puja108 puja108 deleted the patch-1 branch June 2, 2017 16:03
@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.45% when pulling cf4ad26 on puja108:patch-1 into f148465 on kubernetes:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants