-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add service update to nginx-ingress-controller RBAC #815
Conversation
Ahhh... Following the provided link to this comment: When using named ports, the ingress controller will add an annotation to the service. That's why this permission is required. Cluster wide permission is required because the services can be in any namespace. Could you also update the Readme file to reflect this permission change. It would be helpful if a comment was added that update permissions for services are only required when using named ports so that an annotation can be added to the service. That way an end user following the example can understand why it's needed. |
Absolutely! I'll make this change soon. |
Look good? |
@@ -39,6 +39,7 @@ rules: | |||
- get | |||
- list | |||
- watch | |||
- update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you maybe use alphabetical order here? (Since this is probably the only order many people could agree upon and keeping potential diffs lean)
Certainly. Good catch, thank you. |
I don't know about this. Looks like #749 removed the need for that permission. There was discussion on this in #747 (comment) as well. |
Closing. This is not required now after #846 |
I believe this was missed from #266. @ankon points it out here: #266 (comment).