Skip to content
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

Support for passing build-time variables in build context #15182

Merged
merged 2 commits into from Sep 17, 2015

Conversation

Projects
None yet
@mapuri
Copy link
Contributor

mapuri commented Jul 30, 2015

This PR builds on top of the changes proposed in #9176 and addresses the additional requirements for build-time variables as tracked by #14634

Below is a quick usage and overview of the feature. For more context on the use-cases this enables, please refer the discussion in #9176 and #14634 :

Usage:

docker build --build-arg http_proxy=http://my.proxy.url  --build-arg foo=bar <<MARK
FROM busybox
RUN <command that need http_proxy>
ARG --description="foo's description" foo
USER $foo
MARK

Brief overview:

  • The build-time variables are passed as environment-context for command(s)
    run as part of the RUN primitve. These variables are not persisted in environment of
    intermediate and final images when passed as context for RUN. The build environment
    is prepended to the intermediate continer's command string for aiding cache lookups.
    It also helps with build traceability. But this also makes the feature less secure from
    point of view of passing build time secrets.
  • The build-time variables also get used to expand the symbols used in certain
    Dockerfile primitves like ADD, COPY, USER etc, without an explicit prior definiton using a
    ENV primitive. These variables get persisted in the intermediate and final images
    whenever they are expanded.
  • The build-time variables are only expanded or passed to the RUN primtive if they
    are defined in Dockerfile using the ARG primitive or belong to list of built-in variables.
    HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy, FTP_PROXY and NO_PROXY are built-in
    variables that needn't be explicitly defined in Dockerfile to use this feature.
@@ -17,6 +17,7 @@ weight=1

-f, --file="" Name of the Dockerfile (Default is 'PATH/Dockerfile')
--force-rm=false Always remove intermediate containers
--build-env=[] Set build-time environment variables

This comment has been minimized.

@icecrime

icecrime Jul 30, 2015

Contributor

--build-arg

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Jul 30, 2015

Thanks so much @mapuri! I just gave docs/reference/commandline/build.md a quick read, and it seems to perfectly match what has been discussed <3

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 30, 2015

Personal opinion: i don't like the --description flag on ARG :S

MacAddress string // Mac Address of the container
OnBuild []string // ONBUILD metadata that were defined on the image Dockerfile
Labels map[string]string // List of labels set to this container
TrustedBuildArgs map[string][]string // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.

This comment has been minimized.

@cpuguy83

cpuguy83 Jul 31, 2015

Contributor

A little weary of calling these TrustedBuildArgs as Trusted Build has some historical meaning in the project... and also don't want to conflate these with the whole distribution/trust nomenclature.

This comment has been minimized.

@mapuri

mapuri Jul 31, 2015

Author Contributor

ok, makes sense.

I used TrustedBuildArgs just to bring out the fact that this is the list of variables (defined using ARG) which a Dockerfile author trusts it's ok to pass values to the builder.

We can also call them AllowedBuildArgs or DefinedBuildArgs? WDYT

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Jul 31, 2015

This looks pretty good, but wondering should be force a default value for ARGs... this would also simplify the syntax and pretty much match ENV.

@mapuri

This comment has been minimized.

Copy link
Contributor Author

mapuri commented Jul 31, 2015

@tiborvass

regarding --description flag on ARG, are you saying you don't see need for it? Or does it need to be renamed? I assume former. If so as I understand the need for this flag comes from the desire to document and persist info about the ARG like it's expected use, purpose etc. Personally I feel it is good to have but I am fine removing it. It's an optional flag so the author still has a choice to not use it.

@cpuguy83

wondering should be force a default value for ARGs

An optional default value for ARG offers the flexibility to not expand or pass ARG around if it is not passed at all by --build-arg i.e. it just acts as a placeholder of values that can be accepted as a --build-arg but doesn't imply a value to be assumed on it's own. I think this comment from @duglin #9176 (comment) brings out a need for this well.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 2, 2015

Wondering how we should go about this change, given that the ROADMAP.md mentions we won't be accepting changes to the Dockerfile format. I know other proposals were turned down (or put on hold) because of that, and accepting this PR may lead to complaints.

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Aug 3, 2015

@thaJeztah I know, but the number of people complaining about that missing feature is just ridiculously high, and we have been discussing this in grouped review: I think we should take it.

@shouze

This comment has been minimized.

Copy link
Contributor

shouze commented Aug 4, 2015

Can't wait this one to be merged, this is effectively a huge improvement for builds.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Aug 4, 2015

