Fix setid #662

Bolodya1997 · 2021-01-22T04:00:45Z

Issue

setid registry chain element doesn't generate new name if NSE already has name.

Solution

Create unique names for all newly registrated endpoints.

denis-tingaikin

Please cover this by sandbox tests to make sure that it is working as expected with other applications.

pkg/registry/common/setid/gen.go

Bolodya1997 · 2021-01-25T17:17:33Z

Needs updated sandbox by #668.

Bolodya1997 · 2021-02-09T04:05:12Z

Please cover this by sandbox tests to make sure that it is working as expected with other applications.

Done.

denis-tingaikin · 2021-02-10T09:03:24Z

pkg/registry/common/setid/server.go

+}
+
+func (s *setIDServer) Register(ctx context.Context, nse *registry.NetworkServiceEndpoint) (*registry.NetworkServiceEndpoint, error) {
+	name, suffix := interDomainName(nse.Name)


Why setid knows about interdomain?

Because if we want to register NSE in some other domain, we shouldn't override domain suffix:

sdk/pkg/registry/core/interdomain/interdomain_nse_test.go

Lines 239 to 246 in 536f38f

_, err := domain2.Registry.NetworkServiceEndpointRegistryServer().Register(

context.Background(),

&registryapi.NetworkServiceEndpoint{

Name: "nse-1@" + floatingRegistryDomain,

Url: "test://publicNSMGRurl",

ExpirationTime: expirationTime,

},

)

Yes, but this chain element shouldn't know about the interdomain.

Fixed with id prefix.

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

…ases Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

…k@master networkservicemesh/sdk#662 networkservicemesh/sdk PR link: networkservicemesh/sdk#662 networkservicemesh/sdk commit message: commit 146ee8bc456c7ae7a4d6c421445811d5f7d03d98 Author: Vladimir Popov <vladimir.popov@xored.com> Date: Wed Feb 10 20:01:49 2021 +0700 Fix setid (#662) * Rework registry setid Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Rework passThroughClient in nsmgr_test Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Rework registry setid to work in interdomain and in remote registry cases Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Fix header Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Add sandbox test Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Remove non-need setid from discover test Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Remove interpose NSE prefix to suffix Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> * Rework setid to use in NSMgr chain Signed-off-by: Vladimir Popov <vladimir.popov@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

Bolodya1997 marked this pull request as draft January 22, 2021 04:06

Bolodya1997 force-pushed the fix-setid branch from d70d5db to 0b0550e Compare January 22, 2021 05:25

Bolodya1997 marked this pull request as ready for review January 22, 2021 05:25

Bolodya1997 force-pushed the fix-setid branch 2 times, most recently from 90c5064 to a7f0ab4 Compare January 22, 2021 05:31

denis-tingaikin requested changes Jan 22, 2021

View reviewed changes

haiodo reviewed Jan 22, 2021

View reviewed changes

pkg/registry/common/setid/gen.go Outdated Show resolved Hide resolved

Bolodya1997 force-pushed the fix-setid branch from 511b5f0 to dbc97e9 Compare January 26, 2021 15:19

Bolodya1997 marked this pull request as draft February 1, 2021 10:04

Bolodya1997 force-pushed the fix-setid branch 2 times, most recently from 1415d4a to 0b7a7cb Compare February 8, 2021 09:47

Bolodya1997 marked this pull request as ready for review February 9, 2021 04:05

denis-tingaikin reviewed Feb 10, 2021

View reviewed changes

Bolodya1997 marked this pull request as draft February 10, 2021 09:55

Vladimir Popov added 8 commits February 10, 2021 17:36

Rework registry setid

b66f2ca

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Rework passThroughClient in nsmgr_test

fb77d30

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Rework registry setid to work in interdomain and in remote registry c…

128b290

…ases Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Fix header

e794abd

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Add sandbox test

eccb319

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Remove non-need setid from discover test

d8c9132

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Remove interpose NSE prefix to suffix

9b293dd

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Rework setid to use in NSMgr chain

63d1537

Signed-off-by: Vladimir Popov <vladimir.popov@xored.com>

Bolodya1997 force-pushed the fix-setid branch from 463c1a9 to 63d1537 Compare February 10, 2021 11:09

Bolodya1997 marked this pull request as ready for review February 10, 2021 11:10

denis-tingaikin approved these changes Feb 10, 2021

View reviewed changes

denis-tingaikin merged commit 146ee8b into networkservicemesh:master Feb 10, 2021

Bolodya1997 deleted the fix-setid branch February 12, 2021 05:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setid #662

Fix setid #662

Bolodya1997 commented Jan 22, 2021

denis-tingaikin left a comment

Bolodya1997 commented Jan 25, 2021

Bolodya1997 commented Feb 9, 2021

denis-tingaikin Feb 10, 2021

Bolodya1997 Feb 10, 2021

denis-tingaikin Feb 10, 2021

Bolodya1997 Feb 10, 2021

	_, err := domain2.Registry.NetworkServiceEndpointRegistryServer().Register(
	context.Background(),
	&registryapi.NetworkServiceEndpoint{
	Name: "nse-1@" + floatingRegistryDomain,
	Url: "test://publicNSMGRurl",
	ExpirationTime: expirationTime,
	},
	)

Fix setid #662

Fix setid #662

Conversation

Bolodya1997 commented Jan 22, 2021

Issue

Solution

denis-tingaikin left a comment

Choose a reason for hiding this comment

Bolodya1997 commented Jan 25, 2021

Bolodya1997 commented Feb 9, 2021

denis-tingaikin Feb 10, 2021

Choose a reason for hiding this comment

Bolodya1997 Feb 10, 2021

Choose a reason for hiding this comment

denis-tingaikin Feb 10, 2021

Choose a reason for hiding this comment

Bolodya1997 Feb 10, 2021

Choose a reason for hiding this comment