Improve merge of ConnectionContext with external requests (#1238)

Also improved greatly documentation in various places

Should resolve #1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>

pkg/networkservice/common/begin/merge.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2021 Cisco and/or its affiliates.
+// Copyright (c) 2021-2022 Cisco and/or its affiliates.
 //
 // SPDX-License-Identifier: Apache-2.0
 //
@@ -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 {

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