Skip to content
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

fix(cwl): Pass in active credentials when creating CWL client for LiveTail #5993

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

keeganirby
Copy link

@keeganirby keeganirby commented Nov 13, 2024

Problem

When creating the CloudWatchClient for a LiveTail session, we are not specifying which credentials to use. This is causing the client to always use the Default credential profile, even if a different AWS Credential profile is selected within AWSToolkit.

Solution

Resolve the active AWS credential from globals.awsContext, and supply that to the LiveTailSession constructor. These credentials are then use to construct the CWL client.


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@keeganirby keeganirby requested a review from a team as a code owner November 13, 2024 03:00
Copy link

  • 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.

@@ -25,12 +25,16 @@ export async function tailLogGroup(
if (!wizardResponse) {
throw new CancellationError('user')
}

const awsCredentials = await globals.awsContext.getCredentials()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with how this works under the hood. Will it fall back to default credentials if nobody is logged into the toolkit? Or does it require someone to be logged in? If so, maybe we should re-direct them to login with credentials if they do not exist?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have no selected credential, the explorer side bar will prompt you to add/select one.
Screenshot 2024-11-13 at 10 39 36 AM

If the selected credential is expired, it will say that, and give you an option to re-authenticate.
Screenshot 2024-11-13 at 10 40 49 AM

If your credential is expired/unselected and you try to select TailLogGroup from the command palette, this first picker to select a LogGroup will show [Error loading Items] Toolkit is not logged in.
Screenshot 2024-11-13 at 10 39 15 AM

@justinmk3
Copy link
Contributor

Note that this is part of the motivation for #5940 , where we are starting work on a v3 "client builder" which will handle this stuff automatically. We have a v2 clientbuilder already, but are missing one for v3. @Hweinstock

const liveTailSessionConfig: LiveTailSessionConfiguration = {
logGroupArn: wizardResponse.regionLogGroupSubmenuResponse.data,
logStreamFilter: wizardResponse.logStreamFilter,
logEventFilterPattern: wizardResponse.filterPattern,
region: wizardResponse.regionLogGroupSubmenuResponse.region,
awsCredentials: awsCredentials,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to call globals.awsContext.getCredentials() implicitly in your client, instead of plumbing it through like this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explored that. But ran into an issue. We currently build & set the CWL client on theLiveTailSession in its constructor. globals.awsContext.getCredentials() is async, which cannot be awaited in the constructor since constructors cannot also be aysnc.
Hence why we are awaiting the credentials in tailLogGroup command, and plumbing it.

Something I just considered is that we could maybe construct and set the CWL client only when startLiveTail is actually called, since that is when the Client is actually needed. That method can be async. I'll explore that more and follow up.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lobals.awsContext.getCredentials() is async, which cannot be awaited in the constructor since constructors cannot also be aysnc.

We usually add a create() "factory function" to work around that, and make the constructor private.

@justinmk3 justinmk3 merged commit 30d9217 into aws:feature/cwltail Nov 13, 2024
32 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants