Plugins content addressability #29487

tonistiigi · 2016-12-16T17:45:40Z

This addresses:

plugin distribution is moved to a shared stack with images. This fixes quirks with previous implementation including pull-by-digest, content verification and progressbars. fixes [Feature request] Show progress in plugin install #28622
creates immutable plugin config for configuration that matches schema2 requirements. Hash of this config defines the whole distributable part of the config.
Ensures data being pushed is the same that was created/pulled so that plugin can't leak anything in there.
Keeps immutable distribution artifacts in blobstore and uses that for the data that is pushed.
Runs the init layer setup on every plugin start to match the safety guarantees of containers
Fixes some possible breakouts caused by unsafe filesystem accesses or extraction.
Verifies the tar sent on plugin create and splits it up to a config and rootfs.
Adds docker install --alias to control the name of the plugin.
Uses normalized references for default names to avoid collisions when using default hosts/tags.
Some refactoring of the plugin manager to support the change, like removing the singleton manager and adding manager config stuct.

Does not address:

Runtime related refactoring like removing the controller and Store have been left out
Likely concurrency problems with manager commands(except create/push/pull).

aaronlehmann · 2016-12-16T18:18:07Z

api/types/plugin.go

-
-	// tag
-	// Required: true
-	Tag string `json:"Tag"`


This will require a small change in the executors (both SwarmKit's and Docker's), but it will be a simplification.

https://github.com/docker/docker/pull/29378/files#diff-ac864daec98b5e7e257026311a914256R71

What exactly is a plugin tag?

I see that daemon/cluster/executor/container/executor.go has been updated in this PR. SwarmKit will need a similar change. Making a note here so we don't forget.

aaronlehmann · 2016-12-16T18:18:29Z

api/types/plugin.go

