fixed updates of excluded prefixes #1180

Mixaster995 · 2021-11-29T09:54:07Z

Signed-off-by: Mikhail Avramenko avramenkomihail15@gmail.com

Description

Excluded prefixes never removed, only appended. It's not desired behaviour.

Issue link

closes #1179

How Has This Been Tested?

Added unit testing to cover
Tested manually
Tested by integration testing
Have not tested

Types of changes

denis-tingaikin · 2021-12-01T13:23:17Z

pkg/networkservice/common/excludedprefixes/server.go

+	prefixPool       atomic.Value
+	once             sync.Once
+	configPath       string
+	previousPrefixes map[string]struct{}


Could you explain why is this not per connection?

denis-tingaikin

as far as I can see these changes are still not resistant to the following scenarios:

Client changed his prefixes on refresh.
Client's and server's prefixes have a collision on the first request and do not have a collision on the next request. request1: {clientPrefixes: a, serverPrefixes:a, finalPrefixes: a}, Actual: request2: {clientPrefixes: a, serverPrefixes:b, finalPrefixes: b}.
Expected: request2: {clientPrefixes: a, serverPrefixes:b, finalPrefixes: ab}.

Mixaster995 · 2021-12-08T04:36:59Z

@denis-tingaikin reworked some logic, added unit test for describe scenario - please, take a look

denis-tingaikin · 2021-12-12T18:39:23Z

pkg/networkservice/common/excludedprefixes/server.go

+	prefixesFromFile := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes()
+	// excluded prefixes from client
+	clientPrefixes := conn.GetContext().GetIpContext().GetExcludedPrefixes()
+
+	sort.Strings(prefixesFromFile)
+	sort.Strings(clientPrefixes)
+
+	if IsEqual(prefixesFromFile, clientPrefixes) {
+		return next.Server(ctx).Request(ctx, request)
+	}
+
+	prefixesInfo, _ := load(ctx)
+
+	clientPrefixesChanged := !IsEqual(prefixesInfo.previousClientPrefixes, clientPrefixes)
+	filePrefixesChanged := !IsEqual(prefixesInfo.previousFilePrefixes, prefixesFromFile)
+
+	finalPrefixes := clientPrefixes
+	if !clientPrefixesChanged && filePrefixesChanged {
+		finalPrefixes = removePrefixes(clientPrefixes, prefixesInfo.previousDiff)
+	}
+
+	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)
+	}
+
+	prefixesInfo.previousDiff = Subtract(clientPrefixes, prefixesFromFile)
+
+	log.FromContext(ctx).
+		WithField("excludedPrefixes", "server").
+		WithField("prefixesFromFile", prefixesFromFile).
+		WithField("previousPrefixesInfo", prefixesInfo).
+		Debugf("adding excluded prefixes to connection")
+
+	store(ctx, prefixesInfo)



it starts to look more and more complicated.

I think we should consider the problem on the client(nsc) side within exludeprefix.Client chain element that will simply translate response prefixes to client prefixes. In this case all applications in the chain of request can always append prefixes and don't care about diff/conflict cases.

@denis-tingaikin ok, it sounds like a good solution, added it

denis-tingaikin · 2021-12-22T20:07:17Z

@Mixaster995 Could you rebase this?

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

Mixaster995 · 2021-12-23T10:33:51Z

@denis-tingaikin ok, done

denis-tingaikin · 2021-12-23T23:59:58Z

pkg/networkservice/common/excludedprefixes/utils.go

+// 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)
+}


Consider using github.com/google/go-cmp/cmp

We already have dep on it https://github.com/networkservicemesh/sdk/blob/main/go.mod#L17

Suggested change

// 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)

}

@denis-tingaikin ok, done

denis-tingaikin

Looks good. Only one comment to fix and this can be merged.

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

…k@main PR link: networkservicemesh/sdk#1180 Commit: eefb4a2 Author: Авраменко Михаил Date: 2021-12-24 19:34:52 +0700 Message: - fixed updates of excluded prefixes (#1180) * fixed updates of excluded prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * added metadata Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * addressed review changes, refactored unit tests Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * modified updating logic, added diff cutting of prefixes Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * returned old excludedPrefixServer implementation, added client Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com> * removed custom equality method Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

