[DO NOT MERGE] secrets enhancements #18586

pmorie · 2015-12-11T18:31:16Z

Forking from #18298 to consider together with WIP on #4789. Currently includes #18580 to get unit tests working.

This WIP adds secrets in envs and fine-grained control over the location of secret data within secret volumes.

@erictune @thockin @bgrant0607 @liggitt @deads2k @smarterclayton

Known issues at this time (will fix in next pass):

Needs validation to enforce variant type constraint on EnvVarSource
Secret in env implementation is inefficient and should cache secrets as they are fetched

k8s-github-robot · 2015-12-11T18:34:15Z

Labelling this PR as size/XL

k8s-bot · 2015-12-11T19:13:42Z

GCE e2e test build/test passed for commit 60145451ec23be6b41679669ebcb2a84edfba4cd.

k8s-bot · 2015-12-11T20:31:30Z

GCE e2e test build/test passed for commit 68efb8c.

k8s-bot · 2015-12-19T19:19:26Z

GCE e2e test build/test passed for commit 68efb8c.

thockin · 2015-12-31T23:26:39Z

pkg/api/types.go

+// SecretVolumeFile represents a single secret key projected into a file.
+type SecretVolumeFile struct {
+	// A reference to the secret key to project into the volume.
+	SecretKeySelector `json:",inline"`


Two observations on this for discussion.

This allows multiple Secrets to project into a single mount. I don't have a problem with this.

This makes the case of projecting multiple keys from a single Secret harder in that you have to name the Secret over and over. I feel like this would be a common case, especially for ConfigMap which should probably follow the same pattern as Secret.

How bad would it be for the volume to look like:

name: MySecrets secret: secretName: app-secret items: - key: username path: username.txt - key: password path: password.txt

(note this breaks the multi-secret-in-one-mount property)

and the env to look like:

- name: USERNAME valueFrom: secretRef: secretName: app-secret key: username

(note that this means they use different structs)

@thockin

How bad would it be for the volume to look like:

name: MySecrets
secret:
secretName: app-secret
items:
- key: username
path: username.txt
- key: password
path: password.txt

I like this better from a stylistic standpoint, actually. It feels really chatty having to specify the secret name for each file. You are correct, the current state of this PR does make it possible to mount multiple secrets into a volume. I have no problem with using different structs to implement your API though from a class standpoint it seems a little fine-grained to have to split types, I don't think it's a huge deal.

We wanted the config map and secrets to be similar from an API standpoint, and what you suggest is a change from what we agreed to for config map in https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/configmap.md#consuming-configmap-as-volumes. So, do we want to consider whether the API for configmap volumes should also change? From a user standpoint, I guess the question is whether people need to be able to compose a single volume from multiple instances of a resource, or whether volume plugins should deal with a single instance of a resource only.

Thinking about API compatibility, the current state of this PR does seem a little dicey to me now. An older client would always have to submit valid pods with secretName set, but older clients would see a secret volume without secretName set, which are invalid. @bgrant0607 is that an issue?

cc @smarterclayton

You're right that this looks like an API break. I think my proposal avoids that, too.

I do think we should use the same model for ConfigMap and Secrets, and I think the linked proposal should be changed.

If we eventually need to compose multiple ConfigMaps or Secrets into a single directory, we could pluralize and have a list of {name, items[]}, leaving the singular one as the first instance. It's a little clunky but it gives us a compatible API that can be fixed in v2.

Alt model: a single "managed data volume source" that covers downward api, config map, and secret:

name: my-mega-dir items: - configMapName: my-config-map files: - key: config file: myapp.conf - secretName: my-secret files: - key: password file: path/passwd.txt - dowardAPI: apiVersion: v1 fieldPath: metadata.name file: myapp.name.txt

I am not sure this is better, but it's an idea.

@thockin we are getting 10 replies deep to a comment on a commit, breaking this discussion out to the main PR.

@pmorie We cannot change fields from required to optional or vice versa, since the former breaks readers and the latter breaks writers. We have fixed the specification of omitempty to match the existing behavior a few times, but that's different than changing the observed behavior.

pmorie · 2016-01-06T19:06:48Z

@thockin thanks for the review comments -- I've just rebased this and noticed your comments in between meetings, will think on them. I'm not opposed to changing this around a bit.

k8s-bot · 2016-01-06T19:29:14Z

GCE e2e build/test failed for commit 9596d3cb33beb3abc8c84d5bd6097bd664647d3d.

pmorie · 2016-01-07T06:02:44Z

@sdminonne This is also relevant to your interests

pmorie · 2016-01-13T14:52:11Z

Replying on main PR to comment on commit at #18586 (comment)

@thockin @smarterclayton @bgrant0607 @erictune

Alt model: a single "managed data volume source" that covers downward api, config map, and secret:

I was thinking about that possibility this weekend. Here's where I eventually wound up on it after some digestion today. TL;DR might be an efficient way to combine these concerns in a way that users like.

From a compatibility standpoint a new volume type is a least-risk change
All-in-one is probably a lot less shipped code, and more test code
Arguably all-in-one is most intuitive for users

Now, let me submit some additional brain damage for consideration. I think what users want in addition to
an all-in-one volume is the ability to project a file or files into an existing directory without mounting over the contents of that directory. I will peel off a separate issue for this, but I mention it because I can see a user wanting to use an all-in-one, and then project that entire volume onto an existing directory.

bgrant0607 · 2016-01-13T16:24:03Z

@pmorie @thockin Re. an all-in-one volume source:

As long as it's possible to have multiple ManagedDataVolumeSources, I guess that would be ok. People will want to automatically inject new such volume sources.

For instance, for servicing linking, I'd imagine automatically injecting environment variables, downward API, secret, and configmap data.

I totally agree that people will want to map config files into /etc and other standard linux directories.

Do you think anyone would want service link info in the form of files, or only env vars? I would like to move to explicit downward API for service links at some point.

ncdc · 2016-01-13T20:34:04Z

@bgrant0607

Do you think anyone would want service link info in the form of files, or only env vars? I would like to move to explicit downward API for service links at some point.

I would guess that most web developers are used to env vars. That doesn't mean they couldn't switch to using files, but env vars are fairly common.

pmorie · 2016-01-14T15:17:52Z

@bgrant0607

Do you think anyone would want service link info in the form of files, or only env vars? I would like to move to explicit downward API for service links at some point.

I don't see a reason not to make the information available in volume form, especially if it's a matter of adding a new thing to the all-in-one-volume

pmorie · 2016-01-14T23:15:39Z

Rebase is just to recut #18298 from this before I close it out and open a new PR for the all-in-one volume.

k8s-bot · 2016-01-14T23:34:31Z

GCE e2e build/test failed for commit 5ae0da33df8f74c2dd3168cfac3e7b8ef088c9c9.

thockin · 2016-01-15T18:44:36Z

Injecting files into directories requires a) making a null file as a
mountpoint and b) accepting that you can not dynamically update that file
(see the /etc/hosts fiasco, it actually is possible but it is not
pretty).

