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: 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") + } + }) } 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/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.go b/management/server/policy.go index 9b5242b17..12cfbd0db 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -463,15 +463,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 } 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) { 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) + }) +} diff --git a/management/server/route_test.go b/management/server/route_test.go index 42f856aee..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) @@ -1800,8 +1810,8 @@ 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 - t.Run("creating route no routing peer and no peers groups", func(t *testing.T) { + // 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", Network: netip.MustParsePrefix("100.65.250.202/32"), @@ -1836,8 +1846,8 @@ func TestRouteAccountPeersUpdate(t *testing.T) { }) - // Creating route no routing peer and has 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) { + // Creating a route with no routing peer and having peers in groups should update account peers and send peer update + 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"), @@ -1908,7 +1918,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} @@ -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") + } + }) }