timeout: take the minimum expiration time of pathSegments #1439
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1439 +/- ##
=======================================
Coverage ? 70.28%
=======================================
Files ? 242
Lines ? 11006
Branches ? 0
=======================================
Hits ? 7736
Misses ? 2770
Partials ? 500 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think these changes make sense, but it is not required. It's about optimization.
@edwarnicke What do you think?
| func after(ctx context.Context, conn *networkservice.Connection) time.Duration { | ||
| clockTime := clock.FromContext(ctx) | ||
|
|
||
| var minTimeout *time.Duration |
There was a problem hiding this comment.
| var minTimeout *time.Duration | |
| var minTimeout time.Duration = 1 |
There was a problem hiding this comment.
we can't set 1 here because we won't find a value less than that in Path segments
| if minTimeout == nil { | ||
| minTimeout = new(time.Duration) | ||
| } |
There was a problem hiding this comment.
| if minTimeout == nil { | |
| minTimeout = new(time.Duration) | |
| } |
| minTimeout = new(time.Duration) | ||
| } | ||
|
|
||
| *minTimeout = timeout |
There was a problem hiding this comment.
| *minTimeout = timeout | |
| minTimeout = timeout |
| if minTimeout == nil || *minTimeout <= 0 { | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
| if minTimeout == nil || *minTimeout <= 0 { | |
| return 1 | |
| } |
| } | ||
| log.FromContext(ctx).Infof("expiration after %s at %s", minTimeout.String(), expireTime.UTC()) | ||
|
|
||
| return *minTimeout |
There was a problem hiding this comment.
| return *minTimeout | |
| return minTimeout |
| @@ -97,3 +92,34 @@ func (s *timeoutServer) Close(ctx context.Context, conn *networkservice.Connecti | |||
| } | |||
| return &empty.Empty{}, err | |||
| } | |||
|
|
|||
| func after(ctx context.Context, conn *networkservice.Connection) time.Duration { | |||
There was a problem hiding this comment.
I think we need to rename it.
For example,
| func after(ctx context.Context, conn *networkservice.Connection) time.Duration { | |
| func minTokenTimeout(ctx context.Context, conn *networkservice.Connection) time.Duration { |
Signed-off-by: Artem Glazychev <[email protected]>
Signed-off-by: Artem Glazychev <[email protected]>
Description
The previous implementation took into account only the previous segment. This is mistake.
For example:
NSC (1min) ---> NSMgr (5min) ---> Fwd (5min) --->...
If NSC and NSMgr die, then the Fwd timeout will work only after 5 minutes. But
authorizepolicies look at the whole path, so an error will occur when closing (the client token has already expired).Issue link
networkservicemesh/deployments-k8s#8882
How Has This Been Tested?
Types of changes