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 note to Kubernetes RBAC docs about RoleBindings and namespaces #2498

Merged
merged 5 commits into from
Dec 5, 2017

Conversation

jmara
Copy link
Contributor

@jmara jmara commented Nov 30, 2017

What does this PR do?

I've updated the documentation to give the user a hint that Rolebinding per namespace in only available in version 1.5+

Motivation

Spend an hour digging through Github Issues/PRs to find that the changes how Traefik uses the kubernetes api (namespaves) was not merged into the 1.4 branch. The current docs lead to the impression that this (RoleBinding instead of ClusterRoleBinding) should just work out of the box.

Please see #1626 and #1895

@timoreimann
Copy link
Contributor

Thanks a lot! Could you rebase your PR against the v1.5 branch? That way, we can ensure the upcoming release will have the note as well.

@jmara jmara changed the base branch from master to v1.5 November 30, 2017 19:30
@jmara
Copy link
Contributor Author

jmara commented Nov 30, 2017

Hmm that doesn't look right, do you have a hint for me how to rebase correctly?

@timoreimann
Copy link
Contributor

I need to rebase your commit onto the v1.5 branch.

I can also take care of it for you if you don't mind. Let me know.

@jmara
Copy link
Contributor Author

jmara commented Nov 30, 2017

Sure, please go ahead. I'm glad I could help.

@timoreimann
Copy link
Contributor

@jmara alright, rebase complete. (By the way, I did it my doing a git rebase onto like this: git rebase --onto upstream/v1.5 HEAD^.)

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

Apart from a wording improvement, I'm totally fine with it. 👍

@@ -21,6 +21,9 @@ If your cluster is configured with RBAC, you will need to authorize Træfik to u

RoleBindings per namespace enable to restrict granted permissions to the very namespaces only that Træfik is watching over, thereby following the least-privileges principle. This is the preferred approach if Træfik is not supposed to watch all namespaces, and the set of namespaces does not change dynamically. Otherwise, a single ClusterRoleBinding must be employed.

!!! note
RoleBindings per namespace will be available in Træfik 1.5+ please use ClusterRoleBinding for any older version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rewrite this as following:

RoleBindings per namespace are available in Træfik 1.5 and later. Please use ClusterRoleBindings for older versions.

(Present tense; replace plus by verbatim text; second sentence; plural for ClusterRoleBinding too; older versions.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure sounds good. From a documentation perspective in the 1.5 branch present tens makes totally sense. The rest is fine with me. I'll try to make the changes :)

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. 👍

Copy link
Contributor

@dtomcej dtomcej left a comment

Choose a reason for hiding this comment

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

LGTM
:shipit:

@ldez ldez added this to the 1.5 milestone Dec 5, 2017
@ldez ldez removed the bot/no-merge label Dec 5, 2017
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants