Secret-specific protos by cyli · Pull Request #1511 · moby/swarmkit

cyli · 2016-09-07T23:29:14Z

This does not include any implementation yet - the goal is:

merge Use assignments instead of tasks #1508
merge this
open 1-2 secrets-implementation PRs

aaronlehmann · 2016-09-07T23:34:27Z

api/objects.proto

+
+	Meta meta = 2 [(gogoproto.nullable) = false];
+
+	map<string, SecretData> secret_data = 3;


Please document this field, including the meaning of the map keys.

codecov-io · 2016-09-07T23:35:47Z

Current coverage is 54.06% (diff: 0.00%)

Merging #1511 into master will increase coverage by 0.03%

@@             master      #1511   diff @@
==========================================
  Files            82         83     +1   
  Lines         13605      13613     +8   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7351       7360     +9   
+ Misses         5261       5258     -3   
- Partials        993        995     +2

Powered by Codecov. Last update 8b9cdbb...ac092de

aaronlehmann · 2016-09-07T23:36:40Z

api/objects.proto

+	string digest = 4;
+
+	// Size represents the size of the secret
+	int32 size = 5 [(gogoproto.customname) = "SecretSize"];


Should this be uint32, or do negative values have a valid meaning?

Yes, uint32, thank you!

aaronlehmann · 2016-09-07T23:39:19Z

api/secrets.proto

+	repeated Secret secrets = 1;
+}
+
+// Note CreateSecretRequest does not contain a SecretSpec, since that can contain multiple SecretData objects, which have digests


This comment doesn't look right; CreateSecretRequest does contain a SecretSpec.

cyli · 2016-09-08T16:40:02Z

Just to make sure I remembered correctly - did we want the secrets in a different RPC service entirely, because the control one was too big? Or did we want it in the same RPC service but a different file? cc @stevvooe

stevvooe · 2016-09-08T19:16:11Z

api/secrets.proto