Personally I'd still prefer to require a default for an arg, then later we can add the flag if it is really truly necessary.
Also agree we don't need --description since these can be documented with comments... and again if we do find it is necessary we can add the flag later.

What do we need to do to move this forward?

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

re: default value - I like the idea of a default value but I would make it optional otherwise we have no way of representing "not set at all" - the best we could do is set it to "" which isn't the same thing.

ARG foo    # defines 'foo' but no default value, which means it doesn't show up in "RUN env"
           # output unless --build-arg is used
ARG foo=bar   # foo set to 'bar' and "RUN env" shows "foo=bar"
ARG foo bar   # same as previous

and in all 3 cases, --build-arg foo=cat would result in "RUN env" showing "foo=cat"

re: --description - I actually see value in this. If there are 3rd party tools that wish to be able to prompt people for info prior to doing a build then this is a nice programmatic way to extract info about the arg. I do agree that we could technically add it later if we wanted, but I'm +1 for keeping it. However, its really only useful if that info is stored in the image since we wouldn't be able to query for inherited ARG's descriptions w/o it. Haven't looked at the code to know if this is saved in the image or not. @mapuri ?

}
headers.Add("X-Docker-BuildArgs", base64.URLEncoding.EncodeToString(buf))

if context != nil {

This comment has been minimized.

@duglin

duglin Aug 4, 2015

Contributor

I know its premature since we're still in design review, but is this "context" stuff part of this PR or due to some other issue?

This comment has been minimized.

@mapuri

mapuri Aug 5, 2015

Author Contributor

Somehow missed this comment. yes, it's not part of this patch. It might have come from code change in this area and rebasing my diffs over them. I will remove it.

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

Couple of comments:
1 - the idea of an image author being able to, in essence, say "here are some variables that a builder may want to set/override" is nice. Documenting this as part of an image is goodness - especially if we include the "description" field in the image metadata - which should be possible based on looking at the code.
2 - right now if someone specifies a "build-arg" that isn't also named via a "ARG" then it is ignored. We should either flag it as an error so the user knows they specified useless info, or we should use it anyway. I'd prefer to use it.

Either way we go on # 2 I'm ok with the design and suggest we move it to code review.

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

rebase needed

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Aug 4, 2015

I'd prefer to error out, I think the point of the design is to make sure args are specified in the Dockerfile.

@@ -1334,6 +1334,19 @@ Query Parameters:

- **Content-type** – Set to `"application/tar"`.
- **X-Registry-Config** – base64-encoded ConfigFile object
- **X-Docker-BuildArgs** – base64-encoded JSON map of string pairs for build-time

This comment has been minimized.

@thaJeztah

thaJeztah Aug 4, 2015

Member

I think there was some discussion on this in the previous PR (not sure); is there a specific reason to pass these args as a header, and not just add it to the query-parameters? e.g. &arg[foo]=bar&arg[bar]=baz?

This comment has been minimized.

@mapuri

mapuri Aug 4, 2015

Author Contributor

No specific reason, I saw config being passed as header so I used same approach for build-args as well. If there is a precedence or liking to do it through query-parameters I can look into changing this to query-parameters.

This comment has been minimized.

@thaJeztah

thaJeztah Aug 4, 2015

Member

I'd be +1 to query params ( don't like the other header as well)

This comment has been minimized.

@duglin

duglin Aug 4, 2015

Contributor

We should be consistent though - unless we're going to move all of 'em to query params I'm not sure it makes sense to just move this one.

This comment has been minimized.

@thaJeztah

thaJeztah Aug 4, 2015

Member

my thought was to not add more; using headers to send data is (IMO), well, weird? Also, we're already using query params.

@mapuri

This comment has been minimized.

Copy link
Contributor Author

mapuri commented Aug 4, 2015

thanks for the review folks.

@duglin

1 - the idea of an image ...
... which should be possible based on looking at the code.

Yes the ARG description is stored as part of image metadata (i.e. they can queried using docker inspect).

2 - right now if someone specifies a "build-arg" ...
... We should either flag it as an error so the user knows they specified useless info,

Yeah, I had thought about this. I intentionally left the behavior to ignore as I couldn't think of a better/natural handling in a case where an ARG is defined at a line after it is accessed:

Just to show it with a made up scenario where I don't think it is ok to fail:

