Skip to content

plugin/store.Get: return a specific error if plugin is disabled#33640

Merged
vdemeester merged 2 commits intomoby:masterfrom
dsheets:pluginv2-static-start-but-disabled-error
Jun 14, 2017
Merged

plugin/store.Get: return a specific error if plugin is disabled#33640
vdemeester merged 2 commits intomoby:masterfrom
dsheets:pluginv2-static-start-but-disabled-error

Conversation

@dsheets
Copy link
Copy Markdown
Contributor

@dsheets dsheets commented Jun 12, 2017

- What I did

Previously, a 'plugin not found' error would be returned if a plugin to be retrieved was found but disabled. This was misleading and incorrect. Now, a new error plugin.ErrDisabled is returned in this case. This makes the error message when trying to statically start plugins (from daemon.json or dockerd command line) accurate.

- How I did it

I created a new type alias ErrDisabled to mirror ErrNotFound and ErrAmbiguous. It satisfies the error interface.

- How to verify it

docker plugin install --grant-all-permissions riyaz/authz-no-volume-plugin
docker plugin disable riyaz/authz-no-volume-plugin

then restart dockerd and add --authorization-plugin=riyaz/authz-no-volume-plugin to the command line. docker.log will then have the "found but disabled" message instead of the "not found" message.

I tried to write a test for this but I couldn't figure out how to match on the daemon log error.

- Description for the changelog

Probably does not deserve a changelog entry but if you add one:

"Improved error message when trying to start daemon with installed but disabled plugin"

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

Suncus etruscus
By Trebol-a - Own work, CC BY-SA 3.0, https://commons.wikimedia.org/w/index.php?curid=4607913

Suncus etruscus, the Etruscan shrew, is the smallest known mammal by mass and consumes 1.5-2x its own body weight in food per day. It has a very high heart rate of up to 1511 beats/minute.

David Sheets added 2 commits June 12, 2017 18:06
Previously, a 'plugin not found' error would be returned if a plugin to be
retrieved was found but disabled. This was misleading and incorrect. Now,
a new error plugin.ErrDisabled is returned in this case. This makes the
error message when trying to statically start plugins (from daemon.json or
dockerd command line) accurate.

Signed-off-by: David Sheets <dsheets@docker.com>
Signed-off-by: David Sheets <dsheets@docker.com>
@dsheets dsheets force-pushed the pluginv2-static-start-but-disabled-error branch from 269fad7 to 2b79dfc Compare June 12, 2017 17:07
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

libnetwork is consuming this error, but the usage doesn't seem to change at all by changing the error here.

ping @sanimej

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well, but ping @sanimej if changes are needed in libnetwork

@sanimej
Copy link
Copy Markdown

sanimej commented Jun 13, 2017

The way the plugin errors are getting used is incorrect. For ex: libnetwork references the ErrNotFound error from docker/pkg/plugins package. But the plugingetter's Get returns ErrNotFound (or any other for matter) defined in plugin package. So the equality check is not going work. We can move the error definitions from plugin/store.go to pkg/plugins ?

This is an existing issue and not specific to this PR. We can either address it here or do it as a separate PR.

@cpuguy83
Copy link
Copy Markdown
Member

Ugh, I'd prefer not to expand the surface area of plugingetter.

@sanimej
Copy link
Copy Markdown

sanimej commented Jun 13, 2017

plugingetter doesn't have to change. We can move the error types defined in plugin/store.go to pkg/plugins/discovery.go. Since pkg/plugins is included by the plugin package and the clients (like libnetwork) as well the same type will get used.

@sanimej
Copy link
Copy Markdown

sanimej commented Jun 13, 2017

I don't want to block this PR though. This LGTM. Lets fix the incorrect error type reference in a separate PR.

@vdemeester vdemeester merged commit 397a57d into moby:master Jun 14, 2017
@dsheets dsheets deleted the pluginv2-static-start-but-disabled-error branch June 14, 2017 13:52
@thaJeztah
Copy link
Copy Markdown
Member

ping @sanimej were you planning on working on those changes, or do we have to open an issue for tracking that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants