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

RBAC support #344

Merged
merged 1 commit into from
May 31, 2017
Merged

RBAC support #344

merged 1 commit into from
May 31, 2017

Conversation

Bregor
Copy link
Contributor

@Bregor Bregor commented May 7, 2017

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.2

Role allows deis-router:

  • extensions/deployments: get
  • apps/deployments: get

ClusterRole allows deis-router:

  • services: get and list
  • secrets: get
  • endpoints: get

@vdice
Copy link
Member

vdice commented May 8, 2017

Jenkins, OK to test

@vdice vdice added this to the v2.15 milestone May 8, 2017
@codecov-io
Copy link

codecov-io commented May 8, 2017

Codecov Report

Merging #344 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c31737d...c9769fe. Read the comment docs.

@vdice
Copy link
Member

vdice commented May 9, 2017

Jenkins, test this please

{{/*
Set apiVersion based on Kubernetes version
*/}}
{{- define "rbacApiVersion" -}}
Copy link
Contributor

@krancour krancour May 9, 2017

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.

Copy link
Contributor Author

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"]
Copy link
Contributor

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"]
Copy link
Contributor

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"]
Copy link
Contributor

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
Copy link
Contributor

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.

Copy link
Contributor Author

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

@krancour
Copy link
Contributor

krancour commented May 9, 2017

@Bregor thank you for this contribution. I made several comments, but I want to tell you that overall, this looks great!

@Bregor
Copy link
Contributor Author

Bregor commented May 9, 2017

@krancour thank you very much for your comments! I will try to achieve them in couple hours.

@vdice
Copy link
Member

vdice commented May 9, 2017

Jenkins, add to whitelist

@Bregor Bregor mentioned this pull request May 12, 2017
@vdice vdice self-assigned this May 16, 2017
@vdice vdice self-requested a review May 16, 2017 17:01
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
@Bregor
Copy link
Contributor Author

Bregor commented May 16, 2017

@krancour thank you once again for you comments and sorry for too long waiting.
Fixed and rebased.

@krancour
Copy link
Contributor

@Bregor did your own testing show that router still worked fine with the reduced permissions?

@Bregor
Copy link
Contributor Author

Bregor commented May 18, 2017

@krancour I still can create/delete app, set domains an so on.

@krancour krancour added the LGTM1 label May 19, 2017
@krancour
Copy link
Contributor

Thanks, @Bregor. LGTM. Waiting for one other LGTM.

@vdice vdice added the LGTM2 label May 24, 2017
@vdice vdice merged commit 0272d4b into deis:master May 31, 2017
vdice pushed a commit to deis/workflow that referenced this pull request May 31, 2017
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