From c0ccdd079b4a5f2c8b40335d72c8357c26360432 Mon Sep 17 00:00:00 2001 From: Mikhail Avramenko Date: Tue, 7 Dec 2021 18:36:08 +0700 Subject: [PATCH] addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko --- .../common/excludedprefixes/metadata.go | 15 ++-- .../common/excludedprefixes/server.go | 45 +++++++++--- .../common/excludedprefixes/server_test.go | 71 ++++++++++++++++--- .../common/excludedprefixes/utils.go | 34 ++++++++- 4 files changed, 139 insertions(+), 26 deletions(-) diff --git a/pkg/networkservice/common/excludedprefixes/metadata.go b/pkg/networkservice/common/excludedprefixes/metadata.go index cd859f7c7..cb48d198a 100644 --- 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 index 2014d7255..2c1b62213 100644 --- 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 index 7916b5d54..01d024618 100644 --- 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 index 0ed7d9686..709a82585 100644 --- 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) +}