On Wed, Jan 13, 2016 at 8:24 AM, Brian Grant notifications@github.com
wrote:

@pmorie https://github.com/pmorie @thockin https://github.com/thockin
Re. an all-in-one volume source:

As long as it's possible to have multiple ManagedDataVolumeSources, I
guess that would be ok. People will want to automatically inject new such
volume sources.

For instance, for servicing linking, I'd imagine automatically injecting
environment variables, downward API, secret, and configmap data.

I totally agree that people will want to map config files into /etc and
other standard linux directories.

Do you think anyone would want service link info in the form of files, or
only env vars? I would like to move to explicit downward API for service
links at some point.

—
Reply to this email directly or view it on GitHub
#18586 (comment)
.

thockin · 2016-01-15T18:50:41Z

Whether we do an all-in-one should derive from use-cases. Have we heard
any indication that this is what people want? We can always add volume
sources later, but taking away is long and slow.

I think least resistance path is to do a single dir, with multiple
keys-as-files from a single secret/configmap. If we need to tackle
multiple secrets/configmaps or projecting different backend types into the
same dir, we can consider a new all-in-one.

Projecting individual files is a whole different topic that certainly
impacts this but is frought with its own issues.

On Fri, Jan 15, 2016 at 10:44 AM, Tim Hockin thockin@google.com wrote:

