Added proper RBAC rules to Prow #8288

paulangton · 2018-06-07T17:39:44Z

Integrated RBAC rules from Openshift as per @stevekuznetsov 's suggestion. Rules were added en masse to starter.yaml, but also duplicated in separate deployment configs. Should fix #7950 and clean up leftovers from #8088

BenTheElder · 2018-06-07T17:51:27Z

/ok-to-test

cjwagner

We use legacy authorization on this cluster. Will that conflict with RBAC?

cjwagner · 2018-06-07T20:13:29Z

prow/cluster/starter.yaml

@@ -94,6 +94,7 @@ spec:
        app: hook
    spec:
      terminationGracePeriodSeconds: 180
+      serviceAccountName: "hook"


You'll need to add the service accounts to the *_deployment.yaml files as well.

Since our deployments won't be using RBAC we can't actually add the service account since we won't define it. Maybe add it to the deployment files, but comment it out with an explanation?

Addressed in latest commit

It doesn't appear to be?
I still see non-commented lines like serviceAccountName: "plank" in the deployment files.

discussed this offline, definitely comment out with an explanation, someday we will be on RBAC and need to uncomment 🙃 (and also add these to the rules_k8s deployment)

I added service accounts to the deployments and purposefully didnt comment them out, thought Ben's comment below was saying these wouldnt be deployed to prow.k8s.io.

Latest commit contains commented out service accounts in deployment files

BenTheElder · 2018-06-07T20:47:01Z

We use legacy authorization on this cluster. Will that conflict with RBAC?

assuming you mean prow.k8s.io, nothing here should actually get deployed to it AFAICT.

cjwagner · 2018-06-07T21:15:46Z

nothing here should actually get deployed to it AFAICT.

Are we just adding it for completeness and other deployments then?

BenTheElder · 2018-06-07T22:14:33Z

This is for the getting started guide? We have separate config for k8s.io

…

On Thu, Jun 7, 2018, 2:15 PM Cole Wagner ***@***.***> wrote: nothing here should actually get deployed to it AFAICT. Are we just adding it for completeness and other deployments then? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8288 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4BqzJgEqeO9S8m3TjjlZve-v-OPrGKks5t6ZgCgaJpZM4Ue1P5> .

cjwagner · 2018-06-07T23:10:10Z

This is for the getting started guide? We have separate config for k8s.io

It doesn't look like this is all meant for the starter yaml. starter.yaml has its RBAC rules defined there, but a bunch of other RBAC files were also added containing the same rules.

BenTheElder · 2018-06-07T23:17:13Z

edit: ah, in this pr starter.yaml also gains RBAC

in any case, the bazel deployment at least is not applying these other files, and someday we will probably need the same rules outside of starter. I don't think they are harmful.

fejta · 2018-06-08T01:15:52Z

/assign @stevekuznetsov @BenTheElder @cjwagner

paulangton · 2018-06-08T01:28:34Z

I decided to keep a copy of the RBAC deployments separate from starter.yaml for people who may want to deploy their own versions of specific services without having to apply starter.yaml for RBAC.

stevekuznetsov · 2018-06-12T15:40:41Z

@paulangton did you have to edit any of the RBAC rules? Would be good to review the diff between what we have deployed and what you are proposing here. If there was no necessary diff, this LGTM

paulangton · 2018-06-19T17:24:09Z

@stevekuznetsov I had to edit all of the RBAC rules. The ones in the Openshift repo were wrapped in Openshift Template kinds, which I removed. I also split the list of objects into their own documents and created a service account for each deployment.

Example: Diff between Openshift (left) and this PR (right):

apiVersion: template.openshift.io/v1			      |	apiVersion: v1
kind: Template						      |	kind: ServiceAccount
parameters:						      |	metadata:
- description: The name of the component.		      |	  name: "plank"
  name: NAME						      |	---
  value: plank						      |	kind: Role
objects:						      |	apiVersion: rbac.authorization.k8s.io/v1beta1
- apiVersion: v1					      |	metadata:
  kind: ServiceAccount					      |	  name: "plank"
  metadata:						      |	rules:
    name: "${NAME}"					      |	  - apiGroups:
