Add "name" field to secrets and configs to allow interpolation in Compose files by ilyasotkov · Pull Request #668 · docker/cli

ilyasotkov · 2017-11-06T15:44:01Z

See issue #667

In Compose file version 3.4, volumes gained the "name" field which allowed the volumes' names to be interpolated for simple version bumps using environmental variables.

That feature is also needed for secrets and configs. Proposed usage:

version: "3.5" # future Compose file version

secrets:
  my-secret:
    name: my-secret-${VERSION} # name of the Docker Secret with variable interpolation
    file: ./secret.html

configs:
  my-config:
    name: my-config-${VERSION} # same thing for configs
    file: ./default.conf

services:
  web:
    image: nginx:1.13.6-alpine
    ports:
      - "80:80"
    secrets:
      - source: my-secret # reference by the name of the key, consistent with how it is for volumes now in 3.4
        target: /usr/share/nginx/html/index.html
    configs:
      - source: my-config
        target: /etc/nginx/conf.d/default.conf

What I did: I just followed the way the feature is already implemented for volumes. I also bumped the Compose file schema version to 3.5.

codecov-io · 2017-11-06T15:51:48Z

Codecov Report

Merging #668 into master will increase coverage by 0.06%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master     #668      +/-   ##
==========================================
+ Coverage   51.28%   51.34%   +0.06%     
==========================================
  Files         216      216              
  Lines       17743    17761      +18     
==========================================
+ Hits         9099     9120      +21     
+ Misses       8175     8172       -3     
  Partials      469      469

dnephin

Thanks!

Left some comments. I'm going to open a PR to fix some existing issues with the volume implementation of this. I'll also look at see if we can remove some duplication between secrets/configs.

dnephin · 2017-11-06T17:33:56Z

 	for name, secret := range secrets {
+		if secret.Name != "" {
+			name = secret.Name
+		}


This is not used if secret.External.External evaluates to true, so I think this block should move after the next if secret.External.External block.

Also, this name should not be scoped using namespace.Scope(name) (which is done below). So there needs to be an else for doing the namespace.Scope()

Same for configs below (I wish these two types shared more code...)

dnephin · 2017-11-06T17:53:27Z

+				secret.External.Name = name
+				secrets[name] = secret
+			} else {
+				logrus.Warnf("secret %s: secret.external.name is deprecated in favor of secret.name", name)


There's an issue open for this warning with volumes #608

This has the same problem, we'll need to pass in a version here so that we only warn if version >= 3.5

dnephin · 2017-11-06T17:56:29Z


+		if secretSpec.Name != "" {
+			source = namespace.Scope(secretSpec.Name)
+		}


If the name is specified it shouldn't be scoped.

I feel like all internal / stack objects should be scoped. It would be more consistent if everything that gets created via docker stack deploy (or wiped via docker stack rm) is namespaced. That would break compatibility with how it's already implemented with volumes though 🤔

ya, I think it's by design that the name field is not scoped. Only the default names get scoped.

You can use a variable in the name: field to provide the scope to these explicitly named objects.

dnephin · 2017-11-06T22:08:55Z

Opened #670 to fix the warnings on volume. For LoadSecrets and LoadConfigObjs you could pass in the configDetails since they already accept a field from that struct.

#670 also made a change so that external.name is never read

dnephin · 2017-11-06T22:59:42Z

I opened #671 to remove a lot of the duplication between secrets and configs. This PR is going to conflict quite a bit. You might want to rebase on top of #671 so that you don't want to make all the changes twice (once for secrets, and once for configs).

ilyasotkov · 2017-11-10T14:06:59Z

So I rewrote the PR on top of #671 to remove some of the duplication between configs / secrets and fixed some issues the original PR had as well.

I also added a couple of tests for the functionality, but I wasn't sure if duplicating secrets and configs there is the right way to go.

dnephin

looks good, couple more small things.

Also looks like there is a lint and validate failure

dnephin · 2017-11-10T21:31:10Z

+			name = secret.Name
+		} else {
+			name = namespace.Scope(name)
+		}


This if/else block can be moved into fileObjectConfig() to remove the duplication.

dnephin · 2017-11-10T21:43:02Z

