Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified refresh client to calculate the shortest token expiry #1190

Merged
merged 3 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions pkg/networkservice/common/refresh/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package refresh

import (
"context"
"math"
"time"

"github.com/golang/protobuf/ptypes"
Expand Down Expand Up @@ -108,16 +109,23 @@ func (t *refreshClient) Close(ctx context.Context, conn *networkservice.Connecti
func after(ctx context.Context, conn *networkservice.Connection) (time.Duration, error) {
clockTime := clock.FromContext(ctx)

expireTime, err := ptypes.Timestamp(conn.GetCurrentPathSegment().GetExpires())
if err != nil {
return 0, errors.WithStack(err)
}
var minTimeout = time.Duration(math.MaxInt64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sol-0 sol-0 Dec 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the case when there no segments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

expireTime, err := ptypes.Timestamp(seg.GetExpires())
if err != nil {
return 0, errors.WithStack(err)
}

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())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


if timeout <= 0 {
return 1, nil
if timeout <= 0 {
return 1, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if timeout <= 0 {
return 1, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


if timeout < minTimeout {
minTimeout = timeout
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
}
}
log.FromContext(ctx).Infof("expiration after %s at %s", timeout.String(), expireTime.UTC())
if timeout <= 0 {
return 1, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


// A heuristic to reduce the number of redundant requests in a chain
Expand All @@ -129,7 +137,7 @@ func after(ctx context.Context, conn *networkservice.Connection) (time.Duration,
if len(path.PathSegments) > 1 {
scale = 0.2 + 0.2*float64(path.Index)/float64(len(path.PathSegments))
}
duration := time.Duration(float64(timeout) * scale)
duration := time.Duration(float64(minTimeout) * scale)

return duration, nil
}
105 changes: 102 additions & 3 deletions pkg/networkservice/common/refresh/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,25 @@ package refresh_test

import (
"context"
"fmt"
"sync/atomic"
"testing"
"time"

"github.com/networkservicemesh/api/pkg/api/networkservice"
"github.com/networkservicemesh/api/pkg/api/registry"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"google.golang.org/grpc/credentials"

"github.com/networkservicemesh/api/pkg/api/networkservice"
"github.com/networkservicemesh/api/pkg/api/registry"

"github.com/networkservicemesh/sdk/pkg/networkservice/common/begin"
"github.com/networkservicemesh/sdk/pkg/networkservice/common/refresh"
"github.com/networkservicemesh/sdk/pkg/networkservice/common/updatepath"
"github.com/networkservicemesh/sdk/pkg/networkservice/common/updatetoken"
"github.com/networkservicemesh/sdk/pkg/networkservice/core/adapters"
"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
countutil "github.com/networkservicemesh/sdk/pkg/networkservice/utils/count"
"github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injectclock"
"github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror"
"github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata"
Expand All @@ -62,6 +63,28 @@ func testTokenFunc(clockTime clock.Clock) token.GeneratorFunc {
}
}

func testTokenFuncWithTimeout(clockTime clock.Clock, timeout time.Duration) token.GeneratorFunc {
return func(_ credentials.AuthInfo) (token string, expireTime time.Time, err error) {
return "", clockTime.Now().Add(timeout), err
}
}

type captureTickerDuration struct {
*clockmock.Mock

tickerDuration time.Duration
}

func (m *captureTickerDuration) Ticker(d time.Duration) clock.Ticker {
m.tickerDuration = d
return m.Mock.Ticker(d)
}

func (m *captureTickerDuration) Reset(t time.Time) {
m.tickerDuration = 0
m.Set(t)
}

func testClient(
ctx context.Context,
tokenGenerator token.GeneratorFunc,
Expand Down Expand Up @@ -302,6 +325,82 @@ func TestRefreshClient_NoRefreshOnFailure(t *testing.T) {
require.Never(t, cloneClient.validator(2), testWait, testTick)
}

func TestRefreshClient_CalculatesShortestTokenTimeout(t *testing.T) {
t.Cleanup(func() { goleak.VerifyNone(t) })

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

testData := []struct {
Chain []time.Duration
ExpectedRefreshTimeoutMax time.Duration
ExpectedRefreshTimeoutMin time.Duration
}{
{
Chain: []time.Duration{time.Hour},
ExpectedRefreshTimeoutMax: 20*time.Minute + time.Second,
ExpectedRefreshTimeoutMin: 20*time.Minute - time.Second,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use const time range.

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

Copy link
Member

@denis-tingaikin denis-tingaikin Dec 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply this to other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

},
{
Chain: []time.Duration{time.Hour, 3 * time.Minute},
ExpectedRefreshTimeoutMax: 54*time.Second + time.Second/2.,
ExpectedRefreshTimeoutMin: 54*time.Second - time.Second/2.,
},
{
Chain: []time.Duration{time.Hour, 5 * time.Second, 3 * time.Minute},
ExpectedRefreshTimeoutMax: 5*time.Second/3. + time.Second/3.,
ExpectedRefreshTimeoutMin: 5*time.Second/3. - time.Second/3.,
},
{
Chain: []time.Duration{200 * time.Millisecond, 1 * time.Minute, 100 * time.Millisecond, time.Hour},
ExpectedRefreshTimeoutMax: 100*time.Millisecond/3. + 30*time.Millisecond,
ExpectedRefreshTimeoutMin: 100*time.Millisecond/3. - 30*time.Millisecond,
},
}

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

clockMock := captureTickerDuration{
Mock: clockmock.New(ctx),
}

var countClient = &countutil.Client{}

for _, testDataElement := range testData {
clockMock.Reset(timeNow)

var pathChain []networkservice.NetworkServiceClient
var clientChain = []networkservice.NetworkServiceClient{
begin.NewClient(),
metadata.NewClient(),
injectclock.NewClient(&clockMock),
refresh.NewClient(ctx),
}

for _, expireTimeValue := range testDataElement.Chain {
pathChain = append(pathChain,
updatepath.NewClient(fmt.Sprintf("test-%v", expireTimeValue)),
adapters.NewServerToClient(updatetoken.NewServer(testTokenFuncWithTimeout(&clockMock, expireTimeValue))),
)
}

clientChain = append(pathChain, clientChain...)
clientChain = append(clientChain, countClient)
client := chain.NewNetworkServiceClient(clientChain...)

_, err = client.Request(ctx, &networkservice.NetworkServiceRequest{
Connection: new(networkservice.Connection),
})
require.NoError(t, err)

require.Less(t, clockMock.tickerDuration, testDataElement.ExpectedRefreshTimeoutMax)
require.Greater(t, clockMock.tickerDuration, testDataElement.ExpectedRefreshTimeoutMin)
}

require.Equal(t, countClient.Requests(), len(testData))
}

func TestRefreshClient_RefreshOnRefreshFailure(t *testing.T) {
t.Cleanup(func() { goleak.VerifyNone(t) })

Expand Down
3 changes: 3 additions & 0 deletions pkg/networkservice/common/timeout/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,9 @@ func TestTimeoutServer_RefreshFailure(t *testing.T) {
injecterror.WithRequestErrorTimes(1, -1),
injecterror.WithCloseErrorTimes(),
),
updatetoken.NewServer(func(_ credentials.AuthInfo) (string, time.Time, error) {
return "test", clock.FromContext(ctx).Now().Add(tokenTimeout), nil
}),
connServer,
),
tokenTimeout,
Expand Down