Introduce support for CLI plugin hooks

[laurazard]

- What I did

Introduce a "hooks" mechanism for CLI plugins, which enables CLI plugins adding useful, contextual messages/actions after a command has been executed.

This works by defining a special plugin command (such as the metadata command) which the CLI calls with some information about the just-executed command (currently, command and flag names). The plugin then prints a json-encoded to it's stdout with a template for the message it would like the CLI to print.

Using such a system, a plugin to help new docker users could, for example, declare intent to hook into docker build, and return a templated message instructing the user how to run their newly-built image.

A templating approach was chosen to allow plugin messages to include information from the original command execution (such as the tag used for the image in build), so that messages can provide more useful information (such as printing "Run your image with docker run my-image" after a docker build -t my-image .
Helpers are provided in the form of manager.TemplateReplaceFlagValue("tag") for this case, and use Go Templates under the hood.

The templating approach chosen also keeps sensitive data from leaving the CLI process.

This PR also introduces a "features" map into the CLI config.json, to allow for global enabling/disabling of this (and future) features.

Example Plugin

func main() {
	plugin.Run(func(dockerCli command.Cli) *cobra.Command {
		cmd := RootCommand(dockerCli)
		originalPreRun := cmd.PersistentPreRunE
		cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
			if err := plugin.PersistentPreRunE(cmd, args); err != nil {
				//nolint: wrapcheck
				return err
			}
			if originalPreRun != nil {
				return originalPreRun(cmd, args)
			}
			return nil
		}
		return cmd
	},
		manager.Metadata{
			SchemaVersion: "0.1.0",
			Vendor:        "Docker Inc.",
			Version:       "0.1",
			HookCommands:  []string{"build"},  // declare intent to hook into `build` subcommand invocations
		})
}

func RootCommand(dockerCli command.Cli) *cobra.Command {
	rootCmd := &cobra.Command{
		Short:            "Docker Hints",
		Use:              "hints",
		TraverseChildren: true,
	}

	rootCmd.AddCommand(hookCommand(dockerCli))
	return rootCmd
}

func hookCommand(dockerCli command.Cli) *cobra.Command {
	hookCmd := &cobra.Command{
		Use:   manager.HookSubcommandName,  // register hook command that the CLI will invoke
		Short: "runs the plugins hooks",
		RunE: utils.Adapt(func(ctx context.Context, args []string) error {
			runHooks(dockerCli.Out(), []byte(args[0]))
			return nil
		}),
		Args: cobra.ExactArgs(1),
	}

	return hookCmd
}

func runHooks(out io.Writer, input []byte) {
	var c manager.HookPluginData
	_ = json.Unmarshal(input, &c)

	hint := "Run this image with `docker run " + manager.TemplateReplaceFlagValue("tag") + "`" // template helper
	returnType := manager.HookMessage{
		Template: hint,
	}
	enc := json.NewEncoder(out)
	enc.SetEscapeHTML(false)
	enc.SetIndent("", "     ")
	_ = enc.Encode(returnType)
}

Which results in:

---

Performance Callouts

Edited: for current approach + benchmarks see #4376 (comment)

Security Callouts

The current approach works by letting plugins declare a list of strings which are the CLI commands (such as build, run, etc.) whose execution the plugin wants to be hooked into. The CLI then only invokes each plugin for the commands the plugin has declared support for.

Since a plugin might want to be invoked for more than one command, some information must be passed into the plugin when it is invoked so that it can decide what information it wants to add – which for right now, is codified into the HookPluginData type and contains only a string representing the command being executed. Richer output is made possible by employing a templating approach, wherein the plugin can return a template string with special values which the CLI then processes – e.g.

$ docker build -t my-image .

# a plugin returning
You can run the image you just built with `docker run {docker-cli-plugin-hook-flag-tag}`

# gets parsed into
You can run the image you just built with `docker run my-image`

This allows for plugins to provide useful/contextual messages, without passing any potentially sensitive information to any other binary.

