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
buildah: add heredoc
support for RUN
, COPY
and ADD
#5092
buildah: add heredoc
support for RUN
, COPY
and ADD
#5092
Conversation
Needs: openshift/imagebuilder#264 |
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
44c02b3
to
14cf481
Compare
Ephemeral COPR build failed. @containers/packit-build please check. |
891a27f
to
ab6f1fd
Compare
heredoc
support for RUN
, COPY
and ADD
heredoc
support for RUN
, COPY
and ADD
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 |
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 |
9828aad
to
b6bf870
Compare
b6bf870
to
76da8e4
Compare
76da8e4
to
82fdf4c
Compare
cf0e396
to
bc1f310
Compare
This PR is read for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly nits and a conformance test question.
imagebuildah/stage_executor.go
Outdated
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, "<<-") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"(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*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What values does it use? Why is it okay to ignore the differences?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #5171 to generate the buildcontext archive we hand to dockerd with imagebuilder's helper, which sets ownerships in it to 0:0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
34a2111
to
d90ffec
Compare
"(dir):test2:mtime", | ||
"(dir):test:(dir):humans.txt:mtime", | ||
"(dir):test:(dir):robots.txt:mtime", | ||
"(dir):test:(dir):file:mtime", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file comes from the build context, so I'd expect its datestamp to be consistent between different builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, this skip is not needed.
d90ffec
to
7203f93
Compare
@@ -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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the impact of this getting called in between adding heredocs and adding other content on the history and build caching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7203f93
to
88e198e
Compare
5f97532
to
017af77
Compare
@nalind Could you PTAL again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nit, could probably use a rebase.
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>
8bcc377
to
bbb55b8
Compare
/lgtm |
LGTM |
/hold cancel |
Grats on getting this in @flouthoc ! Nice work again! |
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
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?
Closes: #3474