Allow plugins to have --flags which are the same as the top-level #1722

ijc · 2019-03-07T16:18:33Z

This was complicated. The final commit message explains it best:

    allow plugins to have argument which match a top-level flag.
    
    The issue with plugin options clashing with globals is that when cobra is
    parsing the command line and it comes across an argument which doesn't start
    with a `-` it (in the absence of plugins) distinguishes between "argument to
    current command" and "new subcommand" based on the list of registered sub
    commands.
    
    Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra
    parses up to the `plugin`, sees it isn't a registered sub-command of the
    top-level docker (because it isn't, it's a plugin) so it accumulates it as an
    argument to the top-level `docker` command. Then it sees the `-c`, and thinks
    it is the global `-c` (for AKA `--context`) option and tries to treat it as
    that, which fails.
    
    In the specific case of the top-level `docker` subcommand we know that it has
    no arguments which aren't `--flags` (or `-f` short flags) and so anything which
    doesn't start with a `-` must either be a (known) subcommand or an attempt to
    execute a plugin.
    
    We could simply scan for and register all installed plugins at start of day, so
    that cobra can do the right thing, but we want to avoid that since it would
    involve executing each plugin to fetch the metadata, even if the command wasn't
    going to end up hitting a plugin.
    
    Instead we can parse the initial set of global arguments separately before
    hitting the main cobra `Execute` path, which works here exactly because we know
    that the top-level has no non-flag arguments.
    
    One slight wrinkle is that the top-level `PersistentPreRunE` is no longer
    called on the plugins path (since it no longer goes via `Execute`), so we
    arrange for the initialisation done there (which has to be done after global
    flags are parsed to handle e.g. `--config`) to happen explictly after the
    global flags are parsed. Rather than make `newDockerCommand` return the
    complicated set of results needed to make this happen, instead return a closure
    which achieves this.
    
    The new functionality is introduced via a common `TopLevelCommand` abstraction
    which lets us adjust the plugin entrypoint to use the same strategy for parsing
    the global arguments. This isn't strictly required (in this case the stuff in
    cobra's `Execute` works fine) but doing it this way avoids the possibility of
    subtle differences in behaviour.
    
    Fixes #1699, and also, as a side-effect, the first item in #1661.
    
    Signed-off-by: Ian Campbell <[email protected]>

thaJeztah · 2019-03-07T16:42:43Z

One small linting error;

cmd/docker/docker.go:82:23:warning: dockerCli can be github.com/docker/cli/cli/command.Cli (interfacer)
cmd/docker/docker.go:122:18:warning: dockerCli can be github.com/docker/cli/cli/command.Cli (interfacer)
cmd/docker/docker.go:185:19:warning: dockerCli can be github.com/docker/cli/cli/command.Cli (interfacer)

thaJeztah

left one suggestion for the test, but LGTM, thanks!

e2e/cli-plugins/flags_test.go

thaJeztah · 2019-03-07T17:03:25Z

e2e/cli-plugins/flags_test.go

