fix(ec2): remove SSM session state w/ best effort approaches. #5616
Conversation
| @@ -100,3 +101,14 @@ export class Ec2InstanceNode extends AWSTreeNodeBase implements AWSResourceNode | |||
| await vscode.commands.executeCommand('aws.refreshAwsExplorerNode', this) | |||
| } | |||
| } | |||
|
|
|||
| export async function refreshExplorerNode(node?: Ec2Node) { | |||
There was a problem hiding this comment.
I think we have a generalized function somewhere like this already.
There was a problem hiding this comment.
Yea, this is just a wrapper around that function that makes sure the node exists. Avoids having the same logic in each command where node is optional. I changed the name to tryRefreshNode to make this clearer.
| const newConnectionManager = new Ec2ConnectionManager(selection.region) | ||
| connectionManagers.set(selection.region, newConnectionManager) |
There was a problem hiding this comment.
it's a bit subtle that this is actually adding to a global long-lived collection. This logic should live closer to that collection, possibly in a class or something.
Also need to log a warning if the number of connections exceeds a big number, until we have a better way to have visbility and validation of these long-lived collections (which can caused hidden bugs / perf issues).
| connectionManagers = new Map<string, Ec2ConnectionManager>() | ||
| }) | ||
|
|
||
| it('only creates new connection managers once for each region ', async function () { |
There was a problem hiding this comment.
this makes me wonder if there should be 1 manager for everything. having many managers seems like an unnecessary problem, and partly defeats the purpose of a "manager", which is to have one place where visibility/handling can happen.
There was a problem hiding this comment.
I agree, I think the word manager is being overused here. Manager also isn't very informative about what each component is responsible for. I am leaning towards renaming them to directly describe what they "do".
Ex. Ec2ConnectionManager --> Ec2Connecter, Ec2RemoteSessionManager --> Ec2SessionTracker. Does that seem like a good direction?
| import { Ec2RemoteSessionManager } from '../../../awsService/ec2/remoteSessionManager' | ||
| import { SsmClient } from '../../../shared/clients/ssmClient' | ||
|
|
||
| describe('Ec2RemoteSessionManager', async function () { |
There was a problem hiding this comment.
is there a session manager for codecatalyst dev envs? if so, it may be worth generalizing it. All of this new test code suggests that it's worth having 1 generalized concept instead of several specific ones.
There was a problem hiding this comment.
For CodeCatalyst, we start the connection within the connect script here
so we don't have any easy way of getting the resultingSessionId in memory (as far as I can tell). Also, I am unsure if the same problem of sessions not terminating still happens in CodeCatalyst due to some of these implementation differences.
|
|
||
| export async function tryRefreshNode(node?: Ec2Node) { | ||
| if (node) { | ||
| const n = node instanceof Ec2InstanceNode ? node.parent : node |
There was a problem hiding this comment.
This could be generalized. I think possibly was, on the https://github.com/aws/aws-toolkit-vscode-staging/tree/feature/lambda-get-started branch.
No action needed here, for now.
| import { SsmClient } from '../../shared/clients/ssmClient' | ||
| import { Disposable } from 'vscode' | ||
|
|
||
| export class Ec2SessionTracker extends Map<EC2.InstanceId, SSM.SessionId> implements Disposable { |
There was a problem hiding this comment.
I assume we'll generalize this later, e.g. for ECS sessions and possibly others in the future.
| @@ -3,7 +3,11 @@ | |||
| * SPDX-License-Identifier: Apache-2.0 | |||
| */ | |||
|
|
|||
| import { SSM } from 'aws-sdk' | |||
There was a problem hiding this comment.
this is the "v2" sdk. may want to double-check if we already have the v3 SDK for ssm-related stuff (just by searching the codebase). that will avoid needing to migrate this later.
There was a problem hiding this comment.
Unfortunately, @aws-sdk/client-ssm is not a dependency yet which I think is what we want based on (https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-client-ssm/Interface/StartSessionResponse/)
Problem
The remote connection through VSCode does not terminate the SSM session on close consistently.
Notes:
Solution
Leverage two best-effort approaches:
Implementation Details
Ec2RemoteEnvManagerto manage the remote environments. Behaves like a map of instance ids to sessions ids that most importantly maintains the invariant that any deleted item has its session terminated.packages/core/src/awsService/ec2/commands.tsandpackages/core/src/awsService/ec2/activation.tsto allow for state tracking inEC2ConnectionManager. This change also gives us an opportunity to improve the testing infrastructure for this code.License: I confirm that my contribution is made under the terms of the Apache 2.0 license.