+		// if not "external: true"
+	} else {
+		if obj.File == "" {
+			return obj, errors.Errorf("%[1]s %[2]s: specify a file or \"external: true\"", objType, name)


Is this a new error?

It is, although I definitely should've written a test on it 🙃
It doesn't work,secrets.<secret-name> must be a mapping is being called before this one. I'll remove it for now, but it's probably a good idea to make all such errors a bit more specific in the future.

ilyasotkov · 2017-11-11T05:44:36Z

Updated.

dnephin

LGTM

Thanks!

ilyasotkov · 2017-11-22T05:53:25Z

So, what else needs to be done to get this merged? 👋

vdemeester

LGTM 🐯
@ilyasotkov needs a rebase though

Signed-off-by: Ilya Sotkov <[email protected]>

…emove extraneous error call, regenerate schema correctly Signed-off-by: Ilya Sotkov <[email protected]>

ilyasotkov · 2017-11-22T11:46:16Z

Rebased ✅

YuryHrytsuk · 2025-01-28T10:08:49Z

Question:

Why name field of secrets is not documented anywhere below ?

ilyasotkov requested review from dnephin and vdemeester as code owners November 6, 2017 15:44

GordonTheTurtle added dco/no status/0-triage and removed dco/no labels Nov 6, 2017

ilyasotkov mentioned this pull request Nov 6, 2017

Add "name" field to secrets and configs to allow interpolation in Compose files #667

Closed

ilyasotkov changed the title ~~Add "name" field to secrets and configs to allow interpolation in Compose files.~~ Add "name" field to secrets and configs to allow interpolation in Compose files Nov 6, 2017

docker deleted a comment from GordonTheTurtle Nov 6, 2017

dnephin reviewed Nov 6, 2017

View reviewed changes

dnephin added status/2-code-review and removed status/0-triage labels Nov 6, 2017

GordonTheTurtle added dco/no and removed dco/no labels Nov 10, 2017

dnephin reviewed Nov 10, 2017

View reviewed changes

ilyasotkov mentioned this pull request Nov 11, 2017

Are we forgetting docker secret update command in cli? moby/moby#29882

Closed

docker deleted a comment from GordonTheTurtle Nov 13, 2017

dnephin approved these changes Nov 13, 2017

View reviewed changes

vdemeester approved these changes Nov 22, 2017

View reviewed changes

ilyasotkov added 3 commits November 22, 2017 13:18

Add secret.name and config.name in compose.

4f7f3d2

Signed-off-by: Ilya Sotkov <[email protected]>

Add tests for secret.name in compose files.

1f10b31

Signed-off-by: Ilya Sotkov <[email protected]>

Remove duplication for secrets / configs, combine 3.5 loader tests, r…

ec47003

…emove extraneous error call, regenerate schema correctly Signed-off-by: Ilya Sotkov <[email protected]>

thaJeztah added impact/changelog impact/compose-file labels Nov 22, 2017

thaJeztah added the impact/documentation label Nov 22, 2017

dnephin merged commit 8d41ba0 into docker:master Nov 22, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 22, 2017

dnephin mentioned this pull request Nov 22, 2017

Support network.name in the compose schema #711

Merged

thaJeztah mentioned this pull request Jan 23, 2018

Docker Compose Variable substitution - can't use a variable in a yaml/compose key moby/moby#33540

Closed

thaJeztah mentioned this pull request Mar 3, 2018

Secret names should be able to include variables moby/moby#35229

Closed

BretFisher mentioned this pull request Dec 3, 2018

Update data for config and secrets moby/moby#35803

Open

YuryHrytsuk mentioned this pull request Jan 28, 2025

[DOCUMENTATION] Why name field of secret is not documented? docker/compose#12504

Closed

Conversation

ilyasotkov commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Nov 6, 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!

dnephin commented Nov 6, 2017

Uh oh!

dnephin commented Nov 6, 2017

Uh oh!

ilyasotkov commented Nov 10, 2017

Uh oh!

dnephin left a comment • 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!

ilyasotkov commented Nov 11, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

ilyasotkov commented Nov 22, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

ilyasotkov commented Nov 22, 2017

Uh oh!

YuryHrytsuk commented Jan 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ilyasotkov commented Nov 6, 2017 •

edited

Loading

codecov-io commented Nov 6, 2017 •

edited

Loading

dnephin left a comment •

edited

Loading