From 13cb62e0bf1da8544dcfe34d4b827ebd417b19a4 Mon Sep 17 00:00:00 2001 From: Waldemar Quevedo Date: Fri, 21 Jul 2023 00:05:29 -0700 Subject: [PATCH 1/3] Add test checking subscriptions before/after reload Signed-off-by: Waldemar Quevedo --- server/reload_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/server/reload_test.go b/server/reload_test.go index 3f64fa4fa49..0b42bafb3e6 100644 --- a/server/reload_test.go +++ b/server/reload_test.go @@ -2700,6 +2700,42 @@ func TestConfigReloadAccountUsers(t *testing.T) { }) } +func TestConfigReloadAccountWithNoChanges(t *testing.T) { + conf := createConfFile(t, []byte(` + listen: "127.0.0.1:-1" + system_account: sys + accounts { + A { + users = [{ user: a }] + } + B { + users = [{ user: b }] + } + C { + users = [{ user: c }] + } + sys { + users = [{ user: sys }] + } + } + `)) + s, _ := RunServerWithConfig(conf) + defer s.Shutdown() + before := s.NumSubscriptions() + s.Reload() + after := s.NumSubscriptions() + if before != after { + t.Errorf("Number of subscriptions changed after reload: %d -> %d", before, after) + } + + before = s.NumSubscriptions() + s.Reload() + after = s.NumSubscriptions() + if before != after { + t.Errorf("Number of subscriptions changed after reload: %d -> %d", before, after) + } +} + func TestConfigReloadAccountNKeyUsers(t *testing.T) { conf := createConfFile(t, []byte(` listen: "127.0.0.1:-1" From 2b252469caf0b4cba24a4e3faac22c27e1262548 Mon Sep 17 00:00:00 2001 From: Waldemar Quevedo Date: Tue, 1 Aug 2023 22:51:21 -0700 Subject: [PATCH 2/3] fix: add missing default service imports on reload Signed-off-by: Waldemar Quevedo --- server/reload_test.go | 52 ++++++++++++++++++++++++++++++++++++++----- server/server.go | 3 +++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/server/reload_test.go b/server/reload_test.go index 0b42bafb3e6..1ea66194bdc 100644 --- a/server/reload_test.go +++ b/server/reload_test.go @@ -2603,6 +2603,23 @@ func TestConfigReloadAccountUsers(t *testing.T) { t.Fatalf("Error on subscribe: %v", err) } + // confirm subscriptions before and after reload. + var expectedSubs uint32 = 4 + sAcc, _ := s.LookupAccount("synadia") + sAcc.mu.RLock() + n := sAcc.sl.Count() + sAcc.mu.RUnlock() + if n != expectedSubs { + t.Errorf("Synadia account should have %d sub, got %v", expectedSubs, n) + } + nAcc, _ := s.LookupAccount("nats.io") + nAcc.mu.RLock() + n = nAcc.sl.Count() + nAcc.mu.RUnlock() + if n != expectedSubs { + t.Errorf("Nats.io account should have %d sub, got %v", expectedSubs, n) + } + // Remove user from account and whole account reloadUpdateConfig(t, s, conf, ` listen: "127.0.0.1:-1" @@ -2678,8 +2695,8 @@ func TestConfigReloadAccountUsers(t *testing.T) { n = sAcc.sl.Count() barMatch := sAcc.sl.Match("bar") sAcc.mu.RUnlock() - if n != 1 { - return fmt.Errorf("Synadia account should have 1 sub, got %v", n) + if n != expectedSubs { + return fmt.Errorf("Synadia account should have %d sub, got %v", expectedSubs, n) } if len(barMatch.psubs) != 1 { return fmt.Errorf("Synadia account should have bar sub") @@ -2690,8 +2707,8 @@ func TestConfigReloadAccountUsers(t *testing.T) { n = nAcc.sl.Count() batMatch := nAcc.sl.Match("bat") nAcc.mu.RUnlock() - if n != 1 { - return fmt.Errorf("Nats.io account should have 1 sub, got %v", n) + if n != expectedSubs { + return fmt.Errorf("Nats.io account should have %d sub, got %v", expectedSubs, n) } if len(batMatch.psubs) != 1 { return fmt.Errorf("Synadia account should have bar sub") @@ -2719,8 +2736,24 @@ func TestConfigReloadAccountWithNoChanges(t *testing.T) { } } `)) - s, _ := RunServerWithConfig(conf) + s, opts := RunServerWithConfig(conf) defer s.Shutdown() + + ncA, err := nats.Connect(fmt.Sprintf("nats://a:@%s:%d", opts.Host, opts.Port)) + if err != nil { + t.Fatalf("Error on connect: %v", err) + } + defer ncA.Close() + + // Confirm service imports are ok. + resp, err := ncA.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second) + if err != nil { + t.Error(err) + } + if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { + t.Fatal("unexpected data in connz response") + } + before := s.NumSubscriptions() s.Reload() after := s.NumSubscriptions() @@ -2728,6 +2761,15 @@ func TestConfigReloadAccountWithNoChanges(t *testing.T) { t.Errorf("Number of subscriptions changed after reload: %d -> %d", before, after) } + // Confirm this still works... + resp, err = ncA.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second) + if err != nil { + t.Fatal(err) + } + if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { + t.Fatal("unexpected data in connz response") + } + before = s.NumSubscriptions() s.Reload() after = s.NumSubscriptions() diff --git a/server/server.go b/server/server.go index ea6d08ef274..c3885e3e6d8 100644 --- a/server/server.go +++ b/server/server.go @@ -900,6 +900,9 @@ func (s *Server) configureAccounts(reloading bool) (map[string]struct{}, error) c.processUnsub(sid) } acc.addAllServiceImportSubs() + s.mu.Unlock() + s.registerSystemImports(acc) + s.mu.Lock() } // Set the system account if it was configured. From 23b5cb959c2934a03ee9b938bfc714592fe6ae78 Mon Sep 17 00:00:00 2001 From: Waldemar Quevedo Date: Wed, 2 Aug 2023 08:51:08 -0700 Subject: [PATCH 3/3] review fixes Signed-off-by: Waldemar Quevedo --- server/reload_test.go | 64 +++++++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 20 deletions(-) diff --git a/server/reload_test.go b/server/reload_test.go index 1ea66194bdc..946179008cb 100644 --- a/server/reload_test.go +++ b/server/reload_test.go @@ -2605,14 +2605,16 @@ func TestConfigReloadAccountUsers(t *testing.T) { // confirm subscriptions before and after reload. var expectedSubs uint32 = 4 - sAcc, _ := s.LookupAccount("synadia") + sAcc, err := s.LookupAccount("synadia") + require_NoError(t, err) sAcc.mu.RLock() n := sAcc.sl.Count() sAcc.mu.RUnlock() if n != expectedSubs { t.Errorf("Synadia account should have %d sub, got %v", expectedSubs, n) } - nAcc, _ := s.LookupAccount("nats.io") + nAcc, err := s.LookupAccount("nats.io") + require_NoError(t, err) nAcc.mu.RLock() n = nAcc.sl.Count() nAcc.mu.RUnlock() @@ -2671,7 +2673,8 @@ func TestConfigReloadAccountUsers(t *testing.T) { // being reconnected does not mean that resent of subscriptions // has already been processed. checkFor(t, 2*time.Second, 100*time.Millisecond, func() error { - gAcc, _ := s.LookupAccount(globalAccountName) + gAcc, err := s.LookupAccount(globalAccountName) + require_NoError(t, err) gAcc.mu.RLock() n := gAcc.sl.Count() fooMatch := gAcc.sl.Match("foo") @@ -2690,10 +2693,12 @@ func TestConfigReloadAccountUsers(t *testing.T) { return fmt.Errorf("Global account should have baz sub") } - sAcc, _ := s.LookupAccount("synadia") + sAcc, err := s.LookupAccount("synadia") + require_NoError(t, err) sAcc.mu.RLock() n = sAcc.sl.Count() barMatch := sAcc.sl.Match("bar") + sAcc.mu.RUnlock() if n != expectedSubs { return fmt.Errorf("Synadia account should have %d sub, got %v", expectedSubs, n) @@ -2702,7 +2707,8 @@ func TestConfigReloadAccountUsers(t *testing.T) { return fmt.Errorf("Synadia account should have bar sub") } - nAcc, _ := s.LookupAccount("nats.io") + nAcc, err := s.LookupAccount("nats.io") + require_NoError(t, err) nAcc.mu.RLock() n = nAcc.sl.Count() batMatch := nAcc.sl.Match("bat") @@ -2745,15 +2751,31 @@ func TestConfigReloadAccountWithNoChanges(t *testing.T) { } defer ncA.Close() - // Confirm service imports are ok. - resp, err := ncA.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second) - if err != nil { - t.Error(err) - } - if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { - t.Fatal("unexpected data in connz response") + // Confirm default service imports are ok. + checkSubs := func(t *testing.T) { + resp, err := ncA.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second) + if err != nil { + t.Error(err) + } + if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { + t.Fatal("unexpected data in connz response") + } + resp, err = ncA.Request("$SYS.REQ.SERVER.PING.CONNZ", nil, time.Second) + if err != nil { + t.Error(err) + } + if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { + t.Fatal("unexpected data in connz response") + } + resp, err = ncA.Request("$SYS.REQ.ACCOUNT.PING.STATZ", nil, time.Second) + if err != nil { + t.Error(err) + } + if resp == nil || !strings.Contains(string(resp.Data), `"conns":1`) { + t.Fatal("unexpected data in connz response") + } } - + checkSubs(t) before := s.NumSubscriptions() s.Reload() after := s.NumSubscriptions() @@ -2761,15 +2783,17 @@ func TestConfigReloadAccountWithNoChanges(t *testing.T) { t.Errorf("Number of subscriptions changed after reload: %d -> %d", before, after) } - // Confirm this still works... - resp, err = ncA.Request("$SYS.REQ.ACCOUNT.PING.CONNZ", nil, time.Second) - if err != nil { - t.Fatal(err) - } - if resp == nil || !strings.Contains(string(resp.Data), `"num_connections":1`) { - t.Fatal("unexpected data in connz response") + // Confirm this still works after a reload... + checkSubs(t) + before = s.NumSubscriptions() + s.Reload() + after = s.NumSubscriptions() + if before != after { + t.Errorf("Number of subscriptions changed after reload: %d -> %d", before, after) } + // Do another extra reload just in case. + checkSubs(t) before = s.NumSubscriptions() s.Reload() after = s.NumSubscriptions()