Support templated secrets and configs #2133

aaronlehmann · 2017-04-21T23:23:55Z

Add the ability for secrets and configs to expand templates, which can
reference the following fields:

The same set of fields available to ContainerSpec templates
({{.Service.Name}}, {{.Node.ID}}, etc)
Environment variables: {{Env "envvar"}}
Other secrets: {{Secret "sometarget"}}
Other configs: {{Config "sometarget"}}

Secrets and configs can be accessed either by source (secret name) or
target (file name).

Templating of secrets/configs is optional, and is disabled by default
because it's very possible for Go template syntax to conflict with the
syntax of the payload.

The only change that will be necessary in the executor is wrapping the
DependencyGetter in NewTemplatedDependencyGetter before handing it off
to the execution backend.

cc @aluzzardi @stevvooe @diogomonica @cyli

stevvooe · 2017-04-24T17:53:53Z

template/context.go

+	t            *api.Task
+}
+
+func (sg secretPayloadGetter) GetBySource(source string) (string, error) {


In the theme of brevity, could this just be ByTarget?

stevvooe · 2017-04-24T17:57:18Z

template/context.go

+// invoke those methods.
+type PayloadContext struct {
+	Context
+	Secrets secretPayloadGetter


Why not add this to the base context? It will keep the scope of what is available in a template a lot clearer.

Presumably, this particular context is only available for contexts after env variables have been evaluated?

I decided to avoid adding these to the base Context type because I wanted to avoid exposing secrets during template substitution on labels, environment variables, etc. This might be a desirable feature to have in its own right, but I was worried there might be more opportunities for injection here, so didn't want to bundle it into this PR.

@diogomonica: Any thoughts on where templates should be able to use secrets or not?

That said, I think when I change this to use the func map as you suggested, this structure will no longer be necessary. There will just be Context with the func map injected (in places where we allow secret/config substitution), and Context without the func map (where we disallow secret/config substitution). This is probably a cleaner result.

stevvooe · 2017-04-24T18:04:04Z

@aaronlehmann This looks like it has the correct layout, but I am slightly worried about exposing methods on the injected objects. Would it be safer for us to inject these into the func map?

For example, we'd have the following, syntactically, as a result:

{{secret "foo"}}
{{config "bar"}}

Such a change would allow us to have a "no method" policy on context objects.

I'm not sure I understand the distinction behind source and target (other than being the "origin of the secret"). I know we do this in the command line to allow one to select the secret but does it make sense here when the target is constant?

aaronlehmann · 2017-04-24T20:02:46Z

This looks like it has the correct layout, but I am slightly worried about exposing methods on the injected objects. Would it be safer for us to inject these into the func map?

I definitely agree. I missed the func map in my cursory read of the template docs. I'll update the PR to work like this instead. Thanks for the suggestion.

I'm not sure I understand the distinction behind source and target (other than being the "origin of the secret"). I know we do this in the command line to allow one to select the secret but does it make sense here when the target is constant?

The target is considered more "portable" - rotating a secret would involve creating a new one with a different name, and updating the service to mount that new secret under the same target (filename). However, I felt it might be weird to only be able to reference secrets by target, and not by secret name - so I included both options here. If people think one or the other makes sense as a canonical method of reference, I'm fine with just choosing one.

stevvooe · 2017-04-24T22:34:41Z

If people think one or the other makes sense as a canonical method of reference, I'm fine with just choosing one.

Let's make the default the syntactically nice option, then have an alternate for the other. It sounds like target is the "right" option, but I can see the need for referencing source.

codecov · 2017-04-25T00:31:46Z

Codecov Report

Merging #2133 into master will increase coverage by 0.21%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master    #2133      +/-   ##
==========================================
+ Coverage   60.06%   60.28%   +0.21%     
==========================================
  Files         119      120       +1     
  Lines       19847    19972     +125     
==========================================
+ Hits        11921    12040     +119     
+ Misses       6568     6556      -12     
- Partials     1358     1376      +18

aaronlehmann · 2017-04-25T00:35:48Z

I've updated the PR to inject functions into the template instead of relying on methods. This also allowed switching to more usable syntax. The injected functions are:

{{Secret "secretname"}}     # by target
{{SecretBySource "secretname"}}
{{Config "configname"}}     # by target
{{ConfigBySource "configname"}}
{{Env "envvar"}}

I kept the PayloadContext for now, but can unify it with Context if this still makes sense. I just wasn't sure about adding secret and config related fields to a structure which may be used in a context where we (currently) don't want to support secret/config substitution.

diogomonica · 2017-04-25T21:46:50Z

@aaronlehmann since by default in docker secret create NAME NAME is the source, shouldn't we invert SecretBySource to be SecretByTarget?

aaronlehmann · 2017-04-25T22:11:05Z

I guess the question is which way should be the default. Specifying a secret by its target is more "portable", since the source may change as the secret data is rotated. In the original version of the PR I called the functions SecretBySource and SecretByTarget to be very explicit, but @stevvooe suggested having a simple function and, and an alternate one for the less common use case:

Let's make the default the syntactically nice option, then have an alternate for the other. It sounds like target is the "right" option, but I can see the need for referencing source.

Another thing I can think of is making the function take two arguments:

{{Secret "source" "mysecret"}}

or borrowing the k/v syntax we use in the CLI to specify a source or target:

{{Secret "source=mysecret"}}

I'm not sure what's best here.

stevvooe · 2017-04-28T23:00:52Z

template/context.go

+		"Secret":         ctx.secretGetterByTarget,
+		"ConfigBySource": ctx.configGetterBySource,
+		"Config":         ctx.configGetterByTarget,
+		"Env":            ctx.envGetter,


I think the convention for template functions is to be lowercase.

I made them uppercase for consistency with the existing template variables such as .Service.Name, but I'm happy to change them to lowercase if that's better.

The "best practice" seems to be that variables are uppercase and functions are lowercase. This was followed with join.

Here is the builtin function set: https://golang.org/pkg/text/template/#hdr-Functions.

I think we follow this convention in docker, as well: https://github.com/moby/moby/blob/master/pkg/templates/templates.go#L11.

No problem, I'll make them lowercase.

stevvooe · 2017-04-28T23:02:20Z

template/template.go

-func newTemplate(s string) (*template.Template, error) {
-	return template.New("expansion").Option("missingkey=error").Funcs(funcMap).Parse(s)
+func newTemplate(s string, extraFuncs template.FuncMap) (*template.Template, error) {
+	tmpl := template.New("expansion").Option("missingkey=error").Funcs(funcMap)


Does this merge the func maps?

Does this merge the func maps?

Yeah.

stevvooe · 2017-04-28T23:05:18Z

What about this?

{{secret "mysecret" }} // target secret
{{secret "mysecret" "bysource=true"}} // source secret

This will allow us to parameterize secret access in templates in the future?

aaronlehmann · 2017-04-28T23:07:14Z

What about this?

{{secret "mysecret" }} // target secret
{{secret "mysecret" "bysource=true"}} // source secret

SGTM. I'll wait for Diogo and Andrea to chime in before making any changes.

diogomonica · 2017-04-29T17:58:20Z

@aaronlehmann I like it

aaronlehmann · 2017-05-02T02:22:26Z

Pushed some updates:

Changed function names to lowercase
Replaced SecretBySource and ConfigBySource with an optional bysource=true. @aluzzardi suggested dropping support for referencing by source entirely, so we could do that instead if the access by source is not useful.
Changed SecretGetter and ConfigGetter interfaces to return an error from their Get methods, so a template error can propagate up to the executor and fail the task.

diogomonica · 2017-05-03T18:15:33Z

@aaronlehmann agreed on dropping it by source.

stevvooe · 2017-05-03T21:08:39Z

@aaronlehmann agreed on dropping it by source.

Seconded. We can always add the parameter later.

aaronlehmann · 2017-05-03T21:14:47Z

I've removed the bysource option. PTAL.

stevvooe · 2017-05-03T22:07:12Z

LGTM

diogomonica · 2017-05-05T00:06:36Z

LGTM

aaronlehmann · 2017-05-08T20:18:27Z

ping @aluzzardi

aluzzardi · 2017-05-08T20:42:55Z

Overall looks good. My 2 cents:

I'm questioning the usefulness of secret templating. The same could be achieved by importing a secret inside a config template
If we go that route anyway, what happens when including a templatized secret? Do we apply the template? If so, do we run into recursive issues?
Rather than having a Templating option, wouldn't it be more future proof to have a Type (defaults to PLAIN, can be GO_TEMPLATE)? Maybe I'm overthinking it
I'm not sure about best practices regarding templating. I know InfraKit is using lowercase functions (https://github.com/infrakit/examples/blob/master/swarm/worker-init.sh), either way it's fine.

aaronlehmann · 2017-05-08T21:01:39Z

I'm questioning the usefulness of secret templating. The same could be achieved by importing a secret inside a config template

The only reason I can think of is that the actual config files are not backed by tmpfs, but secrets are. @diogomonica any thoughts?

If we go that route anyway, what happens when including a templatized secret? Do we apply the template? If so, do we run into recursive issues?

In this PR, only top-level templates are evaluated, to avoid recursion. There is a unit test related to this. I suppose we could support multiple levels of templating, and have some kind of cycle detector, but only applying one template seemed like the simplest thing to do for now.

Rather than having a Templating option, wouldn't it be more future proof to have a Type (defaults to PLAIN, can be GO_TEMPLATE)? Maybe I'm overthinking it

Sounds okay to me, any opinions for/against @diogomonica @stevvooe? I feel like in the future we might add other properties that are not mutually exclusive with templating, which is why I made the field specific to templating, but then again we might add properties that are mutually exclusive with templating...

I'm not sure about best practices regarding templating. I know InfraKit is using lowercase functions (https://github.com/infrakit/examples/blob/master/swarm/worker-init.sh), either way it's fine.

There was some discussion above that resulted in the functions becoming lowercase: #2133 (comment)

stevvooe · 2017-05-09T20:54:08Z

If we go that route anyway, what happens when including a templatized secret? Do we apply the template? If so, do we run into recursive issues?

I think it is clear that secrets are evaluated in line. This must be a dag, in that a template can consume a secret, but not vice versa.

Rather than having a Templating option, wouldn't it be more future proof to have a Type (defaults to PLAIN, can be GO_TEMPLATE)? Maybe I'm overthinking it

For consistency with secret, we may be past this point. We may have wanted "external types" at some point, but we just have to deal with sibling fields for that case. I actually think a bool here for template or not would be fine (template_enable). If we have other types, we can add them later.

I'm not sure about best practices regarding templating. I know InfraKit is using lowercase functions (https://github.com/infrakit/examples/blob/master/swarm/worker-init.sh), either way it's fine.

I hunted this down. In docker, we use lowercase for functions and that seems to be the best practice across the Go community.

aaronlehmann · 2017-05-10T23:52:27Z

I'm tempted to just keep the protos as they are in this PR. It feels a bit simpler to have a single field related to templating than a bool and a template type field. And if we leave the template type field for the future, existing implementations wouldn't be aware of it, and they would try to evaluate everything as a Go template.

But I really don't feel strongly. If there is a preference for switching to a bool, I'm fine with doing that. If there's a preference for renaming the field to Type, I'm fine with that as well.

@aluzzardi

stevvooe · 2017-05-10T23:56:57Z

@aaronlehmann I would prefer the bool. I'm only bringing it up because that is unusual.

aaronlehmann · 2017-05-11T00:53:00Z

I've updated the protobufs to use the Driver type to specify the templating engine, instead of an enum. This will allow us to potentially support templating plugins in the future, and to pass options to templating engines.

PTAL

stevvooe · 2017-05-11T23:46:47Z

LGTM

aaronlehmann · 2017-05-17T01:38:57Z

Rebased

Add the ability for secrets and configs to expand templates, which can reference the following fields: - The same set of fields available to ContainerSpec templates ({{.Service.Name}}, {{.Node.ID}}, etc) - Environment variables: {{Env "envvar"}} - Other secrets: {{Secret "sometarget"}} - Other configs: {{Config "sometarget"}} Secrets and configs can be accessed either by source (secret name) or target (file name). Templating of secrets/configs is optional, and is disabled by default because it's very possible for Go template syntax to conflict with the syntax of the payload. The only change that will be necessary in the executor is wrapping the DependencyGetter in NewTemplatedDependencyGetter before handing it off to the execution backend. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

This allows a templating error to fail a task. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Rename Secret, Config, and Env to lowercase variants. Replace SecretBySource/ConfigBySource with optional bysource=true arguments to "secret" and "config". Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

aaronlehmann · 2017-05-18T21:53:34Z

Rebased

diogomonica · 2017-05-21T07:17:39Z

LGTM!

aluzzardi · 2017-06-13T22:18:08Z

LGTM

/cc @dhiltgen

wsong

I'm not really qualified to make specific comments on the code, but it LGTM overall. Design seems sensible.

stevvooe reviewed Apr 24, 2017

View reviewed changes

aaronlehmann force-pushed the templating branch from f54d9b1 to 80b3585 Compare April 25, 2017 00:31

aaronlehmann force-pushed the templating branch from 80b3585 to 7a88763 Compare April 25, 2017 00:32

stevvooe reviewed Apr 28, 2017

View reviewed changes

aaronlehmann force-pushed the templating branch from bb28e48 to c5b7d1a Compare May 17, 2017 01:38

Aaron Lehmann added 5 commits May 18, 2017 14:52

Change SecretGetter's Get to return an error

9e0cd5a

This allows a templating error to fail a task. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Rename template functions

4273ee8

Rename Secret, Config, and Env to lowercase variants. Replace SecretBySource/ConfigBySource with optional bysource=true arguments to "secret" and "config". Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

templating: Remove bysource option

e002d36

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Change Templating to use the Driver type

b9bdb4d

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

aaronlehmann force-pushed the templating branch from c5b7d1a to b9bdb4d Compare May 18, 2017 21:53

aaronlehmann mentioned this pull request May 18, 2017

Swarm mode templating support moby/moby#31218

Closed

3 tasks

wsong reviewed Jun 15, 2017

View reviewed changes

aaronlehmann merged commit 72e1943 into moby:master Jun 15, 2017

aaronlehmann deleted the templating branch June 15, 2017 18:07

aaronlehmann mentioned this pull request Jun 15, 2017

Templated secrets and configs moby/moby#33702

Merged

gbarr01 mentioned this pull request Mar 12, 2018

To doc: Add support for templated secrets and configs docker/docs#6207

Open

Support templated secrets and configs #2133

Support templated secrets and configs #2133

Uh oh!

Conversation

aaronlehmann commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

stevvooe commented Apr 24, 2017

Uh oh!

aaronlehmann commented Apr 24, 2017

Uh oh!

stevvooe commented Apr 24, 2017

Uh oh!

codecov bot commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aaronlehmann commented Apr 25, 2017

Uh oh!

diogomonica commented Apr 25, 2017

Uh oh!

aaronlehmann commented Apr 25, 2017

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!

stevvooe commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann commented Apr 28, 2017

Uh oh!

diogomonica commented Apr 29, 2017

Uh oh!

aaronlehmann commented May 2, 2017

Uh oh!

diogomonica commented May 3, 2017

Uh oh!

stevvooe commented May 3, 2017

Uh oh!

aaronlehmann commented May 3, 2017

Uh oh!

stevvooe commented May 3, 2017

Uh oh!

diogomonica commented May 5, 2017

Uh oh!

aaronlehmann commented May 8, 2017

Uh oh!

aluzzardi commented May 8, 2017

Uh oh!

aaronlehmann commented May 8, 2017

Uh oh!

stevvooe commented May 9, 2017

Uh oh!

aaronlehmann commented May 10, 2017

Uh oh!

stevvooe commented May 10, 2017

Uh oh!

aaronlehmann commented May 11, 2017

Uh oh!

stevvooe commented May 11, 2017

Uh oh!

aaronlehmann commented May 17, 2017

Uh oh!

aaronlehmann commented Apr 21, 2017 •

edited

Loading

codecov bot commented Apr 25, 2017 •

edited

Loading

stevvooe commented Apr 28, 2017 •

edited

Loading