$ docker build --build-arg user=foo - << MARK
USER ${user:-bar}         <<< right now, this just evaluates to 'bar' and behaves in similar way USER primitive will behave today. Note we don't want to use the `-build-arg` value yet as it is not defined till this point. Similarly, it doesn't seem ok to error this build out as well.
ARG user
USER $user         <<< Since now ARG is defined, this shall use the value passed from cli i.e. foo
MARK

or we should use it anyway. I'd prefer to use it.

Since we are introducing ARG primitive to provide a list of allowed args I think it might not be ok to use the build-args until the point they are defined in Dockerfile.

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

@mapuri I agree that your second scenario should not fail, but that's not the case I was thinking of. If we do want to error on the case of "unknown --build-args", then by the time we reach the end of the Dockerfile, any arg that was specified via "--build-arg" that was not used should cause an error to be generated.

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

I noticed that the default build args are saved in the image - I don't think we should do that for a couple of reasons:
1 - it'll add extra noise to images that don't know or care about build-args
2 - the list of default build-args should be configurable based on the install/admin. Meaning, unless someone explicitly used ARG to add "http_proxy" then it should be up to the admin of the docker daemon to decide if they want "http_proxy" to be available via --build-args. So, let's add a config field to allow admins to define this. Absence of the property means the builder will use the default list, but presence of the property with a value (or empty list) means they don't want any default ones to be available except the ones mentioned.
3 - related to # 2, over time the list of default args that Docker has may change (grow or shrink) and I wouldn't want to force everyone to have to rebuild all of their images just to have that new list applied.

@mapuri mapuri force-pushed the mapuri:build-arg branch from 9611138 to c153483 Aug 4, 2015

@mapuri

This comment has been minimized.

Copy link
Contributor Author

mapuri commented Aug 4, 2015

ah, ok... that should work. I will look at if there is a post build step where I can put this validation and fail the fail the build in case there are one or more unused --build-args.

@icecrime

This comment has been minimized.

Copy link
Contributor

icecrime commented Aug 4, 2015

  • Regarding handling of extraneous --build-arg, I agree with @cpuguy83 here #15182 (comment): we should ignore or reject them, but not silently accept them.
  • I'm not sure I see the point of making the default set of build arguments configurable by the admin as long as we keep this default set general purpose and harmless (which I believe HTTP_PROXY and such absolutely are). What would be the incentive to restrict this list and make it impossible to specify a proxy?
@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 4, 2015

re: configurable - its about who knows what's best. I'm very uncomfortable with the assumption that we, right now, know what the valid list of default ARGs should be for all installs. We've focused on proxy type of stuff, but perhaps some installs have other env vars (or other variants of proxy env vars) that they want to allow w/o having to modify every single image or Dockerfile. As I said, I'm ok with us having a predefined list but let's not make it set in stone such that it can't be changed w/o a PR.

@mapuri

This comment has been minimized.

Copy link
Contributor Author

mapuri commented Aug 4, 2015

@duglin

I agree with '1' but I save built-in args to the image so that they show-up and are documented in a way identical to author defined args. This saves some special handling for these other than just adding them to list of trusted/allowed args when the build starts. Note that the Dockerfile author may decide to override the default values and description of built-in args in which case it will be desirable to document them.

Having said above, I am ok not saving them in image metadata if they are not overridden by the author. Let me know.

2 - the list of default build-args should be configurable based on the install/admin.

This does provides more flexibility but we will still need some sort of builtin list when a list is not specified by admin (sort of default for a default :)). I think starting off with just a built-in list might be ok for now and we can add this over this change as a separate PR if needed?

3 - related to # 2, over time the list of default args that Docker has may change (grow or shrink) and I wouldn't want to force everyone to have to rebuild all of their images just to have that new list applied.

This is somewhat taken care of in case the list grows here. If an image (img1) was built with say an old built-in list of args. And now someone wants to build a new image using img1 as base and new builder code that has additional built-in args then we add the additional args without needing to build img1.

But I don't remove stuff if the list shrinks as right now the args are inherited from parent image and this somewhat goes back to no special handling for built-in args argument above i.e. if parent image brings in args (including built-in ones) then we don't remove them, which is ok imo as consider parent image might be doing something with those built-in args (like ONBUILD steps?).

@Thell

This comment has been minimized.

Copy link
Contributor

Thell commented Oct 13, 2015

Love the --build-args and that it has finally been added! But, why should it be a failure to not consume a build arg? What's the logic there?

Hopefully this handling is still open for discussion.

Imagine a shell script that builds all the *.dockerfiles in a path. A simple example might be

#!/bin/bash -ex

