Merge pull request #3676 from crazy-max/build-default-builder

build: set default context builder if not specified

cmd/docker/aliases.go

@@ -18,26 +18,27 @@ var allowedAliases = map[string]struct{}{
 	keyBuilderAlias: {},
 }
 
-func processAliases(dockerCli command.Cli, cmd *cobra.Command, args, osArgs []string) ([]string, []string, error) {
+func processAliases(dockerCli command.Cli, cmd *cobra.Command, args, osArgs []string) ([]string, []string, []string, error) {
 	var err error
+	var envs []string
 	aliasMap := dockerCli.ConfigFile().Aliases
 	aliases := make([][2][]string, 0, len(aliasMap))
 
 	for k, v := range aliasMap {
 		if _, ok := allowedAliases[k]; !ok {
-			return args, osArgs, errors.Errorf("not allowed to alias %q (allowed: %#v)", k, allowedAliases)
+			return args, osArgs, envs, errors.Errorf("not allowed to alias %q (allowed: %#v)", k, allowedAliases)
 		}
 		if c, _, err := cmd.Find(strings.Split(v, " ")); err == nil {
 			if !pluginmanager.IsPluginCommand(c) {
-				return args, osArgs, errors.Errorf("not allowed to alias with builtin %q as target", v)
+				return args, osArgs, envs, errors.Errorf("not allowed to alias with builtin %q as target", v)
 			}
 		}
 		aliases = append(aliases, [2][]string{{k}, {v}})
 	}
 
-	args, osArgs, err = processBuilder(dockerCli, cmd, args, os.Args)
+	args, osArgs, envs, err = processBuilder(dockerCli, cmd, args, os.Args)
 	if err != nil {
-		return args, os.Args, err
+		return args, os.Args, envs, err
 	}
 
 	for _, al := range aliases {
@@ -49,5 +50,5 @@ func processAliases(dockerCli command.Cli, cmd *cobra.Command, args, osArgs []st
 		}
 	}
 
-	return args, osArgs, nil
+	return args, osArgs, envs, nil
 }

cmd/docker/builder.go

@@ -2,13 +2,16 @@ package main
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"strconv"
+	"strings"
 
 	pluginmanager "github.com/docker/cli/cli-plugins/manager"
 	"github.com/docker/cli/cli/command"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 )
 
 const (
@@ -40,15 +43,17 @@ func newBuilderError(warn bool, err error) error {
 	return fmt.Errorf("%s", errorMsg)
 }
 
-func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, error) {
+//nolint:gocyclo
-	var useLegacy, useBuilder bool
+func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, []string, error) {
+	var useLegacy, useBuilder, useAlias bool
+	var envs []string
 
 	// check DOCKER_BUILDKIT env var is present and
 	// if not assume we want to use the builder component
 	if v, ok := os.LookupEnv("DOCKER_BUILDKIT"); ok {
 		enabled, err := strconv.ParseBool(v)
 		if err != nil {
-			return args, osargs, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
+			return args, osargs, nil, errors.Wrap(err, "DOCKER_BUILDKIT environment variable expects boolean value")
 		}
 		if !enabled {
 			useLegacy = true
@@ -63,19 +68,20 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st
 	aliasMap := dockerCli.ConfigFile().Aliases
 	if v, ok := aliasMap[keyBuilderAlias]; ok {
 		useBuilder = true
+		useAlias = true
 		builderAlias = v
 	}
 
 	// is this a build that should be forwarded to the builder?
 	fwargs, fwosargs, forwarded := forwardBuilder(builderAlias, args, osargs)
 	if !forwarded {
-		return args, osargs, nil
+		return args, osargs, nil, nil
 	}
 
 	// wcow build command must use the legacy builder
 	// if not opt-in through a builder component
 	if !useBuilder && dockerCli.ServerInfo().OSType == "windows" {
-		return args, osargs, nil
+		return args, osargs, nil, nil
 	}
 
 	if useLegacy {
@@ -83,7 +89,7 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st
 		if dockerCli.ServerInfo().OSType != "windows" {
 			_, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, nil))
 		}
-		return args, osargs, nil
+		return args, osargs, nil, nil
 	}
 
 	// check plugin is available if cmd forwarded
@@ -94,14 +100,26 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st
 	if perr != nil {
 		// if builder enforced with DOCKER_BUILDKIT=1, cmd must fail if plugin missing or broken
 		if useBuilder {
-			return args, osargs, newBuilderError(false, perr)
+			return args, osargs, nil, newBuilderError(false, perr)
 		}
 		// otherwise, display warning and continue
 		_, _ = fmt.Fprintln(dockerCli.Err(), newBuilderError(true, perr))
-		return args, osargs, nil
+		return args, osargs, nil, nil
 	}
 
-	return fwargs, fwosargs, nil
+	// If build subcommand is forwarded, user would expect "docker build" to
+	// always create a local docker image (default context builder). This is
+	// for better backward compatibility in case where a user could switch to
+	// a docker container builder with "docker buildx --use foo" which does
+	// not --load by default. Also makes sure that an arbitrary builder name
+	// is not being set in the command line or in the environment before
+	// setting the default context and keep "buildx install" behavior if being
+	// set (builder alias).
+	if forwarded && !useAlias && !hasBuilderName(args, os.Environ()) {
+		envs = append([]string{"BUILDX_BUILDER=" + dockerCli.CurrentContext()}, envs...)
+	}
+
+	return fwargs, fwosargs, envs, nil
 }
 
 func forwardBuilder(alias string, args, osargs []string) ([]string, []string, bool) {
@@ -127,3 +145,22 @@ func forwardBuilder(alias string, args, osargs []string) ([]string, []string, bo
 	}
 	return args, osargs, false
 }
