Updated strict IPAM and added dualstack IP pool (#1679)

* add fix for ipam Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * another fix Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * add a unit test for ipam issue Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * add fix for ipam Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * another fix Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * add ip context validation Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * properly delete addresses Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * rework ip context validation Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * temporarily skip failing tests Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix CI issues Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix all tests Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix unit tests Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix go linter issues Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * cleanup Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * add ipv6 unit test Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * cleanup Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * fix go linter issues Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> * Replaced strict ipam by filteripam implementation Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> * Added dualstack ippool, updated tests Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> * Fixed dualstack ippool Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> * Fix linter errors Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> * Fixed ippool test input data format Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> --------- Signed-off-by: NikitaSkrynnik <nikita.skrynnik@xored.com> Signed-off-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com> Co-authored-by: NikitaSkrynnik <nikita.skrynnik@xored.com> Co-authored-by: Vladislav Byrgazov <vladislav.byrgazov@xored.com>
networkservicemesh · Oct 16, 2024 · 3801206 · 3801206
1 parent 7ebf92e
commit 3801206
Show file tree

Hide file tree

Showing 6 changed files with 466 additions and 20 deletions.
diff --git a/pkg/networkservice/common/excludedprefixes/client_test.go b/pkg/networkservice/common/excludedprefixes/client_test.go
@@ -1,6 +1,6 @@
 // Copyright (c) 2021 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
 //
@@ -32,6 +32,7 @@ import (
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/adapters"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/ipam/strictipam"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkconnection"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkrequest"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror"
@@ -75,6 +76,55 @@ func TestExcludedPrefixesClient_Request_SanityCheck(t *testing.T) {
 	require.NotEqual(t, srcIPs[0], destIPs[0])
 }
 
+func TestExcludedPrefixesClient_Request_ResponseExcludedPrefixesCheck(t *testing.T) {
+	t.Cleanup(func() { goleak.VerifyNone(t) })
+
+	_, ipNet, err := net.ParseCIDR("172.16.1.100/29")
+	require.NoError(t, err)
+
+	client := excludedprefixes.NewClient()
+	client2 := excludedprefixes.NewClient()
+
+	server := chain.NewNetworkServiceClient(
+		adapters.NewServerToClient(strictipam.NewServer(point2pointipam.NewServer, ipNet)),
+	)
+
+	request1 := &networkservice.NetworkServiceRequest{
+		Connection: &networkservice.Connection{
+			Id: "2",
+			Context: &networkservice.ConnectionContext{
+				IpContext: &networkservice.IPContext{
+					SrcIpAddrs: []string{"172.16.1.97/32"},
+					DstIpAddrs: []string{"172.16.1.96/32"},
+				},
+			},
+		},
+	}
+
+	// Client had this src and dst IPs before endpoint restart, client contains "172.16.1.99/32", "172.16.1.98/32" as other's client exluded IPs
+	request2 := &networkservice.NetworkServiceRequest{
+		Connection: &networkservice.Connection{
+			Id: "1",
+			Context: &networkservice.ConnectionContext{
+				IpContext: &networkservice.IPContext{
+					SrcIpAddrs:       []string{"172.16.1.97/32"},
+					DstIpAddrs:       []string{"172.16.1.96/32"},
+					ExcludedPrefixes: []string{"172.16.1.99/32", "172.16.1.98/32"},
+				},
+			},
+		},
+	}
+
+	_, err = chain.NewNetworkServiceClient(client2, server).Request(context.Background(), request1.Clone())
+	require.NoError(t, err)
+
+	resp, err := chain.NewNetworkServiceClient(client, server).Request(context.Background(), request2.Clone())
+	require.NoError(t, err)
+	// Ensure strict IMAP doesn't delete excluded prefixes from response
+	respExcludedPrefixes := resp.GetContext().GetIpContext().GetExcludedPrefixes()
+	require.NotEmpty(t, respExcludedPrefixes)
+}
+
 func TestExcludedPrefixesClient_Request_SrcAndDestPrefixesAreDifferent(t *testing.T) {
 	t.Cleanup(func() { goleak.VerifyNone(t) })
 

diff --git 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/strictipam/server.go b/pkg/networkservice/ipam/strictipam/server.go
@@ -14,57 +14,128 @@
 // 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 prevents IP context configuration out of the settings scope
+// 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 strictipam
 
 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"
+	"github.com/networkservicemesh/sdk/pkg/tools/dualstack"
 )
 
 type strictIPAMServer struct {
-	ipPool *ippool.IPPool
+	ipPool *dualstack.IPPool
 }
 
-// NewServer - returns a new ipam networkservice.NetworkServiceServer that validates the incoming IP context parameters and resets them based on the validation result.
+// NewServer - creates a new strict IPAM server
 func NewServer(newIPAMServer func(...*net.IPNet) networkservice.NetworkServiceServer, prefixes ...*net.IPNet) networkservice.NetworkServiceServer {
 	if newIPAMServer == nil {
 		panic("newIPAMServer should not be nil")
 	}
-	var ipPool = ippool.New(net.IPv6len)
+	var ipPool = dualstack.New()
 	for _, p := range prefixes {
-		ipPool.AddNet(p)
+		ipPool.AddIPNet(p)
 	}
 	return next.NewNetworkServiceServer(
 		&strictIPAMServer{ipPool: ipPool},
 		newIPAMServer(prefixes...),
 	)
 }
 
-func (n *strictIPAMServer) areAddressesValid(addresses []string) bool {
-	for _, srcIP := range addresses {
-		if !n.ipPool.ContainsString(srcIP) {
-			return false
+func (s *strictIPAMServer) 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 *strictIPAMServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
+	s.free(conn.Context.IpContext)
+	return next.Server(ctx).Close(ctx, conn)
+}
+
+func (s *strictIPAMServer) 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
+		}
+
+		if !s.ipPool.ContainsIPString(prefix.Addr().String()) {
+			invalidAddrs = append(invalidAddrs, prefixString)
 		}
 	}
-	return true
+
+	return invalidAddrs
 }
 
-func (n *strictIPAMServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
-	if !n.areAddressesValid(request.GetConnection().GetContext().GetIpContext().GetSrcIpAddrs()) ||
-		!n.areAddressesValid(request.GetConnection().GetContext().GetIpContext().GetDstIpAddrs()) {
-		request.GetConnection().GetContext().IpContext = &networkservice.IPContext{}
+func (s *strictIPAMServer) validateIPContext(ipContext *networkservice.IPContext) {
+	for _, addr := range s.getInvalidAddrs(ipContext.SrcIpAddrs) {
+		deleteAddr(&ipContext.SrcIpAddrs, addr)
+		deleteRoute(&ipContext.DstRoutes, addr)
 	}
 
-	return next.Server(ctx).Request(ctx, request)
+	for _, addr := range s.getInvalidAddrs(ipContext.DstIpAddrs) {
+		deleteAddr(&ipContext.DstIpAddrs, addr)
+		deleteRoute(&ipContext.SrcRoutes, addr)
+	}
 }
 
-func (n *strictIPAMServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
-	return next.Server(ctx).Close(ctx, conn)
+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 *strictIPAMServer) pullAddrs(ipContext *networkservice.IPContext) {
+	for _, addr := range ipContext.SrcIpAddrs {
+		_, _ = s.ipPool.PullIPString(addr)
+	}
+
+	for _, addr := range ipContext.DstIpAddrs {
+		_, _ = s.ipPool.PullIPString(addr)
+	}
+}
+
+func (s *strictIPAMServer) free(ipContext *networkservice.IPContext) {
+	for _, addr := range ipContext.SrcIpAddrs {
+		_, ipNet, err := net.ParseCIDR(addr)
+		if err != nil {
+			return
+		}
+		s.ipPool.AddIPNet(ipNet)
+	}
+
+	for _, addr := range ipContext.DstIpAddrs {
+		_, ipNet, err := net.ParseCIDR(addr)
+		if err != nil {
+			return
+		}
+		s.ipPool.AddIPNet(ipNet)
+	}
 }
diff --git a/pkg/networkservice/ipam/strictipam/server_test.go b/pkg/networkservice/ipam/strictipam/server_test.go
@@ -0,0 +1,157 @@
+// 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 strictipam_test
+
+import (
+	"context"
+	"net"
+	"testing"
+
+	"github.com/networkservicemesh/api/pkg/api/networkservice"
+	"github.com/stretchr/testify/require"
+
+	"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/ipam/strictipam"
+	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkrequest"
+)
+
+func newRequest(connID string) *networkservice.NetworkServiceRequest {
+	return &networkservice.NetworkServiceRequest{
+		Connection: &networkservice.Connection{
+			Id: connID,
+			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)
+	}
+}
+
+// nolint: dupl
+func TestOverlappingAddresses(t *testing.T) {
+	_, ipNet, err := net.ParseCIDR("172.16.0.0/24")
+	require.NoError(t, err)
+
+	srv := next.NewNetworkServiceServer(strictipam.NewServer(point2pointipam.NewServer, ipNet))
+
+	emptyRequest := newRequest("empty")
+
+	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"}}
+	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"})
+}
+
+// nolint: dupl
+func TestOverlappingAddressesIPv6(t *testing.T) {
+	_, ipNet, err := net.ParseCIDR("fe80::/64")
+	require.NoError(t, err)
+
+	srv := next.NewNetworkServiceServer(strictipam.NewServer(point2pointipam.NewServer, ipNet))
+
+	emptyRequest := newRequest("empty")
+
+	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"}
+	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"})
+}
+
+func Test_StrictIPAM_PositiveScenario(t *testing.T) {
+	_, ipNet, err := net.ParseCIDR("172.16.1.0/29")
+	require.NoError(t, err)
+
+	var s = strictipam.NewServer(func(i ...*net.IPNet) networkservice.NetworkServiceServer {
+		return chain.NewNetworkServiceServer(
+			checkrequest.NewServer(t, func(t *testing.T, nsr *networkservice.NetworkServiceRequest) {
+				require.NotEqual(t, networkservice.IPContext{}, *nsr.GetConnection().Context.GetIpContext(), "ip context should not be empty")
+			}),
+			point2pointipam.NewServer(ipNet))
+	}, ipNet)
+
+	_, err = s.Request(context.TODO(), &networkservice.NetworkServiceRequest{
+		Connection: &networkservice.Connection{
+			Context: &networkservice.ConnectionContext{
+				IpContext: &networkservice.IPContext{
+					SrcIpAddrs: []string{"172.16.1.0/32"},
+				},
+			},
+		},
+	})
+	require.NoError(t, err)
+}
+
+func TestNSEReplace(t *testing.T) {
+	_, ipNet, err := net.ParseCIDR("172.16.2.0/29")
+	require.NoError(t, err)
+
+	srv := next.NewNetworkServiceServer(strictipam.NewServer(point2pointipam.NewServer, ipNet))
+
+	request := newRequest("id1")
+	request.Connection.Context.IpContext.SrcIpAddrs = []string{"172.16.1.1/32"}
+	request.Connection.Context.IpContext.DstIpAddrs = []string{"172.16.1.0/32"}
+	request.Connection.Context.IpContext.SrcRoutes = []*networkservice.Route{{Prefix: "172.16.1.0/32"}}
+	request.Connection.Context.IpContext.DstRoutes = []*networkservice.Route{{Prefix: "172.16.1.1/32"}}
+
+	conn, err := srv.Request(context.Background(), request.Clone())
+	require.NoError(t, err)
+	validateConns(t, conn, []string{"172.16.2.0/32"}, []string{"172.16.2.1/32"})
+}