Gut reaction is I think this has the highest probability of success of all the options i've seen for this.

is this really worthwhile?

Not sure yet. Creating secrets from templates or user input is currently pretty rough.

Definitely the cleanest option.

I think we should reconsider this for 1.3

Why? Is this really a significant source of user pain?

Yes. It makes any for of config templates extremely difficult to use for
user input secrets unless your template language also offers a nice base64
encoder.

On Apr 30, 2016, at 1:23 AM, Tim Hockin notifications@github.com wrote:

Why? Is this really a significant source of user pain?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#22059 (comment)

ok. I guess I buy this, but I hate the solution. No I do not have a
better one that is back-compatible.

On Sat, Apr 30, 2016 at 9:27 AM, Clayton Coleman notifications@github.com
wrote:

Yes. It makes any for of config templates extremely difficult to use for
user input secrets unless your template language also offers a nice base64
encoder.

On Apr 30, 2016, at 1:23 AM, Tim Hockin notifications@github.com wrote:

Why? Is this really a significant source of user pain?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
<
#22059 (comment)

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#22059 (comment)

I agree that it's a "worst, except for all the others" sort of thing

Least worst amongst a curated array of terrible solutions

On Sun, May 1, 2016 at 9:52 AM, Jordan Liggitt notifications@github.com
wrote:

I agree that it's a "worst, except for all the others" sort of thing

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#22059 (comment)

GCE e2e build/test passed for commit 83381ea.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@liggitt PR needs rebase

@thockin @bgrant0607 are you all ok with proceeding with something like this?

@ncdc @liggitt @pmorie

Adding new fields is complicated, but this also breaks compatibility for secret consumers, which would likely try to decode the fields as base64.

@bgrant0607 this is a one way conversion on write to enable templates or other writers that don't have base64 conversion capabilities. Output is always base64, so secret consumers aren't affected

@liggitt Ah, I didn't read the description carefully enough.

So it breaks the rule that the server shouldn't ever mutate fields populated by the client. :-)

I definitely would not want to do this anywhere else, since it would break kubectl apply and probably other things. However, users shouldn't use kubectl apply on secrets, so that reason isn't relevant in this case. Are there other get+diff or hash implementations we need to be concerned out?

On Jun 28, 2016, at 11:46 PM, Brian Grant notifications@github.com wrote:

@liggitt https://github.com/liggitt Ah, I didn't read the description
carefully enough.

So it breaks the rule that the server shouldn't ever mutate fields
populated by the client. :-)

I definitely would not want to do this anywhere else, since it would break
kubectl apply and probably other things. However, users shouldn't use
kubectl apply on secrets, so that reason isn't relevant in this case. Are
there other get+diff or hash implementations we need to be concerned out?

Isn't apply already broken on new fields we add (even if backward compat)?
Ie have field A, add new field B which if A is set is ignored, but if the
user applies B without A, A is merged ignoring B? So either way user
intent gets lost with apply across field changes. Adding a new new field
"stringData" that is converted to "data" would have to be dropped on the
server side anyway.

@smarterclayton New fields whose purpose overlaps existing fields break all forms of updates.

So, yes, I'm not suggesting new string fields.

Rather than a string: prefix, what about a bool field to indicate that all the fields should be interpreted as strings?

@bgrant0607 I had considered something like that (either a field or an annotation) but I wasn't sure if it made sense to persist that bool. I guess we could always reset it to false prior to storing it in etcd.

Rather than a string: prefix, what about a bool field to indicate that all the fields should be interpreted as strings?

I was hoping to limit the changes to the unmarshal phase, especially since I don't really want protobuf to have to change for this.

I also like the ability to do it per value, rather than applying to the whole object (makes a patch nice if you want to update one key's value using a string value)

Also, a separate boolean field that says "change the meaning of this other field" means that old servers can accidentally accept string data as binary data {"dataAreStrings":true,"data":{"key":"abcd"}} would be misunderstood by an old server, rather than throwing an error

I do like the per-value application as well

@liggitt @ncdc

I dislike the precedent this sets, and the lack of discoverability. The latter could be addressed via documentation on the API fields, but I'm going to throw out a few other ideas first.

We could use an alternative API endpoint (a subresource or alternate resource path) to indicate that a different type is being sent, which would contain both a string map and a byte map, but the object created and persisted would only contain the current Secret resource (bytes).

Or, we could add the string map to Secret, and clear it once the object was created, similar to the rollbackTo stanza in Deployment -- intended for imperative APIs rather than normal declarative/REST usage.

On Jun 29, 2016, at 6:57 PM, Brian Grant notifications@github.com wrote:

@liggitt https://github.com/liggitt @ncdc https://github.com/ncdc

I dislike the precedent this sets, and the lack of discoverability. The
latter could be addressed via documentation on the API fields, but I'm
going to throw out a few other ideas first.

We could use an alternative API endpoint (a subresource or alternate
resource path) to indicate that a different type is being sent, which would
contain both a string map and a byte map, but the object created and
persisted would only contain the current Secret resource (bytes).

We'd have to create an alternate resource path, since subresources wouldn't
be usable in lists or templates.

Or, we could add the string map to Secret, and clear it once the object was
created, similar to the rollbackTo stanza in Deployment -- intended for
imperative APIs rather than normal declarative/REST usage.

I'm fine with that as well.

I agree on precedent (weird APIs-within-an-API) and discoverability.

Of the options, I like a peer stringData map[string]string field the best, documented as a write-only convenience, and never output when reading a Secret from the API.

I would expect the stringData keys/values to always be moved to the data map, overwriting existing keys in the data map. That would allow patch to work as expected.

The one shortcoming is that there's no way to tell if the server knows about stringData without inspecting the Secret that got created... I was hoping to have a system an old server would reject rather than create a Secret with missing data, but I can live without it

opened #28263 with the proposed peer field approach

closing in favor of #28263