Merge pull request #2424 from thaJeztah/lazy_feature_detection

cli: perform feature detection lazily

cli/command/cli.go

@@ -8,6 +8,9 @@ import (
 	"path/filepath"
 	"runtime"
 	"strconv"
+	"strings"
+	"sync"
+	"time"
 
 	"github.com/docker/cli/cli/config"
 	cliconfig "github.com/docker/cli/cli/config"
@@ -133,9 +136,12 @@ func (cli *DockerCli) loadConfigFile() {
 	cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)
 }
 
+var fetchServerInfo sync.Once
+
 // ServerInfo returns the server version details for the host this client is
 // connected to
 func (cli *DockerCli) ServerInfo() ServerInfo {
+	fetchServerInfo.Do(cli.initializeFromClient)
 	return cli.serverInfo
 }
 
@@ -270,11 +276,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
 			return err
 		}
 	}
-	err = cli.loadClientInfo()
-	if err != nil {
-		return err
-	}
-	cli.initializeFromClient()
 	return nil
 }
 
@@ -365,7 +366,16 @@ func isEnabled(value string) (bool, error) {
 }
 
 func (cli *DockerCli) initializeFromClient() {
-	ping, err := cli.client.Ping(context.Background())
+	ctx := context.Background()
+	if strings.HasPrefix(cli.DockerEndpoint().Host, "tcp://") {
+		// @FIXME context.WithTimeout doesn't work with connhelper / ssh connections
+		// time="2020-04-10T10:16:26Z" level=warning msg="commandConn.CloseWrite: commandconn: failed to wait: signal: killed"
+		var cancel func()
+		ctx, cancel = context.WithTimeout(ctx, 2*time.Second)
+		defer cancel()
+	}
+
+	ping, err := cli.client.Ping(ctx)
 	if err != nil {
 		// Default to true if we fail to connect to daemon
 		cli.serverInfo = ServerInfo{HasExperimental: true}

cli/command/image/build.go

@@ -113,6 +113,25 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
 		},
 	}
 
+	// Wrap the global pre-run to handle non-BuildKit use of the --platform flag.
+	//
+	// We're doing it here so that we're only contacting the daemon when actually
+	// running the command, and not during initialization.
+	// TODO remove this hack once we no longer support the experimental use of --platform
+	rootFn := cmd.Root().PersistentPreRunE
+	cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
+		if ok, _ := command.BuildKitEnabled(dockerCli.ServerInfo()); !ok {
+			f := cmd.Flag("platform")
+			delete(f.Annotations, "buildkit")
+			f.Annotations["version"] = []string{"1.32"}
+			f.Annotations["experimental"] = nil
+		}
+		if rootFn != nil {
+			return rootFn(cmd, args)
+		}
+		return nil
+	}
+
 	flags := cmd.Flags()
 
 	flags.VarP(&options.tags, "tag", "t", "Name and optionally a tag in the 'name:tag' format")
