Deliver secrets using assignments by cyli · Pull Request #1544 · moby/swarmkit

cyli · 2016-09-15T22:28:59Z

Depends on #1511 (as in, this is stacked on top of the changes in #1511)

This adds secrets to the raft store, and implements dispatching of secrets.

diogomonica · 2016-09-15T22:35:37Z

aaronlehmann · 2016-09-15T22:41:52Z


+// Secrets is a map that keeps all the currenty available secrets to the agent
+type Secrets struct {
+	sync.RWMutex


It's a good idea to unembed the mutex as a private field so callers in other packages can't directly lock or unlock it.

mu sync.RWMutex

Or even better, unexport the Secrets type if it will only be used in this package.

Have done both in the latest changes.

aaronlehmann · 2016-09-15T22:43:12Z

 	db        *bolt.DB
 	executor  exec.Executor
 	listeners map[*statusReporterKey]struct{}
+	secrets   *Secrets


Not sure this should be a pointer

If we added the Get/Add/Remove apis to secrets, seems like they should have pointer receivers because they're mutating internal state. In that case, should this be a pointer in order to call those APIs?

This doesn't have to be referenced through a pointer from the worker struct for its methods to have pointer receivers.

Oh cool - I thought it had to be a pointer in order to call functions that had pointer receivers. Thanks

Changed to be not a pointer

aaronlehmann · 2016-09-15T22:44:15Z

+	if fullSnapshot {
+		w.secrets.m = make(map[string]*api.Secret)
+		for _, secret := range added {
+			w.secrets.m[secret.Spec.Annotations.Name] = secret


Modifying the map with the read side of the lock held looks like a mistake.

Changed these to call CRD apis on secret instead, which calls mu.Lock instead when adding or removing or resetting.

codecov-io · 2016-09-15T22:44:37Z

Current coverage is 54.90% (diff: 89.47%)

Merging #1544 into master will increase coverage by 1.15%

@@             master      #1544   diff @@
==========================================
  Files            83         85     +2   
  Lines         13989      14120   +131   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           7519       7752   +233   
+ Misses         5456       5350   -106   
- Partials       1014       1018     +4

Powered by Codecov. Last update 0ff1041...cb2eb0a

stevvooe

Where is this injected into the container?

stevvooe · 2016-09-15T22:44:29Z

 )

+// Secrets is a map that keeps all the currenty available secrets to the agent
+type Secrets struct {


Time for a new file.

That contains just this secret struct with its APIs, or also all the worker apis that also deal with secrets?

Added a secrets.go under the agent package that contains the secret structure, along with some additional APIs that manipulate the map.

stevvooe · 2016-09-15T22:46:37Z

+	if fullSnapshot {
+		w.secrets.m = make(map[string]*api.Secret)
+		for _, secret := range added {
+			w.secrets.m[secret.Spec.Annotations.Name] = secret


Let's add an api to the secrets object to avoid manipulate it's locks directly.

aaronlehmann · 2016-09-15T22:47:15Z

+				},
+				indexName: {
+					Name:    indexName,
+					Unique:  true,


This says secret names must be unique, but I think we decided against that.

Oh good point, thanks I missed this one.

diogomonica

LGTM, but I think no Spec changes should be made in this PR.

diogomonica · 2016-09-16T12:57:10Z

+}
+
+// SecretSpec specifies a user-provided secret.
+message SecretSpec {


Should the SecretSpec be here? Maybe this PR should be rebased out of #1511 so it doesn't include these portions?

It is rebased out of #1511, but since it's not being merged into #1511 (it's going to be merged into master, and #1511 itself is not yet merged into master), the diff contains all the changes from both #1511 and this change.

diogomonica · 2016-09-16T12:57:47Z

 }
+
+// SecretReference is the linkage between a service and a secret that it uses.
+message SecretReference {


Same thing here. SecretReference should have been define in #1511

aluzzardi · 2016-09-26T21:55:57Z

@cyli Now that #1511 can you rebase this? Or do we want to review the control API PR first?

cyli · 2016-09-26T21:57:54Z

@aluzzardi Ah apologies, could you please review #1567 first? We probably should merge some of the changes from there (to the secret state store), and IIRC this one may need some changes to the way assignments are done as per comments in #1511.

cc @diogomonica

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>

…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>

Assignments now provides a stream with incremental task and secret updates. Additional object types can be supported in the assignment set in the future. The first message returned from the Assignments stream is the complete set of tasks and secrets for the node, and this is used to synchronize the node's view with the manager's. Additional messages returned by the stream are incremental updates that add, update, or remove one or more tasks or secrets. If the agent gets out of sync with the manager, it can reinitiate the Assignments stream to sync up. Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>

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

…nt from secret state store. Signed-off-by: cyli <cyli@twistedmatrix.com>

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

cyli · 2016-10-06T05:22:16Z

+
+			for _, secretRef := range container.Secrets {
+				secretID := secretRef.SecretID
+				if tasksUsingSecret[secretID] == nil {


I think this check isn't necessary, since deleting an item from a nil map does not panic. https://play.golang.org/p/e3GXoOb6SC
The check below it should cover the case of adding removeSecrets[secretID] = struct{}{} and updating modified.
Will remove this check.

Confirmed in the spec:

If the map m is nil or the element m[k] does not exist, delete is a no-op.

cyli · 2016-10-06T05:39:31Z

 			Checks: []state.TaskCheckFunc{state.TaskCheckNodeID}},
 		state.EventDeleteTask{Task: &api.Task{NodeID: nodeID},
 			Checks: []state.TaskCheckFunc{state.TaskCheckNodeID}},
+		state.EventUpdateSecret{},


Non-blocking question: does this part works without providing it a secret? Can you watch just all generic events this way? I tried deleting a secret that was being used in a task in TestAssignments, but nothing happened (no changes were pushed). I'm not familiar with the dispatcher though, so my expectations might just be wrong.

Having state.EventUpdateSecret{} should receive all updates to any secret.

I tried deleting a secret that was being used in a task in TestAssignments, but nothing happened (no changes were pushed)

That sounds like a bug to me. Could you try debugging the code under case state.EventDeleteSecret: (seeing if it triggers, what it does, etc...)?

Ah you're right, sorry, it was looking up the secrets by name, thanks. :) Fixed and uncommented test.

After discussion with @diogomonica, since we probably don't want to delete secrets out from under a task, going to make this a noop for now.

cyli · 2016-10-06T06:02:26Z

Also non-blocking question: looks like Dispatcher.Tasks still exists - is that kept for backwards compatibility?

…re sent down. Also, if a Secret corresponding to a SecretReference doesn't exist, there is no error. Signed-off-by: cyli <ying.li@docker.com>

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

aaronlehmann · 2016-10-06T10:22:07Z

Also non-blocking question: looks like Dispatcher.Tasks still exists - is that kept for backwards compatibility?

Yes

aaronlehmann · 2016-10-06T11:19:02Z


+func reconcileSecrets(ctx context.Context, w *worker, assignments []*api.AssignmentChange, fullSnapshot bool) error {
+	var (
+		updatedSecrets []api.Secret


Why does this use []api.Secrets as opposed to reconcileTaskState which uses []*api.Task? Not saying it's a problem, but wondering if there's a reason for storing pointers in one case and copies of the object in another.

I'm guessing it's because the secrets map object takes copies of secrets. I can change that to take pointers to secrets instead, but it seems like the golang wiki encourages you to pass (small) structs by value unless you're modifying it. Secrets seem small to me (we aren't modifying them) but I don't really know at what point they'd consider a struct "large" :)

I also don't really have any strong feelings on the matter and I'd be happy to go with whatever the preferred convention is.

Updated the tasks in reconcileTaskState to be a list of tasks rather than pointers to tasks.

aaronlehmann · 2016-10-06T11:20:53Z

+message Assignment {
+	oneof item {
+		Task task = 2;
+		Secret secret = 3;


It seems a bit strange to remove a secret by sending down a copy of the secret. Not sure I understand how this works. In the old code, we removed a secret by sending the ID of the secret to remove, which makes more sense to me.

I'm not sure about this one. :) Can this be addressed in a future PR? Seems like then an AssignmentChange should be one of AssignmentUpdateChange (which has an Assignment field) or AssignmentRemoveChange (which has a string field representing IDs). Not sure if in the future we'd want to be able to remove by more than just ID, and that change might limit that ability.

We do this to avoid creating another enum type for each message type. The packed object works similar to a tagged enum, without extra pageantry.

aaronlehmann · 2016-10-06T11:23:14Z

+		removedSecrets []string
+	)
+	for _, a := range assignments {
+		if s, ok := a.Assignment.GetItem().(*api.Assignment_Secret); ok {


a.Assignment.GetSecret()

aaronlehmann · 2016-10-06T11:23:53Z

+		removedTasks []*api.Task
+	)
+	for _, a := range assignments {
+		if t, ok := a.Assignment.GetItem().(*api.Assignment_Task); ok {


if t := a.Assignment.GetTask(); t != nil

aaronlehmann · 2016-10-06T11:29:25Z

 						if equality.TasksEqualStable(oldTask, v.Task) && v.Task.Status.State > api.TaskStateAssigned {
 							// this update should not trigger a task change for the agent
 							tasksMap[v.Task.ID] = v.Task
+							// If this task go updated to a final state, lets release


Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2016-10-06T22:11:26Z

On Thu, Oct 06, 2016 at 02:38:33PM -0700, Ying Li wrote:

I'm guessing it's because the secrets map object takes copies of secrets. I can change that to take pointers to secrets instead, but it seems like the golang wiki encourages you to pass (small) structs by value unless you're modifying it. Secrets seem small to me (we aren't modifying them) but I don't really know at what point they'd consider a struct "large" :)

I also don't really have any strong feelings on the matter and I'd be happy to go with whatever the preferred convention is.

I agree with that in general. Don't let this block the PR. Ideally I
would make it consistent between tasks and secrets, though.

Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2016-10-07T05:57:23Z

I'm not sure about this one. :) Can this be addressed in a future PR? Seems like then an AssignmentChange should be one of AssignmentUpdateChange (which has an Assignment field) or AssignmentRemoveChange (which has a string field representing IDs). Not sure if in the future we'd want to be able to remove by more than just ID, and that change might limit that ability.

Since it's a protobuf change, I think we should get it right here.

ping @diogomonica: Any thoughts?

stevvooe · 2016-10-07T07:10:51Z


+message Assignment {
+	oneof item {
+		Task task = 2;


This should be field number 1.

aaronlehmann

LGTM

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

cyli changed the title ~~WIP: assignments with secrets~~ Deliver secrets using assignments Sep 15, 2016

cyli force-pushed the assignments-with-secrets branch from 56d6c73 to 08b109f Compare September 15, 2016 22:38

aaronlehmann reviewed Sep 15, 2016

View reviewed changes

stevvooe suggested changes Sep 15, 2016

View reviewed changes

aaronlehmann reviewed Sep 15, 2016

View reviewed changes

cyli force-pushed the assignments-with-secrets branch from c00303f to d2028a9 Compare September 15, 2016 23:48

diogomonica approved these changes Sep 16, 2016

View reviewed changes

cyli changed the title ~~Deliver secrets using assignments~~ WIP: Deliver secrets using assignments Sep 26, 2016

diogomonica force-pushed the assignments-with-secrets branch from d2028a9 to c92dbc1 Compare October 1, 2016 00:24

diogomonica and others added 11 commits September 30, 2016 17:59

Initial secrets protobuf changes

d0562a3

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>

Restructured Secret Protos

0dd8f07

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

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

7e86510

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

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

f452c57

…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

4b9a169

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

Added simple Agent secret store

132dd46

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

added secret addition and removal test to dispatcher

22d0a56

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

removing secrets on terminal tasks

7546f82

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

Change order of operations for secret addition and removal

7440aa0

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

Add CRD api to the agent secret map, and remove unique name requireme…

65f43b3

…nt from secret state store. Signed-off-by: cyli <cyli@twistedmatrix.com>

diogomonica force-pushed the assignments-with-secrets branch from c92dbc1 to 3787807 Compare October 1, 2016 01:06

diogomonica changed the title ~~WIP: Deliver secrets using assignments~~ Deliver secrets using assignments Oct 1, 2016

Changed the Worker API back to Assign and Update, but using Assignmen…

c05a63d

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

cyli force-pushed the assignments-with-secrets branch from 5f9222a to a9e0e46 Compare October 6, 2016 05:08

cyli commented Oct 6, 2016

View reviewed changes

cyli force-pushed the assignments-with-secrets branch from a9e0e46 to 7d2f5d6 Compare October 6, 2016 05:36

cyli commented Oct 6, 2016

View reviewed changes

Also test that when a stream is started, existing tasks and secrets a…

86453e2

…re sent down. Also, if a Secret corresponding to a SecretReference doesn't exist, there is no error. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the assignments-with-secrets branch from 7d2f5d6 to 86453e2 Compare October 6, 2016 07:39

Add old Tasks tests for backwards compatibility

2b2af92

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

aaronlehmann reviewed Oct 6, 2016

View reviewed changes

Updating and Deleting secrets should not affect assignments

9491db8

Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the assignments-with-secrets branch from 4e81e86 to 9491db8 Compare October 6, 2016 19:54

cyli force-pushed the assignments-with-secrets branch 3 times, most recently from 2280581 to 2a24ac8 Compare October 6, 2016 23:07

Agent cleanups

cb2eb0a

Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the assignments-with-secrets branch from 2a24ac8 to cb2eb0a Compare October 6, 2016 23:09

stevvooe reviewed Oct 7, 2016

View reviewed changes

stevvooe approved these changes Oct 7, 2016

View reviewed changes

aaronlehmann approved these changes Oct 7, 2016

View reviewed changes

diogomonica merged commit c7ade46 into moby:master Oct 7, 2016

cyli deleted the assignments-with-secrets branch October 7, 2016 21:15

cyli mentioned this pull request Oct 7, 2016

Small fix to ensure Assignment field indexes start at 1 #1604

Merged

aaronlehmann mentioned this pull request Dec 17, 2016

Add cyli to maintainers #1818

Merged

Conversation

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

Uh oh!

Uh oh!

diogomonica commented Sep 15, 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!

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!

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

Uh oh!

Current coverage is 54.90% (diff: 89.47%)

Uh oh!

stevvooe left a comment

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!

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!

diogomonica left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyli Sep 16, 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!

aluzzardi commented Sep 26, 2016

Uh oh!

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

Uh oh!

Uh oh!

cyli Oct 6, 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!

cyli Oct 6, 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!

cyli commented Oct 6, 2016

cyli commented Sep 15, 2016 •

edited

Loading

codecov-io commented Sep 15, 2016 •

edited

Loading

cyli Sep 16, 2016 •

edited

Loading

cyli commented Sep 26, 2016 •

edited

Loading

cyli Oct 6, 2016 •

edited

Loading

cyli Oct 6, 2016 •

edited

Loading