Skip to content

Allow ci-admins to crud prowjobs - #150

Merged
0xmichalis merged 1 commit into
openshift:masterfrom
0xmichalis:sync-ci-admins
Sep 30, 2017
Merged

0xmichalis merged 1 commit into
openshift:masterfrom
0xmichalis:sync-ci-admins

Conversation

@0xmichalis

Copy link
Copy Markdown
Contributor

/cc @enj

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 17, 2017
@enj

enj commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

Does this need to be compatible with older clusters? If not, define the new role / bindings using RBAC instead of origin authz. Also, I believe @smarterclayton said he wanted ci-admins to be admins cluster wide. But maybe he wanted a different group with that power instead?

Comment thread cluster/ci/config/roles.yaml Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@enj is already in the ci-admins group in our cluster. I am not sure what's the scope of the group honestly. cc: @smarterclayton

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This group gives cluster level access to everything. I don't think Mo needs that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was given access to help figure out some OAuth / RBAC issues. You are welcome to scope that down as you see fit.

@0xmichalis

Copy link
Copy Markdown
Contributor Author

define the new role / bindings using RBAC instead of origin authz

Unclear what this means.. Do you mean that I should use the upstream types?

@stevekuznetsov

Copy link
Copy Markdown
Contributor

wanted ci-admins to be admins cluster wide. But maybe he wanted a different group with that power instead?

@enj we have cluster-reader and sudoer which is sufficient. Giving access to e.g. oc delete is dangerous (lol)

@enj

enj commented Sep 17, 2017

Copy link
Copy Markdown
Contributor

Unclear what this means.. Do you mean that I should use the upstream types?

Yes, there are the canonical representation in 3.7. The Origin authz resources are a proxy.

we have cluster-reader and sudoer which is sufficient. Giving access to e.g. oc delete is dangerous (lol)

That is fine, but it will fail the SAR check that Jenkins performs.

@stevekuznetsov

Copy link
Copy Markdown
Contributor

We should have admin in the ci namespace from our GitHub team sync, that should take care of Jenkins

@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2017
@0xmichalis 0xmichalis changed the title Sync ci-admins, allow cruding prowjobs Allow ci-admins to crud prowjobs Sep 19, 2017
@0xmichalis

Copy link
Copy Markdown
Contributor Author

Keeping just the cluster{role,rolebinding} for now. @smarterclayton where do I need to plumb this manifest so it is applied on cluster creation/upgrade?

@enj

enj commented Sep 20, 2017

Copy link
Copy Markdown
Contributor

@Kargakis let the server do the conversion for you:

oc create -f prowjob_access.yaml
oc export clusterrolebinding.rbac.authorization.k8s.io/ci-admins-prowjobs clusterrole.rbac.authorization.k8s.io/prowjob-access -o yaml
apiVersion: v1
items:
- apiVersion: rbac.authorization.k8s.io/v1beta1
  kind: ClusterRoleBinding
  metadata:
    creationTimestamp: null
    name: ci-admins-prowjobs
  roleRef:
    apiGroup: rbac.authorization.k8s.io
    kind: ClusterRole
    name: prowjob-access
  subjects:
  - apiGroup: rbac.authorization.k8s.io
    kind: Group
    name: ci-admins
- apiVersion: rbac.authorization.k8s.io/v1beta1
  kind: ClusterRole
  metadata:
    creationTimestamp: null
    name: prowjob-access
  rules:
  - apiGroups:
    - prow.k8s.io
    resources:
    - prowjobs
    verbs:
    - create
    - delete
    - get
    - list
kind: List
metadata: {}

@0xmichalis

0xmichalis commented Sep 20, 2017

Copy link
Copy Markdown
Contributor Author

@enj I am not as worried. There are other roles inside this repo that need to be converted, too, and I would prefer to handle them holistically.

@0xmichalis

Copy link
Copy Markdown
Contributor Author

@smarterclayton we need to figure out how to apply this as part of the upgrade.

@0xmichalis
0xmichalis merged commit 1e186ee into openshift:master Sep 30, 2017
@0xmichalis
0xmichalis deleted the sync-ci-admins branch September 30, 2017 03:06
@0xmichalis

Copy link
Copy Markdown
Contributor Author

or fresh installation

@smarterclayton

smarterclayton commented Oct 2, 2017 via email

Copy link
Copy Markdown
Contributor

@0xmichalis

Copy link
Copy Markdown
Contributor Author

Added in #206

droslean pushed a commit to droslean/release that referenced this pull request Jan 14, 2022
Add horologium prow component manifest files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants