New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `--filter enabled=true` for `docker plugin ls` #28627

Merged
merged 2 commits into from Feb 1, 2017

Conversation

@yongtang
Member

yongtang commented Nov 19, 2016

- What I did

This fix adds --filter enabled=true to docker plugin ls, as was specified in #28624.

- How I did it

The related API and docs has been updated.

- How to verify it

An integration test has been added.

- Description for the changelog

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

2903397

This fix fixes #28624.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 19, 2016

Member

@yongtang Wow, you're fast!
Does this also support the inverse variant enabled=false, only listing those plugins that are disabled? If not, please add this as well.

Member

albers commented Nov 19, 2016

@yongtang Wow, you're fast!
Does this also support the inverse variant enabled=false, only listing those plugins that are disabled? If not, please add this as well.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 20, 2016

Member

Thanks @albers. The PR has been updated. Now enabled=true will only show enabled plugins and enabled=false will only show disabled plugins. In case no enabled filter is specified, all plugins will be shown.

I also take a look at the plugin type. It seems now plugin takes interface types. I am not sure the interface types could be directly translated into the normal plugin types (like volume, network, etc)

$ docker plugin inspect tiborvass/no-remove
...
            "Interface": {
                "Socket": "plugin.sock",
                "Types": [
                    "docker.volumedriver/1.0"
                ]
            },
...

Member

yongtang commented Nov 20, 2016

Thanks @albers. The PR has been updated. Now enabled=true will only show enabled plugins and enabled=false will only show disabled plugins. In case no enabled filter is specified, all plugins will be shown.

I also take a look at the plugin type. It seems now plugin takes interface types. I am not sure the interface types could be directly translated into the normal plugin types (like volume, network, etc)

$ docker plugin inspect tiborvass/no-remove
...
            "Interface": {
                "Socket": "plugin.sock",
                "Types": [
                    "docker.volumedriver/1.0"
                ]
            },
...

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 20, 2016

Member

I just verified the enabled filter. It works perfectly.
If the type filter requires more prior investigation, how about adding it in another PR?

Member

albers commented Nov 20, 2016

I just verified the enabled filter. It works perfectly.
If the type filter requires more prior investigation, how about adding it in another PR?

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 22, 2016

Member

@yongtang Are you planning to add the type filter in this PR or in another?

Member

albers commented Nov 22, 2016

@yongtang Are you planning to add the type filter in this PR or in another?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 23, 2016

Member

@albers The PR has been updated to allow filtering based on types.
Based on the docs (docs/extend/config.md) only two types are supported at the moment:

  • docker.volumedriver/1.0
  • docker.authz/1.0

Please take a look and let me know if there are any issues.

Member

yongtang commented Nov 23, 2016

@albers The PR has been updated to allow filtering based on types.
Based on the docs (docs/extend/config.md) only two types are supported at the moment:

  • docker.volumedriver/1.0
  • docker.authz/1.0

Please take a look and let me know if there are any issues.

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 23, 2016

Member

@yongtang I cannot verify the type filter because I get an error message:

$ docker plugin ls --filter type=docker.volumedriver/1.0
Error response from daemon: Invalid filter 'type'
$ docker version
Client:
 Version:      1.14.0-dev
 API version:  1.26
 Go version:   go1.7.3
 Git commit:   dceb38a
 Built:        Wed Nov 23 09:31:03 2016
 OS/Arch:      linux/amd64

Server:
 Version:             1.14.0-dev
 API version:         1.26
 Minimum API version: 1.12
 Go version:          go1.7.3
 Git commit:          dceb38a
 Built:               Wed Nov 23 09:31:03 2016
 OS/Arch:             linux/amd64
 Experimental:        false

docker version shows that I'm using the correct commit. Does it really work for you?

I'm also in doubt that the filter values are useful. As a user, I would like to get a list of e.g. all volume plugins. The value docker.volumedriver/1.0 is very technical and also too specifc. What if there are different versions of volumedrivers? Would the user have to specify -f type=docker.volumedriver/1.0 and -f type=docker.volumedriver/2.0 and possibly more values? The user would have to know all interface versions.

docker inspect --type uses the values network and volume. I think it is better to map the interface names to these known values and not to introduce new names for these concepts. Perhaps mapping could use regular expressions to decrease maintenance efforts.
As there is dockerd --authorization-plugin, I would suggest the value authorization instead of docker.authz/1.0

@thaJeztah WDYT, how should the type filters be named?

Member

albers commented Nov 23, 2016

@yongtang I cannot verify the type filter because I get an error message:

$ docker plugin ls --filter type=docker.volumedriver/1.0
Error response from daemon: Invalid filter 'type'
$ docker version
Client:
 Version:      1.14.0-dev
 API version:  1.26
 Go version:   go1.7.3
 Git commit:   dceb38a
 Built:        Wed Nov 23 09:31:03 2016
 OS/Arch:      linux/amd64

Server:
 Version:             1.14.0-dev
 API version:         1.26
 Minimum API version: 1.12
 Go version:          go1.7.3
 Git commit:          dceb38a
 Built:               Wed Nov 23 09:31:03 2016
 OS/Arch:             linux/amd64
 Experimental:        false

docker version shows that I'm using the correct commit. Does it really work for you?

I'm also in doubt that the filter values are useful. As a user, I would like to get a list of e.g. all volume plugins. The value docker.volumedriver/1.0 is very technical and also too specifc. What if there are different versions of volumedrivers? Would the user have to specify -f type=docker.volumedriver/1.0 and -f type=docker.volumedriver/2.0 and possibly more values? The user would have to know all interface versions.

docker inspect --type uses the values network and volume. I think it is better to map the interface names to these known values and not to introduce new names for these concepts. Perhaps mapping could use regular expressions to decrease maintenance efforts.
As there is dockerd --authorization-plugin, I would suggest the value authorization instead of docker.authz/1.0

@thaJeztah WDYT, how should the type filters be named?

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 23, 2016

Member

@albers In the last PR, there was an issue when I did the rebase and one line was missing. That was why in the last PR type was not working. Sorry about.

I updated the PR but there are some further changes.

