Support using swarm Configs as CredentialSpecs in Services. by dperny · Pull Request #1781 · docker/cli

dperny · 2019-03-27T20:49:59Z

- What I did
Builds on and obsoletes #1656. Uses the final API that went into the engine.

Additionally, in the process, discovered and fixed a bug where the --credential-spec flag would not have been respected on service updates.

- How I did it

When handling configs, add additional logic for handling runtime configs. Additionally, adds logic to replace the config name which the user passes with the config ID, which is what the engine API needs.

- How to verify it

Includes unit tests.

- Description for the changelog

Add ability to use swarm Configs as CredentialSpecs on Services.

codecov-io · 2019-03-28T19:06:57Z

Codecov Report

Merging #1781 into master will increase coverage by 0.4%.
The diff coverage is 91.17%.

@@ Coverage Diff @@## master #1781 +/- ##
=========================================
+ Coverage 56.33% 56.73% +0.4% 
=========================================
Files 308 308 Lines 21504 21610 +106 =========================================
+ Hits 12114 12261 +147 + Misses 8498 8449 -49 - Partials 892 900 +8

dperny · 2019-03-28T19:07:21Z

Added tests, removed WIP.

dperny · 2019-03-28T19:39:01Z

Refactored to reduce cyclomatic complexity, which should hopefully be enough to pass lint.

dperny · 2019-04-01T18:38:23Z

Added compose support.

thaJeztah · 2019-04-03T14:23:32Z