+
+// hasBuilderName checks if a builder name is defined in args or env vars
+func hasBuilderName(args []string, envs []string) bool {
+	var builder string
+	flagset := pflag.NewFlagSet("buildx", pflag.ContinueOnError)
+	flagset.Usage = func() {}
+	flagset.SetOutput(io.Discard)
+	flagset.StringVar(&builder, "builder", "", "")
+	_ = flagset.Parse(args)
+	if builder != "" {
+		return true
+	}
+	for _, e := range envs {
+		if strings.HasPrefix(e, "BUILDX_BUILDER=") && e != "BUILDX_BUILDER=" {
+			return true
+		}
+	}
+	return false
+}

cmd/docker/builder_test.go

@@ -3,10 +3,12 @@ package main
 import (
 	"bytes"
 	"os"
-	"runtime"
+	"path/filepath"
 	"testing"
 
 	"github.com/docker/cli/cli/command"
+	"github.com/docker/cli/cli/context/store"
+	"github.com/docker/cli/cli/flags"
 	"github.com/docker/cli/internal/test/output"
 	"gotest.tools/v3/assert"
 	"gotest.tools/v3/fs"
@@ -14,23 +16,77 @@ import (
 
 var pluginFilename = "docker-buildx"
 
-func init() {
+func TestBuildWithBuilder(t *testing.T) {
-	if runtime.GOOS == "windows" {
+	testcases := []struct {
-		pluginFilename = pluginFilename + ".exe"
+		name         string
-	}
+		context      string
+		builder      string
+		alias        bool
+		expectedEnvs []string
+	}{
+		{
+			name:         "default",
+			context:      "default",
+			alias:        false,
+			expectedEnvs: []string{"BUILDX_BUILDER=default"},
+		},
+		{
+			name:         "custom context",
+			context:      "foo",
+			alias:        false,
+			expectedEnvs: []string{"BUILDX_BUILDER=foo"},
+		},
+		{
+			name:         "custom builder name",
+			builder:      "mybuilder",
+			alias:        false,
+			expectedEnvs: nil,
+		},
+		{
+			name:         "buildx install",
+			alias:        true,
+			expectedEnvs: nil,
+		},
 	}
 
-func TestBuildWithBuilder(t *testing.T) {
 	dir := fs.NewDir(t, t.Name(),
 		fs.WithFile(pluginFilename, `#!/bin/sh
 echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortDescription":"Build with BuildKit"}'`, fs.WithMode(0o777)),
 	)
 	defer dir.Remove()
 
+	for _, tt := range testcases {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.builder != "" {
+				t.Setenv("BUILDX_BUILDER", tt.builder)
+			}
+
 			var b bytes.Buffer
 			dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b))
 			assert.NilError(t, err)
+			assert.NilError(t, dockerCli.Initialize(flags.NewClientOptions()))
+
+			if tt.context != "" {
+				if tt.context != command.DefaultContextName {
+					assert.NilError(t, dockerCli.ContextStore().CreateOrUpdate(store.Metadata{
+						Name: tt.context,
+						Endpoints: map[string]interface{}{
+							"docker": map[string]interface{}{
+								"host": "unix://" + filepath.Join(t.TempDir(), "docker.sock"),
+							},
+						},
+					}))
+				}
+				opts := flags.NewClientOptions()
+				opts.Common.Context = tt.context
+				assert.NilError(t, dockerCli.Initialize(opts))
+			}
+
 			dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
+			if tt.alias {
+				dockerCli.ConfigFile().Aliases = map[string]string{"builder": "buildx"}
+			}
 
 			tcmd := newDockerCommand(dockerCli)
 			tcmd.SetArgs([]string{"build", "."})
@@ -38,9 +94,17 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD
 			cmd, args, err := tcmd.HandleGlobalFlags()
 			assert.NilError(t, err)
 
-	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+			var envs []string
+			args, os.Args, envs, err = processBuilder(dockerCli, cmd, args, os.Args)
 			assert.NilError(t, err)
 			assert.DeepEqual(t, []string{builderDefaultPlugin, "build", "."}, args)
+			if tt.expectedEnvs != nil {
+				assert.DeepEqual(t, tt.expectedEnvs, envs)
+			} else {
+				assert.Check(t, len(envs) == 0)
+			}
+		})
+	}
 }
 
 func TestBuildkitDisabled(t *testing.T) {
@@ -55,6 +119,7 @@ func TestBuildkitDisabled(t *testing.T) {
 
 	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
 	assert.NilError(t, err)
+	assert.NilError(t, dockerCli.Initialize(flags.NewClientOptions()))
 	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
 
 	tcmd := newDockerCommand(dockerCli)
@@ -63,9 +128,11 @@ func TestBuildkitDisabled(t *testing.T) {
 	cmd, args, err := tcmd.HandleGlobalFlags()
 	assert.NilError(t, err)
 
-	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+	var envs []string
+	args, os.Args, envs, err = processBuilder(dockerCli, cmd, args, os.Args)
 	assert.NilError(t, err)
 	assert.DeepEqual(t, []string{"build", "."}, args)
+	assert.Check(t, len(envs) == 0)
 
 	output.Assert(t, b.String(), map[int]func(string) error{
 		0: output.Suffix("DEPRECATED: The legacy builder is deprecated and will be removed in a future release."),
@@ -82,6 +149,7 @@ func TestBuilderBroken(t *testing.T) {
 
 	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
 	assert.NilError(t, err)
+	assert.NilError(t, dockerCli.Initialize(flags.NewClientOptions()))
 	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
 
 	tcmd := newDockerCommand(dockerCli)
@@ -90,9 +158,11 @@ func TestBuilderBroken(t *testing.T) {
 	cmd, args, err := tcmd.HandleGlobalFlags()
 	assert.NilError(t, err)
 
-	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+	var envs []string
+	args, os.Args, envs, err = processBuilder(dockerCli, cmd, args, os.Args)
 	assert.NilError(t, err)
 	assert.DeepEqual(t, []string{"build", "."}, args)
+	assert.Check(t, len(envs) == 0)
 
 	output.Assert(t, b.String(), map[int]func(string) error{
 		0: output.Prefix("failed to fetch metadata:"),
@@ -112,6 +182,7 @@ func TestBuilderBrokenEnforced(t *testing.T) {
 
 	dockerCli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(b))
 	assert.NilError(t, err)
+	assert.NilError(t, dockerCli.Initialize(flags.NewClientOptions()))
 	dockerCli.ConfigFile().CLIPluginsExtraDirs = []string{dir.Path()}
 
 	tcmd := newDockerCommand(dockerCli)
@@ -120,11 +191,59 @@ func TestBuilderBrokenEnforced(t *testing.T) {
 	cmd, args, err := tcmd.HandleGlobalFlags()
 	assert.NilError(t, err)
 
-	args, os.Args, err = processBuilder(dockerCli, cmd, args, os.Args)
+	var envs []string
+	args, os.Args, envs, err = processBuilder(dockerCli, cmd, args, os.Args)
 	assert.DeepEqual(t, []string{"build", "."}, args)
+	assert.Check(t, len(envs) == 0)
 
 	output.Assert(t, err.Error(), map[int]func(string) error{
 		0: output.Prefix("failed to fetch metadata:"),
 		2: output.Suffix("ERROR: BuildKit is enabled but the buildx component is missing or broken."),
 	})
 }
+
+func TestHasBuilderName(t *testing.T) {
+	cases := []struct {
+		name     string
+		args     []string
+		envs     []string
+		expected bool
+	}{
+		{
+			name:     "no args",
+			args:     []string{"docker", "build", "."},
+			envs:     []string{"FOO=bar"},
+			expected: false,
+		},
+		{
+			name:     "env var",
+			args:     []string{"docker", "build", "."},
+			envs:     []string{"BUILDX_BUILDER=foo"},
+			expected: true,
+		},
+		{
+			name:     "empty env var",
+			args:     []string{"docker", "build", "."},
+			envs:     []string{"BUILDX_BUILDER="},
+			expected: false,
+		},
+		{
+			name:     "flag",
+			args:     []string{"docker", "build", "--builder", "foo", "."},
+			envs:     []string{"FOO=bar"},
+			expected: true,
+		},
+		{
+			name:     "both",
+			args:     []string{"docker", "build", "--builder", "foo", "."},
+			envs:     []string{"BUILDX_BUILDER=foo"},
+			expected: true,
+		},
+	}
+	for _, tt := range cases {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, hasBuilderName(tt.args, tt.envs))
+		})
+	}
+}

cmd/docker/builder_windows_test.go

@@ -0,0 +1,5 @@
+package main
+
+func init() {
+	pluginFilename = pluginFilename + ".exe"
+}

cmd/docker/docker.go

@@ -178,11 +178,12 @@ func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) {
 	})
 }
 
-func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string) error {
+func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error {
 	plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, subcommand, cmd)
 	if err != nil {
 		return err
 	}
+	plugincmd.Env = append(envs, plugincmd.Env...)
 
 	go func() {
 		// override SIGTERM handler so we let the plugin shut down first
@@ -217,7 +218,8 @@ func runDocker(dockerCli *command.DockerCli) error {
 		return err
 	}
 
-	args, os.Args, err = processAliases(dockerCli, cmd, args, os.Args)
+	var envs []string
+	args, os.Args, envs, err = processAliases(dockerCli, cmd, args, os.Args)
 	if err != nil {
 		return err
 	}
@@ -230,7 +232,7 @@ func runDocker(dockerCli *command.DockerCli) error {
 	if len(args) > 0 {
 		ccmd, _, err := cmd.Find(args)
 		if err != nil || pluginmanager.IsPluginCommand(ccmd) {
-			err := tryPluginRun(dockerCli, cmd, args[0])
+			err := tryPluginRun(dockerCli, cmd, args[0], envs)
 			if !pluginmanager.IsNotFound(err) {
 				return err
 			}