-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should pre-parse the Dockerfile rather than waiting until the end... but I suppose this is an optimization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we can do it that way. I just latched this check at end as by then we have the parsed state of I think if I have to do it before real dispatch I can walk over There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually thinking more there is a gotcha with doing this check before dispatch in the case when |
||
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 | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
"http_proxy": true, | ||
"HTTPS_PROXY": true, | ||
"https_proxy": true, | ||
"FTP_PROXY": true, | ||
"ftp_proxy": true, | ||
"NO_PROXY": true, | ||
"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() { | ||
|
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 might get noisy no?
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.
it was already there, was just moved.
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.
ah ok cool beans