+	// file in the container. This is used for supporting CredentialSpec
+	// configs.
+	if config.Runtime {
+ if config.Target != "" || config.UID != "" || config.GID != "" || config.Mode != nil {


Wouldn't target and uid/gid still be configurable? (i.e., mount a config as credential-spec, but specify which uid/gid and/or path to use?

No, because a credentialspec is never mounted into the container, or written to the filesystem. And in any case, the actual API type (which is a ConfigReference with a ConfigReferenceRuntimeTarget) does not allow setting file attributes.

🤦‍♂️ ah, yes, makes sense

thaJeztah · 2019-04-03T14:24:53Z

+	// the service. Specifically, this currently only indicates that the Config
+	// is used as a CredentialSpec. If this field is set `true`, then all other
+	// fields besides `Source` must be empty.
+	Runtime bool `yaml:",omitempty" json:"runtime,omitempty"`


Can we deduct this from the Source value? (IIUC, it will be prefixed with config:// ?)

No. The string config:// should never actually be in the compose code, I think.
We could theoretically determine that a config is a Runtime config by looking for the absence of all other fields, but I usually think it's better to affirmatively determine something rather than make assumptions from the absence.
It would be cleaner to have a whole new type besides FileReferenceConfig but doing so would be more trouble than it's worth, IMHO.

Would it make sense to use a string instead of a bool?
I'd like to avoid a situation where we are adding multiple bools here.

A string representing what? The target, file vs runtime?

One thing to take note is that this type (FileReferenceConfig) is used both for configs and secrets, but we'll only use the "runtime" option for configs; so we might have to create a separate type for configs?
Thinking (UX-wise); in order to use a config as credential-spec, I would have to;
define the Config
version: "3.8"configs: my_credentials_spec: file: ./credentials-spec-file.json
(or, if the config already exists):
version: "3.8"configs: my_credentials_spec: external: true
add the Config to the Configs section of the compose file (I guess this can only be with the long syntax, because I'd have to specify the runtime option);
version: "3.8"services: myservice: image: myimage:latestconfigs: - source: my_credentials_specruntime: trueconfigs: my_credentials_spec: file: ./credentials-spec-file.json
configure the service to use the config as source for the credentials-spec;
version: "3.8"services: myservice: image: myimage:latestcredential_spec: config: my_credentials_specconfigs: - source: my_credentials_specruntime: trueconfigs: my_credentials_spec: file: ./my-credential-spec.json
That's a lot of duplication; can we somehow skip step 2. ? i.e., if credential_spec uses config: <some name>, we already know that we want to use a config as source. That config must be defined in the compose-file, but we could attach it automatically to the service (if needed) (or can we do that step even "behind the scenes", daemon-side?)

Sure, why not.

andrewhsu · 2019-04-03T17:30:18Z

I had a quick look, but I'm not as familiar with this codebase. @cpuguy83 is this patch also something you could have a look at if you have time?

cpuguy83 · 2019-04-08T18:23:10Z

+	// all configs in spec.Configs should have both a Name and ID, because
+	// they come from an already-accepted spec.
+	for _, config := range spec.Configs {
+ // if the config is a Runtime target, make sure it's still in use right


Is this a bug waiting to happen? Is there something we can do now to prevent someone from overlooking this in the future?

sort of. any future use for runtime specs will probably require similar alterations to the configs logic, which means the implementer will have to look here anyway. then, they'll see this comment, and add the correct removal code.

cpuguy83 · 2019-04-08T18:25:45Z

- File: &file,
- ConfigName: obj.Name,
- })
+ // if the obj.File is identical to the zero-value of


As above, is this a bug waiting to happen?

Possibly, but the thing is I'm not sure under what case we'll add another non-file ConfigReference type, so I can't really code defensively against it, and even if I do, it's unlikely that something else will even be implemented at any point.

Off the top of my head, a value explicitly for runtime targets.

Yes, it would make sense lol
I've made this change, and added tests to boot.

dperny · 2019-04-11T18:28:04Z

Altered the compose code to infer the need for a Config by the existence of a value in the CredentialSpec.Config field, meaning there is no need to define a Runtime config in the service's configs; merely specifying a CredentialSpec with a Config is sufficient.

This means a service using a CredentialSpec Config would look like this:

version: "3.8"services:
myservice:
image: myimage:latestcredential_spec:
config: my_credentials_specconfigs:
my_credentials_spec:
file: ./my-credential-spec.json|

whereas before this change, it would have looked like this:

version: "3.8"services:
myservice:
image: myimage:latestcredential_spec:
config: my_credentials_specconfigs:
- source: my_credentials_specruntime: trueconfigs:
my_credentials_spec:
file: ./my-credential-spec.json

thaJeztah · 2019-04-11T19:27:49Z

Thanks! That looks a lot more convenient (and less confusing) 🤗
Also sorry for the long delay between review/comments. Lots of plates to keep spinning 😓

I'll have another look at the code shortly @cpuguy83 @vdemeester if you have some time 🙏

thaJeztah

some comments/questions, but I think it looks pretty sane now

thaJeztah · 2019-04-11T21:02:07Z

+	// if we're using a swarm Config for the credential spec, over-write it
+	// here with the config ID
+	if swarmCredSpec.Config != "" {
+ for _, config := range refs {


Was wondering if we should have a resolveConfig / resolveConfigID function somewhere, but (I think) all the lookup loops are just slightly different

thaJeztah

LGTM

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Updates the CredentialSpec handling code for services to allow using swarm Configs. Additionally, fixes a bug where the `--credential-spec` flag would not be respected on service updates. Signed-off-by: Drew Erny <drew.erny@docker.com>

Adds tests for setting and updating swarm service CredentialSpecs, especially when using a Config as a credential spec. Signed-off-by: Drew Erny <drew.erny@docker.com>

Signed-off-by: Drew Erny <drew.erny@docker.com>

cpuguy83

LGTM

dperny · 2019-04-12T17:05:47Z

i went and googled "cat in a pope hat" to give this extra powerful blessings

GordonTheTurtle added the status/0-triage label Mar 27, 2019

dperny changed the title ~~[WIP] Support using swarm Configs as CredentialSpecs in Services.~~Support using swarm Configs as CredentialSpecs in Services.Mar 28, 2019

dperny force-pushed the swarm-credentialspec branch from 007e491 to 23b54b8Compare March 28, 2019 19:36

dperny force-pushed the swarm-credentialspec branch from 56ea44b to 999fab0Compare April 1, 2019 18:53

thaJeztah added status/2-code-review and removed status/0-triage labels Apr 3, 2019

thaJeztah reviewed Apr 3, 2019
View reviewed changes

thaJeztah mentioned this pull request Apr 3, 2019
Add driver field to top-level secret object #1783
Merged

cpuguy83 reviewed Apr 8, 2019
View reviewed changes

dperny force-pushed the swarm-credentialspec branch 3 times, most recently from 6bdd646 to 34ebae4Compare April 11, 2019 18:26

dperny force-pushed the swarm-credentialspec branch from 34ebae4 to 074a96cCompare April 11, 2019 18:46

thaJeztah requested changes Apr 11, 2019
View reviewed changes

thaJeztah added impact/changelog impact/compose-file impact/documentation labels Apr 12, 2019

thaJeztah approved these changes Apr 12, 2019
View reviewed changes

thaJeztahand others added 4 commits April 12, 2019 11:17

Add support for using Configs as CredentialSpecs in services
6511da8
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Update CredentialSpec code to allow using configs
01f4f2e
Updates the CredentialSpec handling code for services to allow using swarm Configs. Additionally, fixes a bug where the `--credential-spec` flag would not be respected on service updates. Signed-off-by: Drew Erny <drew.erny@docker.com>

Add CredentialSpec tests
4cacd13
Adds tests for setting and updating swarm service CredentialSpecs, especially when using a Config as a credential spec. Signed-off-by: Drew Erny <drew.erny@docker.com>

add support for config credentialspecs to compose
42ec51e
Signed-off-by: Drew Erny <drew.erny@docker.com>

dperny force-pushed the swarm-credentialspec branch from 074a96c to 42ec51eCompare April 12, 2019 16:17

cpuguy83 approved these changes Apr 12, 2019
View reviewed changes

cpuguy83 merged commit 58ec72a into docker:masterApr 12, 2019

GordonTheTurtle added this to the 19.03.0 milestone Apr 12, 2019

jorisscheppers mentioned this pull request Sep 26, 2019
Credentialspec from Swarm config not working #2109
Open

Uh oh!

Conversation

dperny commented Mar 27, 2019• edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Mar 28, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dperny commented Mar 28, 2019

Uh oh!

dperny commented Mar 28, 2019

Uh oh!

dperny commented Apr 1, 2019

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!

thaJeztahApr 11, 2019• 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!

andrewhsu commented Apr 3, 2019

Uh oh!

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!

dperny commented Apr 11, 2019

Uh oh!

thaJeztah commented Apr 11, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

dperny commented Apr 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dperny commented Mar 27, 2019•
edited by thaJeztah
Loading

codecov-io commented Mar 28, 2019•
edited
Loading

thaJeztahApr 11, 2019•
edited
Loading

thaJeztah commented Apr 11, 2019•
edited
Loading