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
Merged
Changes from all commits
Commits
File filter
Filter file types
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.

Always

Just for now

@@ -35,6 +35,7 @@ import (
"github.com/docker/docker/pkg/units"
"github.com/docker/docker/pkg/urlutil"
"github.com/docker/docker/registry"
"github.com/docker/docker/runconfig"
"github.com/docker/docker/utils"
)

@@ -64,6 +65,8 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
flCPUSetCpus := cmd.String([]string{"-cpuset-cpus"}, "", "CPUs in which to allow execution (0-3, 0,1)")
flCPUSetMems := cmd.String([]string{"-cpuset-mems"}, "", "MEMs in which to allow execution (0-3, 0,1)")
flCgroupParent := cmd.String([]string{"-cgroup-parent"}, "", "Optional parent cgroup for the container")
flBuildArg := opts.NewListOpts(opts.ValidateEnv)
cmd.Var(&flBuildArg, []string{"-build-arg"}, "Set build-time variables")

ulimits := make(map[string]*ulimit.Ulimit)
flUlimits := opts.NewUlimitOpt(&ulimits)
@@ -257,6 +260,14 @@ func (cli *DockerCli) CmdBuild(args ...string) error {
}
v.Set("ulimits", string(ulimitsJSON))

// collect all the build-time environment variables for the container
buildArgs := runconfig.ConvertKVStringsToMap(flBuildArg.GetAll())
buildArgsJSON, err := json.Marshal(buildArgs)
if err != nil {
return err
}
v.Set("buildargs", string(buildArgsJSON))

