Merge pull request #72 from Bolodya1997/fix

Rename interfaces back on connection close

.github/workflows/ci.yaml

@@ -31,7 +31,7 @@ jobs:
       - name: Build
         run: go build -race  ./...
       - name: Test
-        run: sudo go test -race ./...
+        run: sudo -E PATH="$PATH" bash -c "go test -race ./..."
 
   golangci-lint:
     name: golangci-lint

go.sum

@@ -63,6 +63,7 @@ github.com/dgryski/go-sip13 v0.0.0-20181026042036-e10d5fee7954/go.mod h1:vAd38F8
 github.com/edwarnicke/exechelper v1.0.2/go.mod h1:/T271jtNX/ND4De6pa2aRy2+8sNtyCDB1A2pp4M+fUs=
 github.com/edwarnicke/grpcfd v0.0.0-20200920223154-d5b6e1f19bd0/go.mod h1:rHihB9YvNMixz8rS+ZbwosI2kj65VLkeyYAI2M+/cGA=
 github.com/edwarnicke/serialize v0.0.0-20200705214914-ebc43080eecf/go.mod h1:XvbCO/QGsl3X8RzjBMoRpkm54FIAZH5ChK2j+aox7pw=
+github.com/edwarnicke/serialize v1.0.7 h1:geX8vmyu8Ij2S5fFIXjy9gBDkKxXnrMIzMoDvV0Ddac=
 github.com/edwarnicke/serialize v1.0.7/go.mod h1:y79KgU2P7ALH/4j37uTSIdNavHFNttqN7pzO6Y8B2aw=
 github.com/envoyproxy/go-control-plane v0.9.0/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=
 github.com/envoyproxy/go-control-plane v0.9.1-0.20191026205805-5f8ba28d4473/go.mod h1:YTl/9mNaCwkRvm6d1a2C3ymFceY/DCBVvsKhRF0iEA4=

pkg/kernel/networkservice/inject/server.go

