diff --git a/pkg/networkservice/common/begin/client.go b/pkg/networkservice/common/begin/client.go index 48d8803c6..4c5d73ae8 100644 --- a/pkg/networkservice/common/begin/client.go +++ b/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 } diff --git a/pkg/networkservice/common/begin/event_factory.go b/pkg/networkservice/common/begin/event_factory.go index a4335fa4f..d500faf7c 100644 --- a/pkg/networkservice/common/begin/event_factory.go +++ b/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 { diff --git a/pkg/networkservice/common/begin/merge.go b/pkg/networkservice/common/begin/merge.go index 3d92174b9..970680bae 100644 --- a/pkg/networkservice/common/begin/merge.go +++ b/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 } 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 000000000..e45aead45 --- /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()) +} diff --git a/pkg/networkservice/common/begin/server.go b/pkg/networkservice/common/begin/server.go index 4cb49028d..5fe8216a9 100644 --- a/pkg/networkservice/common/begin/server.go +++ b/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 }