refactor plugin install #28963

vieux · 2016-11-30T00:41:03Z

before

calling /pull was pulling, installing a plugin, then a lists a privileges was returned.
upon a user accepting the privileges, it would enable the plugin.

problems:

the plugin was installed before the user accepted the privileges.
<ctrl-c> the "do you accept privileges" question would leave the plugin installed on the daemon
the whole privileges concept was entirely client side

now

calling /privileges will return the list of privileges required by the plugin (noting is installed)
update a user accepting the privileges, /pull is called with the list of privileges accepted
the daemon now compares the required privileges and the privileges sent by the user before installing

it forces a round-trip so even through the API there is an acceptance mechanism

ping @tiborvass @anusha-ragunathan

AkihiroSuda · 2016-11-30T07:00:32Z

Compilation failing for windows

02:09:46 Building: bundles/1.14.0-dev/cross/windows/amd64/dockerd-1.14.0-dev.exe
02:11:05 # github.com/docker/docker/cmd/dockerd
02:11:05 cmd/dockerd/daemon.go:474: cannot use "github.com/docker/docker/plugin".GetManager() (type *"github.com/docker/docker/plugin".Manager) as type "github.com/docker/docker/api/server/router/plugin".Backend in argument to "github.com/docker/docker/api/server/router/plugin".NewRouter:
02:11:05 	*"github.com/docker/docker/plugin".Manager does not implement "github.com/docker/docker/api/server/router/plugin".Backend (missing Privileges method)

LK4D4 · 2016-11-30T20:11:23Z

@vieux psst

vieux · 2016-11-30T22:47:53Z

@LK4D4 thanks

vieux · 2016-12-01T01:37:41Z

ping @tiborvass @anusha-ragunathan

anusha-ragunathan · 2016-12-02T16:56:40Z

client/plugin_install.go

@@ -65,7 +74,12 @@ func (cli *Client) PluginInstall(ctx context.Context, name string, options types
 	return cli.PluginEnable(ctx, name, types.PluginEnableOptions{Timeout: 0})
 }