+
+// TestClashWithGlobalArgs ensures correct behaviour when a plugin
+// has an argument with the same name as one of the globals.
+func TestClashWithGlobalArgs(t *testing.T) {


(not a blocker for this PR); we should evaluate the naming of these tests, so make them easier to "grep" (which can be useful to run a subset of tests), i.e. give them a consistent prefix (e.g. TestPluginGlobalArgsClash, TestPluginGlobalArgsUnknown)

codecov-io · 2019-03-08T10:24:21Z

Codecov Report

Merging #1722 into master will decrease coverage by 0.08%.
The diff coverage is 18.33%.

@@            Coverage Diff             @@
##           master    #1722      +/-   ##
==========================================
- Coverage   56.13%   56.04%   -0.09%     
==========================================
  Files         306      306              
  Lines       21038    21050      +12     
==========================================
- Hits        11809    11798      -11     
- Misses       8374     8399      +25     
+ Partials      855      853       -2

ijc · 2019-03-08T10:33:17Z

and also, as a side-effect, the first item in #1661.

I'm going to add an e2e for this case here too once I'm done with the feedback so far

ijc · 2019-03-08T10:57:03Z

Test failure ends with

10:44:23 Step 2/6 : RUN apk --no-cache add shadow openssh-server &&   groupadd -f docker &&   useradd -m penguin &&   usermod -aG docker penguin &&   usermod -p $(head -c32 /dev/urandom | base64) penguin &&   chsh -s /bin/sh penguin &&   ssh-keygen -A
10:44:23  ---> Running in da24ce61c17f
10:44:23 fetch http://dl-cdn.alpinelinux.org/alpine/v3.9/main/x86_64/APKINDEX.tar.gz
10:44:23 fetch http://dl-cdn.alpinelinux.org/alpine/v3.9/community/x86_64/APKINDEX.tar.gz
10:44:23 (1/5) Installing openssh-keygen (7.9_p1-r4)
10:44:23 (2/5) Installing openssh-server-common (7.9_p1-r4)
10:44:23 (3/5) Installing openssh-server (7.9_p1-r4)
10:44:23 (4/5) Installing linux-pam (1.3.0-r0)
10:44:23 (5/5) Installing shadow (4.5-r0)
10:44:23 Executing busybox-1.29.3-r10.trigger
10:44:23 OK: 22 MiB in 44 packages
10:44:23 Creating mailbox file: No such file or directory
10:44:24 Password: chsh: PAM: Authentication token manipulation error
10:44:24 �[0mService 'engine' failed to build: The command '/bin/sh -c apk --no-cache add shadow openssh-server &&   groupadd -f docker &&   useradd -m penguin &&   usermod -aG docker penguin &&   usermod -p $(head -c32 /dev/urandom | base64) penguin &&   chsh -s /bin/sh penguin &&   ssh-keygen -A' returned a non-zero code: 1
10:44:24 docker.Makefile:152: recipe for target 'test-e2e-connhelper-ssh' failed
10:44:24 make: *** [test-e2e-connhelper-ssh] Error 1

Which I don't think can be due to my changes here... I'm about to push an update so if this was a flake it'll go away then I guess.

silvin-lubecki · 2019-03-11T16:50:02Z

cli/cobra.go

+	// flags.Args() and then continuing on and finding other
+	// arguments which we try and treat as globals (when they are
+	// actually arguments to the subcommand).
+	flags.SetInterspersed(false)


I think this is smelly, but the only other way I see is to clone the flags, and then set the interspersed, add the flag set with persistent flags... so HandleGlobalFlags has no side effects anymore.
That said, the effort may not be worth it...

Ah, no, that's a good idea actually. I shall investigate.

Hrm, looks like cloning the flags is not as trivial as I would have hoped :-/ at least there is no handy looking method I can see and I'm not sure if a simple copy of the pointed to object is sufficient (but I think probably not).

As far as I remember, there's a VisitAll on flags but yep it's not trivial. Maybe we can add
a Clone method upstream?

@silvin-lubecki Please have a look at 73f5adb and see if you think it is ok.

It doesn't try to copy all of the settings from the flagset to our local one, but I think it does enough.

Great, you found a nice way to construct another flag set 👍

cmd/docker/docker_test.go

silvin-lubecki · 2019-03-12T13:40:58Z

The validate task failed

make[1]: Entering directory '/go/src/github.com/docker/cli'
rm -rf vendor
bash -c 'vndr |& grep -v -i clone'
2019/03/12 12:35:21 Collecting initial packages
Too long with no output (exceeded 10m0s)``` 
😞

ijc · 2019-03-12T17:19:43Z

Too long with no output (exceeded 10m0s)

It worked for me locally in about 7m. GH was having an outage earlier, it's probably that.

`TestRunGoodArgument` fits here. Signed-off-by: Ian Campbell <[email protected]>

These won't pass right now due to docker#1699 ("Plugins can't re-use the same flags as cli global flags") and the change in 935d47b ("Ignore unknown arguments on the top-level command."), but the intention is to fix them now. Signed-off-by: Ian Campbell <[email protected]>

silvin-lubecki

LGTM, thank you @ijc 👍

The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <[email protected]>

Previous commits fixed the first issue on docker#1661, this simply adds a test for it. Note that this is testing the current behaviour, without regard for the second issue in docker#1661 which proposes a different behaviour. Signed-off-by: Ian Campbell <[email protected]>

This makes things more idempotent, rather than relying on undoing the interspersed settings. Note that the underlying `Flag`s remain shared, it's just the `FlagSet` which is duplicated. Signed-off-by: Ian Campbell <[email protected]>

GordonTheTurtle added the status/0-triage label Mar 7, 2019

thaJeztah added status/2-code-review area/plugins and removed status/0-triage labels Mar 7, 2019

thaJeztah approved these changes Mar 7, 2019

View reviewed changes

ijc force-pushed the plugins-global-arg-clash branch from e585336 to 88a1249 Compare March 8, 2019 10:24

ijc mentioned this pull request Mar 8, 2019

Add a method to expose unknown flags back to the user spf13/pflag#199

Open

ijc force-pushed the plugins-global-arg-clash branch from 88a1249 to 5dce8a6 Compare March 8, 2019 10:38

ijc force-pushed the plugins-global-arg-clash branch 2 times, most recently from 1fce752 to 9457f1f Compare March 11, 2019 15:44

silvin-lubecki reviewed Mar 11, 2019

View reviewed changes

silvin-lubecki reviewed Mar 12, 2019

View reviewed changes

cmd/docker/docker_test.go Show resolved Hide resolved

ijc force-pushed the plugins-global-arg-clash branch 2 times, most recently from 73f5adb to 7f18a66 Compare March 12, 2019 12:34

ijc force-pushed the plugins-global-arg-clash branch from 7f18a66 to 33fc528 Compare March 12, 2019 17:20

Ian Campbell added 2 commits March 13, 2019 10:54

e2e: start a new file for cli plugins arguments tests

4e5f0af

`TestRunGoodArgument` fits here. Signed-off-by: Ian Campbell <[email protected]>

silvin-lubecki approved these changes Mar 13, 2019

View reviewed changes

Ian Campbell added 3 commits March 13, 2019 11:28

Use a copy of root flagset in HandleGlobalFlags

e824bc8

This makes things more idempotent, rather than relying on undoing the interspersed settings. Note that the underlying `Flag`s remain shared, it's just the `FlagSet` which is duplicated. Signed-off-by: Ian Campbell <[email protected]>

ijc force-pushed the plugins-global-arg-clash branch from 33fc528 to e824bc8 Compare March 13, 2019 11:28

silvin-lubecki merged commit 237bdbf into docker:master Mar 13, 2019

GordonTheTurtle added this to the 19.03.0 milestone Mar 13, 2019

ijc deleted the plugins-global-arg-clash branch March 13, 2019 14:55

thaJeztah mentioned this pull request Apr 3, 2019

docker -H /var/run/docker.sock always fail #1801

Closed

ijc mentioned this pull request Apr 3, 2019

Only parse global args once #1802

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow plugins to have --flags which are the same as the top-level #1722

Allow plugins to have --flags which are the same as the top-level #1722

ijc commented Mar 7, 2019

thaJeztah commented Mar 7, 2019

thaJeztah left a comment

thaJeztah Mar 7, 2019

codecov-io commented Mar 8, 2019 •

edited

Loading

ijc commented Mar 8, 2019

ijc commented Mar 8, 2019

silvin-lubecki Mar 11, 2019

ijc Mar 11, 2019

ijc Mar 11, 2019

silvin-lubecki Mar 12, 2019

ijc Mar 12, 2019

silvin-lubecki Mar 13, 2019

silvin-lubecki commented Mar 12, 2019 •

edited

Loading

ijc commented Mar 12, 2019

silvin-lubecki left a comment

Allow plugins to have --flags which are the same as the top-level #1722

Allow plugins to have --flags which are the same as the top-level #1722

Conversation

ijc commented Mar 7, 2019

thaJeztah commented Mar 7, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Mar 7, 2019

Choose a reason for hiding this comment

codecov-io commented Mar 8, 2019 • edited Loading

Codecov Report

ijc commented Mar 8, 2019

ijc commented Mar 8, 2019

silvin-lubecki Mar 11, 2019

Choose a reason for hiding this comment

ijc Mar 11, 2019

Choose a reason for hiding this comment

ijc Mar 11, 2019

Choose a reason for hiding this comment

silvin-lubecki Mar 12, 2019

Choose a reason for hiding this comment

ijc Mar 12, 2019

Choose a reason for hiding this comment

silvin-lubecki Mar 13, 2019

Choose a reason for hiding this comment

silvin-lubecki commented Mar 12, 2019 • edited Loading

ijc commented Mar 12, 2019

silvin-lubecki left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 8, 2019 •

edited

Loading

silvin-lubecki commented Mar 12, 2019 •

edited

Loading