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

[stable/kube-lego]: add RBAC support #1287

Merged
merged 4 commits into from
Aug 15, 2017
Merged

[stable/kube-lego]: add RBAC support #1287

merged 4 commits into from
Aug 15, 2017

Conversation

SamClinckspoor
Copy link
Contributor

@SamClinckspoor SamClinckspoor commented Jun 13, 2017 •
edited
Loading

Adds support in kube-lego for RBAC.
Permissions sourced from jetstack/kube-lego#99 and adjusted slightly.

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 add kube-lego RBAC support kube-lego: add RBAC support Jun 13, 2017
@SamClinckspoor SamClinckspoor changed the title kube-lego: add RBAC support [stable/kube-lego]: add RBAC support Jun 13, 2017
@unguiculus unguiculus requested a review from mgoodness June 13, 2017 16:46
@unguiculus
Copy link
Member

/cc @jackzampolin

@k8s-ci-robot
Copy link
Contributor

@unguiculus: GitHub didn't allow me to request PR reviews from the following users: jackzampolin.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @jackzampolin

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.

@SamClinckspoor
Copy link
Contributor Author

SamClinckspoor commented Jun 27, 2017 •
edited
Loading

pinging for a review @jackzampolin

@cknowles cknowles mentioned this pull request Jun 28, 2017
Merged
@jackzampolin
Copy link
Contributor

@SamClinckspoor Thanks for the ping. Pulled this down and it looks good. Thanks for adding this! LGTM! 👍

@dmccaffery
Copy link

@mgoodness what do we need to do to get this merged? I'd love to have RBAC "official"

@mgoodness mgoodness self-assigned this Jul 17, 2017
@martell martell mentioned this pull request Jul 25, 2017
Closed
@mgoodness
Copy link
Contributor

@dmccaffery give me just a couple more days. I'm on a roll with RBAC-related merges, so we should get this merged pretty quickly.

@mgoodness mgoodness mentioned this pull request Aug 14, 2017
Closed
mgoodness
mgoodness suggested changes Aug 14, 2017
View reviewed changes
Copy link
Contributor

@mgoodness mgoodness left a comment

Choose a reason for hiding this comment

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

Just a few requested changes to conform with the repo-wide RBAC pattern we've adopted. Should be a quick merge after that.

stable/kube-lego/templates/deployment.yaml Outdated
@@ -19,6 +19,9 @@ spec:
app: {{ template "name" . }}
release: {{ .Release.Name }}
spec:
{{- if .Values.rbac.enabled }}
serviceAccountName: {{ template "fullname" . }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Use serviceAccountName: {{ if .Values.rbac.create }}{{ template "fullname" . }}{{ else }}"{{ .Values.rbac.serviceAccountName }}"{{ end }}

stable/kube-lego/templates/role.yaml Outdated
rules:
- apiGroups:
- ""
- "extensions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: don't need quotes around extensions

stable/kube-lego/templates/role.yaml Outdated
- update
- delete
- apiGroups:
- "extensions"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: no quotes required

stable/kube-lego/templates/role.yaml Outdated
- delete
- watch
- apiGroups:
- "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be explicit, no?

stable/kube-lego/values.yaml Outdated
@@ -44,3 +44,6 @@ resources: {}
# requests:
# cpu: 20m
# memory: 8Mi

rbac:
enabled: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this create. Add rbac.serviceAccountName and set to default.

stable/kube-lego/README.md Outdated
@@ -53,6 +53,7 @@ Parameter | Description | Default
`podAnnotations` | annotations to be added to pods | `{}`
`replicaCount` | desired number of pods | `1`
`resources` | kube-lego resource requests and limits (YAML) |`{}`
`rbac.enabled` | Enable role and serviceaccount creation | `false`
Copy link
Contributor

Choose a reason for hiding this comment

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

Update with changes made to values.yaml

@SamClinckspoor
Copy link
Contributor Author

@mgoodness Changed to the new RBAC pattern and also trimmed the role permissions as per jetstack/kube-lego#99 (comment)

@mgoodness
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 Aug 15, 2017
@mgoodness
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2017
@k8s-ci-robot
Copy link
Contributor

@SamClinckspoor: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e c9fa61f link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@mgoodness
Copy link
Contributor

CI failure is expected due to the required LEGO_EMAIL value (which still needs to be addressed).

@mgoodness mgoodness merged commit fe072f4 into helm:master Aug 15, 2017
@SamClinckspoor SamClinckspoor deleted the rbac/kube-lego branch August 15, 2017 13:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mgoodness mgoodness mgoodness approved these changes

Assignees

@mgoodness mgoodness

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. size/small
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SamClinckspoor @k8s-ci-robot @unguiculus @jackzampolin @dmccaffery @mgoodness