buildah: add `heredoc` support for `RUN`, `COPY` and `ADD` by flouthoc · Pull Request #5092 · containers/buildah

flouthoc · 2023-10-19T09:18:41Z

Following PR is a attempt to add Heredoc support to buildah.

Once this PR is merged buildah is supposed to honor heredoc syntax while processing containerfiles

Expected syntax to work

FROM docker.io/library/python:latest
RUN <<EOF
echo "Hello" >> /hello
echo "World!" >> /hello
EOF

RUN python3 <<EOF
with open("/hello", "w") as f:
    print("Hello", file=f)
    print("Something", file=f)
EOF

RUN ls -a
RUN cat hello

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

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?

buildah: add heredoc support for RUN, COPY and ADD

Closes: #3474

flouthoc · 2023-10-19T09:19:23Z

Needs: openshift/imagebuilder#264

openshift-ci · 2023-10-19T09:19:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [flouthoc]

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

packit-as-a-service · 2023-10-20T12:03:19Z

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

flouthoc · 2023-10-23T09:04:41Z

This needs: openshift/imagebuilder#264

Other then the imagebuilder PR, following PR is ready to review.

@nalind @rhatdan @TomSweeneyRedHat @vrothberg @giuseppe @containers/buildah-maintainers PTAL

/hold wait till openshift/imagebuilder#264

TomSweeneyRedHat · 2023-10-24T19:21:50Z

Changes here LGTM in general. I assume you'll also be updating the Containerfile man page in in c/common with the heredocs examples?

flouthoc · 2023-10-25T09:11:26Z

Changes here LGTM in general. I assume you'll also be updating the Containerfile man page in in c/common with the heredocs examples?

@TomSweeneyRedHat Yes I'll open a PR in c/common once this gets merged. As of now I am waiting for openshift/imagebuilder#264

flouthoc · 2023-11-10T15:42:55Z

This PR is read for review.

nalind

Mostly nits and a conformance test question.

nalind · 2023-11-16T16:08:35Z

imagebuildah/stage_executor.go

