From 4e47f53a84fe03d17fcb6c81a8e41e90d7d10717 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Wed, 17 Jul 2024 20:46:18 +0700 Subject: [PATCH 01/16] add a unit test for ipam issue Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server_test.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index 417497a4d..d2ffeddb6 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -536,3 +536,39 @@ func TestRecoveryServers(t *testing.T) { require.NoError(t, err) validateConns(t, conn3, []string{"192.168.10.0/32", "fe80::fa00/128"}, []string{"192.168.10.1/32", "fe80::fa01/128"}) } + +func TestRaceCond(t *testing.T) { + + _, ipNet, err := net.ParseCIDR("172.16.0.0/24") + require.NoError(t, err) + + srv := newIpamServer(ipNet) + + emptyRequest := newRequest() + emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = append(emptyRequest.Connection.Context.IpContext.ExcludedPrefixes, "10.96.0.0/16", "10.244.0.0/16") + + request := newRequest() + + request.Connection.Context.IpContext.SrcIpAddrs = append(request.Connection.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") + request.Connection.Context.IpContext.SrcIpAddrs = append(request.Connection.Context.IpContext.SrcIpAddrs, "172.16.0.25/32") + + request.Connection.Context.IpContext.DstIpAddrs = append(request.Connection.Context.IpContext.DstIpAddrs, "172.16.0.0/32") + request.Connection.Context.IpContext.DstIpAddrs = append(request.Connection.Context.IpContext.DstIpAddrs, "172.16.0.24/32") + + request.Connection.Context.IpContext.SrcRoutes = append(request.Connection.Context.IpContext.SrcRoutes, &networkservice.Route{Prefix: "172.16.0.2/32"}) + request.Connection.Context.IpContext.SrcRoutes = append(request.Connection.Context.IpContext.SrcRoutes, &networkservice.Route{Prefix: "172.16.0.24/32"}) + + request.Connection.Context.IpContext.DstRoutes = append(request.Connection.Context.IpContext.DstRoutes, &networkservice.Route{Prefix: "172.16.0.3/32"}) + request.Connection.Context.IpContext.DstRoutes = append(request.Connection.Context.IpContext.DstRoutes, &networkservice.Route{Prefix: "172.16.0.25/32"}) + + request.Connection.Context.IpContext.ExcludedPrefixes = append(request.Connection.Context.IpContext.ExcludedPrefixes, "10.96.0.0/16", "10.244.0.0/16", "100:100::1/128", "100:100::/128") + + conn, err := srv.Request(context.Background(), emptyRequest) + require.NoError(t, err) + validateConn(t, conn, "172.16.0.0/32", "172.16.0.1/32") + + conn, err = srv.Request(context.Background(), request) + require.NoError(t, err) + require.NotContains(t, conn.Context.IpContext.DstIpAddrs, "172.16.0.0/32") + require.NotContains(t, conn.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") +} From 0c6af3749efacbdd74e26003e0ca7beb0a04e6e5 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Wed, 17 Jul 2024 21:10:51 +0700 Subject: [PATCH 02/16] add fix for ipam Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/point2pointipam/server.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 6e83498b9..60ca6eab1 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -139,14 +139,18 @@ func (s *ipamServer) recoverAddrs(srcAddrs, dstAddrs []string, excludeIP4, exclu } for _, ipPool := range s.ipPools { var srcAddr, dstAddr *net.IPNet - for _, addr := range srcAddrs { + for i, addr := range srcAddrs { if srcAddr, err = ipPool.PullIPString(addr, excludeIP4, excludeIP6); err == nil { break + } else { + srcAddrs = append(srcAddrs[:i], srcAddrs[i+1:]...) } } - for _, addr := range dstAddrs { + for i, addr := range dstAddrs { if dstAddr, err = ipPool.PullIPString(addr, excludeIP4, excludeIP6); err == nil { break + } else { + dstAddrs = append(dstAddrs[:i], dstAddrs[i+1:]...) } } if srcAddr != nil && dstAddr != nil { From 79333243e514d36d0d56d6795bfdd1f6b7419e3b Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Wed, 17 Jul 2024 23:13:49 +0700 Subject: [PATCH 03/16] another fix Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server.go | 3 ++ .../ipam/point2pointipam/server_test.go | 40 +++++++++++++------ pkg/tools/ippool/ippool.go | 4 ++ 3 files changed, 35 insertions(+), 12 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 60ca6eab1..9967ae80e 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -116,6 +116,8 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ s.Store(conn.GetId(), connInfo) } + ipContext = &networkservice.IPContext{} + addAddr(&ipContext.SrcIpAddrs, connInfo.srcAddr) addRoute(&ipContext.SrcRoutes, connInfo.dstAddr) @@ -140,6 +142,7 @@ func (s *ipamServer) recoverAddrs(srcAddrs, dstAddrs []string, excludeIP4, exclu for _, ipPool := range s.ipPools { var srcAddr, dstAddr *net.IPNet for i, addr := range srcAddrs { + if srcAddr, err = ipPool.PullIPString(addr, excludeIP4, excludeIP6); err == nil { break } else { diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index d2ffeddb6..50a188a98 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -537,31 +537,47 @@ func TestRecoveryServers(t *testing.T) { validateConns(t, conn3, []string{"192.168.10.0/32", "fe80::fa00/128"}, []string{"192.168.10.1/32", "fe80::fa01/128"}) } -func TestRaceCond(t *testing.T) { - +func TestOverlappingAddresses(t *testing.T) { _, ipNet, err := net.ParseCIDR("172.16.0.0/24") require.NoError(t, err) srv := newIpamServer(ipNet) emptyRequest := newRequest() - emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = append(emptyRequest.Connection.Context.IpContext.ExcludedPrefixes, "10.96.0.0/16", "10.244.0.0/16") + emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} request := newRequest() + request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32", "172.16.0.25/32"} + request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32", "172.16.0.24/32"} + request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.2/32"}, {Prefix: "172.16.0.24/32"}} + request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.0.3/32"}, {Prefix: "172.16.0.25/32"}} + request.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} - request.Connection.Context.IpContext.SrcIpAddrs = append(request.Connection.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") - request.Connection.Context.IpContext.SrcIpAddrs = append(request.Connection.Context.IpContext.SrcIpAddrs, "172.16.0.25/32") + conn, err := srv.Request(context.Background(), emptyRequest) + require.NoError(t, err) + validateConn(t, conn, "172.16.0.0/32", "172.16.0.1/32") - request.Connection.Context.IpContext.DstIpAddrs = append(request.Connection.Context.IpContext.DstIpAddrs, "172.16.0.0/32") - request.Connection.Context.IpContext.DstIpAddrs = append(request.Connection.Context.IpContext.DstIpAddrs, "172.16.0.24/32") + conn, err = srv.Request(context.Background(), request) + require.NoError(t, err) + require.NotContains(t, conn.Context.IpContext.DstIpAddrs, "172.16.0.0/32") + require.NotContains(t, conn.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") +} - request.Connection.Context.IpContext.SrcRoutes = append(request.Connection.Context.IpContext.SrcRoutes, &networkservice.Route{Prefix: "172.16.0.2/32"}) - request.Connection.Context.IpContext.SrcRoutes = append(request.Connection.Context.IpContext.SrcRoutes, &networkservice.Route{Prefix: "172.16.0.24/32"}) +func TestOverlappingAddresses2(t *testing.T) { + _, ipNet, err := net.ParseCIDR("172.16.0.0/24") + require.NoError(t, err) - request.Connection.Context.IpContext.DstRoutes = append(request.Connection.Context.IpContext.DstRoutes, &networkservice.Route{Prefix: "172.16.0.3/32"}) - request.Connection.Context.IpContext.DstRoutes = append(request.Connection.Context.IpContext.DstRoutes, &networkservice.Route{Prefix: "172.16.0.25/32"}) + srv := newIpamServer(ipNet) - request.Connection.Context.IpContext.ExcludedPrefixes = append(request.Connection.Context.IpContext.ExcludedPrefixes, "10.96.0.0/16", "10.244.0.0/16", "100:100::1/128", "100:100::/128") + emptyRequest := newRequest() + emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} + + request := newRequest() + request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32"} + request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32"} + request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.0/32"}} + request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.0.1/32"}} + request.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} conn, err := srv.Request(context.Background(), emptyRequest) require.NoError(t, err) diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index 144dd4f38..c91265a73 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -238,6 +238,10 @@ func (tree *IPPool) PullIPString(ipString string, exclude ...*IPPool) (*net.IPNe return nil, errors.Wrapf(err, "failed to parse %s as a CIDR", ipString) } + if len(ip) != tree.ipLength { + return nil, errors.Errorf("length of %s is not equal to tree's ip length", ipString) + } + return tree.PullIP(ip, exclude...) } From edda11a069f559f4d9f5caef215ba13b7e3e4225 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Thu, 18 Jul 2024 16:56:16 +0700 Subject: [PATCH 04/16] add ip context validation Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server.go | 30 +++++++++++++------ pkg/tools/ippool/ippool.go | 16 +++++++--- 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 9967ae80e..5d437e322 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -95,6 +95,8 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ excludeIP4, excludeIP6 := exclude(ipContext.GetExcludedPrefixes()...) + s.validateIPContext(ipContext, excludeIP4, excludeIP6) + connInfo, loaded := s.Load(conn.GetId()) var err error if loaded && (connInfo.shouldUpdate(excludeIP4) || connInfo.shouldUpdate(excludeIP6)) { @@ -116,8 +118,6 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ s.Store(conn.GetId(), connInfo) } - ipContext = &networkservice.IPContext{} - addAddr(&ipContext.SrcIpAddrs, connInfo.srcAddr) addRoute(&ipContext.SrcRoutes, connInfo.dstAddr) @@ -135,25 +135,37 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ return conn, nil } +func (s *ipamServer) validateIPContext(ipContext *networkservice.IPContext, excludeIP4, excludeIP6 *ippool.IPPool) { + for _, ipPool := range s.ipPools { + for _, addr := range ipContext.SrcIpAddrs { + if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { + deleteAddr(&ipContext.SrcIpAddrs, addr) + deleteRoute(&ipContext.DstRoutes, addr) + } + } + for _, addr := range ipContext.DstIpAddrs { + if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { + deleteAddr(&ipContext.DstIpAddrs, addr) + deleteRoute(&ipContext.SrcRoutes, addr) + } + } + } +} + func (s *ipamServer) recoverAddrs(srcAddrs, dstAddrs []string, excludeIP4, excludeIP6 *ippool.IPPool) (connInfo *connectionInfo, err error) { if len(srcAddrs) == 0 || len(dstAddrs) == 0 { return nil, errors.New("addresses cannot be empty for recovery") } for _, ipPool := range s.ipPools { var srcAddr, dstAddr *net.IPNet - for i, addr := range srcAddrs { - + for _, addr := range srcAddrs { if srcAddr, err = ipPool.PullIPString(addr, excludeIP4, excludeIP6); err == nil { break - } else { - srcAddrs = append(srcAddrs[:i], srcAddrs[i+1:]...) } } - for i, addr := range dstAddrs { + for _, addr := range dstAddrs { if dstAddr, err = ipPool.PullIPString(addr, excludeIP4, excludeIP6); err == nil { break - } else { - dstAddrs = append(dstAddrs[:i], dstAddrs[i+1:]...) } } if srcAddr != nil && dstAddr != nil { diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index c91265a73..c1bfa0f8e 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -22,6 +22,7 @@ package ippool import ( "math" "net" + "strings" "sync" "github.com/pkg/errors" @@ -238,10 +239,6 @@ func (tree *IPPool) PullIPString(ipString string, exclude ...*IPPool) (*net.IPNe return nil, errors.Wrapf(err, "failed to parse %s as a CIDR", ipString) } - if len(ip) != tree.ipLength { - return nil, errors.Errorf("length of %s is not equal to tree's ip length", ipString) - } - return tree.PullIP(ip, exclude...) } @@ -338,6 +335,17 @@ func (tree *IPPool) GetPrefixes() []string { return prefixes } +func (tree *IPPool) Belongs(ipString string) bool { + if strings.Count(ipString, ":") < 2 && tree.ipLength == net.IPv4len { + return true + } + if strings.Count(ipString, ":") >= 2 && tree.ipLength == net.IPv6len { + return true + } + + return false +} + func (tree *IPPool) excludePool(exclude *IPPool) { if exclude == nil { return From ebee3804905c65e60657b874548acbbb298a7c8e Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Mon, 22 Jul 2024 22:03:00 +1100 Subject: [PATCH 05/16] properly delete addresses Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server.go | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 5d437e322..057c6a678 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -136,20 +136,30 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ } func (s *ipamServer) validateIPContext(ipContext *networkservice.IPContext, excludeIP4, excludeIP6 *ippool.IPPool) { + srcAddrsToDelete := make([]string, 0) + dstAddrsToDelete := make([]string, 0) for _, ipPool := range s.ipPools { for _, addr := range ipContext.SrcIpAddrs { if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { - deleteAddr(&ipContext.SrcIpAddrs, addr) - deleteRoute(&ipContext.DstRoutes, addr) + srcAddrsToDelete = append(srcAddrsToDelete, addr) } } for _, addr := range ipContext.DstIpAddrs { if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { - deleteAddr(&ipContext.DstIpAddrs, addr) - deleteRoute(&ipContext.SrcRoutes, addr) + dstAddrsToDelete = append(dstAddrsToDelete, addr) } } } + + for _, addr := range srcAddrsToDelete { + deleteAddr(&ipContext.SrcIpAddrs, addr) + deleteRoute(&ipContext.DstRoutes, addr) + } + + for _, addr := range dstAddrsToDelete { + deleteAddr(&ipContext.DstIpAddrs, addr) + deleteRoute(&ipContext.SrcRoutes, addr) + } } func (s *ipamServer) recoverAddrs(srcAddrs, dstAddrs []string, excludeIP4, excludeIP6 *ippool.IPPool) (connInfo *connectionInfo, err error) { From ed5ad4eaa5992e724f2e25d160555f731748a496 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Tue, 23 Jul 2024 16:52:16 +1100 Subject: [PATCH 06/16] rework ip context validation Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server.go | 51 ++++++++++++++----- pkg/tools/ippool/ippool.go | 12 +---- 2 files changed, 41 insertions(+), 22 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 057c6a678..2fb49f232 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -22,6 +22,7 @@ package point2pointipam import ( "context" "net" + "net/netip" "sync" "github.com/edwarnicke/genericsync" @@ -135,28 +136,54 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ return conn, nil } -func (s *ipamServer) validateIPContext(ipContext *networkservice.IPContext, excludeIP4, excludeIP6 *ippool.IPPool) { - srcAddrsToDelete := make([]string, 0) - dstAddrsToDelete := make([]string, 0) - for _, ipPool := range s.ipPools { - for _, addr := range ipContext.SrcIpAddrs { - if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { - srcAddrsToDelete = append(srcAddrsToDelete, addr) +func addrBelongsToIPPool(ipPool *ippool.IPPool, addr netip.Addr) bool { + ipLen := ipPool.IPLength() + return (ipLen == net.IPv4len && addr.Is4()) || (ipLen == net.IPv6len && addr.Is6()) +} + +func (s *ipamServer) getInvalidAddrs(addrs []string, excludeIP4, excludeIP6 *ippool.IPPool) []string { + invalidAddrs := make([]string, 0) + for _, ipString := range addrs { + ip, parseErr := netip.ParsePrefix(ipString) + if parseErr != nil { + invalidAddrs = append(invalidAddrs, ipString) + continue + } + + versionMatches := false + for _, ipPool := range s.ipPools { + if addrBelongsToIPPool(ipPool, ip.Addr()) { + versionMatches = true + break } } - for _, addr := range ipContext.DstIpAddrs { - if _, err := ipPool.PullIPString(addr, excludeIP4, excludeIP6); err != nil && ipPool.Belongs(addr) { - dstAddrsToDelete = append(dstAddrsToDelete, addr) + if !versionMatches { + continue + } + + valid := false + for _, ipPool := range s.ipPools { + if _, err := ipPool.PullIPString(ipString, excludeIP4, excludeIP6); err == nil { + valid = true + break } } + + if !valid { + invalidAddrs = append(invalidAddrs, ipString) + } } - for _, addr := range srcAddrsToDelete { + return invalidAddrs +} + +func (s *ipamServer) validateIPContext(ipContext *networkservice.IPContext, excludeIP4, excludeIP6 *ippool.IPPool) { + for _, addr := range s.getInvalidAddrs(ipContext.SrcIpAddrs, excludeIP4, excludeIP6) { deleteAddr(&ipContext.SrcIpAddrs, addr) deleteRoute(&ipContext.DstRoutes, addr) } - for _, addr := range dstAddrsToDelete { + for _, addr := range s.getInvalidAddrs(ipContext.DstIpAddrs, excludeIP4, excludeIP6) { deleteAddr(&ipContext.DstIpAddrs, addr) deleteRoute(&ipContext.SrcRoutes, addr) } diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index c1bfa0f8e..b01583b8e 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -22,7 +22,6 @@ package ippool import ( "math" "net" - "strings" "sync" "github.com/pkg/errors" @@ -335,15 +334,8 @@ func (tree *IPPool) GetPrefixes() []string { return prefixes } -func (tree *IPPool) Belongs(ipString string) bool { - if strings.Count(ipString, ":") < 2 && tree.ipLength == net.IPv4len { - return true - } - if strings.Count(ipString, ":") >= 2 && tree.ipLength == net.IPv6len { - return true - } - - return false +func (tree *IPPool) IPLength() int { + return tree.ipLength } func (tree *IPPool) excludePool(exclude *IPPool) { From 0c1f7601ed5141d1bf298335a4fc1e9b854e8323 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Tue, 23 Jul 2024 16:56:22 +1100 Subject: [PATCH 07/16] temporarily skip failing tests Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/point2pointipam/server_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index 50a188a98..e957b3d2d 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -428,6 +428,7 @@ func TestRefreshRequestMultiServer(t *testing.T) { //nolint:dupl func TestRecoveryServer(t *testing.T) { + t.Skip() _, ipNet, err := net.ParseCIDR("192.168.3.4/16") require.NoError(t, err) @@ -464,6 +465,8 @@ func TestRecoveryServer(t *testing.T) { //nolint:dupl func TestRecoveryServerIPv6(t *testing.T) { + t.Skip() + _, ipNet, err := net.ParseCIDR("fe80::/64") require.NoError(t, err) @@ -499,6 +502,7 @@ func TestRecoveryServerIPv6(t *testing.T) { //nolint:dupl func TestRecoveryServers(t *testing.T) { + t.Skip() _, ipNet1, err := net.ParseCIDR("192.168.3.4/16") require.NoError(t, err) _, ipNet2, err := net.ParseCIDR("fe80::/64") From a0975880e8b45bc9a603f6159dee977fafeabd69 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Tue, 23 Jul 2024 17:01:12 +1100 Subject: [PATCH 08/16] fix CI issues Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/point2pointipam/server.go | 2 +- pkg/networkservice/ipam/point2pointipam/server_test.go | 2 +- pkg/tools/ippool/ippool.go | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 2fb49f232..938118f83 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -1,6 +1,6 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2022-2023 Cisco and/or its affiliates. +// Copyright (c) 2022-2024 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index e957b3d2d..f35db60dd 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -1,6 +1,6 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2022 Cisco and/or its affiliates. +// Copyright (c) 2022-2024 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index b01583b8e..0c299f1a9 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -1,6 +1,6 @@ // Copyright (c) 2021-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2022-2023 Cisco and/or its affiliates. +// Copyright (c) 2022-2024 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -334,6 +334,7 @@ func (tree *IPPool) GetPrefixes() []string { return prefixes } +// IPLength returns the length of ip addresses allocated by IP Pool func (tree *IPPool) IPLength() int { return tree.ipLength } From 9118e534d3f1edecd12700e99b44d0085363546b Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Tue, 13 Aug 2024 14:25:32 +1100 Subject: [PATCH 09/16] fix all tests Signed-off-by: NikitaSkrynnik --- .../ipam/point2pointipam/server.go | 13 +++---- .../ipam/point2pointipam/server_test.go | 36 ++----------------- pkg/tools/ippool/ippool.go | 4 ++- 3 files changed, 13 insertions(+), 40 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 938118f83..34f58a884 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -143,16 +143,16 @@ func addrBelongsToIPPool(ipPool *ippool.IPPool, addr netip.Addr) bool { func (s *ipamServer) getInvalidAddrs(addrs []string, excludeIP4, excludeIP6 *ippool.IPPool) []string { invalidAddrs := make([]string, 0) - for _, ipString := range addrs { - ip, parseErr := netip.ParsePrefix(ipString) + for _, prefixString := range addrs { + prefix, parseErr := netip.ParsePrefix(prefixString) if parseErr != nil { - invalidAddrs = append(invalidAddrs, ipString) + invalidAddrs = append(invalidAddrs, prefixString) continue } versionMatches := false for _, ipPool := range s.ipPools { - if addrBelongsToIPPool(ipPool, ip.Addr()) { + if addrBelongsToIPPool(ipPool, prefix.Addr()) { versionMatches = true break } @@ -161,16 +161,17 @@ func (s *ipamServer) getInvalidAddrs(addrs []string, excludeIP4, excludeIP6 *ipp continue } + addrString := prefix.Addr().String() valid := false for _, ipPool := range s.ipPools { - if _, err := ipPool.PullIPString(ipString, excludeIP4, excludeIP6); err == nil { + if ipPool.ContainsString(addrString) && !excludeIP4.ContainsString(addrString) && !excludeIP6.ContainsString(addrString) { valid = true break } } if !valid { - invalidAddrs = append(invalidAddrs, ipString) + invalidAddrs = append(invalidAddrs, prefixString) } } diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index f35db60dd..cfd3fc68e 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -428,7 +428,6 @@ func TestRefreshRequestMultiServer(t *testing.T) { //nolint:dupl func TestRecoveryServer(t *testing.T) { - t.Skip() _, ipNet, err := net.ParseCIDR("192.168.3.4/16") require.NoError(t, err) @@ -451,7 +450,7 @@ func TestRecoveryServer(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"192.168.10.0/32", "192.168.0.0/32"}, []string{"192.168.10.1/32", "192.168.0.1/32"}) + validateConns(t, conn2, []string{"192.168.0.0/32"}, []string{"192.168.0.1/32"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) @@ -465,8 +464,6 @@ func TestRecoveryServer(t *testing.T) { //nolint:dupl func TestRecoveryServerIPv6(t *testing.T) { - t.Skip() - _, ipNet, err := net.ParseCIDR("fe80::/64") require.NoError(t, err) @@ -488,7 +485,7 @@ func TestRecoveryServerIPv6(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"fe80::fa00/128", "fe80::/128"}, []string{"fe80::fa01/128", "fe80::1/128"}) + validateConns(t, conn2, []string{"fe80::/128"}, []string{"fe80::1/128"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) @@ -502,7 +499,6 @@ func TestRecoveryServerIPv6(t *testing.T) { //nolint:dupl func TestRecoveryServers(t *testing.T) { - t.Skip() _, ipNet1, err := net.ParseCIDR("192.168.3.4/16") require.NoError(t, err) _, ipNet2, err := net.ParseCIDR("fe80::/64") @@ -529,7 +525,7 @@ func TestRecoveryServers(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"192.168.10.0/32", "fe80::fa00/128", "192.168.0.0/32", "fe80::/128"}, []string{"192.168.10.1/32", "fe80::fa01/128", "192.168.0.1/32", "fe80::1/128"}) + validateConns(t, conn2, []string{"192.168.0.0/32", "fe80::/128"}, []string{"192.168.0.1/32", "fe80::1/128"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) @@ -566,29 +562,3 @@ func TestOverlappingAddresses(t *testing.T) { require.NotContains(t, conn.Context.IpContext.DstIpAddrs, "172.16.0.0/32") require.NotContains(t, conn.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") } - -func TestOverlappingAddresses2(t *testing.T) { - _, ipNet, err := net.ParseCIDR("172.16.0.0/24") - require.NoError(t, err) - - srv := newIpamServer(ipNet) - - emptyRequest := newRequest() - emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} - - request := newRequest() - request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32"} - request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32"} - request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.0/32"}} - request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.0.1/32"}} - request.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} - - conn, err := srv.Request(context.Background(), emptyRequest) - require.NoError(t, err) - validateConn(t, conn, "172.16.0.0/32", "172.16.0.1/32") - - conn, err = srv.Request(context.Background(), request) - require.NoError(t, err) - require.NotContains(t, conn.Context.IpContext.DstIpAddrs, "172.16.0.0/32") - require.NotContains(t, conn.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") -} diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index 0c299f1a9..b4a8bf45b 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -192,7 +192,9 @@ func (tree *IPPool) Contains(ip net.IP) bool { return tree.lookup(ipAddressFromIP(ip)) != nil } -// ContainsString - check the pool contains ip by string value +// ContainsString - check the pool contains ip by string value. +// Returns true if pool can allocate address stored in string, +// otherwise returns false func (tree *IPPool) ContainsString(in string) bool { return tree.Contains(net.ParseIP(in)) } From 81deeea76a886f9a676e3e73f78b0b76831e69a1 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 11:28:15 +1100 Subject: [PATCH 10/16] make a wrapper for p2p ipam that filters invalid addrs and routes Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/filteripam/server.go | 192 ++++++++++++++++++ .../ipam/filteripam/server_test.go | 80 ++++++++ .../ipam/point2pointipam/server.go | 57 ------ .../ipam/point2pointipam/server_test.go | 26 --- pkg/networkservice/ipam/strictipam/server.go | 14 +- 5 files changed, 285 insertions(+), 84 deletions(-) create mode 100644 pkg/networkservice/ipam/filteripam/server.go create mode 100644 pkg/networkservice/ipam/filteripam/server_test.go diff --git a/pkg/networkservice/ipam/filteripam/server.go b/pkg/networkservice/ipam/filteripam/server.go new file mode 100644 index 000000000..38e5f4549 --- /dev/null +++ b/pkg/networkservice/ipam/filteripam/server.go @@ -0,0 +1,192 @@ +// Copyright (c) 2024 Cisco and its affiliates. +// +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Package strictipam provides a networkservice.NetworkService Server chain element for building an IPAM server that +// filters some invalid addresses and routes in IP context +package filteripam + +import ( + "context" + "net" + "net/netip" + + "github.com/golang/protobuf/ptypes/empty" + "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" + "github.com/networkservicemesh/sdk/pkg/tools/ippool" +) + +type filterIPAMServer struct { + ipPool *ippool.IPPool +} + +func NewServer(newIPAMServer func(...*net.IPNet) networkservice.NetworkServiceServer, prefixes ...*net.IPNet) networkservice.NetworkServiceServer { + if newIPAMServer == nil { + panic("newIPAMServer should not be nil") + } + ipPool := ippool.New(net.IPv6len) + for _, p := range prefixes { + ipPool.AddNet(ipNetToIpv6Net(p)) + } + return next.NewNetworkServiceServer( + &filterIPAMServer{ipPool: ipPool}, + newIPAMServer(prefixes...), + ) +} + +func (s *filterIPAMServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { + s.validateIPContext(request.Connection.Context.IpContext) + conn, err := next.Server(ctx).Request(ctx, request) + if err != nil { + return nil, err + } + + s.pullAddrs(conn.Context.IpContext) + return conn, nil +} + +func (s *filterIPAMServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { + s.free(conn.Context.IpContext) + return next.Server(ctx).Close(ctx, conn) +} + +func ipNetToIpv6Net(ipNet *net.IPNet) *net.IPNet { + if len(ipNet.IP) == net.IPv6len { + return ipNet + } + ipv6Net := new(net.IPNet) + ipv6Net.IP = ipNet.IP.To16() + ipv6Net.Mask = make([]byte, 16) + copy(ipv6Net.Mask[12:], ipNet.Mask) + + return ipv6Net +} + +func (s *filterIPAMServer) getInvalidAddrs(addrs []string) []string { + invalidAddrs := make([]string, 0) + for _, prefixString := range addrs { + prefix, parseErr := netip.ParsePrefix(prefixString) + if parseErr != nil { + invalidAddrs = append(invalidAddrs, prefixString) + continue + } + + // versionMatches := false + // for _, ipPool := range s.ipPools { + // if addrBelongsToIPPool(ipPool, prefix.Addr()) { + // versionMatches = true + // break + // } + // } + // if !versionMatches { + // continue + // } + + // addrString := prefix.Addr().String() + // valid := false + // for _, ipPool := range s.ipPools { + // if ipPool.ContainsString(addrString) { + // valid = true + // break + // } + // } + + if !s.ipPool.ContainsString(prefix.Addr().String()) { + invalidAddrs = append(invalidAddrs, prefixString) + + } + + // if !valid { + // invalidAddrs = append(invalidAddrs, prefixString) + // } + } + + return invalidAddrs +} + +func (s *filterIPAMServer) validateIPContext(ipContext *networkservice.IPContext) { + for _, addr := range s.getInvalidAddrs(ipContext.SrcIpAddrs) { + deleteAddr(&ipContext.SrcIpAddrs, addr) + deleteRoute(&ipContext.DstRoutes, addr) + } + + for _, addr := range s.getInvalidAddrs(ipContext.DstIpAddrs) { + deleteAddr(&ipContext.DstIpAddrs, addr) + deleteRoute(&ipContext.SrcRoutes, addr) + } +} + +func deleteRoute(routes *[]*networkservice.Route, prefix string) { + for i, route := range *routes { + if route.Prefix == prefix { + *routes = append((*routes)[:i], (*routes)[i+1:]...) + return + } + } +} + +func deleteAddr(addrs *[]string, addr string) { + for i, a := range *addrs { + if a == addr { + *addrs = append((*addrs)[:i], (*addrs)[i+1:]...) + return + } + } +} + +func (s *filterIPAMServer) pullAddrs(ipContext *networkservice.IPContext) { + // for _, addr := range ipContext.SrcIpAddrs { + // for _, ippool := range s.ipPools { + // if _, err := ippool.PullIPString(addr); err == nil { + // break + // } + // } + // } + + // for _, addr := range ipContext.DstIpAddrs { + // for _, ippool := range s.ipPools { + // if _, err := ippool.PullIPString(addr); err == nil { + // break + // } + // } + // } + + for _, addr := range ipContext.SrcIpAddrs { + s.ipPool.PullIPString(addr) + } + + for _, addr := range ipContext.DstIpAddrs { + s.ipPool.PullIPString(addr) + } +} + +func (s *filterIPAMServer) free(ipContext *networkservice.IPContext) { + for _, addr := range ipContext.SrcIpAddrs { + _, ipNet, err := net.ParseCIDR(addr) + if err != nil { + return + } + s.ipPool.AddNet(ipNetToIpv6Net(ipNet)) + } + + for _, addr := range ipContext.DstIpAddrs { + _, ipNet, err := net.ParseCIDR(addr) + if err != nil { + return + } + s.ipPool.AddNet(ipNetToIpv6Net(ipNet)) + } +} diff --git a/pkg/networkservice/ipam/filteripam/server_test.go b/pkg/networkservice/ipam/filteripam/server_test.go new file mode 100644 index 000000000..6dfe40ce6 --- /dev/null +++ b/pkg/networkservice/ipam/filteripam/server_test.go @@ -0,0 +1,80 @@ +// Copyright (c) 2024 Cisco and/or its affiliates. +// +// SPDX-License-Identifier: Apache-2.0 +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at: +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package filteripam_test + +import ( + "context" + "net" + "testing" + + "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" + "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/filteripam" + "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam" + "github.com/stretchr/testify/require" +) + +func newRequest() *networkservice.NetworkServiceRequest { + return &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{ + Context: &networkservice.ConnectionContext{ + IpContext: new(networkservice.IPContext), + }, + }, + } +} +func validateConns(t *testing.T, conn *networkservice.Connection, dsts, srcs []string) { + for i, dst := range dsts { + require.Equal(t, conn.Context.IpContext.DstIpAddrs[i], dst) + require.Equal(t, conn.Context.IpContext.SrcRoutes[i].Prefix, dst) + } + for i, src := range srcs { + require.Equal(t, conn.Context.IpContext.SrcIpAddrs[i], src) + require.Equal(t, conn.Context.IpContext.DstRoutes[i].Prefix, src) + } +} + +func TestOverlappingAddresses(t *testing.T) { + _, ipNet, err := net.ParseCIDR("172.16.0.0/24") + require.NoError(t, err) + + srv := next.NewNetworkServiceServer(filteripam.NewServer(point2pointipam.NewServer, ipNet)) + + emptyRequest := newRequest() + + request := newRequest() + request.Connection.Id = "id" + request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32", "172.16.0.25/32"} + request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32", "172.16.0.24/32"} + request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.0/32"}, {Prefix: "172.16.0.24/32"}} + request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.0.1/32"}, {Prefix: "172.16.0.25/32"}} + + conn1, err := srv.Request(context.Background(), emptyRequest) + require.NoError(t, err) + validateConns(t, conn1, []string{"172.16.0.0/32"}, []string{"172.16.0.1/32"}) + + conn2, err := srv.Request(context.Background(), request.Clone()) + require.NoError(t, err) + validateConns(t, conn2, []string{"172.16.0.24/32"}, []string{"172.16.0.25/32"}) + + _, err = srv.Close(context.Background(), conn1) + require.NoError(t, err) + + conn2, err = srv.Request(context.Background(), request) + require.NoError(t, err) + validateConns(t, conn2, []string{"172.16.0.0/32", "172.16.0.24/32"}, []string{"172.16.0.1/32", "172.16.0.25/32"}) +} diff --git a/pkg/networkservice/ipam/point2pointipam/server.go b/pkg/networkservice/ipam/point2pointipam/server.go index 34f58a884..599062bff 100644 --- a/pkg/networkservice/ipam/point2pointipam/server.go +++ b/pkg/networkservice/ipam/point2pointipam/server.go @@ -22,7 +22,6 @@ package point2pointipam import ( "context" "net" - "net/netip" "sync" "github.com/edwarnicke/genericsync" @@ -96,8 +95,6 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ excludeIP4, excludeIP6 := exclude(ipContext.GetExcludedPrefixes()...) - s.validateIPContext(ipContext, excludeIP4, excludeIP6) - connInfo, loaded := s.Load(conn.GetId()) var err error if loaded && (connInfo.shouldUpdate(excludeIP4) || connInfo.shouldUpdate(excludeIP6)) { @@ -136,60 +133,6 @@ func (s *ipamServer) Request(ctx context.Context, request *networkservice.Networ return conn, nil } -func addrBelongsToIPPool(ipPool *ippool.IPPool, addr netip.Addr) bool { - ipLen := ipPool.IPLength() - return (ipLen == net.IPv4len && addr.Is4()) || (ipLen == net.IPv6len && addr.Is6()) -} - -func (s *ipamServer) getInvalidAddrs(addrs []string, excludeIP4, excludeIP6 *ippool.IPPool) []string { - invalidAddrs := make([]string, 0) - for _, prefixString := range addrs { - prefix, parseErr := netip.ParsePrefix(prefixString) - if parseErr != nil { - invalidAddrs = append(invalidAddrs, prefixString) - continue - } - - versionMatches := false - for _, ipPool := range s.ipPools { - if addrBelongsToIPPool(ipPool, prefix.Addr()) { - versionMatches = true - break - } - } - if !versionMatches { - continue - } - - addrString := prefix.Addr().String() - valid := false - for _, ipPool := range s.ipPools { - if ipPool.ContainsString(addrString) && !excludeIP4.ContainsString(addrString) && !excludeIP6.ContainsString(addrString) { - valid = true - break - } - } - - if !valid { - invalidAddrs = append(invalidAddrs, prefixString) - } - } - - return invalidAddrs -} - -func (s *ipamServer) validateIPContext(ipContext *networkservice.IPContext, excludeIP4, excludeIP6 *ippool.IPPool) { - for _, addr := range s.getInvalidAddrs(ipContext.SrcIpAddrs, excludeIP4, excludeIP6) { - deleteAddr(&ipContext.SrcIpAddrs, addr) - deleteRoute(&ipContext.DstRoutes, addr) - } - - for _, addr := range s.getInvalidAddrs(ipContext.DstIpAddrs, excludeIP4, excludeIP6) { - deleteAddr(&ipContext.DstIpAddrs, addr) - deleteRoute(&ipContext.SrcRoutes, addr) - } -} - func (s *ipamServer) recoverAddrs(srcAddrs, dstAddrs []string, excludeIP4, excludeIP6 *ippool.IPPool) (connInfo *connectionInfo, err error) { if len(srcAddrs) == 0 || len(dstAddrs) == 0 { return nil, errors.New("addresses cannot be empty for recovery") diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index cfd3fc68e..f0940340a 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -536,29 +536,3 @@ func TestRecoveryServers(t *testing.T) { require.NoError(t, err) validateConns(t, conn3, []string{"192.168.10.0/32", "fe80::fa00/128"}, []string{"192.168.10.1/32", "fe80::fa01/128"}) } - -func TestOverlappingAddresses(t *testing.T) { - _, ipNet, err := net.ParseCIDR("172.16.0.0/24") - require.NoError(t, err) - - srv := newIpamServer(ipNet) - - emptyRequest := newRequest() - emptyRequest.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} - - request := newRequest() - request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32", "172.16.0.25/32"} - request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32", "172.16.0.24/32"} - request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.2/32"}, {Prefix: "172.16.0.24/32"}} - request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.0.3/32"}, {Prefix: "172.16.0.25/32"}} - request.Connection.Context.IpContext.ExcludedPrefixes = []string{"10.96.0.0/16", "10.244.0.0/16"} - - conn, err := srv.Request(context.Background(), emptyRequest) - require.NoError(t, err) - validateConn(t, conn, "172.16.0.0/32", "172.16.0.1/32") - - conn, err = srv.Request(context.Background(), request) - require.NoError(t, err) - require.NotContains(t, conn.Context.IpContext.DstIpAddrs, "172.16.0.0/32") - require.NotContains(t, conn.Context.IpContext.SrcIpAddrs, "172.16.0.1/32") -} diff --git a/pkg/networkservice/ipam/strictipam/server.go b/pkg/networkservice/ipam/strictipam/server.go index 2c940c76a..b17d00012 100644 --- a/pkg/networkservice/ipam/strictipam/server.go +++ b/pkg/networkservice/ipam/strictipam/server.go @@ -39,7 +39,7 @@ func NewServer(newIPAMServer func(...*net.IPNet) networkservice.NetworkServiceSe } var ipPool = ippool.New(net.IPv6len) for _, p := range prefixes { - ipPool.AddNet(p) + ipPool.AddNet(ipNetToIpv6Net(p)) } return next.NewNetworkServiceServer( &strictIPAMServer{ipPool: ipPool}, @@ -68,3 +68,15 @@ func (n *strictIPAMServer) Request(ctx context.Context, request *networkservice. func (n *strictIPAMServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { return next.Server(ctx).Close(ctx, conn) } + +func ipNetToIpv6Net(ipNet *net.IPNet) *net.IPNet { + if len(ipNet.IP) == net.IPv6len { + return ipNet + } + ipv6Net := new(net.IPNet) + ipv6Net.IP = ipNet.IP.To16() + ipv6Net.Mask = make([]byte, 16) + copy(ipv6Net.Mask[12:], ipNet.Mask) + + return ipv6Net +} From ea784c20f4c810fa728ed326069ebc1584002441 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 11:33:24 +1100 Subject: [PATCH 11/16] fix unit tests Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/point2pointipam/server_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/networkservice/ipam/point2pointipam/server_test.go b/pkg/networkservice/ipam/point2pointipam/server_test.go index f0940340a..417497a4d 100644 --- a/pkg/networkservice/ipam/point2pointipam/server_test.go +++ b/pkg/networkservice/ipam/point2pointipam/server_test.go @@ -1,6 +1,6 @@ // Copyright (c) 2020-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2022-2024 Cisco and/or its affiliates. +// Copyright (c) 2022 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -450,7 +450,7 @@ func TestRecoveryServer(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"192.168.0.0/32"}, []string{"192.168.0.1/32"}) + validateConns(t, conn2, []string{"192.168.10.0/32", "192.168.0.0/32"}, []string{"192.168.10.1/32", "192.168.0.1/32"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) @@ -485,7 +485,7 @@ func TestRecoveryServerIPv6(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"fe80::/128"}, []string{"fe80::1/128"}) + validateConns(t, conn2, []string{"fe80::fa00/128", "fe80::/128"}, []string{"fe80::fa01/128", "fe80::1/128"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) @@ -525,7 +525,7 @@ func TestRecoveryServers(t *testing.T) { // Recovery failed. Added new ones conn2, err := srv.Request(context.Background(), request2) require.NoError(t, err) - validateConns(t, conn2, []string{"192.168.0.0/32", "fe80::/128"}, []string{"192.168.0.1/32", "fe80::1/128"}) + validateConns(t, conn2, []string{"192.168.10.0/32", "fe80::fa00/128", "192.168.0.0/32", "fe80::/128"}, []string{"192.168.10.1/32", "fe80::fa01/128", "192.168.0.1/32", "fe80::1/128"}) // Close - addresses release _, err = srv.Close(context.Background(), conn1) From 052572fecf54053be943d95d50534b30bcee88a0 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 11:39:01 +1100 Subject: [PATCH 12/16] fix go linter issues Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/filteripam/server.go | 49 ++----------------- .../ipam/filteripam/server_test.go | 3 +- 2 files changed, 7 insertions(+), 45 deletions(-) diff --git a/pkg/networkservice/ipam/filteripam/server.go b/pkg/networkservice/ipam/filteripam/server.go index 38e5f4549..c3d3d30b1 100644 --- a/pkg/networkservice/ipam/filteripam/server.go +++ b/pkg/networkservice/ipam/filteripam/server.go @@ -14,7 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package strictipam provides a networkservice.NetworkService Server chain element for building an IPAM server that +// Package filteripam provides a networkservice.NetworkService Server chain element for building an IPAM server that // filters some invalid addresses and routes in IP context package filteripam @@ -25,6 +25,7 @@ import ( "github.com/golang/protobuf/ptypes/empty" "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" "github.com/networkservicemesh/sdk/pkg/tools/ippool" ) @@ -33,6 +34,7 @@ type filterIPAMServer struct { ipPool *ippool.IPPool } +// NewServer - creates a new filter IPAM server func NewServer(newIPAMServer func(...*net.IPNet) networkservice.NetworkServiceServer, prefixes ...*net.IPNet) networkservice.NetworkServiceServer { if newIPAMServer == nil { panic("newIPAMServer should not be nil") @@ -84,34 +86,9 @@ func (s *filterIPAMServer) getInvalidAddrs(addrs []string) []string { continue } - // versionMatches := false - // for _, ipPool := range s.ipPools { - // if addrBelongsToIPPool(ipPool, prefix.Addr()) { - // versionMatches = true - // break - // } - // } - // if !versionMatches { - // continue - // } - - // addrString := prefix.Addr().String() - // valid := false - // for _, ipPool := range s.ipPools { - // if ipPool.ContainsString(addrString) { - // valid = true - // break - // } - // } - if !s.ipPool.ContainsString(prefix.Addr().String()) { invalidAddrs = append(invalidAddrs, prefixString) - } - - // if !valid { - // invalidAddrs = append(invalidAddrs, prefixString) - // } } return invalidAddrs @@ -148,28 +125,12 @@ func deleteAddr(addrs *[]string, addr string) { } func (s *filterIPAMServer) pullAddrs(ipContext *networkservice.IPContext) { - // for _, addr := range ipContext.SrcIpAddrs { - // for _, ippool := range s.ipPools { - // if _, err := ippool.PullIPString(addr); err == nil { - // break - // } - // } - // } - - // for _, addr := range ipContext.DstIpAddrs { - // for _, ippool := range s.ipPools { - // if _, err := ippool.PullIPString(addr); err == nil { - // break - // } - // } - // } - for _, addr := range ipContext.SrcIpAddrs { - s.ipPool.PullIPString(addr) + _, _ = s.ipPool.PullIPString(addr) } for _, addr := range ipContext.DstIpAddrs { - s.ipPool.PullIPString(addr) + _, _ = s.ipPool.PullIPString(addr) } } diff --git a/pkg/networkservice/ipam/filteripam/server_test.go b/pkg/networkservice/ipam/filteripam/server_test.go index 6dfe40ce6..68156409b 100644 --- a/pkg/networkservice/ipam/filteripam/server_test.go +++ b/pkg/networkservice/ipam/filteripam/server_test.go @@ -22,10 +22,11 @@ import ( "testing" "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/stretchr/testify/require" + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/filteripam" "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam" - "github.com/stretchr/testify/require" ) func newRequest() *networkservice.NetworkServiceRequest { From c851ba3f54b82cfabd740871823359695b6f3726 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 12:07:24 +1100 Subject: [PATCH 13/16] cleanup Signed-off-by: NikitaSkrynnik --- pkg/tools/ippool/ippool.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index b4a8bf45b..d304cbf05 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -1,6 +1,6 @@ // Copyright (c) 2021-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2022-2024 Cisco and/or its affiliates. +// Copyright (c) 2022-2023 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -193,8 +193,6 @@ func (tree *IPPool) Contains(ip net.IP) bool { } // ContainsString - check the pool contains ip by string value. -// Returns true if pool can allocate address stored in string, -// otherwise returns false func (tree *IPPool) ContainsString(in string) bool { return tree.Contains(net.ParseIP(in)) } @@ -336,11 +334,6 @@ func (tree *IPPool) GetPrefixes() []string { return prefixes } -// IPLength returns the length of ip addresses allocated by IP Pool -func (tree *IPPool) IPLength() int { - return tree.ipLength -} - func (tree *IPPool) excludePool(exclude *IPPool) { if exclude == nil { return From a4f673722f40aad1c897b6b865b2cbe1d6d77fa7 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 12:10:42 +1100 Subject: [PATCH 14/16] add ipv6 unit test Signed-off-by: NikitaSkrynnik --- .../ipam/filteripam/server_test.go | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/pkg/networkservice/ipam/filteripam/server_test.go b/pkg/networkservice/ipam/filteripam/server_test.go index 68156409b..3f748673c 100644 --- a/pkg/networkservice/ipam/filteripam/server_test.go +++ b/pkg/networkservice/ipam/filteripam/server_test.go @@ -79,3 +79,34 @@ func TestOverlappingAddresses(t *testing.T) { require.NoError(t, err) validateConns(t, conn2, []string{"172.16.0.0/32", "172.16.0.24/32"}, []string{"172.16.0.1/32", "172.16.0.25/32"}) } + +func TestOverlappingAddressesIPv6(t *testing.T) { + _, ipNet, err := net.ParseCIDR("fe80::/64") + require.NoError(t, err) + + srv := next.NewNetworkServiceServer(filteripam.NewServer(point2pointipam.NewServer, ipNet)) + + emptyRequest := newRequest() + + request := newRequest() + request.Connection.Id = "id" + request.Connection.Context.IpContext.SrcIpAddrs = []string{"fe80::1/128", "fe80::fa01/128"} + request.Connection.Context.IpContext.DstIpAddrs = []string{"fe80::/128", "fe80::fa00/128"} + request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "fe80::/128"}, {Prefix: "fe80::fa00/128"}} + request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "fe80::1/128"}, {Prefix: "fe80::fa01/128"}} + + conn1, err := srv.Request(context.Background(), emptyRequest) + require.NoError(t, err) + validateConns(t, conn1, []string{"fe80::/128"}, []string{"fe80::1/128"}) + + conn2, err := srv.Request(context.Background(), request.Clone()) + require.NoError(t, err) + validateConns(t, conn2, []string{"fe80::fa00/128"}, []string{"fe80::fa01/128"}) + + _, err = srv.Close(context.Background(), conn1) + require.NoError(t, err) + + conn2, err = srv.Request(context.Background(), request) + require.NoError(t, err) + validateConns(t, conn2, []string{"fe80::/128", "fe80::fa00/128"}, []string{"fe80::1/128", "fe80::fa01/128"}) +} From b480c736d1e7967d764a53b2bc3ffd17ad6a04ec Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 12:11:22 +1100 Subject: [PATCH 15/16] cleanup Signed-off-by: NikitaSkrynnik --- pkg/tools/ippool/ippool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/tools/ippool/ippool.go b/pkg/tools/ippool/ippool.go index d304cbf05..144dd4f38 100644 --- a/pkg/tools/ippool/ippool.go +++ b/pkg/tools/ippool/ippool.go @@ -192,7 +192,7 @@ func (tree *IPPool) Contains(ip net.IP) bool { return tree.lookup(ipAddressFromIP(ip)) != nil } -// ContainsString - check the pool contains ip by string value. +// ContainsString - check the pool contains ip by string value func (tree *IPPool) ContainsString(in string) bool { return tree.Contains(net.ParseIP(in)) } From d1b0bb45add5d004d8dd0e590713dbcb1dbca227 Mon Sep 17 00:00:00 2001 From: NikitaSkrynnik Date: Fri, 16 Aug 2024 12:19:44 +1100 Subject: [PATCH 16/16] fix go linter issues Signed-off-by: NikitaSkrynnik --- pkg/networkservice/ipam/filteripam/server_test.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/networkservice/ipam/filteripam/server_test.go b/pkg/networkservice/ipam/filteripam/server_test.go index 3f748673c..3c5c81a60 100644 --- a/pkg/networkservice/ipam/filteripam/server_test.go +++ b/pkg/networkservice/ipam/filteripam/server_test.go @@ -29,9 +29,10 @@ import ( "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam" ) -func newRequest() *networkservice.NetworkServiceRequest { +func newRequest(connID string) *networkservice.NetworkServiceRequest { return &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{ + Id: connID, Context: &networkservice.ConnectionContext{ IpContext: new(networkservice.IPContext), }, @@ -49,16 +50,16 @@ func validateConns(t *testing.T, conn *networkservice.Connection, dsts, srcs []s } } +// nolint: dupl func TestOverlappingAddresses(t *testing.T) { _, ipNet, err := net.ParseCIDR("172.16.0.0/24") require.NoError(t, err) srv := next.NewNetworkServiceServer(filteripam.NewServer(point2pointipam.NewServer, ipNet)) - emptyRequest := newRequest() + emptyRequest := newRequest("empty") - request := newRequest() - request.Connection.Id = "id" + request := newRequest("id") request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.0.1/32", "172.16.0.25/32"} request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.0.0/32", "172.16.0.24/32"} request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.0.0/32"}, {Prefix: "172.16.0.24/32"}} @@ -80,15 +81,16 @@ func TestOverlappingAddresses(t *testing.T) { validateConns(t, conn2, []string{"172.16.0.0/32", "172.16.0.24/32"}, []string{"172.16.0.1/32", "172.16.0.25/32"}) } +// nolint: dupl func TestOverlappingAddressesIPv6(t *testing.T) { _, ipNet, err := net.ParseCIDR("fe80::/64") require.NoError(t, err) srv := next.NewNetworkServiceServer(filteripam.NewServer(point2pointipam.NewServer, ipNet)) - emptyRequest := newRequest() + emptyRequest := newRequest("empty") - request := newRequest() + request := newRequest("id") request.Connection.Id = "id" request.Connection.Context.IpContext.SrcIpAddrs = []string{"fe80::1/128", "fe80::fa01/128"} request.Connection.Context.IpContext.DstIpAddrs = []string{"fe80::/128", "fe80::fa00/128"}