[stable/traefik] RBAC support for Traefik

[cknowles]

For #948.

When switching RBAC from true to false, the serviceAccountName stays
as the Traefik specific one so set it to default. Not sure if this is
a Helm issue or further upstream.

[k8s-ci-robot]

Hi @c-knowles. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

[skorski]

Hi @c-knowles, I used this successfully once on a cluster but now I'm getting an error

Error: release righteous-snake failed: clusterroles.rbac.authorization.k8s.io "righteous-snake-traefik-dev" is 
forbidden: attempt to grant extra privileg]} {[watch] [] [pods] [] []} {[get] [] [services] [] []} {[list] [] [services] [] 
[]} {[watch] [] [services] [] []} {[get] [] [endpoints] [] []} {[list] } {[get] [extensions] [ingresses] [] []} {[list] 
[extensions] [ingresses] [] []} {[watch] [extensions] [ingresses] [] []}] user=&{system:serviceaccount:k9c0f 
[system:serviceaccounts system:serviceaccounts:kube-system 
system:authenticated] map[]} ownerrules=[{[create] [authorization.k8s.io] [selfsubjectaccis /apis/* /healthz 
/swaggerapi /swaggerapi/* /version]}] ruleResolutionErrors=[]

I don't think it has anything to do with your code because I can manually fill in the variables and execute the commands with a kubectl apply. Any ideas why this might happen?

[cknowles]

@skorski I've not seen that one before. Is that an error output from a helm install? Do you have some steps to reproduce? Also, are you setting the --service-account param when doing helm init?

[skorski]

I was trying to install it after instantiating a new cluster from kargo. Turns out there were a lot of other issue with the kargo install. I dug around a bit deeper and realized kargo didn't create the cluster roles I was expecting and I'm wondering if helm didn't get the right permission set.

I'll post more information once I get to work tomorrow but your code seems sound.

Yes, I was setting the service account.

[skorski]

So I did a bit more digging and as I suspected, it was related to the kargo / helm install. I thought I had the right service account but I didn't.

Tiller needs to have a service account for this to work helm thread.

These are the commands that are important:

kubectl create serviceaccount --namespace kube-system tiller
kubectl create clusterrolebinding tiller-cluster-rule --clusterrole=cluster-admin --serviceaccount=kube-system:tiller
helm init --upgrade --service-account=tiller

[cknowles]

@skorski Ok great. It seems various people are working on the Helm/Tiller side of things, e.g. more restrictive permissions, better docs etc. Good to know in the meantime though. I'm using kube-aws which right now grants more permissive access to deploys in the kube-system namespace.

[kachkaev]

Really waiting for this. Updating traefik to 1.3 would be also great!

[krancour]

Updating traefik to 1.3 would be also great!

That should be a separate issue and PR. Feel free to open it.

[cknowles]

@krancour Any other hold ups on this? There's a couple charts with this already as pointed out by #1235 (comment) and a few other PRs in flight - #1286, #1287, #1295. I don't have much of a view on the naming, I've used rbac.enabled here but other charts are using either that or rbac.install.

[krancour]

@c-knowles this looks pretty good to me, although I haven't tried it. Do you know with certainty that this is an adequate level of permissions Traefik (and also not more than it requires)?

One other thing is you'd have to increment the minor version number.

[cknowles]

@krancour the list comes directly from https://docs.traefik.io/user-guide/kubernetes/#prerequisites so I hope that it's a sufficient for the current version. As for it being the least privilege required, it seems pretty slim since it's read only on pods, services, endpoints, ingresses. I think it cannot run with anything less.

Bumped the version now and added a small note about k8s version for RBAC.

[kachkaev]

@krancour sorry for offtopic, but I'm here regarding the version bump to 1.3.1 again. Should the PR just be like this one https://github.com/kubernetes/charts/pull/873/files?

What should be the chart version given that @c-knowles has already used 1.4.0?

@c-knowles if you are playing with the chart anyway and you have a git clone of this repo, could you please bump traefik in this or another PR?

 + 💯 for RBAC support – love it!

[cknowles]

Sure, I can bump at some point just it's lower priority for us as it's easy to set the value when you do a Helm install.

[krancour]

@kachkaev things have changed a little bit since #873.

You'd need to do the following:

• values.yaml: imageTag to 1.3.1
• chart.yaml: appVersion to 1.3.1
• chart.yaml: version to... I would make a call here that bumping the chart to default to a new minor release of the software would qualify the chart itself for a minor release. So... you'd use 1.5.0, assuming @c-knowles PR gets merged first. (This is just a reality of dealing with Helm charts-- they're continuously deployed, so every merge to this repo == a release of some chart, meaning we need to always be vigilant about incrementing the version number appropriately in every PR.)

if you are playing with the chart anyway and you have a git clone of this repo, could you please bump traefik in this or another PR

I'd really like it to be a separate PR. As a general principle, lumping unrelated changes into a single PR is frowned upon. (I know the whole 1 PR == 1 release thing that I explained above increases the temptation to bundle multiple changes into a single PR, but please let's avoid it.)

[krancour]

@c-knowles this LGTM. cc @lachie83 @viglesiasce @prydonius

[kachkaev]

Thanks for clarifying the details @krancour! See #1401.