Injecting files into directories requires a) making a null file as a
mountpoint and b) accepting that you can not dynamically update that file
(see the /etc/hosts fiasco, it actually is possible but it is not
pretty).

On Wed, Jan 13, 2016 at 8:24 AM, Brian Grant notifications@github.com
wrote:

@pmorie https://github.com/pmorie @thockin https://github.com/thockin
Re. an all-in-one volume source:

As long as it's possible to have multiple ManagedDataVolumeSources, I
guess that would be ok. People will want to automatically inject new such
volume sources.

For instance, for servicing linking, I'd imagine automatically injecting
environment variables, downward API, secret, and configmap data.

I totally agree that people will want to map config files into /etc and
other standard linux directories.

Do you think anyone would want service link info in the form of files, or
only env vars? I would like to move to explicit downward API for service
links at some point.

—
Reply to this email directly or view it on GitHub
#18586 (comment)
.

pmorie · 2016-01-16T18:09:50Z

@thockin I'm inclined to agree with you about this:

I think least resistance path is to do a single dir, with multiple keys-as-files from a single secret/configmap. If we need to tackle multiple secrets/configmaps or projecting different backend types into the same dir, we can consider a new all-in-one.

I'll proceed for configMap with the agreed upon design, and peel off issues for all-in-one volume and projecting individual files into container FS.

k8s-github-robot · 2016-01-16T18:57:43Z

Labelling this PR as size/XXL

k8s-bot · 2016-01-16T19:23:52Z

GCE e2e test build/test passed for commit a8d1f5545a6f77fd07884cf90ab3cc8a85c7f17e.

pmorie · 2016-01-16T19:26:37Z

Once again, I am in a situation where the codecgen generates changes, and verify-codecgen rejects them.

Currently on Golang 1.4. I'll switch to 1.5 and see what happens. In this specific situation, the exact behavior is that verify-codecgen.sh fails on my dev machine, jenkins, and travis. In the past there have been other permutations.

pmorie · 2016-01-18T16:47:39Z

#19768 seems to have fixed the issues I was having earlier.

k8s-github-robot · 2016-01-18T16:52:14Z

Labelling this PR as size/XL

k8s-bot · 2016-01-18T17:18:57Z

GCE e2e test build/test passed for commit d7ce434.

thockin · 2016-01-21T18:31:55Z

docs/user-guide/secrets/secret-volume-files-pod.yaml

+    - name: secret-volume
+      secret:
+        items:
+          - name: test-secret


I thought we had agreed that the design would move to:

volumes: - name: secret-volume secret: secretName: test-secret items: - key: data-1 path: special/path/__special_thing-1.dat - key: data-2 path: special/path/__special_thing-2.dat

for all of the examples, showing 2 keys makes the YAML much clearer.

as per a comment in a different PR, this might want to change to:

volumes: - name: secret-volume secret: name: test-secret # an embedded LocalObjectRef items: - key: data-1 path: special/path/__special_thing-1.dat - key: data-2 path: special/path/__special_thing-2.dat

pmorie · 2016-01-21T20:22:47Z

I'm closing this PR out and will open a new one with changes to the secret volume plugin as a follow-up.

pmorie · 2016-01-30T21:36:16Z

