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

port https://github.com/tektoncd/pipeline/pull/2967 to triggers #707

Merged
merged 1 commit into from
Oct 8, 2020

Conversation

gabemontero
Copy link
Contributor

@gabemontero gabemontero commented Aug 11, 2020

Changes

Submitter Checklist

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

See the contribution guide for more details.

Release Notes

    The PodSecurityPolicy configuration was updated so that container must run as non-root, and the RBAC for utilizing the PSP was moved from cluster scoped to namespace scoped to better ensure triggers can not utilize other PSPs unrelated to this project

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 11, 2020
@gabemontero
Copy link
Contributor Author

@vdemeester @nikhil-thomas @sthaha FYI

IIRC the analogous upstream pipeline PR that modified PSP/roles had implications for us downstream ?

@gabemontero
Copy link
Contributor Author

@dibyom @savitaashture I finally pushed updates based on your comments last month

I've also reached out to @skaegi about verifications in his env's vs. the verifications I'll be doing.

@gabemontero
Copy link
Contributor Author

I'm going to remove the WIP label ... I've completed the testing in the secured environments I have access to

@gabemontero gabemontero changed the title WIP: port https://github.com/tektoncd/pipeline/pull/2967 to triggers port https://github.com/tektoncd/pipeline/pull/2967 to triggers Sep 18, 2020
@tekton-robot tekton-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. release-note-none Denotes a PR that doesnt merit a release note. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 18, 2020
@dibyom
Copy link
Member

dibyom commented Sep 21, 2020

Could we drop the WIP from the commit message and make it a bit more descriptive? (also, add to the release notes?)

@savitaashture
Copy link
Contributor

Hi @gabemontero

I tried to install trigger using this PR on minikube and i see pods's are not coming up

kubectl get pods -n tekton-pipelines  -w
NAME                                           READY   STATUS             RESTARTS   AGE
tekton-triggers-controller-5995c76b5b-bgkfb    0/1     CrashLoopBackOff   5          10m
tekton-triggers-webhook-5c94c6cdc-cwmpx        0/1     CrashLoopBackOff   6          10m

with below error

ae8bdf6a3207335c"
  Warning  BackOff    100s (x5 over 2m26s)   kubelet, minikube  Back-off restarting failed container
  Normal   Created    87s (x5 over 3m6s)     kubelet, minikube  Created container tekton-triggers-controller
  Normal   Pulled     87s (x4 over 2m54s)    kubelet, minikube  Container image "docker.io/savita3020/controller-f656ca31de179ab913fa76abc255c315@sha256:0d2d80595fdfa8ee8462532fe03d04447dc6d35fc35c2e93ae8bdf6a3207335c" already present on machine
  Warning  Failed     86s (x5 over 3m6s)     kubelet, minikube  Error: failed to start container "tekton-triggers-controller": Error response from daemon: OCI runtime create failed: container_linux.go:345: starting container process caused "chdir to cwd (\"/home/nonroot\") set in config.json failed: permission denied": unknown

same issue occurring for pipeline tektoncd/pipeline#3273

@gabemontero
Copy link
Contributor Author

Ah ha ... thanks for trying that out @savitaashture

Am I correct that you had to explicitly turn on pod security policy controller with your minikube installation (as I recall PSP controller is off by default with k8s ... or does minikube turn in on automatically under the covers) ?

I agree that it looks like the same situation in pipelines.

/hold

let's see how tektoncd/pipeline#3273 sorts out

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2020
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2020
@tekton-robot tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2020
@gabemontero gabemontero changed the title port https://github.com/tektoncd/pipeline/pull/2967 to triggers port https://github.com/tektoncd/pipeline/pull/2967 and https://github.com/tektoncd/pipeline/pull/3342 to triggers Oct 8, 2020
@gabemontero
Copy link
Contributor Author

/hold cancel

I've pulled in tektoncd/pipeline#3342 which addressed @savitaashture 's issue

/assign @dibyom
/assign @savitaashture

PTAL ... thanks

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020
@gabemontero
Copy link
Contributor Author

check that ... @dibyom pulled in that latest fix with 09539a8

@gabemontero gabemontero changed the title port https://github.com/tektoncd/pipeline/pull/2967 and https://github.com/tektoncd/pipeline/pull/3342 to triggers port https://github.com/tektoncd/pipeline/pull/2967 to triggers Oct 8, 2020
move PSP from clusterrole to role
@dibyom
Copy link
Member

dibyom commented Oct 8, 2020

This LGTM. One minor nit - can we make the commit message more descriptive? maybe, it could be the same as the message from the pipelines PR. Also, let's add to the release notes?

@gabemontero
Copy link
Contributor Author

OK cleaned up commit message and got the rebase sorted out

PTAL @dibyom and @savitaashture

@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesnt merit a release note. labels Oct 8, 2020
@gabemontero
Copy link
Contributor Author

release note updated as well @dibyom

@dibyom
Copy link
Member

dibyom commented Oct 8, 2020

/approve
/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2020
@tekton-robot
Copy link

[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 /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 Oct 8, 2020
@tekton-robot tekton-robot merged commit 04561fc into tektoncd:master Oct 8, 2020
@gabemontero gabemontero deleted the psp-fixes branch October 8, 2020 20:12
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. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants