From c4688efc1d5986aee7020b8e71d18b9b7ba47a0f Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Fri, 10 Dec 2021 17:37:57 +0700 Subject: [PATCH 1/7] Added client chain element that excludes already used prefixes Signed-off-by: Oleg Solodkov --- pkg/networkservice/chains/client/client.go | 5 +- .../common/excludedprefixes/client.go | 97 ++++++++++ .../common/excludedprefixes/client_test.go | 183 ++++++++++++++++++ .../common/excludedprefixes/utils.go | 24 ++- .../inject/injectexcludedprefixes/doc.go | 18 ++ .../inject/injectexcludedprefixes/server.go | 58 ++++++ 6 files changed, 381 insertions(+), 4 deletions(-) create mode 100644 pkg/networkservice/common/excludedprefixes/client.go create mode 100644 pkg/networkservice/common/excludedprefixes/client_test.go create mode 100644 pkg/networkservice/utils/inject/injectexcludedprefixes/doc.go create mode 100644 pkg/networkservice/utils/inject/injectexcludedprefixes/server.go diff --git a/pkg/networkservice/chains/client/client.go b/pkg/networkservice/chains/client/client.go index dda83f8ad..233f3712f 100644 --- a/pkg/networkservice/chains/client/client.go +++ b/pkg/networkservice/chains/client/client.go @@ -23,15 +23,15 @@ import ( "github.com/google/uuid" "github.com/networkservicemesh/api/pkg/api/networkservice" - "github.com/networkservicemesh/sdk/pkg/networkservice/common/trimpath" - "github.com/networkservicemesh/sdk/pkg/networkservice/common/begin" "github.com/networkservicemesh/sdk/pkg/networkservice/common/clientconn" "github.com/networkservicemesh/sdk/pkg/networkservice/common/clienturl" "github.com/networkservicemesh/sdk/pkg/networkservice/common/connect" "github.com/networkservicemesh/sdk/pkg/networkservice/common/dial" + "github.com/networkservicemesh/sdk/pkg/networkservice/common/excludedprefixes" "github.com/networkservicemesh/sdk/pkg/networkservice/common/null" "github.com/networkservicemesh/sdk/pkg/networkservice/common/refresh" + "github.com/networkservicemesh/sdk/pkg/networkservice/common/trimpath" "github.com/networkservicemesh/sdk/pkg/networkservice/common/updatepath" "github.com/networkservicemesh/sdk/pkg/networkservice/core/chain" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata" @@ -58,6 +58,7 @@ func NewClient(ctx context.Context, clientOpts ...Option) networkservice.Network metadata.NewClient(), opts.refreshClient, clienturl.NewClient(opts.clientURL), + excludedprefixes.NewClient(), clientconn.NewClient(opts.cc), opts.healClient, dial.NewClient(ctx, diff --git a/pkg/networkservice/common/excludedprefixes/client.go b/pkg/networkservice/common/excludedprefixes/client.go new file mode 100644 index 000000000..a877e465e --- /dev/null +++ b/pkg/networkservice/common/excludedprefixes/client.go @@ -0,0 +1,97 @@ +// Copyright (c) 2021 Doc.ai 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 excludedprefixes + +import ( + "context" + + "github.com/edwarnicke/serialize" + "github.com/golang/protobuf/ptypes/empty" + "github.com/networkservicemesh/api/pkg/api/networkservice" + "google.golang.org/grpc" + + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" + "github.com/networkservicemesh/sdk/pkg/tools/log" +) + +type excludedPrefixesClient struct { + excludedPrefixes []string + executor serialize.Executor +} + +// NewClient - creates a networkservice.NetworkServiceClient chain element that excludes prefixes already used by other NetworkServices +func NewClient() networkservice.NetworkServiceClient { + return &excludedPrefixesClient{ + excludedPrefixes: make([]string, 0), + } +} + +func (epc *excludedPrefixesClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) { + conn := request.GetConnection() + if conn.GetContext() == nil { + conn.Context = &networkservice.ConnectionContext{} + } + + if conn.GetContext().GetIpContext() == nil { + conn.Context.IpContext = &networkservice.IPContext{} + } + + ipCtx := conn.GetContext().GetIpContext() + + if len(epc.excludedPrefixes) > 0 { + <-epc.executor.AsyncExec(func() { + log.FromContext(ctx).Debugf("ExcludedPrefixesClient: adding new excluded IPs to the request: %v", epc.excludedPrefixes) + excludedPrefixes := ipCtx.GetExcludedPrefixes() + excludedPrefixes = append(excludedPrefixes, epc.excludedPrefixes...) + excludedPrefixes = removeDuplicates(excludedPrefixes) + + log.FromContext(ctx).Debugf("ExcludedPrefixesClient: excluded prefixes from request - %v", excludedPrefixes) + ipCtx.ExcludedPrefixes = excludedPrefixes + }) + } + + resp, err := next.Client(ctx).Request(ctx, request, opts...) + if err != nil { + return resp, err + } + + log.FromContext(ctx).Debugf("ExcludedPrefixesClient: request excluded IPs - srcIPs: %v, dstIPs: %v", + resp.GetContext().GetIpContext().GetSrcIpAddrs(), resp.GetContext().GetIpContext().GetDstIpAddrs()) + + <-epc.executor.AsyncExec(func() { + epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()...) + epc.excludedPrefixes = removeDuplicates(epc.excludedPrefixes) + log.FromContext(ctx).Debugf("Added excluded prefixes: %v", epc.excludedPrefixes) + }) + + return resp, err +} + +func (epc *excludedPrefixesClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { + ipCtx := conn.GetContext().GetIpContext() + + <-epc.executor.AsyncExec(func() { + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) + log.FromContext(ctx).Debugf("Excluded prefixes after closing connection: %v", epc.excludedPrefixes) + }) + + return next.Client(ctx).Close(ctx, conn, opts...) +} diff --git a/pkg/networkservice/common/excludedprefixes/client_test.go b/pkg/networkservice/common/excludedprefixes/client_test.go new file mode 100644 index 000000000..87423d5f2 --- /dev/null +++ b/pkg/networkservice/common/excludedprefixes/client_test.go @@ -0,0 +1,183 @@ +// Copyright (c) 2021 Doc.ai 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 excludedprefixes_test + +import ( + "context" + "net" + "testing" + + "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" + + "github.com/networkservicemesh/sdk/pkg/networkservice/common/excludedprefixes" + "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/utils/inject/injectexcludedprefixes" +) + +func TestExcludedPrefixesClient_Request_PrefixesAreDifferent(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client := excludedprefixes.NewClient() + + srcCidr := "172.16.0.100/30" + _, ipNet, err := net.ParseCIDR(srcCidr) + require.NoError(t, err) + + server1 := adapters.NewServerToClient( + point2pointipam.NewServer(ipNet), + ) + + request := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request.Clone()) + require.NoError(t, err) + + expectedExcludedIPs := append(resp.GetContext().GetIpContext().GetSrcIpAddrs(), + resp.GetContext().GetIpContext().GetDstIpAddrs()...) + + destIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, destIPs, 1) + firstSrcIP := destIPs[0] + + server2 := adapters.NewServerToClient( + point2pointipam.NewServer(ipNet), + ) + + resp, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request.Clone()) + require.NoError(t, err) + + destIPs = resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, destIPs, 1) + secondSrcIP := destIPs[0] + + require.NotEqual(t, firstSrcIP, secondSrcIP) + + excludedIPs := resp.GetContext().GetIpContext().GetExcludedPrefixes() + require.Equal(t, excludedIPs, expectedExcludedIPs) +} + +func TestExcludedPrefixesClient_Close_PrefixesAreEmpty(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client := excludedprefixes.NewClient() + + firstRequest := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{ + Context: &networkservice.ConnectionContext{ + IpContext: &networkservice.IPContext{ + SrcIpAddrs: []string{"172.16.0.100/32"}, + DstIpAddrs: []string{"172.16.0.103/32"}, + }, + }, + }, + } + + secondRequest := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + firstResp, err := client.Request(ctx, firstRequest.Clone()) + require.NoError(t, err) + + expectedExcludedIPs := append(firstRequest.GetConnection().GetContext().GetIpContext().GetSrcIpAddrs(), + firstRequest.GetConnection().GetContext().GetIpContext().GetDstIpAddrs()...) + + secondResp, err := client.Request(ctx, secondRequest.Clone()) + require.NoError(t, err) + + excludedIPs := secondResp.GetContext().GetIpContext().GetExcludedPrefixes() + require.Equal(t, excludedIPs, expectedExcludedIPs) + + _, err = client.Close(ctx, firstResp) + require.NoError(t, err) + + secondResp, err = client.Request(ctx, secondRequest.Clone()) + require.NoError(t, err) + + require.Empty(t, secondResp.GetContext().GetIpContext().GetExcludedPrefixes()) + + _, err = client.Close(ctx, secondResp) + require.NoError(t, err) + + require.Empty(t, secondResp.GetContext().GetIpContext().GetExcludedPrefixes()) +} + +func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") + require.NoError(t, err) + + ctx := context.Background() + + excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100"} + + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(ctx, excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err := chain.NewNetworkServiceClient(client, server1).Request(context.Background(), request.Clone()) + require.NoError(t, err) + + // sanity checks + possibleIPs := []string{"172.16.0.97/32", "172.16.0.99/32", "172.16.0.101/32", "172.16.0.103/32"} + srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + require.Contains(t, possibleIPs, srcIPs[0]) + + destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() + require.Len(t, destIPs, 1) + require.Contains(t, possibleIPs, destIPs[0]) + + require.NotEqual(t, srcIPs[0], destIPs[0]) + + expectedExcludedPrefixes := make([]string, 0) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, destIPs...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) + + // second request + server2 := adapters.NewServerToClient( + point2pointipam.NewServer(ipNet), + ) + + resp, err = chain.NewNetworkServiceClient(client, server2).Request(context.Background(), request.Clone()) + require.NoError(t, err) + + require.Equal(t, resp.GetContext().GetIpContext().GetExcludedPrefixes(), expectedExcludedPrefixes) +} diff --git a/pkg/networkservice/common/excludedprefixes/utils.go b/pkg/networkservice/common/excludedprefixes/utils.go index 51090b091..0aaa78199 100644 --- a/pkg/networkservice/common/excludedprefixes/utils.go +++ b/pkg/networkservice/common/excludedprefixes/utils.go @@ -1,6 +1,6 @@ -// Copyright (c) 2020 Doc.ai and/or its affiliates. +// Copyright (c) 2020-2021 Doc.ai and/or its affiliates. // -// Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2020-2021 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -31,3 +31,23 @@ func removeDuplicates(elements []string) []string { } return result } + +func exclude(source, exclude []string) []string { + var result []string + + var isExcluded bool + for _, s := range source { + isExcluded = false + for _, e := range exclude { + if s == e { + isExcluded = true + break + } + } + + if !isExcluded { + result = append(result, s) + } + } + return result +} diff --git a/pkg/networkservice/utils/inject/injectexcludedprefixes/doc.go b/pkg/networkservice/utils/inject/injectexcludedprefixes/doc.go new file mode 100644 index 000000000..1f170cf9c --- /dev/null +++ b/pkg/networkservice/utils/inject/injectexcludedprefixes/doc.go @@ -0,0 +1,18 @@ +// Copyright (c) 2021 Doc.ai 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 injectexcludedprefixes provides a chain element injecting specified excluded prefixes on Request into IP context +package injectexcludedprefixes diff --git a/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go b/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go new file mode 100644 index 000000000..e14ab2ebc --- /dev/null +++ b/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go @@ -0,0 +1,58 @@ +// Copyright (c) 2021 Doc.ai 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 injectexcludedprefixes + +import ( + "context" + + "github.com/golang/protobuf/ptypes/empty" + "github.com/networkservicemesh/api/pkg/api/networkservice" + + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" +) + +type injectExcludedPrefixesServer struct { + ctx context.Context + prefixes []string +} + +func (ieps *injectExcludedPrefixesServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { + conn := request.GetConnection() + if conn.GetContext() == nil { + conn.Context = &networkservice.ConnectionContext{} + } + if conn.GetContext().GetIpContext() == nil { + conn.Context.IpContext = &networkservice.IPContext{} + } + + ipCtx := conn.GetContext().GetIpContext() + ipCtx.ExcludedPrefixes = ieps.prefixes + + return next.Server(ctx).Request(ctx, request) +} + +func (ieps *injectExcludedPrefixesServer) Close(ctx context.Context, connection *networkservice.Connection) (*empty.Empty, error) { + return next.Server(ctx).Close(ctx, connection) +} + +// NewServer - creates a networkservice.NetworkServiceServer chain element injecting specified excluded prefixes on Request into IP context +func NewServer(ctx context.Context, excludedPrefixes []string) networkservice.NetworkServiceServer { + return &injectExcludedPrefixesServer{ + ctx: ctx, + prefixes: excludedPrefixes, + } +} From 9e67583875a1fe9406e6a1fb62a4990df963977b Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Mon, 13 Dec 2021 18:12:37 +0700 Subject: [PATCH 2/7] Addressed review comments Signed-off-by: Oleg Solodkov --- .../common/excludedprefixes/client.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/networkservice/common/excludedprefixes/client.go b/pkg/networkservice/common/excludedprefixes/client.go index a877e465e..e4d746b8d 100644 --- a/pkg/networkservice/common/excludedprefixes/client.go +++ b/pkg/networkservice/common/excludedprefixes/client.go @@ -50,26 +50,31 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network conn.Context.IpContext = &networkservice.IPContext{} } + logger := log.FromContext(ctx).WithField("ExcludedPrefixesClient", "Request") ipCtx := conn.GetContext().GetIpContext() + oldExcludedPrefixes := ipCtx.GetExcludedPrefixes() if len(epc.excludedPrefixes) > 0 { <-epc.executor.AsyncExec(func() { - log.FromContext(ctx).Debugf("ExcludedPrefixesClient: adding new excluded IPs to the request: %v", epc.excludedPrefixes) + logger.Debugf("Adding new excluded IPs to the request: %+v", epc.excludedPrefixes) excludedPrefixes := ipCtx.GetExcludedPrefixes() excludedPrefixes = append(excludedPrefixes, epc.excludedPrefixes...) excludedPrefixes = removeDuplicates(excludedPrefixes) - log.FromContext(ctx).Debugf("ExcludedPrefixesClient: excluded prefixes from request - %v", excludedPrefixes) + logger.Debugf("Excluded prefixes from request - %+v", excludedPrefixes) ipCtx.ExcludedPrefixes = excludedPrefixes }) } resp, err := next.Client(ctx).Request(ctx, request, opts...) if err != nil { + if oldExcludedPrefixes != nil { + ipCtx.ExcludedPrefixes = oldExcludedPrefixes + } return resp, err } - log.FromContext(ctx).Debugf("ExcludedPrefixesClient: request excluded IPs - srcIPs: %v, dstIPs: %v", + logger.Debugf("Request excluded IPs - srcIPs: %v, dstIPs: %v", resp.GetContext().GetIpContext().GetSrcIpAddrs(), resp.GetContext().GetIpContext().GetDstIpAddrs()) <-epc.executor.AsyncExec(func() { @@ -77,20 +82,21 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()...) epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()...) epc.excludedPrefixes = removeDuplicates(epc.excludedPrefixes) - log.FromContext(ctx).Debugf("Added excluded prefixes: %v", epc.excludedPrefixes) + logger.Debugf("Added excluded prefixes: %+v", epc.excludedPrefixes) }) return resp, err } func (epc *excludedPrefixesClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { + logger := log.FromContext(ctx).WithField("ExcludedPrefixesClient", "Close") ipCtx := conn.GetContext().GetIpContext() <-epc.executor.AsyncExec(func() { epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) - log.FromContext(ctx).Debugf("Excluded prefixes after closing connection: %v", epc.excludedPrefixes) + logger.Debugf("Excluded prefixes after closing connection: %+v", epc.excludedPrefixes) }) return next.Client(ctx).Close(ctx, conn, opts...) From 91a5b2b4136a61d17b62b2b5668e08d428a1987f Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Mon, 13 Dec 2021 20:37:40 +0700 Subject: [PATCH 3/7] Addressed review comments Signed-off-by: Oleg Solodkov --- pkg/networkservice/chains/client/client.go | 2 -- pkg/networkservice/common/excludedprefixes/client.go | 11 ++++++----- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/pkg/networkservice/chains/client/client.go b/pkg/networkservice/chains/client/client.go index 233f3712f..394a55122 100644 --- a/pkg/networkservice/chains/client/client.go +++ b/pkg/networkservice/chains/client/client.go @@ -28,7 +28,6 @@ import ( "github.com/networkservicemesh/sdk/pkg/networkservice/common/clienturl" "github.com/networkservicemesh/sdk/pkg/networkservice/common/connect" "github.com/networkservicemesh/sdk/pkg/networkservice/common/dial" - "github.com/networkservicemesh/sdk/pkg/networkservice/common/excludedprefixes" "github.com/networkservicemesh/sdk/pkg/networkservice/common/null" "github.com/networkservicemesh/sdk/pkg/networkservice/common/refresh" "github.com/networkservicemesh/sdk/pkg/networkservice/common/trimpath" @@ -58,7 +57,6 @@ func NewClient(ctx context.Context, clientOpts ...Option) networkservice.Network metadata.NewClient(), opts.refreshClient, clienturl.NewClient(opts.clientURL), - excludedprefixes.NewClient(), clientconn.NewClient(opts.cc), opts.healClient, dial.NewClient(ctx, diff --git a/pkg/networkservice/common/excludedprefixes/client.go b/pkg/networkservice/common/excludedprefixes/client.go index e4d746b8d..e265b584a 100644 --- a/pkg/networkservice/common/excludedprefixes/client.go +++ b/pkg/networkservice/common/excludedprefixes/client.go @@ -74,13 +74,14 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network return resp, err } - logger.Debugf("Request excluded IPs - srcIPs: %v, dstIPs: %v", - resp.GetContext().GetIpContext().GetSrcIpAddrs(), resp.GetContext().GetIpContext().GetDstIpAddrs()) + respIpContext := resp.GetContext().GetIpContext() + logger.Debugf("Request excluded IPs - srcIPs: %v, dstIPs: %v, excluded prefixes: %v", respIpContext.GetSrcIpAddrs(), + respIpContext.GetDstIpAddrs(), respIpContext.GetExcludedPrefixes()) <-epc.executor.AsyncExec(func() { - epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()...) - epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()...) - epc.excludedPrefixes = append(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetSrcIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetDstIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetExcludedPrefixes()...) epc.excludedPrefixes = removeDuplicates(epc.excludedPrefixes) logger.Debugf("Added excluded prefixes: %+v", epc.excludedPrefixes) }) From 8e228335902796be84a79fb72c984a16abbdc4fa Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Fri, 17 Dec 2021 02:09:24 +0700 Subject: [PATCH 4/7] Added IPs/routes validation Signed-off-by: Oleg Solodkov --- .../common/excludedprefixes/client.go | 105 ++++- .../common/excludedprefixes/client_test.go | 386 ++++++++++++++++-- .../common/excludedprefixes/server.go | 2 +- .../common/excludedprefixes/utils.go | 28 +- .../inject/injectexcludedprefixes/server.go | 4 +- .../utils/inject/injectipam/doc.go | 18 + .../utils/inject/injectipam/server.go | 65 +++ 7 files changed, 526 insertions(+), 82 deletions(-) create mode 100644 pkg/networkservice/utils/inject/injectipam/doc.go create mode 100644 pkg/networkservice/utils/inject/injectipam/server.go diff --git a/pkg/networkservice/common/excludedprefixes/client.go b/pkg/networkservice/common/excludedprefixes/client.go index e265b584a..97decb71a 100644 --- a/pkg/networkservice/common/excludedprefixes/client.go +++ b/pkg/networkservice/common/excludedprefixes/client.go @@ -18,14 +18,18 @@ package excludedprefixes import ( "context" + "net" "github.com/edwarnicke/serialize" "github.com/golang/protobuf/ptypes/empty" "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/pkg/errors" "google.golang.org/grpc" "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" + "github.com/networkservicemesh/sdk/pkg/tools/ippool" "github.com/networkservicemesh/sdk/pkg/tools/log" + "github.com/networkservicemesh/sdk/pkg/tools/postpone" ) type excludedPrefixesClient struct { @@ -53,36 +57,58 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network logger := log.FromContext(ctx).WithField("ExcludedPrefixesClient", "Request") ipCtx := conn.GetContext().GetIpContext() + var newExcludedPrefixes []string oldExcludedPrefixes := ipCtx.GetExcludedPrefixes() if len(epc.excludedPrefixes) > 0 { <-epc.executor.AsyncExec(func() { logger.Debugf("Adding new excluded IPs to the request: %+v", epc.excludedPrefixes) - excludedPrefixes := ipCtx.GetExcludedPrefixes() - excludedPrefixes = append(excludedPrefixes, epc.excludedPrefixes...) - excludedPrefixes = removeDuplicates(excludedPrefixes) + newExcludedPrefixes = ipCtx.GetExcludedPrefixes() + newExcludedPrefixes = append(newExcludedPrefixes, epc.excludedPrefixes...) + newExcludedPrefixes = RemoveDuplicates(newExcludedPrefixes) - logger.Debugf("Excluded prefixes from request - %+v", excludedPrefixes) - ipCtx.ExcludedPrefixes = excludedPrefixes + // excluding IPs for current request/connection before calling next client for the refresh use-case + newExcludedPrefixes = Exclude(newExcludedPrefixes, append(ipCtx.GetSrcIpAddrs(), ipCtx.GetDstIpAddrs()...)) + + logger.Debugf("Excluded prefixes from request - %+v", newExcludedPrefixes) + ipCtx.ExcludedPrefixes = newExcludedPrefixes }) } + postponeCtxFunc := postpone.ContextWithValues(ctx) + resp, err := next.Client(ctx).Request(ctx, request, opts...) if err != nil { - if oldExcludedPrefixes != nil { - ipCtx.ExcludedPrefixes = oldExcludedPrefixes - } + ipCtx.ExcludedPrefixes = oldExcludedPrefixes return resp, err } - respIpContext := resp.GetContext().GetIpContext() - logger.Debugf("Request excluded IPs - srcIPs: %v, dstIPs: %v, excluded prefixes: %v", respIpContext.GetSrcIpAddrs(), - respIpContext.GetDstIpAddrs(), respIpContext.GetExcludedPrefixes()) + respIPContext := resp.GetContext().GetIpContext() + + err = validateIPs(respIPContext, newExcludedPrefixes) + if err != nil { + closeCtx, cancelFunc := postponeCtxFunc() + defer cancelFunc() + + logger.Errorf("Source or destination IPs are overlapping with excluded prefixes, srcIPs: %+v, dstIPs: %+v, excluded prefixes: %+v, error: %s", + respIPContext.GetSrcIpAddrs(), respIPContext.GetDstIpAddrs(), newExcludedPrefixes, err.Error()) + + if _, closeErr := next.Client(ctx).Close(closeCtx, conn, opts...); closeErr != nil { + err = errors.Wrapf(err, "connection closed with error: %s", closeErr.Error()) + } + + return nil, err + } + + logger.Debugf("Request excluded IPs - srcIPs: %v, dstIPs: %v, excluded prefixes: %v", respIPContext.GetSrcIpAddrs(), + respIPContext.GetDstIpAddrs(), respIPContext.GetExcludedPrefixes()) <-epc.executor.AsyncExec(func() { - epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetSrcIpAddrs()...) - epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetDstIpAddrs()...) - epc.excludedPrefixes = append(epc.excludedPrefixes, respIpContext.GetExcludedPrefixes()...) - epc.excludedPrefixes = removeDuplicates(epc.excludedPrefixes) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIPContext.GetSrcIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIPContext.GetDstIpAddrs()...) + epc.excludedPrefixes = append(epc.excludedPrefixes, getRoutePrefixes(respIPContext.GetSrcRoutes())...) + epc.excludedPrefixes = append(epc.excludedPrefixes, getRoutePrefixes(respIPContext.GetDstRoutes())...) + epc.excludedPrefixes = append(epc.excludedPrefixes, respIPContext.GetExcludedPrefixes()...) + epc.excludedPrefixes = RemoveDuplicates(epc.excludedPrefixes) logger.Debugf("Added excluded prefixes: %+v", epc.excludedPrefixes) }) @@ -94,11 +120,54 @@ func (epc *excludedPrefixesClient) Close(ctx context.Context, conn *networkservi ipCtx := conn.GetContext().GetIpContext() <-epc.executor.AsyncExec(func() { - epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) - epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) - epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) + epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) + epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) + epc.excludedPrefixes = Exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetSrcRoutes())) + epc.excludedPrefixes = Exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetDstRoutes())) + epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) logger.Debugf("Excluded prefixes after closing connection: %+v", epc.excludedPrefixes) }) return next.Client(ctx).Close(ctx, conn, opts...) } + +func getRoutePrefixes(routes []*networkservice.Route) []string { + var rv []string + for _, route := range routes { + rv = append(rv, route.GetPrefix()) + } + + return rv +} + +func validateIPs(ipContext *networkservice.IPContext, excludedPrefixes []string) error { + ip4Pool := ippool.New(net.IPv4len) + ip6Pool := ippool.New(net.IPv6len) + + for _, prefix := range excludedPrefixes { + _, ipNet, err := net.ParseCIDR(prefix) + if err != nil { + return err + } + + ip4Pool.AddNet(ipNet) + ip6Pool.AddNet(ipNet) + } + + prefixes := make([]string, 0, len(ipContext.GetSrcIpAddrs())+len(ipContext.GetDstIpAddrs())) + prefixes = append(prefixes, ipContext.GetSrcIpAddrs()...) + prefixes = append(prefixes, ipContext.GetDstIpAddrs()...) + + for _, prefix := range prefixes { + ip, _, err := net.ParseCIDR(prefix) + if err != nil { + return err + } + + if ip4Pool.Contains(ip) || ip6Pool.Contains(ip) { + return errors.Errorf("IP %v is excluded, but it was found in response IPs", ip) + } + } + + return nil +} diff --git a/pkg/networkservice/common/excludedprefixes/client_test.go b/pkg/networkservice/common/excludedprefixes/client_test.go index 87423d5f2..fbb5e25a1 100644 --- a/pkg/networkservice/common/excludedprefixes/client_test.go +++ b/pkg/networkservice/common/excludedprefixes/client_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/pkg/errors" "github.com/stretchr/testify/require" "go.uber.org/goleak" @@ -29,10 +30,48 @@ 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/utils/inject/injecterror" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectexcludedprefixes" + "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectipam" ) -func TestExcludedPrefixesClient_Request_PrefixesAreDifferent(t *testing.T) { +func TestExcludedPrefixesClient_Request_SanityCheck(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") + require.NoError(t, err) + + excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} + + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err := chain.NewNetworkServiceClient(client, server1).Request(context.Background(), request.Clone()) + require.NoError(t, err) + + possibleIPs := []string{"172.16.0.97/32", "172.16.0.99/32", "172.16.0.101/32", "172.16.0.103/32"} + srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + require.Contains(t, possibleIPs, srcIPs[0]) + + destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() + require.Len(t, destIPs, 1) + require.Contains(t, possibleIPs, destIPs[0]) + + require.NotEqual(t, srcIPs[0], destIPs[0]) +} + +func TestExcludedPrefixesClient_Request_SrcAndDestPrefixesAreDifferent(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) }) ctx, cancel := context.WithCancel(context.Background()) @@ -48,46 +87,52 @@ func TestExcludedPrefixesClient_Request_PrefixesAreDifferent(t *testing.T) { point2pointipam.NewServer(ipNet), ) - request := &networkservice.NetworkServiceRequest{ + request1 := &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{}, } - resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request.Clone()) + resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request1) require.NoError(t, err) expectedExcludedIPs := append(resp.GetContext().GetIpContext().GetSrcIpAddrs(), resp.GetContext().GetIpContext().GetDstIpAddrs()...) + expectedExcludedIPs = append(expectedExcludedIPs, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) + expectedExcludedIPs = append(expectedExcludedIPs, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) + expectedExcludedIPs = excludedprefixes.RemoveDuplicates(expectedExcludedIPs) - destIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() - require.Len(t, destIPs, 1) - firstSrcIP := destIPs[0] + srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + srcIP1 := srcIPs[0] server2 := adapters.NewServerToClient( point2pointipam.NewServer(ipNet), ) - resp, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request.Clone()) + request2 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request2) require.NoError(t, err) - destIPs = resp.GetContext().GetIpContext().GetSrcIpAddrs() - require.Len(t, destIPs, 1) - secondSrcIP := destIPs[0] + srcIPs = resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + srcIP2 := srcIPs[0] - require.NotEqual(t, firstSrcIP, secondSrcIP) + require.NotEqual(t, srcIP1, srcIP2) excludedIPs := resp.GetContext().GetIpContext().GetExcludedPrefixes() - require.Equal(t, excludedIPs, expectedExcludedIPs) + require.ElementsMatch(t, expectedExcludedIPs, excludedIPs) } -func TestExcludedPrefixesClient_Close_PrefixesAreEmpty(t *testing.T) { +func TestExcludedPrefixesClient_Close_PrefixesAreRemoved(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) }) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx := context.Background() client := excludedprefixes.NewClient() - firstRequest := &networkservice.NetworkServiceRequest{ + request1 := &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{ Context: &networkservice.ConnectionContext{ IpContext: &networkservice.IPContext{ @@ -98,37 +143,145 @@ func TestExcludedPrefixesClient_Close_PrefixesAreEmpty(t *testing.T) { }, } - secondRequest := &networkservice.NetworkServiceRequest{ + resp1, err := client.Request(ctx, request1) + require.NoError(t, err) + + request2 := &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{}, } - firstResp, err := client.Request(ctx, firstRequest.Clone()) + _, err = client.Close(ctx, resp1) + require.NoError(t, err) + + respCheckEmpty, err := client.Request(ctx, request2) + require.NoError(t, err) + + require.Empty(t, respCheckEmpty.GetContext().GetIpContext().GetExcludedPrefixes()) +} + +func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") require.NoError(t, err) - expectedExcludedIPs := append(firstRequest.GetConnection().GetContext().GetIpContext().GetSrcIpAddrs(), - firstRequest.GetConnection().GetContext().GetIpContext().GetDstIpAddrs()...) + ctx := context.Background() + + excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} + + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request1 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } - secondResp, err := client.Request(ctx, secondRequest.Clone()) + resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request1) require.NoError(t, err) - excludedIPs := secondResp.GetContext().GetIpContext().GetExcludedPrefixes() - require.Equal(t, excludedIPs, expectedExcludedIPs) + srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + + destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() + require.Len(t, destIPs, 1) + + expectedExcludedPrefixes := make([]string, 0) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs[0], destIPs[0]) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) + expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) + + request2 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + server2 := adapters.NewServerToClient( + point2pointipam.NewServer(ipNet), + ) - _, err = client.Close(ctx, firstResp) + resp, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request2) require.NoError(t, err) - secondResp, err = client.Request(ctx, secondRequest.Clone()) + require.ElementsMatch(t, expectedExcludedPrefixes, resp.GetContext().GetIpContext().GetExcludedPrefixes()) +} + +func TestExcludedPrefixesClient_Request_PrefixesUnchangedAfterError(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") require.NoError(t, err) - require.Empty(t, secondResp.GetContext().GetIpContext().GetExcludedPrefixes()) + ctx := context.Background() + + excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} - _, err = client.Close(ctx, secondResp) + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request1 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request1) require.NoError(t, err) - require.Empty(t, secondResp.GetContext().GetIpContext().GetExcludedPrefixes()) + srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() + require.Len(t, srcIPs, 1) + + destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() + require.Len(t, destIPs, 1) + + expectedExcludedPrefixes := make([]string, 0) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs[0], destIPs[0]) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) + expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) + + // request with error + request2 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + server2 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + injecterror.NewClient(injecterror.WithError(errors.Errorf("Test error"))), + ) + + _, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request2) + require.Error(t, err) + + // third request to get the final prefixes + request3 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + server3 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + resp, err = chain.NewNetworkServiceClient(client, server3).Request(ctx, request3) + require.NoError(t, err) + + require.ElementsMatch(t, expectedExcludedPrefixes, resp.GetContext().GetIpContext().GetExcludedPrefixes()) } -func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { +func TestExcludedPrefixesClient_Request_SuccessfulRefresh(t *testing.T) { t.Cleanup(func() { goleak.VerifyNone(t) }) client := excludedprefixes.NewClient() @@ -138,11 +291,11 @@ func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { ctx := context.Background() - excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100"} + excludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} server1 := chain.NewNetworkServiceClient( adapters.NewServerToClient( - injectexcludedprefixes.NewServer(ctx, excludedPrefixes)), + injectexcludedprefixes.NewServer(excludedPrefixes)), adapters.NewServerToClient( point2pointipam.NewServer(ipNet)), ) @@ -151,33 +304,176 @@ func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { Connection: &networkservice.Connection{}, } - resp, err := chain.NewNetworkServiceClient(client, server1).Request(context.Background(), request.Clone()) + resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request) require.NoError(t, err) - // sanity checks - possibleIPs := []string{"172.16.0.97/32", "172.16.0.99/32", "172.16.0.101/32", "172.16.0.103/32"} srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() require.Len(t, srcIPs, 1) - require.Contains(t, possibleIPs, srcIPs[0]) destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() require.Len(t, destIPs, 1) - require.Contains(t, possibleIPs, destIPs[0]) - - require.NotEqual(t, srcIPs[0], destIPs[0]) + // expected excluded prefixes for refresh use-case + // src/dest IPs won't be present in IPContext expectedExcludedPrefixes := make([]string, 0) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, destIPs...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) + expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) + expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) + expectedExcludedPrefixes = excludedprefixes.Exclude(expectedExcludedPrefixes, []string{srcIPs[0], destIPs[0]}) + + // expected excluded prefixes for a non-refresh use-case + // src/dest IPs from first server should still be present in a request to another server + expectedExcludedPrefixes2 := make([]string, 0) + expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, srcIPs[0], destIPs[0]) + expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) + expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) + expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, excludedPrefixes...) + expectedExcludedPrefixes2 = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes2) + + resp, err = chain.NewNetworkServiceClient(client, server1).Request(ctx, request) + require.NoError(t, err) - // second request - server2 := adapters.NewServerToClient( - point2pointipam.NewServer(ipNet), + require.ElementsMatch(t, expectedExcludedPrefixes, resp.GetContext().GetIpContext().GetExcludedPrefixes()) + + server2 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), ) - resp, err = chain.NewNetworkServiceClient(client, server2).Request(context.Background(), request.Clone()) + request2 := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + resp, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request2) require.NoError(t, err) - require.Equal(t, resp.GetContext().GetIpContext().GetExcludedPrefixes(), expectedExcludedPrefixes) + require.ElementsMatch(t, expectedExcludedPrefixes2, resp.GetContext().GetIpContext().GetExcludedPrefixes()) +} + +func TestExcludedPrefixesClient_Request_EndpointConflicts(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") + require.NoError(t, err) + + ctx := context.Background() + + excludedPrefixes := []string{"172.16.0.100/32"} + + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + _, err = chain.NewNetworkServiceClient(client, server1).Request(ctx, request.Clone()) + require.NoError(t, err) + + // conflict with existing routes (172.16.0.96/32) + server2 := chain.NewNetworkServiceClient( + adapters.NewServerToClient(injectipam.NewServer( + []string{"172.16.0.101/32"}, + []string{"172.16.0.96/32"}, + []*networkservice.Route{ + { + Prefix: "172.16.0.101/32", + NextHop: "", + }, + }, + []*networkservice.Route{ + { + Prefix: "172.16.0.96/32", + NextHop: "", + }, + }))) + + _, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request.Clone()) + require.Error(t, err) + + // conflict with already excluded prefixes (172.16.0.100/32) + server3 := chain.NewNetworkServiceClient( + adapters.NewServerToClient(injectipam.NewServer( + []string{"172.16.0.100/32"}, + []string{"172.16.0.103/32"}, + []*networkservice.Route{ + { + Prefix: "172.16.0.103/32", + NextHop: "", + }, + }, + []*networkservice.Route{ + { + Prefix: "172.16.0.100/32", + NextHop: "", + }, + }))) + + _, err = chain.NewNetworkServiceClient(client, server3).Request(ctx, request.Clone()) + require.Error(t, err) +} + +func TestExcludedPrefixesClient_Request_EndpointConflictCloseError(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + client := excludedprefixes.NewClient() + + _, ipNet, err := net.ParseCIDR("172.16.0.96/29") + require.NoError(t, err) + + ctx := context.Background() + + excludedPrefixes := []string{"172.16.0.100/32"} + + server1 := chain.NewNetworkServiceClient( + adapters.NewServerToClient( + injectexcludedprefixes.NewServer(excludedPrefixes)), + adapters.NewServerToClient( + point2pointipam.NewServer(ipNet)), + ) + + request := &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{}, + } + + _, err = chain.NewNetworkServiceClient(client, server1).Request(ctx, request.Clone()) + require.NoError(t, err) + + // conflict with existing routes (172.16.0.96/32) + server2 := chain.NewNetworkServiceClient( + adapters.NewServerToClient(injectipam.NewServer( + []string{"172.16.0.101/32"}, + []string{"172.16.0.96/32"}, + []*networkservice.Route{ + { + Prefix: "172.16.0.101/32", + NextHop: "", + }, + }, + []*networkservice.Route{ + { + Prefix: "172.16.0.96/32", + NextHop: "", + }, + })), + injecterror.NewClient(injecterror.WithCloseErrorTimes(-1), injecterror.WithRequestErrorTimes(-2))) + + _, err = chain.NewNetworkServiceClient(client, server2).Request(ctx, request.Clone()) + require.Error(t, err) + require.Contains(t, err.Error(), "connection closed") +} + +func getPrefixes(routes []*networkservice.Route) []string { + var rv []string + for _, route := range routes { + rv = append(rv, route.GetPrefix()) + } + return rv } diff --git a/pkg/networkservice/common/excludedprefixes/server.go b/pkg/networkservice/common/excludedprefixes/server.go index 400e1f37a..eb96459ef 100644 --- a/pkg/networkservice/common/excludedprefixes/server.go +++ b/pkg/networkservice/common/excludedprefixes/server.go @@ -90,7 +90,7 @@ func (eps *excludedPrefixesServer) Request(ctx context.Context, request *network prefixes := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes() log.FromContext(ctx).Debugf("ExcludedPrefixesService: adding excluded prefixes to connection: %v", prefixes) ipCtx := conn.GetContext().GetIpContext() - ipCtx.ExcludedPrefixes = removeDuplicates(append(ipCtx.GetExcludedPrefixes(), prefixes...)) + ipCtx.ExcludedPrefixes = RemoveDuplicates(append(ipCtx.GetExcludedPrefixes(), prefixes...)) return next.Server(ctx).Request(ctx, request) } diff --git a/pkg/networkservice/common/excludedprefixes/utils.go b/pkg/networkservice/common/excludedprefixes/utils.go index 0aaa78199..88c4f793a 100644 --- a/pkg/networkservice/common/excludedprefixes/utils.go +++ b/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 { encountered := map[string]bool{} var result []string @@ -32,22 +33,19 @@ func removeDuplicates(elements []string) []string { return result } -func exclude(source, exclude []string) []string { - var result []string +// Exclude - excludes strings from a slice +func Exclude(source, exclude []string) []string { + var prefix string - var isExcluded bool - for _, s := range source { - isExcluded = false - for _, e := range exclude { - if s == e { - isExcluded = true - break + 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-- } } - - if !isExcluded { - result = append(result, s) - } } - return result + + return source } diff --git a/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go b/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go index e14ab2ebc..9e57f8f28 100644 --- a/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go +++ b/pkg/networkservice/utils/inject/injectexcludedprefixes/server.go @@ -26,7 +26,6 @@ import ( ) type injectExcludedPrefixesServer struct { - ctx context.Context prefixes []string } @@ -50,9 +49,8 @@ func (ieps *injectExcludedPrefixesServer) Close(ctx context.Context, connection } // NewServer - creates a networkservice.NetworkServiceServer chain element injecting specified excluded prefixes on Request into IP context -func NewServer(ctx context.Context, excludedPrefixes []string) networkservice.NetworkServiceServer { +func NewServer(excludedPrefixes []string) networkservice.NetworkServiceServer { return &injectExcludedPrefixesServer{ - ctx: ctx, prefixes: excludedPrefixes, } } diff --git a/pkg/networkservice/utils/inject/injectipam/doc.go b/pkg/networkservice/utils/inject/injectipam/doc.go new file mode 100644 index 000000000..0c4019680 --- /dev/null +++ b/pkg/networkservice/utils/inject/injectipam/doc.go @@ -0,0 +1,18 @@ +// Copyright (c) 2021 Doc.ai 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 injectipam provides a chain element injecting specified routes/IPs on Request into IP context +package injectipam diff --git a/pkg/networkservice/utils/inject/injectipam/server.go b/pkg/networkservice/utils/inject/injectipam/server.go new file mode 100644 index 000000000..096aa3273 --- /dev/null +++ b/pkg/networkservice/utils/inject/injectipam/server.go @@ -0,0 +1,65 @@ +// Copyright (c) 2021 Doc.ai 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 injectipam + +import ( + "context" + + "github.com/golang/protobuf/ptypes/empty" + "github.com/networkservicemesh/api/pkg/api/networkservice" + + "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" +) + +type injectIPAMServer struct { + srcIPs []string + dstIPs []string + srcRoutes []*networkservice.Route + dstRoutes []*networkservice.Route +} + +func (s *injectIPAMServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { + conn := request.GetConnection() + if conn.GetContext() == nil { + conn.Context = &networkservice.ConnectionContext{} + } + if conn.GetContext().GetIpContext() == nil { + conn.Context.IpContext = &networkservice.IPContext{} + } + + ipCtx := conn.GetContext().GetIpContext() + ipCtx.SrcIpAddrs = s.srcIPs + ipCtx.DstIpAddrs = s.dstIPs + ipCtx.SrcRoutes = s.srcRoutes + ipCtx.DstRoutes = s.dstRoutes + + return next.Server(ctx).Request(ctx, request) +} + +func (s *injectIPAMServer) Close(ctx context.Context, connection *networkservice.Connection) (*empty.Empty, error) { + return next.Server(ctx).Close(ctx, connection) +} + +// 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 { + return &injectIPAMServer{ + srcIPs: srcIPs, + dstIPs: dstIPs, + srcRoutes: srcRoutes, + dstRoutes: dstRoutes, + } +} From 0f5b9095f1510d50ea5db68f262734225132815b Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Fri, 17 Dec 2021 20:41:36 +0700 Subject: [PATCH 5/7] Addressed review comments Signed-off-by: Oleg Solodkov --- .../common/excludedprefixes/utils.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/networkservice/common/excludedprefixes/utils.go b/pkg/networkservice/common/excludedprefixes/utils.go index 88c4f793a..17176bf66 100644 --- a/pkg/networkservice/common/excludedprefixes/utils.go +++ b/pkg/networkservice/common/excludedprefixes/utils.go @@ -35,15 +35,18 @@ func RemoveDuplicates(elements []string) []string { // Exclude - excludes strings from a slice func Exclude(source, exclude []string) []string { - var prefix string + var s string + var excludeMap = make(map[string]struct{}) + + for _, e := range exclude { + excludeMap[e] = struct{}{} + } 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-- - } + s = source[i] + if _, ok := excludeMap[s]; ok { + source = append(source[:i], source[i+1:]...) + i-- } } From a25a0d8cae3a3f4bbdde7db4609f7858b79e07a5 Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Mon, 20 Dec 2021 21:12:42 +0700 Subject: [PATCH 6/7] Addressed review comments Signed-off-by: Oleg Solodkov --- .../common/excludedprefixes/client.go | 16 +++---- .../common/excludedprefixes/client_test.go | 42 +++---------------- .../common/excludedprefixes/server.go | 2 +- .../common/excludedprefixes/utils.go | 6 +-- 4 files changed, 16 insertions(+), 50 deletions(-) diff --git a/pkg/networkservice/common/excludedprefixes/client.go b/pkg/networkservice/common/excludedprefixes/client.go index 97decb71a..f9607df8e 100644 --- a/pkg/networkservice/common/excludedprefixes/client.go +++ b/pkg/networkservice/common/excludedprefixes/client.go @@ -64,10 +64,10 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network logger.Debugf("Adding new excluded IPs to the request: %+v", epc.excludedPrefixes) newExcludedPrefixes = ipCtx.GetExcludedPrefixes() newExcludedPrefixes = append(newExcludedPrefixes, epc.excludedPrefixes...) - newExcludedPrefixes = RemoveDuplicates(newExcludedPrefixes) + newExcludedPrefixes = removeDuplicates(newExcludedPrefixes) // excluding IPs for current request/connection before calling next client for the refresh use-case - newExcludedPrefixes = Exclude(newExcludedPrefixes, append(ipCtx.GetSrcIpAddrs(), ipCtx.GetDstIpAddrs()...)) + newExcludedPrefixes = exclude(newExcludedPrefixes, append(ipCtx.GetSrcIpAddrs(), ipCtx.GetDstIpAddrs()...)) logger.Debugf("Excluded prefixes from request - %+v", newExcludedPrefixes) ipCtx.ExcludedPrefixes = newExcludedPrefixes @@ -108,7 +108,7 @@ func (epc *excludedPrefixesClient) Request(ctx context.Context, request *network epc.excludedPrefixes = append(epc.excludedPrefixes, getRoutePrefixes(respIPContext.GetSrcRoutes())...) epc.excludedPrefixes = append(epc.excludedPrefixes, getRoutePrefixes(respIPContext.GetDstRoutes())...) epc.excludedPrefixes = append(epc.excludedPrefixes, respIPContext.GetExcludedPrefixes()...) - epc.excludedPrefixes = RemoveDuplicates(epc.excludedPrefixes) + epc.excludedPrefixes = removeDuplicates(epc.excludedPrefixes) logger.Debugf("Added excluded prefixes: %+v", epc.excludedPrefixes) }) @@ -120,11 +120,11 @@ func (epc *excludedPrefixesClient) Close(ctx context.Context, conn *networkservi ipCtx := conn.GetContext().GetIpContext() <-epc.executor.AsyncExec(func() { - epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) - epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) - epc.excludedPrefixes = Exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetSrcRoutes())) - epc.excludedPrefixes = Exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetDstRoutes())) - epc.excludedPrefixes = Exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetSrcIpAddrs()) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetDstIpAddrs()) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetSrcRoutes())) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, getRoutePrefixes(ipCtx.GetDstRoutes())) + epc.excludedPrefixes = exclude(epc.excludedPrefixes, ipCtx.GetExcludedPrefixes()) logger.Debugf("Excluded prefixes after closing connection: %+v", epc.excludedPrefixes) }) diff --git a/pkg/networkservice/common/excludedprefixes/client_test.go b/pkg/networkservice/common/excludedprefixes/client_test.go index fbb5e25a1..cfdd04b5a 100644 --- a/pkg/networkservice/common/excludedprefixes/client_test.go +++ b/pkg/networkservice/common/excludedprefixes/client_test.go @@ -94,11 +94,7 @@ func TestExcludedPrefixesClient_Request_SrcAndDestPrefixesAreDifferent(t *testin resp, err := chain.NewNetworkServiceClient(client, server1).Request(ctx, request1) require.NoError(t, err) - expectedExcludedIPs := append(resp.GetContext().GetIpContext().GetSrcIpAddrs(), - resp.GetContext().GetIpContext().GetDstIpAddrs()...) - expectedExcludedIPs = append(expectedExcludedIPs, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) - expectedExcludedIPs = append(expectedExcludedIPs, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) - expectedExcludedIPs = excludedprefixes.RemoveDuplicates(expectedExcludedIPs) + expectedExcludedIPs := []string{"172.16.0.100/32", "172.16.0.101/32"} srcIPs := resp.GetContext().GetIpContext().GetSrcIpAddrs() require.Len(t, srcIPs, 1) @@ -191,12 +187,7 @@ func TestExcludedPrefixesClient_Request_WithExcludedPrefixes(t *testing.T) { destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() require.Len(t, destIPs, 1) - expectedExcludedPrefixes := make([]string, 0) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs[0], destIPs[0]) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) - expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) + expectedExcludedPrefixes := []string{"172.16.0.99/32", "172.16.0.97/32", "172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} request2 := &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{}, @@ -244,12 +235,7 @@ func TestExcludedPrefixesClient_Request_PrefixesUnchangedAfterError(t *testing.T destIPs := resp.GetContext().GetIpContext().GetDstIpAddrs() require.Len(t, destIPs, 1) - expectedExcludedPrefixes := make([]string, 0) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, srcIPs[0], destIPs[0]) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) - expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) + expectedExcludedPrefixes := []string{"172.16.0.99/32", "172.16.0.97/32", "172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} // request with error request2 := &networkservice.NetworkServiceRequest{ @@ -315,21 +301,11 @@ func TestExcludedPrefixesClient_Request_SuccessfulRefresh(t *testing.T) { // expected excluded prefixes for refresh use-case // src/dest IPs won't be present in IPContext - expectedExcludedPrefixes := make([]string, 0) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) - expectedExcludedPrefixes = append(expectedExcludedPrefixes, excludedPrefixes...) - expectedExcludedPrefixes = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes) - expectedExcludedPrefixes = excludedprefixes.Exclude(expectedExcludedPrefixes, []string{srcIPs[0], destIPs[0]}) + expectedExcludedPrefixes := []string{"172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} // expected excluded prefixes for a non-refresh use-case // src/dest IPs from first server should still be present in a request to another server - expectedExcludedPrefixes2 := make([]string, 0) - expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, srcIPs[0], destIPs[0]) - expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, getPrefixes(resp.GetContext().GetIpContext().GetSrcRoutes())...) - expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, getPrefixes(resp.GetContext().GetIpContext().GetDstRoutes())...) - expectedExcludedPrefixes2 = append(expectedExcludedPrefixes2, excludedPrefixes...) - expectedExcludedPrefixes2 = excludedprefixes.RemoveDuplicates(expectedExcludedPrefixes2) + expectedExcludedPrefixes2 := []string{"172.16.0.99/32", "172.16.0.97/32", "172.16.0.96/32", "172.16.0.98/32", "172.16.0.100/32"} resp, err = chain.NewNetworkServiceClient(client, server1).Request(ctx, request) require.NoError(t, err) @@ -469,11 +445,3 @@ func TestExcludedPrefixesClient_Request_EndpointConflictCloseError(t *testing.T) require.Error(t, err) require.Contains(t, err.Error(), "connection closed") } - -func getPrefixes(routes []*networkservice.Route) []string { - var rv []string - for _, route := range routes { - rv = append(rv, route.GetPrefix()) - } - return rv -} diff --git a/pkg/networkservice/common/excludedprefixes/server.go b/pkg/networkservice/common/excludedprefixes/server.go index eb96459ef..400e1f37a 100644 --- a/pkg/networkservice/common/excludedprefixes/server.go +++ b/pkg/networkservice/common/excludedprefixes/server.go @@ -90,7 +90,7 @@ func (eps *excludedPrefixesServer) Request(ctx context.Context, request *network prefixes := eps.prefixPool.Load().(*ippool.PrefixPool).GetPrefixes() log.FromContext(ctx).Debugf("ExcludedPrefixesService: adding excluded prefixes to connection: %v", prefixes) ipCtx := conn.GetContext().GetIpContext() - ipCtx.ExcludedPrefixes = RemoveDuplicates(append(ipCtx.GetExcludedPrefixes(), prefixes...)) + ipCtx.ExcludedPrefixes = removeDuplicates(append(ipCtx.GetExcludedPrefixes(), prefixes...)) return next.Server(ctx).Request(ctx, request) } diff --git a/pkg/networkservice/common/excludedprefixes/utils.go b/pkg/networkservice/common/excludedprefixes/utils.go index 17176bf66..78a3cdbf9 100644 --- a/pkg/networkservice/common/excludedprefixes/utils.go +++ b/pkg/networkservice/common/excludedprefixes/utils.go @@ -18,8 +18,7 @@ package excludedprefixes -// RemoveDuplicates - removes duplicate strings from a slice -func RemoveDuplicates(elements []string) []string { +func removeDuplicates(elements []string) []string { encountered := map[string]bool{} var result []string @@ -33,8 +32,7 @@ func RemoveDuplicates(elements []string) []string { return result } -// Exclude - excludes strings from a slice -func Exclude(source, exclude []string) []string { +func exclude(source, exclude []string) []string { var s string var excludeMap = make(map[string]struct{}) From 8aa4a0beca709ae8761beca237c8ffefd873a2c1 Mon Sep 17 00:00:00 2001 From: Oleg Solodkov Date: Tue, 21 Dec 2021 02:06:41 +0700 Subject: [PATCH 7/7] Addressed review comments Signed-off-by: Oleg Solodkov --- .../common/excludedprefixes/client_test.go | 80 ++++++++++--------- .../{injectipam => injectipcontext}/doc.go | 4 +- .../{injectipam => injectipcontext}/server.go | 32 +++----- 3 files changed, 58 insertions(+), 58 deletions(-) rename pkg/networkservice/utils/inject/{injectipam => injectipcontext}/doc.go (84%) rename pkg/networkservice/utils/inject/{injectipam => injectipcontext}/server.go (60%) diff --git a/pkg/networkservice/common/excludedprefixes/client_test.go b/pkg/networkservice/common/excludedprefixes/client_test.go index cfdd04b5a..ecb397b65 100644 --- a/pkg/networkservice/common/excludedprefixes/client_test.go +++ b/pkg/networkservice/common/excludedprefixes/client_test.go @@ -32,7 +32,7 @@ import ( "github.com/networkservicemesh/sdk/pkg/networkservice/ipam/point2pointipam" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectexcludedprefixes" - "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectipam" + "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectipcontext" ) func TestExcludedPrefixesClient_Request_SanityCheck(t *testing.T) { @@ -355,19 +355,21 @@ func TestExcludedPrefixesClient_Request_EndpointConflicts(t *testing.T) { // conflict with existing routes (172.16.0.96/32) server2 := chain.NewNetworkServiceClient( - adapters.NewServerToClient(injectipam.NewServer( - []string{"172.16.0.101/32"}, - []string{"172.16.0.96/32"}, - []*networkservice.Route{ - { - Prefix: "172.16.0.101/32", - NextHop: "", + adapters.NewServerToClient(injectipcontext.NewServer( + &networkservice.IPContext{ + SrcIpAddrs: []string{"172.16.0.101/32"}, + DstIpAddrs: []string{"172.16.0.96/32"}, + SrcRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.101/32", + NextHop: "", + }, }, - }, - []*networkservice.Route{ - { - Prefix: "172.16.0.96/32", - NextHop: "", + DstRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.96/32", + NextHop: "", + }, }, }))) @@ -376,19 +378,21 @@ func TestExcludedPrefixesClient_Request_EndpointConflicts(t *testing.T) { // conflict with already excluded prefixes (172.16.0.100/32) server3 := chain.NewNetworkServiceClient( - adapters.NewServerToClient(injectipam.NewServer( - []string{"172.16.0.100/32"}, - []string{"172.16.0.103/32"}, - []*networkservice.Route{ - { - Prefix: "172.16.0.103/32", - NextHop: "", + adapters.NewServerToClient(injectipcontext.NewServer( + &networkservice.IPContext{ + SrcIpAddrs: []string{"172.16.0.100/32"}, + DstIpAddrs: []string{"172.16.0.103/32"}, + SrcRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.103/32", + NextHop: "", + }, }, - }, - []*networkservice.Route{ - { - Prefix: "172.16.0.100/32", - NextHop: "", + DstRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.100/32", + NextHop: "", + }, }, }))) @@ -424,19 +428,21 @@ func TestExcludedPrefixesClient_Request_EndpointConflictCloseError(t *testing.T) // conflict with existing routes (172.16.0.96/32) server2 := chain.NewNetworkServiceClient( - adapters.NewServerToClient(injectipam.NewServer( - []string{"172.16.0.101/32"}, - []string{"172.16.0.96/32"}, - []*networkservice.Route{ - { - Prefix: "172.16.0.101/32", - NextHop: "", + adapters.NewServerToClient(injectipcontext.NewServer( + &networkservice.IPContext{ + SrcIpAddrs: []string{"172.16.0.101/32"}, + DstIpAddrs: []string{"172.16.0.96/32"}, + SrcRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.101/32", + NextHop: "", + }, }, - }, - []*networkservice.Route{ - { - Prefix: "172.16.0.96/32", - NextHop: "", + DstRoutes: []*networkservice.Route{ + { + Prefix: "172.16.0.96/32", + NextHop: "", + }, }, })), injecterror.NewClient(injecterror.WithCloseErrorTimes(-1), injecterror.WithRequestErrorTimes(-2))) diff --git a/pkg/networkservice/utils/inject/injectipam/doc.go b/pkg/networkservice/utils/inject/injectipcontext/doc.go similarity index 84% rename from pkg/networkservice/utils/inject/injectipam/doc.go rename to pkg/networkservice/utils/inject/injectipcontext/doc.go index 0c4019680..b899186e2 100644 --- a/pkg/networkservice/utils/inject/injectipam/doc.go +++ b/pkg/networkservice/utils/inject/injectipcontext/doc.go @@ -14,5 +14,5 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package injectipam provides a chain element injecting specified routes/IPs on Request into IP context -package injectipam +// Package injectipcontext provides a chain element injecting specified IPContext on Request +package injectipcontext diff --git a/pkg/networkservice/utils/inject/injectipam/server.go b/pkg/networkservice/utils/inject/injectipcontext/server.go similarity index 60% rename from pkg/networkservice/utils/inject/injectipam/server.go rename to pkg/networkservice/utils/inject/injectipcontext/server.go index 096aa3273..6f298b3ac 100644 --- a/pkg/networkservice/utils/inject/injectipam/server.go +++ b/pkg/networkservice/utils/inject/injectipcontext/server.go @@ -14,7 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package injectipam +package injectipcontext import ( "context" @@ -25,14 +25,11 @@ import ( "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" ) -type injectIPAMServer struct { - srcIPs []string - dstIPs []string - srcRoutes []*networkservice.Route - dstRoutes []*networkservice.Route +type injectIPContext struct { + ipContext *networkservice.IPContext } -func (s *injectIPAMServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { +func (s *injectIPContext) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { conn := request.GetConnection() if conn.GetContext() == nil { conn.Context = &networkservice.ConnectionContext{} @@ -42,24 +39,21 @@ func (s *injectIPAMServer) Request(ctx context.Context, request *networkservice. } ipCtx := conn.GetContext().GetIpContext() - ipCtx.SrcIpAddrs = s.srcIPs - ipCtx.DstIpAddrs = s.dstIPs - ipCtx.SrcRoutes = s.srcRoutes - ipCtx.DstRoutes = s.dstRoutes + ipCtx.SrcIpAddrs = s.ipContext.GetSrcIpAddrs() + ipCtx.DstIpAddrs = s.ipContext.GetDstIpAddrs() + ipCtx.SrcRoutes = s.ipContext.GetSrcRoutes() + ipCtx.DstRoutes = s.ipContext.GetDstRoutes() return next.Server(ctx).Request(ctx, request) } -func (s *injectIPAMServer) Close(ctx context.Context, connection *networkservice.Connection) (*empty.Empty, error) { +func (s *injectIPContext) Close(ctx context.Context, connection *networkservice.Connection) (*empty.Empty, error) { return next.Server(ctx).Close(ctx, connection) } -// 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 { - return &injectIPAMServer{ - srcIPs: srcIPs, - dstIPs: dstIPs, - srcRoutes: srcRoutes, - dstRoutes: dstRoutes, +// NewServer - creates a networkservice.NetworkServiceServer chain element injecting specified IPContext on Request +func NewServer(ipContext *networkservice.IPContext) networkservice.NetworkServiceServer { + return &injectIPContext{ + ipContext: ipContext, } }