Close leafnode connection when same cluster name detected

[derekcollison]

Signed-off-by: Derek Collison derek@nats.io

/cc @nats-io/core

[matthiashanel]

LGTM, some tests fail due to usage of DefaultOptions

@@ -1262,6 +1262,7 @@ func TestLeafNodePermissions(t *testing.T) {

        u, _ := url.Parse(fmt.Sprintf("nats://%s:%d", lo1.LeafNode.Host, lo1.LeafNode.Port))
        lo2 := DefaultOptions()
+       lo2.Cluster.Name = "cde"
        lo2.LeafNode.ReconnectInterval = 5 * time.Millisecond
        lo2.LeafNode.connDelay = 100 * time.Millisecond
        lo2.LeafNode.Remotes = []*RemoteLeafOpts{
@@ -1405,6 +1406,7 @@ func TestLeafNodePermissionsConcurrentAccess(t *testing.T) {

        u, _ := url.Parse(fmt.Sprintf("nats://%s:%d", lo1.LeafNode.Host, lo1.LeafNode.Port))
        lo2 := DefaultOptions()
+       lo2.Cluster.Name = "cde"
        lo2.LeafNode.ReconnectInterval = 5 * time.Millisecond
        lo2.LeafNode.connDelay = 500 * time.Millisecond
        lo2.LeafNode.Remotes = []*RemoteLeafOpts{
@@ -1705,6 +1707,7 @@ func TestLeafNodeTLSVerifyAndMap(t *testing.T) {
        } {
                t.Run(test.name, func(t *testing.T) {
                        o := DefaultOptions()
+                       o.Cluster.Name = "cde"
                        o.Accounts = []*Account{acc}
                        o.LeafNode.Host = "127.0.0.1"
                        o.LeafNode.Port = -1
@@ -1969,7 +1972,7 @@ func TestLeafNodeOriginClusterInfo(t *testing.T) {
                        remotes [ { url: "nats://127.0.0.1:%d" } ]
                }
                cluster {
-                       name: "abc"
+                       name: "cde"
                        listen: "127.0.0.1:-1"
                }
        `, hopts.LeafNode.Port)))
@@ -1987,7 +1990,7 @@ func TestLeafNodeOriginClusterInfo(t *testing.T) {
        checkLeafNodeConnected(t, s)

        l = grabLeaf()
-       if rc := l.remoteCluster(); rc != "abc" {
+       if rc := l.remoteCluster(); rc != "cde" {
                t.Fatalf("Expected a remote cluster name of \"abc\", got %q", rc)
        }
        pcid := l.cid
@@ -4101,15 +4104,16 @@ leafnodes:{
        test(jsAA, jsLL)
 }

[derekcollison]

Fixed test.

[kozlovic]

This test seem to fail too, is that the one that was fixed?

=== RUN   TestClientClampMaxSubsErrReport
    client_test.go:2534: Expected 1 connected leafnode(s) for server "NCZSORUN25U6ESFJWOYBYBFGSR62NZJLHN7BWUNBZJ2EKIVCIBWGREP5", got 0
--- FAIL: TestClientClampMaxSubsErrReport (5.06s)

[kozlovic]

@derekcollison Maybe one more test?

$ go test -race -v -run=TestReconnectErrorReports ./server/ -count=1 -vet=off
=== RUN   TestReconnectErrorReports
=== RUN   TestReconnectErrorReports/route_attempt_1
=== RUN   TestReconnectErrorReports/route_attempt_2
=== RUN   TestReconnectErrorReports/route_attempt_3
=== RUN   TestReconnectErrorReports/route_attempt_4
=== RUN   TestReconnectErrorReports/route_attempt_6
=== RUN   TestReconnectErrorReports/route_attempt_7
=== CONT  TestReconnectErrorReports
    server_test.go:1845: Expected 1 connected leafnode(s) for server "NBWUQD6UOYFBOJKSPESAZEPNY4ND3H7N2XKRVSBELIBIVMLA32YQ5YA3", got 0
--- FAIL: TestReconnectErrorReports (6.32s)
    --- PASS: TestReconnectErrorReports/route_attempt_1 (0.08s)
    --- PASS: TestReconnectErrorReports/route_attempt_2 (0.00s)
    --- PASS: TestReconnectErrorReports/route_attempt_3 (0.03s)
    --- PASS: TestReconnectErrorReports/route_attempt_4 (0.00s)
    --- PASS: TestReconnectErrorReports/route_attempt_6 (0.05s)
    --- PASS: TestReconnectErrorReports/route_attempt_7 (0.00s)
FAIL

[kozlovic]

LGTM, assuming that we are able to get a green on Travis.

[derekcollison]

Will squash once green then merge.

[ripienaar]

We will need to call this out in release notes warning people before upgrade it might end up splitting their networks and require config changes to fix that

[kozlovic]

@ripienaar Added the the "CHANGED" section of the release notes, but noted I will probably a section at the top to point to that one.

[smlx]

I'm trying to understand the implications of this change on my NATS configuration.

I see that "spoke" and "hub" cluster names for leaf clusters must now be unique. However does this mean that if there are multiple "spoke" leaf clusters they all require unique names? Or can leaf clusters have the same name as long as it is different to the "hub" cluster name?

Could anyone comment on the motivation for this change?

[kozlovic]

@smlx The motivation is that cluster name is required to properly route messages between hub and spoke and with separate leaf clusters but with the same name, the hub would not be able to know where to route messages.

[smlx]

Ok thanks. So that means every leaf cluster needs a unique name?

[kozlovic]

So that means every leaf cluster needs a unique name?

Yes.