Closing; will open a new PR for secrets volume changes.

googlebot added the cla: yes label Dec 11, 2015

k8s-github-robot assigned smarterclayton Dec 11, 2015

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 11, 2015

pmorie force-pushed the secret-files branch from 6014545 to 68efb8c Compare December 11, 2015 19:47

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2015

thockin reviewed Dec 31, 2015
View reviewed changes

pmorie force-pushed the secret-files branch from 68efb8c to 9596d3c Compare January 6, 2016 18:54

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 12, 2016

pmorie force-pushed the secret-files branch from 9596d3c to 5ae0da3 Compare January 14, 2016 23:14

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2016

pmorie mentioned this pull request Jan 16, 2016

All-in-one managed data volume #19762

Closed

pmorie force-pushed the secret-files branch from 5ae0da3 to a8d1f55 Compare January 16, 2016 18:45

k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2016

k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 16, 2016

pmorie added 2 commits January 18, 2016 11:36

WIP: Secrets enhancements

b175484

Regenerate for secrets enhancements

d7ce434

pmorie force-pushed the secret-files branch from a8d1f55 to d7ce434 Compare January 18, 2016 16:46

k8s-github-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 18, 2016

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 18, 2016

pmorie changed the title ~~WIP: secrets enhancements~~ [DO NOT MERGE] secrets enhancements Jan 18, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2016

pmorie mentioned this pull request Jan 21, 2016

Update proposal for ConfigMap volume #19923

Merged

thockin reviewed Jan 21, 2016
View reviewed changes

pmorie closed this Jan 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] secrets enhancements #18586

[DO NOT MERGE] secrets enhancements #18586

pmorie commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-bot commented Dec 19, 2015

thockin Dec 31, 2015

pmorie Jan 9, 2016

thockin Jan 12, 2016

thockin Jan 12, 2016

pmorie Jan 13, 2016

bgrant0607 Jan 13, 2016

pmorie commented Jan 6, 2016

k8s-bot commented Jan 6, 2016

pmorie commented Jan 7, 2016

pmorie commented Jan 13, 2016

bgrant0607 commented Jan 13, 2016

ncdc commented Jan 13, 2016

pmorie commented Jan 14, 2016

pmorie commented Jan 14, 2016

k8s-bot commented Jan 14, 2016

thockin commented Jan 15, 2016

thockin commented Jan 15, 2016

pmorie commented Jan 16, 2016

k8s-github-robot commented Jan 16, 2016

k8s-bot commented Jan 16, 2016

pmorie commented Jan 16, 2016

pmorie commented Jan 18, 2016

k8s-github-robot commented Jan 18, 2016

k8s-bot commented Jan 18, 2016

thockin Jan 21, 2016

thockin Jan 21, 2016

pmorie commented Jan 21, 2016

pmorie commented Jan 30, 2016

[DO NOT MERGE] secrets enhancements #18586

[DO NOT MERGE] secrets enhancements #18586

Conversation

pmorie commented Dec 11, 2015

k8s-github-robot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-bot commented Dec 11, 2015

k8s-bot commented Dec 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Jan 6, 2016

k8s-bot commented Jan 6, 2016

pmorie commented Jan 7, 2016

pmorie commented Jan 13, 2016

bgrant0607 commented Jan 13, 2016

ncdc commented Jan 13, 2016

pmorie commented Jan 14, 2016

pmorie commented Jan 14, 2016

k8s-bot commented Jan 14, 2016

thockin commented Jan 15, 2016

thockin commented Jan 15, 2016

pmorie commented Jan 16, 2016

k8s-github-robot commented Jan 16, 2016

k8s-bot commented Jan 16, 2016

pmorie commented Jan 16, 2016

pmorie commented Jan 18, 2016

k8s-github-robot commented Jan 18, 2016

k8s-bot commented Jan 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie commented Jan 21, 2016

pmorie commented Jan 30, 2016