Redundant switch. IMO should be

mode := swarmapi.SecretReference_FILE
if s.Mode == types.SecretReferenceSystem {
    mode = swarmapi.SecretReference_SYSTEM
}

maybe make convertSecretReferenceMode function

I think you can embed Version and Spec in structure initialization

Maybe add some words here.

just return this here

wrong import

you can use WithFields for creating such structured messages :)

ya thx :)

we have pkg/errors vendored now. Would you mind to use errors.Wrap{f} in such cases?

shouldn't this func cleanup on error? I mean like unmount and remove secrets dir.

ok i added a defer func that should cleanup. ptal thx!

Sounds like this warning will be logged on each container run. Is that right?

Can we use api/errors with something like a BadRequest?

Also lower-case errors

thx -- i was following the convention. i'll update

Would this only ever be an internal error? If not I'd not wrap the error here and make sure the backend provides a sufficient message.

👎
This format is nice in the happy case, but in the long run is a nightmare.

ok. do you have an alternative?

I think @stevvooe once suggested the unicode arrow ;)

But seriously, I think the code here has the right approach. The secret name and target need to be specified adjacent to each other instead of using separate options. We could use key-value pairs, but I think that would be really ugly and unintuitive.

@cpuguy83 @aaronlehmann what about something like

--secret ssh-dev,target=id_rsa,uid=1000,gid=1000,mode=0600

any thoughts on the above @cpuguy83 @aaronlehmann ?

I would say in general it looks good.
I think ssh-dev should be source=ssh-dev to match --mount (big discussion on whether to allow the positional syntax from that PR).

Only question I'd ask is, how does this work with if/when an alternative secrets backend is supported (e.g. Vault). Would the current options work on all backends/platforms? Can Windows support this?

Not a big fan of source=. It complicates the normal case where all you want to specify is a source, which now involves key/value pairs. --secret-add source=secretname doesn't look like a particularly friendly CLI to me.

What about keeping the key/value pairs as the general case, but allowing a shorthand of --secret-add secretname (i.e. if no = is found, take the argument as the source)? I'm not completely sure I like this, but I think I like it more than forcing key/value syntax for this argument.

Find by me.

But mixing positional and k/v in the same spec (e.g. --secret-add secretname,target=mysecret) not so much.

I agree with @aaronlehmann's suggestion. We should have a shorthand of --secret-add secretname

not ramfs?

we decided to not use ramfs since the swarmkit storage swaps to disk so we don't gain anything by using ramfs.

makes sense

@ehazlett does this mean secrets are effectively stored plain-text on disk?

For the container they are stored in tmpfs (in memory). They are encrypted at rest in the Swarm storage.

ok awesome, thanks @ehazlett 😁

yw :)

Why do we have a slice but only ever append one thing to it?

it was to add more in the future but good point. i'll remove the slice.

doesn't match the doc string

Don't like that we're shoving secrets in here purely for indexing, but I guess we are doing it for other things as well right now.

Doesn't work when a name is specified.

Nor prefixes

correct. swarmkit only supports getting by id atm

swarmkit can filter by name or name prefix using the ListSecrets RPC. This is the same as other object types like service and node - Get is always by ID.

So we could implement a GetSecretByName that effectively uses ListSecrets with a name filter under it.

Spec should not be a pointer.

We probably don't want omitempty for SecretSize. Probably not for any of these fields, actually.

Again, probably don't want to omit the Data field if the secret is zero-length.

I suppose we still need to add the ability for the user to specify a custom target?

There seems to be an inconsistency between service create and service update. Shouldn't they both take secrets in the format that specifies a name and target (i.e. use parseSecrets)?

ya we actually don't even need this because i set it outside in order to do the lookup. good catch thx!

targetName = secretName to avoid the else case below?

+1

This looks wrong. It will set every map value to point to the same iterator. Just make the map values secret IDs instead of pointers to secrets. That will make things simpler.

no such secret: %s

Is it correct to return an empty Mount structure in this case? I see that it's unconditionally appended to a slice in createSpec.

I had this as a nil check before; i'll switch back

This could be router.NewPostRoute("/secrets", sr.createSecret) 👼

Ya I was trying to follow the convention but I like /secrets better. thx!

This looks like it's unused.

Args to a deferred function are evaluated at the time of the defer statement, so this won't work as expected.

ah ok thx

i'm using a named return now so i think this should work. PTAL

It still looks wrong. You shouldn't pass setupErr as an argument to the deferred function. You should just let it capture the variable from the outer scope.

you are correct. thx

Is it correct to continue if the directory can't be created?

nope -- i need to rework the cleanup