Assist: mark individual nodes as finished#28477
Merged
Conversation
4 tasks
added 5 commits
June 30, 2023 15:04
`Envelope` is the outer layer used (in protobuf format) for execution / terminal sessions. Meanwhile `outEnvelope` is the inner layer used (in JSON) spefically with assist (execution), when outer envelope is of "raw" type Using `Envelope` where decoding `outEnvelope` in tests previously worked "by accident" due to matching field names
justinas
changed the title
jakule
reviewed
Jun 30, 2023
gzdunek
approved these changes
Jul 3, 2023
This commit changes how command execution and WebSocket handling are performed in the code. Instead of manually managing session close signals and command execution notifications via WebSocket, we have wrapped it into more easily manageable form. Changed parts: assist and websocket libraries, test and several components of 'teleport'. Why: This allows cleaner command management and error handling, leading to more reliable and maintainable application. Also made code more readable and easy to understand. Made WebSocket more precise with its handling to prevent cases where stale or incorrect data might interrupt the session. Details: Instead of sending a close signal after all commands have been executed, we now send individual session end updates for each command. Sessions no longer remain active unnecessarily. Message handling in command execution has been refactored for better error propagation. All these changes were also adjusted and reflected in the associated test cases. Also, names and types of various data structures are edited to reflect their actual usage.
Contributor
|
@ryanclark Can you take a look at this? |
This commit replaces occurrences of `ServerID` with `NodeID` across several files. `ServerID` was misleading and causing confusion as it was serving as an identifier for nodes, so to improve comprehension and consistency in the codebase, all instances of `ServerID` have been replaced with `NodeID`. Tests have also been updated to reflect this change.
ravicious
pushed a commit
that referenced
this pull request
Jul 11, 2023
* Mark nodes as done when command finishes in Assist * Split Close/CloseWithPayload * Expect a close message before the summary in test * Remediate confusing type usage in test `Envelope` is the outer layer used (in protobuf format) for execution / terminal sessions. Meanwhile `outEnvelope` is the inner layer used (in JSON) spefically with assist (execution), when outer envelope is of "raw" type Using `Envelope` where decoding `outEnvelope` in tests previously worked "by accident" due to matching field names * Assert that server ID is set in the close message * Refactor command execution logic and adjust WebSocket handling This commit changes how command execution and WebSocket handling are performed in the code. Instead of manually managing session close signals and command execution notifications via WebSocket, we have wrapped it into more easily manageable form. Changed parts: assist and websocket libraries, test and several components of 'teleport'. Why: This allows cleaner command management and error handling, leading to more reliable and maintainable application. Also made code more readable and easy to understand. Made WebSocket more precise with its handling to prevent cases where stale or incorrect data might interrupt the session. Details: Instead of sending a close signal after all commands have been executed, we now send individual session end updates for each command. Sessions no longer remain active unnecessarily. Message handling in command execution has been refactored for better error propagation. All these changes were also adjusted and reflected in the associated test cases. Also, names and types of various data structures are edited to reflect their actual usage. * Change ServerID to NodeID to improve code consistency This commit replaces occurrences of `ServerID` with `NodeID` across several files. `ServerID` was misleading and causing confusion as it was serving as an identifier for nodes, so to improve comprehension and consistency in the codebase, all instances of `ServerID` have been replaced with `NodeID`. Tests have also been updated to reflect this change. --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com>
jakule
added a commit
that referenced
this pull request
Jul 14, 2023
* Mark nodes as done when command finishes in Assist * Split Close/CloseWithPayload * Expect a close message before the summary in test * Remediate confusing type usage in test `Envelope` is the outer layer used (in protobuf format) for execution / terminal sessions. Meanwhile `outEnvelope` is the inner layer used (in JSON) spefically with assist (execution), when outer envelope is of "raw" type Using `Envelope` where decoding `outEnvelope` in tests previously worked "by accident" due to matching field names * Assert that server ID is set in the close message * Refactor command execution logic and adjust WebSocket handling This commit changes how command execution and WebSocket handling are performed in the code. Instead of manually managing session close signals and command execution notifications via WebSocket, we have wrapped it into more easily manageable form. Changed parts: assist and websocket libraries, test and several components of 'teleport'. Why: This allows cleaner command management and error handling, leading to more reliable and maintainable application. Also made code more readable and easy to understand. Made WebSocket more precise with its handling to prevent cases where stale or incorrect data might interrupt the session. Details: Instead of sending a close signal after all commands have been executed, we now send individual session end updates for each command. Sessions no longer remain active unnecessarily. Message handling in command execution has been refactored for better error propagation. All these changes were also adjusted and reflected in the associated test cases. Also, names and types of various data structures are edited to reflect their actual usage. * Change ServerID to NodeID to improve code consistency This commit replaces occurrences of `ServerID` with `NodeID` across several files. `ServerID` was misleading and causing confusion as it was serving as an identifier for nodes, so to improve comprehension and consistency in the codebase, all instances of `ServerID` have been replaced with `NodeID`. Tests have also been updated to reflect this change. --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com>
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Jul 14, 2023
* [Assist] Clear the refresh websocket timeout when closing Assist (#28610) * Clear the refresh websocket timeout when closing Assist * Missing semicolon to please prettier * Add comment to remove once the new session implementation is done * fix: Decode JSON directly from stream in `waitForCommandOutput` (#28040) * Assist: mark individual nodes as finished (#28477) * Mark nodes as done when command finishes in Assist * Split Close/CloseWithPayload * Expect a close message before the summary in test * Remediate confusing type usage in test `Envelope` is the outer layer used (in protobuf format) for execution / terminal sessions. Meanwhile `outEnvelope` is the inner layer used (in JSON) spefically with assist (execution), when outer envelope is of "raw" type Using `Envelope` where decoding `outEnvelope` in tests previously worked "by accident" due to matching field names * Assert that server ID is set in the close message * Refactor command execution logic and adjust WebSocket handling This commit changes how command execution and WebSocket handling are performed in the code. Instead of manually managing session close signals and command execution notifications via WebSocket, we have wrapped it into more easily manageable form. Changed parts: assist and websocket libraries, test and several components of 'teleport'. Why: This allows cleaner command management and error handling, leading to more reliable and maintainable application. Also made code more readable and easy to understand. Made WebSocket more precise with its handling to prevent cases where stale or incorrect data might interrupt the session. Details: Instead of sending a close signal after all commands have been executed, we now send individual session end updates for each command. Sessions no longer remain active unnecessarily. Message handling in command execution has been refactored for better error propagation. All these changes were also adjusted and reflected in the associated test cases. Also, names and types of various data structures are edited to reflect their actual usage. * Change ServerID to NodeID to improve code consistency This commit replaces occurrences of `ServerID` with `NodeID` across several files. `ServerID` was misleading and causing confusion as it was serving as an identifier for nodes, so to improve comprehension and consistency in the codebase, all instances of `ServerID` have been replaced with `NodeID`. Tests have also been updated to reflect this change. --------- Co-authored-by: Jakub Nyckowski <jakub.nyckowski@goteleport.com> --------- Co-authored-by: Ryan Clark <ryan.clark@goteleport.com> Co-authored-by: Alan Parra <alan.parra@goteleport.com> Co-authored-by: Justinas Stankevičius <justinas@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes https://github.com/gravitational/teleport.e/issues/1741 .
Makes sure that server ID is sent in the individual close messages. We do this in a quite naive way by sending the same
sessionDatathat is sent when the session is established. This means that we possibly have some stale data (such aslast_active), but for us onlyserver_idis important. We could fix this by sending a leaner custom structure in this case.