+
+// GetSecretRequest is the request to get a `Secret` object given a secret name.
+message GetSecretRequest {
+	string name = 1;


Most objects are fetched by id, rather than name.

I figured you'd want to look up a secret by name vs ID - I guess they should use ListSecret for that?

In that case, you list by name, then use the id to access it via get.

In that case, you list by name, then use the id to access it via get.

Agreed, this is how it works with other objects in controlapi.

stevvooe · 2016-09-08T19:23:38Z

@cyli The structure and file layout of this PR looks optimal.

We may want to discuss with @al but since secrets aren't necessarily a cluster only operation, this will allow us to relocate the component running the actual service. Effectively, we could decouple this from the cluster types, if need be.

al · 2016-09-08T19:26:47Z

Not the right @al I'm afraid. Who is this guy? He seems to be rather
prolific. I almost wish I was he.

On 8 Sep 2016 20:23, "Stephen Day" notifications@github.com wrote:

@cyli https://github.com/cyli The structure and file layout of this PR
looks optimal.

We may want to discuss with @al https://github.com/al but since secrets
aren't necessarily a cluster only operation, this will allow us to relocate
the component running the actual service. Effectively, we could decouple
this from the cluster types, if need be.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1511 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAApv97Tp-r9Mb-j6PpcHIISy3mk5gS3ks5qoGC-gaJpZM4J3dfm
.

stevvooe · 2016-09-08T19:42:53Z

@al My bad. Hah.

PTAL @aluzzardi

diogomonica · 2016-09-08T21:06:49Z

api/types.proto

+// SecretType provides information about what kind of secret this is
+enum SecretType {
+	CONTAINER = 0 [(gogoproto.enumvalue_customname) = "ContainerSecret"];
+	NODE = 1 [(gogoproto.enumvalue_customname) = "NodeSecret"];


@cyli let's remove node for now

@diogomonica For the network encryption key we decided to add a new secret type. Should we add a type 'NETWORK' to SecretType ? What is its intended purpose, is it for type checking ? ie: a SecretReference from a Service has to be a secret of type CONTAINER ?

We should add the network specific functionality in a future PR.

@diogomonica We need to think about this use cases now.

We need to cover the registry auth case, as well.

aaronlehmann · 2016-09-23T17:14:57Z

Thanks for the really quick turnaround!

Left the global secret lamport clock because since users have to version their own via name, it'd be nice to have a reliable ordering when they list

I know I was the one who asked for this, but I feel it may not be worth the complexity anymore now that there's no internal need for it. For user display purposes, I think ordering by timestamp would work well enough.

aaronlehmann · 2016-09-23T17:16:59Z

api/types.proto

+
+// SecretReference is the linkage between a service and a secret that it uses.
+message SecretReference {
+	// Name is the name of the secret that this references.


As discussed, this name will be informational, and secret_id below will be used to actually resolve the secret. Let's make this clear in the comments.

Good point. I've also moved this to the end of the message (although I don't know that ordering implies any sort of importance).

sanimej · 2016-09-23T18:03:29Z

@cyli @aaronlehmann Did we decide to drop the lamport_time field ? For network gossip encryption we need a reliable ordering to delete the old key and insert the new key on every rotation. It can be handled in the client side by having a lamport_time for each secret reference. But if its part of the Secret it will be convenient.

cyli · 2016-09-23T18:05:44Z

@sanimej I've dropped it as per @aaronlehmann's suggestion, but I can also easily add it back if it's needed.

aaronlehmann · 2016-09-23T18:08:37Z

How are network gossip keys going to be managed in a world where we have immutable secrets and no built-in versioning?

sanimej · 2016-09-23T18:23:53Z

How are network gossip keys going to be managed in a world where we have immutable secrets and no built-in versioning?

For each network we will have a SecretSpec and three SecretReference in the Network object corresponding to three keys needed for gossip encryption. Network key manager in the swarm leader will be responsible for deleting the old secret, create a new one on each rotation and update the reference in the Network object. Having a lamport_time will avoid having to maintain extra state in the client (key manager in this case).

Updated network object will be sent in the Assignment stream only to the nodes that have task with that network attachment.

sanimej · 2016-09-23T18:33:48Z

api/control.proto

+// removes all versions of the secret.
+message RemoveSecretRequest {
+	string secret_id = 1 [(gogoproto.customname) = "SecretID"];
+}


Can we make this a repeated field ?

I have no objection, but none of the other remove requests work this way, so would like further input from @aaronlehmann or @stevvooe or @aluzzardi :)

Sorry, to clarify this statement I think supporting removing multiple objects in a single transaction might make sense, especially since the network secrets come bundled together. However, it would be an inconsistency in what the Remove* argument takes.

I think it wouldn't be too hard to support this for other objects in the control API as well on the server side, but migrating their existing APIs over to support that may be tricky, if previously it took a single string and now it takes a list of strings.

Will the network manager actually be using the control API, or will it be updating the secret store behind the scenes? There is also an internal bool on the Secret object, which the control API does not provide any way to set - I think the assumption was that the network manager would create that Secret itself and store it?

It's behind the scenes, so it can use a transaction.

I think having RemoveSecret remove a single secret is most consistent with the other external APIs.

Sounds good. Leaving this as is, then, and have also removed the max number of secrets in the cluster.

Agreed - consistency first.

It makes sense, but if we want to do it, we should do it across the board on every other Remove* request so it's outside of the scope of this PR.

aaronlehmann · 2016-09-23T18:36:17Z

Thanks for explaining.

I think it may be preferable to keep the state in the key manager, versus maintaining the lamport_time field.

The reason is that lamport_time is inefficient to maintain. It involves listing all secrets every time a new secret is created, or maintaining state somewhere else to keep track of this. I expect the key manager would have less overhead because it doesn't need to maintain a global ordering of all secrets (maybe it could just keep the SecretsReferences sorted inside the Network object)?

sanimej · 2016-09-23T18:50:06Z

I think it may be preferable to keep the state in the key manager, versus maintaining the lamport_time field.

If we don't foresee other users/clients of the secret infra having a similar requirement I think its ok to leave the versioning part to the key manager.

stevvooe · 2016-09-23T22:16:46Z

api/types.proto

 	uint32 election_tick = 5;
 }

+


Extra space. ಠ_ಠ

stevvooe · 2016-09-23T22:18:22Z

api/types.proto

+
+	// Name is the name of the secret that this references, but this is just provided for
+	// lookup/display purposes.  The secret in the reference will be identified by its ID.
+	string name = 4;


SecretName?

stevvooe · 2016-09-23T22:18:51Z

LGTM

aluzzardi · 2016-09-23T22:40:39Z

api/control.proto

+// removes all versions of the secret.
+message RemoveSecretRequest {
+	string secret_id = 1 [(gogoproto.customname) = "SecretID"];
+}


Agreed - consistency first.

It makes sense, but if we want to do it, we should do it across the board on every other Remove* request so it's outside of the scope of this PR.

aluzzardi · 2016-09-23T22:43:55Z

api/specs.proto

+	Annotations annotations = 1 [(gogoproto.nullable) = false];
+
+	// Secret payload.
+	bytes data = 2;


nit: naming? Not opposed to data but wondering if someone has a better name in mind (@stevvooe ?)

nit: Add a comment to data to explain the maximum allowed length

This is as good as anything.

data it is - discard that comment

Included a max size constant in the secrets.go control API stub, and added a comment here.