Mixaster995 force-pushed the fix/excluded-prefixes branch 3 times, most recently from 80e48a3 to 7a8881c Compare December 1, 2021 10:32

Mixaster995 requested review from denis-tingaikin and glazychev-art December 1, 2021 10:37

Mixaster995 self-assigned this Dec 1, 2021

denis-tingaikin reviewed Dec 1, 2021

View reviewed changes

Mixaster995 requested a review from denis-tingaikin December 2, 2021 03:02

Mixaster995 assigned glazychev-art Dec 2, 2021

glazychev-art approved these changes Dec 3, 2021

View reviewed changes

denis-tingaikin requested changes Dec 5, 2021

View reviewed changes

Mixaster995 requested a review from denis-tingaikin December 8, 2021 09:55

denis-tingaikin requested changes Dec 12, 2021

View reviewed changes

Mixaster995 requested a review from denis-tingaikin December 13, 2021 08:20

Mixaster995 mentioned this pull request Dec 14, 2021

Exclude-prefixes not updated correctly #1179

Closed

Mixaster995 added 4 commits December 23, 2021 16:35

fixed updates of excluded prefixes

50483d3

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

added metadata

02815db

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

addressed review changes, refactored unit tests

c0ccdd0

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

modified updating logic, added diff cutting of prefixes

df2bfcf

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

Mixaster995 force-pushed the fix/excluded-prefixes branch 2 times, most recently from fb9f391 to 90d3615 Compare December 23, 2021 09:43

returned old excludedPrefixServer implementation, added client

1b302ad

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

Mixaster995 force-pushed the fix/excluded-prefixes branch from 90d3615 to 1b302ad Compare December 23, 2021 10:21

denis-tingaikin reviewed Dec 23, 2021

View reviewed changes

denis-tingaikin approved these changes Dec 24, 2021

View reviewed changes

removed custom equality method

51d027d

Signed-off-by: Mikhail Avramenko <avramenkomihail15@gmail.com>

denis-tingaikin approved these changes Dec 24, 2021

View reviewed changes

denis-tingaikin merged commit eefb4a2 into networkservicemesh:main Dec 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed updates of excluded prefixes #1180

fixed updates of excluded prefixes #1180

Mixaster995 commented Nov 29, 2021 •

edited

Loading

denis-tingaikin Dec 1, 2021

Mixaster995 Dec 1, 2021

denis-tingaikin left a comment

Mixaster995 commented Dec 8, 2021

denis-tingaikin Dec 12, 2021

Mixaster995 Dec 13, 2021 •

edited

Loading

denis-tingaikin commented Dec 22, 2021

Mixaster995 commented Dec 23, 2021

denis-tingaikin Dec 23, 2021

Mixaster995 Dec 24, 2021

denis-tingaikin left a comment

fixed updates of excluded prefixes #1180

fixed updates of excluded prefixes #1180

Conversation

Mixaster995 commented Nov 29, 2021 • edited Loading

Description

Issue link

How Has This Been Tested?

Types of changes

denis-tingaikin Dec 1, 2021

Choose a reason for hiding this comment

Mixaster995 Dec 1, 2021

Choose a reason for hiding this comment

denis-tingaikin left a comment

Choose a reason for hiding this comment

Mixaster995 commented Dec 8, 2021

denis-tingaikin Dec 12, 2021

Choose a reason for hiding this comment

Mixaster995 Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

denis-tingaikin commented Dec 22, 2021

Mixaster995 commented Dec 23, 2021

denis-tingaikin Dec 23, 2021

Choose a reason for hiding this comment

Mixaster995 Dec 24, 2021

Choose a reason for hiding this comment

denis-tingaikin left a comment

Choose a reason for hiding this comment

Mixaster995 commented Nov 29, 2021 •

edited

Loading

Mixaster995 Dec 13, 2021 •

edited

Loading