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 #798 - RBAC for leader election #804

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Conversation

weitzj
Copy link
Contributor

@weitzj weitzj commented Jun 1, 2017

Fixes: #798

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)

And another error message concerning update (I do not have the logs any more)

Related to:

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

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

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 1, 2017
@weitzj
Copy link
Contributor Author

weitzj commented Jun 1, 2017

I have now signed the agreement.

@weitzj
Copy link
Contributor Author

weitzj commented Jun 1, 2017

@aledbf @nevetS It would be nice if you could review this PR.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 1ff0191 on weitzj:fix/798 into ** on kubernetes:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling cfef998 on weitzj:fix/798 into ** on kubernetes:master**.

@aledbf
Copy link
Member

aledbf commented Jun 1, 2017

@weitzj this LGTM but I think you should add a comment indicating that the name of the configmap can change if the flag --election-id and --ingress-class are specified
The default value is <ingress-controller-leader>-<nginx>

https://github.com/kubernetes/ingress/blob/master/core/pkg/ingress/controller/launch.go#L87

@weitzj
Copy link
Contributor Author

weitzj commented Jun 1, 2017

@aledbf I did add a comment and updated the README. If this is fine with you, I will squash my branch and force-push.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling b84c9b4 on weitzj:fix/798 into ** on kubernetes:master**.

@aledbf
Copy link
Member

aledbf commented Jun 1, 2017

@weitzj please squash so we can merge this PR. Thanks!

@aledbf aledbf self-assigned this Jun 1, 2017
@@ -64,14 +64,27 @@ rules:
- apiGroups:
- ""
resources:
- configmaps
Copy link

Choose a reason for hiding this comment

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

If nginx is configured with a configmap for custom configuration we need to be able to read that, too.

Copy link
Contributor Author

@weitzj weitzj Jun 1, 2017

Choose a reason for hiding this comment

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

Ok. Then I will readd the old get for configmap as was present before this PR and just add the new readwrite configmap for the resourceName

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)
@weitzj
Copy link
Contributor Author

weitzj commented Jun 1, 2017

  • I have reverted the deletion of configmaps:get in the yaml as well as REAMD.md (as suggested by @abh )
  • @aledbf The branch was squashed and force-pushed.
  • Related PR, which might be obsolete when this PR gets merged: fix ingress rbac roles #806 (unless one needs a ClusterRole instead of a Role)

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 3ad6fa8 on weitzj:fix/798 into ** on kubernetes:master**.

@aledbf
Copy link
Member

aledbf commented Jun 2, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017
@aledbf
Copy link
Member

aledbf commented Jun 2, 2017

@weitzj thanks!

@aledbf aledbf merged commit f148465 into kubernetes:master Jun 2, 2017
@nevetS
Copy link

nevetS commented Jun 2, 2017

Fantastic! Sorry to take so long to get here. @aledbf moves quickly. @weitzj - much appreciated.

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. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nginx: RBAC for leader election
7 participants