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

Add GCP Viewer role to adjustpermissions.py #926

Merged
merged 1 commit into from
Dec 7, 2021

Conversation

lbernick
Copy link
Member

@lbernick lbernick commented Nov 23, 2021

Changes

The only level of access currently grantable by the permissions
script includes sensitive permissions like storage admin.
This commit provides a default option for a lower level of access
to the dogfooding cluster.

Please note: I have not audited all the permissions provided by
the viewer role. While it doesn't include access to contents of secrets,
it does include a large number of other permissions. This is simply
meant as a safer default option.

Closes #920.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

/kind misc

@tekton-robot tekton-robot added the kind/misc Categorizes issue or PR as a miscellaneuous one. label Nov 23, 2021
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 23, 2021
@bobcatfish
Copy link
Contributor

Thanks @lbernick !

/approve
/lgtm

For the record I've since run the script for @lbernick and given her the full access the other build captains have for now - the main reason was to contribute to dogfooding, which can be very hard unless you have access (i.e. if you want to actually verify anything works before submitting it)

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2021
@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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:

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 2, 2021
@lbernick
Copy link
Member Author

lbernick commented Dec 3, 2021

/test plumbing yamllint

@nikhil-thomas
Copy link
Member

/lgtm

@lbernick
Copy link
Member Author

lbernick commented Dec 6, 2021

/test plumbing-yamllint
/test plumbing-unit-tests

@afrittoli
Copy link
Member

/test plumbing-yamllint

@afrittoli
Copy link
Member

I'm trying to figure out what's wrong with the CI jobs. These are the taskruns executed for a specific github webhook invocation (/test plumbing-yamllint): https://dashboard.dogfooding.tekton.dev/#/taskruns?labelSelector=prow.k8s.io%2Fbuild-id%3D10085f90-56e0-11ec-8195-75543178328f

image

Something is wrong in the new webhook, because the github-set-status with pending should not have been invoked
for the unit tests and for the image build, since the check name did not match for me.

@afrittoli
Copy link
Member

afrittoli commented Dec 6, 2021

From the el pod logs:

{"level":"info","ts":"2021-12-06T22:01:48.106Z","logger":"eventlistener","caller":"sink/sink.go:323","msg":"ResolvedParams : [{Name:taskRunNamespace Value:tekton-ci} {Name:parentPipelineRunName Value:plumbing-image-build-t5h5z} {Name:parentPipelineRunTaskName Value:clone-repo} {Name:buildUUID Value:10085f90-56e0-11ec-8195-75543178328f} {Name:checkName Value:plumbing-image-build} {Name:taskRunName Value:plumbing-image-build-t5h5z-clone-repo} {Name:pullRequestNumber Value:926} {Name:gitRevision Value:0dccd2a3a6bf5582d66a704c901bb5bb4737c992} {Name:gitHubCheckStatus Value:pending} {Name:gitHubCheckDescription Value:Job Triggered.} {Name:gitHubRepo Value:tektoncd/plumbing}]","eventlistener":"tekton-events","namespace":"default","eventlistenerUID":"b37832bb-3e30-4516-9b04-c41d626e9321","/triggers-eventid":"3634f34f-4897-460a-944d-705263b8fad5","/trigger":"github-check-start"}

So it's the "/trigger":"github-check-start"

@afrittoli
Copy link
Member

- name: github-check-start
interceptors:
- cel:
filter: >-
header.match('ce-type', 'dev.tekton.event.taskrun.started.v1') &&
body.taskRun.metadata.labels['tekton.dev/kind'] == 'ci' &&
body.taskRun.metadata.labels['triggers.tekton.dev/eventlistener'] in ['tekton-ci-webhook', 'tekton-ci'] &&
!('tekton.dev/conditionName' in body.taskRun.metadata.labels) &&
!('ci.tekton.dev/condition' in body.taskRun.metadata.annotations) &&
(body.taskRun.metadata.name.indexOf('-check-') == -1) &&
(body.taskRun.metadata.name.indexOf('-clone-repo-') == -1) &&
'ownerReferences' in body.taskRun.metadata
overlays:
- key: repo
expression: body.taskRun.metadata.annotations['tekton.dev/gitURL'].parseURL().path.substring(1)

@afrittoli
Copy link
Member

Oh, I got it! The name of the clone task is plumbing-image-build-4bmlg-clone-repo.
Since the change in the resource naming in the latest release, there is no -<random> at the end anymore, so the CEL filter that excludes the git-clone task in the new webhook does not work anymore.
Fixing coming up.

@afrittoli
Copy link
Member

Fix: #943

@afrittoli
Copy link
Member

If you could push a new sha for this PR once #943 is merge, it will reset the statuses

@afrittoli
Copy link
Member

/test plumbing-unit-tests

The only level of access currently grantable by the permissions
script includes sensitive permissions like storage admin.
This commit provides a default option for a lower level of access
to the dogfooding cluster.

Please note: I have not audited all the permissions provided by
the viewer role. While it doesn't include access to contents of secrets,
it does include a large number of other permissions. This is simply
meant as a safer default option.

Addresses tektoncd#920.
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2021
@afrittoli
Copy link
Member

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2021
@afrittoli
Copy link
Member

/test plumbing-golang-lint

@afrittoli
Copy link
Member

/test plumbing-yamllint

@afrittoli
Copy link
Member

/test plumbing-golang-lint

@tekton-robot tekton-robot merged commit 3a2e029 into tektoncd:main Dec 7, 2021
bobcatfish added a commit to bobcatfish/plumbing that referenced this pull request Apr 11, 2022
tektoncd#926 added a `--readonly` flag
for adding readonly user permissions, however it added it with defualt
"true" which means that by default when running this script to add or
remove permissions, it will only modify the read only permissions and
will leave any write permissions as-is.

This means that when I went to update the governing board permissions -
i.e. adding permissions for new members and removing permissions from
old members, the script did very little and I couldn't figure out why.

This commit switches the default to the writer roles - defaulting to
reader is probably a safer default, however I think it's a confusing
change given how the script has been used so far. Can be convinced that
instead we should keep the default and add better documentation and
examples (but someone else will probably need to pick that up!)
bobcatfish added a commit to bobcatfish/plumbing that referenced this pull request Apr 11, 2022
tektoncd#926 added a `--readonly` flag
for adding readonly user permissions, however it added it with defualt
"true" which means that by default when running this script to add or
remove permissions, it will only modify the read only permissions and
will leave any write permissions as-is.

This means that when I went to update the governing board permissions -
i.e. adding permissions for new members and removing permissions from
old members, the script did very little and I couldn't figure out why.

This commit switches the default to the writer roles - defaulting to
reader is probably a safer default, however I think it's a confusing
change given how the script has been used so far. Can be convinced that
instead we should keep the default and add better documentation and
examples (but someone else will probably need to pick that up!)
tekton-robot pushed a commit that referenced this pull request Apr 15, 2022
#926 added a `--readonly` flag
for adding readonly user permissions, however it added it with defualt
"true" which means that by default when running this script to add or
remove permissions, it will only modify the read only permissions and
will leave any write permissions as-is.

This means that when I went to update the governing board permissions -
i.e. adding permissions for new members and removing permissions from
old members, the script did very little and I couldn't figure out why.

This commit switches the default to the writer roles - defaulting to
reader is probably a safer default, however I think it's a confusing
change given how the script has been used so far. Can be convinced that
instead we should keep the default and add better documentation and
examples (but someone else will probably need to pick that up!)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/misc Categorizes issue or PR as a miscellaneuous one. lgtm Indicates that a PR is ready to be merged. 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.

Read-only dogfooding cluster permissions
5 participants