- kind: Role						      |	      - ""
  apiVersion: rbac.authorization.k8s.io/v1beta1		      |	    resources:
  metadata:						      |	      - pods
    name: "${NAME}"					      |	    verbs:
  rules:						      |	      - create
    - apiGroups:					      |	      - delete
        - ""						      |	      - list
      resources:					      |	  - apiGroups:
        - pods						      |	      - "prow.k8s.io"
      verbs:						      |	    resources:
        - create					      |	      - prowjobs
        - delete					      |	    verbs:
        - list						      |	      - create
    - apiGroups:					      |	      - list
        - "prow.k8s.io"					      |	      - update
      resources:					      |	---
        - prowjobs					      |	kind: RoleBinding
      verbs:						      |	apiVersion: rbac.authorization.k8s.io/v1beta1
        - create					      |	metadata:
        - list						      |	  name: "plank"
        - update					      |	roleRef:
- kind: RoleBinding					      |	  apiGroup: rbac.authorization.k8s.io
  apiVersion: rbac.authorization.k8s.io/v1beta1		      |	  kind: Role
  metadata:						      |	  name: "plank"
    name: "${NAME}"					      |	subjects:
  roleRef:						      |	- kind: ServiceAccount
    apiGroup: rbac.authorization.k8s.io			      |	  name: "plank"
    kind: Role						      <
    name: "${NAME}"					      <
  subjects:						      <
  - kind: ServiceAccount				      <
    name: "${NAME}"					      <

stevekuznetsov · 2018-06-19T18:51:29Z

OK, you could also use List objects to keep them nicely in one document per file. If the changes were:

remove Template and use List instead
evaluate the template parameters

Then these LGTM

paulangton · 2018-06-20T00:03:14Z

@stevekuznetsov Unless there is some notable advantage I am not seeing to using a List type, I am going to leave it as is. K8s docs on general configuration as well as RBAC specify config files containing multiple objects separated by the triple hyphen instead of combined in a List type.

stevekuznetsov · 2018-06-20T20:00:02Z

Sure, fine either way. Please add GET for ConfigMaps to the hook role, xref openshift/release#969 as required by recent additions to the config-updater plugin

paulangton · 2018-06-21T00:36:26Z

hook Role now allows get

paulangton · 2018-06-22T19:06:51Z

@stevekuznetsov @cjwagner lgty?

cjwagner

One change, otherwise this LGTM.

cjwagner · 2018-06-25T20:30:17Z

prow/cluster/tide_deployment.yaml

@@ -25,6 +25,7 @@ spec:
      labels:
        app: tide
    spec:
+      serviceAccountName: "tide"


This should be commented as well.

Good catch, fixed in latest

removed permissive policy added service account to deployments, removed tide rbac from starter.yaml commented out serviceaccounts for use with RBAC added get to hook rbac fixed tide deployment

cjwagner

/lgtm

k8s-ci-robot · 2018-06-25T21:19:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, paulangton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~prow/OWNERS~~ [cjwagner]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

0xmichalis · 2018-06-25T21:55:06Z

prow/cluster/deck_rbac.yaml

+kind: ClusterRole
+apiVersion: rbac.authorization.k8s.io/v1beta1
+metadata:
+  name: "deck-oauth"


You don't need this here; we use it in openshift because we run a deck instance for our private repos behind an oauth proxy. Unfortunately, the oauth proxy works only on openshift today :/

0xmichalis · 2018-06-25T21:57:31Z

prow/cluster/horologium_deployment.yaml

@@ -25,6 +25,7 @@ spec:
      labels:
        app: horologium
    spec:
+      # serviceAccountName: "horologium" # Uncomment for use with RBAC


I think you should be able to use this service account regardless of running with rbac or not so it may be worth to uncomment upfront and move the service accounts alongside the deployment manifests.

0xmichalis · 2018-06-25T22:02:23Z

I decided to keep a copy of the RBAC deployments separate from starter.yaml for people who may want to deploy their own versions of specific services without having to apply starter.yaml for RBAC.

It would be nice to include the jenkins operator rbac profile too. I don't know what's the best place to put that though, maybe just add it in the source code?

paulangton requested review from cjwagner and stevekuznetsov as code owners June 7, 2018 17:39

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2018

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 Jun 7, 2018

cjwagner reviewed Jun 7, 2018

View reviewed changes

k8s-ci-robot assigned BenTheElder, cjwagner and stevekuznetsov Jun 8, 2018

paulangton force-pushed the proper-prow-rbac branch from 9e1a9cf to 87345b4 Compare June 19, 2018 17:33

paulangton force-pushed the proper-prow-rbac branch from 87345b4 to 711babc Compare June 20, 2018 00:04

paulangton force-pushed the proper-prow-rbac branch from 711babc to 60e3057 Compare June 21, 2018 00:35

cjwagner reviewed Jun 25, 2018

View reviewed changes

pulled rules out of templates, added rbac to starter.yaml

82d585b

removed permissive policy added service account to deployments, removed tide rbac from starter.yaml commented out serviceaccounts for use with RBAC added get to hook rbac fixed tide deployment

paulangton force-pushed the proper-prow-rbac branch from 60e3057 to 82d585b Compare June 25, 2018 20:39

cjwagner approved these changes Jun 25, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 25, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2018

k8s-ci-robot merged commit 89f42c7 into kubernetes:master Jun 25, 2018

0xmichalis reviewed Jun 25, 2018

View reviewed changes

This was referenced Jul 27, 2019

Remove unnecessary deck-oauth ClusterRole and binding. #13640

Merged

deploy all the rbac rules for prow.k8s.io #9867

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added proper RBAC rules to Prow #8288

Added proper RBAC rules to Prow #8288

paulangton commented Jun 7, 2018

BenTheElder commented Jun 7, 2018

cjwagner left a comment

cjwagner Jun 7, 2018

cjwagner Jun 12, 2018

paulangton Jun 19, 2018

cjwagner Jun 19, 2018

BenTheElder Jun 19, 2018

paulangton Jun 19, 2018

paulangton Jun 20, 2018

BenTheElder commented Jun 7, 2018

cjwagner commented Jun 7, 2018

BenTheElder commented Jun 7, 2018 via email

cjwagner commented Jun 7, 2018

BenTheElder commented Jun 7, 2018 •

edited

Loading

fejta commented Jun 8, 2018

paulangton commented Jun 8, 2018

stevekuznetsov commented Jun 12, 2018

paulangton commented Jun 19, 2018

stevekuznetsov commented Jun 19, 2018

paulangton commented Jun 20, 2018

stevekuznetsov commented Jun 20, 2018

paulangton commented Jun 21, 2018

paulangton commented Jun 22, 2018

cjwagner left a comment

cjwagner Jun 25, 2018

paulangton Jun 25, 2018

cjwagner left a comment

k8s-ci-robot commented Jun 25, 2018

0xmichalis Jun 25, 2018

0xmichalis Jun 25, 2018

0xmichalis commented Jun 25, 2018

Added proper RBAC rules to Prow #8288

Added proper RBAC rules to Prow #8288

Conversation

paulangton commented Jun 7, 2018

BenTheElder commented Jun 7, 2018

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Jun 7, 2018

cjwagner commented Jun 7, 2018

BenTheElder commented Jun 7, 2018 via email

cjwagner commented Jun 7, 2018

BenTheElder commented Jun 7, 2018 • edited Loading

fejta commented Jun 8, 2018

paulangton commented Jun 8, 2018

stevekuznetsov commented Jun 12, 2018

paulangton commented Jun 19, 2018

stevekuznetsov commented Jun 19, 2018

paulangton commented Jun 20, 2018

stevekuznetsov commented Jun 20, 2018

paulangton commented Jun 21, 2018

paulangton commented Jun 22, 2018

cjwagner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jun 25, 2018

BenTheElder commented Jun 7, 2018 •

edited

Loading