Error on cluster spec name change. by anshulpundir · Pull Request #2436 · moby/swarmkit

anshulpundir · 2017-11-09T09:09:47Z

Currently, the cluster name is hardcoded and is expected to not change for the swarmkit to function. Enforcing that by returning an error if cluster name change is attempted.

Signed-off-by: Anshul Pundir anshul.pundir@docker.com

thaJeztah · 2017-11-09T09:13:08Z

manager/controlapi/cluster.go

 	}

+	if spec.Annotations.Name != store.DefaultClusterName {
+		return grpc.Errorf(codes.PermissionDenied, "Modification of cluster name is node allowed")


s/node/not/ 😉

Also can you lowercase Modification?

Also wondering;

is this validation done on create / init as well? (probably should, but in that case the error message may need some tweaking; e.g. "custom cluster names are not supported")

should "empty" values be updated to store.DefaultClusterName? (as is done in cluster: Refuse swarm spec not named "default" moby#28914)

is this validation done on create / init as well?

Creation name is hardcoded, so I don't think a validation is needed there.

should "empty" values be updated to store.DefaultClusterName

Not sure what the motivation behind this is, but its probably better to be explicit and fail if it changes. I believe clients are expected to perform a read-modify-write ?

I guess setting the default would have been much cleaner if there was a way to specify the default value during init (e.g. inline init or in the constructor).

thaJeztah · 2017-11-09T09:33:30Z

Copying here as well (re: moby/moby currently doesn't validate on update) #2435 (comment);

Yes, was just digging the code, and looks to be only done on init; we should validate it on update too (in that case, we may want to do that by comparing before/after instead of comparing to "default").

Should we move that validation to SwarmKit entirely? That way SwarmKit is able to control what's supported and what not

Or.. if SwarmKit does support it (in standalone configurations), we should move this validation to Moby

anshulpundir · 2017-11-09T19:16:17Z

Should we move that validation to SwarmKit entirely? That way SwarmKit is able to control what's supported and what not

I agree that the validation should probably be moved to swarmkit since adding this dependency to moby is exposing internal details best kept inside swarmkit IMO. I can make this change if there are no objections @nishanttotla @aaronlehmann

codecov · 2017-11-09T19:35:44Z

Codecov Report

Merging #2436 into master will decrease coverage by 3.27%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2436      +/-   ##
==========================================
- Coverage   63.89%   60.62%   -3.28%     
==========================================
  Files          64      128      +64     
  Lines       11788    26410   +14622     
==========================================
+ Hits         7532    16011    +8479     
- Misses       3649     9005    +5356     
- Partials      607     1394     +787

nishanttotla

I agree that this should be on SwarmKit. As far as cluster names go, I believe the motivation was to be able to support multiple cluster objects?

cc @aluzzardi

marcusmartins · 2017-11-10T00:00:00Z

@anshulpundir are you going to add tests?

anshulpundir · 2017-11-10T00:06:20Z

@nishanttotla @thaJeztah init cluster from moby/moby calls UpdateCluster(). So the check in UpdateCluster() should do the trick and we can remove the check from moby/moby.

Also, updated tests latest patch @marcusmartins

thaJeztah · 2017-11-10T00:22:17Z

manager/controlapi/cluster.go

 	}

+	if spec.Annotations.Name != store.DefaultClusterName {
+		return grpc.Errorf(codes.PermissionDenied, "modification of cluster name is not allowed")


codes.InvalidArgument perhaps?

hm, feels a bit odd, because caller has access to this endpoint, but passed an invalid argument (The value was not accepted, because the only valid value here is "default"). I don't think it's any different than the other validations performed in this function (e.g. you cannot set any value other than "bcrypt" here; https://github.com/anshulpundir/swarmkit/blob/21e99c146ef0fce96ee89befb950bc75f3f73e68/manager/controlapi/cluster.go#L44

Not sure I follow. Why do you think having access to an endpoint but passing invalid arg is odd ? @thaJeztah

That's not odd, but returning a permission denied instead of invalid argument (like for all the other validation check in this function) feels odd.

agreed. I already changed it to invalid argument. LMK if this looks good @thaJeztah @nishanttotla

I agree with using codes.InvalidArgument.

thaJeztah · 2017-11-10T13:13:36Z

I did a bit of digging in the code, and my concern with this change is that this patch will effectively disable the possibility to have multiple swarms. I realize this functionality is not supported by Docker, but it is part of the SwarmKit design / functionality.

This may also be the reason the initial validation was added in the moby/moby repository, instead of in SwarmKit.

Digging through the code (sorry, not too familiar with the whole SwarmKit codebase);

If a node becomes the "leader" (see manager.becomeLeader()
Even though the default cluster is created (see above), when using SwarmKit, additional clusters can be created using store.CreateCluster()
Docker / Moby never calls store.CreateCluster() because it's only using the default cluster (which has already been created). So "creating" a cluster from Docker's perspective means actually updating the existing cluster with new information.

So:

Because SwarmKit is designed for multiple clusters, we cannot perform this check/validation on CreateCluster() in SwarmKit
If SwarmKit supports renaming an existing cluster (I assume this is supported), we cannot make using the "default" name a requirement for UpdateCluster()

For that reason, I think this validation should be moved to moby/moby, where we can:

Make the assumption that only a single cluster is supported
Make the assumption that that cluster must be named "default"
If no clustername is provided on "update" or "create", always replace with "default"
If another clustername is provided, produce an error

The bug at hand here is that Moby only validates when "initializing" the cluster, but not when updating the cluster (#2435 (comment)). With the above information, this should be an easy fix there.

thaJeztah · 2017-11-10T17:01:09Z

Was working on moby/moby#35464 - but failed to reproduce the "empty name" problem locally, so perhaps it's not correct 😅

marcusmartins · 2017-11-10T17:24:18Z

@thaJeztah it might be part of the design but it doesn't seem to be fully implemented in swarmkit itself based on @aaronlehmann comment on moby/moby@9dba9e3:

...Some parts of swarmkit depend on having a cluster object named
"default". Reject any specs that use other names.

My take is that independent of the swarmkit design if swarmkit cannot handle different names at the moment it should guard against that instead of relying on the caller. And that limitation can be removed once swarmkit fixes that behavior.

marcusmartins · 2017-11-10T17:25:39Z

cc @aluzzardi

anshulpundir · 2017-11-10T19:41:40Z

I couldn't have said it better @marcusmartins
To add to it: swarmkit design might support multiple clusters, but the implementation does not. Besides, this is a check, and checks are good to reduce the number of invariants and makes the system tighter. And, it an just as easily be removed once that functionality comes in. @thaJeztah

I'd like to get this in sooner than later. Please LMK if there are any other concerns.

thaJeztah · 2017-11-10T20:48:36Z

manager/controlapi/cluster_test.go

+				},
+			},
+			c: codes.InvalidArgument,
+		},


Can you add one case that uses a non-empty invalid name?

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

thaJeztah

LGTM as it addresses the problem

it doesn't seem to be fully implemented in swarmkit itself based on @aaronlehmann comment on moby/moby@9dba9e3:

The situation described is matching my earlier analysis: Docker does not explicitly create a new cluster (only uses the one that's implicitly created by manager.becomeLeader()). So any action performed by Docker against SwarmKit should use the "default" one.

I suspect there may still be a bug in UpdateCluster() as that function's documentation mentions Returns ErrNotExist if the cluster doesn't exist., but the code only seems to check for a conflicting cluster (existing != nil), and will continue with tx.update() if the cluster was not found.

stevvooe · 2017-11-10T23:28:23Z

Any reason we can't support renaming of the cluster? The only challenge seems to be that of self-identification, but I think most of the code just assumes there is a single cluster.

stevvooe · 2017-11-10T23:40:45Z

Also, looks like https://github.com/docker/swarmkit/blob/master/manager/keymanager/keymanager.go#L190 needs a fix. This should probably not query by name, but just get all and the result should be checked. Leaving that section as is creates a possible panic situation for the future.

anshulpundir · 2017-11-10T23:42:20Z

Any reason we can't support renaming of the cluster? The only challenge seems to be that of self-identification, but I think most of the code just assumes there is a single cluster.

Yea, identification seems to be the main challenge. Also, as is, I can't think of a (important) use-case to support cluster rename, in which case I don't see the need to support it.

anshulpundir · 2017-11-10T23:44:15Z

Also, looks like https://github.com/docker/swarmkit/blob/master/manager/keymanager/keymanager.go#L190 needs a fix. This should probably not query by name, but just get all and the result should be checked. Leaving that section as is creates a possible panic situation for the future.

Although the assumption in swarmkit is to expect a cluster by the name "default", I think this is a fair point. I don't mind making that change.

stevvooe · 2017-11-10T23:44:16Z

@anshulpundir Supporting cluster renaming is just about being able to have multiple clusters report together. Imagine a UI which manages several clusters. How do you tell the difference between each cluster? The code to support it here isn't much more than is already present and may expose problems with the existing model that could crop up in the future.

anshulpundir · 2017-11-10T23:46:15Z

I see your point. I'm not sure its worthwhile supporting/testing that functionality right now since only a single cluster is supported (at least externally). I completely agree about supporting cluster rename when multiple clusters are supported @stevvooe

aaronlehmann

FWIW this looks good to me. I'm fine with the idea of removing the dependence on the name "default", but that could be a future enhancement.

thaJeztah requested changes Nov 9, 2017

View reviewed changes

anshulpundir force-pushed the name branch from 65dc11b to 78ead92 Compare November 9, 2017 19:35

nishanttotla reviewed Nov 9, 2017

View reviewed changes

anshulpundir force-pushed the name branch from 78ead92 to 21e99c1 Compare November 10, 2017 00:04

thaJeztah reviewed Nov 10, 2017

View reviewed changes

anshulpundir force-pushed the name branch 2 times, most recently from ef8fe59 to e8a48fc Compare November 10, 2017 01:02

thaJeztah reviewed Nov 10, 2017

View reviewed changes

Error on cluster spec name change.

0b078d1

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir force-pushed the name branch from e8a48fc to 0b078d1 Compare November 10, 2017 21:30

nishanttotla approved these changes Nov 10, 2017

View reviewed changes

thaJeztah approved these changes Nov 10, 2017

View reviewed changes

thaJeztah mentioned this pull request Nov 11, 2017

Remove test_update_swarm_name docker/docker-py#1805

Merged

aaronlehmann approved these changes Nov 11, 2017

View reviewed changes

anshulpundir merged commit 663fa6e into moby:master Nov 13, 2017

thaJeztah mentioned this pull request Nov 14, 2017

Bump docker-py to 1d6b5b203222ba5df7dedfcd1ee061a452f99c8a moby/moby#35492

Merged

andrewhsu mentioned this pull request Nov 17, 2017

Backport Error on cluster spec name change. #2443

Merged

seemethere mentioned this pull request Nov 17, 2017

[17.11] Error on cluster spec name change. #2453

Closed

nishanttotla mentioned this pull request Nov 18, 2017

[17.09] Error on cluster spec name change #2454

Merged

andrewhsu mentioned this pull request Dec 5, 2017

Vendor swarmkit to 4429c763 moby/moby#35698

Merged

Conversation

anshulpundir commented Nov 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 9, 2017 • 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!

thaJeztah commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anshulpundir commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nishanttotla left a comment

Choose a reason for hiding this comment

Uh oh!

marcusmartins commented Nov 10, 2017

Uh oh!

anshulpundir commented Nov 10, 2017 • 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 10, 2017

Uh oh!

thaJeztah commented Nov 10, 2017

Uh oh!

marcusmartins commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcusmartins commented Nov 10, 2017

Uh oh!

anshulpundir commented Nov 10, 2017 • 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!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Nov 10, 2017

Uh oh!

stevvooe commented Nov 10, 2017

Uh oh!

anshulpundir commented Nov 10, 2017

Uh oh!

anshulpundir commented Nov 10, 2017

Uh oh!

stevvooe commented Nov 10, 2017

Uh oh!

anshulpundir commented Nov 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aaronlehmann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

thaJeztah Nov 9, 2017 •

edited

Loading

thaJeztah commented Nov 9, 2017 •

edited

Loading

anshulpundir commented Nov 9, 2017 •

edited

Loading

codecov bot commented Nov 9, 2017 •

edited

Loading

anshulpundir commented Nov 10, 2017 •

edited

Loading

marcusmartins commented Nov 10, 2017 •

edited

Loading

anshulpundir commented Nov 10, 2017 •

edited

Loading

anshulpundir commented Nov 10, 2017 •

edited

Loading