Fix timeout/expire chain elements #1404

glazychev-art · 2022-12-23T12:18:47Z

Signed-off-by: Artem Glazychev artem.glazychev@xored.com

Description

Main changes:

networkservice timeout: at the time of the call Close() on timeout, the token may already be expired. Therefore, we need to call Close() in advance - (tokenTimeout - requestTimeout)
registry expire: since we now have tokens in the registry, we must follow the same logic as in the networkservice (see above). Expiration time selection has been extended. Now it is - min(peertoken.ExpirationTime, token.ExpirationTime, nse.ExpirationTIme) - registerTimeout.
grpcmetadata.NewNetworkServiceEndpointRegistryClient - grpc.Header was added to Unregister() to process an updated path on the way back (like here - https://github.com/networkservicemesh/sdk/blob/main/pkg/registry/common/grpcmetadata/nse_client.go#L47-L48)
registry authorize and updatepath were placed before begin, because in case of an event that triggers an eventFactory, we don't need to re-authorize.
registry begin - when updating the context, we need to keep a copy of path, since we access it by reference (we should not be able to accidentally change it after begin)

Issue link

#1398

How Has This Been Tested?

Added unit testing to cover
Tested manually
Tested by integration testing
Have not tested

Types of changes

Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>

denis-tingaikin · 2022-12-28T01:02:46Z

pkg/networkservice/chains/nsmgr/heal_test.go

-	if withNSEExpiration {
-		builder = builder.SetRegistryExpiryDuration(time.Second / 2)
-	}
-


Why do we change this?

We now rely mostly on the token rather than registryExpirationDuration. Therefore, below we create a token with the expiration we need.

denis-tingaikin · 2022-12-28T01:13:34Z

pkg/registry/common/expire/nse_server.go

@@ -32,35 +32,38 @@ import (
 )

 type expireNSEServer struct {
-	nseExpiration time.Duration
-	ctx           context.Context
+	defaultNseExpiration time.Duration


Should we get rid of default expiration? Seems like the update path is going to always put the expiration time for nses after these changes.

Suggested change

defaultNseExpiration time.Duration

For unit tests, we can add injectexpirationtime chain element.

Totally agree
But should we consider the situation where the user hasn't updatepath?

Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>

…k@main PR link: networkservicemesh/sdk#1404 Commit: d41eaac Author: Artem Glazychev Date: 2022-12-29 08:57:32 +0700 Message: - Fix timeout/expire chain elements (#1404) * Fix timeout/expire chain elements Signed-off-by: Artem Glazychev <artem.glazychev@xored.com> * Remove default expiration for registry Signed-off-by: Artem Glazychev <artem.glazychev@xored.com> Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

With networkservicemesh/sdk#1404 NSE expiration time is controlled by NSM's MaxTokenLifetime parameter (defaults to 10 minutes). Introduce a custom chain function to ensure LB/Proxy NSEs are registered using 1 minute lifetime like before.

glazychev-art force-pushed the expiration branch 10 times, most recently from aed2dbe to dc57328 Compare December 26, 2022 09:28

glazychev-art marked this pull request as ready for review December 26, 2022 09:55

glazychev-art marked this pull request as draft December 26, 2022 13:03

glazychev-art marked this pull request as ready for review December 26, 2022 13:19

Fix timeout/expire chain elements

e94ae98

Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>

glazychev-art marked this pull request as draft December 27, 2022 13:22

glazychev-art force-pushed the expiration branch from dc57328 to e94ae98 Compare December 27, 2022 13:25

glazychev-art marked this pull request as ready for review December 27, 2022 13:32

denis-tingaikin reviewed Dec 28, 2022

View reviewed changes

glazychev-art force-pushed the expiration branch from 2d231f6 to 3d91c09 Compare December 28, 2022 05:57

Remove default expiration for registry

430d162

Signed-off-by: Artem Glazychev <artem.glazychev@xored.com>

glazychev-art force-pushed the expiration branch from 3d91c09 to 430d162 Compare December 28, 2022 06:05

denis-tingaikin approved these changes Dec 29, 2022

View reviewed changes

denis-tingaikin merged commit d41eaac into networkservicemesh:main Dec 29, 2022

denis-tingaikin mentioned this pull request Jan 17, 2023

Authorize on timeout #1398

Closed

zolug mentioned this pull request Jul 10, 2023

Increased NSE expiration time might cause traffic disturbance Nordix/Meridio#438

Closed

zolug mentioned this pull request Jul 10, 2023

Retain legacy calculation of NSE expiration time Nordix/Meridio#439

Merged

10 tasks

ljkiraly mentioned this pull request Sep 19, 2023

missing interface in NSE after relocation networkservicemesh/deployments-k8s#9863

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix timeout/expire chain elements #1404

Fix timeout/expire chain elements #1404

glazychev-art commented Dec 23, 2022 •

edited

Loading

denis-tingaikin Dec 28, 2022

glazychev-art Dec 28, 2022

denis-tingaikin Dec 29, 2022

denis-tingaikin Dec 28, 2022

glazychev-art Dec 28, 2022

Fix timeout/expire chain elements #1404

Fix timeout/expire chain elements #1404

Conversation

glazychev-art commented Dec 23, 2022 • edited Loading

Description

Issue link

How Has This Been Tested?

Types of changes

denis-tingaikin Dec 28, 2022

Choose a reason for hiding this comment

glazychev-art Dec 28, 2022

Choose a reason for hiding this comment

denis-tingaikin Dec 29, 2022

Choose a reason for hiding this comment

denis-tingaikin Dec 28, 2022

Choose a reason for hiding this comment

glazychev-art Dec 28, 2022

Choose a reason for hiding this comment

glazychev-art commented Dec 23, 2022 •

edited

Loading