Added chain element that excludes already used prefixes #1197

sol-0 · 2021-12-10T17:09:28Z

Signed-off-by: Oleg Solodkov oleg.solodkov@xored.com

Description

Added client chain element that excludes prefixes already used by other network services

Issue link

Fixes #1188

How Has This Been Tested?

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

Types of changes

sol-0 · 2021-12-10T17:28:04Z

Hi @edwarnicke ,
This is a suggested solution for #1188, could you please take a look and share your thoughts?

Also you mentioned that endpoints might be buggy in the issue discussion, could you please elaborate more on that?

edwarnicke · 2021-12-10T17:48:39Z

Hi @edwarnicke , This is a suggested solution for #1188, could you please take a look and share your thoughts?

Also you mentioned that endpoints might be buggy in the issue discussion, could you please elaborate more on that?

Sure. Say an endpoint receives a set of exclude prefixes. The endpoint shouldn't set Src/Dst IP addresses Routes with Dst prefixes that conflict with those exclude prefixes. But what if it does? What happens then?

denis-tingaikin

Looks about correct,

I think we need to test it on integration tests and also consider a case with a buggy endpoint as @edwarnicke pointed.

denis-tingaikin · 2021-12-13T08:59:43Z

pkg/networkservice/common/excludedprefixes/client.go

+	if conn.GetContext().GetIpContext() == nil {
+		conn.Context.IpContext = &networkservice.IPContext{}
+	}
+


logger = logger.FromContext(ctx).WithField("ExcludedPrefixesClient", "Request")

denis-tingaikin · 2021-12-13T09:00:45Z

pkg/networkservice/common/excludedprefixes/client.go

