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

Ensure pullrequest-init is based on a root image #3055

Merged
merged 1 commit into from Aug 4, 2020
Merged

Ensure pullrequest-init is based on a root image #3055

merged 1 commit into from Aug 4, 2020

Conversation

ghost
Copy link

@ghost ghost commented Aug 4, 2020

fixes #3054

Changes

The PullRequest Resource, when used as an output, is able to
read in a pr.json to determine if there have been any changes
that require syncing to github. pr.json may have been written
by any prior Step with any ownership settings. If pr.json
was written with root permissions then the PullRequest Resource
needs to be have permissions to read that file.

The PullRequest Resource image has been based on a nonroot
image in our .ko.yaml since 0.13 of Tekton Pipelines (.ko.yaml was
updated here
).

However, the published images did not match the configuration in the
.ko.yaml until 0.15.0 (our tekton/publish.yaml was brought into line
with .ko.yaml here
).

Given that copying or writing pr.json in a Step can result in the file
being owned by root using a nonroot image is not a suitable choice
of base image - the output PullRequest attempts to open pr.json and
hits a permissions error.

This commit updates the PullRequest image to be based on
distroless static instead of nonroot and adds an example yaml
file that should exercise the behaviour of copying the file from
an input to output pullrequest resource.

Submitter Checklist

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

  • Includes tests (if functionality changed/added)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

Fix an issue where an output pullrequest resource could fail because a pr.json file was written by a Step with root ownership.

@ghost ghost added the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Aug 4, 2020
@tekton-robot tekton-robot added 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. labels Aug 4, 2020
@tekton-robot tekton-robot requested review from dibyom and dlorenc August 4, 2020 16:41
@ghost
Copy link
Author

ghost commented Aug 4, 2020

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Aug 4, 2020
@ghost ghost added this to the Pipelines v0.15 milestone Aug 4, 2020
@ghost ghost requested review from imjasonh and vdemeester August 4, 2020 17:01
The PullRequest Resource, when used as an output, is able to
read in a pr.json to determine if there have been any changes
that require syncing to github. pr.json may have been written
by any prior Step with any ownership settings. If pr.json
was written with root permissions then the PullRequest Resource
needs to be have permissions to read that file.

The PullRequest Resource image has been based on a nonroot
image in our `.ko.yaml` since 0.13 of Tekton Pipelines ([`.ko.yaml` was
updated here](#2606)).

However, the published images did not match the configuration in the
`.ko.yaml` until 0.15.0 ([our `tekton/publish.yaml` was brought into line
with `.ko.yaml` here](#3018)).

Given that copying or writing pr.json in a Step can result in the file
being owned by root using a nonroot image is not a suitable choice
of base image - the output PullRequest attempts to open pr.json and
hits a permissions error.

This commit updates the PullRequest image to be based on
distroless static instead of nonroot and adds an example yaml
file that should exercise the behaviour of copying the file from
an input to output pullrequest resource.
@dlorenc
Copy link
Contributor

dlorenc commented Aug 4, 2020

However, the published images did not match the configuration in the
.ko.yaml until 0.15.0 (our tekton/publish.yaml was brought into line with .ko.yaml here).

Ohhhh! Nice debugging.

@dlorenc
Copy link
Contributor

dlorenc commented Aug 4, 2020

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 4, 2020
@bobcatfish
Copy link
Collaborator

/approve

@tekton-robot
Copy link
Collaborator

[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 Aug 4, 2020
@tekton-robot tekton-robot merged commit d0a8324 into tektoncd:master Aug 4, 2020
@@ -6,6 +6,9 @@ baseImageOverrides:
github.com/tektoncd/pipeline/cmd/git-init: gcr.io/tekton-nightly/github.com/tektoncd/pipeline/build-base:latest
# GCS fetcher needs root due to workspace permissions
github.com/tektoncd/pipeline/vendor/github.com/GoogleCloudPlatform/cloud-builders/gcs-fetcher/cmd/gcs-fetcher: gcr.io/distroless/static:latest
# PullRequest resource needs root because in output mode it needs to access pr.json
# which might have been copied or written with any level of permissions.
github.com/tektoncd/pipeline/cmd/pullrequest-init: gcr.io/distroless/static:latest
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it true that this would apply to any container that needs to operate on any file written by a previous step? if so this feels like it might be a slightly larger problem since it feels like communicating b/w steps is a big part of our design 🤔 (maybe im over exaggerating that?)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I do think that's a risk we might want to spend a bit more time analysing. Here I'm fixing the immediate issue we observed in dogfooding wrt the PR pipeline resource.

@bobcatfish bobcatfish removed the needs-cherry-pick Indicates a PR needs to be cherry-pick to a release branch label Sep 11, 2020
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/bug Categorizes issue or PR as related to a bug. 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.

Sharing pullrequest metadata from input to output PR resource results in permission error
3 participants