Use a new base image for the `git-init` image. #4758

mattmoor · 2022-04-13T18:59:27Z

This swaps out the Dockerfile based git-init base image in favor of ghcr.io/distroless/git since it is now possible to produce a distroless Git image without losing one's sanity.

This does NOT remove the Dockerfile or any logic to build it just yet, in case we want to roll this back, but that should follow in a subsequent change (tracked by the issue below).

Related: #4752

/kind cleanup
/hold

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

The git-init image is now based on ghcr.io/distroless/git with fewer unused packages installed! 🎉

mattmoor · 2022-04-13T19:01:41Z

I threw the hold on mostly to ensure this doesn't merge too quickly and I get some explicit 👀 on the tekton/publish.yaml changes in here, which I doubt are tested presubmit (and I'm not sure how to test manually).

Also let me know if there are any additional things worth calling out in the release notes for this.

mattmoor · 2022-04-13T19:46:58Z

I'll have to find some time this afternoon to debug a bit what's going on here 🤔

mattmoor · 2022-04-14T04:04:59Z

I spent most of my coding time today trying to get Trivy scans set up for the images, so we can make sure we are tracking those for these images, so I didn't have time yet to dig into why this is failing. The next two days are relatively meeting free, so I am holding out hope I will get to this tomorrow 🤞

mattmoor · 2022-04-14T16:18:25Z

Rebasing to pull in the git 2.35.2 fixes to be sure it's not a bad interaction there, and then I'll start to poke through this locally. 🤞

imjasonh

This change looks right to me, just a couple comments/questions that don't need to block.

/lgtm

imjasonh · 2022-04-14T16:56:11Z

config/controller.yaml

@@ -75,10 +75,10 @@ spec:

          # This is gcr.io/google.com/cloudsdktool/cloud-sdk:302.0.0-slim
          "-gsutil-image", "gcr.io/google.com/cloudsdktool/cloud-sdk@sha256:27b2c22bf259d9bc1a291e99c63791ba0c27a04d2db0a43241ba0f1f20f4067f",
-          # The shell image must be root in order to create directories and copy files to PVCs.
-          # gcr.io/distroless/base:debug as of February 17, 2022
+          # The shell image must allow root in order to create directories and copy files to PVCs.


