-
Notifications
You must be signed in to change notification settings - Fork 416
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
Bind admin role so that SA can use PSP #863
Conversation
@fiunchinho: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Hi @fiunchinho. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
examples/rbac.yaml
Outdated
@@ -37,6 +37,18 @@ roleRef: | |||
kind: Role | |||
name: tekton-triggers-example-minimal | |||
--- | |||
apiVersion: rbac.authorization.k8s.io/v1 | |||
kind: RoleBinding |
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.
Instead of adding a binding to the admin role, could we add the rule of using pod security policies to the role defined above in this file?
Question: Is this needed due to #862? Also, /cc @gabemontero our resident PSP expert! |
Yes I believe it is @dibyom .... the slack discussion thread we've had with these folks on their issue is https://tektoncd.slack.com/archives/CKUSJ2A5D/p1607949867225500 Subsequent to getting this to work for them, I am curious how long triggers has been broke with PSP turned on. Or if it ever worked with PSP turned on. I would think it had worked with PSP sometime in the past but am not certain. If it did, that might mean something changed in the image or controller to necessitate these changes. By comparison, the PSP analogous feature in openshift, SCC, (which is what I typically run with), has not seen this (unless we need to retry with the very latest ... it has been a couple of weeks for me). @savitaashture attested to the same experience.
oooh, that is a very dangerous characterization man ;-) .... "expert" at best is a relative term here :-) |
well, #862 got merged yesterday so I'm guessing since then 😛 The conroller/webhook already had roles to use PSPs. The EL sink deployment did not have this role, but the deployment spec also did not have a |
the opposite actually ... I suggested that to them, along with this PR, to help get past this ... basically mimic what we do for the controller / webhook ... they were seeing the issue before that merged |
Makes sense. controllers/webhook had the right policies/securityContext while EL sinks did not. So in environments requiring PSPs, the controller/webhook would work but not the EL. |
I will take over this PR from @fiunchinho - he is currently on vacation but I'd like to push this forward in the meantime. |
Thank you 🙏 |
Addressed review comment, please take a look. Thanks. |
Looks good. One small thing -- could you squash up the two commits? Thanks! /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom 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 |
Done |
/lgtm |
Nice 🎉 |
Changes
Examples fail because the
ServiceAccount
used has no permissions to use thePodSecurityPolicy
. We need to bind the admin role to theServiceAccount
so that it can use the PSP.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them: