diff --git a/builder/dockerfile/buildargs.go b/builder/dockerfile/buildargs.go index 738ee86eef..44687aaff4 100644 --- a/builder/dockerfile/buildargs.go +++ b/builder/dockerfile/buildargs.go @@ -3,6 +3,7 @@ package dockerfile import ( "fmt" "github.com/docker/docker/runconfig/opts" + "io" ) // builtinAllowedBuildArgs is list of built-in allowed build args @@ -40,16 +41,20 @@ func newBuildArgs(argsFromOptions map[string]*string) *buildArgs { } } -// UnreferencedOptionArgs returns the list of args that were set from options but -// were never referenced from the Dockerfile -func (b *buildArgs) UnreferencedOptionArgs() []string { +// WarnOnUnusedBuildArgs checks if there are any leftover build-args that were +// passed but not consumed during build. Print a warning, if there are any. +func (b *buildArgs) WarnOnUnusedBuildArgs(out io.Writer) { leftoverArgs := []string{} for arg := range b.argsFromOptions { - if _, ok := b.referencedArgs[arg]; !ok { + _, isReferenced := b.referencedArgs[arg] + _, isBuiltin := builtinAllowedBuildArgs[arg] + if !isBuiltin && !isReferenced { leftoverArgs = append(leftoverArgs, arg) } } - return leftoverArgs + if len(leftoverArgs) > 0 { + fmt.Fprintf(out, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs) + } } // ResetAllowed clears the list of args that are allowed to be used by a @@ -69,13 +74,13 @@ func (b *buildArgs) AddArg(key string, value *string) { b.referencedArgs[key] = struct{}{} } -// IsUnreferencedBuiltin checks if the key is a built-in arg, or if it has been -// referenced by the Dockerfile. Returns true if the arg is a builtin that has -// not been referenced in the Dockerfile. -func (b *buildArgs) IsUnreferencedBuiltin(key string) bool { +// IsReferencedOrNotBuiltin checks if the key is a built-in arg, or if it has been +// referenced by the Dockerfile. Returns true if the arg is not a builtin or +// if the builtin has been referenced in the Dockerfile. +func (b *buildArgs) IsReferencedOrNotBuiltin(key string) bool { _, isBuiltin := builtinAllowedBuildArgs[key] _, isAllowed := b.allowedBuildArgs[key] - return isBuiltin && !isAllowed + return isAllowed || !isBuiltin } // GetAllAllowed returns a mapping with all the allowed args diff --git a/builder/dockerfile/buildargs_test.go b/builder/dockerfile/buildargs_test.go index 625a02b154..0288770665 100644 --- a/builder/dockerfile/buildargs_test.go +++ b/builder/dockerfile/buildargs_test.go @@ -3,6 +3,7 @@ package dockerfile import ( "testing" + "bytes" "github.com/stretchr/testify/assert" ) @@ -62,3 +63,38 @@ func TestGetAllMeta(t *testing.T) { } assert.Equal(t, expected, all) } + +func TestWarnOnUnusedBuildArgs(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ThisArgIsUsed": strPtr("fromopt1"), + "ThisArgIsNotUsed": strPtr("fromopt2"), + "HTTPS_PROXY": strPtr("referenced builtin"), + "HTTP_PROXY": strPtr("unreferenced builtin"), + }) + buildArgs.AddArg("ThisArgIsUsed", nil) + buildArgs.AddArg("HTTPS_PROXY", nil) + + buffer := new(bytes.Buffer) + buildArgs.WarnOnUnusedBuildArgs(buffer) + out := buffer.String() + assert.NotContains(t, out, "ThisArgIsUsed") + assert.NotContains(t, out, "HTTPS_PROXY") + assert.NotContains(t, out, "HTTP_PROXY") + assert.Contains(t, out, "ThisArgIsNotUsed") +} + +func TestIsUnreferencedBuiltin(t *testing.T) { + buildArgs := newBuildArgs(map[string]*string{ + "ThisArgIsUsed": strPtr("fromopt1"), + "ThisArgIsNotUsed": strPtr("fromopt2"), + "HTTPS_PROXY": strPtr("referenced builtin"), + "HTTP_PROXY": strPtr("unreferenced builtin"), + }) + buildArgs.AddArg("ThisArgIsUsed", nil) + buildArgs.AddArg("HTTPS_PROXY", nil) + + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsUsed")) + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("ThisArgIsNotUsed")) + assert.True(t, buildArgs.IsReferencedOrNotBuiltin("HTTPS_PROXY")) + assert.False(t, buildArgs.IsReferencedOrNotBuiltin("HTTP_PROXY")) +} diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index aec68571c8..ac346d5cc8 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -159,7 +159,7 @@ func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*buil return nil, errors.Errorf("failed to reach build target %s in Dockerfile", b.options.Target) } - b.warnOnUnusedBuildArgs() + b.buildArgs.WarnOnUnusedBuildArgs(b.Stderr) if dispatchState.imageID == "" { buildsFailed.WithValues(metricsDockerfileEmptyError).Inc() @@ -240,15 +240,6 @@ func addNodesForLabelOption(dockerfile *parser.Node, labels map[string]string) { dockerfile.Children = append(dockerfile.Children, node) } -// check if there are any leftover build-args that were passed but not -// consumed during build. Print a warning, if there are any. -func (b *Builder) warnOnUnusedBuildArgs() { - leftoverArgs := b.buildArgs.UnreferencedOptionArgs() - if len(leftoverArgs) > 0 { - fmt.Fprintf(b.Stderr, "[Warning] One or more build-args %v were not consumed\n", leftoverArgs) - } -} - // BuildFromConfig builds directly from `changes`, treating it as if it were the contents of a Dockerfile // It will: // - Call parse.Parse() to get an AST root for the concatenated Dockerfile entries. diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 1f46b1e0d0..af9f67f3c9 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -418,7 +418,7 @@ func prependEnvOnCmd(buildArgs *buildArgs, buildArgVars []string, cmd strslice.S var tmpBuildEnv []string for _, env := range buildArgVars { key := strings.SplitN(env, "=", 2)[0] - if !buildArgs.IsUnreferencedBuiltin(key) { + if buildArgs.IsReferencedOrNotBuiltin(key) { tmpBuildEnv = append(tmpBuildEnv, env) } }