From 25f04876d16b5ad2f120ca04ed77c71c6373637f Mon Sep 17 00:00:00 2001 From: Anca Iordache Date: Fri, 20 Sep 2019 17:13:26 +0200 Subject: [PATCH 1/3] app-214 Load Client info in getter function Signed-off-by: Anca Iordache Possible approach for client info - split ClientInfo() into ClientInfo() and loadClientInfo() - split ConfigFile() into ConfigFile() and loadConfigFile() - ConfigFile() and ClientInfo() call their corresponding loadXX function if it has not yet been loaded; this allows them to be used before Initialize() was called. - Initialize() *always* (re-)loads the configuration; this makes sure that the correct configuration is used when actually calling commands. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 22a5dad847f53dd5d1bde9a61027d13c0cbce94d) Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 56 +++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index 186854f866..c2f601d866 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -25,6 +25,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 +77,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 +88,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,9 +127,16 @@ 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) +} + // ServerInfo returns the server version details for the host this client is // connected to func (cli *DockerCli) ServerInfo() ServerInfo { @@ -137,7 +145,34 @@ func (cli *DockerCli) ServerInfo() 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 +242,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,18 +274,9 @@ 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) + err = cli.loadClientInfo() if err != nil { - return errors.Wrap(err, "Experimental field") - } - cli.clientInfo = ClientInfo{ - DefaultVersion: cli.client.ClientVersion(), - HasExperimental: hasExperimental, + return err } cli.initializeFromClient() return nil From c44c18e088d669612ff724e3c3212eee00a18d0d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 7 Apr 2020 23:24:20 +0200 Subject: [PATCH 2/3] docker build: check experimental --platform on pre-run Signed-off-by: Sebastiaan van Stijn (cherry picked from commit a88a1bea2360506a794d71f2be2a7477f8b4ebbb) Signed-off-by: Sebastiaan van Stijn --- cli/command/image/build.go | 29 +++++++++++++++++++++-------- cli/command/image/build_test.go | 9 ++++++++- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/cli/command/image/build.go b/cli/command/image/build.go index eeb8b3eb94..528137cb80 100644 --- a/cli/command/image/build.go +++ b/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) diff --git a/cli/command/image/build_test.go b/cli/command/image/build_test.go index fced1bfca7..3c8c875eb9 100644 --- a/cli/command/image/build_test.go +++ b/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 From cf663b526a34f3e7911e6e60138138c2023aa844 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 8 Apr 2020 00:57:41 +0200 Subject: [PATCH 3/3] cli: perform feature detection lazily - Perform feature detection when actually needed, instead of during initializing - Version negotiation is performed either when making an API request, or when (e.g.) running `docker help` (to hide unsupported features) - Use a 2 second timeout when 'pinging' the daemon; this should be sufficient for most cases, and when feature detection failed, the daemon will still perform validation (and produce an error if needed) - context.WithTimeout doesn't currently work with ssh connections (connhelper), so we're only applying this timeout for tcp:// connections, otherwise keep the old behavior. Before this change: time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null' real 0m32.919s user 0m0.370s sys 0m0.227s time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null' real 0m32.072s user 0m0.029s sys 0m0.023s After this change: time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker help &> /dev/null' real 0m 2.28s user 0m 0.03s sys 0m 0.03s time sh -c 'DOCKER_HOST=tcp://42.42.42.41:4242 docker context ls &> /dev/null' real 0m 0.13s user 0m 0.02s sys 0m 0.02s Signed-off-by: Sebastiaan van Stijn (cherry picked from commit b39739123b845f872549e91be184cc583f5b387c) Signed-off-by: Sebastiaan van Stijn --- cli/command/cli.go | 22 +++++-- cmd/docker/docker.go | 133 +++++++++++++++++++++---------------------- 2 files changed, 81 insertions(+), 74 deletions(-) diff --git a/cli/command/cli.go b/cli/command/cli.go index c2f601d866..962e71cd72 100644 --- a/cli/command/cli.go +++ b/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" @@ -137,9 +140,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 } @@ -274,11 +280,6 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...Initialize return err } } - err = cli.loadClientInfo() - if err != nil { - return err - } - cli.initializeFromClient() return nil } @@ -369,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} diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index dafaea0703..23839c1046 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -312,63 +312,65 @@ type versionDetails interface { ServerInfo() command.ServerInfo } -func hideFeatureFlag(f *pflag.Flag, hasFeature bool, annotation string) { - if hasFeature { +func hideFlagIf(f *pflag.Flag, condition func(string) bool, annotation string) { + if f.Hidden { return } - if _, ok := f.Annotations[annotation]; ok { - f.Hidden = true + if values, ok := f.Annotations[annotation]; ok && len(values) > 0 { + if condition(values[0]) { + f.Hidden = true + } } } -func hideFeatureSubCommand(subcmd *cobra.Command, hasFeature bool, annotation string) { - if hasFeature { +func hideSubcommandIf(subcmd *cobra.Command, condition func(string) bool, annotation string) { + if subcmd.Hidden { return } - if _, ok := subcmd.Annotations[annotation]; ok { - subcmd.Hidden = true + if v, ok := subcmd.Annotations[annotation]; ok { + if condition(v) { + subcmd.Hidden = true + } } } func hideUnsupportedFeatures(cmd *cobra.Command, details versionDetails) error { - clientVersion := details.Client().ClientVersion() - osType := details.ServerInfo().OSType - hasExperimental := details.ServerInfo().HasExperimental - hasExperimentalCLI := details.ClientInfo().HasExperimental - hasBuildKit, err := command.BuildKitEnabled(details.ServerInfo()) - if err != nil { - return err - } + var ( + buildKitDisabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return !v } + buildKitEnabled = func(_ string) bool { v, _ := command.BuildKitEnabled(details.ServerInfo()); return v } + notExperimental = func(_ string) bool { return !details.ServerInfo().HasExperimental } + notExperimentalCLI = func(_ string) bool { return !details.ClientInfo().HasExperimental } + notOSType = func(v string) bool { return v != details.ServerInfo().OSType } + 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 { - f.Hidden = !findCommand(cmd, commands) + if cmds, ok := f.Annotations["top-level"]; ok { + 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") - hideFeatureSubCommand(subcmd, hasExperimentalCLI, "experimentalCLI") - hideFeatureSubCommand(subcmd, hasBuildKit, "buildkit") - hideFeatureSubCommand(subcmd, !hasBuildKit, "no-buildkit") - // hide subcommands not supported by the server - if subcmdVersion, ok := subcmd.Annotations["version"]; ok && versions.LessThan(clientVersion, subcmdVersion) { - subcmd.Hidden = true - } - if v, ok := subcmd.Annotations["ostype"]; ok && v != osType { - subcmd.Hidden = true - } + hideSubcommandIf(subcmd, buildKitDisabled, "buildkit") + hideSubcommandIf(subcmd, buildKitEnabled, "no-buildkit") + hideSubcommandIf(subcmd, notExperimental, "experimental") + hideSubcommandIf(subcmd, notExperimentalCLI, "experimentalCLI") + hideSubcommandIf(subcmd, notOSType, "ostype") + hideSubcommandIf(subcmd, versionOlderThan, "version") } 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 !isVersionSupported(f, clientVersion) { - 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 !f.Changed { + return } + 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) { - return fmt.Errorf("%s requires API version %s, but the Docker daemon API version is %s", cmd.CommandPath(), cmdVersion, clientVersion) + 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, details.Client().ClientVersion()) } - if os, ok := curr.Annotations["ostype"]; ok && 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, 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, 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()) } }