-
Notifications
You must be signed in to change notification settings - Fork 56
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
RBAC support #344
RBAC support #344
Conversation
Jenkins, OK to test |
Codecov Report
@@ Coverage Diff @@
## master #344 +/- ##
=======================================
Coverage 56.47% 56.47%
=======================================
Files 6 6
Lines 425 425
=======================================
Hits 240 240
Misses 159 159
Partials 26 26 Continue to review full report at Codecov.
|
Jenkins, test this please |
{{/* | ||
Set apiVersion based on Kubernetes version | ||
*/}} | ||
{{- define "rbacApiVersion" -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we observe Go's established naming conventions, this should be rbacAPIVersion
. See doc here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krancour thank you, naming updated.
rules: | ||
- apiGroups: ["extensions", "apps"] | ||
resources: ["deployments"] | ||
verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only deployment that the router is concerned with is its own. It's retrieved by name-- i.e. a get. Could you please try removing list and watch here and verify that the router still works?
verbs: ["get", "list", "watch"] | ||
- apiGroups: [""] | ||
resources: ["services"] | ||
verbs: ["get", "list", "watch"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's an issue open (#274) that would require watch for services, but in the current implementation, watch isn't used (but list is). Could you please try removing it here and verify that the router still works? On principle, we should be giving the minimum set of permissions required.
verbs: ["get", "list", "watch"] | ||
- apiGroups: [""] | ||
resources: ["secrets"] | ||
verbs: ["get", "list"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secrets are retrieved by name-- i.e. a get-- so list shouldn't be needed. Could you please try removing list here and verify that the router still works?
kind: ClusterRole | ||
apiVersion: {{ template "rbacApiVersion" . }} | ||
metadata: | ||
name: deis:deis-router |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this colon allowed here? Even if so, I would consider changing this name. My understanding is that names of most things in k8s should look like valid DNS names, which I believe excludes colons. Even if it's technically permissible in this particular spot, I would rather we still deferred to that convention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krancour of course it is. More than that: it's recommended for "global" (not namespaced) resources.
Look at clusterroles, defined by kubernetes itself:
$ kubectl get clusterrole
NAME AGE
...
system:auth-delegator 1h
system:basic-user 6h
system:controller:attachdetach-controller 1h
system:controller:certificate-controller 1h
system:controller:cronjob-controller 1h
system:controller:daemon-set-controller 1h
system:controller:deployment-controller 1h
system:controller:disruption-controller 1h
system:controller:endpoint-controller 1h
system:controller:generic-garbage-collector 1h
system:controller:horizontal-pod-autoscaler 1h
system:controller:job-controller 1h
@Bregor thank you for this contribution. I made several comments, but I want to tell you that overall, this looks great! |
@krancour thank you very much for your comments! I will try to achieve them in couple hours. |
Jenkins, add to whitelist |
With this change deis-router became available to work in RBAC-only clusters Works with both Kubernetes 1.5 and 1.6 (see templates/_helpers.tmpl for details) Actually tested with 1.5.7 and 1.6.3 Role allows deis-router: - extensions/deployments: get - apps/deployments: get ClusterRole allows deis-router: - services: get and list - secrets: get - endpoints: get
@krancour thank you once again for you comments and sorry for too long waiting. |
@Bregor did your own testing show that router still worked fine with the reduced permissions? |
@krancour I still can create/delete app, set domains an so on. |
Thanks, @Bregor. LGTM. Waiting for one other LGTM. |
Adds RBAC support for Workflow, see also: - deis/builder#513 - deis/controller#1292 - deis/fluentd#96 - deis/monitor#195 - deis/router#344
With this change
deis-router
became available to work in RBAC-only clustersWorks with both Kubernetes 1.5 and 1.6 (see
templates/_helpers.tmpl
for details)Actually tested with 1.5.7 and 1.6.2
Role allows
deis-router
:extensions/deployments
:get
apps/deployments
:get
ClusterRole allows
deis-router
:services
:get
andlist
secrets
:get
endpoints
:get