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

Add service update to nginx-ingress-controller RBAC #815

Closed
wants to merge 3 commits into from

Conversation

jsulinski
Copy link

I believe this was missed from #266. @ankon points it out here: #266 (comment).

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

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 46.507% when pulling 4f36246 on federacy:master into 2829997 on kubernetes:master.

@nevetS
Copy link

nevetS commented Jun 5, 2017

Ahhh...

Following the provided link to this comment:
kubernetes-retired/contrib#766 (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.

@jsulinski
Copy link
Author

Absolutely! I'll make this change soon.

@jsulinski
Copy link
Author

Look good?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 46.522% when pulling af5a3a3 on federacy:master into 2829997 on kubernetes:master.

@@ -39,6 +39,7 @@ rules:
- get
- list
- watch
- update
Copy link
Contributor

@weitzj weitzj Jun 8, 2017

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)

@jsulinski
Copy link
Author

Certainly. Good catch, thank you.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 46.42% when pulling a64d828 on federacy:master into 2829997 on kubernetes:master.

@jonpulsifer
Copy link
Contributor

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.

@aledbf
Copy link
Member

aledbf commented Jun 13, 2017

Closing. This is not required now after #846

@aledbf aledbf closed this Jun 13, 2017
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