Plugable secrets backend by liron-l · Pull Request #366 · docker/cli

liron-l · 2017-07-20T18:27:22Z

This commit extends SwarmKit secret management with pluggable secret
backends support.
Following previous commits:

Added driver parameter to docker secret command.
Specifically:

docker secret create [secret_name] --driver [driver_name]
Displaying the driver in

    $ docker secret ls
    $ docker secret inspect [secret_name]
    $ docker secret inspect [secret_name] -pretty

Signed-off-by: Liron Levin liron@twistlock.com

codecov-io · 2017-07-20T18:32:02Z

Codecov Report

Merging #366 into master will increase coverage by 0.04%.
The diff coverage is 72.09%.

@@            Coverage Diff             @@
##           master     #366      +/-   ##
==========================================
+ Coverage   49.01%   49.06%   +0.04%     
==========================================
  Files         199      199              
  Lines       16392    16422      +30     
==========================================
+ Hits         8035     8057      +22     
- Misses       7939     7946       +7     
- Partials      418      419       +1

liron-l · 2017-07-20T18:32:27Z

@dnephin @thaJeztah I closed and re-opend #348 since the tests did not get triggered on push.
I hope it's OK if we continue the discussion here.

dnephin

LGTM

liron-l · 2017-07-20T18:43:14Z

@dnephin thanks! I think that the commit approval is what's causing the builds to stop.

dnephin · 2017-07-20T18:46:32Z

Hmm, I don't know about that. We approve plenty of PRs that are still able to build afterward.

I think it's something about the files/changes. On my PR which had this issue (#107) I also retried recreating it, and after the first build it also stopped building again, and the new PR doesn't have any approvals.

liron-l · 2017-07-20T18:52:14Z

Makes sense @dnephin. Let me know what further steps are required to complete this PR.

dnephin · 2017-07-20T19:04:04Z

Once someone else gives their approval we can run the tests manually and merge. Let's not worry about the github bug for now

liron-l · 2017-07-20T19:04:24Z

Great @dnephin thanks!

liron-l · 2017-07-25T13:45:15Z

@thaJeztah PTAL, I separated vendor.conf bump to another commit.
Let me know if I need to bump additional packages. Thanks!

liron-l · 2017-07-30T11:40:37Z

@thaJeztah @cpuguy83 please take a look, I've updated the docker vendor dependencies again. Thanks!

cpuguy83

LGTM

vdemeester

LGTM 🐸
/cc @thaJeztah needs some docs update ? 👼

liron-l · 2017-08-09T15:47:07Z

@thaJeztah PTAL

liron-l · 2017-08-13T06:25:26Z

@vdemeester, @cpuguy83, @diogomonica, @thaJeztah, should I add additional changes to this commit?

thaJeztah · 2017-08-14T08:33:23Z

cli/command/secret/create.go

 	}
 	flags := cmd.Flags()
 	flags.VarP(&options.labels, "label", "l", "Secret labels")
+	flags.StringVar(&options.driver, "driver", "", "Secret driver")


This flag needs to have a version annotation, so that it is hidden / produces an error on older API versions

Also, to be consistent with docker network create, docker volume create, in this case I would be +1 to add a -d shorthand as well

Thanks @thaJeztah I set the version annotation to 1.31, hope this makes sense.

thaJeztah · 2017-08-14T08:53:34Z

cli/command/secret/create.go

