fix(sagemaker): Removed the dependency on AWS CLI by msgupta-amazon · Pull Request #8703 · aws/aws-toolkit-vscode

msgupta-amazon · 2026-03-31T23:02:14Z

Problem

The listing functionality for parker does not work on windows. The root cause for the same was the dependency we had on AWS CLI which was our initial assumption while implementing Parker but after some discussions, we are aligned on that this won't be the case all the time and therefore we are removing the dependency of AWS CLI from our code base.

Solution

We have removed the dependency on AWS CLI while listing the dev spaces and establishing connections.
Please review this PR as well which covers unit test cases for the same.
https://github.com/msgupta-amazon/aws-toolkit-vscode/pull/2/changes

As per the current architecture for supporting connections in sagemaker the duplicate code check is failing. The code duplicated is less than 15 code lines and are required.

Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

amazon-inspector-ohio · 2026-03-31T23:02:19Z

⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done

github-actions · 2026-03-31T23:02:28Z

This pull request modifies code in src/* but no tests were added/updated.
- Confirm whether tests should be added or ensure the PR description explains why tests are not required.
This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
- Note: beta or "experiment" features that have active users should announce fixes in the changelog.
- If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

amazon-inspector-ohio · 2026-03-31T23:02:50Z

✅ I finished the code review, and didn't find any security or code quality issues.

packages/core/resources/hyperpod_connect.ps1

packages/core/src/awsService/sagemaker/detached-server/kubectlClientStub.ts

aws-ajangg · 2026-04-01T00:11:47Z

packages/core/src/awsService/sagemaker/detached-server/kubectlClientStub.ts

+            const status = body.status
+            const presignedUrl = status?.workspaceConnectionUrl
+            const connectionType = status?.workspaceConnectionType
+            const token = status?.tokenValue ?? ''


if token and session are empty, how does this affect connection?

added the response in the comment in the code.

packages/core/src/awsService/sagemaker/detached-server/kubectlClientStub.ts

packages/core/src/shared/clients/kubectlClient.ts

packages/core/src/awsService/sagemaker/model.ts

packages/core/src/shared/clients/kubectlClient.ts

aws-ajangg · 2026-04-01T17:47:44Z

what kind of testing have we completed?

msgupta-amazon · 2026-04-01T19:04:08Z

We have completed the basic testing of the feature

packages/core/src/awsService/sagemaker/detached-server/kubectlClientStub.ts

packages/core/src/shared/clients/eksTokenGenerator.ts

packages/core/src/shared/clients/kubectlClient.ts

sgganjo

nit: Do we also need a changelog entry since this is a fix

packages/core/src/awsService/sagemaker/detached-server/kubectlClientStub.ts

msgupta-amazon · 2026-04-06T19:31:29Z

Added the changelog entry

aws-ajangg · 2026-04-06T19:48:50Z

can we fix ci for lint dup code

msgupta-amazon · 2026-04-06T20:06:22Z

It will be there as we have some test cases for the functionality in detached-server and then in normal directory as well.

aws-ajangg · 2026-04-06T20:19:01Z

can we move to shared or utils in detached-server and import to kubectlClient test? same thing we do for any shared code btwn detached-server and the rest of the directory

msgupta-amazon · 2026-04-06T20:25:57Z

We have already removed the duplicate code during implementation by using the above approach.
Right now, only the code in test cases which is responsible for generating tokens is getting duplicated which is necessary before the tests execution and should not be a blocker here.

packages/core/src/awsService/sagemaker/explorer/sagemakerHyperpodNode.ts

aws-ajangg · 2026-04-07T19:40:26Z

What does basic testing entail? Has anyone tested your vsix for confirmation or have we conducted a bug bash? This implementation changes how the listing feature obtains credentials so I think we should have extensive testing coverage. This change is intended for Windows but will also apply for macOS so have we tested that edge case? Have we tried testing on fresh slate to replicate assuming the role of a new customer?

…n one file and have updated the test cases

…entials at load time for the list

msgupta-amazon · 2026-04-07T20:46:32Z

@aws-ajangg I have shared with the team earlier. I think you missed it. Extensive testing is been done to verify the feature in both the environments and sanity testing is also completed.

edwardps · 2026-04-07T18:16:46Z

packages/core/resources/hyperpod_connect.ps1

+    $jsonObj = @{ streamUrl = $streamUrl; tokenValue = $token; sessionId = $sessionId }
+    $jsonStr = $jsonObj | ConvertTo-Json -Compress
+    $envVarName = "AWS_SSM_START_SESSION_RESPONSE"
+    $env:AWS_SSM_START_SESSION_RESPONSE = $jsonStr


Question: why do we need to make this change?

After removing the AWS CLI dependency, connection started failing due to quoting/escaping rules of powershell and hence we made the change to resolve the issue.

edwardps · 2026-04-07T18:49:30Z

packages/core/src/shared/clients/eksTokenGenerator.ts

+        headers: {
+            host: `sts.${region}.amazonaws.com`,
+            'x-k8s-aws-id': clusterName,
+        },


we have the typescript AWS SDK, why we do this http based access?

This underlying uses @smithy/signature-v4 and @smithy/protocol-http which are part of the AWS SDK v3. EKS token generation requires presigning an STS GetCallerIdentity request, and the JS SDK v3 doesn't have a higher-level API for this — it's the same approach aws eks get-token uses under the hood.

msgupta-amazon requested a review from a team as a code owner March 31, 2026 23:02

aws-ajangg reviewed Mar 31, 2026

View reviewed changes

packages/core/resources/hyperpod_connect.ps1 Show resolved Hide resolved