From a835121d9cbe2d323b62412933a0013e06e2391e Mon Sep 17 00:00:00 2001 From: Vladimir Popov Date: Fri, 6 Nov 2020 11:12:06 +0700 Subject: [PATCH 1/5] Rename kernel interface back on Close Signed-off-by: Vladimir Popov --- pkg/kernel/networkservice/rename/gen.go | 25 +++++++ .../networkservice/rename/renames_map.gen.go | 73 +++++++++++++++++++ pkg/kernel/networkservice/rename/server.go | 72 ++++++++++++++---- 3 files changed, 154 insertions(+), 16 deletions(-) create mode 100644 pkg/kernel/networkservice/rename/gen.go create mode 100644 pkg/kernel/networkservice/rename/renames_map.gen.go diff --git a/pkg/kernel/networkservice/rename/gen.go b/pkg/kernel/networkservice/rename/gen.go new file mode 100644 index 00000000..2b40dd6d --- /dev/null +++ b/pkg/kernel/networkservice/rename/gen.go @@ -0,0 +1,25 @@ +// Copyright (c) 2020 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 rename + +import ( + "sync" +) + +//go:generate go-syncmap -output renames_map.gen.go -type renamesMap + +type renamesMap sync.Map diff --git a/pkg/kernel/networkservice/rename/renames_map.gen.go b/pkg/kernel/networkservice/rename/renames_map.gen.go new file mode 100644 index 00000000..ec1ba9eb --- /dev/null +++ b/pkg/kernel/networkservice/rename/renames_map.gen.go @@ -0,0 +1,73 @@ +// Code generated by "-output renames_map.gen.go -type renamesMap -output renames_map.gen.go -type renamesMap"; DO NOT EDIT. +package rename + +import ( + "sync" // Used by sync.Map. +) + +// Generate code that will fail if the constants change value. +func _() { + // An "cannot convert renamesMap literal (type renamesMap) to type sync.Map" compiler error signifies that the base type have changed. + // Re-run the go-syncmap command to generate them again. + _ = (sync.Map)(renamesMap{}) +} + +var _nil_renamesMap_string_value = func() (val string) { return }() + +// Load returns the value stored in the map for a key, or nil if no +// value is present. +// The ok result indicates whether value was found in the map. +func (m *renamesMap) Load(key string) (string, bool) { + value, ok := (*sync.Map)(m).Load(key) + if value == nil { + return _nil_renamesMap_string_value, ok + } + return value.(string), ok +} + +// Store sets the value for a key. +func (m *renamesMap) Store(key string, value string) { + (*sync.Map)(m).Store(key, value) +} + +// LoadOrStore returns the existing value for the key if present. +// Otherwise, it stores and returns the given value. +// The loaded result is true if the value was loaded, false if stored. +func (m *renamesMap) LoadOrStore(key string, value string) (string, bool) { + actual, loaded := (*sync.Map)(m).LoadOrStore(key, value) + if actual == nil { + return _nil_renamesMap_string_value, loaded + } + return actual.(string), loaded +} + +// LoadAndDelete deletes the value for a key, returning the previous value if any. +// The loaded result reports whether the key was present. +func (m *renamesMap) LoadAndDelete(key string) (value string, loaded bool) { + actual, loaded := (*sync.Map)(m).LoadAndDelete(key) + if actual == nil { + return _nil_renamesMap_string_value, loaded + } + return actual.(string), loaded +} + +// Delete deletes the value for a key. +func (m *renamesMap) Delete(key string) { + (*sync.Map)(m).Delete(key) +} + +// Range calls f sequentially for each key and value present in the map. +// If f returns false, range stops the iteration. +// +// Range does not necessarily correspond to any consistent snapshot of the Map's +// contents: no key will be visited more than once, but if the value for any key +// is stored or deleted concurrently, Range may reflect any mapping for that key +// from any point during the Range call. +// +// Range may be O(N) with the number of elements in the map even if f returns +// false after a constant number of calls. +func (m *renamesMap) Range(f func(key string, value string) bool) { + (*sync.Map)(m).Range(func(key, value interface{}) bool { + return f(key.(string), value.(string)) + }) +} diff --git a/pkg/kernel/networkservice/rename/server.go b/pkg/kernel/networkservice/rename/server.go index 8b8478fd..a24dc42c 100644 --- a/pkg/kernel/networkservice/rename/server.go +++ b/pkg/kernel/networkservice/rename/server.go @@ -21,16 +21,19 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" + "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 { + renames renamesMap +} // NewServer returns a new link rename server chain element func NewServer() networkservice.NetworkServiceServer { @@ -38,17 +41,58 @@ func NewServer() networkservice.NetworkServiceServer { } 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 + } + + if n, renamed := s.renames.LoadAndDelete(vfConfig.VFInterfaceName); renamed { + oldIfName = n + } + s.renames.Store(ifName, 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) + oldIfName, renamed := s.renames.LoadAndDelete(ifName) + if renamed { + renameErr = renameLink(ifName, oldIfName) } } - return next.Server(ctx).Request(ctx, request) + + 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 +107,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) -} From b8b98629804c54ea26a8f0d5fc230e0c213a1ffc Mon Sep 17 00:00:00 2001 From: Vladimir Popov Date: Fri, 6 Nov 2020 11:28:23 +0700 Subject: [PATCH 2/5] First close next chain, then injact interface back Signed-off-by: Vladimir Popov --- pkg/kernel/networkservice/inject/server.go | 49 ++++++++++++++-------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/pkg/kernel/networkservice/inject/server.go b/pkg/kernel/networkservice/inject/server.go index 16478253..ce6900f4 100644 --- a/pkg/kernel/networkservice/inject/server.go +++ b/pkg/kernel/networkservice/inject/server.go @@ -46,6 +46,9 @@ 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() if err != nil { @@ -81,29 +84,39 @@ 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 forwarderNetNS, 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 forwarderNetNS, injectErr = nshandle.Current(); injectErr != nil { + goto exit + } + defer func() { _ = forwarderNetNS.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, clientNetNS, forwarderNetNS); 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 { From be68e5f2442c3c2cc2df3b4b452a4b4aff2276c2 Mon Sep 17 00:00:00 2001 From: Vladimir Popov Date: Fri, 6 Nov 2020 14:35:29 +0700 Subject: [PATCH 3/5] Fix injectServer Signed-off-by: Vladimir Popov --- pkg/kernel/networkservice/inject/server.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/kernel/networkservice/inject/server.go b/pkg/kernel/networkservice/inject/server.go index ce6900f4..fb57b2a1 100644 --- a/pkg/kernel/networkservice/inject/server.go +++ b/pkg/kernel/networkservice/inject/server.go @@ -50,11 +50,11 @@ func (s *injectServer) Request(ctx context.Context, request *networkservice.Netw 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()) @@ -64,7 +64,7 @@ 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 } @@ -72,7 +72,7 @@ func (s *injectServer) Request(ctx context.Context, request *networkservice.Netw 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) @@ -88,13 +88,13 @@ func (s *injectServer) Close(ctx context.Context, conn *networkservice.Connectio var injectErr error if mech := kernel.ToMechanism(conn.GetMechanism()); mech != nil { - var forwarderNetNS, clientNetNS netns.NsHandle + var curNetNS, clientNetNS netns.NsHandle var ifName string - if forwarderNetNS, injectErr = nshandle.Current(); injectErr != nil { + if curNetNS, injectErr = nshandle.Current(); injectErr != nil { goto exit } - defer func() { _ = forwarderNetNS.Close() }() + defer func() { _ = curNetNS.Close() }() if clientNetNS, injectErr = nshandle.FromURL(mech.GetNetNSURL()); injectErr != nil { goto exit @@ -102,7 +102,7 @@ func (s *injectServer) Close(ctx context.Context, conn *networkservice.Connectio defer func() { _ = clientNetNS.Close() }() ifName = mech.GetInterfaceName(conn) - if injectErr = moveInterfaceToAnotherNamespace(ifName, clientNetNS, forwarderNetNS); injectErr != nil { + if injectErr = moveInterfaceToAnotherNamespace(ifName, curNetNS, clientNetNS, curNetNS); injectErr != nil { goto exit } @@ -119,8 +119,8 @@ exit: 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) From 60ae16f94f08cded43f2f0c667fb5b04b952ae92 Mon Sep 17 00:00:00 2001 From: Vladimir Popov Date: Thu, 19 Nov 2020 10:22:48 +0700 Subject: [PATCH 4/5] Fix sudo go test Signed-off-by: Vladimir Popov --- .github/workflows/ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index e97ed4e7..20f59805 100644 --- a/.github/workflows/ci.yaml +++ b/.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 From 213a3f99cfdbffcc05c174f0cc6954f7694e6de4 Mon Sep 17 00:00:00 2001 From: Vladimir Popov Date: Wed, 2 Dec 2020 16:35:28 +0700 Subject: [PATCH 5/5] Rework rename server to use metadata Signed-off-by: Vladimir Popov --- go.sum | 1 + .../rename/{gen.go => meta_data.go} | 19 ++++- .../networkservice/rename/renames_map.gen.go | 73 ------------------- pkg/kernel/networkservice/rename/server.go | 16 ++-- 4 files changed, 25 insertions(+), 84 deletions(-) rename pkg/kernel/networkservice/rename/{gen.go => meta_data.go} (57%) delete mode 100644 pkg/kernel/networkservice/rename/renames_map.gen.go diff --git a/go.sum b/go.sum index b71c0006..d5da6798 100644 --- a/go.sum +++ b/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= diff --git a/pkg/kernel/networkservice/rename/gen.go b/pkg/kernel/networkservice/rename/meta_data.go similarity index 57% rename from pkg/kernel/networkservice/rename/gen.go rename to pkg/kernel/networkservice/rename/meta_data.go index 2b40dd6d..cc43cdf8 100644 --- a/pkg/kernel/networkservice/rename/gen.go +++ b/pkg/kernel/networkservice/rename/meta_data.go @@ -1,4 +1,4 @@ -// Copyright (c) 2020 Cisco and/or its affiliates. +// Copyright (c) 2020 Doc.ai and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -17,9 +17,20 @@ package rename import ( - "sync" + "context" + + "github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata" ) -//go:generate go-syncmap -output renames_map.gen.go -type renamesMap +type keyType string + +func storeOldIfName(ctx context.Context, id, oldIfName string) { + metadata.Map(ctx, false).Store(keyType(id), oldIfName) +} -type renamesMap sync.Map +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 +} diff --git a/pkg/kernel/networkservice/rename/renames_map.gen.go b/pkg/kernel/networkservice/rename/renames_map.gen.go deleted file mode 100644 index ec1ba9eb..00000000 --- a/pkg/kernel/networkservice/rename/renames_map.gen.go +++ /dev/null @@ -1,73 +0,0 @@ -// Code generated by "-output renames_map.gen.go -type renamesMap -output renames_map.gen.go -type renamesMap"; DO NOT EDIT. -package rename - -import ( - "sync" // Used by sync.Map. -) - -// Generate code that will fail if the constants change value. -func _() { - // An "cannot convert renamesMap literal (type renamesMap) to type sync.Map" compiler error signifies that the base type have changed. - // Re-run the go-syncmap command to generate them again. - _ = (sync.Map)(renamesMap{}) -} - -var _nil_renamesMap_string_value = func() (val string) { return }() - -// Load returns the value stored in the map for a key, or nil if no -// value is present. -// The ok result indicates whether value was found in the map. -func (m *renamesMap) Load(key string) (string, bool) { - value, ok := (*sync.Map)(m).Load(key) - if value == nil { - return _nil_renamesMap_string_value, ok - } - return value.(string), ok -} - -// Store sets the value for a key. -func (m *renamesMap) Store(key string, value string) { - (*sync.Map)(m).Store(key, value) -} - -// LoadOrStore returns the existing value for the key if present. -// Otherwise, it stores and returns the given value. -// The loaded result is true if the value was loaded, false if stored. -func (m *renamesMap) LoadOrStore(key string, value string) (string, bool) { - actual, loaded := (*sync.Map)(m).LoadOrStore(key, value) - if actual == nil { - return _nil_renamesMap_string_value, loaded - } - return actual.(string), loaded -} - -// LoadAndDelete deletes the value for a key, returning the previous value if any. -// The loaded result reports whether the key was present. -func (m *renamesMap) LoadAndDelete(key string) (value string, loaded bool) { - actual, loaded := (*sync.Map)(m).LoadAndDelete(key) - if actual == nil { - return _nil_renamesMap_string_value, loaded - } - return actual.(string), loaded -} - -// Delete deletes the value for a key. -func (m *renamesMap) Delete(key string) { - (*sync.Map)(m).Delete(key) -} - -// Range calls f sequentially for each key and value present in the map. -// If f returns false, range stops the iteration. -// -// Range does not necessarily correspond to any consistent snapshot of the Map's -// contents: no key will be visited more than once, but if the value for any key -// is stored or deleted concurrently, Range may reflect any mapping for that key -// from any point during the Range call. -// -// Range may be O(N) with the number of elements in the map even if f returns -// false after a constant number of calls. -func (m *renamesMap) Range(f func(key string, value string) bool) { - (*sync.Map)(m).Range(func(key, value interface{}) bool { - return f(key.(string), value.(string)) - }) -} diff --git a/pkg/kernel/networkservice/rename/server.go b/pkg/kernel/networkservice/rename/server.go index a24dc42c..8aaf7165 100644 --- a/pkg/kernel/networkservice/rename/server.go +++ b/pkg/kernel/networkservice/rename/server.go @@ -21,6 +21,7 @@ import ( "context" "github.com/golang/protobuf/ptypes/empty" + "github.com/google/uuid" "github.com/pkg/errors" "github.com/vishvananda/netlink" @@ -32,12 +33,15 @@ import ( ) type renameServer struct { - renames renamesMap + // 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) { @@ -66,10 +70,9 @@ func (s *renameServer) Request(ctx context.Context, request *networkservice.Netw return nil, err } - if n, renamed := s.renames.LoadAndDelete(vfConfig.VFInterfaceName); renamed { - oldIfName = n + if _, renamed := loadOldIfName(ctx, s.id); !renamed { + storeOldIfName(ctx, s.id, oldIfName) } - s.renames.Store(ifName, oldIfName) return conn, nil } @@ -80,8 +83,7 @@ func (s *renameServer) Close(ctx context.Context, conn *networkservice.Connectio var renameErr error if mech := kernel.ToMechanism(conn.GetMechanism()); mech != nil { ifName := mech.GetInterfaceName(conn) - oldIfName, renamed := s.renames.LoadAndDelete(ifName) - if renamed { + if oldIfName, renamed := loadOldIfName(ctx, s.id); renamed { renameErr = renameLink(ifName, oldIfName) } }