[management] Fix duplicate resource routes when routing peer is part …

…of the source group (#3095) * Remove duplicate resource routes when routing peer is part of the source group Signed-off-by: bcmmbaga <[email protected]> * Add tests Signed-off-by: bcmmbaga <[email protected]> --------- Signed-off-by: bcmmbaga <[email protected]>
netbirdio · Dec 20, 2024 · 7ee7ada · 7ee7ada
1 parent 82b4e58
commit 7ee7ada
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 4 deletions.
diff --git a/management/server/route_test.go b/management/server/route_test.go
@@ -2175,6 +2175,7 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 		peerJIp = "100.65.29.65"
 		peerKIp = "100.65.29.66"
 		peerMIp = "100.65.29.67"
+		peerOIp = "100.65.29.68"
 	)
 
 	account := &types.Account{
@@ -2256,6 +2257,20 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 				IP:     net.ParseIP(peerMIp),
 				Status: &nbpeer.PeerStatus{},
 			},
+			"peerN": {
+				ID:     "peerN",
+				IP:     net.ParseIP("100.65.20.18"),
+				Key:    "peerN",
+				Status: &nbpeer.PeerStatus{},
+				Meta: nbpeer.PeerSystemMeta{
+					GoOS: "linux",
+				},
+			},
+			"peerO": {
+				ID:     "peerO",
+				IP:     net.ParseIP(peerOIp),
+				Status: &nbpeer.PeerStatus{},
+			},
 		},
 		Groups: map[string]*types.Group{
 			"router1": {
@@ -2330,6 +2345,14 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 				Name:  "Pipeline",
 				Peers: []string{"peerM"},
 			},
+			"metrics": {
+				ID:    "metrics",
+				Name:  "Metrics",
+				Peers: []string{"peerN", "peerO"},
+				Resources: []types.Resource{
+					{ID: "resource6"},
+				},
+			},
 		},
 		Networks: []*networkTypes.Network{
 			{
@@ -2352,6 +2375,10 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 				ID:   "network5",
 				Name: "Pipeline Network",
 			},
+			{
+				ID:   "network6",
+				Name: "Metrics Network",
+			},
 		},
 		NetworkRouters: []*routerTypes.NetworkRouter{
 			{
@@ -2389,6 +2416,13 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 				Masquerade: false,
 				Metric:     9999,
 			},
+			{
+				ID:         "router6",
+				NetworkID:  "network6",
+				Peer:       "peerN",
+				Masquerade: false,
+				Metric:     9999,
+			},
 		},
 		NetworkResources: []*resourceTypes.NetworkResource{
 			{
@@ -2426,6 +2460,13 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 				Type:      "host",
 				Prefix:    netip.MustParsePrefix("10.12.12.1/32"),
 			},
+			{
+				ID:        "resource6",
+				NetworkID: "network6",
+				Name:      "Resource 6",
+				Type:      "domain",
+				Domain:    "*.google.com",
+			},
 		},
 		Policies: []*types.Policy{
 			{
@@ -2527,6 +2568,24 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 					},
 				},
 			},
+			{
+				ID:      "policyResource6",
+				Name:    "policyResource6",
+				Enabled: true,
+				Rules: []*types.PolicyRule{
+					{
+						ID:            "ruleResource6",
+						Name:          "ruleResource6",
+						Bidirectional: true,
+						Enabled:       true,
+						Protocol:      types.PolicyRuleProtocolTCP,
+						Action:        types.PolicyTrafficActionAccept,
+						Ports:         []string{"9090"},
+						Sources:       []string{"metrics"},
+						Destinations:  []string{"metrics"},
+					},
+				},
+			},
 		},
 	}
 
@@ -2553,6 +2612,10 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 		// which is part of the destination in the policies (policyResource3 and policyResource4)
 		policies = account.GetPoliciesForNetworkResource("resource4")
 		assert.Len(t, policies, 2, "resource4 should have exactly 2 policy applied via access control groups")
+
+		// Test case: Resource6 is applied to the access control groups (metrics),
+		policies = account.GetPoliciesForNetworkResource("resource6")
+		assert.Len(t, policies, 1, "resource6 should have exactly 1 policy applied via access control groups")
 	})
 
 	t.Run("validate routing peer firewall rules for network resources", func(t *testing.T) {
@@ -2663,5 +2726,9 @@ func TestAccount_GetPeerNetworkResourceFirewallRules(t *testing.T) {
 		_, routes, sourcePeers = account.GetNetworkResourcesRoutesToSync(context.Background(), "peerM", resourcePoliciesMap, resourceRoutersMap)
 		assert.Len(t, routes, 1)
 		assert.Len(t, sourcePeers, 0)
+
+		_, routes, sourcePeers = account.GetNetworkResourcesRoutesToSync(context.Background(), "peerN", resourcePoliciesMap, resourceRoutersMap)
+		assert.Len(t, routes, 1)
+		assert.Len(t, sourcePeers, 2)
 	})
 }
diff --git a/management/server/types/account.go b/management/server/types/account.go
@@ -1330,10 +1330,8 @@ func (a *Account) GetNetworkResourcesRoutesToSync(ctx context.Context, peerID st
 				// routing peer should be able to connect with all source peers
 				if addSourcePeers {
 					allSourcePeers = append(allSourcePeers, group.Peers...)
-				}
-
-				// add routes for the resource if the peer is in the distribution group
-				if slices.Contains(group.Peers, peerID) {
+				} else if slices.Contains(group.Peers, peerID) {
+					// add routes for the resource if the peer is in the distribution group
 					for peerId, router := range networkRoutingPeers {
 						routes = append(routes, a.getNetworkResourcesRoutes(resource, peerId, router, resourcePolicies)...)
 					}