From 7ee7ada27347bec20d0650a9c0feeb991779b2d4 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Fri, 20 Dec 2024 21:10:53 +0300 Subject: [PATCH] [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 * Add tests Signed-off-by: bcmmbaga --------- Signed-off-by: bcmmbaga --- management/server/route_test.go | 67 ++++++++++++++++++++++++++++++ management/server/types/account.go | 6 +-- 2 files changed, 69 insertions(+), 4 deletions(-) diff --git a/management/server/route_test.go b/management/server/route_test.go index 5390cb66b94..2ef2b01732c 100644 --- 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 index 5d0f1201984..b36b719e491 100644 --- 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)...) }