Don't expand RUN heredocs ourselves, let the shell do it #5473

nalind · 2024-04-11T20:18:23Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

When handling RUN instructions that use heredoc syntax, don't bother interpolating environment variables and argument values, and let the command that's running handle it.

Try to improve error reporting when we're committing an image, so that we don't lose out-of-space errors through layers of compression and editing of file headers in the layer diffs.

How to verify it

New conformance test!

Which issue(s) this PR fixes:

Special notes for your reviewer:

We fail to build e.g. https://github.com/devfile/developer-images/tree/main/universal/ubi8 when the heredoc references environment variables which are set in the heredoc itself, since we currently expand them before passing the heredoc as an argument to the command being invoked.

Does this PR introduce a user-facing change?

Environment variables referenced in RUN instructions which use heredoc syntax will now be evaluated by the command being invoked rather than by the builder.

packit-as-a-service · 2024-04-11T20:29:19Z

Ephemeral COPR build failed. @containers/packit-build please check.

flouthoc · 2024-04-12T15:52:13Z

go.mod

-go 1.20
+go 1.21
+
+toolchain go1.21.9


@nalind I am not sure but I think this directive is a blocking thing because of podman, I created a thread here. #5472 (comment)

Okay, I'm going to try some things in the vendor Makefile target.

Luap99

I suggest you wait with updating go until we have formal understanding on how to do it consistently across all our repos.
I would suggest to focus on containers/skopeo#2297 first and then we apply the changes from there in the other repos.

Luap99 · 2024-04-16T08:58:49Z

Makefile

- GO111MODULE=on $(GO) mod tidy
- GO111MODULE=on $(GO) mod vendor
- GO111MODULE=on $(GO) mod verify
+ if go env | grep -q GOTOOLCHAIN ; then go mod edit -toolchain=none ; fi


this will break all renovate PRs as it doesn't use our makefile

Ugh. Backing it out.

Going to try adding the variable to our local configuration for renovate.

Luap99 · 2024-04-16T08:59:52Z

Makefile

- GO111MODULE=on $(GO) mod vendor
- GO111MODULE=on $(GO) mod verify
+ if go env | grep -q GOTOOLCHAIN ; then go mod edit -toolchain=none ; fi
+ GOTOOLCHAIN=local GO111MODULE=on $(GO) mod tidy


not sure why you force GOTOOLCHAIN=local here only in the vendor target.
If anything it must be set for every go command.

Backing it out.

Okay, trying again with "export" this time.

edsantiago

As always you're way ahead of me. Thank you. One request inline.

tests/bud/no-history/Dockerfile

edsantiago · 2024-04-18T19:25:42Z

tests/bud/no-history/Dockerfile

@@ -1,7 +1,8 @@
 # The important thing about that first base image is that it has no history
 # entries, but it does have at least one layer.

-FROM nixery.dev/shell AS first-stage
+FROM example.com/shell AS first-stage


I was thinking anything on quay.io, or basically anything that does not introduce a new failure pain point.

Hmm, using example.com has the advantage of producing an image reference that can't be confused with something that exists (or might once have existed, if we come back to this in the future) in an actual registry somewhere. How strongly are you in favor of a quay.io name?

IMO, something like fakeregistry.podman.io/notreal makes the intend clear that this is not a real image. And in cases where we ever try to pull by accident we at least hit a domain under our control and name lookup should fail for that.
And ideally you add a comment in the Dockerfile that this image is being created in the test and should not be pulled.

I think my question was: instead of on-demand building each time, can this image be built once (with saved-committed-tracked script and instructions), say onto quay.io/libpod/nohistory:20240422, and then fetched on demand. This is a very difficult test to review. If any step ever breaks in a CI run, it will cost a lot of energy to investigate.

I don't believe I'm authorized to push to that repository, so that's not a thing I can do in this PR, or set up for this PR to use. The rest I can do.