-func (cli *Client) tryPluginPull(ctx context.Context, query url.Values, registryAuth string) (serverResponse, error) {
+func (cli *Client) tryPluginGetPrivileges(ctx context.Context, query url.Values, registryAuth string) (serverResponse, error) {


nit: can be tryPluginPrivileges to be consistent with other client function names.

anusha-ragunathan · 2016-12-02T17:21:43Z

plugin/distribution/pull.go

@@ -177,7 +177,6 @@ func WritePullData(pd PullData, dest string, extract bool) error {
 	if err := json.Unmarshal(config, &p); err != nil {
 		return err
 	}
-	logrus.Debugf("%#v", p)


Lets keep this. Its useful to look at the payload during debugging.

anusha-ragunathan · 2016-12-02T17:23:22Z

api/swagger.yaml

+  /plugins/privileges:
+    get:
+      summary: "Get plugin privileges"
+      operationId: "GetPluginsPrivileges"


Its a single plugin. So GetPluginPrivileges

anusha-ragunathan · 2016-12-02T17:45:02Z

api/swagger.yaml

+            $ref: "#/definitions/ErrorResponse"
+      parameters:
+        - name: "name"
+          in: "query"


I'm trying to produce a human readable engine-api docs out of this. According to instructions on https://github.com/docker/docker/blob/master/api/README.md, I need to Copy api/swagger.yaml in this repository to engine/api/[VERSION_NUMBER]/swagger.yaml in the documentation repository.

Interestingly, there's no such file in https://github.com/docker/docker.github.io. How did you verify this doc, @vieux ?

@mstanleyjones @bfirsh : Vendoring the swagger.yaml frequently to the docs repo is supposed to generate a human readable API reference, yes? Few questions:

How frequent is this vendoring?

When and where can I get the 1.25 api reference doc? We have to send the link to a few partners working on plugins and there's no such doc in any repo at the moment. @khudgins

The latest version for a particular version is vendored automatically on every deploy. There has been some back and forth between whether this should be pinned to a commit or automatically the latest, so that might change.

As @cpuguy83 says, Add make command for previewing API docs #29078 should help you there. :)

anusha-ragunathan · 2016-12-02T19:42:53Z

plugin/backend_linux.go


-	if p, _ := pm.pluginStore.GetByName(name); p != nil {


We should keep this check in Pull to avoid a race where another client has already pulled the plugin by now.

Pull is calling pull that is doing this check.

anusha-ragunathan · 2016-12-02T19:50:18Z

plugin/backend_linux.go

 	}

-	priv, err := pm.pull(ref, metaHeader, authConfig, pluginID)
-	if err != nil {
+	removePluginDir := func() {


Its more elegant if we change this to a defer that acts on a named return error. Then we can remove L182 and L189.

defer func() { if err != nil { os.RemoveAll(pluginDir)... } }()

Signed-off-by: Victor Vieux <[email protected]>

vieux · 2016-12-02T23:25:00Z

@anusha-ragunathan PTAL

anusha-ragunathan · 2016-12-02T23:29:58Z

LGTM (if tests pass)

tiborvass · 2016-12-05T20:00:23Z

LGTM

refactor plugin install

vieux added area/api area/plugins impact/api impact/changelog process/cherry-pick status/2-code-review labels Nov 30, 2016

vieux added this to the 1.13.0 milestone Nov 30, 2016

GordonTheTurtle added the status/0-triage label Nov 30, 2016

vieux removed the status/0-triage label Nov 30, 2016

vdemeester added the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 30, 2016

vieux force-pushed the refactor_plugin_install branch from 9adeceb to f31b378 Compare November 30, 2016 22:47

vieux force-pushed the refactor_plugin_install branch from f31b378 to 521287e Compare November 30, 2016 23:34

vieux removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 1, 2016

vieux force-pushed the refactor_plugin_install branch 2 times, most recently from aef8096 to c7b0481 Compare December 2, 2016 02:03

vieux assigned tiborvass and anusha-ragunathan Dec 2, 2016

anusha-ragunathan reviewed Dec 2, 2016

View reviewed changes

vieux force-pushed the refactor_plugin_install branch from c7b0481 to 0db0ec2 Compare December 2, 2016 23:22

vieux added 2 commits December 2, 2016 15:23

refactor plugin install

fa3b61a

Signed-off-by: Victor Vieux <[email protected]>

update docs

30db51c

Signed-off-by: Victor Vieux <[email protected]>

vieux force-pushed the refactor_plugin_install branch from 0db0ec2 to 30db51c Compare December 2, 2016 23:24

tiborvass merged commit 1c96879 into moby:master Dec 5, 2016

vieux added process/cherry-picked and removed process/cherry-pick labels Dec 8, 2016

vieux mentioned this pull request Dec 8, 2016

1.13.0-rc4 cherry-picks: part1 #29229

Merged

vieux deleted the refactor_plugin_install branch December 13, 2016 01:26

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

Merge pull request moby#28963 from vieux/refactor_plugin_install

cff6855

refactor plugin install

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor plugin install #28963

refactor plugin install #28963

vieux commented Nov 30, 2016

AkihiroSuda commented Nov 30, 2016

LK4D4 commented Nov 30, 2016

vieux commented Nov 30, 2016

vieux commented Dec 1, 2016

anusha-ragunathan Dec 2, 2016

anusha-ragunathan Dec 2, 2016

anusha-ragunathan Dec 2, 2016

anusha-ragunathan Dec 2, 2016

cpuguy83 Dec 2, 2016

anusha-ragunathan Dec 2, 2016

bfirsh Dec 8, 2016

anusha-ragunathan Dec 2, 2016 •

edited

Loading

vieux Dec 2, 2016

anusha-ragunathan Dec 2, 2016 •

edited

Loading

vieux commented Dec 2, 2016

anusha-ragunathan commented Dec 2, 2016

tiborvass commented Dec 5, 2016

refactor plugin install #28963

refactor plugin install #28963

Conversation

vieux commented Nov 30, 2016

before

problems:

now

AkihiroSuda commented Nov 30, 2016

LK4D4 commented Nov 30, 2016

vieux commented Nov 30, 2016

vieux commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anusha-ragunathan Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

vieux commented Dec 2, 2016

anusha-ragunathan commented Dec 2, 2016

tiborvass commented Dec 5, 2016

anusha-ragunathan Dec 2, 2016 •

edited

Loading

anusha-ragunathan Dec 2, 2016 •

edited

Loading