feat(cloudwatch): add accountId property to support Cross-Account visibility for Logs and Metric Widgets
#31512
Conversation
This file contains 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
… prop in LogQueryWidget and the 4 Metrics Widgets (AlarmWidget, GaugeWidget, GraphWidget, and SingleValueSidget ) Signed-off-by: Sumu <sumughan@amazon.com>
… prop in LogQueryWidget and the 4 Metrics Widgets (AlarmWidget, GaugeWidget, GraphWidget, and SingleValueSidget )cd Signed-off-by: Sumu <sumughan@amazon.com>
…tests for each Widget type Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
(integ.alarm-and-dashboard.ts) Signed-off-by: Sumu <sumughan@amazon.com>
github-actions
bot
added
effort/small
Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
sumupitchayan
changed the title
accountId property to support Cross-Account/Cross-region with Logs and Metric WidgetsaccountId property to support Cross-Account with Logs and Metric Widgets
sumupitchayan
changed the title
accountId property to support Cross-Account with Logs and Metric WidgetsaccountId property to support Cross-Account visibility for Logs and Metric Widgets
aws-cdk-automation
added
the
pr/needs-maintainer-review
kaizencc
requested changes
Sep 30, 2024
There was a problem hiding this comment.
@sumupitchayan i've got a couple of questions before i give my approval
| @@ -61,6 +61,8 @@ dashboard.addWidgets(new cloudwatch.SingleValueWidget({ | |||
| })); | |||
| dashboard.addWidgets(new cloudwatch.LogQueryWidget({ | |||
| title: 'Errors in my log group', | |||
| // Setting an account ID just for this LogQueryWidget | |||
| accountId: '123456789', | |||
There was a problem hiding this comment.
we have specific accountIds that won't get flagged by security. is this one of them? also, account ids are 12 numbers long
There was a problem hiding this comment.
Oh that's good to know - I just put in a random number here, will update it 👍
| * | ||
| * @default - None | ||
| */ | ||
| readonly accountId?: string; |
There was a problem hiding this comment.
is the default really none? or is the default that the accountid is the same as the account doing the cfn deploy
There was a problem hiding this comment.
Good callout - the property is not required, but it uses the current account by default (docs)
I'll update the comment
| })); | ||
|
|
||
| dashboard.addWidgets(new cloudwatch.GraphWidget({ | ||
| title: 'Percentage of messages in each queue as pie chart', | ||
| left: [metricA, metricB], | ||
| view: cloudwatch.GraphWidgetView.PIE, | ||
| setPeriodToTimeRange: true, | ||
| accountId: '123456789', |
There was a problem hiding this comment.
how are any of these integ tests passing if this isn't a real account?
aws-cdk-automation
removed
the
pr/needs-maintainer-review
Signed-off-by: Sumu <sumughan@amazon.com>
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #26105
Reason for this change
Support cross-account visibility on Metric and Log Widget Objects by allowing customers to specify the AWS Account ID that the data (logs or metrics) originates from. For more information, see Cross-Account Cross-Region CloudWatch Console.
CloudWatch docs shows that
accountIdis supported in CloudFormation for Logs Widget Objects and Metric Widget Objects.Description of changes
Added an optional
accountIdproperty toLogQueryWidgetPropsandMetricWidgetProps. This allows you to set an Account ID onLogQueryWidgetand the four different metric widgets:AlarmWidgetGaugeWidgetGraphWidgetSingleValueWidgetDescription of how you validated changes
Unit Tests
In
graph.test.ts, I added theaccountIdproperty to existing tests on each of the five Widget types mentioned above, validating that they appear in the synthesized stack.Integ Tests
Same as above, I added
accountIdproperty to existing integ tests (integ.math-alarm-and-dashboard.tsandinteg.alarm-and-dashboard.ts) on each of the five Widget types, updated the snapshots and successfully deployed the integ tests.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license