+
+	if len(epc.excludedPrefixes) > 0 {
+		<-epc.executor.AsyncExec(func() {
+			log.FromContext(ctx).Debugf("ExcludedPrefixesClient: adding new excluded IPs to the request: %v", epc.excludedPrefixes)


Suggested change

log.FromContext(ctx).Debugf("ExcludedPrefixesClient: adding new excluded IPs to the request: %v", epc.excludedPrefixes)

logger..Debugf("adding new excluded IPs %+v", epc.excludedPrefixes)

Apply this for similar places.

sol-0 · 2021-12-13T13:05:48Z

Hi @edwarnicke , This is a suggested solution for #1188, could you please take a look and share your thoughts?
Also you mentioned that endpoints might be buggy in the issue discussion, could you please elaborate more on that?

Sure. Say an endpoint receives a set of exclude prefixes. The endpoint shouldn't set Src/Dst IP addresses Routes with Dst prefixes that conflict with those exclude prefixes. But what if it does? What happens then?

@edwarnicke,

Let me summarize the use-case with a buggy endpoint:

We have endpoint A, it has CIDR from env and optional excluded prefixes from a file.
Client makes the first request to endpoint A, gets src/dst IPs allocated by IPAM and optional excluded prefixes; client saves this info using new chain element.
Client makes the second request to endpoint B and adds prefixes saved on step 2 as ExcludedPrefixes for this new request.
If endpoint B is incorrectly configured and it's not using our IPAM, then it might return the same/conflicting IPs.
- actual: we'll create interfaces with conflicting IPs
- expected: should we check that in our element, return an error and not create an interface in this case?

Is my understanding correct?

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

denis-tingaikin · 2021-12-17T11:41:31Z

pkg/networkservice/common/excludedprefixes/utils.go

+	for i := 0; i < len(source); i++ {
+		prefix = source[i]
+		for _, ipCtxPrefix := range exclude {
+			if prefix == ipCtxPrefix {
+				source = append(source[:i], source[i+1:]...)
+				i--
+			}
+		}
+	}


Please use the map[string]struct{} and reduce the time complexity from O(n*m) to O(n + m)

denis-tingaikin · 2021-12-17T11:41:56Z

pkg/networkservice/common/excludedprefixes/utils.go

@@ -18,7 +18,8 @@

 package excludedprefixes

-func removeDuplicates(elements []string) []string {
+// RemoveDuplicates - removes duplicate strings from a slice
+func RemoveDuplicates(elements []string) []string {


Why is this public?

This was made public to avoid code duplication in test code. Shall we move this to tools?

denis-tingaikin · 2021-12-17T11:41:59Z

pkg/networkservice/common/excludedprefixes/utils.go

@@ -31,3 +32,20 @@ func removeDuplicates(elements []string) []string {
 	}
 	return result
 }
+
+// Exclude - excludes strings from a slice
+func Exclude(source, exclude []string) []string {


Why is this public?

This was made public to avoid code duplication in test code. Shall we move this to tools?

denis-tingaikin · 2021-12-17T11:42:23Z

pkg/networkservice/utils/inject/injectipam/server.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package injectipam


Why do we need this?

Can we use in tests point2pointipam?

This element emulates a buggy endpoint issuing IPs/routes conflicting with already existing IPs/routes/excluded prefixes.

OK, I see. Can we rework it to injectipcontext?

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

denis-tingaikin · 2021-12-20T17:06:30Z

pkg/networkservice/utils/inject/injectipam/server.go

+	srcIPs    []string
+	dstIPs    []string
+	srcRoutes []*networkservice.Route
+	dstRoutes []*networkservice.Route


Suggested change

srcIPs []string

dstIPs []string

srcRoutes []*networkservice.Route

dstRoutes []*networkservice.Route

ipContext networkservice.IPContext

denis-tingaikin · 2021-12-20T17:06:50Z

pkg/networkservice/utils/inject/injectipam/server.go

+}
+
+// NewServer - creates a networkservice.NetworkServiceServer chain element injecting specified routes/IPs on Request into IP context
+func NewServer(srcIPs, dstIPs []string, srcRoutes, dstRoutes []*networkservice.Route) networkservice.NetworkServiceServer {


Suggested change

func NewServer(srcIPs, dstIPs []string, srcRoutes, dstRoutes []*networkservice.Route) networkservice.NetworkServiceServer {

func NewServer(ipContext networkservice.IPContext) networkservice.NetworkServiceServer {

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

…k@main PR link: networkservicemesh/sdk#1197 Commit: 218e633 Author: Sol-0 Date: 2021-12-21 15:36:53 +0700 Message: - Added chain element that excludes already used prefixes (#1197) * Added client chain element that excludes already used prefixes Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Added IPs/routes validation Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

sol-0 force-pushed the feature-1188 branch 2 times, most recently from ac31b7c to b95073d Compare December 10, 2021 17:20

sol-0 marked this pull request as ready for review December 13, 2021 08:19

denis-tingaikin reviewed Dec 13, 2021

View reviewed changes

sol-0 force-pushed the feature-1188 branch from 2227bce to dc34371 Compare December 13, 2021 11:15

sol-0 force-pushed the feature-1188 branch from ed815ef to d31e64f Compare December 14, 2021 10:44

sol-0 added 4 commits December 17, 2021 04:18

Added client chain element that excludes already used prefixes

c4688ef

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

Addressed review comments

9e67583

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

Addressed review comments

91a5b2b

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

Added IPs/routes validation

8e22833

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

sol-0 force-pushed the feature-1188 branch from fed2486 to 8e22833 Compare December 16, 2021 21:18

glazychev-art self-requested a review December 17, 2021 08:13

denis-tingaikin reviewed Dec 17, 2021

View reviewed changes

sol-0 added 2 commits December 17, 2021 20:41

Addressed review comments

0f5b909

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

Addressed review comments

a25a0d8

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

sol-0 force-pushed the feature-1188 branch from 580c467 to a25a0d8 Compare December 20, 2021 14:13

denis-tingaikin reviewed Dec 20, 2021

View reviewed changes

Addressed review comments

8aa4a0b

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

denis-tingaikin approved these changes Dec 21, 2021

View reviewed changes

denis-tingaikin merged commit 218e633 into networkservicemesh:main Dec 21, 2021

This was referenced Dec 21, 2021

Added excluded prefixes client networkservicemesh/cmd-nsc-init#327

Merged

Added excluded prefixes client networkservicemesh/cmd-nsc#349

Merged

Added excluded prefixes client networkservicemesh/cmd-nsc-vpp#336

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added chain element that excludes already used prefixes #1197

Added chain element that excludes already used prefixes #1197

sol-0 commented Dec 10, 2021

sol-0 commented Dec 10, 2021

edwarnicke commented Dec 10, 2021

denis-tingaikin left a comment

denis-tingaikin Dec 13, 2021

sol-0 Dec 13, 2021

denis-tingaikin Dec 13, 2021

denis-tingaikin Dec 13, 2021

sol-0 Dec 13, 2021

sol-0 commented Dec 13, 2021 •

edited

Loading

denis-tingaikin Dec 17, 2021

sol-0 Dec 17, 2021

denis-tingaikin Dec 17, 2021

sol-0 Dec 17, 2021 •

edited

Loading

sol-0 Dec 20, 2021

denis-tingaikin Dec 17, 2021

sol-0 Dec 17, 2021 •

edited

Loading

sol-0 Dec 20, 2021

denis-tingaikin Dec 17, 2021

sol-0 Dec 17, 2021

denis-tingaikin Dec 20, 2021

sol-0 Dec 20, 2021

denis-tingaikin Dec 20, 2021

sol-0 Dec 20, 2021

denis-tingaikin Dec 20, 2021

sol-0 Dec 20, 2021

	log.FromContext(ctx).Debugf("ExcludedPrefixesClient: adding new excluded IPs to the request: %v", epc.excludedPrefixes)
	logger..Debugf("adding new excluded IPs %+v", epc.excludedPrefixes)

	func NewServer(srcIPs, dstIPs []string, srcRoutes, dstRoutes []*networkservice.Route) networkservice.NetworkServiceServer {
	func NewServer(ipContext networkservice.IPContext) networkservice.NetworkServiceServer {

Added chain element that excludes already used prefixes #1197

Added chain element that excludes already used prefixes #1197

Conversation

sol-0 commented Dec 10, 2021

Description

Issue link

How Has This Been Tested?

Types of changes

sol-0 commented Dec 10, 2021

edwarnicke commented Dec 10, 2021

denis-tingaikin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol-0 commented Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol-0 Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol-0 Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sol-0 commented Dec 13, 2021 •

edited

Loading

sol-0 Dec 17, 2021 •

edited

Loading

sol-0 Dec 17, 2021 •

edited

Loading