Rework begin to simplify merge of Context

This is a simplification and rework in response to #1234

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

pkg/networkservice/common/begin/client.go

@@ -67,7 +67,7 @@ func (b *beginClient) Request(ctx context.Context, request *networkservice.Netwo
 		}
 
 		ctx = withEventFactory(ctx, eventFactoryClient)
-		request.Connection = mergeConnection(eventFactoryClient.returnedConnection, request.GetConnection(), eventFactoryClient.request.GetConnection())
+		request.Connection = mergeConnection(eventFactoryClient.request.GetConnection(), request.GetConnection())
 		conn, err = next.Client(ctx).Request(ctx, request, opts...)
 		if err != nil {
 			if eventFactoryClient.state != established {
@@ -80,8 +80,6 @@ func (b *beginClient) Request(ctx context.Context, request *networkservice.Netwo
 		eventFactoryClient.request.Connection = conn.Clone()
 		eventFactoryClient.opts = opts
 		eventFactoryClient.state = established
-
-		eventFactoryClient.returnedConnection = conn.Clone()
 	})
 	return conn, err
 }

pkg/networkservice/common/begin/event_factory.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
 //
@@ -45,14 +45,13 @@ type EventFactory interface {
 }
 
 type eventFactoryClient struct {
-	state              connectionState
-	executor           serialize.Executor
-	ctxFunc            func() (context.Context, context.CancelFunc)
-	request            *networkservice.NetworkServiceRequest
-	returnedConnection *networkservice.Connection
-	opts               []grpc.CallOption
-	client             networkservice.NetworkServiceClient
-	afterCloseFunc     func()
+	state          connectionState
+	executor       serialize.Executor
+	ctxFunc        func() (context.Context, context.CancelFunc)
+	request        *networkservice.NetworkServiceRequest
+	opts           []grpc.CallOption
+	client         networkservice.NetworkServiceClient
+	afterCloseFunc func()
 }
 
 func newEventFactoryClient(ctx context.Context, afterClose func(), opts ...grpc.CallOption) *eventFactoryClient {
@@ -142,13 +141,12 @@ func (f *eventFactoryClient) Close(opts ...Option) <-chan error {
 var _ EventFactory = &eventFactoryClient{}
 
 type eventFactoryServer struct {
-	state              connectionState
-	executor           serialize.Executor
-	ctxFunc            func() (context.Context, context.CancelFunc)
-	request            *networkservice.NetworkServiceRequest
-	returnedConnection *networkservice.Connection
-	afterCloseFunc     func()
-	server             networkservice.NetworkServiceServer
+	state          connectionState
+	executor       serialize.Executor
+	ctxFunc        func() (context.Context, context.CancelFunc)
+	request        *networkservice.NetworkServiceRequest
+	afterCloseFunc func()
+	server         networkservice.NetworkServiceServer
 }
 
 func newEventFactoryServer(ctx context.Context, afterClose func()) *eventFactoryServer {

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,45 +21,32 @@ import (
 	"google.golang.org/protobuf/proto"
 )
 
-func mergeConnection(returnedConnection, requestedConnection, connection *networkservice.Connection) *networkservice.Connection {
-	if returnedConnection == nil || connection == nil {
+// mergeConnection - deals explicitly with the problem of "What should we allow the 'main' of a client executable
+// to update from outside the chain after the initial request for a connection.
+// One minor point to keep in mind here is that a passthrough NSE, one that is a server that incorporates a client
+// into its own chain, begin's at the server side... and so the server's use of client like functions are 'inside the chain'
+func mergeConnection(returnedConnection, requestedConnection *networkservice.Connection) *networkservice.Connection {
+	// If this request has has yet to return successfully, go with the requesteConnection
+	if returnedConnection == nil {
 		return requestedConnection
 	}
-	conn := connection.Clone()
+
+	// Clone the previously returned connection
+	conn := returnedConnection.Clone()
+
+	// If the Request is asking for a new NSE, use that
 	if returnedConnection.GetNetworkServiceEndpointName() != requestedConnection.GetNetworkServiceEndpointName() {
 		conn.NetworkServiceEndpointName = requestedConnection.GetNetworkServiceEndpointName()
 	}
-	conn.Context = mergeConnectionContext(returnedConnection.GetContext(), requestedConnection.GetContext(), connection.GetContext())
-	return conn
-}
 
-func mergeConnectionContext(returnedConnectionContext, requestedConnectionContext, connectioncontext *networkservice.ConnectionContext) *networkservice.ConnectionContext {
-	rv := proto.Clone(connectioncontext).(*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())
+	// Ifthe Request is asking for a change in ConnectionContext, propagate that.
+	if !proto.Equal(returnedConnection.GetContext(), requestedConnection.GetContext()) {
+		conn.Context = proto.Clone(requestedConnection.GetContext()).(*networkservice.ConnectionContext)
 	}
-	return rv
-}
 
-func mergeMapStringString(returnedMap, requestedMap, mapMap map[string]string) map[string]string {
-	// clone the map
-	rv := make(map[string]string)
-	for k, v := range mapMap {
-		rv[k] = v
-	}
-
-	for k, v := range returnedMap {
-		requestedValue, 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 a key is in returnedMap and not in requestedMap, delete it
-		if !ok {
-			delete(rv, k)
-		}
-	}
+	// Note: We are disallowing at this time changes in requested NetworkService, Mechanism, Labels, or Path.
+	// In the future it may be worth permitting changes in the Labels.
+	// It probably is not a good idea to allow changes in the NetworkService or Path here.
 
-	return rv
+	return conn
 }

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

pkg/networkservice/common/begin/server.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
 //
@@ -73,8 +73,6 @@ func (b *beginServer) Request(ctx context.Context, request *networkservice.Netwo
 		eventFactoryServer.request = request.Clone()
 		eventFactoryServer.request.Connection = conn.Clone()
 		eventFactoryServer.state = established
-
-		eventFactoryServer.returnedConnection = conn.Clone()
 	})
 	return conn, err
 }