(Just in case it is of interest, https://datatracker.ietf.org/doc/html/rfc6761#section-6.4 specifies that the .invalid TLD should never resolve; that might be useful for similar situations.)

edsantiago · 2024-04-22T11:45:40Z

tests/bud.bats

+ # image) with the name the Dockerfile will specify as its base image.
+ run_buildah tag ${configdigest%% *} example.com/shell
+ # Build images using our image-with-no-history as a base, to check that we
+ # don't trip over ourselves when doing so.


Whether you go the build-each-time or build-once-and-pull-when-testing approach, I would really appreciate a step such as run_buildah inspect History and confirm that it is empty.

Adding one.

openshift-ci · 2024-04-29T20:43:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, nalind

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~~ [nalind]

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

Luap99 · 2024-04-30T11:11:14Z

Makefile

+ $(GO) mod tidy
+ $(GO) mod vendor
+ $(GO) mod verify
+ if test -n "$(strip $(shell go env GOTOOLCHAIN))"; then go mod edit -toolchain none ; fi


I rather not remove the toolchain line and have behave consistently across all our projects (containers/skopeo#2297, containers/common#1963 and others ...)
I don't think this is wrong, the end result is the same as far as I can tell but consistency is key IMO

cc @mtrmac

Whatever changes we do here impact the vendoring validation tests. I'm not averse to splitting it out, but it has effects on things that come after it.

https://go.dev/ref/mod:

For reproducibility, the go command writes its own toolchain name in a toolchain line any time it is updating the go version in the go.mod file

and IIRC that’s the only time that happens. So, in c/image and Skopeo we have a toolchain 1.21.0 line; then go get does not trigger any updates to 1.21.x as long as we stay on Go 1.21.

A possible unanticipated downside of the toolchain 1.21.0 approach is that Renovate files PRs just to update the toolchain, as in containers/image#2382 .

I don’t think we know for sure what Renovate does with Go ≥ 1.21 if there is no toolchain directive at all, AFAIK all updates to 1.21 were using the toolchain 1.21.0 approach so far.

… I think what I’m saying here is that IIRC we haven’t considered the approach of just editing out the toolchain dependency.

And it might actually be a pretty good solution; if nothing else, reflects our intent more precisely than a 1.21.0 value.

… but also, I… don’t personally want to be spending much more time on toolchain. If this approach is better, I’ll happily help switch other repos, but I’d rather not invest much time evaluating it myself.

Luap99

Also as far as actual review goes I am totally confused about the totally different set of changes here. Te me this look like a bunch of independent changes thrown together.

Luap99 · 2024-04-30T11:18:01Z

tests/bud.bats

@@ -333,7 +333,7 @@ _EOF
 }

 @test "bud build with heredoc content" {
- _prefetch fedora
+ _prefetch quay.io/fedora/python-311


please only ever use images from quay.io/libpod/...
Second to we actually have to pull a large python image for this? I don't see the relevance of python specific code in herdoc. Can just as well test a simple shell script IMO.

There's probably some value in checking that the right thing happens when the designated consumer of the heredoc is something other than the shell. I don't think anything the quay.io/libpod organization includes a python interpreter, but if there is one, I wouldn't mind using it instead.

Luap99 · 2024-04-30T11:19:35Z

tests/bud.bats

@@ -4547,17 +4547,17 @@ EOM
 }

 @test "bud arg and env var with same name" {
- _prefetch centos:8
+ _prefetch busybox


same for the other image please make sure to use quay.io/libpod images if we already change them.

For historical reasons there's some super complicated bizarre logic in the tests such that "busybox" and "alpine" translate to quay.io/libpod. See https://github.com/containers/buildah/blob/3763681506611b476cdd3377ee669ecaa7014b8b/tests/registries.conf

mtrmac

I also have the impression that this is a collection of quite unrelated changes; although most are individually desirable, it’s harder to understand and discuss.

In particular, it took me quite a long time to understand that the stated topic of the PR is being achieved by an update of github.com/openshift/imagebuilder.

mtrmac · 2024-04-30T18:40:17Z

Makefile

@@ -1,4 +1,6 @@
 export GOPROXY=https://proxy.golang.org
+export GOTOOLCHAIN=local
+export GO111MODULE=on


(Maybe not in this PR?) I think with current Go versions (the go 1.21 directive in go.mod is being enforced, so we can assume a recent version), it should be possible to never refer to this variable, and to assume modules throughout.

“this variable” being GO111MODULE, to be explicit.

mtrmac · 2024-04-30T18:42:00Z

Makefile

+ $(GO) mod tidy
+ $(GO) mod vendor
+ $(GO) mod verify
+ if test -n "$(strip $(shell go env GOTOOLCHAIN))"; then go mod edit -toolchain none ; fi


What is this go env condition checking for? It’s not absolutely obvious to me, my best guess is “is this a new enough Go to understand GOTOOLCHAIN?”

And if that’s the question, the go 1.21 directive in go.mod ensures the answer is “yes” (or, alternatively, if building with a much older Go which does not enforce the go 1.21 directive, the build is going to fail on a missing library feature anyway).

mtrmac · 2024-04-30T18:49:52Z

digester.go

+ if err == nil {
+ err = err1
+ }
+ pipeReader.CloseWithError(err)


I always worry excessively that a failing goroutine will leave the consumer hanging.

Consider something like

func someOperationGoroutine() { err := errors.New("Internal error: unexpected panic in someOperationGoroutine") defer func() { // Ensure we _always_ wake up the caller _ = pipeReader.CloseWithError(err) // CloseWithError(nil) is equivalent to Close(), always returns nil wg.Done() }() err = someOperation() } func someOperation() { // straight-line worry-free Go code with liberally sprinkled early `return`s and any other control flow }

nalind · 2024-04-30T21:08:20Z

Spun out #5493, #5494, #5495, #5496, #5497, #5498, #5499.

When handling RUN instructions that use heredoc syntax, don't bother interpolating environment variables and argument values, and let the command that's running handle it. Signed-off-by: Nalin Dahyabhai <[email protected]>

When we get a tried-to-write-to-closed-pipe error while encoding something for a coprocess, try to capture error output from the coprocess and add it to the error message, to hopefully catch a flake we're seeing in CI. Signed-off-by: Nalin Dahyabhai <[email protected]>

rhatdan · 2024-05-20T19:33:33Z

LGTM

rhatdan · 2024-05-20T19:33:46Z

@mtrmac @Luap99 PTAL

mtrmac

/lgtm

openshift-ci bot added do-not-merge/work-in-progress kind/bug Categorizes issue or PR as related to a bug. approved labels Apr 11, 2024

This was referenced Apr 11, 2024

Don't expand RUN heredocs ourselves, let the shell do it openshift/imagebuilder#281

Merged

Dockerfiles: run tar x with --no-same-owner devfile/developer-images#167

Merged

nalind force-pushed the heredoc-quoting branch from 120b10c to d4779a3 Compare April 11, 2024 20:22

nalind force-pushed the heredoc-quoting branch 5 times, most recently from 6ad8b5f to f06f82d Compare April 12, 2024 15:46

flouthoc reviewed Apr 12, 2024

View reviewed changes

nalind force-pushed the heredoc-quoting branch 2 times, most recently from b25d215 to c3c0536 Compare April 12, 2024 17:38

Luap99 reviewed Apr 16, 2024

View reviewed changes

nalind force-pushed the heredoc-quoting branch from c3c0536 to 4d79e13 Compare April 16, 2024 13:54

nalind changed the title ~~WIP: don't expand RUN heredocs ourselves, let the shell do it~~ Don't expand RUN heredocs ourselves, let the shell do it Apr 16, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Apr 16, 2024

nalind force-pushed the heredoc-quoting branch from 4d79e13 to 2ffeb99 Compare April 17, 2024 20:50

nalind mentioned this pull request Apr 18, 2024

Nuke the new nixery image #5479

Closed

edsantiago reviewed Apr 18, 2024

View reviewed changes

tests/bud/no-history/Dockerfile Outdated Show resolved Hide resolved

nalind force-pushed the heredoc-quoting branch from 2ffeb99 to b4af111 Compare April 18, 2024 18:25

edsantiago reviewed Apr 18, 2024

View reviewed changes

nalind mentioned this pull request Apr 19, 2024

HEREDOC run command: shell variables #5465

Open

LecrisUT mentioned this pull request Apr 20, 2024

HEREDOC: Escaping $ seems off #5422

Open

edsantiago reviewed Apr 22, 2024

View reviewed changes

nalind force-pushed the heredoc-quoting branch 2 times, most recently from 58903c2 to 11f190a Compare April 22, 2024 13:49

edsantiago mentioned this pull request Apr 22, 2024

Tests: new tool to build a no-history image #5486

Closed

Luap99 reviewed Apr 30, 2024

View reviewed changes

mtrmac reviewed Apr 30, 2024

View reviewed changes

nalind force-pushed the heredoc-quoting branch from 7b673b0 to 4e76327 Compare April 30, 2024 21:08

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 3, 2024

nalind force-pushed the heredoc-quoting branch from 4e76327 to 91736b2 Compare May 6, 2024 18:29

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2024

nalind force-pushed the heredoc-quoting branch 5 times, most recently from 0823914 to c84b0f6 Compare May 9, 2024 19:29

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2024

nalind force-pushed the heredoc-quoting branch from c84b0f6 to 1a06b7e Compare May 10, 2024 18:05

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2024

nalind force-pushed the heredoc-quoting branch 3 times, most recently from fdcc7a6 to f837728 Compare May 16, 2024 17:25

nalind added 2 commits May 20, 2024 09:35

Don't expand RUN heredocs ourselves, let the shell do it

97382f9

When handling RUN instructions that use heredoc syntax, don't bother interpolating environment variables and argument values, and let the command that's running handle it. Signed-off-by: Nalin Dahyabhai <[email protected]>

nalind force-pushed the heredoc-quoting branch from f837728 to d9191e1 Compare May 20, 2024 13:35

mtrmac reviewed May 20, 2024

View reviewed changes

openshift-ci bot assigned mtrmac May 20, 2024

openshift-ci bot added the lgtm label May 20, 2024

openshift-merge-bot bot merged commit 7f0a322 into containers:main May 20, 2024
32 of 34 checks passed

nalind deleted the heredoc-quoting branch May 20, 2024 20:48

Don't expand RUN heredocs ourselves, let the shell do it #5473

Don't expand RUN heredocs ourselves, let the shell do it #5473

Conversation

nalind commented Apr 11, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nalind commented Apr 30, 2024

rhatdan commented May 20, 2024

rhatdan commented May 20, 2024

mtrmac left a comment

Choose a reason for hiding this comment

nalind commented Apr 11, 2024 •

edited