From ea6d037b17282b844ce8d528b6ec92b09225872c Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 17:26:06 +0300 Subject: [PATCH 01/11] skip spell check for GroupD Signed-off-by: bcmmbaga --- .github/workflows/golangci-lint.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml index 32a04d1ba..dacb1922b 100644 --- a/.github/workflows/golangci-lint.yml +++ b/.github/workflows/golangci-lint.yml @@ -19,7 +19,7 @@ jobs: - name: codespell uses: codespell-project/actions-codespell@v2 with: - ignore_words_list: erro,clienta,hastable,iif,groupD + ignore_words_list: erro,clienta,hastable,iif,groupd skip: go.mod,go.sum only_warn: 1 golangci: From 9b0424ea5558501d48416a0983fd8917224e03a1 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 17:30:31 +0300 Subject: [PATCH 02/11] update account policy check before verifying policy status Signed-off-by: bcmmbaga --- management/server/policy.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/management/server/policy.go b/management/server/policy.go index cf695e5e2..055542430 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -461,15 +461,14 @@ func (am *DefaultAccountManager) savePolicy(account *Account, policyToSave *Poli } oldPolicy := account.Policies[policyIdx] + // Update the existing policy + account.Policies[policyIdx] = policyToSave + if !policyToSave.Enabled && !oldPolicy.Enabled { return false, nil } - updateAccountPeers := anyGroupHasPeers(account, oldPolicy.ruleGroups()) || anyGroupHasPeers(account, policyToSave.ruleGroups()) - // Update the existing policy - account.Policies[policyIdx] = policyToSave - return updateAccountPeers, nil } From a6dc54c21ab8716c42af178b376880e535b559ca Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Mon, 21 Oct 2024 19:01:02 +0300 Subject: [PATCH 03/11] Update management/server/route_test.go Co-authored-by: Maycon Santos --- management/server/route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 42f856aee..8b2890686 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1801,7 +1801,7 @@ func TestRouteAccountPeersUpdate(t *testing.T) { }) // Creating route no routing peer and no peers groups should not update account peers and not send peer update - t.Run("creating route no routing peer and no peers groups", func(t *testing.T) { + t.Run("creating route no routing peer and no peers in groups", func(t *testing.T) { route := route.Route{ ID: "testingRoute1", Network: netip.MustParsePrefix("100.65.250.202/32"), From 5df1973cafa6e513340c81b4baeacf1f6680ae13 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Mon, 21 Oct 2024 19:01:17 +0300 Subject: [PATCH 04/11] Update management/server/route_test.go Co-authored-by: Maycon Santos --- management/server/route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 8b2890686..77febf050 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1800,7 +1800,7 @@ func TestRouteAccountPeersUpdate(t *testing.T) { manager.peersUpdateManager.CloseChannel(context.Background(), peer1ID) }) - // Creating route no routing peer and no peers groups should not update account peers and not send peer update + // Creating a route with no routing peer and no peers in PeerGroups or Groups should not update account peers and not send peer update t.Run("creating route no routing peer and no peers in groups", func(t *testing.T) { route := route.Route{ ID: "testingRoute1", From 6a8cb2b0f9258b1ec8ad33b7ba510bdc82415396 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Mon, 21 Oct 2024 19:01:32 +0300 Subject: [PATCH 05/11] Update management/server/route_test.go Co-authored-by: Maycon Santos --- management/server/route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 77febf050..21d42d95d 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1836,7 +1836,7 @@ func TestRouteAccountPeersUpdate(t *testing.T) { }) - // Creating route no routing peer and has peers in groups should update account peers and send peer update + // Creating a route with no routing peer and having peers in groups should update account peers and send peer update t.Run("creating route no routing peer and has peers in groups", func(t *testing.T) { route := route.Route{ ID: "testingRoute2", From f891959d64b598c08f2558f8e76204b1562dfd46 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Mon, 21 Oct 2024 19:01:50 +0300 Subject: [PATCH 06/11] Update management/server/route_test.go Co-authored-by: Maycon Santos --- management/server/route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 21d42d95d..69f43bdcb 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1837,7 +1837,7 @@ func TestRouteAccountPeersUpdate(t *testing.T) { }) // Creating a route with no routing peer and having peers in groups should update account peers and send peer update - t.Run("creating route no routing peer and has peers in groups", func(t *testing.T) { + t.Run("creating a route with peers in PeerGroups and Groups", func(t *testing.T) { route := route.Route{ ID: "testingRoute2", Network: netip.MustParsePrefix("192.0.2.0/32"), From 4a9f755b13cce5d9106d97be54cbadc50d7e800c Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Mon, 21 Oct 2024 19:02:05 +0300 Subject: [PATCH 07/11] Update management/server/route_test.go Co-authored-by: Maycon Santos --- management/server/route_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 69f43bdcb..ef0503d45 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1908,7 +1908,7 @@ func TestRouteAccountPeersUpdate(t *testing.T) { } }) - // Updating the route should update account peers and send peer update + // Updating the route should update account peers and send peer update when there is peers in group t.Run("updating route", func(t *testing.T) { baseRoute.Groups = []string{routeGroup1, routeGroup2} From eb68e35f442be489979531680cb557ee97cdadf3 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 19:57:56 +0300 Subject: [PATCH 08/11] add tests missing tests for dns setting groups Signed-off-by: bcmmbaga --- management/server/dns_test.go | 66 +++++++++++++++++++++++++++++------ 1 file changed, 56 insertions(+), 10 deletions(-) diff --git a/management/server/dns_test.go b/management/server/dns_test.go index 5e671e41c..c675fc12c 100644 --- a/management/server/dns_test.go +++ b/management/server/dns_test.go @@ -479,13 +479,20 @@ func TestToProtocolDNSConfigWithCache(t *testing.T) { } } -func TestDNSAccountPeerUpdate(t *testing.T) { +func TestDNSAccountPeersUpdate(t *testing.T) { manager, account, peer1, peer2, peer3 := setupNetworkMapTest(t) - err := manager.SaveGroup(context.Background(), account.Id, userID, &group.Group{ - ID: "group-id", - Name: "GroupA", - Peers: []string{}, + err := manager.SaveGroups(context.Background(), account.Id, userID, []*group.Group{ + { + ID: "groupA", + Name: "GroupA", + Peers: []string{}, + }, + { + ID: "groupB", + Name: "GroupB", + Peers: []string{}, + }, }) assert.NoError(t, err) @@ -503,7 +510,7 @@ func TestDNSAccountPeerUpdate(t *testing.T) { }() err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ - DisabledManagementGroups: []string{"group-id"}, + DisabledManagementGroups: []string{"groupA"}, }) assert.NoError(t, err) @@ -515,7 +522,7 @@ func TestDNSAccountPeerUpdate(t *testing.T) { }) err = manager.SaveGroup(context.Background(), account.Id, userID, &group.Group{ - ID: "group-id", + ID: "groupA", Name: "GroupA", Peers: []string{peer1.ID, peer2.ID, peer3.ID}, }) @@ -527,7 +534,7 @@ func TestDNSAccountPeerUpdate(t *testing.T) { NSType: dns.UDPNameServerType, Port: dns.DefaultDNSPort, }}, - []string{"group-id"}, + []string{"groupA"}, true, []string{}, true, userID, false, ) assert.NoError(t, err) @@ -541,7 +548,7 @@ func TestDNSAccountPeerUpdate(t *testing.T) { }() err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ - DisabledManagementGroups: []string{"group-id"}, + DisabledManagementGroups: []string{"groupA", "groupB"}, }) assert.NoError(t, err) @@ -562,7 +569,7 @@ func TestDNSAccountPeerUpdate(t *testing.T) { }() err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ - DisabledManagementGroups: []string{"group-id"}, + DisabledManagementGroups: []string{"groupA", "groupB"}, }) assert.NoError(t, err) @@ -573,4 +580,43 @@ func TestDNSAccountPeerUpdate(t *testing.T) { } }) + // Removing group with no peers from DNS settings should not trigger updates to account peers or send peer updates + t.Run("removing group with no peers from dns settings", func(t *testing.T) { + done := make(chan struct{}) + go func() { + peerShouldNotReceiveUpdate(t, updMsg) + close(done) + }() + + err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ + DisabledManagementGroups: []string{"groupA"}, + }) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldNotReceiveUpdate") + } + }) + + // Removing group with peers from DNS settings should trigger updates to account peers and send peer updates + t.Run("removing group with peers from dns settings", func(t *testing.T) { + done := make(chan struct{}) + go func() { + peerShouldReceiveUpdate(t, updMsg) + close(done) + }() + + err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ + DisabledManagementGroups: []string{}, + }) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldReceiveUpdate") + } + }) } From 62899df75db4ec7b1981aa762305b7e9301d43fa Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 20:07:45 +0300 Subject: [PATCH 09/11] add tests for posture checks changes Signed-off-by: bcmmbaga --- management/server/posture_checks_test.go | 78 ++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/management/server/posture_checks_test.go b/management/server/posture_checks_test.go index 6adca3db1..ba67d112c 100644 --- a/management/server/posture_checks_test.go +++ b/management/server/posture_checks_test.go @@ -453,3 +453,81 @@ func TestPostureCheckAccountPeersUpdate(t *testing.T) { } }) } + +func TestArePostureCheckChangesAffectingPeers(t *testing.T) { + account := &Account{ + Policies: []*Policy{ + { + ID: "policyA", + Rules: []*PolicyRule{ + { + Enabled: true, + Sources: []string{"groupA"}, + Destinations: []string{"groupA"}, + }, + }, + SourcePostureChecks: []string{"checkA"}, + }, + }, + Groups: map[string]*group.Group{ + "groupA": { + ID: "groupA", + Peers: []string{"peer1"}, + }, + "groupB": { + ID: "groupB", + Peers: []string{}, + }, + }, + PostureChecks: []*posture.Checks{ + { + ID: "checkA", + }, + { + ID: "checkB", + }, + }, + } + + t.Run("posture check exists and is linked to policy with peers", func(t *testing.T) { + result := arePostureCheckChangesAffectingPeers(account, "checkA", true) + assert.True(t, result) + }) + + t.Run("posture check exists but is not linked to any policy", func(t *testing.T) { + result := arePostureCheckChangesAffectingPeers(account, "checkB", true) + assert.False(t, result) + }) + + t.Run("posture check does not exist", func(t *testing.T) { + result := arePostureCheckChangesAffectingPeers(account, "unknown", false) + assert.False(t, result) + }) + + t.Run("posture check is linked to policy with no peers in source groups", func(t *testing.T) { + account.Policies[0].Rules[0].Sources = []string{"groupB"} + account.Policies[0].Rules[0].Destinations = []string{"groupA"} + result := arePostureCheckChangesAffectingPeers(account, "checkA", true) + assert.True(t, result) + }) + + t.Run("posture check is linked to policy with no peers in destination groups", func(t *testing.T) { + account.Policies[0].Rules[0].Sources = []string{"groupA"} + account.Policies[0].Rules[0].Destinations = []string{"groupB"} + result := arePostureCheckChangesAffectingPeers(account, "checkA", true) + assert.True(t, result) + }) + + t.Run("posture check is linked to policy with non-existent group", func(t *testing.T) { + account.Policies[0].Rules[0].Sources = []string{"nonExistentGroup"} + account.Policies[0].Rules[0].Destinations = []string{"nonExistentGroup"} + result := arePostureCheckChangesAffectingPeers(account, "checkA", true) + assert.False(t, result) + }) + + t.Run("posture check is linked to policy but no peers in groups", func(t *testing.T) { + account.Groups["groupA"].Peers = []string{} + result := arePostureCheckChangesAffectingPeers(account, "checkA", true) + assert.False(t, result) + }) +} From 09c9f21a8bd6675f6acb48f7b24c444af3de9960 Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 20:35:33 +0300 Subject: [PATCH 10/11] add ns group and policy tests Signed-off-by: bcmmbaga --- management/server/nameserver_test.go | 18 ++++++++++++++ management/server/policy_test.go | 35 ++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/management/server/nameserver_test.go b/management/server/nameserver_test.go index 806125a3f..96637cd39 100644 --- a/management/server/nameserver_test.go +++ b/management/server/nameserver_test.go @@ -1094,4 +1094,22 @@ func TestNameServerAccountPeersUpdate(t *testing.T) { t.Error("timeout waiting for peerShouldNotReceiveUpdate") } }) + + // Deleting a nameserver group should update account peers and send peer update + t.Run("deleting nameserver group", func(t *testing.T) { + done := make(chan struct{}) + go func() { + peerShouldReceiveUpdate(t, updMsg) + close(done) + }() + + err = manager.DeleteNameServerGroup(context.Background(), account.Id, newNameServerGroupB.ID, userID) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldReceiveUpdate") + } + }) } diff --git a/management/server/policy_test.go b/management/server/policy_test.go index e213490fc..5b1411702 100644 --- a/management/server/policy_test.go +++ b/management/server/policy_test.go @@ -1035,6 +1035,41 @@ func TestPolicyAccountPeersUpdate(t *testing.T) { } }) + // Updating disabled policy with destination and source groups containing peers should not update account's peers + // or send peer update + t.Run("updating disabled policy with source and destination groups with peers", func(t *testing.T) { + policy := Policy{ + ID: "policy-source-destination-peers", + Description: "updated description", + Enabled: false, + Rules: []*PolicyRule{ + { + ID: xid.New().String(), + Enabled: true, + Sources: []string{"groupA"}, + Destinations: []string{"groupA"}, + Bidirectional: true, + Action: PolicyTrafficActionAccept, + }, + }, + } + + done := make(chan struct{}) + go func() { + peerShouldNotReceiveUpdate(t, updMsg1) + close(done) + }() + + err := manager.SavePolicy(context.Background(), account.Id, userID, &policy, true) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldNotReceiveUpdate") + } + }) + // Enabling policy with destination and source groups containing peers should update account's peers // and send peer update t.Run("enabling policy with source and destination groups with peers", func(t *testing.T) { From 356d3624c49ba69748e410e418914e5c10ed5dcf Mon Sep 17 00:00:00 2001 From: bcmmbaga Date: Mon, 21 Oct 2024 22:34:58 +0300 Subject: [PATCH 11/11] add route and group tests Signed-off-by: bcmmbaga --- management/server/group_test.go | 32 ++++++++++++ management/server/route_test.go | 89 +++++++++++++++++++++++++++++++++ 2 files changed, 121 insertions(+) diff --git a/management/server/group_test.go b/management/server/group_test.go index c5d6d2acf..1e59b74ef 100644 --- a/management/server/group_test.go +++ b/management/server/group_test.go @@ -407,6 +407,11 @@ func TestGroupAccountPeersUpdate(t *testing.T) { Name: "GroupC", Peers: []string{peer1.ID, peer3.ID}, }, + { + ID: "groupD", + Name: "GroupD", + Peers: []string{}, + }, }) assert.NoError(t, err) @@ -663,4 +668,31 @@ func TestGroupAccountPeersUpdate(t *testing.T) { t.Error("timeout waiting for peerShouldReceiveUpdate") } }) + + // Saving a group linked to dns settings should update account peers and send peer update + t.Run("saving group linked to dns settings", func(t *testing.T) { + err := manager.SaveDNSSettings(context.Background(), account.Id, userID, &DNSSettings{ + DisabledManagementGroups: []string{"groupD"}, + }) + assert.NoError(t, err) + + done := make(chan struct{}) + go func() { + peerShouldReceiveUpdate(t, updMsg) + close(done) + }() + + err = manager.SaveGroup(context.Background(), account.Id, userID, &nbgroup.Group{ + ID: "groupD", + Name: "GroupD", + Peers: []string{peer1.ID}, + }) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldReceiveUpdate") + } + }) } diff --git a/management/server/route_test.go b/management/server/route_test.go index ef0503d45..a4b320c7e 100644 --- a/management/server/route_test.go +++ b/management/server/route_test.go @@ -1792,6 +1792,16 @@ func TestRouteAccountPeersUpdate(t *testing.T) { Name: "GroupA", Peers: []string{}, }, + { + ID: "groupB", + Name: "GroupB", + Peers: []string{}, + }, + { + ID: "groupC", + Name: "GroupC", + Peers: []string{}, + }, }) assert.NoError(t, err) @@ -1966,4 +1976,83 @@ func TestRouteAccountPeersUpdate(t *testing.T) { } }) + // Adding peer to route peer groups that do not have any peers should update account peers and send peer update + t.Run("adding peer to route peer groups that do not have any peers", func(t *testing.T) { + newRoute := route.Route{ + Network: netip.MustParsePrefix("192.168.12.0/16"), + NetID: "superNet", + NetworkType: route.IPv4Network, + PeerGroups: []string{"groupB"}, + Description: "super", + Masquerade: false, + Metric: 9999, + Enabled: true, + Groups: []string{routeGroup1}, + } + _, err := manager.CreateRoute( + context.Background(), account.Id, newRoute.Network, newRoute.NetworkType, newRoute.Domains, newRoute.Peer, + newRoute.PeerGroups, newRoute.Description, newRoute.NetID, newRoute.Masquerade, newRoute.Metric, + newRoute.Groups, []string{}, true, userID, newRoute.KeepRoute, + ) + require.NoError(t, err) + + done := make(chan struct{}) + go func() { + peerShouldReceiveUpdate(t, updMsg) + close(done) + }() + + err = manager.SaveGroup(context.Background(), account.Id, userID, &nbgroup.Group{ + ID: "groupB", + Name: "GroupB", + Peers: []string{peer1ID}, + }) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldReceiveUpdate") + } + }) + + // Adding peer to route groups that do not have any peers should update account peers and send peer update + t.Run("adding peer to route groups that do not have any peers", func(t *testing.T) { + newRoute := route.Route{ + Network: netip.MustParsePrefix("192.168.13.0/16"), + NetID: "superNet", + NetworkType: route.IPv4Network, + PeerGroups: []string{"groupB"}, + Description: "super", + Masquerade: false, + Metric: 9999, + Enabled: true, + Groups: []string{"groupC"}, + } + _, err := manager.CreateRoute( + context.Background(), account.Id, newRoute.Network, newRoute.NetworkType, newRoute.Domains, newRoute.Peer, + newRoute.PeerGroups, newRoute.Description, newRoute.NetID, newRoute.Masquerade, newRoute.Metric, + newRoute.Groups, []string{}, true, userID, newRoute.KeepRoute, + ) + require.NoError(t, err) + + done := make(chan struct{}) + go func() { + peerShouldReceiveUpdate(t, updMsg) + close(done) + }() + + err = manager.SaveGroup(context.Background(), account.Id, userID, &nbgroup.Group{ + ID: "groupC", + Name: "GroupC", + Peers: []string{peer1ID}, + }) + assert.NoError(t, err) + + select { + case <-done: + case <-time.After(time.Second): + t.Error("timeout waiting for peerShouldReceiveUpdate") + } + }) }