Merge pull request #1417 from alebcay/fix-1408

Support build args without explicit value
containerd · Oct 19, 2022 · 06e529a · 06e529a
2 parents 5d80778 + a9dda52
commit 06e529a
Show file tree

Hide file tree

Showing 3 changed files with 92 additions and 13 deletions.
diff --git a/cmd/nerdctl/build.go b/cmd/nerdctl/build.go
@@ -382,20 +382,34 @@ func generateBuildctlArgs(cmd *cobra.Command, buildkitHost string, platform, arg
 		return "", nil, false, "", nil, cleanup, err
 	}
 	for _, ba := range strutil.DedupeStrSlice(buildArgsValue) {
-		buildctlArgs = append(buildctlArgs, "--opt=build-arg:"+ba)
-
-		// Support `--build-arg BUILDKIT_INLINE_CACHE=1` for compatibility with `docker buildx build`
-		// https://github.com/docker/buildx/blob/v0.6.3/docs/reference/buildx_build.md#-export-build-cache-to-an-external-cache-destination---cache-to
-		if strings.HasPrefix(ba, "BUILDKIT_INLINE_CACHE=") {
-			bic := strings.TrimPrefix(ba, "BUILDKIT_INLINE_CACHE=")
-			bicParsed, err := strconv.ParseBool(bic)
-			if err == nil {
-				if bicParsed {
-					buildctlArgs = append(buildctlArgs, "--export-cache=type=inline")
-				}
+		arr := strings.Split(ba, "=")
+		if len(arr) == 1 && len(arr[0]) > 0 {
+			// Avoid masking default build arg value from Dockerfile if environment variable is not set
+			// https://github.com/moby/moby/issues/24101
+			val, ok := os.LookupEnv(arr[0])
+			if ok {
+				buildctlArgs = append(buildctlArgs, fmt.Sprintf("--opt=build-arg:%s=%s", ba, val))
 			} else {
-				logrus.WithError(err).Warnf("invalid BUILDKIT_INLINE_CACHE: %q", bic)
+				logrus.Debugf("ignoring unset build arg %q", ba)
+			}
+		} else if len(arr) > 1 && len(arr[0]) > 0 {
+			buildctlArgs = append(buildctlArgs, "--opt=build-arg:"+ba)
+
+			// Support `--build-arg BUILDKIT_INLINE_CACHE=1` for compatibility with `docker buildx build`
+			// https://github.com/docker/buildx/blob/v0.6.3/docs/reference/buildx_build.md#-export-build-cache-to-an-external-cache-destination---cache-to
+			if strings.HasPrefix(ba, "BUILDKIT_INLINE_CACHE=") {
+				bic := strings.TrimPrefix(ba, "BUILDKIT_INLINE_CACHE=")
+				bicParsed, err := strconv.ParseBool(bic)
+				if err == nil {
+					if bicParsed {
+						buildctlArgs = append(buildctlArgs, "--export-cache=type=inline")
+					}
+				} else {
+					logrus.WithError(err).Warnf("invalid BUILDKIT_INLINE_CACHE: %q", bic)
+				}
 			}
+		} else {
+			return "", nil, false, "", nil, nil, fmt.Errorf("invalid build arg %q", ba)
 		}
 	}
 

diff --git a/cmd/nerdctl/build_test.go b/cmd/nerdctl/build_test.go
@@ -215,6 +215,58 @@ func createBuildContext(dockerfile string) (string, error) {
 	return tmpDir, nil
 }
 
+func TestBuildWithBuildArg(t *testing.T) {
+	testutil.RequiresBuild(t)
+	base := testutil.NewBase(t)
+	defer base.Cmd("builder", "prune").Run()
+	imageName := testutil.Identifier(t)
+	defer base.Cmd("rmi", imageName).Run()
+
+	dockerfile := fmt.Sprintf(`FROM %s
+ARG TEST_STRING=1
+ENV TEST_STRING=$TEST_STRING
+CMD echo $TEST_STRING
+	`, testutil.CommonImage)
+
+	buildCtx, err := createBuildContext(dockerfile)
+	assert.NilError(t, err)
+	defer os.RemoveAll(buildCtx)
+
+	base.Cmd("build", buildCtx, "-t", imageName).AssertOK()
+	base.Cmd("run", "--rm", imageName).AssertOutExactly("1\n")
+
+	validCases := []struct {
+		name     string
+		arg      string
+		envValue string
+		envSet   bool
+		expected string
+	}{
+		{"ArgValueOverridesDefault", "TEST_STRING=2", "", false, "2\n"},
+		{"EmptyArgValueOverridesDefault", "TEST_STRING=", "", false, "\n"},
+		{"UnsetArgKeyPreservesDefault", "TEST_STRING", "", false, "1\n"},
+		{"EnvValueOverridesDefault", "TEST_STRING", "3", true, "3\n"},
+		{"EmptyEnvValueOverridesDefault", "TEST_STRING", "", true, "\n"},
+	}
+
+	for _, tc := range validCases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.envSet {
+				err := os.Setenv("TEST_STRING", tc.envValue)
+				assert.NilError(t, err)
+				defer os.Unsetenv("TEST_STRING")
+			}
+
+			base.Cmd("build", buildCtx, "-t", imageName, "--build-arg", tc.arg).AssertOK()
+			base.Cmd("run", "--rm", imageName).AssertOutExactly(tc.expected)
+		})
+	}
+
+	t.Run("InvalidBuildArgCausesError", func(t *testing.T) {
+		base.Cmd("build", buildCtx, "-t", imageName, "--build-arg", "=TEST_STRING").AssertFail()
+	})
+}
+
 func TestBuildWithIIDFile(t *testing.T) {
 	t.Parallel()
 	testutil.RequiresBuild(t)

diff --git a/cmd/nerdctl/builder.go b/cmd/nerdctl/builder.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"strings"
 
 	"github.com/containerd/nerdctl/pkg/buildkitutil"
 	"github.com/containerd/nerdctl/pkg/defaults"
@@ -128,7 +129,19 @@ func builderDebugAction(cmd *cobra.Command, args []string) error {
 		return err
 	} else if len(buildArgsValue) > 0 {
 		for _, v := range buildArgsValue {
-			buildgArgs = append(buildgArgs, "--build-arg="+v)
+			arr := strings.Split(v, "=")
+			if len(arr) == 1 && len(arr[0]) > 0 {
+				// Avoid masking default build arg value from Dockerfile if environment variable is not set
+				// https://github.com/moby/moby/issues/24101
+				val, ok := os.LookupEnv(arr[0])
+				if ok {
+					buildgArgs = append(buildgArgs, fmt.Sprintf("--build-arg=%s=%s", v, val))
+				}
+			} else if len(arr) > 1 && len(arr[0]) > 0 {
+				buildgArgs = append(buildgArgs, "--build-arg="+v)
+			} else {
+				return fmt.Errorf("invalid build arg %q", v)
+			}
 		}
 	}