-
Notifications
You must be signed in to change notification settings - Fork 3
incorporate session id into renewal #289
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
Conversation
depends on signadot/signadot#6457 and signadot/go-sdk#76 previously, the api re-claimed on renewal, creating a corner case hole where more than 1 local connect could be running at the same time. Please review signadot/signadot#6457 for details and explanation of the flow and what has been tested. Side Effects This change removed the need for dynamic session id updates. Misc Fixes The check for --cluster was pushed to earlier to avoid the possibility of registering and claiming a devbox and then failing due to a mis-specified cluster.
| } | ||
| } | ||
|
|
||
| func (sbw *sbmWatcher) readStream(ctx context.Context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should simplify this func, and go back to the original implementation:
cli/internal/locald/sandboxmanager/sandboxes_watcher.go
Lines 86 to 125 in e36e5e6
| func (sbw *sbmWatcher) readStream(ctx context.Context, | |
| sbwClient tunapiv1.TunnelAPI_WatchLocalSandboxesClient) { | |
| for { | |
| event, err := sbwClient.Recv() | |
| if err == nil { | |
| sbw.setSuccess(ctx) | |
| sbw.processStreamEvent(event) | |
| continue | |
| } | |
| // just return if the context has been cancelled | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| default: | |
| } | |
| // extract the grpc status | |
| grpcStatus, ok := status.FromError(err) | |
| if !ok { | |
| sbw.setError("sandboxes watch grpc stream error: no status", err) | |
| break | |
| } | |
| switch grpcStatus.Code() { | |
| case codes.OK: | |
| sbw.log.Debug("sandboxes watch error code is ok") | |
| sbw.processStreamEvent(event) | |
| continue | |
| case codes.Internal: | |
| sbw.setError("sandboxes watch internal grpc error", err) | |
| <-time.After(3 * time.Second) | |
| case codes.Unimplemented: | |
| sbw.setError(SandboxesWatcherUnimplemented, nil) | |
| // in this case, check again in 1 minutes | |
| <-time.After(1 * time.Minute) | |
| default: | |
| sbw.setError("sandbox watch error", err) | |
| <-time.After(3 * time.Second) | |
| } | |
| break | |
| } | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This func is unused.
daniel-de-vera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left a few comments.
depends on https://github.com/signadot/signadot/pull/6457 and signadot/go-sdk#76
previously, the api re-claimed on renewal, creating a corner case hole where more than 1 local connect could be running at the same time.
Please review https://github.com/signadot/signadot/pull/6457 for details and explanation of the flow and what has been tested.
Side Effects
This change removed the need for dynamic session id updates.
Misc Fixes
The check for --cluster was pushed to earlier to avoid the possibility of registering and claiming a devbox and then failing due to a mis-specified cluster.