The plugin in docker consists of two versions:

  • Legacy Plugin
    Those are the plugins that was installed through daemon. For example, the auth, volume, or network plugin. Those plugins will implement a "Implements" API to specify which feature it will support. one plugin support multiple features are possible. For example, it is quite common for one plugin to support IpamDriver and NetworkDriver at the same time.
    The documentation has been quite missing or incomplete for legacy plugin. Though I believe below is the list of possible values of "Implements":

    • GraphDriver
    • VolumeDriver
    • IpamDriver
    • NetworkDriver
    • authz
  • New Plugin (V2)
    The new plugin system types consists of three fields:

    • Capability (Currently either volumedriver or authz)
    • Prefix (Currently docker
    • Version (Currently it is 1.0)
      Combined of the above fields is the type: <Prefix>.<Capability>/<Version>. That is where docker.volumedriver/1.0 comes from.

When docker plugin ls or docker plugin install is invoked, only V2 plugins are impacted.

The closest match between New Plugin and Legacy Plugin probably is the Capability in V2 and Implements in Legacy.

I updated the PR to add the filter of --filter capability=volumedriver/--filter capability=authz (replaced the type field in the previous one). It is a separate commit in this PR for ease of review.

Please take a look and see if --filter capability=<volumedriver|authz> makes sense. And let me know if there are any alternative suggestions.

cc @vdemeester @thaJeztah

Member

yongtang commented Nov 23, 2016

@albers In the last PR, there was an issue when I did the rebase and one line was missing. That was why in the last PR type was not working. Sorry about.

I updated the PR but there are some further changes.

The plugin in docker consists of two versions:

  • Legacy Plugin
    Those are the plugins that was installed through daemon. For example, the auth, volume, or network plugin. Those plugins will implement a "Implements" API to specify which feature it will support. one plugin support multiple features are possible. For example, it is quite common for one plugin to support IpamDriver and NetworkDriver at the same time.
    The documentation has been quite missing or incomplete for legacy plugin. Though I believe below is the list of possible values of "Implements":

    • GraphDriver
    • VolumeDriver
    • IpamDriver
    • NetworkDriver
    • authz
  • New Plugin (V2)
    The new plugin system types consists of three fields:

    • Capability (Currently either volumedriver or authz)
    • Prefix (Currently docker
    • Version (Currently it is 1.0)
      Combined of the above fields is the type: <Prefix>.<Capability>/<Version>. That is where docker.volumedriver/1.0 comes from.

When docker plugin ls or docker plugin install is invoked, only V2 plugins are impacted.

The closest match between New Plugin and Legacy Plugin probably is the Capability in V2 and Implements in Legacy.

I updated the PR to add the filter of --filter capability=volumedriver/--filter capability=authz (replaced the type field in the previous one). It is a separate commit in this PR for ease of review.

Please take a look and see if --filter capability=<volumedriver|authz> makes sense. And let me know if there are any alternative suggestions.

cc @vdemeester @thaJeztah

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 23, 2016

Member

Just realized that the output of docker info does not include V2 Plugin for volume drivers at the moment. Will open a separate PR about the Plugins in docker info shortly.

Member

yongtang commented Nov 23, 2016

Just realized that the output of docker info does not include V2 Plugin for volume drivers at the moment. Will open a separate PR about the Plugins in docker info shortly.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 23, 2016

Member

Added a PR #28764 to address the issue for the output of Plugin sections in docker info.

Member

yongtang commented Nov 23, 2016

Added a PR #28764 to address the issue for the output of Plugin sections in docker info.

@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 23, 2016

Member

@yongtang Now it works.
I only checked with the tiborvass/no-remove plugin. Do you know an auth plugin I could use for testing?

Thanks for the excellent elaboration on the plugin APIs. I like your suggestion of a capability filter.

Member

albers commented Nov 23, 2016

@yongtang Now it works.
I only checked with the tiborvass/no-remove plugin. Do you know an auth plugin I could use for testing?

Thanks for the excellent elaboration on the plugin APIs. I like your suggestion of a capability filter.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 28, 2016

Member

Thanks @albers. I created a pass-through auth plugin yongtang/no-authz that could be used for test:
https://hub.docker.com/r/yongtang/no-authz/
https://github.com/yongtang/no-authz

ubuntu@ubuntu:~/$ docker plugin install yongtang/no-authz
Plugin "yongtang/no-authz" is requesting the following privileges:
 - network: [host]
Do you grant the above permissions? [y/N] y
yongtang/no-authz
ubuntu@ubuntu:~/$ docker plugin install tiborvass/no-remove
Plugin "tiborvass/no-remove" is requesting the following privileges:
 - network: []
 - mount: [/data]
Do you grant the above permissions? [y/N] y
tiborvass/no-remove
ubuntu@ubuntu:~/$ docker plugin ls
NAME                  TAG                 DESCRIPTION                  ENABLED
yongtang/no-authz     latest              No-AuthZ plugin for Docker   true
tiborvass/no-remove   latest              A test plugin for Docker     true
ubuntu@ubuntu:~/$ docker plugin ls --filter capability=volumedriver
NAME                  TAG                 DESCRIPTION                ENABLED
tiborvass/no-remove   latest              A test plugin for Docker   true
ubuntu@ubuntu:~/$ docker plugin ls --filter capability=authdriver
NAME                TAG                 DESCRIPTION                  ENABLED
yongtang/no-authz   latest              No-AuthZ plugin for Docker   true
ubuntu@ubuntu:~/$ 
Member

yongtang commented Nov 28, 2016

Thanks @albers. I created a pass-through auth plugin yongtang/no-authz that could be used for test:
https://hub.docker.com/r/yongtang/no-authz/
https://github.com/yongtang/no-authz

ubuntu@ubuntu:~/$ docker plugin install yongtang/no-authz
Plugin "yongtang/no-authz" is requesting the following privileges:
 - network: [host]
Do you grant the above permissions? [y/N] y
yongtang/no-authz
ubuntu@ubuntu:~/$ docker plugin install tiborvass/no-remove
Plugin "tiborvass/no-remove" is requesting the following privileges:
 - network: []
 - mount: [/data]
Do you grant the above permissions? [y/N] y
tiborvass/no-remove
ubuntu@ubuntu:~/$ docker plugin ls
NAME                  TAG                 DESCRIPTION                  ENABLED
yongtang/no-authz     latest              No-AuthZ plugin for Docker   true
tiborvass/no-remove   latest              A test plugin for Docker     true
ubuntu@ubuntu:~/$ docker plugin ls --filter capability=volumedriver
NAME                  TAG                 DESCRIPTION                ENABLED
tiborvass/no-remove   latest              A test plugin for Docker   true
ubuntu@ubuntu:~/$ docker plugin ls --filter capability=authdriver
NAME                TAG                 DESCRIPTION                  ENABLED
yongtang/no-authz   latest              No-AuthZ plugin for Docker   true
ubuntu@ubuntu:~/$ 
@albers

This comment has been minimized.

Show comment
Hide comment
@albers

albers Nov 29, 2016

Member

@yongtang Wow, thanks a log for providing a sample authz plugin!
I verified the --filters and everything works perfectly!
LGTM

Member

albers commented Nov 29, 2016

@yongtang Wow, thanks a log for providing a sample authz plugin!
I verified the --filters and everything works perfectly!
LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Nov 29, 2016

Member

Desgin LGTM but I think this will be for 1.14.0 though 👼

Member

vdemeester commented Nov 29, 2016

Desgin LGTM but I think this will be for 1.14.0 though 👼

@vdemeester vdemeester added this to the 1.14.0 milestone Nov 29, 2016

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 8, 2016

Member

looks like this needs a rebase, @yongtang 😢

Member

thaJeztah commented Dec 8, 2016

looks like this needs a rebase, @yongtang 😢

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 8, 2016

Member

Thanks @thaJeztah. The PR has been rebased.

Member

yongtang commented Dec 8, 2016

Thanks @thaJeztah. The PR has been rebased.

@vdemeester

SGTM 👼
Needs a rebase and added a few comments

Show outdated Hide outdated plugin/backend_linux.go Outdated
Show outdated Hide outdated plugin/backend_linux.go Outdated
"golang.org/x/net/context"
)
// PluginList returns the installed plugins
func (cli *Client) PluginList(ctx context.Context) (types.PluginsListResponse, error) {
func (cli *Client) PluginList(ctx context.Context, filter filters.Args) (types.PluginsListResponse, error) {

This comment has been minimized.

@vdemeester

vdemeester Dec 27, 2016

Member

Could we add unit tests to validate the filters are sent (see other client method tests) 👼

@vdemeester

vdemeester Dec 27, 2016

Member

Could we add unit tests to validate the filters are sent (see other client method tests) 👼

This comment has been minimized.

@yongtang

yongtang Dec 27, 2016

Member

Thanks. The unit test has been updated.

@yongtang

yongtang Dec 27, 2016

Member

Thanks. The unit test has been updated.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 27, 2016

Member

@vdemeester Thanks for the review. The PR has been updated. Please take a look and let me know if there are any additional issues.

Member

yongtang commented Dec 27, 2016

@vdemeester Thanks for the review. The PR has been updated. Please take a look and let me know if there are any additional issues.

@vdemeester

The code as of now will return all plugin if --filter enable=false ? (I would think it should only return plugin that are not enabled)

return nil, err
}
enabledOnly := false

This comment has been minimized.

@vdemeester

vdemeester Dec 28, 2016

Member

I think you misunderstood my comment 😅 I just think we don't support 1 and 0 for boolean filters (--filter dangling=true works, --filter dangling=1 does not).

@vdemeester

vdemeester Dec 28, 2016

Member

I think you misunderstood my comment 😅 I just think we don't support 1 and 0 for boolean filters (--filter dangling=true works, --filter dangling=1 does not).

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 28, 2016

Member

@vdemeester I was looking into the keyword dangling and was mislead by the followingdangling: 😅
https://github.com/docker/docker/blob/v1.13.0-rc4/daemon/list.go#L641-L649

	danglingOnly := false
	if filter.Include("dangling") {
		if filter.ExactMatch("dangling", "true") || filter.ExactMatch("dangling", "1") {
			danglingOnly = true
		} else if !filter.ExactMatch("dangling", "false") && !filter.ExactMatch("dangling", "0") {
			return nil, fmt.Errorf("Invalid filter 'dangling=%s'", filter.Get("dangling"))
		}
		retVols = daemon.volumes.FilterByUsed(retVols, !danglingOnly)
	}

The PR has been updated now. Please take a look.

Member

yongtang commented Dec 28, 2016

@vdemeester I was looking into the keyword dangling and was mislead by the followingdangling: 😅
https://github.com/docker/docker/blob/v1.13.0-rc4/daemon/list.go#L641-L649

	danglingOnly := false
	if filter.Include("dangling") {
		if filter.ExactMatch("dangling", "true") || filter.ExactMatch("dangling", "1") {
			danglingOnly = true
		} else if !filter.ExactMatch("dangling", "false") && !filter.ExactMatch("dangling", "0") {
			return nil, fmt.Errorf("Invalid filter 'dangling=%s'", filter.Get("dangling"))
		}
		retVols = daemon.volumes.FilterByUsed(retVols, !danglingOnly)
	}

The PR has been updated now. Please take a look.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 28, 2016

Member

@vdemeester On a side note, I am wondering if it makes sense to remove the option of 1|0 on the boolean filter for volumes (as was referenced above)?

I don't have a personal preference on true|false or 1|0, either way is fine for me but would like to see consistency across the board. Maybe we should open a separate PR to remove it?

Member

yongtang commented Dec 28, 2016

@vdemeester On a side note, I am wondering if it makes sense to remove the option of 1|0 on the boolean filter for volumes (as was referenced above)?

I don't have a personal preference on true|false or 1|0, either way is fine for me but would like to see consistency across the board. Maybe we should open a separate PR to remove it?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Dec 28, 2016

Member

@yongtang oh I didn't know that.. Yeah I'm all for consistency here. Summoning @thaJeztah to get opinion 👼

Member

vdemeester commented Dec 28, 2016

@yongtang oh I didn't know that.. Yeah I'm all for consistency here. Summoning @thaJeztah to get opinion 👼

Add `capability` filter to `docker plugin ls`
This fix adds `--filter capability=[volumedriver|authz]` to `docker plugin ls`.

The related docs has been updated.

An integration test has been added.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jan 27, 2017

Member

Moving to docs-review /cc @thaJeztah

Member

vdemeester commented Jan 27, 2017

Moving to docs-review /cc @thaJeztah

@vdemeester

docs LGTM 🐸
/cc @thaJeztah for docs revisit

@vdemeester vdemeester merged commit 4c1b40b into moby:master Feb 1, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30039 has succeeded
Details
janky Jenkins build Docker-PRs 38651 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9680 has succeeded
Details

@vdemeester vdemeester removed their assignment Feb 1, 2017

@yongtang yongtang deleted the yongtang:28624-docker-plugin-ls branch Feb 1, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Feb 1, 2017

Member

Thanks @yongtang, docs LGTM, but could you also create a markdown for the man-pages, with just the "example" and "description", similar to https://github.com/docker/docker/blob/master/man/src/container/ls.md

That file will have to be in a "plugins" subdirectory inside man/src/

Member

thaJeztah commented Feb 1, 2017

Thanks @yongtang, docs LGTM, but could you also create a markdown for the man-pages, with just the "example" and "description", similar to https://github.com/docker/docker/blob/master/man/src/container/ls.md

That file will have to be in a "plugins" subdirectory inside man/src/

@thaJeztah thaJeztah removed the docs/revisit label Feb 1, 2017

yongtang added a commit to yongtang/docker that referenced this pull request Feb 1, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang added a commit to yongtang/docker that referenced this pull request Feb 2, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

danielpanteleit added a commit to danielpanteleit/docker that referenced this pull request Feb 7, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28627 from yongtang/28624-docker-plugin-ls
Add `--filter enabled=true` for `docker plugin ls`

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#28627 from yongtang/28624-docker-plugin-ls
Add `--filter enabled=true` for `docker plugin ls`

tiborvass added a commit to tiborvass/cli that referenced this pull request May 11, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby/moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request Jun 5, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby/moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Upstream-commit: 6686179ff5d712a67d4a4119d33be2dc3fd4eea8
Component: cli

dnephin pushed a commit to dnephin/cli that referenced this pull request Jun 14, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby/moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

srust added a commit to srust/moby that referenced this pull request Nov 30, 2017

Add markdown for man page of `docker plugin ls`
This fix adds markdown for man page of `docker plugin ls`,
based on moby#28627 (comment)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment