Treat args the same way `docker build` does #151

carbonin · 2020-03-13T14:31:58Z

Before this change a Dockerfile like this:

FROM alpine
ARG SECRET
RUN echo $SECRET

FROM alpine
RUN echo "$SECRET" > test_file

would expose the arg "SECRET" in every layer of the build.

Now the ARG is only accessible in the layer it is defined.
This gives us parity with the Docker behavior described in
https://docs.docker.com/engine/reference/builder/#scope

"An ARG instruction goes out of scope at the end of the build
stage where it was defined. To use an arg in multiple stages,
each stage must include the ARG instruction."

This was originally reported as containers/buildah#2210
cc @rhatdan

I wasn't sure how to run the tests locally so I hope this one works... 😆

EDIT:

This PR now attempts to handle all the gaps between imagebuilder's ARG handling and docker build's, one of which was the issue described above and in containers/buildah#2210

I have tried to create test cases or edit existing ones to cover all of the cases, but I'll also list them here.

An ARG should only be available after its ARG command in the current stage

This is the original problem, described above. As well as the issue raised by @bparees in #151 (comment)

A later ARG default value should override an earlier one in the same stage

FROM alpine
ARG FOO=foo
ARG FOO=bar
RUN echo "$FOO"

The above Dockerfile should print "bar". Before this patch, the behavior was the opposite, an arg was not changed once set (this old behavior effectively achieved the user-provided case, but the combination of the two necessitated a new Builder struct field for user args).

User provided args should override all args default values

ARG TAG=latest
FROM centos:$TAG
ARG TAG
RUN echo "$TAG"

FROM alpine
ARG TAG="1.2.3"
RUN echo "$TAG"

Building the above Docerfile with --build-arg TAG=7 should build the first stage from centos:7 and print "7" in both stages.

openshift-ci-robot · 2020-03-13T14:32:12Z

Hi @carbonin. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

carbonin · 2020-03-13T14:35:43Z

/assign @nalind

rhatdan · 2020-03-13T16:38:16Z

/approve

rhatdan · 2020-03-13T16:38:43Z

@TomSweeneyRedHat PTAL

rhatdan · 2020-03-13T16:45:22Z

@bparees PTAL

bparees · 2020-03-13T17:07:32Z

will this handle:

ARG FOO=centos
FROM $FOO
RUN echo hello $FOO

which in docker at least, does substitute in the FROM line, but does not do anything in the RUN:

$ docker build --pull --no-cache .
Sending build context to Docker daemon   5.12kB
Step 1/4 : ARG FOO=centos
Step 2/4 : FROM $FOO
latest: Pulling from library/centos
Digest: sha256:fe8d824220415eed5477b63addf40fb06c3b049404242b31982106ac204f6700
Status: Image is up to date for centos:latest
 ---> 470671670cac
Step 3/4 : RUN echo hello $FOO
 ---> Running in 695dfdf3b572
hello
Removing intermediate container 695dfdf3b572
 ---> 09813fb15eec
Step 4/4 : RUN exit 1
 ---> Running in 23dcc64b005b
The command '/bin/sh -c exit 1' returned a non-zero code: 1

carbonin · 2020-03-13T17:22:14Z

@bparees actually it looks like that's an error with this change.

master branch:

$ ../imagebuilder .
--> Image centos was not found, pulling ...
--> FROM centos as 0
--> RUN echo hello $FOO
hello centos
--> Committing changes ...
--> Done

This patch:

$ ../imagebuilder .
json: cannot unmarshal array into Go value of type docker.Image

Will look into that when I get a chance.

carbonin · 2020-03-13T19:58:37Z

@bparees It looks like

imagebuilder/builder_test.go

Line 165 in 47f96c1