Possible future developments to make this more secure are things such as always being explicit when a plugin is invoked, so that the user is aware, or prompting the user whether they are okay with a plugin hooking into a command execution on first execution.

---

- A picture of a cute animal (not mandatory but encouraged)

Release Notes:

Added plugin "hooks" which can be enabled (if the plugin supports it) via the config file.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 30.71429% with 97 lines in your changes missing coverage. Please review.

Project coverage is 61.59%. Comparing base (ba64608) to head (c5016c6).
Report is 632 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4376       +/-   ##
===========================================
+ Coverage        0   61.59%   +61.59%     
===========================================
  Files           0      292      +292     
  Lines           0    20402    +20402     
===========================================
+ Hits            0    12566    +12566     
- Misses          0     6941     +6941     
- Partials        0      895      +895     

[vvoland]

Looks like "list" layout is enforced here. Should we also make sure that each message is 1-line only?

[laurazard]

Unsure about this one. I think it would be fine to have a multiline list type of hints, e.g.:

Next steps:
    - Keep in mind that lorem ipsum dolor sit amet.
(also, pellentesque ac eleifend tellus, ac malesuada dolor)
    - Nullam luctus rutrum fringilla

What does everyone think?

[krissetto]

Maybe we could format each line of a message (after the first) so they at least start with the same indentaton? (a small terminal would wrap the lines, so in that case it'd look ugly anyway)

[laurazard]

Maybe we could format each line of a message (after the first) so they at least start with the same indentaton?

Next steps:
    - Keep in mind that lorem ipsum dolor sit amet.
      (also, pellentesque ac eleifend tellus, ac malesuada dolor)
    - Nullam luctus rutrum fringilla

Like this? Not sure if this is clearer or not for me.

[krissetto]

Exactly! I think it looks much better (but that might just be a strong bias on my side 😅)

[eunomie]

Today in docker scout we don't display them as list items, even if there's multiple items:

[laurazard]

I'll take a jab at it – I like the idea, but slightly concerned about totally breaking formatting with the indentation in smaller terminals.

[tonistiigi]

It's still about the same. It seems like the performance penalty does not depend on the number of plugins, as we thought (from the original PR text):

You can see in your output how user/system gradually grows with every added plugin, spinning the CPU more each time. In the last example, the resource usage is 14x of baseline, 9x before scout is added.

As these plugins all run in their own goroutine, the real time depends on cpu count, OS's exec implementation(including malware detection mechanisms), prewarmed cache, system load, and other factors.

Docker is run by millions of users, with various os/arch/perf configurations. Afaik Desktop does not come with a free 10+ core machine. If you compare absolute numbers then obviously users with low perf and low cpu count would see the impact more (and CLI is released to some really low-powered machines) than machines with a lot of resources to spare. To compare the absolute numbers, you would need a big test matrix of all the OS(maybe even kernels & infra providers), different ranges of cpu counts and perf generations, memory pressure, invocation frequency etc. But we don't need to compare the absolute numbers, just that previously we needed resources to run 1 binary and now we need resources to run 10 (and more with any future update or extra plugin the user has added). Unless you can defy some laws of physics, the latter will be slower for the users.

Adding scout:

Scout binary is more than 4x bigger than dev/debug/init/feedback etc small plugins. Minimally, the extra resources need to be spent to load these binaries into memory, but it is not impossible that there is some init() rule that can be optimized as well(and other bottlenecks for spawning binaries in parallel are likely OS implementation-specific). Compared to buildx, what is only slightly smaller, scout's metadata cmd takes 25ms vs 15ms. How much impact a bigger binary or init() function has when loading a big bundle of binaries may not be universal in different configurations. This does show another weakness of this approach though - should any other binary make a mistake or just be unnecessarily bloated, it slows down the whole docker experience for any command.

Currently, the performance penalty is as we expect,

That's another position where we differ. To me, any measurable performance regression from spinning the CPU needlessly should be avoided. Comparing this to other historic regressions I've either fixed or reported in the past, I think this is the biggest one yet (at least if the high multi-core machines are excluded).

[laurazard]

This (current) performance discussion is moot as we'll be making some implementation changes that allow us to not have to scan/execute all the plugins, but for discussion's sake:

You can see in your output how user/system gradually grows with every added plugin

Yes, obviously cpu time scales with # plugins. I'd expected that we were discussing real time before, esp. since we're talking about the Desktop plugin bundles that most people would be running on their main development computers, and not in CI/etc. Otherwise if we're discussing CI environments why are you testing with more than 2 plugins? We can discuss either case, but discussing the worst case scenario of "extremely restricted environment but for some reason with Docker Desktop + all the plugins bundled installed" doesn't seem very productive.

Docker is run by millions of users, with various os/arch/perf configurations. Afaik Desktop does not come with a free 10+ core machine. If you compare absolute numbers then obviously users with low perf and low cpu count would see the impact more (and CLI is released to some really low-powered machines) than machines with a lot of resources to spare.

Yes, it is. That still does not mean that we cannot incur a performance hit ever, especially when this is done as a step in the direction of overall improving performance.

This does show another weakness of this approach though - should any other binary make a mistake or just be unnecessarily bloated, it slows down the whole docker experience for any command.

Maybe I wasn't clear in my previous reply, but this sentence – I'll also be adding a very naive timeout to the plugin scanning logic, to make sure we cap that out and never let it go longer than 80 or 100ms – refers to the plugin execution time as well.

To me, any measurable performance regression from spinning the CPU needlessly should be avoided

I agree. Which is why we're implementing this – so we can remove the wrapper and the "needless cpu spinning" that it does.

---

Regardless, I'll amend the PR soon – I was discussing with @thaJeztah and we were thinking about instead storing whether a plugin supports hooks in the CLI's config file, which would negate the need to cycle over/execute all the plugins to check for support.

[laurazard]

Pushed a change reworking the plugin hook listing+scanning logic – instead of scanning the plugin folders and executing every candidate to check their metadata for hook support, we now expect plugins with hooks to be configured in the CLI's config.json. For example:

{
  "auths": {},
  "plugins": {
    "hints": {
      "hooks": "pull,build"
    }
  }
}

The new logic looks only for plugins configured this way, which significantly reduces execution time (and makes it so that performance does not get worse with the amount of plugins bundled).

Applying the above patch (executes plugins even if not attached to a tty):

Benchmark 1: ./build/docker version
  Time (mean ± σ):       9.3 ms ±   1.0 ms    [User: 3.7 ms, System: 3.1 ms]
  Range (min … max):     6.8 ms …  13.7 ms    211 runs

Benchmark 2: ./build/docker-without-hooks version
  Time (mean ± σ):       9.4 ms ±   2.0 ms    [User: 3.6 ms, System: 3.2 ms]
  Range (min … max):     6.8 ms …  40.6 ms    288 runs

Summary
  ./build/docker version ran
    1.01 ± 0.24 times faster than ./build/docker-without-hooks version

That is for docker version, which no plugins register hook from. Measuring w/ a plugin enabled – with the example plugin, and instrumenting RunPluginHooks to time it's execution:

./build/docker pull alpine
Using default tag: latest
latest: Pulling from library/alpine
Digest: sha256:c5b1261d6d3e43071626931fc004f70149baeba2c8ec672bd4f27761f8e1ad6b
Status: Image is up to date for alpine:latest
docker.io/library/alpine:latest

Next steps:
    - Run this image with `docker run alpine`
RunPluginHooks took 12.409638ms
...
RunPluginHooks took 8.642533ms
...
RunPluginHooks took 14.853688ms

from end to end, including calling the hook plugin, which is a lot more reasonable.

Still want to do a quick run through of some codebases to check no plugin is going to break if we do this.

[Benehiko]

LGTM