@@ -46,12 +46,15 @@ func (s *injectServer) Request(ctx context.Context, request *networkservice.Netw
 
 	connID := request.GetConnection().GetId()
 	mech := kernel.ToMechanism(request.GetConnection().GetMechanism())
+	if mech == nil {
+		return next.Server(ctx).Request(ctx, request)
+	}
 
-	forwarderNetNS, err := nshandle.Current()
+	curNetNS, err := nshandle.Current()
 	if err != nil {
 		return nil, err
 	}
-	defer func() { _ = forwarderNetNS.Close() }()
+	defer func() { _ = curNetNS.Close() }()
 
 	var clientNetNS netns.NsHandle
 	clientNetNS, err = nshandle.FromURL(mech.GetNetNSURL())
@@ -61,15 +64,15 @@ func (s *injectServer) Request(ctx context.Context, request *networkservice.Netw
 	defer func() { _ = clientNetNS.Close() }()
 
 	ifName := mech.GetInterfaceName(request.GetConnection())
-	err = moveInterfaceToAnotherNamespace(ifName, forwarderNetNS, clientNetNS)
+	err = moveInterfaceToAnotherNamespace(ifName, curNetNS, curNetNS, clientNetNS)
 	if err != nil {
 		return nil, err
 	}
 	logEntry.Infof("moved network interface %s into the Client's namespace for connection %s", ifName, connID)
 
 	conn, err := next.Server(ctx).Request(ctx, request)
 	if err != nil {
-		if errMovingBack := moveInterfaceToAnotherNamespace(ifName, clientNetNS, forwarderNetNS); errMovingBack != nil {
+		if errMovingBack := moveInterfaceToAnotherNamespace(ifName, curNetNS, clientNetNS, curNetNS); errMovingBack != nil {
 			logEntry.Warnf("failed to move network interface %s into the Forwarder's namespace for connection %s", ifName, connID)
 		} else {
 			logEntry.Infof("moved network interface %s into the Forwarder's namespace for connection %s", ifName, connID)
@@ -81,33 +84,43 @@ func (s *injectServer) Request(ctx context.Context, request *networkservice.Netw
 func (s *injectServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
 	logEntry := log.Entry(ctx).WithField("injectServer", "Close")
 
-	mech := kernel.ToMechanism(conn.GetMechanism())
+	_, err := next.Server(ctx).Close(ctx, conn)
 
-	forwarderNetNS, err := nshandle.Current()
-	if err != nil {
-		return nil, err
-	}
-	defer func() { _ = forwarderNetNS.Close() }()
+	var injectErr error
+	if mech := kernel.ToMechanism(conn.GetMechanism()); mech != nil {
+		var curNetNS, clientNetNS netns.NsHandle
+		var ifName string
 
-	var clientNetNS netns.NsHandle
-	clientNetNS, err = nshandle.FromURL(mech.GetNetNSURL())
-	if err != nil {
-		return nil, err
-	}
-	defer func() { _ = clientNetNS.Close() }()
+		if curNetNS, injectErr = nshandle.Current(); injectErr != nil {
+			goto exit
+		}
+		defer func() { _ = curNetNS.Close() }()
 
-	ifName := mech.GetInterfaceName(conn)
-	err = moveInterfaceToAnotherNamespace(ifName, clientNetNS, forwarderNetNS)
-	if err != nil {
-		return nil, err
+		if clientNetNS, injectErr = nshandle.FromURL(mech.GetNetNSURL()); injectErr != nil {
+			goto exit
+		}
+		defer func() { _ = clientNetNS.Close() }()
+
+		ifName = mech.GetInterfaceName(conn)
+		if injectErr = moveInterfaceToAnotherNamespace(ifName, curNetNS, clientNetNS, curNetNS); injectErr != nil {
+			goto exit
+		}
+
+		logEntry.Infof("moved network interface %s into the Forwarder's namespace for connection %s", ifName, conn.GetId())
 	}
-	logEntry.Infof("moved network interface %s into the Forwarder's namespace for connection %s", ifName, conn.GetId())
 
-	return next.Server(ctx).Close(ctx, conn)
+exit:
+	if err != nil && injectErr != nil {
+		return nil, errors.Wrap(err, injectErr.Error())
+	}
+	if injectErr != nil {
+		return nil, injectErr
+	}
+	return &empty.Empty{}, err
 }
 
-func moveInterfaceToAnotherNamespace(ifName string, fromNetNS, toNetNS netns.NsHandle) error {
-	return nshandle.RunIn(fromNetNS, toNetNS, func() error {
+func moveInterfaceToAnotherNamespace(ifName string, curNetNS, fromNetNS, toNetNS netns.NsHandle) error {
+	return nshandle.RunIn(curNetNS, fromNetNS, func() error {
 		link, err := netlink.LinkByName(ifName)
 		if err != nil {
 			return errors.Wrapf(err, "failed to get net interface: %v", ifName)

pkg/kernel/networkservice/rename/meta_data.go

@@ -0,0 +1,36 @@
+// Copyright (c) 2020 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 rename
+
+import (
+	"context"
+
+	"github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata"
+)
+
+type keyType string
+
+func storeOldIfName(ctx context.Context, id, oldIfName string) {
+	metadata.Map(ctx, false).Store(keyType(id), oldIfName)
+}
+
+func loadOldIfName(ctx context.Context, id string) (string, bool) {
+	if raw, ok := metadata.Map(ctx, false).Load(keyType(id)); ok {
+		return raw.(string), true
+	}
+	return "", false
+}

pkg/kernel/networkservice/rename/server.go

@@ -21,34 +21,80 @@ import (
 	"context"
 
 	"github.com/golang/protobuf/ptypes/empty"
+	"github.com/google/uuid"
+	"github.com/pkg/errors"
+	"github.com/vishvananda/netlink"
+
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
 	"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
-	"github.com/pkg/errors"
-	"github.com/vishvananda/netlink"
 
 	"github.com/networkservicemesh/sdk-kernel/pkg/kernel/networkservice/vfconfig"
 )
 
-type renameServer struct{}
+type renameServer struct {
+	// If we have 2 rename servers in chain, they should manage their own mappings.
+	id string
+}
 
 // NewServer returns a new link rename server chain element
 func NewServer() networkservice.NetworkServiceServer {
-	return &renameServer{}
+	return &renameServer{
+		id: uuid.New().String(),
+	}
 }
 
 func (s *renameServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) {
-	if mech := kernel.ToMechanism(request.GetConnection().GetMechanism()); mech != nil {
-		if vfConfig := vfconfig.Config(ctx); vfConfig != nil {
-			if ifName := mech.GetInterfaceName(request.GetConnection()); vfConfig.VFInterfaceName != ifName {
-				if err := renameLink(vfConfig.VFInterfaceName, ifName); err != nil {
-					return nil, err
-				}
-				vfConfig.VFInterfaceName = ifName
-			}
+	mech := kernel.ToMechanism(request.GetConnection().GetMechanism())
+	if mech == nil {
+		return next.Server(ctx).Request(ctx, request)
+	}
+	ifName := mech.GetInterfaceName(request.GetConnection())
+
+	vfConfig := vfconfig.Config(ctx)
+	if vfConfig == nil || vfConfig.VFInterfaceName == ifName {
+		return next.Server(ctx).Request(ctx, request)
+	}
+
+	if err := renameLink(vfConfig.VFInterfaceName, ifName); err != nil {
+		return nil, err
+	}
+	oldIfName := vfConfig.VFInterfaceName
+	vfConfig.VFInterfaceName = ifName
+
+	conn, err := next.Server(ctx).Request(ctx, request)
+	if err != nil {
+		if renameErr := renameLink(ifName, oldIfName); renameErr != nil {
+			err = errors.Wrapf(err, renameErr.Error())
 		}
+		return nil, err
 	}
-	return next.Server(ctx).Request(ctx, request)
+
+	if _, renamed := loadOldIfName(ctx, s.id); !renamed {
+		storeOldIfName(ctx, s.id, oldIfName)
+	}
+
+	return conn, nil
+}
+
+func (s *renameServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
+	_, err := next.Server(ctx).Close(ctx, conn)
+
+	var renameErr error
+	if mech := kernel.ToMechanism(conn.GetMechanism()); mech != nil {
+		ifName := mech.GetInterfaceName(conn)
+		if oldIfName, renamed := loadOldIfName(ctx, s.id); renamed {
+			renameErr = renameLink(ifName, oldIfName)
+		}
+	}
+
+	if err != nil && renameErr != nil {
+		return nil, errors.Wrap(err, renameErr.Error())
+	}
+	if renameErr != nil {
+		return nil, renameErr
+	}
+	return &empty.Empty{}, err
 }
 
 func renameLink(oldName, newName string) error {
@@ -63,7 +109,3 @@ func renameLink(oldName, newName string) error {
 
 	return nil
 }
-
-func (s *renameServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) {
-	return next.Server(ctx).Close(ctx, conn)
-}