From 74848aa0eefbdc93d7f11d69e14c1ab0cc3c3bc7 Mon Sep 17 00:00:00 2001 From: Denis Tingaikin <49399980+denis-tingajkin@users.noreply.github.com> Date: Mon, 9 Nov 2020 23:35:09 +0700 Subject: [PATCH] fix: Filter mechanism chain element works incorrectly in remote case (#576) * Revert "fix: Filter mechanism chain element works incorrectly in remote case (#575)" This reverts commit 39537ac8948dcb90efe9b2545b02d656e676f1ab. Signed-off-by: denis-tingajkin * Fix 573 Signed-off-by: denis-tingajkin * fix typo Signed-off-by: denis-tingajkin --- .../common/filtermechanisms/README.md | 94 ---------------- .../common/filtermechanisms/const.go | 23 ---- .../common/filtermechanisms/options.go | 27 ----- .../common/filtermechanisms/server.go | 26 ++--- .../common/filtermechanisms/server_test.go | 102 +++++------------- pkg/registry/common/endpointurls/gen.go | 6 +- pkg/registry/common/endpointurls/server.go | 14 +-- .../common/endpointurls/sync_map.gen.go | 46 ++++++++ .../common/endpointurls/sync_set.gen.go | 74 ------------- pkg/registry/common/interpose/server.go | 7 +- 10 files changed, 99 insertions(+), 320 deletions(-) delete mode 100644 pkg/networkservice/common/filtermechanisms/README.md delete mode 100644 pkg/networkservice/common/filtermechanisms/const.go delete mode 100644 pkg/networkservice/common/filtermechanisms/options.go create mode 100644 pkg/registry/common/endpointurls/sync_map.gen.go delete mode 100644 pkg/registry/common/endpointurls/sync_set.gen.go diff --git a/pkg/networkservice/common/filtermechanisms/README.md b/pkg/networkservice/common/filtermechanisms/README.md deleted file mode 100644 index fe763e1fe..000000000 --- a/pkg/networkservice/common/filtermechanisms/README.md +++ /dev/null @@ -1,94 +0,0 @@ -## Intro - -Package `filtermechanisms` filters out remote mechanisms if communicating by remote url -filters out local mechanisms otherwise. Can be used with eNSMGR. - -## Options - -Supported option `WithExternalThreshold` that sets max path segments count for eNSMgrs. - -## Examples -Note: **Is local** true when path segment index less than localThreshold and nsmgr knows NSE url - - 1. Local: - ``` - Scheme: nsc-->NSMgr-->cross-nse-->NSMgr-->nse - NSMgr knows nse: true - filterMechanismsServer.localThreshold: 5 - len(request.Connection.Path.PathSegments): 4 - Is local: true -``` - - 2. Local eNSM: - ``` - Scheme: nsc-->eNSMgr-->nse - eNSMgr knows nse: true - filterMechanismsServer.localThreshold: 3 - len(request.Connection.Path.PathSegments): 2 - Is local: true -``` - - 3. Remote eNSM: - ``` - Scheme: nsc-->eNSMgr1-->eNSMgr2-->nse - eNSMgr1: - knows nse: false - filterMechanismsServer.localThreshold = 3 - len(request.Connection.Path.PathSegments) = 2 - Is local: false - - eNSMgr2: - knows nse: true - filterMechanismsServer.localThreshold = 3 - len(request.Connection.Path.PathSegments) = 3 - Is local: false -``` - - 4. Remote NSMgr + eNSMGR - ``` - Scheme: nsc-->NSMgr1-->cross-nse1-->NSMgr1-->eNSMgr1-->nse - NSMgr1: - knows nse: false - filterMechanismsServer.localThreshold = 5 - len(request.Connection.Path.PathSegments) = 4 - Is local: false - - eNSMgr1: - knows nse: true - filterMechanismsServer.localThreshold = 3 - len(request.Connection.Path.PathSegments) = 5 - Is local: false -``` - - 5. Remote eNSMGR + NSMgr - ``` - Scheme: nsc-->eNSMgr1-->NSMgr1-->cross-nse1-->NSMgr1-->eNSMgr2-->nse - eNSMgr1: - knows nse: false - filterMechanismsServer.localThreshold = 3 - len(request.Connection.Path.PathSegments) = 2 - Is local: false - - NSMgr1: - knows nse: true - filterMechanismsServer.localThreshold = 5 - len(request.Connection.Path.PathSegments) = 5 - Is local: false -``` - -6. Remote - -``` - Scheme: nsc-->NSMgr1-->cross-nse1-->NSMgr1-->NSMgr2-->cross-nse2-->NSMgr2-->nse - NSMgr2: - knows nse: false - filterMechanismsServer.localThreshold = 5 - len(request.Connection.Path.PathSegments) = 4 - Is local: false - - NSMgr1: - knows nse: true - filterMechanismsServer.localThreshold = 5 - len(request.Connection.Path.PathSegments) = 7 - Is local: false -``` \ No newline at end of file diff --git a/pkg/networkservice/common/filtermechanisms/const.go b/pkg/networkservice/common/filtermechanisms/const.go deleted file mode 100644 index 63be5c81b..000000000 --- a/pkg/networkservice/common/filtermechanisms/const.go +++ /dev/null @@ -1,23 +0,0 @@ -// 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 filtermechanisms - -const ( - // See at examples section in README.md - defaultThreshold = 5 - externalThreshold = 3 -) diff --git a/pkg/networkservice/common/filtermechanisms/options.go b/pkg/networkservice/common/filtermechanisms/options.go deleted file mode 100644 index 5b1b60647..000000000 --- a/pkg/networkservice/common/filtermechanisms/options.go +++ /dev/null @@ -1,27 +0,0 @@ -// 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 filtermechanisms - -// Option is option applier for filterMechanismsServer -type Option func(server *filterMechanismsServer) - -// WithExternalThreshold configures filterMechanismsServer to work with external NSMgr. -func WithExternalThreshold() Option { - return Option(func(server *filterMechanismsServer) { - server.localThreshold = externalThreshold - }) -} diff --git a/pkg/networkservice/common/filtermechanisms/server.go b/pkg/networkservice/common/filtermechanisms/server.go index 8a1f75df2..ec76d81cd 100644 --- a/pkg/networkservice/common/filtermechanisms/server.go +++ b/pkg/networkservice/common/filtermechanisms/server.go @@ -21,6 +21,8 @@ package filtermechanisms import ( "context" + "github.com/networkservicemesh/sdk/pkg/registry/common/interpose" + "github.com/networkservicemesh/sdk/pkg/tools/clienturlctx" "github.com/golang/protobuf/ptypes/empty" @@ -33,30 +35,22 @@ import ( ) type filterMechanismsServer struct { - urls endpointurls.Set - localThreshold int + nses endpointurls.Map } // NewServer - filters out remote mechanisms if connection is received from a unix file socket, otherwise filters -// out local mechanisms. -func NewServer(registryServer *registry.NetworkServiceEndpointRegistryServer, options ...Option) networkservice.NetworkServiceServer { - result := &filterMechanismsServer{ - localThreshold: defaultThreshold, - } - for _, applyOption := range options { - applyOption(result) - } - *registryServer = endpointurls.NewNetworkServiceEndpointRegistryServer(&result.urls) +// out local mechanisms +func NewServer(registryServer *registry.NetworkServiceEndpointRegistryServer) networkservice.NetworkServiceServer { + result := &filterMechanismsServer{} + *registryServer = endpointurls.NewNetworkServiceEndpointRegistryServer(&result.nses) return result } func (f *filterMechanismsServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { u := clienturlctx.ClientURL(ctx) - - if _, ok := f.urls.Load(*u); ok { - filteredMechanisms := filterMechanismsByCls(request.GetMechanismPreferences(), cls.LOCAL) - if len(filteredMechanisms) > 0 { - request.MechanismPreferences = filteredMechanisms + if name, ok := f.nses.Load(*u); ok { + if !interpose.Is(name) { + request.MechanismPreferences = filterMechanismsByCls(request.GetMechanismPreferences(), cls.LOCAL) } } else { request.MechanismPreferences = filterMechanismsByCls(request.GetMechanismPreferences(), cls.REMOTE) diff --git a/pkg/networkservice/common/filtermechanisms/server_test.go b/pkg/networkservice/common/filtermechanisms/server_test.go index e83766132..b6163db55 100644 --- a/pkg/networkservice/common/filtermechanisms/server_test.go +++ b/pkg/networkservice/common/filtermechanisms/server_test.go @@ -21,8 +21,6 @@ import ( "net/url" "testing" - "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" - "github.com/networkservicemesh/api/pkg/api/networkservice" "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/cls" "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel" @@ -36,72 +34,6 @@ import ( "github.com/networkservicemesh/sdk/pkg/tools/clienturlctx" ) -func TestFilterMechanisms_RemoteExternal(t *testing.T) { - request := &networkservice.NetworkServiceRequest{ - MechanismPreferences: []*networkservice.Mechanism{ - { - Cls: cls.REMOTE, - Type: vxlan.MECHANISM, - }, - { - Cls: cls.LOCAL, - Type: kernel.MECHANISM, - }, - }, - Connection: &networkservice.Connection{ - Path: &networkservice.Path{ - PathSegments: make([]*networkservice.PathSegment, 3), - }, - }, - } - - var localRegistryServer registry.NetworkServiceEndpointRegistryServer - localServer := filtermechanisms.NewServer(&localRegistryServer) - var remoteRegistryServer registry.NetworkServiceEndpointRegistryServer - remoteServer := filtermechanisms.NewServer(&remoteRegistryServer, filtermechanisms.WithExternalThreshold()) - u := &url.URL{Path: "test"} - _, _ = remoteRegistryServer.Register(context.Background(), ®istry.NetworkServiceEndpoint{ - Url: u.String(), - }) - _, err := next.NewNetworkServiceServer(localServer, remoteServer).Request(clienturlctx.WithClientURL(context.Background(), u), request) - require.NoError(t, err) - require.Len(t, request.MechanismPreferences, 1) - require.Equal(t, cls.REMOTE, request.MechanismPreferences[0].Cls) -} - -func TestFilterMechanisms_Remote(t *testing.T) { - request := &networkservice.NetworkServiceRequest{ - MechanismPreferences: []*networkservice.Mechanism{ - { - Cls: cls.REMOTE, - Type: vxlan.MECHANISM, - }, - { - Cls: cls.LOCAL, - Type: kernel.MECHANISM, - }, - }, - Connection: &networkservice.Connection{ - Path: &networkservice.Path{ - PathSegments: make([]*networkservice.PathSegment, 5), - }, - }, - } - - var localRegistryServer registry.NetworkServiceEndpointRegistryServer - localServer := filtermechanisms.NewServer(&localRegistryServer) - var remoteRegistryServer registry.NetworkServiceEndpointRegistryServer - remoteServer := filtermechanisms.NewServer(&remoteRegistryServer) - u := &url.URL{Path: "test"} - _, _ = remoteRegistryServer.Register(context.Background(), ®istry.NetworkServiceEndpoint{ - Url: u.String(), - }) - _, err := next.NewNetworkServiceServer(localServer, remoteServer).Request(clienturlctx.WithClientURL(context.Background(), u), request) - require.NoError(t, err) - require.Len(t, request.MechanismPreferences, 1) - require.Equal(t, cls.REMOTE, request.MechanismPreferences[0].Cls) -} - func TestFilterMechanismsServer_Request(t *testing.T) { request := func() *networkservice.NetworkServiceRequest { return &networkservice.NetworkServiceRequest{ @@ -127,6 +59,7 @@ func TestFilterMechanismsServer_Request(t *testing.T) { } samples := []struct { Name string + EndpointName string ClientURL *url.URL RegisterURLs []url.URL ClsResult string @@ -140,12 +73,25 @@ func TestFilterMechanismsServer_Request(t *testing.T) { Host: "localhost:5000", }, }, - ClsResult: cls.LOCAL, + EndpointName: "nse-1", + ClsResult: cls.LOCAL, }, { - Name: "Remote mechanisms", - ClientURL: &url.URL{Scheme: "tcp", Host: "localhost:5000"}, - ClsResult: cls.REMOTE, + Name: "Remote mechanisms", + ClientURL: &url.URL{Scheme: "tcp", Host: "localhost:5000"}, + EndpointName: "nse-1", + ClsResult: cls.REMOTE, + }, + { + Name: "Pass mechanisms to forwarder", + ClientURL: &url.URL{Scheme: "tcp", Host: "localhost:5000"}, + EndpointName: "interpose-nse#nse-1", + RegisterURLs: []url.URL{ + { + Scheme: "tcp", + Host: "localhost:5000", + }, + }, }, } @@ -154,7 +100,8 @@ func TestFilterMechanismsServer_Request(t *testing.T) { s := filtermechanisms.NewServer(®istryServer) for _, u := range sample.RegisterURLs { _, err := registryServer.Register(context.Background(), ®istry.NetworkServiceEndpoint{ - Url: u.String(), + Name: sample.EndpointName, + Url: u.String(), }) require.NoError(t, err) } @@ -163,8 +110,13 @@ func TestFilterMechanismsServer_Request(t *testing.T) { _, err := s.Request(ctx, req) require.NoError(t, err) require.NotEmpty(t, req.MechanismPreferences) - for _, m := range req.MechanismPreferences { - require.Equal(t, sample.ClsResult, m.Cls, "filtermechanisms chain element should properly filter mechanisms") + + if sample.ClsResult != "" { + for _, m := range req.MechanismPreferences { + require.Equal(t, sample.ClsResult, m.Cls, "filtermechanisms chain element should properly filter mechanisms") + } + } else { + require.Equal(t, request().String(), req.String()) } } } diff --git a/pkg/registry/common/endpointurls/gen.go b/pkg/registry/common/endpointurls/gen.go index 8e66d8a2d..1ea42d789 100644 --- a/pkg/registry/common/endpointurls/gen.go +++ b/pkg/registry/common/endpointurls/gen.go @@ -20,8 +20,8 @@ import ( "sync" ) -//go:generate go-syncmap -output sync_set.gen.go -type Set +//go:generate go-syncmap -output sync_map.gen.go -type Map -// Set is like a Go map[url.URL]struct{} but is safe for concurrent use +// Map is like a Go map[url.URL]string but is safe for concurrent use // by multiple goroutines without additional locking or coordination -type Set sync.Map +type Map sync.Map diff --git a/pkg/registry/common/endpointurls/server.go b/pkg/registry/common/endpointurls/server.go index 446502fe6..4ed769e34 100644 --- a/pkg/registry/common/endpointurls/server.go +++ b/pkg/registry/common/endpointurls/server.go @@ -14,7 +14,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package endpointurls provides registry.NetworkServiceEndpointRegistryServer that can be injected in the chain of registry.NetworkServiceEndpointRegistryServer to get an actual set of registry.NetworkServiceEndpoint URLs. +// Package endpointurls provides registry.NetworkServiceEndpointRegistryServer that can be injected in the chain of registry.NetworkServiceEndpointRegistryServer to get an actual nses of registry.NetworkServiceEndpoint URLs. package endpointurls import ( @@ -28,12 +28,12 @@ import ( ) type endpointURLsServer struct { - set *Set + nses *Map } func (e *endpointURLsServer) Register(ctx context.Context, endpoint *registry.NetworkServiceEndpoint) (*registry.NetworkServiceEndpoint, error) { if u, err := url.Parse(endpoint.Url); err == nil { - e.set.Store(*u, struct{}{}) + e.nses.Store(*u, endpoint.Name) } return next.NetworkServiceEndpointRegistryServer(ctx).Register(ctx, endpoint) } @@ -44,12 +44,12 @@ func (e *endpointURLsServer) Find(query *registry.NetworkServiceEndpointQuery, s func (e *endpointURLsServer) Unregister(ctx context.Context, endpoint *registry.NetworkServiceEndpoint) (*empty.Empty, error) { if u, err := url.Parse(endpoint.Url); err == nil { - e.set.Delete(*u) + e.nses.Delete(*u) } return next.NetworkServiceEndpointRegistryServer(ctx).Unregister(ctx, endpoint) } -// NewNetworkServiceEndpointRegistryServer returns new registry.NetworkServiceEndpointRegistryServer with injected endpoint urls set -func NewNetworkServiceEndpointRegistryServer(set *Set) registry.NetworkServiceEndpointRegistryServer { - return &endpointURLsServer{set: set} +// NewNetworkServiceEndpointRegistryServer returns new registry.NetworkServiceEndpointRegistryServer with injected endpoint urls nses +func NewNetworkServiceEndpointRegistryServer(m *Map) registry.NetworkServiceEndpointRegistryServer { + return &endpointURLsServer{nses: m} } diff --git a/pkg/registry/common/endpointurls/sync_map.gen.go b/pkg/registry/common/endpointurls/sync_map.gen.go new file mode 100644 index 000000000..d5ee43d1e --- /dev/null +++ b/pkg/registry/common/endpointurls/sync_map.gen.go @@ -0,0 +1,46 @@ +// Code generated by "go-syncmap -output sync_map.gen.go -type Map"; DO NOT EDIT. + +package endpointurls + +import ( + "net/url" + "sync" +) + +func _() { + // An "cannot convert Map literal (type Map) 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)(Map{}) +} + +var _nil_Map_string_value = func() (val string) { return }() + +func (m *Map) Store(key url.URL, value string) { + (*sync.Map)(m).Store(key, value) +} + +func (m *Map) LoadOrStore(key url.URL, value string) (string, bool) { + actual, loaded := (*sync.Map)(m).LoadOrStore(key, value) + if actual == nil { + return _nil_Map_string_value, loaded + } + return actual.(string), loaded +} + +func (m *Map) Load(key url.URL) (string, bool) { + value, ok := (*sync.Map)(m).Load(key) + if value == nil { + return _nil_Map_string_value, ok + } + return value.(string), ok +} + +func (m *Map) Delete(key url.URL) { + (*sync.Map)(m).Delete(key) +} + +func (m *Map) Range(f func(key url.URL, value string) bool) { + (*sync.Map)(m).Range(func(key, value interface{}) bool { + return f(key.(url.URL), value.(string)) + }) +} diff --git a/pkg/registry/common/endpointurls/sync_set.gen.go b/pkg/registry/common/endpointurls/sync_set.gen.go deleted file mode 100644 index feb7791e0..000000000 --- a/pkg/registry/common/endpointurls/sync_set.gen.go +++ /dev/null @@ -1,74 +0,0 @@ -// Code generated by "-output sync_set.gen.go -type Set -output sync_set.gen.go -type Set"; DO NOT EDIT. -package endpointurls - -import ( - "net/url" - "sync" // Used by sync.Map. -) - -// Generate code that will fail if the constants change value. -func _() { - // An "cannot convert Set literal (type Set) 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)(Set{}) -} - -var _nil_Set_struct___value = func() (val struct{}) { 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 *Set) Load(key url.URL) (struct{}, bool) { - value, ok := (*sync.Map)(m).Load(key) - if value == nil { - return _nil_Set_struct___value, ok - } - return value.(struct{}), ok -} - -// Store sets the value for a key. -func (m *Set) Store(key url.URL, value struct{}) { - (*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 *Set) LoadOrStore(key url.URL, value struct{}) (struct{}, bool) { - actual, loaded := (*sync.Map)(m).LoadOrStore(key, value) - if actual == nil { - return _nil_Set_struct___value, loaded - } - return actual.(struct{}), 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 *Set) LoadAndDelete(key url.URL) (value struct{}, loaded bool) { - actual, loaded := (*sync.Map)(m).LoadAndDelete(key) - if actual == nil { - return _nil_Set_struct___value, loaded - } - return actual.(struct{}), loaded -} - -// Delete deletes the value for a key. -func (m *Set) Delete(key url.URL) { - (*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 *Set) Range(f func(key url.URL, value struct{}) bool) { - (*sync.Map)(m).Range(func(key, value interface{}) bool { - return f(key.(url.URL), value.(struct{})) - }) -} diff --git a/pkg/registry/common/interpose/server.go b/pkg/registry/common/interpose/server.go index aebdc377b..98a6a5298 100644 --- a/pkg/registry/common/interpose/server.go +++ b/pkg/registry/common/interpose/server.go @@ -42,8 +42,13 @@ type interposeRegistry struct { endpoints *stringurl.Map } +// Is returns true if passed name contains interpose identity +func Is(name string) bool { + return strings.HasPrefix(name, interposeNSEName) +} + func (l *interposeRegistry) Register(ctx context.Context, request *registry.NetworkServiceEndpoint) (*registry.NetworkServiceEndpoint, error) { - if strings.HasPrefix(request.Name, interposeNSEName) { + if Is(request.Name) { endpointURL, err := url.Parse(request.Url) if err != nil { return nil, err