Merge branch 'feature/validate-group-association' into feature/validate-group-association-debug

.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:

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",
+	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")
+		}
+	})
 }

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")
+		}
+	})
 }

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")
+		}
+	})
 }

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
 	}
 

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) {

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)
+	})
+}

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")
+		}
+	})
 }