@@ -161,14 +180,8 @@ func NewBuildCommand(dockerCli command.Cli) *cobra.Command {
 	command.AddTrustVerificationFlags(flags, &options.untrusted, dockerCli.ContentTrustEnabled())
 
 	flags.StringVar(&options.platform, "platform", os.Getenv("DOCKER_DEFAULT_PLATFORM"), "Set platform if server is multi-platform capable")
-	// Platform is not experimental when BuildKit is used
+	flags.SetAnnotation("platform", "version", []string{"1.38"})
-	buildkitEnabled, err := command.BuildKitEnabled(dockerCli.ServerInfo())
+	flags.SetAnnotation("platform", "buildkit", nil)
-	if err == nil && buildkitEnabled {
-		flags.SetAnnotation("platform", "version", []string{"1.38"})
-	} else {
-		flags.SetAnnotation("platform", "version", []string{"1.32"})
-		flags.SetAnnotation("platform", "experimental", nil)
-	}
 
 	flags.BoolVar(&options.squash, "squash", false, "Squash newly built layers into a single new layer")
 	flags.SetAnnotation("squash", "experimental", nil)

cli/command/image/build_test.go

@@ -20,11 +20,13 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/moby/buildkit/session/secrets/secretsprovider"
 	"gotest.tools/v3/assert"
+	"gotest.tools/v3/env"
 	"gotest.tools/v3/fs"
 	"gotest.tools/v3/skip"
 )
 
 func TestRunBuildDockerfileFromStdinWithCompress(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
 	buffer := new(bytes.Buffer)
 	fakeBuild := newFakeBuild()
 	fakeImageBuild := func(ctx context.Context, context io.Reader, options types.ImageBuildOptions) (types.ImageBuildResponse, error) {
@@ -60,6 +62,7 @@ func TestRunBuildDockerfileFromStdinWithCompress(t *testing.T) {
 }
 
 func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
 	skip.If(t, os.Getuid() != 0, "root is required to chown files")
 	fakeBuild := newFakeBuild()
 	cli := test.NewFakeCli(&fakeClient{imageBuildFunc: fakeBuild.build})
@@ -90,6 +93,7 @@ func TestRunBuildResetsUidAndGidInContext(t *testing.T) {
 }
 
 func TestRunBuildDockerfileOutsideContext(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
 	dir := fs.NewDir(t, t.Name(),
 		fs.WithFile("data", "data file"))
 	defer dir.Remove()
@@ -122,7 +126,8 @@ COPY data /data
 // TODO: test "context selection" logic directly when runBuild is refactored
 // to support testing (ex: docker/cli#294)
 func TestRunBuildFromGitHubSpecialCase(t *testing.T) {
-	cmd := NewBuildCommand(test.NewFakeCli(nil))
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
+	cmd := NewBuildCommand(test.NewFakeCli(&fakeClient{}))
 	// Clone a small repo that exists so git doesn't prompt for credentials
 	cmd.SetArgs([]string{"github.com/docker/for-win"})
 	cmd.SetOutput(ioutil.Discard)
@@ -135,6 +140,7 @@ func TestRunBuildFromGitHubSpecialCase(t *testing.T) {
 // starting with `github.com` takes precedence over the `github.com` special
 // case.
 func TestRunBuildFromLocalGitHubDir(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
 	tmpDir, err := ioutil.TempDir("", "docker-build-from-local-dir-")
 	assert.NilError(t, err)
 	defer os.RemoveAll(tmpDir)
@@ -154,6 +160,7 @@ func TestRunBuildFromLocalGitHubDir(t *testing.T) {
 }
 
 func TestRunBuildWithSymlinkedContext(t *testing.T) {
+	defer env.Patch(t, "DOCKER_BUILDKIT", "0")()
 	dockerfile := `
 FROM alpine:3.6
 RUN echo hello world

cmd/docker/docker.go

@@ -312,63 +312,65 @@ type versionDetails interface {
 	ServerInfo() command.ServerInfo
 }
 
-func hideFeatureFlag(f *pflag.Flag, hasFeature bool, annotation string) {
+func hideFlagIf(f *pflag.Flag, condition func(string) bool, annotation string) {
-	if hasFeature {
+	if f.Hidden {
 		return
 	}
-	if _, ok := f.Annotations[annotation]; ok {
+	if values, ok := f.Annotations[annotation]; ok && len(values) > 0 {
-		f.Hidden = true
+		if condition(values[0]) {
+			f.Hidden = true
+		}
 	}
 }
 
-func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation string) {
+func hideSubcommandIf(subcmd *cobra.Command, condition func(string) bool, annotation string) {
-	if hasFeature {
+	if subcmd.Hidden {
 		return
 	}
-	if _, ok := subcmd.Annotations[annotation]; ok {
+	if v, ok := subcmd.Annotations[annotation]; ok {
-		subcmd.Hidden = true
+		if condition(v) {
+			subcmd.Hidden = true
+		}
 	}
 }
 
 func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error {
-	clientVersion := details.Client().ClientVersion()
+	var (
-	osType := details.ServerInfo().OSType
+		buildKitDisabled   = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v }
-	hasExperimental := details.ServerInfo().HasExperimental
+		buildKitEnabled    = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v }
-	hasExperimentalCLI := details.ClientInfo().HasExperimental
+		notExperimental    = func(_ string) bool { return !details.ServerInfo().HasExperimental }
-	hasBuildKit, err := command.BuildKitEnabled(details.ServerInfo())
+		notExperimentalCLI = func(_ string) bool { return !details.ClientInfo().HasExperimental }
-	if err != nil {
+		notOSType          = func(v string) bool { return v != details.ServerInfo().OSType }
-		return err
+		versionOlderThan   = func(v string) bool { return versions.LessThan(details.Client().ClientVersion(), v) }
-	}
+	)
 
 	cmd.Flags().VisitAll(func(f *pflag.Flag) {
-		hideFeatureFlag(f, hasExperimental, "experimental")
-		hideFeatureFlag(f, hasExperimentalCLI, "experimentalCLI")
-		hideFeatureFlag(f, hasBuildKit, "buildkit")
-		hideFeatureFlag(f, !hasBuildKit, "no-buildkit")
 		// hide flags not supported by the server
-		if !isOSTypeSupported(f, osType) || !isVersionSupported(f, clientVersion) {
-			f.Hidden = true
-		}
 		// root command shows all top-level flags
 		if cmd.Parent() != nil {
-			if commands, ok := f.Annotations["top-level"]; ok {
+			if cmds, ok := f.Annotations["top-level"]; ok {
-				f.Hidden = !findCommand(cmd, commands)
+				f.Hidden = !findCommand(cmd, cmds)
+			}
+			if f.Hidden {
+				return
 			}
 		}
+
+		hideFlagIf(f, buildKitDisabled, "buildkit")
+		hideFlagIf(f, buildKitEnabled, "no-buildkit")
+		hideFlagIf(f, notExperimental, "experimental")
+		hideFlagIf(f, notExperimentalCLI, "experimentalCLI")
+		hideFlagIf(f, notOSType, "ostype")
+		hideFlagIf(f, versionOlderThan, "version")
 	})
 
 	for _, subcmd := range cmd.Commands() {
-		hideFeatureSubCommand(subcmd, hasExperimental, "experimental")
+		hideSubcommandIf(subcmd, buildKitDisabled, "buildkit")
-		hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI")
+		hideSubcommandIf(subcmd, buildKitEnabled, "no-buildkit")
-		hideFeatureSubCommand(subcmd, hasBuildKit, "buildkit")
+		hideSubcommandIf(subcmd, notExperimental, "experimental")
-		hideFeatureSubCommand(subcmd, !hasBuildKit, "no-buildkit")
+		hideSubcommandIf(subcmd, notExperimentalCLI, "experimentalCLI")
-		// hide subcommands not supported by the server
+		hideSubcommandIf(subcmd, notOSType, "ostype")
-		if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) {
+		hideSubcommandIf(subcmd, versionOlderThan, "version")
-			subcmd.Hidden = true
-		}
-		if v, ok := subcmd.Annotations["ostype"]; ok && v != osType {
-			subcmd.Hidden = true
-		}
 	}
 	return nil
 }
@@ -394,31 +396,31 @@ func isSupported(cmd *cobra.Command, details versionDetails) error {
 }
 
 func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
-	clientVersion := details.Client().ClientVersion()
-	osType := details.ServerInfo().OSType
-	hasExperimental := details.ServerInfo().HasExperimental
-	hasExperimentalCLI := details.ClientInfo().HasExperimental
-
 	errs := []string{}
 
 	cmd.Flags().VisitAll(func(f *pflag.Flag) {
-		if f.Changed {
+		if !f.Changed {
-			if !isVersionSupported(f, clientVersion) {
+			return
-				errs = append(errs, fmt.Sprintf("\"--%s\" requires API version %s, but the Docker daemon API version is %s", f.Name, getFlagAnnotation(f, "version"), clientVersion))
-				return
-			}
-			if !isOSTypeSupported(f, osType) {
-				errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", f.Name, getFlagAnnotation(f, "ostype"), osType))
-				return
-			}
-			if _, ok := f.Annotations["experimental"]; ok && !hasExperimental {
-				errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker daemon with experimental features enabled", f.Name))
-			}
-			if _, ok := f.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
-				errs = append(errs, fmt.Sprintf("\"--%s\" is only supported on a Docker cli with experimental cli features enabled", f.Name))
-			}
-			// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
 		}
+		if !isVersionSupported(f, details.Client().ClientVersion()) {
+			errs = append(errs, fmt.Sprintf(`"--%s" requires API version %s, but the Docker daemon API version is %s`, f.Name, getFlagAnnotation(f, "version"), details.Client().ClientVersion()))
+			return
+		}
+		if !isOSTypeSupported(f, details.ServerInfo().OSType) {
+			errs = append(errs, fmt.Sprintf(
+				`"--%s" is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s`,
+				f.Name,
+				getFlagAnnotation(f, "ostype"), details.ServerInfo().OSType),
+			)
+			return
+		}
+		if _, ok := f.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
+			errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker daemon with experimental features enabled`, f.Name))
+		}
+		if _, ok := f.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
+			errs = append(errs, fmt.Sprintf(`"--%s" is only supported on a Docker cli with experimental cli features enabled`, f.Name))
+		}
+		// buildkit-specific flags are noop when buildkit is not enabled, so we do not add an error in that case
 	})
 	if len(errs) > 0 {
 		return errors.New(strings.Join(errs, "\n"))
@@ -428,23 +430,18 @@ func areFlagsSupported(cmd *cobra.Command, details versionDetails) error {
 
 // Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
 func areSubcommandsSupported(cmd *cobra.Command, details versionDetails) error {
-	clientVersion := details.Client().ClientVersion()
-	osType := details.ServerInfo().OSType
-	hasExperimental := details.ServerInfo().HasExperimental
-	hasExperimentalCLI := details.ClientInfo().HasExperimental
-
 	// Check recursively so that, e.g., `docker stack ls` returns the same output as `docker stack`
 	for curr := cmd; curr != nil; curr = curr.Parent() {
-		if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(clientVersion, cmdVersion) {
+		if cmdVersion, ok := curr.Annotations["version"]; ok && versions.LessThan(details.Client().ClientVersion(), cmdVersion) {
-			return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, clientVersion)
+			return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, details.Client().ClientVersion())
 		}
-		if os, ok := curr.Annotations["ostype"]; ok && os != osType {
+		if os, ok := curr.Annotations["ostype"]; ok && os != details.ServerInfo().OSType {
-			return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), os, osType)
+			return fmt.Errorf("%s is only supported on a Docker daemon running on %s, but the Docker daemon is running on %s", cmd.CommandPath(), os, details.ServerInfo().OSType)
 		}
-		if _, ok := curr.Annotations["experimental"]; ok && !hasExperimental {
+		if _, ok := curr.Annotations["experimental"]; ok && !details.ServerInfo().HasExperimental {
 			return fmt.Errorf("%s is only supported on a Docker daemon with experimental features enabled", cmd.CommandPath())
 		}
-		if _, ok := curr.Annotations["experimentalCLI"]; ok && !hasExperimentalCLI {
+		if _, ok := curr.Annotations["experimentalCLI"]; ok && !details.ClientInfo().HasExperimental {
 			return fmt.Errorf("%s is only supported on a Docker cli with experimental cli features enabled", cmd.CommandPath())
 		}
 	}