From 566543dc882daed846e92a118d7bf2b05db69296 Mon Sep 17 00:00:00 2001 From: Ed Warnicke Date: Sun, 13 Mar 2022 18:47:54 -0500 Subject: [PATCH] Improve merge of ConnectionContext with external requests Also improved greatly documentation in various places Should resolve https://github.com/networkservicemesh/sdk/pull/1234 Signed-off-by: Ed Warnicke --- pkg/networkservice/common/begin/merge.go | 44 ++++++---- .../common/begin/rerequest_client_test.go | 81 +++++++++++++++++++ 2 files changed, 111 insertions(+), 14 deletions(-) create mode 100644 pkg/networkservice/common/begin/rerequest_client_test.go diff --git a/pkg/networkservice/common/begin/merge.go b/pkg/networkservice/common/begin/merge.go index 3d92174b9b..59e6f1c02b 100644 --- a/pkg/networkservice/common/begin/merge.go +++ b/pkg/networkservice/common/begin/merge.go @@ -21,39 +21,55 @@ import ( "google.golang.org/protobuf/proto" ) -func mergeConnection(returnedConnection, requestedConnection, connection *networkservice.Connection) *networkservice.Connection { - if returnedConnection == nil || connection == nil { +// mergeConnection - preforms the three way merge of the returnedConnection, requestedConnection and connection +// returnedConnection - the Connection last returned from the begin.Request(...) +// requestedConnection - the Connection passed in to the begin.Request(...) +// currentConnection - the last value for the Connection in EventFactory. Since Refreshes, Heals, etc +// can result in changes that have *not* been returned from begin.Request(...) because +// they originated in events internal to the chain (instead of external via calls to +// begin.Request(...)) it is possible that connection differs from returnedConnection +func mergeConnection(returnedConnection, requestedConnection, currentConnection *networkservice.Connection) *networkservice.Connection { + if returnedConnection == nil || currentConnection == nil { return requestedConnection } - conn := connection.Clone() + conn := currentConnection.Clone() if returnedConnection.GetNetworkServiceEndpointName() != requestedConnection.GetNetworkServiceEndpointName() { conn.NetworkServiceEndpointName = requestedConnection.GetNetworkServiceEndpointName() } - conn.Context = mergeConnectionContext(returnedConnection.GetContext(), requestedConnection.GetContext(), connection.GetContext()) + conn.Context = mergeConnectionContext(returnedConnection.GetContext(), requestedConnection.GetContext(), currentConnection.GetContext()) return conn } -func mergeConnectionContext(returnedConnectionContext, requestedConnectionContext, connectioncontext *networkservice.ConnectionContext) *networkservice.ConnectionContext { - rv := proto.Clone(connectioncontext).(*networkservice.ConnectionContext) +func mergeConnectionContext(returnedConnectionContext, requestedConnectionContext, currentConnectionContext *networkservice.ConnectionContext) *networkservice.ConnectionContext { + if currentConnectionContext == nil { + return requestedConnectionContext + } + rv := proto.Clone(currentConnectionContext).(*networkservice.ConnectionContext) if !proto.Equal(returnedConnectionContext, requestedConnectionContext) { - // TODO: IPContext, DNSContext, EthernetContext, do we need to do MTU? - rv.ExtraContext = mergeMapStringString(returnedConnectionContext.GetExtraContext(), requestedConnectionContext.GetExtraContext(), connectioncontext.GetExtraContext()) + rv.IpContext = requestedConnectionContext.GetIpContext() + rv.EthernetContext = requestedConnectionContext.GetEthernetContext() + rv.DnsContext = requestedConnectionContext.GetDnsContext() + rv.MTU = requestedConnectionContext.GetMTU() + rv.ExtraContext = mergeMapStringString(returnedConnectionContext.GetExtraContext(), requestedConnectionContext.GetExtraContext(), currentConnectionContext.GetExtraContext()) } return rv } -func mergeMapStringString(returnedMap, requestedMap, mapMap map[string]string) map[string]string { - // clone the map +func mergeMapStringString(returnedMap, requestedMap, currentMap map[string]string) map[string]string { + // clone the currentMap rv := make(map[string]string) - for k, v := range mapMap { + for k, v := range currentMap { rv[k] = v } + // Only intentional changes between the returnedMap (which was values last returned from calls to begin.Request(...)) + // and requestedMap (the values passed into begin.Request for this call) are considered for application to the existing + // map (currentMap - the last set of values remembered by the EventFactory). for k, v := range returnedMap { - requestedValue, ok := requestedMap[k] + srcValue, ok := requestedMap[k] // If a key is in returnedMap and its value differs from requestedMap, update the value - if ok && requestedValue != v { - rv[k] = requestedValue + if ok && srcValue != v { + rv[k] = srcValue } // If a key is in returnedMap and not in requestedMap, delete it if !ok { diff --git a/pkg/networkservice/common/begin/rerequest_client_test.go b/pkg/networkservice/common/begin/rerequest_client_test.go new file mode 100644 index 0000000000..e45aead450 --- /dev/null +++ b/pkg/networkservice/common/begin/rerequest_client_test.go @@ -0,0 +1,81 @@ +// Copyright (c) 2022 Cisco 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 begin_test + +import ( + "context" + "testing" + + "github.com/networkservicemesh/api/pkg/api/networkservice" + "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel" + "github.com/stretchr/testify/require" + "go.uber.org/goleak" + "google.golang.org/protobuf/proto" + + "github.com/networkservicemesh/sdk/pkg/networkservice/common/begin" + "github.com/networkservicemesh/sdk/pkg/networkservice/core/chain" +) + +func TestReRequestClient(t *testing.T) { + t.Cleanup(func() { goleak.VerifyNone(t) }) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + client := chain.NewNetworkServiceClient( + begin.NewClient(), + ) + + connOriginal, err := client.Request(ctx, &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{ + Id: "id", + }, + }) + + require.NoError(t, err) + require.NotNil(t, connOriginal) + + conn := connOriginal.Clone() + conn.Context = &networkservice.ConnectionContext{ + IpContext: &networkservice.IPContext{ + SrcIpAddrs: []string{"10.0.0.1/32"}, + }, + EthernetContext: &networkservice.EthernetContext{ + SrcMac: "00:00:00:00:00:00", + }, + DnsContext: &networkservice.DNSContext{ + Configs: []*networkservice.DNSConfig{ + { + DnsServerIps: []string{"1.1.1.1"}, + }, + }, + }, + ExtraContext: map[string]string{"foo": "bar"}, + } + conn.Mechanism = kernel.New("") + conn.Labels = map[string]string{"foo": "bar"} + + connReturned, err := client.Request(ctx, &networkservice.NetworkServiceRequest{ + Connection: conn, + }) + + require.NoError(t, err) + require.NotNil(t, connReturned) + require.Equal(t, connOriginal.GetMechanism(), connReturned.GetMechanism()) + require.True(t, proto.Equal(conn.GetContext(), connReturned.GetContext())) + require.Equal(t, connOriginal.GetLabels(), connReturned.GetLabels()) +}