headers := http.Header(make(map[string][]string))
buf, err := json.Marshal(cli.configFile.AuthConfigs)
if err != nil {
@@ -323,6 +323,15 @@ func (s *Server) postBuild(ctx context.Context, w http.ResponseWriter, r *http.R
buildConfig.Ulimits = buildUlimits
}

var buildArgs = map[string]string{}
buildArgsJSON := r.FormValue("buildargs")
if buildArgsJSON != "" {
if err := json.NewDecoder(strings.NewReader(buildArgsJSON)).Decode(&buildArgs); err != nil {
return err
}
}
buildConfig.BuildArgs = buildArgs

// Job cancellation. Note: not all job types support this.
if closeNotifier, ok := w.(http.CloseNotifier); ok {
finished := make(chan struct{})
@@ -18,6 +18,7 @@ const (
Volume = "volume"
User = "user"
StopSignal = "stopsignal"
Arg = "arg"
)

// Commands is list of all Dockerfile commands
@@ -37,4 +38,5 @@ var Commands = map[string]struct{}{
Volume: {},
User: {},
StopSignal: {},
Arg: {},
}
@@ -327,15 +327,59 @@ func run(b *builder, args []string, attributes map[string]bool, original string)
return err
}

// stash the cmd
cmd := b.Config.Cmd
// set Cmd manually, this is special case only for Dockerfiles
b.Config.Cmd = config.Cmd
runconfig.Merge(b.Config, config)
// stash the config environment
env := b.Config.Env

defer func(cmd *stringutils.StrSlice) { b.Config.Cmd = cmd }(cmd)
defer func(env []string) { b.Config.Env = env }(env)

// derive the net build-time environment for this run. We let config
// environment override the build time environment.
// This means that we take the b.buildArgs list of env vars and remove
// any of those variables that are defined as part of the container. In other
// words, anything in b.Config.Env. What's left is the list of build-time env
// vars that we need to add to each RUN command - note the list could be empty.
//
// We don't persist the build time environment with container's config
// environment, but just sort and prepend it to the command string at time
// of commit.
// This helps with tracing back the image's actual environment at the time
// of RUN, without leaking it to the final image. It also aids cache
// lookup for same image built with same build time environment.
cmdBuildEnv := []string{}
configEnv := runconfig.ConvertKVStringsToMap(b.Config.Env)
for key, val := range b.buildArgs {
if !b.isBuildArgAllowed(key) {
// skip build-args that are not in allowed list, meaning they have
// not been defined by an "ARG" Dockerfile command yet.
// This is an error condition but only if there is no "ARG" in the entire
// Dockerfile, so we'll generate any necessary errors after we parsed
// the entire file (see 'leftoverArgs' processing in evaluator.go )
continue

This comment has been minimized.

@duglin

duglin Aug 7, 2015
Contributor

I'm thinking it might be good to generate an error here. As an end user if I specified --build-arg newUser=john and that wasn't in the list of allowed args I'd like to know I was specifying invalid/useless info. What do you think?

This comment has been minimized.

@duglin

duglin Aug 7, 2015
Contributor

Never mind - I forgot that we may have an ARG later on that defines it, which means at this point in time its ok we have extra cmd line build args specified. Right?

This comment has been minimized.

@mapuri

mapuri Aug 7, 2015
Author Contributor

that is correct!

}
if _, ok := configEnv[key]; !ok {
cmdBuildEnv = append(cmdBuildEnv, fmt.Sprintf("%s=%s", key, val))
}
}

logrus.Debugf("[BUILDER] Command to be executed: %v", b.Config.Cmd)
// derive the command to use for probeCache() and to commit in this container.
// Note that we only do this if there are any build-time env vars. Also, we
// use the special argument "|#" at the start of the args array. This will
// avoid conflicts with any RUN command since commands can not
// start with | (vertical bar). The "#" (number of build envs) is there to
// help ensure proper cache matches. We don't want a RUN command
// that starts with "foo=abc" to be considered part of a build-time env var.
saveCmd := config.Cmd
if len(cmdBuildEnv) > 0 {
sort.Strings(cmdBuildEnv)
tmpEnv := append([]string{fmt.Sprintf("|%d", len(cmdBuildEnv))}, cmdBuildEnv...)
saveCmd = stringutils.NewStrSlice(append(tmpEnv, saveCmd.Slice()...)...)
}

b.Config.Cmd = saveCmd
hit, err := b.probeCache()
if err != nil {
return err
@@ -344,6 +388,13 @@ func run(b *builder, args []string, attributes map[string]bool, original string)
return nil
}

// set Cmd manually, this is special case only for Dockerfiles
b.Config.Cmd = config.Cmd
// set build-time environment for 'run'.
b.Config.Env = append(b.Config.Env, cmdBuildEnv...)

logrus.Debugf("[BUILDER] Command to be executed: %v", b.Config.Cmd)

This comment has been minimized.

@jessfraz

jessfraz Sep 8, 2015
Contributor

this might get noisy no?

This comment has been minimized.

@tiborvass

tiborvass Sep 8, 2015
Collaborator

it was already there, was just moved.

This comment has been minimized.

@jessfraz

jessfraz Sep 8, 2015
Contributor

ah ok cool beans


c, err := b.create()
if err != nil {
return err
@@ -358,6 +409,12 @@ func run(b *builder, args []string, attributes map[string]bool, original string)
if err != nil {
return err
}

// revert to original config environment and set the command string to
// have the build-time env vars in it (if any) so that future cache look-ups
// properly match it.
b.Config.Env = env
b.Config.Cmd = saveCmd
if err := b.commit(c.ID, cmd, "run"); err != nil {
return err
}
@@ -557,3 +614,47 @@ func stopSignal(b *builder, args []string, attributes map[string]bool, original
b.Config.StopSignal = sig
return b.commit("", b.Config.Cmd, fmt.Sprintf("STOPSIGNAL %v", args))
}

// ARG name[=value]
//
// Adds the variable foo to the trusted list of variables that can be passed
// to builder using the --build-arg flag for expansion/subsitution or passing to 'run'.
// Dockerfile author may optionally set a default value of this variable.
func arg(b *builder, args []string, attributes map[string]bool, original string) error {
if len(args) != 1 {
return fmt.Errorf("ARG requires exactly one argument definition")
}

var (
name string
value string
hasDefault bool
)

arg := args[0]
// 'arg' can just be a name or name-value pair. Note that this is different
// from 'env' that handles the split of name and value at the parser level.
// The reason for doing it differently for 'arg' is that we support just
// defining an arg and not assign it a value (while 'env' always expects a
// name-value pair). If possible, it will be good to harmonize the two.
if strings.Contains(arg, "=") {

This comment has been minimized.

@tiborvass

tiborvass Sep 11, 2015
Collaborator

I would add a comment above the function definition, to explain that env handles the = at the parser level, while arg handles it in the evaluator. It's fine for now but would be great to harmonize it in the future.

This comment has been minimized.

@mapuri

mapuri Sep 11, 2015
Author Contributor

sure, will do.

parts := strings.SplitN(arg, "=", 2)
name = parts[0]
value = parts[1]
hasDefault = true
} else {
name = arg
hasDefault = false
}
// add the arg to allowed list of build-time args from this step on.
b.allowedBuildArgs[name] = true

// If there is a default value associated with this arg then add it to the
// b.buildArgs if one is not already passed to the builder. The args passed
// to builder override the defaut value of 'arg'.
if _, ok := b.buildArgs[name]; !ok && hasDefault {
b.buildArgs[name] = value
}

return b.commit("", b.Config.Cmd, fmt.Sprintf("ARG %s", arg))
}
@@ -54,6 +54,7 @@ var replaceEnvAllowed = map[string]struct{}{
command.Volume: {},
command.User: {},
command.StopSignal: {},
command.Arg: {},
}

var evaluateTable map[string]func(*builder, []string, map[string]bool, string) error
@@ -75,6 +76,7 @@ func init() {
command.Volume: volume,
command.User: user,
command.StopSignal: stopSignal,
command.Arg: arg,
}
}

@@ -111,6 +113,9 @@ type builder struct {

Config *runconfig.Config // runconfig for cmd, run, entrypoint etc.

buildArgs map[string]string // build-time args received in build context for expansion/substitution and commands in 'run'.
allowedBuildArgs map[string]bool // list of build-time args that are allowed for expansion/substitution and passing to commands in 'run'.

// both of these are controlled by the Remove and ForceRemove options in BuildOpts
TmpContainers map[string]struct{} // a map of containers used for removes

@@ -194,6 +199,18 @@ func (b *builder) Run(context io.Reader) (string, error) {
}
}

// check if there are any leftover build-args that were passed but not
// consumed during build. Return an error, if there are any.
leftoverArgs := []string{}

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 6, 2015
Collaborator

We should pre-parse the Dockerfile rather than waiting until the end... but I suppose this is an optimization.

This comment has been minimized.

@mapuri

mapuri Aug 6, 2015
Author Contributor

Yes we can do it that way.

I just latched this check at end as by then we have the parsed state of b.Config.AllowedBuildArgs ready and I was able to depend on ARG evaluator and dispatcher function to throw out any errors etc.

I think if I have to do it before real dispatch I can walk over b.dockerfile.Children and look at the parsed children for ARG nodes. Let me try it out in a separate commit in this PR and see how it comes out.

This comment has been minimized.

@mapuri

mapuri Aug 6, 2015
Author Contributor

Actually thinking more there is a gotcha with doing this check before dispatch in the case when b.Config.AllowedBuildArgs are going to be inherited from parent image. In that case just looking at pre-parsed dockerfile will error incorrectly. I think doing this check post build is correct.

for arg := range b.buildArgs {
if !b.isBuildArgAllowed(arg) {
leftoverArgs = append(leftoverArgs, arg)
}
}
if len(leftoverArgs) > 0 {
return "", fmt.Errorf("One or more build-args %v were not consumed, failing build.", leftoverArgs)
}

if b.image == "" {
return "", fmt.Errorf("No image was generated. Is your Dockerfile empty?")
}
@@ -268,6 +285,18 @@ func (b *builder) readDockerfile() error {
return nil
}

// determine if build arg is part of built-in args or user
// defined args in Dockerfile at any point in time.
func (b *builder) isBuildArgAllowed(arg string) bool {
if _, ok := BuiltinAllowedBuildArgs[arg]; ok {
return true
}
if _, ok := b.allowedBuildArgs[arg]; ok {
return true
}
return false
}

// This method is the entrypoint to all statement handling routines.
//
// Almost all nodes will have this structure:
@@ -330,13 +359,34 @@ func (b *builder) dispatch(stepN int, ast *parser.Node) error {
msgList := make([]string, n)

var i int
// Append the build-time args to config-environment.
// This allows builder config to override the variables, making the behavior similar to
// a shell script i.e. `ENV foo bar` overrides value of `foo` passed in build
// context. But `ENV foo $foo` will use the value from build context if one
// isn't already been defined by a previous ENV primitive.
// Note, we get this behavior because we know that ProcessWord() will
// stop on the first occurrence of a variable name and not notice
// a subsequent one. So, putting the buildArgs list after the Config.Env
// list, in 'envs', is safe.
envs := b.Config.Env
for key, val := range b.buildArgs {
if !b.isBuildArgAllowed(key) {
// skip build-args that are not in allowed list, meaning they have
// not been defined by an "ARG" Dockerfile command yet.
// This is an error condition but only if there is no "ARG" in the entire
// Dockerfile, so we'll generate any necessary errors after we parsed
// the entire file (see 'leftoverArgs' processing in evaluator.go )
continue
}
envs = append(envs, fmt.Sprintf("%s=%s", key, val))
}
for ast.Next != nil {
ast = ast.Next
var str string
str = ast.Value
if _, ok := replaceEnvAllowed[cmd]; ok {
var err error
str, err = ProcessWord(ast.Value, b.Config.Env)
str, err = ProcessWord(ast.Value, envs)
if err != nil {
return err
}
@@ -46,6 +46,18 @@ var validCommitCommands = map[string]bool{
"workdir": true,
}

// BuiltinAllowedBuildArgs is list of built-in allowed build args
var BuiltinAllowedBuildArgs = map[string]bool{
"HTTP_PROXY": true,

This comment has been minimized.

@duglin

duglin Aug 5, 2015
Contributor

if I'm reading the code correctly, the "true" isn't used any place so I'm wondering if we should replace it with a "description" value. Then 3rd party tools that query this map have our default description too.

This comment has been minimized.

@mapuri

mapuri Aug 5, 2015
Author Contributor

Yes, that is correct, 'true' is just a dummy value as I wanted a map for ease of search. I will make it a string with a default description.

But note that since now we don't save these args in image metadata, so they can't be queried using docker inspect. But a third party tool that vendors in builder package will get access to these. Hope that is fine.

"http_proxy": true,
"HTTPS_PROXY": true,
"https_proxy": true,
"FTP_PROXY": true,
"ftp_proxy": true,
"NO_PROXY": true,

This comment has been minimized.

@thaJeztah

thaJeztah Aug 29, 2015
Member

Not related to the last changes, but I keep forgetting to ask; is there a reason FTP_PROXY and NO_PROXY don't have a lowercase variant (unlike the other ones)?

This comment has been minimized.

@mapuri

mapuri Aug 30, 2015
Author Contributor

actually I didn't know. I added the lower case http_proxy and https_proxy as that's what I use in my proxy environment.

But a quick google search shows that the lower cases version of FTP_PROXY and NO_PROXY are valid as well. I will add them to the list.

"no_proxy": true,
}

// Config contains all configs for a build job
type Config struct {
DockerfileName string
@@ -66,6 +78,7 @@ type Config struct {
CgroupParent string
Ulimits []*ulimit.Ulimit
AuthConfigs map[string]cliconfig.AuthConfig
BuildArgs map[string]string

Stdout io.Writer
Context io.ReadCloser
@@ -191,26 +204,28 @@ func Build(d *daemon.Daemon, buildConfig *Config) error {
Writer: buildConfig.Stdout,
StreamFormatter: sf,
},
Verbose: !buildConfig.SuppressOutput,
UtilizeCache: !buildConfig.NoCache,
Remove: buildConfig.Remove,
ForceRemove: buildConfig.ForceRemove,
Pull: buildConfig.Pull,
OutOld: buildConfig.Stdout,
StreamFormatter: sf,
AuthConfigs: buildConfig.AuthConfigs,
dockerfileName: buildConfig.DockerfileName,
cpuShares: buildConfig.CPUShares,
cpuPeriod: buildConfig.CPUPeriod,
cpuQuota: buildConfig.CPUQuota,
cpuSetCpus: buildConfig.CPUSetCpus,
cpuSetMems: buildConfig.CPUSetMems,
cgroupParent: buildConfig.CgroupParent,
memory: buildConfig.Memory,
memorySwap: buildConfig.MemorySwap,
ulimits: buildConfig.Ulimits,
cancelled: buildConfig.WaitCancelled(),
id: stringid.GenerateRandomID(),
Verbose: !buildConfig.SuppressOutput,
UtilizeCache: !buildConfig.NoCache,
Remove: buildConfig.Remove,
ForceRemove: buildConfig.ForceRemove,
Pull: buildConfig.Pull,
OutOld: buildConfig.Stdout,
StreamFormatter: sf,
AuthConfigs: buildConfig.AuthConfigs,
dockerfileName: buildConfig.DockerfileName,
cpuShares: buildConfig.CPUShares,
cpuPeriod: buildConfig.CPUPeriod,
cpuQuota: buildConfig.CPUQuota,
cpuSetCpus: buildConfig.CPUSetCpus,
cpuSetMems: buildConfig.CPUSetMems,
cgroupParent: buildConfig.CgroupParent,
memory: buildConfig.Memory,
memorySwap: buildConfig.MemorySwap,
ulimits: buildConfig.Ulimits,
cancelled: buildConfig.WaitCancelled(),
id: stringid.GenerateRandomID(),
buildArgs: buildConfig.BuildArgs,
allowedBuildArgs: make(map[string]bool),
}

defer func() {
ProTip! Use n and p to navigate between commits in a pull request.