Modified refresh client to calculate the shortest token expiry #1190

sol-0 · 2021-12-02T17:11:52Z

Signed-off-by: Oleg Solodkov oleg.solodkov@xored.com

Description

Refresh client calculates the shortest token expiry and sets refresh time according to that value

Issue link

Fixes #1189

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: Oleg Solodkov <oleg.solodkov@xored.com>

denis-tingaikin · 2021-12-05T14:37:48Z

pkg/networkservice/common/refresh/client.go

-	if err != nil {
-		return 0, errors.WithStack(err)
-	}
+	var minTimeout = time.Duration(math.MaxInt64)


Suggested change

var minTimeout = time.Duration(math.MaxInt64)

var minTimeout time.Duration

If we don't initialize minTimeout, it will always be 0. It's initialized with max possible duration so that it's always greater than any value.

Please fix the case when there no segments.

denis-tingaikin · 2021-12-05T14:38:36Z

pkg/networkservice/common/refresh/client.go

-	timeout := clockTime.Until(expireTime)
-	log.FromContext(ctx).Infof("expiration after %s at %s", timeout.String(), expireTime.UTC())
+		timeout := clockTime.Until(expireTime)
+		log.FromContext(ctx).Infof("expiration after %s at %s", timeout.String(), expireTime.UTC())


Suggested change

log.FromContext(ctx).Infof("expiration after %s at %s", timeout.String(), expireTime.UTC())

denis-tingaikin · 2021-12-05T14:39:49Z

pkg/networkservice/common/refresh/client.go

+
+		if timeout < minTimeout {
+			minTimeout = timeout
+		}
 	}


Suggested change

}

}

log.FromContext(ctx).Infof("expiration after %s at %s", timeout.String(), expireTime.UTC())

if timeout <= 0 {

return 1, nil

}

denis-tingaikin · 2021-12-05T14:40:00Z

pkg/networkservice/common/refresh/client.go

+		if timeout <= 0 {
+			return 1, nil
+		}


Suggested change

if timeout <= 0 {

return 1, nil

}

denis-tingaikin · 2021-12-05T14:41:49Z

pkg/networkservice/common/refresh/client.go

-		return 0, errors.WithStack(err)
-	}
+	var minTimeout = time.Duration(math.MaxInt64)
+	for _, seg := range conn.GetPath().GetPathSegments() {


Suggested change

for _, seg := range conn.GetPath().GetPathSegments() {

for _, segment := range conn.GetPath().GetPathSegments() {

denis-tingaikin · 2021-12-05T14:43:15Z

pkg/networkservice/common/refresh/client_test.go

+			ExpectedRefreshTimeoutMax: 20*time.Minute + time.Second,
+			ExpectedRefreshTimeoutMin: 20*time.Minute - time.Second,


Please use const time range.

Suggested change

ExpectedRefreshTimeoutMax: 20*time.Minute + time.Second,

ExpectedRefreshTimeoutMin: 20*time.Minute - time.Second,

ExpectedRefreshTimeout: 20*time.Minute

Apply this to other places.

denis-tingaikin · 2021-12-05T14:48:42Z

pkg/networkservice/common/refresh/client_test.go

+	timeNow, err := time.Parse("2006-01-02 15:04:05", "2009-11-10 23:00:00")
+	require.NoError(t, err)
+


time.Date() is not returning error.

Suggested change

timeNow, err := time.Parse("2006-01-02 15:04:05", "2009-11-10 23:00:00")

require.NoError(t, err)

timeNow := time.Date(/*todo*/)

denis-tingaikin · 2021-12-05T14:49:47Z

@edwarnicke We are thinking to solve #1189 with this PR.
Please have a look and share your thoughts.

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

denis-tingaikin

LGTM

@edwarnicke Could you have a look?

…k@main PR link: networkservicemesh/sdk#1190 Commit: 8e96470 Author: Sol-0 Date: 2021-12-10 00:07:40 +0700 Message: - Modified refresh client to calculate the shortest token expiry (#1190) * Modified refresh client to calculate the shortest token expiry Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com> * Addressed review comments Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>

sol-0 force-pushed the bugfix-1189 branch 2 times, most recently from a8be34c to 40a5de6 Compare December 3, 2021 07:42

Modified refresh client to calculate the shortest token expiry

8fb159f

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

sol-0 force-pushed the bugfix-1189 branch from 40a5de6 to 8fb159f Compare December 3, 2021 07:44

sol-0 marked this pull request as ready for review December 3, 2021 07:51

sol-0 self-assigned this Dec 3, 2021

Mixaster995 self-assigned this Dec 3, 2021

denis-tingaikin requested changes Dec 5, 2021

View reviewed changes

denis-tingaikin reviewed Dec 5, 2021

View reviewed changes

Mixaster995 approved these changes Dec 6, 2021

View reviewed changes

sol-0 added 2 commits December 6, 2021 16:20

Addressed review comments

2a51462

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

Addressed review comments

cce5cfd

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

sol-0 force-pushed the bugfix-1189 branch from 58ba1da to cce5cfd Compare December 7, 2021 13:58

denis-tingaikin approved these changes Dec 7, 2021

View reviewed changes

edwarnicke merged commit 8e96470 into networkservicemesh:main Dec 9, 2021

nsmbot mentioned this pull request Dec 9, 2021

Update from update/networkservicemesh/sdk networkservicemesh/sdk-kernel#396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified refresh client to calculate the shortest token expiry #1190

Modified refresh client to calculate the shortest token expiry #1190

sol-0 commented Dec 2, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 6, 2021 •

edited

Loading

denis-tingaikin Dec 6, 2021

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

denis-tingaikin Dec 5, 2021 •

edited

Loading

sol-0 Dec 7, 2021

denis-tingaikin Dec 5, 2021

sol-0 Dec 7, 2021

denis-tingaikin commented Dec 5, 2021

denis-tingaikin left a comment

	var minTimeout = time.Duration(math.MaxInt64)
	var minTimeout time.Duration

	for _, seg := range conn.GetPath().GetPathSegments() {
	for _, segment := range conn.GetPath().GetPathSegments() {

		ExpectedRefreshTimeoutMax: 20*time.Minute + time.Second,
		ExpectedRefreshTimeoutMin: 20*time.Minute - time.Second,

	ExpectedRefreshTimeoutMax: 20*time.Minute + time.Second,
	ExpectedRefreshTimeoutMin: 20*time.Minute - time.Second,
	ExpectedRefreshTimeout: 20*time.Minute

		timeNow, err := time.Parse("2006-01-02 15:04:05", "2009-11-10 23:00:00")
		require.NoError(t, err)

Modified refresh client to calculate the shortest token expiry #1190

Modified refresh client to calculate the shortest token expiry #1190

Conversation

sol-0 commented Dec 2, 2021

Description

Issue link

How Has This Been Tested?

Types of changes

Choose a reason for hiding this comment

sol-0 Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denis-tingaikin commented Dec 5, 2021

denis-tingaikin left a comment

Choose a reason for hiding this comment

sol-0 Dec 6, 2021 •

edited

Loading

denis-tingaikin Dec 5, 2021 •

edited

Loading