addressed review changes, refactored unit tests

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>
networkservicemesh · Dec 23, 2021 · c0ccdd0 · c0ccdd0
1 parent 02815db
commit c0ccdd0
Show file tree

Hide file tree

Showing 4 changed files with 139 additions and 26 deletions.
diff --git a/pkg/networkservice/common/excludedprefixes/metadata.go b/pkg/networkservice/common/excludedprefixes/metadata.go
@@ -24,15 +24,20 @@ import (
 
 type keyType struct{}
 
-func load(ctx context.Context) (map[string]struct{}, bool) {
+type prefixesInfo struct {
+	previousFilePrefixes   []string
+	previousClientPrefixes []string
+}
+
+func load(ctx context.Context) (prefixesInfo, bool) {
 	val, ok := metadata.Map(ctx, false).Load(keyType{})
 	if !ok {
-		return map[string]struct{}{}, false
+		return prefixesInfo{}, false
 	}
 
-	return val.(map[string]struct{}), true
+	return val.(prefixesInfo), true
 }
 
-func store(ctx context.Context, prefixes map[string]struct{}) {
-	metadata.Map(ctx, false).Store(keyType{}, prefixes)
+func store(ctx context.Context, info prefixesInfo) {
+	metadata.Map(ctx, false).Store(keyType{}, info)
 }
diff --git a/pkg/networkservice/common/excludedprefixes/server.go b/pkg/networkservice/common/excludedprefixes/server.go
@@ -22,6 +22,7 @@ package excludedprefixes
 
 import (
 	"context"
+	"sort"
 	"sync"
 	"sync/atomic"
 
@@ -87,25 +88,49 @@ func (eps *excludedPrefixesServer) Request(ctx context.Context, request *network
 	if conn.GetContext().GetIpContext() == nil {
 		conn.Context.IpContext = &networkservice.IPContext{}
 	}
-	prefixes := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes()
+	// excluded prefixes from file
+	prefixesFromFile := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes()
+	// excluded prefixes from client
+	clientPrefixes := conn.GetContext().GetIpContext().GetExcludedPrefixes()
 
-	previousPrefixes, _ := load(ctx)
+	sort.Strings(prefixesFromFile)
+	sort.Strings(clientPrefixes)
 
-	ipCtx := conn.GetContext().GetIpContext()
-	withoutPrevious := removePrefixes(ipCtx.GetExcludedPrefixes(), previousPrefixes)
-	ipCtx.ExcludedPrefixes = removeDuplicates(append(withoutPrevious, prefixes...))
+	if IsEqual(prefixesFromFile, clientPrefixes) {
+		return next.Server(ctx).Request(ctx, request)
+	}
+
+	prefixesInfo, _ := load(ctx)
+
+	clientPrefixesChanged := !IsEqual(prefixesInfo.previousClientPrefixes, clientPrefixes)
+	filePrefixesChanged := !IsEqual(prefixesInfo.previousFilePrefixes, prefixesFromFile)
 
-	for _, p := range prefixes {
-		previousPrefixes[p] = struct{}{}
+	finalPrefixes := clientPrefixes
+	if !clientPrefixesChanged && filePrefixesChanged {
+		finalPrefixes = removePrefixes(clientPrefixes, prefixesInfo.previousFilePrefixes)
+	}
+
+	finalPrefixes = removeDuplicates(append(finalPrefixes, prefixesFromFile...))
+	request.GetConnection().GetContext().GetIpContext().ExcludedPrefixes = finalPrefixes
+
+	if filePrefixesChanged {
+		prefixesInfo.previousFilePrefixes = make([]string, len(prefixesFromFile))
+		copy(prefixesInfo.previousFilePrefixes, prefixesFromFile)
+	}
+
+	if clientPrefixesChanged {
+		prefixesInfo.previousClientPrefixes = make([]string, len(finalPrefixes))
+		copy(prefixesInfo.previousClientPrefixes, finalPrefixes)
 	}
-	store(ctx, previousPrefixes)
 
 	log.FromContext(ctx).
 		WithField("excludedPrefixes", "server").
-		WithField("loadedPrefixes", prefixes).
-		WithField("previousPrefixes", previousPrefixes).
+		WithField("prefixesFromFile", prefixesFromFile).
+		WithField("previousPrefixesInfo", prefixesInfo).
 		Debugf("adding excluded prefixes to connection")
 
+	store(ctx, prefixesInfo)
+
 	return next.Server(ctx).Request(ctx, request)
 }
 

diff --git a/pkg/networkservice/common/excludedprefixes/server_test.go b/pkg/networkservice/common/excludedprefixes/server_test.go
@@ -20,6 +20,7 @@ package excludedprefixes_test
 
 import (
 	"context"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -129,7 +130,7 @@ func TestUniqueRequestPrefixes(t *testing.T) {
 		metadata.NewServer(),
 		excludedprefixes.NewServer(context.Background(), excludedprefixes.WithConfigPath(configPath)),
 		checkrequest.NewServer(t, func(t *testing.T, request *networkservice.NetworkServiceRequest) {
-			require.Equal(t, uniquePrefixes, request.Connection.Context.IpContext.ExcludedPrefixes)
+			require.True(t, excludedprefixes.IsEqual(uniquePrefixes, request.Connection.Context.IpContext.ExcludedPrefixes))
 		}),
 	)
 
@@ -148,7 +149,7 @@ func TestUniqueRequestPrefixes(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestChangedPrefixes(t *testing.T) {
+func TestFilePrefixesChanged(t *testing.T) {
 	prefixes := []string{"172.16.1.0/24", "10.32.0.0/12", "10.96.0.0/12", "10.20.128.0/17", "10.20.64.0/18", "10.20.8.0/21", "10.20.4.0/22"}
 	reqPrefixes := []string{"100.1.1.0/13", "10.32.0.0/12", "10.96.0.0/12", "10.20.0.0/24", "10.20.128.0/17", "10.20.64.0/18", "10.20.16.0/20", "10.20.2.0/23"}
 	diffPrefxies := []string{"100.1.1.0/13", "10.32.0.0/12", "10.96.0.0/12", "10.20.0.0/24", "10.20.128.0/17", "10.20.64.0/18", "10.20.16.0/20", "10.20.2.0/23", "10.20.4.0/22", "10.20.8.0/21", "172.16.1.0/24"}
@@ -168,7 +169,7 @@ func TestChangedPrefixes(t *testing.T) {
 		excludedprefixes.NewServer(context.Background(), excludedprefixes.WithConfigPath(configPath)),
 		checkrequest.NewServer(t, func(t *testing.T, request *networkservice.NetworkServiceRequest) {
 			if isFirst {
-				require.Equal(t, diffPrefxies, request.Connection.Context.IpContext.ExcludedPrefixes)
+				require.True(t, excludedprefixes.IsEqual(diffPrefxies, request.Connection.Context.IpContext.ExcludedPrefixes))
 				isFirst = false
 			}
 		}),
@@ -190,15 +191,67 @@ func TestChangedPrefixes(t *testing.T) {
 	newExcludedPrefixes := []string{"172.16.2.0/24"}
 	require.NoError(t, ioutil.WriteFile(configPath, []byte(strings.Join(append([]string{"prefixes:"}, newExcludedPrefixes...), "\n- ")), os.ModePerm))
 
+	newDiffPrefixes := []string{"100.1.1.0/13", "10.20.0.0/24", "10.20.16.0/20", "10.20.2.0/23", "172.16.2.0/24"}
+
 	require.Eventually(t, func() bool {
-		var prev []string
-		copy(prev, req.Connection.Context.IpContext.ExcludedPrefixes)
 		_, err = server.Request(context.Background(), req)
-		return len(req.Connection.Context.IpContext.ExcludedPrefixes) != len(prev)
-	}, time.Minute, time.Millisecond*100)
+		fmt.Printf("%v\n", req.Connection.Context.IpContext.ExcludedPrefixes)
+		return excludedprefixes.IsEqual(req.Connection.Context.IpContext.ExcludedPrefixes, newDiffPrefixes)
+	}, time.Second*10, time.Millisecond*100)
 
-	newDiffPrefixes := []string{"100.1.1.0/13", "10.20.0.0/24", "10.20.16.0/20", "10.20.2.0/23", "172.16.2.0/24"}
-	require.Equal(t, newDiffPrefixes, req.Connection.Context.IpContext.ExcludedPrefixes)
+	require.NoError(t, err)
+}
+
+// request1: {clientPrefixes: a, serverPrefixes:a, finalPrefixes: a}
+// request2: {clientPrefixes: a, serverPrefixes:b, finalPrefixes: ab}
+func TestClientAndFilePrefixesAreSame(t *testing.T) {
+	prefixes := []string{"172.16.1.0/24", "10.32.0.0/12"}
+	reqPrefixes := []string{"172.16.1.0/24", "10.32.0.0/12"}
+	firstDiffPrefxies := []string{"172.16.1.0/24", "10.32.0.0/12"}
+
+	dir := filepath.Join(os.TempDir(), t.Name())
+	defer func() { _ = os.RemoveAll(dir) }()
+	require.NoError(t, os.MkdirAll(dir, os.ModePerm))
+
+	testConfig := strings.Join(append([]string{"prefixes:"}, prefixes...), "\n- ")
+	configPath := filepath.Join(dir, defaultPrefixesFileName)
+	require.NoError(t, ioutil.WriteFile(configPath, []byte(testConfig), os.ModePerm))
+	defer func() { _ = os.Remove(configPath) }()
+
+	isFirst := true
+	server := chain.NewNetworkServiceServer(
+		metadata.NewServer(),
+		excludedprefixes.NewServer(context.Background(), excludedprefixes.WithConfigPath(configPath)),
+		checkrequest.NewServer(t, func(t *testing.T, request *networkservice.NetworkServiceRequest) {
+			if isFirst {
+				require.True(t, excludedprefixes.IsEqual(firstDiffPrefxies, request.Connection.Context.IpContext.ExcludedPrefixes))
+				isFirst = false
+			}
+		}),
+	)
+
+	req := &networkservice.NetworkServiceRequest{
+		Connection: &networkservice.Connection{
+			Id: "1",
+			Context: &networkservice.ConnectionContext{
+				IpContext: &networkservice.IPContext{
+					ExcludedPrefixes: reqPrefixes,
+				},
+			},
+		},
+	}
+
+	_, err := server.Request(context.Background(), req)
+
+	newExcludedPrefixes := []string{"172.16.2.0/24"}
+	require.NoError(t, ioutil.WriteFile(configPath, []byte(strings.Join(append([]string{"prefixes:"}, newExcludedPrefixes...), "\n- ")), os.ModePerm))
+
+	newDiffPrefixes := []string{"172.16.1.0/24", "10.32.0.0/12", "172.16.2.0/24"}
+
+	require.Eventually(t, func() bool {
+		_, err = server.Request(context.Background(), req)
+		return excludedprefixes.IsEqual(req.Connection.Context.IpContext.ExcludedPrefixes, newDiffPrefixes)
+	}, time.Second*5, time.Millisecond*100)
 
 	require.NoError(t, err)
 }

diff --git a/pkg/networkservice/common/excludedprefixes/utils.go b/pkg/networkservice/common/excludedprefixes/utils.go
@@ -18,6 +18,11 @@
 
 package excludedprefixes
 
+import (
+	"reflect"
+	"sort"
+)
+
 func removeDuplicates(elements []string) []string {
 	encountered := map[string]bool{}
 	var result []string
@@ -51,17 +56,42 @@ func exclude(source, exclude []string) []string {
 	return source
 }
 
-func removePrefixes(origin []string, prefixesToRemove map[string]struct{}) []string {
+func removePrefixes(origin, prefixesToRemove []string) []string {
 	if len(origin) == 0 || len(prefixesToRemove) == 0 {
 		return origin
 	}
 
+	prefixesMap := toMap(prefixesToRemove)
 	var rv []string
 	for _, p := range origin {
-		if _, ok := prefixesToRemove[p]; !ok {
+		if _, ok := prefixesMap[p]; !ok {
 			rv = append(rv, p)
 		}
 	}
 
 	return rv
 }
+
+func toMap(arr []string) map[string]struct{} {
+	rv := map[string]struct{}{}
+
+	for _, a := range arr {
+		rv[a] = struct{}{}
+	}
+
+	return rv
+}
+
+// IsEqual check if two slices contains equal strings, no matter the order
+func IsEqual(s1, s2 []string) bool {
+	s1copy := make([]string, len(s1))
+	s2copy := make([]string, len(s2))
+
+	copy(s1copy, s1)
+	copy(s2copy, s2)
+
+	sort.Strings(s1copy)
+	sort.Strings(s2copy)
+
+	return reflect.DeepEqual(s1copy, s2copy)
+}