Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 13, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed79310
Pass in active credentials when created CWL client for LiveTail
Nov 13, 2024
1c5201d
Pass in mock creds in LiveTailRegistry tests
Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/core/src/awsService/cloudWatchLogs/commands/tailLogGroup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
LiveTailSessionUpdate,
StartLiveTailResponseStream,
} from '@aws-sdk/client-cloudwatch-logs'
import { getLogger, ToolkitError } from '../../../shared'
import { getLogger, globals, ToolkitError } from '../../../shared'
import { uriToKey } from '../cloudWatchLogsUtils'

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

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

@jpinkney-aws jpinkney-aws Nov 13, 2024

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
Contributor Author

@keeganirby keeganirby Nov 13, 2024

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

if (awsCredentials === undefined) {
throw new ToolkitError('Failed to start LiveTail session: credentials are undefined.')
}
const liveTailSessionConfig: LiveTailSessionConfiguration = {
logGroupArn: wizardResponse.regionLogGroupSubmenuResponse.data,
logStreamFilter: wizardResponse.logStreamFilter,
logEventFilterPattern: wizardResponse.filterPattern,
region: wizardResponse.regionLogGroupSubmenuResponse.region,
awsCredentials: awsCredentials,
Copy link
Contributor

@justinmk3 justinmk3 Nov 13, 2024

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
Contributor Author

@keeganirby keeganirby Nov 13, 2024

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

@justinmk3 justinmk3 Nov 13, 2024

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.

}
const session = new LiveTailSession(liveTailSessionConfig)
if (registry.has(uriToKey(session.uri))) {
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/awsService/cloudWatchLogs/registry/liveTailSession.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/
import * as vscode from 'vscode'
import * as AWS from '@aws-sdk/types'
import {
CloudWatchLogsClient,
StartLiveTailCommand,
Expand All @@ -19,6 +20,7 @@ export type LiveTailSessionConfiguration = {
logStreamFilter?: LogStreamFilterResponse
logEventFilterPattern?: string
region: string
awsCredentials: AWS.Credentials
}

export type LiveTailSessionClient = {
Expand Down Expand Up @@ -49,6 +51,7 @@ export class LiveTailSession {
this.logStreamFilter = configuration.logStreamFilter
this.liveTailClient = {
cwlClient: new CloudWatchLogsClient({
credentials: configuration.awsCredentials,
region: configuration.region,
customUserAgent: getUserAgent(),
}),
Expand Down
20 changes: 19 additions & 1 deletion packages/core/src/test/awsService/cloudWatchLogs/commands/tailLogGroup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ import {
import { getTestWindow } from '../../../shared/vscode/window'
import { CloudWatchLogsSettings, uriToKey } from '../../../../awsService/cloudWatchLogs/cloudWatchLogsUtils'
import { installFakeClock } from '../../../testUtil'
import { DefaultAwsContext } from '../../../../shared'
import { DefaultAwsContext, ToolkitError } from '../../../../shared'

describe('TailLogGroup', function () {
const testLogGroup = 'test-log-group'
const testRegion = 'test-region'
const testMessage = 'test-message'
const testAwsAccountId = '1234'
const testAwsCredentials = {} as any as AWS.Credentials

let sandbox: sinon.SinonSandbox
let registry: LiveTailSessionRegistry
Expand Down Expand Up @@ -57,6 +58,8 @@ describe('TailLogGroup', function () {

it('starts LiveTailSession and writes to document. Closes tab and asserts session gets closed.', async function () {
sandbox.stub(DefaultAwsContext.prototype, 'getCredentialAccountId').returns(testAwsAccountId)
sandbox.stub(DefaultAwsContext.prototype, 'getCredentials').returns(Promise.resolve(testAwsCredentials))

wizardSpy = sandbox.stub(TailLogGroupWizard.prototype, 'run').callsFake(async function () {
return getTestWizardResponse()
})
Expand Down Expand Up @@ -122,6 +125,19 @@ describe('TailLogGroup', function () {
assert.strictEqual(stopLiveTailSessionSpy.calledOnce, true)
})

it('throws if crendentials are undefined', async function () {
sandbox.stub(DefaultAwsContext.prototype, 'getCredentials').returns(Promise.resolve(undefined))
wizardSpy = sandbox.stub(TailLogGroupWizard.prototype, 'run').callsFake(async function () {
return getTestWizardResponse()
})
await assert.rejects(async () => {
await tailLogGroup(registry, {
groupName: testLogGroup,
regionName: testRegion,
})
}, ToolkitError)
})

it('closeSession removes session from registry and calls underlying stopLiveTailSession function.', function () {
stopLiveTailSessionSpy = sandbox
.stub(LiveTailSession.prototype, 'stopLiveTailSession')
Expand All @@ -132,6 +148,7 @@ describe('TailLogGroup', function () {
const session = new LiveTailSession({
logGroupArn: testLogGroup,
region: testRegion,
awsCredentials: testAwsCredentials,
})
registry.set(uriToKey(session.uri), session)

Expand All @@ -145,6 +162,7 @@ describe('TailLogGroup', function () {
const session = new LiveTailSession({
logGroupArn: testLogGroup,
region: testRegion,
awsCredentials: testAwsCredentials,
})
const testData = 'blah blah blah'
const document = await vscode.workspace.openTextDocument(session.uri)
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/test/awsService/cloudWatchLogs/registry/liveTailRegistry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,14 @@ import { cloudwatchLogsLiveTailScheme } from '../../../../shared/constants'
describe('LiveTailSession URI', async function () {
const testLogGroupName = 'test-log-group'
const testRegion = 'test-region'
const testAwsCredentials = {} as any as AWS.Credentials
const expectedUriBase = `${cloudwatchLogsLiveTailScheme}:${testRegion}:${testLogGroupName}`

it('is correct with no logStream filter, no filter pattern', function () {
const config: LiveTailSessionConfiguration = {
logGroupArn: testLogGroupName,
region: testRegion,
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(expectedUriBase)
const uri = createLiveTailURIFromArgs(config)
Expand All @@ -28,6 +30,7 @@ describe('LiveTailSession URI', async function () {
logGroupArn: testLogGroupName,
region: testRegion,
logEventFilterPattern: 'test-filter',
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(`${expectedUriBase}:test-filter`)
const uri = createLiveTailURIFromArgs(config)
Expand All @@ -41,6 +44,7 @@ describe('LiveTailSession URI', async function () {
logStreamFilter: {
type: 'all',
},
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(`${expectedUriBase}:all`)
const uri = createLiveTailURIFromArgs(config)
Expand All @@ -55,6 +59,7 @@ describe('LiveTailSession URI', async function () {
type: 'prefix',
filter: 'test-prefix',
},
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(`${expectedUriBase}:prefix:test-prefix`)
const uri = createLiveTailURIFromArgs(config)
Expand All @@ -69,6 +74,7 @@ describe('LiveTailSession URI', async function () {
type: 'specific',
filter: 'test-stream',
},
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(`${expectedUriBase}:specific:test-stream`)
const uri = createLiveTailURIFromArgs(config)
Expand All @@ -84,6 +90,7 @@ describe('LiveTailSession URI', async function () {
filter: 'test-stream',
},
logEventFilterPattern: 'test-filter',
awsCredentials: testAwsCredentials,
}
const expectedUri = vscode.Uri.parse(`${expectedUriBase}:specific:test-stream:test-filter`)
const uri = createLiveTailURIFromArgs(config)
Expand Down
Loading