+				for _, src := range copy.Src {
+					// If this source is not a heredoc, then it is a regular file from
+					// build context or from another stage (`--from=`) so treat this differently.
+					if !strings.HasPrefix(src, "<<") && !strings.HasPrefix(src, "<<-") {


If the value doesn't have << as a prefix, it can't have <<- as a prefix, so I don't think we need to be checking for both of these.

imagebuildah/stage_executor.go

nalind · 2023-11-16T16:11:10Z

imagebuildah/stage_executor.go

+				data = strings.TrimPrefix(data, "\n")
+				// add breakline when heredoc ends for docker compat
+				data = data + "\n"
+				tmpFile, err := os.Create(filepath.Join(parse.GetTempDir(), filepath.Base(filepath.ToSlash(file.Name))))


After passing a pathname through filepath.ToSlash(), the result should be manipulated using the path package rather than filepath, since filepath expects the platform-specific path separator, which isn't / on Windows hosts.

nalind · 2023-11-16T16:17:24Z

tests/conformance/conformance_test.go

+			"(dir):test2:(dir):robots.txt:mtime",
+			"(dir):test2:(dir):image_file:mtime",
+			"(dir):test2:(dir):image_file:uid", /* buildkit's regular COPY sets UID which is inconsistent with docker and buildah*/
+			"(dir):test2:(dir):image_file:gid", /* buildkit's regular COPY sets GID which is inconsistent with docker and buildah*/


What values does it use? Why is it okay to ignore the differences?

Buildkit sets uid and gid to 1000 with COPY instruction in conformance test and buildah/docker keeps it 0, i suspect either problem is with buildkit client in conformance test or could be a buildkit issue, this happens on upstream as well so I was not able to spend time on it to figure out root cause for it. Since this was not related to this PR hence I did not touch it for now ( will investigate issue in a different PR )

Issue reproducer for upstream:

Following problem is with all the COPY ( where uid and gid are not modified ) related tests included in conformance_test.go

For example a try out example is

diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 6ec8f3837..e494cb116 100644 --- a/tests/conformance/conformance_test.go +++ b/tests/conformance/conformance_test.go @@ -2784,6 +2784,7 @@ var internalTestCases = []testCase{ { name: "copy-for-user", // flip side of issue #2518 contextDir: "copy", + dockerUseBuildKit: true, dockerfileContents: strings.Join([]string{ `FROM alpine`, `USER 66:66`,

and output fails

[fl@fedora buildah]$ sudo GOROOT=/usr/local/go go test -v -timeout=30m -run TestConformance/copy-for-user ./tests/conformance === RUN TestConformance === RUN TestConformance/copy-for-user conformance_test.go:482: Error Trace: /home/fl/heredoc-exp/buildah/tests/conformance/conformance_test.go:482 /home/fl/heredoc-exp/buildah/tests/conformance/conformance_test.go:333 /home/fl/heredoc-exp/buildah/tests/conformance/conformance_test.go:151 Error: Filesystem contents differ Test: TestConformance/copy-for-user Messages: File attributes in both versions have different values: File:attr Docker buildah /script:uid 1000 0 /script:gid 1000 0 conformance_test.go:355: Context "/home/fl/heredoc-exp/buildah/tests/conformance/testdata/copy" conformance_test.go:367: Dockerfile contents: FROM alpine USER 66:66 COPY /script /script (no final end-of-line) === RUN TestConformance/copy-for-user-with-chown --- FAIL: TestConformance (13.23s) --- FAIL: TestConformance/copy-for-user (6.97s) --- PASS: TestConformance/copy-for-user-with-chown (6.26s) FAIL FAIL github.com/containers/buildah/tests/conformance 13.244s FAIL

So BuildKit is retaining the uid/gid of the files in the context directory, which are owned by your unprivileged user, instead of setting them to 0?

I guess it's expecting the client to hand it an archive with ownerships set to 0:0 and go-dockerclient isn't doing that.

Opened #5171 to generate the buildcontext archive we hand to dockerd with imagebuilder's helper, which sets ownerships in it to 0:0.

Thanks I verified against #5171 and it works, I have removed uid and gid skips, will rebase after #5171

nalind · 2023-11-16T18:24:02Z

tests/conformance/conformance_test.go

+			"(dir):test2:mtime",
+			"(dir):test:(dir):humans.txt:mtime",
+			"(dir):test:(dir):robots.txt:mtime",
+			"(dir):test:(dir):file:mtime",


This file comes from the build context, so I'd expect its datestamp to be consistent between different builds.

correct, this skip is not needed.

nalind · 2023-11-16T18:54:49Z

imagebuildah/stage_executor.go

@@ -348,6 +350,7 @@ func (s *StageExecutor) volumeCacheRestore() error {
 // imagebuilder tells us the instruction was "ADD" and not "COPY".
 func (s *StageExecutor) Copy(excludes []string, copies ...imagebuilder.Copy) error {
 	s.builder.ContentDigester.Restart()


What's the impact of this getting called in between adding heredocs and adding other content on the history and build caching?

Refactored now digester does not restarts between heredoc and regular copy. Before this it was affecting history of the image keeping only the most recent copy in the history.

flouthoc · 2023-11-17T11:14:55Z

@nalind Could you PTAL again.

nalind

Non-blocking nit, could probably use a rebase.

imagebuildah/stage_executor.go

Signed-off-by: Aditya R <arajan@redhat.com>

Following PR is a attempt to add `Heredoc` support to buildah. Once this PR is merged buildah is supposed to honor heredoc syntax while processing containerfiles Expected syntax to work ```Dockerfile FROM docker.io/library/python:latest RUN <<EOF echo "Hello" >> /hello echo "World!" >> /hello EOF RUN python3 <<EOF with open("/hello", "w") as f: print("Hello", file=f) print("Something", file=f) EOF RUN ls -a RUN cat hello ``` Signed-off-by: Aditya R <arajan@redhat.com>

nalind · 2023-11-17T15:51:48Z

/lgtm

TomSweeneyRedHat · 2023-11-17T16:12:07Z

LGTM
Now play nicely CI!

flouthoc · 2023-11-17T17:30:47Z

/hold cancel

TomSweeneyRedHat · 2023-11-17T17:43:23Z

Grats on getting this in @flouthoc ! Nice work again!

openshift-ci bot added the do-not-merge/work-in-progress label Oct 19, 2023

flouthoc marked this pull request as draft October 19, 2023 09:18

openshift-ci bot added the approved label Oct 19, 2023

flouthoc force-pushed the heredoc-experiment branch from 44c02b3 to 14cf481 Compare October 20, 2023 10:36

flouthoc force-pushed the heredoc-experiment branch 3 times, most recently from 891a27f to ab6f1fd Compare October 23, 2023 09:01

flouthoc marked this pull request as ready for review October 23, 2023 09:02

flouthoc changed the title ~~[WIP] buildah: add heredoc support for RUN, COPY and ADD~~ buildah: add heredoc support for RUN, COPY and ADD Oct 23, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 23, 2023

openshift-ci bot added the do-not-merge/hold label Oct 23, 2023

flouthoc mentioned this pull request Oct 23, 2023

Containerfile: Add heredoc support for RUN, COPY and ADD openshift/imagebuilder#264

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023

flouthoc force-pushed the heredoc-experiment branch 2 times, most recently from 9828aad to b6bf870 Compare November 7, 2023 06:47

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2023

flouthoc force-pushed the heredoc-experiment branch from b6bf870 to 76da8e4 Compare November 10, 2023 15:30

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

flouthoc force-pushed the heredoc-experiment branch from 76da8e4 to 82fdf4c Compare November 10, 2023 15:35

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

flouthoc force-pushed the heredoc-experiment branch 2 times, most recently from cf0e396 to bc1f310 Compare November 10, 2023 15:42

flouthoc requested a review from nalind November 10, 2023 15:43

nalind requested changes Nov 16, 2023

View reviewed changes

flouthoc force-pushed the heredoc-experiment branch 3 times, most recently from 34a2111 to d90ffec Compare November 16, 2023 18:11

nalind reviewed Nov 16, 2023

View reviewed changes

flouthoc force-pushed the heredoc-experiment branch from d90ffec to 7203f93 Compare November 16, 2023 18:26

flouthoc requested a review from nalind November 16, 2023 18:27

nalind reviewed Nov 16, 2023

View reviewed changes

flouthoc force-pushed the heredoc-experiment branch from 7203f93 to 88e198e Compare November 17, 2023 05:48

flouthoc requested a review from nalind November 17, 2023 05:50

flouthoc force-pushed the heredoc-experiment branch 2 times, most recently from 5f97532 to 017af77 Compare November 17, 2023 10:58

nalind reviewed Nov 17, 2023

View reviewed changes

imagebuildah/stage_executor.go Outdated Show resolved Hide resolved

flouthoc added 2 commits November 17, 2023 21:17

vendor: bump imagebuilder to v1.2.6-0.20231110114814-35a50d57f722

69f52f8

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the heredoc-experiment branch from 8bcc377 to bbb55b8 Compare November 17, 2023 15:48

flouthoc requested a review from nalind November 17, 2023 15:48

openshift-ci bot assigned nalind Nov 17, 2023

openshift-ci bot added the lgtm label Nov 17, 2023

openshift-ci bot removed the do-not-merge/hold label Nov 17, 2023

openshift-merge-bot bot merged commit 07482ae into containers:main Nov 17, 2023

flouthoc mentioned this pull request Nov 17, 2023

Heredoc notation doesn't work #3474

Closed

TomSweeneyRedHat mentioned this pull request Nov 20, 2023

[v4.8] Bump to 4.8.0-rc1 containers/podman#20724

Merged

github-actions bot added the locked - please file new issue/PR label Feb 16, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2024

Conversation

flouthoc commented Oct 19, 2023

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?

Uh oh!

flouthoc commented Oct 19, 2023

Uh oh!

openshift-ci bot commented Oct 19, 2023

Uh oh!

packit-as-a-service bot commented Oct 20, 2023

Uh oh!

flouthoc commented Oct 23, 2023

Uh oh!

TomSweeneyRedHat commented Oct 24, 2023

Uh oh!

flouthoc commented Oct 25, 2023

Uh oh!

flouthoc commented Nov 10, 2023

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Nov 17, 2023

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nalind commented Nov 17, 2023

Uh oh!

TomSweeneyRedHat commented Nov 17, 2023

Uh oh!

flouthoc commented Nov 17, 2023

Uh oh!

TomSweeneyRedHat commented Nov 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flouthoc Nov 16, 2023 •

edited

Loading