Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/kube2iam]: add rbac support #1286

Merged
merged 3 commits into from
Jul 6, 2017
Merged

[stable/kube2iam]: add rbac support #1286

merged 3 commits into from
Jul 6, 2017

Conversation

SamClinckspoor
Copy link
Contributor

Adds support in kube2iam for RBAC.

setting rbac.enabled to true will create a role, rolebinding and service account.

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

Hi @SamClinckspoor. 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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2017
@SamClinckspoor SamClinckspoor changed the title kube2iam: add rbac support [stable/kube2iam]: add rbac support Jun 13, 2017
@SamClinckspoor
Copy link
Contributor Author

SamClinckspoor commented Jun 27, 2017

Can I get a review on this?

@icereval
Copy link
Collaborator

Reviewed discussion from @c-knowles' merged traefik PR and this looks good, thanks!

LGTM

@cknowles
Copy link
Contributor

If you'd like to account for users switching RBAC enabled from true to false then currently you'll need to explicitly set serviceAccountName to default. The reason is that there's a deprecated serviceAccount attribute in k8s 1.6 so if you only unset serviceAccountName it will be restored from the deprecated attribute.

You can see the fault by installing a chart that does not do this and then trying to turn RBAC off, you will notice the deployment still has the RBAC service account and not the default service account:

helm install stable/etcd-operator --name tester --wait --set rbac.install=true
helm upgrade tester stable/etcd-operator --install --wait --set rbac.install=false

@cknowles
Copy link
Contributor

I think it's definitely an edge case though so the rest of this PR looks great.

@SamClinckspoor
Copy link
Contributor Author

@c-knowles you mean I should set the following?

{{- if .Values.rbac.enabled }}
      serviceAccountName: {{ template "fullname" . }}
{{- else }}
      serviceAccountName: default
{{- end }}

@cknowles
Copy link
Contributor

@SamClinckspoor yes exactly that. As above it's an edge case but may be worth including. It will go away whenever k8s gets rid of the deprecated attribute.

@icereval
Copy link
Collaborator

icereval commented Jul 4, 2017

@c-knowles, thanks for pointing out the SA default.

@icereval
Copy link
Collaborator

icereval commented Jul 4, 2017

LGTM, cc @lachie83 @viglesiasce @prydonius

@viglesiasce
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 6, 2017
@viglesiasce viglesiasce merged commit 6d5e4ce into helm:master Jul 6, 2017
@SamClinckspoor SamClinckspoor deleted the rbac/kube2iam branch July 6, 2017 16:58
lachie83 added a commit to lachie83/charts that referenced this pull request Jul 10, 2017
* upstream/master: (67 commits)
  Fix json whitespace (helm#1458)
  Use consistent whitespace in template placeholders (helm#1437)
  [stable/selenium] Make hub readiness probe timeout configurable (helm#1391)
  [stable/kube2iam]: add rbac support (helm#1286)
  [stable/traefik] Allow enabling traefik access logs (helm#1302)
  Add Stash chart (helm#1420)
  Add Gearman G2 chart (helm#1421)
  add option to include tolerations to daemonset (helm#1364)
  Moved Artifactory to stable and updated version to 5.3.2 (helm#1314)
  Concourse postgres conditional dependency (helm#1390)
  Typo in helm install command for dask-distributed (helm#1413)
  [stable/fluent-bit] Fluent Bit v0.11.12 (helm#1417)
  fixed cassandra chart's persistence bug (helm#1245)
  Prometheus: modify config to support k8s 1.6 by default (helm#1080)
  Add rocket.chat (helm#752)
  Fix influxdb deployment (helm#1424)
  feat(stable/etcd-operator): add support for supplying additional command args (helm#1418)
  add configurable service annotations helm#1234 (helm#1244)
  [stable/prometheus] extra environment variable for alert manager (helm#1237)
  [stable/heapster] Default service name to Heapster (helm#1266)
  ...
yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017
* add rbac support

* solve and edge-case when turning off rbac
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

5 participants