func TestHeadingArg(t *testing.T) {

should fail in this case, right?

Sorry for the basic question, but how do I run that test case?

It doesn't seem to run using make test as those passed for me before submitting this.
make test-conformance fails for me with this change and on master with this error:

$ make test-conformance
go test -v -tags conformance -timeout 10m ./dockerclient
# github.com/openshift/imagebuilder/dockerclient
dockerclient/archive.go:227:11: Errorf format %w has unknown verb w
FAIL	github.com/openshift/imagebuilder/dockerclient [build failed]
make: *** [Makefile:10: test-conformance] Error 2

bparees · 2020-03-13T20:05:01Z

should fail in this case, right?

i don't think that would fail, it's just going to resolve to FROM busybox: (empty tag).

which means it's not a great test.

bparees · 2020-03-13T20:05:57Z

oh sorry, i see what you mean. i do not know, sorry. this really isn't a repo i deal w/ normally

carbonin · 2020-03-13T20:20:17Z

@TomSweeneyRedHat ?

TomSweeneyRedHat · 2020-03-13T22:19:30Z

Sorry @carbonin I've been buried in a gazillion things recently. Hope to get a good look this weekend.

TomSweeneyRedHat · 2020-03-14T21:07:13Z

builder.go

@@ -264,6 +264,16 @@ func extractNameFromNode(node *parser.Node) (string, bool) {
 	return n.Next.Value, true
 }

+func argsForStage(stageRoot *parser.Node, allArgs map[string]string) map[string]string {


For history's sake, it would be good to have a comment for this function talking about what you expect it to do.

TomSweeneyRedHat · 2020-03-14T21:09:51Z

I too have not been in this project for several months, but I know @nalin has been quite a bit. So if he says anything different than I, trust his word.

First off, something is off with archive.go. I've just fixed that in #152, however now the conformance tests are failing in a number of places with that fix in play. I'm guessing that the conformance tests are not being regularly run.

To run a particular test in builder_test.go, you need to do (-v (verbose) is optional, but very helpful):

go test -v -run TestByTarget where TestByTarget is a function name in bulider_test.go.
or
go test -v -tags archive ./dockerclient #archive tests
or
go test -v -tags conformance ./dockerclient #conformance tests

But those too are also not passing. However, your new test seems to be working:

# go test -v -run TestMultiStageArgScope
=== RUN   TestMultiStageArgScope
--- PASS: TestMultiStageArgScope (0.00s)
PASS
ok  	github.com/openshift/imagebuilder	0.004s

Once you chase down the problem above, it would be good to get @nalin to eyeball this.

carbonin · 2020-03-15T01:18:04Z

Okay I added a commit to alter an existing test case to deal with the situation from @bparees's comment

But while investigating this behavior and how to deal with it I found https://docs.docker.com/engine/reference/builder/#understand-how-arg-and-from-interact#understand-how-arg-and-from-interact which complicates things a bit.

What behavior should we be striving for here? Are we trying to emulate docker exactly? There seems to be considerable confusion around this particular command combination: moby/moby#34129

Right now we're treating the "heading" args as global and applying them to all the following commands, but Docker's handling is only applying the heading args to FROM commands unless they are re-"defined" in a particular stage (in which case the default, if any, is inherited from the first definition).

Is it safe to assume we don't want to tackle all of that in this patch? With that assumption, how far should I take this one? Just fix the test in the new commit I added?

I think restricting the scope of the heading args would be a rather breaking change for users of buildah, but maybe just as breaking as scoping args to a particular stage.

carbonin · 2020-03-15T01:18:52Z

Oh and the edited test now fails like this with my changes:

$ go test -v -run TestMultiStageParseHeadingArg
=== RUN   TestMultiStageParseHeadingArg
--- FAIL: TestMultiStageParseHeadingArg (0.00s)
    builder_test.go:170: expected golang:1.9, got :
FAIL
exit status 1
FAIL	github.com/openshift/imagebuilder	0.003s

bparees · 2020-03-15T04:41:35Z

What behavior should we be striving for here? Are we trying to emulate docker exactly?

for imagebuilder itself i don't have a strong opinion, but for buildah as a consumer of imagebuilder, i think buildah should be emulating docker including all its...idiosyncrasies, since it is routed as a drop-in/aliasable replacement for "docker build"

it may be worth buildah having a "--sane" mode which drops those compatbility behaviors in deference to preferred behavior. (thinking about the recent COPY behavior discoveries we have made)

TomSweeneyRedHat · 2020-03-16T14:26:06Z

I'd just tweak @bparees expectation comment slightly. buildah bud --layers should be equivalent to docker build. We advertise that podman build is equivalent to docker build. Podman calls buildah bud under the covers and when it does so it adds the --layers option.

carbonin · 2020-03-16T14:50:18Z

Okay, so let me ask a more specific question. How do we want NewStages to behave?

Each stage returned by that function has a Builder with args. Should every builder have access to the args defined before the first FROM command when returned from NewStages? I assume we also want to define explicitly how something really awful like this would behave, right?

ARG FOO=centos
ARG BAZ=things

FROM $FOO
RUN echo hello $FOO

FROM alpine
RUN echo hello $FOO
ARG FOO=alpine

FROM $FOO
ARG BAZ
RUN echo hello $FOO
RUN echo hello $BAZ

For what it's worth, right now it's like this:

$ ../imagebuilder .
--> FROM centos as 0
--> RUN echo hello $FOO
hello centos
--> FROM alpine as 1
--> RUN echo hello $FOO
hello centos
--> ARG FOO=alpine
--> FROM centos as 2
--> ARG BAZ
--> RUN echo hello $FOO
hello centos
--> RUN echo hello $BAZ
hello things
--> Committing changes ...
--> Done

Buildah can post-process those stages to get what we want there. But what, for this project, do we want the "correct" behavior to be?

carbonin · 2020-03-16T14:54:17Z

Note that in the previous example the first ARG FOO default value is not overridden by the next default even though nothing was passed in the command line

rhatdan · 2020-03-16T19:25:08Z

We need to match docker build behaviour when working with Dockerfiles.

carbonin · 2020-03-19T02:06:14Z

Okay, this now tries to tackle all of the gaps I could find between imagebuilder and docker relating to ARG handling.

I found one more bug that I'm still trying to work out, but would appreciate a look at what I have here as this is getting quite involved.

rhatdan · 2020-03-19T10:25:28Z

@carbonin Could you open a WIP vendor of this in Buildah to make sure it passes the Buildah CI tests.

rhatdan · 2020-03-19T10:28:44Z

The code looks good to me, and I like that there are lots of tests. The issue is probably getting people more in the know of the deep level of how this is supposed to work.

@kolyshkin PTAL

rhatdan · 2020-03-19T15:55:08Z

/ok-to-test

carbonin · 2020-03-19T20:32:29Z

I've went full circle here and removed the method I added in the first commit 😅

I can squash these if that's preferred ...

carbonin · 2020-03-19T20:44:01Z

Could you open a WIP vendor of this in Buildah to make sure it passes the Buildah CI tests.

@rhatdan added a commit in containers/buildah#2220 to include my branch as the imagebuilder source

rhatdan · 2020-03-20T14:26:17Z

@carbonin Thanks, it will give me better confidence on this if it can pass Buildah and imagebuilder tests.

carbonin · 2020-03-24T16:57:44Z

@rhatdan Seeing as how this is passing tests here and the only tests failing in buildah with this branch are the ones I added to expose this problem, can we move forward with this patch?

Is there someone else I should contact for review?

rhatdan · 2020-03-25T17:30:59Z

lgtm
@nalind @TomSweeneyRedHat @bparees PTAL

There were a few inconsistencies in the way imagebuilder handled arguments vs the way that docker handles them. This commit addresses those gaps. Specifically: - Subsequent ARG commands with default values override previous ones, but they don't override the values passed on the command line. - Heading args (ARG commands before the first FROM) are applied only to FROM commands unless a matching ARG command exists in a particular stage in which case the value from the heading arg carries forward into that stage. This was accomplished by creating a dedicated Builder struct field for user args and heading args so that they can be tracked separately from args declared with ARG commands in individual stages. Tracking these all separately allows us to apply these priority rules as the build progresses. Signed-off-by: Nick Carboni <ncarboni@redhat.com>

nalind · 2020-03-25T21:58:38Z

Patches look reasonable, and the behavior is consistent with what I can observe with the moby-engine-18.09.7-5.ce.git2d0083d.fc30 package on Fedora 30. LGTM.

rhatdan · 2020-03-25T22:44:35Z

/lgtm

openshift-ci-robot · 2020-03-25T22:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, rhatdan

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

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

2220: Scope build args to a single stage in a multi-stage build r=rhatdan a=carbonin  #### What type of PR is this?  /kind bug #### What this PR does / why we need it: This PR uses the changes made in openshift/imagebuilder#151 to handle arguments the same way `docker build` does. In particular, it scopes arguments to the stage in which they are defined and only records arguments in a layer's history if they could have been used in that layer. #### How to verify it ``` $ cat Dockerfile FROM alpine ARG THING FROM alpine RUN echo "$THING" > things $ buildah bud --layers --build-arg THING=things STEP 1: FROM alpine STEP 2: ARG THING --> Using cache 232af6ca4a94e52dbef13f6da08c62b4172eaff7ee2e93cab08aceb4b00e6f81 STEP 3: FROM alpine STEP 4: RUN echo "$THING" > things --> Using cache fad9788d65a3062cc823516c8fff73b39e914463c709149a2855cbea61a10abe fad9788d65a3062cc823516c8fff73b39e914463c709149a2855cbea61a10abe $ podman run --rm fad9788d65a3062cc823516c8fff73b39e914463c709149a2855cbea61a10abe cat things things ``` The above `podman run` command should return an empty file #### Which issue(s) this PR fixes:  Fixes #2210 #### Does this PR introduce a user-facing change?  Yes, it significantly changes the way arguments behave. While it's not a change to how `buildah` would be used. Images built with the same Dockerfile before and after this change could be very different. ```release-note Altered the behavior of the `--build-arg` flag and `ARG` commands to mirror `docker build`. In particular, the following behaviors have changed: - An ARG is only available after its ARG command in the current stage. - Previously, anything provided using the --build-arg flag could be accessed in any stage. After this change, accessing a build arg provided on the command line will require a corresponding `ARG` command in the stage before it is accessed. - Additionally, "heading" args (ARG commands before the first FROM) also now require an additional ARG declaration in the stage to be accessed. Previously, they were accessible without the additional ARG command. - A later ARG default value should override an earlier one in the same stage FROM alpine ARG FOO=foo ARG FOO=bar RUN echo "$FOO" The above Dockerfile should print "bar". Previously, the behavior was the opposite, an arg was not changed once set. Generally this makes buildah handle args as described in https://docs.docker.com/engine/reference/builder/#arg ``` Co-authored-by: Nick Carboni <ncarboni@redhat.com>

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2020

openshift-ci-robot requested review from mrunalp and TomSweeneyRedHat March 13, 2020 14:32

carbonin mentioned this pull request Mar 13, 2020

buildah shows build-arg values in final image of multi-stage build containers/buildah#2210

Closed

openshift-ci-robot assigned nalind Mar 13, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2020

TomSweeneyRedHat reviewed Mar 14, 2020

View reviewed changes

carbonin force-pushed the scope_args_to_stage branch from 37304af to 0032c32 Compare March 15, 2020 01:10

carbonin force-pushed the scope_args_to_stage branch from 0032c32 to f2e3dd0 Compare March 17, 2020 01:30

carbonin changed the title ~~Scope build args to the layer they are defined in~~ [WIP] Scope build args to the layer they are defined in Mar 19, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2020

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 19, 2020

carbonin changed the title ~~[WIP] Scope build args to the layer they are defined in~~ Treat args the same way docker build does Mar 19, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 19, 2020

bparees mentioned this pull request Mar 24, 2020

oc start-build and pass ImageStreamTag version openshift/origin#19483

Closed

carbonin mentioned this pull request Mar 25, 2020

Scope build args to a single stage in a multi-stage build containers/buildah#2220

Merged

carbonin force-pushed the scope_args_to_stage branch from a7abcba to d569093 Compare March 25, 2020 18:50

openshift-ci-robot assigned rhatdan Mar 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

openshift-merge-robot merged commit 8de7174 into openshift:master Mar 25, 2020

carbonin deleted the scope_args_to_stage branch March 26, 2020 00:32

carbonin mentioned this pull request May 22, 2024

MGMT-17896: Add separate Dockerfile for assisted-service with an el8 base openshift/assisted-service#6347

Merged

20 tasks

nalind mentioned this pull request Sep 13, 2024

Podman incorrectly scopes build args when targeting a specific layer containers/podman#23939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat args the same way `docker build` does #151

Treat args the same way `docker build` does #151

carbonin commented Mar 13, 2020 •

edited

Loading

openshift-ci-robot commented Mar 13, 2020

carbonin commented Mar 13, 2020

rhatdan commented Mar 13, 2020

rhatdan commented Mar 13, 2020

rhatdan commented Mar 13, 2020

bparees commented Mar 13, 2020

carbonin commented Mar 13, 2020

carbonin commented Mar 13, 2020 •

edited

Loading

bparees commented Mar 13, 2020

bparees commented Mar 13, 2020

carbonin commented Mar 13, 2020

TomSweeneyRedHat commented Mar 13, 2020

TomSweeneyRedHat Mar 14, 2020

TomSweeneyRedHat commented Mar 14, 2020

carbonin commented Mar 15, 2020

carbonin commented Mar 15, 2020

bparees commented Mar 15, 2020

TomSweeneyRedHat commented Mar 16, 2020

carbonin commented Mar 16, 2020 •

edited

Loading

carbonin commented Mar 16, 2020

rhatdan commented Mar 16, 2020

carbonin commented Mar 19, 2020

rhatdan commented Mar 19, 2020

rhatdan commented Mar 19, 2020

rhatdan commented Mar 19, 2020

carbonin commented Mar 19, 2020

carbonin commented Mar 19, 2020

rhatdan commented Mar 20, 2020

carbonin commented Mar 24, 2020

rhatdan commented Mar 25, 2020

nalind commented Mar 25, 2020

rhatdan commented Mar 25, 2020

openshift-ci-robot commented Mar 25, 2020

Treat args the same way docker build does #151

Treat args the same way docker build does #151

Conversation

carbonin commented Mar 13, 2020 • edited Loading

An ARG should only be available after its ARG command in the current stage

A later ARG default value should override an earlier one in the same stage

User provided args should override all args default values

openshift-ci-robot commented Mar 13, 2020

carbonin commented Mar 13, 2020

rhatdan commented Mar 13, 2020

rhatdan commented Mar 13, 2020

rhatdan commented Mar 13, 2020

bparees commented Mar 13, 2020

carbonin commented Mar 13, 2020

carbonin commented Mar 13, 2020 • edited Loading

bparees commented Mar 13, 2020

bparees commented Mar 13, 2020

carbonin commented Mar 13, 2020

TomSweeneyRedHat commented Mar 13, 2020

TomSweeneyRedHat Mar 14, 2020

Choose a reason for hiding this comment

TomSweeneyRedHat commented Mar 14, 2020

carbonin commented Mar 15, 2020

carbonin commented Mar 15, 2020

bparees commented Mar 15, 2020

TomSweeneyRedHat commented Mar 16, 2020

carbonin commented Mar 16, 2020 • edited Loading

carbonin commented Mar 16, 2020

rhatdan commented Mar 16, 2020

carbonin commented Mar 19, 2020

rhatdan commented Mar 19, 2020

rhatdan commented Mar 19, 2020

rhatdan commented Mar 19, 2020

carbonin commented Mar 19, 2020

carbonin commented Mar 19, 2020

rhatdan commented Mar 20, 2020

carbonin commented Mar 24, 2020

rhatdan commented Mar 25, 2020

nalind commented Mar 25, 2020

rhatdan commented Mar 25, 2020

openshift-ci-robot commented Mar 25, 2020

Treat args the same way `docker build` does #151

Treat args the same way `docker build` does #151

carbonin commented Mar 13, 2020 •

edited

Loading

carbonin commented Mar 13, 2020 •

edited

Loading

carbonin commented Mar 16, 2020 •

edited

Loading