Just double-checking this comment: ghcr.io/distroless/busybox is not root by default, but allows root if we run it as such with runAsUser, correct? Do we add runAsUser for shell-image steps (e.g., anything that uses script:? 🤔

I'm splitting the shell stuff off because clearly the root requirements haven't been plumbed through for shell-image as Scott did for git-init, and will chase that separately.

I'm not sure why our KinD-based tests didn't show the PVC test problems using this, which makes me wonder if the e2e test suite is being clever with t.Skip() or something (or I'm missing a feature flag).

imjasonh · 2022-04-14T16:59:58Z

tekton/publish.yaml

@@ -95,7 +95,7 @@ spec:

      # Combine Distroless with a Windows base image, used for the entrypoint image.
      COMBINED_BASE_IMAGE=$(go run ./vendor/github.com/tektoncd/plumbing/cmd/combine/main.go \
-        gcr.io/distroless/base:debug-nonroot \
+        ghcr.io/distroless/busybox \


(not blocking this PR)

I'm not sure why the entrypoint image has to have a shell at all. I looked up the history and it looks like some dummy named @imjasonh added this in 95b8c71 -- prior to that (and in non-release workflows before and after that change), entrypoint is based on gcr.io/distroless/static:nonroot, which contains no shell.

If nobody remembers why I made that change, I think I'd like to change it back, and make this line ghcr.io/distroless/static.

I don't think it should block this PR, but just in case anybody remembers. @vdemeester

nit: Let's stick with gcr.io/distroless/static until we decide to switch to the apko static image elsewhere?

Also, I think the same sentiment applies to ALL of these, since they are all tested with static:nonroot in CI, not just entrypoint (which you specifically called out).

Ok, so thinking through this has made me realize this use of busybox is wholly distinct from the shell-images cases, so I am going to forget about these, and you should go nuts switching things to OG static:nonroot, and then we can revisit this for NG static as part of a later convo.

mattmoor · 2022-04-14T17:03:09Z

Ok, digging thru the logs, it looks like we need to do some work to run shell-image steps as root where we explicitly need it (like Scott did with git-init 🤩 ). Let me back out the shell portion of this for now, and then I can try to chase down the places we explicitly need root for e2e testing in order to switch shell-image over.

This swaps our `Dockerfile` based `git-init` base image in favor of `ghcr.io/distroless/git` since it is now possible to produce a `distroless` Git image without losing one's sanity. This does NOT remove the `Dockerfile` or any logic to build it just yet, in case we want to roll this back, but that should follow in a subsequent change (tracked by the issue below). Related: tektoncd#4752

mattmoor · 2022-04-14T18:18:27Z

Poking through the failures now, I see at least one place where we don't appear to be running git explicitly as root:

=== CONT  TestYamls/yamls/v1beta1/pipelineruns/pipelinerun-with-final-tasks.yaml
    build_logs.go:37: build logs 
        >>> Container step-check-application-dir-has-source:
        Something went wrong and could not find application source under /workspace/source/application/
        
        >>> Container step-cleanup-workspace:
        2022/04/14 17:30:40 Skipping step because a previous step failed
        
        >>> Container step-verify-application-dir-has-gone:
        2022/04/14 17:30:41 Skipping step because a previous step failed
    build_logs.go:37: build logs 
        >>> Container step-clone:
        {"level":"error","ts":1649957425.0455554,"caller":"git/git.go:55","msg":"Error running git [init /workspace/output/application]: exit status 128\nfatal: cannot mkdir /workspace/output/application: Permission denied\n","stacktrace":"github.com/tektoncd/pipeline/pkg/git.run\n\tgithub.com/tektoncd/pipeline/pkg/git/git.go:55\ngithub.com/tektoncd/pipeline/pkg/git.Fetch\n\tgithub.com/tektoncd/pipeline/pkg/git/git.go:90\nmain.main\n\tgithub.com/tektoncd/pipeline/cmd/git-init/main.go:53\nruntime.main\n\truntime/proc.go:255"}
        {"level":"fatal","ts":1649957425.0456972,"caller":"git-init/main.go:54","msg":"Error fetching git repository: exit status 128","stacktrace":"main.main\n\tgithub.com/tektoncd/pipeline/cmd/git-init/main.go:54\nruntime.main\n\truntime/proc.go:255"}

=== CONT  TestYamls/yamls/v1beta1/pipelineruns/pipelinerun.yaml
    build_logs.go:37: build logs 
        >>> Container step-clone:
        {"level":"error","ts":1649957413.2802649,"caller":"git/git.go:55","msg":"Error running git [init /workspace/output/]: exit status 1\n/workspace/output/.git: Permission denied\n","stacktrace":"github.com/tektoncd/pipeline/pkg/git.run\n\tgithub.com/tektoncd/pipeline/pkg/git/git.go:55\ngithub.com/tektoncd/pipeline/pkg/git.Fetch\n\tgithub.com/tektoncd/pipeline/pkg/git/git.go:90\nmain.main\n\tgithub.com/tektoncd/pipeline/cmd/git-init/main.go:53\nruntime.main\n\truntime/proc.go:255"}
        {"level":"fatal","ts":1649957413.2803946,"caller":"git-init/main.go:54","msg":"Error fetching git repository: exit status 1","stacktrace":"main.main\n\tgithub.com/tektoncd/pipeline/cmd/git-init/main.go:54\nruntime.main\n\truntime/proc.go:255"}

I also see this in the TestExamples failures...

mattmoor · 2022-04-14T18:20:46Z

Yeah, ok these clearly need changes too:

    - name: clone
      image: ko://github.com/tektoncd/pipeline/cmd/git-init
      script: |
        CHECKOUT_DIR="$(workspaces.output.path)/$(params.subdirectory)"

I don't think we're running the yaml/example tests downstream, but we should probably start to.

mattmoor · 2022-04-14T18:26:59Z

Ok, the second commit should address the explicit use of ko://.../git-init for scripting.

All of the remaining failures have to do with git-ssh credential handling, so I am guessing that was missing in the runAsUser: 0 plumbing. I'll chase that next.

mattmoor · 2022-04-14T21:01:34Z

Alright, so I put together chainguard-dev/apko#182 to create /root properly with apko, and I'm going to try running through this with an image built by me to flush out anything else that might need fixing while we get a patch release of apko out and the images rebuilt. 🤞

mattmoor · 2022-04-14T21:11:39Z

/test pull-tekton-pipeline-go-coverage

.ko.yaml

This is very similar to tektoncd#4758 and tektoncd#4717 but for `shell-image`. This switches the default user of the `shell-image` to `nonroot`, which forces Tekton to be explicit about all of the places it needs a shell as `root` using `runAsUser: 0`. This also switches things over to a much leaner `busybox` image (no `glibc`), which is the main thing we use `base:debug` for with the OG distroless images. Fixes: tektoncd#4761 Related: tektoncd#4752

This came up in tektoncd#4758, but when stitching together a linux + windows image as a base for utility images Jason had inadvertently(?) used `base:debug-nonroot` instead of `static:nonroot` for the linux side. Given that ~all testing is done with `static:nonroot` for these images, this is probably SAFER than what we're currently doing releasing based on an untested (albeit superset) configuration. However, the additional things this pulls in are `glibc` and `busybox`, which are unfortunate because `glibc` often has vulnerabilities that these images aren't susceptible to, and `busybox` is a contentious dependency in many settings (to minimizing its use it generally better). AFAIK this is untested presubmit, and I have no idea how to test this properly, so please review thoroughly! /kind cleanup

imjasonh · 2022-04-14T22:56:45Z

/lgtm

Thanks!

mattmoor · 2022-04-15T00:21:25Z

/test pull-tekton-pipeline-alpha-integration-tests

tests timed out 🤔

This came up in #4758, but when stitching together a linux + windows image as a base for utility images Jason had inadvertently(?) used `base:debug-nonroot` instead of `static:nonroot` for the linux side. Given that ~all testing is done with `static:nonroot` for these images, this is probably SAFER than what we're currently doing releasing based on an untested (albeit superset) configuration. However, the additional things this pulls in are `glibc` and `busybox`, which are unfortunate because `glibc` often has vulnerabilities that these images aren't susceptible to, and `busybox` is a contentious dependency in many settings (to minimizing its use it generally better). AFAIK this is untested presubmit, and I have no idea how to test this properly, so please review thoroughly! /kind cleanup

tekton-robot · 2022-04-15T07:40:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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:

~~OWNERS~~ [vdemeester]

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

mattmoor · 2022-04-15T14:05:57Z

/test pull-tekton-pipeline-integration-tests

Going to run e2e once more with the nightly images

mattmoor · 2022-04-15T15:03:41Z

Let's get it baking

/hold cancel

This is very similar to #4758 and #4717 but for `shell-image`. This switches the default user of the `shell-image` to `nonroot`, which forces Tekton to be explicit about all of the places it needs a shell as `root` using `runAsUser: 0`. This also switches things over to a much leaner `busybox` image (no `glibc`), which is the main thing we use `base:debug` for with the OG distroless images. Fixes: #4761 Related: #4752

Similar to tektoncd@ba2e7f3 - with tektoncd#4758, `git-init` now uses `ghcr.io/distroless/git` as its base image, and that image needs to run as root. `git-init` by default does _not_ run as root. So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be changed to run as root, or the example tests will keep failing forever. An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using `git-init:v0.35.0` or later - currently, it's using `v0.29.0`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Similar to ba2e7f3 - with #4758, `git-init` now uses `ghcr.io/distroless/git` as its base image, and that image needs to run as root. `git-init` by default does _not_ run as root. So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be changed to run as root, or the example tests will keep failing forever. An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using `git-init:v0.35.0` or later - currently, it's using `v0.29.0`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

# This is the 1st commit message: change emitting results # This is the commit message #2: fix code # This is the commit message #3: A few minor cleanups in pkg/reconciler/pipelinerun/pipelinerun_test.go These just mildly annoyed me, so I thought I'd clean them up. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com> # This is the commit message #4: Instrument e2e pipelinerun_test.go files for logstream These two (`test/pipelinerun_test.go` and `test/v1alpha1/pipelinerun_test.go`) weren't done in the last PR, because they were messy and I wanted to get that PR in. But I had some time this morning, so here they are, which should be the last things in the e2e tests (other than examples/yamls testing, which are their own bucket of worms) to be instrumented with `helpers.ObjectNameForTest(t)`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com> # This is the commit message #5: Update tutorial links We recently updated the introductory tutorial on the documnentation website. That tutorial covers the same content as the one here. To avoid duplicated efforts and content drift, I'm linkin that doc here and replacing the existing content. Additionally, this removes references to `PipelineResources`. # This is the commit message #6: Set git-clone tasks in examples to run as root Similar to tektoncd@ba2e7f3 - with tektoncd#4758, `git-init` now uses `ghcr.io/distroless/git` as its base image, and that image needs to run as root. `git-init` by default does _not_ run as root. So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be changed to run as root, or the example tests will keep failing forever. An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using `git-init:v0.35.0` or later - currently, it's using `v0.29.0`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com> # This is the commit message #7: adding latest release - 0.35 Updating the table to include the latest release links. # This is the commit message #8: Added a unit test for pod status. Add unit test with large result for setTaskRunStatusBasedOnStepStatus.

This came up in tektoncd#4758, but when stitching together a linux + windows image as a base for utility images Jason had inadvertently(?) used `base:debug-nonroot` instead of `static:nonroot` for the linux side. Given that ~all testing is done with `static:nonroot` for these images, this is probably SAFER than what we're currently doing releasing based on an untested (albeit superset) configuration. However, the additional things this pulls in are `glibc` and `busybox`, which are unfortunate because `glibc` often has vulnerabilities that these images aren't susceptible to, and `busybox` is a contentious dependency in many settings (to minimizing its use it generally better). AFAIK this is untested presubmit, and I have no idea how to test this properly, so please review thoroughly! /kind cleanup

This is very similar to tektoncd#4758 and tektoncd#4717 but for `shell-image`. This switches the default user of the `shell-image` to `nonroot`, which forces Tekton to be explicit about all of the places it needs a shell as `root` using `runAsUser: 0`. This also switches things over to a much leaner `busybox` image (no `glibc`), which is the main thing we use `base:debug` for with the OG distroless images. Fixes: tektoncd#4761 Related: tektoncd#4752

Similar to tektoncd@ba2e7f3 - with tektoncd#4758, `git-init` now uses `ghcr.io/distroless/git` as its base image, and that image needs to run as root. `git-init` by default does _not_ run as root. So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be changed to run as root, or the example tests will keep failing forever. An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using `git-init:v0.35.0` or later - currently, it's using `v0.29.0`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Similar to ba2e7f3 - with #4758, `git-init` now uses `ghcr.io/distroless/git` as its base image, and that image needs to run as root. `git-init` by default does _not_ run as root. So the two examples using copy-pasted old versions of the `git-clone` catalog task need to be changed to run as root, or the example tests will keep failing forever. An equivalent change will be needed in the `git-clone` catalog task once it's bumped to using `git-init:v0.35.0` or later - currently, it's using `v0.29.0`. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

tekton-robot requested review from afrittoli and pritidesai April 13, 2022 18:59

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Apr 13, 2022

vdemeester mentioned this pull request Apr 14, 2022

Support "git-init" for arm32v7 platform #3647

Closed

mattmoor force-pushed the use-apko-distroless branch from a75e4a5 to 399d3f9 Compare April 14, 2022 16:17

imjasonh reviewed Apr 14, 2022

View reviewed changes

tekton-robot assigned imjasonh Apr 14, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2022

mattmoor force-pushed the use-apko-distroless branch from 399d3f9 to 52b8f68 Compare April 14, 2022 17:07

tekton-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 14, 2022

mattmoor changed the title ~~Use new images for shell image and the git-init base.~~ Use a new base image for the git-init image. Apr 14, 2022

Attempt to fix TestYamls

9723fc4

mattmoor mentioned this pull request Apr 14, 2022

git-init should specify to run as root when it needs it. #4711

Closed

mattmoor commented Apr 14, 2022

View reviewed changes

.ko.yaml Outdated Show resolved Hide resolved

mattmoor changed the title ~~[WIP] Use a new base image for the git-init image.~~ Use a new base image for the git-init image. Apr 14, 2022

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 14, 2022

mattmoor mentioned this pull request Apr 14, 2022

Switch the franken-image to use static:nonroot for linux images. #4763

Merged

5 tasks

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2022

vdemeester approved these changes Apr 15, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2022

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2022

tekton-robot merged commit ba2e7f3 into tektoncd:main Apr 15, 2022

mattmoor deleted the use-apko-distroless branch April 15, 2022 15:34

abayer mentioned this pull request Apr 27, 2022

Set git-clone tasks in examples to run as root #4807

Merged

5 tasks

lbernick mentioned this pull request Apr 29, 2022

Changes to images published by Pipelines are not tested in the PR where they are introduced #4810

Closed

imjasonh mentioned this pull request Sep 16, 2022

Why did Tekton change its base image? #5507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a new base image for the `git-init` image. #4758

Use a new base image for the `git-init` image. #4758

mattmoor commented Apr 13, 2022 •

edited

Loading

mattmoor commented Apr 13, 2022

mattmoor commented Apr 13, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

imjasonh left a comment

imjasonh Apr 14, 2022

mattmoor Apr 14, 2022

imjasonh Apr 14, 2022

mattmoor Apr 14, 2022

mattmoor Apr 14, 2022

mattmoor Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

imjasonh commented Apr 14, 2022

mattmoor commented Apr 15, 2022

tekton-robot commented Apr 15, 2022

mattmoor commented Apr 15, 2022

mattmoor commented Apr 15, 2022

Use a new base image for the git-init image. #4758

Use a new base image for the git-init image. #4758

Conversation

mattmoor commented Apr 13, 2022 • edited Loading

Submitter Checklist

Release Notes

mattmoor commented Apr 13, 2022

mattmoor commented Apr 13, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh Apr 14, 2022

Choose a reason for hiding this comment

mattmoor Apr 14, 2022

Choose a reason for hiding this comment

imjasonh Apr 14, 2022

Choose a reason for hiding this comment

mattmoor Apr 14, 2022

Choose a reason for hiding this comment

mattmoor Apr 14, 2022

Choose a reason for hiding this comment

mattmoor Apr 14, 2022

Choose a reason for hiding this comment

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

mattmoor commented Apr 14, 2022

imjasonh commented Apr 14, 2022

mattmoor commented Apr 15, 2022

tekton-robot commented Apr 15, 2022

mattmoor commented Apr 15, 2022

mattmoor commented Apr 15, 2022

Use a new base image for the `git-init` image. #4758

Use a new base image for the `git-init` image. #4758

mattmoor commented Apr 13, 2022 •

edited

Loading