From d1cabdff991fbf4ad796ab6fe0ced96208ac7ea5 Mon Sep 17 00:00:00 2001 From: CrazyMax Date: Thu, 3 Nov 2022 22:30:27 +0100 Subject: [PATCH 1/2] build: set default context builder if not specified Signed-off-by: CrazyMax --- cmd/docker/aliases.go | 13 ++-- cmd/docker/builder.go | 51 +++++++++++--- cmd/docker/builder_test.go | 104 ++++++++++++++++++++++++++--- cmd/docker/builder_windows_test.go | 5 ++ cmd/docker/docker.go | 8 ++- 5 files changed, 154 insertions(+), 27 deletions(-) create mode 100644 cmd/docker/builder_windows_test.go diff --git a/cmd/docker/aliases.go b/cmd/docker/aliases.go index 17068d8ee59a..0a288843d0b9 100644 --- a/cmd/docker/aliases.go +++ b/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 } diff --git a/cmd/docker/builder.go b/cmd/docker/builder.go index 31c6076046c0..f416931c7f76 100644 --- a/cmd/docker/builder.go +++ b/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 +func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, []string, error) { var useLegacy, useBuilder 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 @@ -69,13 +74,13 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st // 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 +88,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 +99,25 @@ 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 in env vars. + if forwarded && !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 +143,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 +} diff --git a/cmd/docker/builder_test.go b/cmd/docker/builder_test.go index b4ade489126e..80c380fc030d 100644 --- a/cmd/docker/builder_test.go +++ b/cmd/docker/builder_test.go @@ -3,10 +3,10 @@ package main import ( "bytes" "os" - "runtime" "testing" "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/flags" "github.com/docker/cli/internal/test/output" "gotest.tools/v3/assert" "gotest.tools/v3/fs" @@ -14,13 +14,37 @@ import ( var pluginFilename = "docker-buildx" -func init() { - if runtime.GOOS == "windows" { - pluginFilename = pluginFilename + ".exe" - } +func TestBuildWithBuildx(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(0777)), + ) + defer dir.Remove() + + var b bytes.Buffer + + t.Setenv("DOCKER_CONTEXT", "default") + 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) + tcmd.SetArgs([]string{"build", "."}) + + cmd, args, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + + 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) + assert.DeepEqual(t, []string{"BUILDX_BUILDER=default"}, envs) } -func TestBuildWithBuilder(t *testing.T) { +func TestBuildWithBuildxAndBuilder(t *testing.T) { + t.Setenv("BUILDX_BUILDER", "mybuilder") + 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(0777)), @@ -28,8 +52,11 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD defer dir.Remove() var b bytes.Buffer + + t.Setenv("DOCKER_CONTEXT", "default") 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) @@ -38,9 +65,11 @@ 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) + assert.Check(t, len(envs) == 0) } func TestBuildkitDisabled(t *testing.T) { @@ -55,6 +84,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 +93,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 +114,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 +123,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 +147,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 +156,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)) + }) + } +} diff --git a/cmd/docker/builder_windows_test.go b/cmd/docker/builder_windows_test.go new file mode 100644 index 000000000000..ce3dcdb08299 --- /dev/null +++ b/cmd/docker/builder_windows_test.go @@ -0,0 +1,5 @@ +package main + +func init() { + pluginFilename = pluginFilename + ".exe" +} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index cfb09a3109d2..c175ef64cb31 100644 --- a/cmd/docker/docker.go +++ b/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 } From 997846918e461102f90269018be0db93f4c3ecf0 Mon Sep 17 00:00:00 2001 From: CrazyMax Date: Thu, 3 Nov 2022 23:35:45 +0100 Subject: [PATCH 2/2] build: keep "buildx install" behavior Signed-off-by: CrazyMax --- cmd/docker/builder.go | 12 ++-- cmd/docker/builder_test.go | 133 +++++++++++++++++++++++-------------- 2 files changed, 91 insertions(+), 54 deletions(-) diff --git a/cmd/docker/builder.go b/cmd/docker/builder.go index f416931c7f76..7d463e4a767d 100644 --- a/cmd/docker/builder.go +++ b/cmd/docker/builder.go @@ -45,7 +45,7 @@ func newBuilderError(warn bool, err error) error { //nolint:gocyclo func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []string) ([]string, []string, []string, error) { - var useLegacy, useBuilder bool + var useLegacy, useBuilder, useAlias bool var envs []string // check DOCKER_BUILDKIT env var is present and @@ -68,6 +68,7 @@ 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 } @@ -110,10 +111,11 @@ func processBuilder(dockerCli command.Cli, cmd *cobra.Command, args, osargs []st // 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 in env vars. - if forwarded && !hasBuilderName(args, os.Environ()) { + // 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...) } diff --git a/cmd/docker/builder_test.go b/cmd/docker/builder_test.go index 80c380fc030d..9a8b40c62284 100644 --- a/cmd/docker/builder_test.go +++ b/cmd/docker/builder_test.go @@ -3,9 +3,11 @@ package main import ( "bytes" "os" + "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" @@ -14,36 +16,38 @@ import ( var pluginFilename = "docker-buildx" -func TestBuildWithBuildx(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(0777)), - ) - defer dir.Remove() - - var b bytes.Buffer - - t.Setenv("DOCKER_CONTEXT", "default") - 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) - tcmd.SetArgs([]string{"build", "."}) - - cmd, args, err := tcmd.HandleGlobalFlags() - assert.NilError(t, err) - - 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) - assert.DeepEqual(t, []string{"BUILDX_BUILDER=default"}, envs) -} - -func TestBuildWithBuildxAndBuilder(t *testing.T) { - t.Setenv("BUILDX_BUILDER", "mybuilder") +func TestBuildWithBuilder(t *testing.T) { + testcases := []struct { + 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, + }, + } dir := fs.NewDir(t, t.Name(), fs.WithFile(pluginFilename, `#!/bin/sh @@ -51,25 +55,56 @@ echo '{"SchemaVersion":"0.1.0","Vendor":"Docker Inc.","Version":"v0.6.3","ShortD ) defer dir.Remove() - var b bytes.Buffer - - t.Setenv("DOCKER_CONTEXT", "default") - 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) - tcmd.SetArgs([]string{"build", "."}) - - cmd, args, err := tcmd.HandleGlobalFlags() - assert.NilError(t, err) - - 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) - assert.Check(t, len(envs) == 0) + 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", "."}) + + cmd, args, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + + 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) {