Merge pull request #2457 from thaJeztah/19.03_backport_lazy_feature_detection

[19.03 backport] cli: perform feature detection lazily
Upstream-commit: 39299333681b5c4ad47935b2b3e3717e80ef5495
Component: cli

Author: silvin-lubecki

components/cli/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"
@@ -25,6 +28,7 @@ import (
 	"github.com/docker/cli/internal/containerizedengine"
 	dopts "github.com/docker/cli/opts"
 	clitypes "github.com/docker/cli/types"
+	"github.com/docker/docker/api"
 	"github.com/docker/docker/api/types"
 	registrytypes "github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/client"
@@ -76,7 +80,7 @@ type DockerCli struct {
 	err                   io.Writer
 	client                client.APIClient
 	serverInfo            ServerInfo
-	clientInfo            ClientInfo
+	clientInfo            *ClientInfo
 	contentTrust          bool
 	newContainerizeClient func(string) (clitypes.ContainerizedClient, error)
 	contextStore          store.Store
@@ -87,7 +91,7 @@ type DockerCli struct {
 
 // DefaultVersion returns api.defaultVersion or DOCKER_API_VERSION if specified.
 func (cli *DockerCli) DefaultVersion() string {
-	return cli.clientInfo.DefaultVersion
+	return cli.ClientInfo().DefaultVersion
 }
 
 // Client returns the APIClient
@@ -126,18 +130,55 @@ func ShowHelp(err io.Writer) func(*cobra.Command, []string) error {
 
 // ConfigFile returns the ConfigFile
 func (cli *DockerCli) ConfigFile() *configfile.ConfigFile {
+	if cli.configFile == nil {
+		cli.loadConfigFile()
+	}
 	return cli.configFile
 }
 
+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
 }
 
 // ClientInfo returns the client details for the cli
 func (cli *DockerCli) ClientInfo() ClientInfo {
-	return cli.clientInfo
+	if cli.clientInfo == nil {
+		_ = cli.loadClientInfo()
+	}
+	return *cli.clientInfo
+}
+
+func (cli *DockerCli) loadClientInfo() error {
+	var experimentalValue string
+	// Environment variable always overrides configuration
+	if experimentalValue = os.Getenv("DOCKER_CLI_EXPERIMENTAL"); experimentalValue == "" {
+		experimentalValue = cli.ConfigFile().Experimental
+	}
+	hasExperimental, err := isEnabled(experimentalValue)
+	if err != nil {
+		return errors.Wrap(err, "Experimental field")
+	}
+
+	var v string
+	if cli.client != nil {
+		v = cli.client.ClientVersion()
+	} else {
+		v = api.DefaultVersion
+	}
+	cli.clientInfo = &ClientInfo{
+		DefaultVersion:  v,
+		HasExperimental: hasExperimental,
+	}
+	return nil
 }
 
 // ContentTrustEnabled returns whether content trust has been enabled by an
@@ -207,7 +248,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
 		debug.Enable()
 	}
 
-	cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err)
+	cli.loadConfigFile()
 
 	baseContextStore := store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig)
 	cli.contextStore = &ContextStoreWithDefault{
@@ -239,20 +280,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize
 			return err
 		}
 	}
-	var experimentalValue string
-	// Environment variable always overrides configuration
-	if experimentalValue = os.Getenv("DOCKER_CLI_EXPERIMENTAL"); experimentalValue == "" {
-		experimentalValue = cli.configFile.Experimental
-	}
-	hasExperimental, err := isEnabled(experimentalValue)
-	if err != nil {
-		return errors.Wrap(err, "Experimental field")
-	}
-	cli.clientInfo = ClientInfo{
-		DefaultVersion:  cli.client.ClientVersion(),
-		HasExperimental: hasExperimental,
-	}
-	cli.initializeFromClient()
 	return nil
 }
 
@@ -343,7 +370,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}

components/cli/cli/command/image/build.go

@@ -115,6 +115,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")
@@ -163,14 +182,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
-	buildkitEnabled, err := command.BuildKitEnabled(dockerCli.ServerInfo())
-	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.SetAnnotation("platform", "version", []string{"1.38"})
+	flags.SetAnnotation("platform", "buildkit", nil)
 
 	flags.BoolVar(&options.squash, "squash", false, "Squash newly built layers into a single new layer")
 	flags.SetAnnotation("squash", "experimental", nil)

components/cli/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