-		in = file
-		defer file.Close()
+	if options.driver != "" && options.file != "" {
+		return errors.Errorf("When using secret driver secret data must be empty")


Could this be handled by the (external) driver? IIUC, with this change the secret's value is taken from the external store, so

$ docker secret create --driver external mysecret

Doesn't really "create" a secret, but "associates" the external secret with a secret in Docker.

Do we need to keep the option open to have external secret stores create new secrets from the Docker CLI? i.e.

$ echo "new secret" | docker secret create --driver external mysecret -

If so, this validation should be done by the driver (just like currently checking for empty values is done in the backend)

$ docker secret create foo Error response from daemon: rpc error: code = InvalidArgument desc = secret data must be larger than 0 and less than 512000 bytes

@thaJeztah according to the secrets plugin subsystem design, the secrets plugin will be readonly (that is, swarm will not populate the secret values).
I think it might be better to prevent incorrect usage as soon as possible (since file is a cli only option).

liron-l · 2017-08-14T10:03:44Z

Thanks @thaJeztah, I've updated the review according to your comments.

Set minimum version for --secret parameter (1.31)
Kept the CLI validation, according to the secret plugin design, we will never populate the secrets from the cli. In the context of plugins, the creation means that we create a real secret in swarmkit (secret name and metadata) and only the value is fetched from the plugin.

liron-l · 2017-08-18T19:27:17Z

@thaJeztah @diogomonica I've updated the code, please take a look if the new secrets driver API makes sense.

liron-l · 2017-08-21T19:42:43Z

@diogomonica @thaJeztah PTAL.

liron-l · 2017-08-27T20:12:00Z

@thaJeztah PTAL, let me know if additional changes are required.

liron-l · 2017-09-03T20:38:49Z

@diogomonica @thaJeztah PTAL, all other dependencies related to this feature are completed. Thanks.

This commit extends SwarmKit secret management with pluggable secret backends support. Following previous commits: 1. moby/swarmkit@eebac27 2. moby/moby@08f7cf0 Added driver parameter to `docker secret` command. Specifically: 1. `docker secret create [secret_name] --driver [driver_name]` 2. Displaying the driver in ``` $ docker secret ls $ docker secret inspect [secret_name] $ docker secret inspect [secret_name] -pretty ``` Signed-off-by: Liron Levin <liron@twistlock.com>

diogomonica · 2017-09-14T18:58:51Z

This LGTM, but I don't have merging rights.

liron-l · 2017-09-15T10:53:58Z

@vdemeester @dnephin @thaJeztah can somebody please merge?

thaJeztah

alright let's get this merged

LGTM 👍

liron-l requested review from dnephin and vdemeester as code owners July 20, 2017 18:27

GordonTheTurtle added the status/0-triage label Jul 20, 2017

dnephin approved these changes Jul 20, 2017

View reviewed changes

liron-l force-pushed the plugable_secrets_backend branch from 343de92 to 64bfcb7 Compare July 20, 2017 18:41

liron-l force-pushed the plugable_secrets_backend branch from 64bfcb7 to 27fcb6d Compare July 25, 2017 13:29

liron-l mentioned this pull request Jul 25, 2017

Add API documentation for plugable secret backends moby/moby#34202

Merged

liron-l force-pushed the plugable_secrets_backend branch from 27fcb6d to d8176e9 Compare July 25, 2017 13:36

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from 8c158ce to 4064118 Compare July 30, 2017 11:33

cpuguy83 added status/2-code-review and removed status/0-triage labels Jul 30, 2017

cpuguy83 approved these changes Jul 30, 2017

View reviewed changes

vdemeester approved these changes Aug 1, 2017

View reviewed changes

thaJeztah reviewed Aug 14, 2017

View reviewed changes

thaJeztah requested changes Aug 14, 2017

View reviewed changes

liron-l force-pushed the plugable_secrets_backend branch from 4064118 to 66ac875 Compare August 14, 2017 10:00

liron-l force-pushed the plugable_secrets_backend branch from 66ac875 to 7133af1 Compare August 14, 2017 10:07

liron-l force-pushed the plugable_secrets_backend branch 4 times, most recently from 3bf9d64 to 74c55de Compare August 18, 2017 19:23

liron-l force-pushed the plugable_secrets_backend branch 3 times, most recently from 1920e7e to 93292ff Compare August 27, 2017 20:05

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from bc8f58d to 736f0a4 Compare September 3, 2017 20:32

liron-l force-pushed the plugable_secrets_backend branch from 736f0a4 to ef47835 Compare September 7, 2017 08:05

liron-l force-pushed the plugable_secrets_backend branch from ef47835 to 0ee9e05 Compare September 7, 2017 08:10

thaJeztah approved these changes Sep 18, 2017

View reviewed changes

thaJeztah merged commit 3edf97e into docker:master Sep 18, 2017

GordonTheTurtle added this to the 17.10.0 milestone Sep 18, 2017

thaJeztah added the impact/changelog label Sep 18, 2017

albers mentioned this pull request Oct 11, 2017

Add bash completion for secret create --driver #618

Merged

kinghuang mentioned this pull request Apr 4, 2018

Feature Request: Docker secrets plugin hashicorp/vault#4273

Open

Comments

Conversation

liron-l commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

liron-l commented Jul 20, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

liron-l commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liron-l commented Jul 20, 2017

Uh oh!

dnephin commented Jul 20, 2017

Uh oh!

liron-l commented Jul 20, 2017

Uh oh!

liron-l commented Jul 25, 2017

Uh oh!

liron-l commented Jul 30, 2017

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

liron-l commented Aug 9, 2017

Uh oh!

liron-l commented Aug 13, 2017

Uh oh!

thaJeztah Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

liron-l Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liron-l Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

liron-l commented Aug 14, 2017

Uh oh!

liron-l commented Aug 18, 2017

Uh oh!

liron-l commented Aug 21, 2017

Uh oh!

liron-l commented Aug 27, 2017

Uh oh!

liron-l commented Sep 3, 2017

Uh oh!

diogomonica commented Sep 14, 2017

Uh oh!

liron-l commented Sep 15, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

liron-l commented Jul 20, 2017 •

edited

Loading

codecov-io commented Jul 20, 2017 •

edited

Loading

liron-l commented Jul 20, 2017 •

edited

Loading

dnephin commented Jul 20, 2017 •

edited

Loading

thaJeztah Aug 14, 2017 •

edited

Loading