Merge pull request #33114 from dnephin/fix-builder-warn-on-unused-builtint

Fix warning for unused build args

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

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"))
+}

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.

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)
 		}
 	}