Add a Secret top-level object type. Add a SecretReference that allows a service to reference the secrets it needs. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

…ipulating secrets. Also update some of the object protos to make things easier to look up. Signed-off-by: cyli <cyli@twistedmatrix.com>

Signed-off-by: cyli <cyli@twistedmatrix.com>

…ets and max number of secret versions. Also add dispatcher protos for secrets. Signed-off-by: cyli <cyli@twistedmatrix.com>

Signed-off-by: cyli <cyli@twistedmatrix.com>

…terface Signed-off-by: cyli <cyli@twistedmatrix.com>

GordonTheTurtle added the status/0-triage label Sep 7, 2016

aaronlehmann reviewed Sep 7, 2016
View reviewed changes

cyli force-pushed the secret-protos branch from cfc444c to c04ce20 Compare September 7, 2016 23:37

aaronlehmann reviewed Sep 7, 2016
View reviewed changes

cyli force-pushed the secret-protos branch 2 times, most recently from a691a40 to 8bcbc6f Compare September 8, 2016 01:26

stevvooe reviewed Sep 8, 2016
View reviewed changes

diogomonica reviewed Sep 8, 2016
View reviewed changes

cyli force-pushed the secret-protos branch 2 times, most recently from 5a73a31 to 6ec5865 Compare September 8, 2016 23:37

aaronlehmann reviewed Sep 23, 2016

View reviewed changes

cyli force-pushed the secret-protos branch from 73b8d39 to 1415b17 Compare September 23, 2016 17:25

sanimej reviewed Sep 23, 2016

View reviewed changes

cyli force-pushed the secret-protos branch from 34d7111 to 4666ab8 Compare September 23, 2016 21:48

stevvooe suggested changes Sep 23, 2016

View reviewed changes

cyli force-pushed the secret-protos branch from 4666ab8 to b0641e9 Compare September 23, 2016 22:20

stevvooe approved these changes Sep 23, 2016

View reviewed changes

aluzzardi approved these changes Sep 23, 2016

View reviewed changes

diogomonica and others added 9 commits September 23, 2016 15:54

Initial secrets protobuf changes

16c699a

Add a Secret top-level object type. Add a SecretReference that allows a service to reference the secrets it needs. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

Changing protos and addressing comments

a3f8c85

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

Restructured Secret Protos

95d5fb4

Signed-off-by: Diogo Monica <diogo.monica@gmail.com>

Add a secrets proto file that contains messages and rpc calls for man…

78be1af

…ipulating secrets. Also update some of the object protos to make things easier to look up. Signed-off-by: cyli <cyli@twistedmatrix.com>

Address code review comments, and document the secret protos.

9f42341

Signed-off-by: cyli <cyli@twistedmatrix.com>

Add a secrets config to the cluster specifying the max number of secr…

df431a1

…ets and max number of secret versions. Also add dispatcher protos for secrets. Signed-off-by: cyli <cyli@twistedmatrix.com>

Unify Secret and SecretValue, and remove UpdateSecret as an API

d40a338

Signed-off-by: cyli <cyli@twistedmatrix.com>

Move secret proto inside the control API

f0da64c

Signed-off-by: cyli <cyli@twistedmatrix.com>

Add stub implementations of the secret control APIs to satisfy the in…

ac092de

…terface Signed-off-by: cyli <cyli@twistedmatrix.com>

cyli force-pushed the secret-protos branch from b0641e9 to ac092de Compare September 23, 2016 22:55

diogomonica merged commit 63600e0 into moby:master Sep 23, 2016

cyli deleted the secret-protos branch September 23, 2016 23:03

cyli mentioned this pull request Sep 24, 2016

secrets control-api implementation #1567

Merged

aaronlehmann mentioned this pull request Dec 17, 2016

Add cyli to maintainers #1818

Merged


		Meta meta = 2 [(gogoproto.nullable) = false];

		map<string, SecretData> secret_data = 3;

Conversation

cyli commented Sep 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 54.06% (diff: 0.00%)

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!

cyli commented Sep 8, 2016

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 Sep 8, 2016

Uh oh!

al commented Sep 8, 2016

Uh oh!

stevvooe commented Sep 8, 2016

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!

aaronlehmann commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanimej commented Sep 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cyli commented Sep 23, 2016

Uh oh!

aaronlehmann commented Sep 23, 2016

Uh oh!

sanimej commented Sep 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 23, 2016 • 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!

aaronlehmann commented Sep 23, 2016

Uh oh!

sanimej commented Sep 23, 2016 • 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

codecov-io commented Sep 7, 2016 •

edited

Loading

sanimej commented Sep 23, 2016 •

edited

Loading

cyli Sep 23, 2016 •

edited

Loading

sanimej commented Sep 23, 2016 •

edited

Loading