Plugable secret backend by liron-l · Pull Request #348 · docker/cli

liron-l · 2017-07-18T09:55:12Z

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

There is a bug in serialization of the secret data. Handled in moby/moby#34157.

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

cpuguy83 · 2017-07-18T12:53:16Z

Validations and tests appear to have failed but I can't see them in CircleCI.

liron-l · 2017-07-18T13:02:59Z

Thanks @cpuguy83, waiting for the fix in moby/moby#34157 (tests should work after updating the vendor folder).

codecov-io · 2017-07-18T16:30:30Z

Codecov Report

Merging #348 into master will increase coverage by 1.86%.
The diff coverage is 48.97%.

@@            Coverage Diff             @@
##           master     #348      +/-   ##
==========================================
+ Coverage   45.49%   47.35%   +1.86%     
==========================================
  Files         193      186       -7     
  Lines       16065    15355     -710     
==========================================
- Hits         7308     7271      -37     
+ Misses       8379     7707     -672     
+ Partials      378      377       -1

dnephin

Thanks!

We'll also need to add this to the Compose file, but we need to get v3.4 in first, which is in #306, so a follow up PR should be fine.

dnephin · 2017-07-18T16:32:06Z

cli/command/formatter/secret.go

 {{- end }}{{ end }}
+Driver:              	{{.Driver}}
 Created at:            	{{.CreatedAt}}
 Updated at:            	{{.UpdatedAt}}`


It looks like the existing lines have a weird mix of tabs and spaces...

I think we'll need to fix this to be all spaces so that the new line aligns properly.

Thanks, modified 👍

dnephin · 2017-07-18T16:35:05Z

cli/command/secret/create.go

 		Use:   "create [OPTIONS] SECRET file|-",
 		Short: "Create a secret from a file or STDIN as content",
-		Args:  cli.ExactArgs(2),
+		Args:  cli.RequiresRangeArgs(1, 2),


The Use: line needs to be updated to make the second arg optional:

create [OPTIONS] SECRET [file | -]

Good catch, thanks 👍

dnephin · 2017-07-18T16:37:51Z

cli/command/secret/create.go

-	if err != nil {
-		return errors.Errorf("Error reading content from %q: %v", options.file, err)
+	var secretData []byte
+	if options.driver == "" {


The intent here will be more obvious is this check is changed to

if options.file != "" { ... }

Fixed as part of function refactoring 👍

dnephin · 2017-07-18T16:41:06Z

cli/command/secret/create.go

+		secretData, err = ioutil.ReadAll(in)
+		if err != nil {
+			return errors.Errorf("Error reading content from %q: %v", options.file, err)
+		}


I know you've just adding another condition here, but now that this function is growing I think it would be appropriate to extract this as a new function.

secretData, err := readSecretData(options.file, dockerCli.In()) ...

Even the initial options.file != "" check could be inside the function.

thanks @dnephin, makes sense.

dnephin · 2017-07-18T18:24:42Z

cli/command/formatter/secret.go

 {{- end }}{{ end }}
+Driver:                 {{.Driver}}
 Created at:            	{{.CreatedAt}}
 Updated at:            	{{.UpdatedAt}}`


I still see tabs here

Got it, you mean I should remove all tabs from the formatter 👍

dnephin · 2017-07-18T18:26:20Z

cli/command/secret/create.go

+		}
+		defer in.Close()
+	}
+	var err error


this line is unused I think

right. Thanks

dnephin · 2017-07-18T18:27:54Z

cli/command/secret/create.go

+	var err error
+	data, err := ioutil.ReadAll(in)
+	if err != nil {
+		return nil, errors.Errorf("Error reading content from %q: %v", file, err)


errors.Wrapf(err, "error reading content from %q", file)

Errors should start with lowercase so they look more appropriate when wrapped.

Thanks, but those lines are not wrapped with anything (they are printed directly in the cli). I returned the error and wrapped with upper case in the main method. Please let me know if this makes sense.

liron-l · 2017-07-18T19:56:32Z

Thanks for the comments @dnephin! do you know what vndr is complaining?
CC @diogomonica @aaronlehmann @cpuguy83

dnephin · 2017-07-18T20:10:16Z

If I run make -f docker.Makefile vendor locally I see the same problem. I think you vendored using an older version of vndr. If you run that command and check in the changes it should fix it.

liron-l · 2017-07-19T08:30:35Z

Thanks @dnephin, it worked!

dnephin

LGTM

cpuguy83 · 2017-07-19T15:24:22Z

Can we do the vendoring in a separate commit?

liron-l · 2017-07-19T15:34:43Z

Sure @cpuguy83 makes sense. I've updated the commit.

thaJeztah · 2017-07-19T15:53:54Z

Does this also update the docker-compose format to support this?

thaJeztah · 2017-07-19T16:04:08Z

I wonder if it would make sense to do the version bump in two stages; one to moby/moby@d0a8e73 (last commit before the upstream related change was merged), and one to moby/moby@0304c98 (the diff is quite large moby/moby@87df0e5...0304c98)

I see there's also some dependencies bumped in moby's vendor.conf; do we have to bump those as well accordingly (if used here?)

liron-l · 2017-07-19T17:29:26Z

Thanks @thaJeztah.
Re docker-compose, I checked that setting external: true for the secret reference works with the new plugin subsystem.
Do we need to support driver: name parameter, which will create the secret value?

Re version bump, let me know which commit history makes sense or which additional packages should be updated.

liron-l · 2017-07-19T18:42:35Z

@thaJeztah and @dnephin I've added a new parameter to secrets definitions:

driver: my-driver

So now it's possible to run:

version: '3.3'
services:
  web:
    image: nginx
    secrets:                    
     - hello-secret
secrets:                       
  hello-secret:
    driver: secrets:next

Can you please review?

dnephin · 2017-07-19T18:54:52Z

Sorry, the change should not be made to v3.3. It should go into v3.4. You can cherry-pick #358 to get v3.4, or we can add the compose changes in a separate PR. I think I would prefer a separate PR

liron-l · 2017-07-19T19:09:36Z

Thanks @dnephin makes sense. I will push those changes in the next commit.
I removed the compose changes, how do I re-trigger a build?

dnephin · 2017-07-19T19:26:41Z

hmm, we've been having problems with github not triggering builds. I'm going to email github support.

liron-l · 2017-07-20T16:39:50Z

Thanks @dnephin! was the issue resolved? can I trigger A build without closing the issue?

dnephin · 2017-07-20T18:19:35Z

github said they are looking into it, and that it's likely a bug on their end.

I've been able to trigger the build by closing and re-opening the PR in some cases, so I'm going to try that now.

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

and in some cases CI fails with "fatal: Could not parse object" which is what's happening this time...

Update vendor to support plugable secret backend Signed-off-by: Liron Levin <liron@twistlock.com>

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>

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

liron-l force-pushed the plugable_secrets_backend branch 3 times, most recently from 40e2d6b to 0f89fe6 Compare July 18, 2017 10:24

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from a248bac to 17c3a85 Compare July 18, 2017 16:30

liron-l force-pushed the plugable_secrets_backend branch from 17c3a85 to 08d9020 Compare July 18, 2017 16:42

dnephin reviewed Jul 18, 2017

View reviewed changes

dnephin added status/2-code-review and removed status/0-triage labels Jul 18, 2017

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from da74b0e to 780465b Compare July 18, 2017 17:04

dnephin reviewed Jul 18, 2017

View reviewed changes

liron-l force-pushed the plugable_secrets_backend branch from 780465b to 2ad48c9 Compare July 18, 2017 19:43

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from 96fa77c to d5f1104 Compare July 18, 2017 21:10

liron-l requested a review from vdemeester as a code owner July 19, 2017 06:10

liron-l force-pushed the plugable_secrets_backend branch from d5f1104 to 67cbc67 Compare July 19, 2017 06:10

dnephin approved these changes Jul 19, 2017

View reviewed changes

liron-l force-pushed the plugable_secrets_backend branch from 67cbc67 to e34fae3 Compare July 19, 2017 15:32

liron-l force-pushed the plugable_secrets_backend branch from e34fae3 to b8f39b7 Compare July 19, 2017 18:39

liron-l force-pushed the plugable_secrets_backend branch from b8f39b7 to bb96a8f Compare July 19, 2017 19:07

liron-l force-pushed the plugable_secrets_backend branch 2 times, most recently from 3e153c4 to 389f146 Compare July 20, 2017 15:07

dnephin closed this Jul 20, 2017

dnephin reopened this Jul 20, 2017

liron-l force-pushed the plugable_secrets_backend branch from 389f146 to 87bd71e Compare July 20, 2017 18:23

liron-l closed this Jul 20, 2017

liron-l reopened this Jul 20, 2017

Liron Levin added 2 commits July 20, 2017 21:26

Update vendor

2d7567d

Update vendor to support plugable secret backend Signed-off-by: Liron Levin <liron@twistlock.com>

liron-l force-pushed the plugable_secrets_backend branch from 87bd71e to 343de92 Compare July 20, 2017 18:26

liron-l closed this Jul 20, 2017

liron-l mentioned this pull request Jul 20, 2017

Plugable secrets backend #366

Merged

Comments

Conversation

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

Uh oh!

Uh oh!

cpuguy83 commented Jul 18, 2017

Uh oh!

liron-l commented Jul 18, 2017

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liron-l Jul 18, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liron-l Jul 18, 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 commented Jul 18, 2017

Uh oh!

dnephin commented Jul 18, 2017

Uh oh!

liron-l commented Jul 19, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jul 19, 2017

Uh oh!

liron-l commented Jul 19, 2017

Uh oh!

thaJeztah commented Jul 19, 2017

Uh oh!

thaJeztah commented Jul 19, 2017

Uh oh!

liron-l commented Jul 19, 2017

Uh oh!

liron-l commented Jul 19, 2017

Uh oh!

dnephin commented Jul 19, 2017

Uh oh!

liron-l commented Jul 19, 2017

Uh oh!

dnephin commented Jul 19, 2017

Uh oh!

liron-l commented Jul 20, 2017

Uh oh!

dnephin commented Jul 20, 2017

Uh oh!

dnephin commented Jul 20, 2017

liron-l commented Jul 18, 2017 •

edited

Loading

codecov-io commented Jul 18, 2017 •

edited

Loading

liron-l Jul 18, 2017 •

edited

Loading

liron-l Jul 18, 2017 •

edited

Loading