@@ -81,6 +77,9 @@ type PluginConfig struct {
 	// workdir
 	// Required: true
 	Workdir string `json:"Workdir"`
+
+	// rootfs
+	Rootfs *PluginConfigRootfs `json:"rootfs,omitempty"`


aaronlehmann · 2016-12-16T18:18:45Z

api/types/plugin.go

+type PluginConfigRootfs struct {
+
+	// diff ids
+	DiffIds []string `json:"diff_ids"`


good luck controlling this in swagger

I'm glad we went to a system we can't control...

Discussed with @dnephin and this should be solvable with x-go-name: but it has some issue atm. The json names are correct so we won't have breaking changes when we switch the go names.

In this vein, the problem becomes obvious when we start looking that the godoc.

aaronlehmann · 2016-12-16T18:21:14Z

cli/command/plugin/install.go

+}
+
+func runInstall(dockerCli *command.DockerCli, opts pluginOptions) error {
+	ref, err := distreference.ParseNamed(opts.name)


Yuck :(. Is this using distreference to avoid the normalization? Needs a huge FIXME to explain why distreference and what should be done when the packages are merged.

I think it's using distreference to allow references with both tags and digest. But not sure if this is needed at all in here. Daemon side should be enough for this. cc @dmcgowan

I can add a comment here, this really needs to converge in 1.14 to not have these differences. This validation is here because there was something here before and I didn't want to just rely on the daemon. The daemon certainly should be doing the right thing already and I can minimize the duplication of validation here if that makes sense.

aaronlehmann · 2016-12-16T18:22:55Z

cli/command/plugin/install.go

 		return err
 	}
-	fmt.Fprintln(dockerCli.Out(), opts.name)
+	fmt.Fprintf(dockerCli.Out(), "Installed plugin %v\n", opts.name) // todo: return proper values from the API for this result


aaronlehmann · 2016-12-16T18:27:08Z

client/plugin_install.go

 	query := url.Values{}
-	query.Set("name", name)
+	if _, err := reference.ParseNamed(options.RemoteRef); err != nil {
+		return nil, err


Wrap error?

aaronlehmann · 2016-12-16T18:34:11Z

distribution/metadata/v2_metadata_service.go

@@ -151,6 +158,9 @@ func (serv *v2MetadataService) GetDiffID(dgst digest.Digest) (layer.DiffID, erro
 // Add associates metadata with a layer DiffID. If too many metadata entries are
 // present, the oldest one is dropped.
 func (serv *v2MetadataService) Add(diffID layer.DiffID, metadata V2Metadata) error {
+	if serv.store == nil {
+		return nil


I think I understand why the getters return an error but the setters silently do nothing, but it might be good to have a comment explaining it.

I will add a comment here. I think the better solution is just to have an inmemory implementation or allow the pullers/pushers to have this service be nil. In the end we probably just want to support this interface backed by disk because it can optimize pushing.

aaronlehmann · 2016-12-16T18:51:54Z

plugin/backend_linux.go

+	// Parse remote reference, supporting remotes with name and tag
+	// NOTE: Using distribution reference to handle references
+	// containing both a name and digest
+	remoteRef, err := distreference.ParseNamed(remote)


Is this going to cause problems wrt normalization? I believe Pull expects a normalized reference, which you won't get from the distribution package.

Never mind, I somehow missed the conversion below.

We end up calling with both, we use the WithName later on from the docker reference package which will normalize. It is a completely mess, but need the tag and digest support from the real reference package and the normalization from the docker one.

aaronlehmann · 2016-12-16T18:56:30Z

plugin/backend_linux.go

+	return computePrivileges(config)
+}
+
+// Pull pulls a plugin, check if the correct privileges are provided and install the plugin.


Please document the difference between name and remote.

aaronlehmann · 2016-12-16T19:01:09Z

plugin/backend_linux.go

 	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/pkg/archive"
+	dockerdist "github.com/docker/docker/distribution"


Why "dockerdist"?

we can update now, previously plugin also had a "distribution" package. But we just removed it

aaronlehmann · 2016-12-16T19:02:28Z

plugin/backend_linux.go

+		tarWriter := tar.NewWriter(pw)
+		defer in.Close()
+
+		hasRootfs := false


hasRootFS

aaronlehmann · 2016-12-16T19:06:42Z

plugin/backend_linux.go

+			content := io.Reader(tarReader)
+			name := filepath.Clean(hdr.Name)
+			if filepath.IsAbs(name) {
+				name = name[1:]


This looks unportable. Granted, we're in an _linux.go file here, so it doesn't matter for now.

aaronlehmann · 2016-12-16T19:08:07Z

plugin/manager.go

-	manager *Manager
-)
+const configFileName = "config.json"
+const rootfsFileName = "rootfs"


rootFSFileName :)

anusha-ragunathan · 2016-12-19T20:00:41Z

plugin/backend_linux.go

+				return
+			}
+			if err != nil {
+				pw.CloseWithError(errors.Wrap(err, "failed to read from tar"))


tarWriter.Close() ?

tarWriter would be just cleaned up by gc. Only need to close it when returning without error so that paddings are correct.

anusha-ragunathan · 2016-12-19T20:02:25Z

plugin/backend_linux.go

+			if name == configFileName {
+				dt, err := ioutil.ReadAll(content)
+				if err != nil {
+					pw.CloseWithError(errors.Wrap(err, "failed to read config.json"))


nit: "failed to read %s", configFileName

anusha-ragunathan · 2016-12-19T21:50:34Z

plugin/backend_linux.go

+	}
+	gzw := gzip.NewWriter(n)
+	digester := digest.Canonical.New()
+	rootfsReader := io.TeeReader(rootfs, io.MultiWriter(gzw, digester.Hash()))


can you not directly call digest.Canonical.Hash()?

digester is used later to get the digest for the data.

anusha-ragunathan · 2016-12-19T21:54:37Z

plugin/backend_linux.go

+}
+
+type insertion struct {
+	io.Writer


Writer can be made private?

The Write() method required by the interface directly comes from the embedded io.Writer. Also insertion itself is private.

anusha-ragunathan · 2016-12-19T21:58:15Z

plugin/backend_linux.go

+		for {
+			hdr, err := tarReader.Next()
+			if err == io.EOF {
+				if !hasRootfs {


why not have a similar logic for !hasConfig?

Config can be easily checked from the caller side in https://github.com/docker/docker/pull/29487/files#diff-28e4bbe4b8a4c1b80bf43d15a145427dR557 where it is cleaner. No way to check rootfs from the caller side so it needs to be included in the middle of tar creation.

anusha-ragunathan · 2016-12-19T22:32:37Z

plugin/backend_linux.go

+	configBlob, err := pm.blobStore.New()
+	if err != nil {
+		return err
+	}


defer configBlob.Close()

Never mind, Looks like you are taking care of it in insertion.

stevvooe · 2016-12-19T22:33:14Z

distribution/push_v2.go

+	reader = progress.NewProgressReader(ioutils.NewCancelReadCloser(ctx, contentReader), progressOutput, size, pd.ID(), "Pushing")
+
+	switch m := pd.layer.MediaType(); m {
+	case "application/vnd.docker.image.rootfs.diff.tar":


Do we have this defined elsewhere?

after rebase this will ago away, we defined in distribution

Also, should this be plugin.rootfs?

The content types are not very consistent. Really we have plugin images and container images, but for the name roots we have

application/vnd.docker.container.image - for container image configs
application/vnd.docker.plugin - for plugin configs
application/vnd.docker.image - for image layers

The plugin image layers are the same as container image layers though and should be able to share a backend.

application/vnd.docker.container.image is the one that went awry. This should have been application/vnd.docker.image.config, which is what is done in OCI.

Nothing is stopping these from being stored together. The mediatype isn't necessarily store with the object. The mediatype is part of the qualified reference, but not a property of the thing itself. For that to work, we'd have to hash the object with its type.

stevvooe · 2016-12-19T22:46:02Z

api/types/plugin.go

@@ -81,6 +77,9 @@ type PluginConfig struct {
 	// workdir
 	// Required: true
 	Workdir string `json:"Workdir"`


anusha-ragunathan · 2016-12-22T23:07:29Z

client/plugin_install.go

 	resp, err := cli.tryPluginPrivileges(ctx, query, options.RegistryAuth)
 	if resp.statusCode == http.StatusUnauthorized && options.PrivilegeFunc != nil {
 		newAuthHeader, privilegeErr := options.PrivilegeFunc()
 		if privilegeErr != nil {
 			ensureReaderClosed(resp)
-			return privilegeErr
+			return nil, privilegeErr


Not this change: If a plugin exists and we try to pull it, it should error out before tryPluginPrivileges. Please add a TODO item.

anusha-ragunathan · 2016-12-22T23:59:32Z

Tested the PR and happy with the changes. LGTM.

dmcgowan · 2016-12-23T01:16:52Z

integration-cli/docker_cli_plugins_test.go

@@ -139,9 +141,16 @@ func (s *DockerSuite) TestPluginInstallArgs(c *check.C) {
 	c.Assert(strings.TrimSpace(env), checker.Equals, "[DEBUG=1]")
 }

-func (s *DockerSuite) TestPluginInstallImage(c *check.C) {
+func (s *DockerRegistrySuite) TestPluginInstallImage(c *check.C) {
 	testRequires(c, DaemonIsLinux, IsAmd64, Network)


Network is no longer required

dmcgowan · 2016-12-23T21:05:49Z

plugin/backend_linux.go

+			if parts := strings.Split(name, "/"); len(parts) != 0 && parts[0] == rootFSFileName {
+				hdr.Name = path.Clean(path.Join(parts[1:]...))
+				if strings.HasPrefix(strings.ToLower(hdr.Linkname), rootFSFileName+"/") {
+					hdr.Linkname = hdr.Linkname[len(rootFSFileName):]


hdr.Linkname = hdr.Linkname[len(rootFSFileName)+1:]

Move plugins to shared distribution stack with images. Create immutable plugin config that matches schema2 requirements. Ensure data being pushed is same as pulled/created. Store distribution artifacts in a blobstore. Run init layer setup for every plugin start. Fix breakouts from unsafe file accesses. Add support for `docker plugin install --alias` Uses normalized references for default names to avoid collisions when using default hosts/tags. Some refactoring of the plugin manager to support the change, like removing the singleton manager and adding manager config struct. Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com> Signed-off-by: Derek McGowan <derek@mcgstyle.net>

dmcgowan · 2016-12-23T21:32:33Z

LGTM

This fix is a follow up based on comment: and a follow up to: moby#29222 (comment) As moby#28789 has been merged in, it is possible for `docker plugin inspect` to search based on Name or ID Prefix. However, ID-based `docker plugin enable/disable/rm/set` are still not possible. This fix addes test for `docker plugin enable/disable/rm/set` to search based on: - Full ID - Full Name - Partial ID (prefix) The actual fix is done in moby#29487. This fix is a follow up of moby#28789 and moby#29487. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

GordonTheTurtle added the status/0-triage label Dec 16, 2016

aaronlehmann reviewed Dec 16, 2016

View reviewed changes

tonistiigi force-pushed the plugins-rework branch from 5ca4bab to 768b9e9 Compare December 16, 2016 18:56

aaronlehmann reviewed Dec 16, 2016

View reviewed changes

plugin/backend_linux.go

tarWriter := tar.NewWriter(pw)

defer in.Close()

hasRootfs := false

Copy link

Contributor

aaronlehmann Dec 16, 2016

Choose a reason for hiding this comment

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

hasRootFS

aaronlehmann reviewed Dec 16, 2016

View reviewed changes

tonistiigi force-pushed the plugins-rework branch from 768b9e9 to 2e791fd Compare December 16, 2016 19:20

thaJeztah added status/1-design-review and removed status/0-triage labels Dec 17, 2016

tonistiigi force-pushed the plugins-rework branch 2 times, most recently from ee06495 to 745923b Compare December 19, 2016 17:37

anusha-ragunathan reviewed Dec 19, 2016

View reviewed changes

stevvooe reviewed Dec 19, 2016

View reviewed changes

tonistiigi force-pushed the plugins-rework branch from 745923b to 6b086e3 Compare December 20, 2016 01:12

tonistiigi changed the title ~~wip: Plugins content addressbility~~ Plugins content addressbility Dec 20, 2016

tonistiigi force-pushed the plugins-rework branch from e57fddd to 94ec8ad Compare December 22, 2016 01:59

tonistiigi mentioned this pull request Dec 22, 2016

Add tests for ID-based docker plugin enable/disable/rm/set #29222

Merged

tonistiigi force-pushed the plugins-rework branch from 94ec8ad to 885705d Compare December 22, 2016 23:05

anusha-ragunathan reviewed Dec 22, 2016

View reviewed changes

dmcgowan mentioned this pull request Dec 23, 2016

Support Docker Content Trust for plugins #29668

Merged

tonistiigi force-pushed the plugins-rework branch from 885705d to 6bf801d Compare December 23, 2016 01:00

dmcgowan reviewed Dec 23, 2016

View reviewed changes

tonistiigi force-pushed the plugins-rework branch from 6bf801d to dc1cebf Compare December 23, 2016 03:38

dmcgowan reviewed Dec 23, 2016

View reviewed changes

tonistiigi force-pushed the plugins-rework branch from dc1cebf to 3d86b0c Compare December 23, 2016 21:30

dmcgowan added status/4-merge and removed status/2-code-review labels Dec 23, 2016

tonistiigi added the rebuild/windowsRS1 label Dec 24, 2016

GordonTheTurtle removed the rebuild/windowsRS1 label Dec 24, 2016

tonistiigi merged commit a9fa38b into moby:master Dec 24, 2016

albers mentioned this pull request Dec 25, 2016

Add bash completion for plugin install --alias #29690

Merged

tonistiigi mentioned this pull request Dec 27, 2016

[v1.13] plugins updates #29734

Merged

tonistiigi added the process/cherry-picked label Dec 28, 2016

vieux removed the process/cherry-pick label Jan 3, 2017

tonistiigi mentioned this pull request Jan 5, 2017

Set plugins used in tests back to original names #29923

Merged

sdurrheimer mentioned this pull request Jan 19, 2017

Add zsh completion for 'docker plugin install --alias' #30268

Merged

Plugins content addressability #29487

Plugins content addressability #29487

Conversation

tonistiigi commented Dec 16, 2016 • edited Loading

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

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

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

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

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 19, 2016 • edited Loading

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

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 commented Dec 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Dec 23, 2016

tonistiigi commented Dec 16, 2016 •

edited

Loading

anusha-ragunathan Dec 19, 2016 •

edited

Loading