args="$@"

time for file in $(ls ./[0-9][0-9]-*); do
  tag=$(basename "$file" .dockerfile)
  tag="${tag#*-}"
  time docker build ${args} \
      --build-arg USER=docker \
      --build-arg USERNAME="Docker Container" \
      --build-arg USEREMAIL=docker@example \
      -f "$file" \
      -t "$tag:latest" ./
    wait || exit $?
done

Where only some of the dockerfiles would make use of these args.

By failing, a check needs to be done prior to the build call to determine which args would be used...

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Oct 13, 2015

I believe the logic was to avoid the cases where someone specified a build arg thinking it was going to be used, only to have it totally ignored. There would be no indication of this to the end user. To many people they view this as "parameterizing a build" - so it would be the equivalent of calling foo(1,2) when foo only accepted one arg, not two.

@Thell

This comment has been minimized.

Copy link
Contributor

Thell commented Oct 13, 2015

A warning could suffice, yes? I don't recall a build tool failing purposefully because of extra environmental values.

@bixu

This comment has been minimized.

Copy link

bixu commented Oct 14, 2015

I assume this feature will not be available until Docker and Toolbox 1.9+ are released?

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 14, 2015

@bixu it will be generally available then, but release candidates for 1.9 can be found here: #17000 (comment)

@bixu

This comment has been minimized.

Copy link

bixu commented Oct 14, 2015

I'll have a look if I can make the time. Is there an approximate ETA for 1.9 GA?

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Oct 14, 2015

@bixu yup, October 29th is our target. (see https://github.com/docker/docker/milestones).

I also asked for Docker toolbox release-candidates, and that's being worked on, pending release candidates for the other projects that are included in toolbox.

@Thell

This comment has been minimized.

Copy link
Contributor

Thell commented Oct 14, 2015

Please consider using Warningf instead of Errorf when len(leftoverArgs) > 0 in evaluator.go.

Something like "Ignoring --build-args [foo,bar,baz] that were not used in dockerfile ARG statements."

The only true call for an error here was

I'd prefer to error out, I think the point of the design is to make sure args are specified in the Dockerfile. ~ @cpuguy83

Respectfully; nothing in the design requires consumption in any the ENV, COPY, ADD or RUN statements. The build-args are explicitly ignored unless explicitly expanded with ARG, right? That silent ignore that @maaquib originally implemented seems to be partially what prompted

right now if someone specifies a "build-arg" that isn't also named via a "ARG" then it is ignored. We should either flag it as an error so the user knows they specified useless info, or we should use it anyway. I'd prefer to use it. ~ @duglin

Its a single word change, yes? Is there a usability or security concern regarding passing --build-arg when there is no corresponding ARG beyond the user wondering why something didn't get used?

`ARG` instruction, any use of a variable results in an empty string.

> **Note:** It is not recommended to use build-time variables for
> passing secrets like github keys, user credentials etc.

This comment has been minimized.

@brynary

brynary Nov 16, 2015

Can someone perhaps clarify what is recommended for build-time secrets that should not be present in the Docker image?

This comment has been minimized.

@thaJeztah

thaJeztah Nov 17, 2015

Member

it depends on your situation for a discussion around handling secrets in general, see #13490

@TheFriendlyCoder

This comment has been minimized.

Copy link

TheFriendlyCoder commented Jan 3, 2017

I see this issue has been rsolved for quite some time, but in the off chance someone is still monitoring the comment thread I'd like to know if anyone ever got around to creating an issue or pull request to add a feature for customizing the default list of build args supported by Docker. There are several comments in the thread above that suggests doing so, but I couldn't find any links to work subsequently done to move this forward.

If anyone has any links or references they can share I'd appreciate it.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jan 3, 2017

@TheFriendlyCoder the list of build-arg that are currently allowed by default can be found here; /builder/dockerfile/builder.go#L40-L50. If there's particular build-args you are missing, please open a new issue (with a description of your use case)

@TheFriendlyCoder

This comment has been minimized.

Copy link

TheFriendlyCoder commented Jan 3, 2017

@thaJeztah
Thanks for the quick response. I assume based on your comment there is currently no way to customize the default list of build arguments other than by updating the "master list" within the Docker source code, correct?

If so, do you know if anyone has created an issue / request to make this list customizable?

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jan 3, 2017

There's no such issue/request currently, so feel free to open one. We should properly look into it from a "portability" perspective though, as changing the defaults means that a Dockerfile can only be built if those defaults are set on a daemon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.