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.

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

feat(cloudwatch): add support for cross-account alarms #16007

Merged
merged 12 commits into from
Aug 25, 2021
Merged

feat(cloudwatch): add support for cross-account alarms #16007

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
340f1df
pipe account to metricdataquery if necessary
kaizencc Aug 11, 2021
2265b01
Merge branch 'aws:master' into xa-alarms
kaizencc Aug 11, 2021
0821355
fix accidental imports
kaizencc Aug 11, 2021
d128fcc
satisfy linter
kaizencc Aug 11, 2021
317b7c1
Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …
kaizencc Aug 11, 2021
749fd0a
update readme for cross-account alarms
kaizencc Aug 11, 2021
ed1fd3a
add renderAsLegacyMetric
kaizencc Aug 13, 2021
f178c89
Merge branch 'master' into xa-alarms
kaizencc Aug 13, 2021
f1451ff
add new test and more readme docs
kaizencc Aug 23, 2021
d34db67
Merge branch 'xa-alarms' of https://github.com/kaizen3031593/aws-cdk …
kaizencc Aug 23, 2021
a7666f1
remove 1 line
kaizencc Aug 23, 2021
e82b5b1
Merge branch 'master' into xa-alarms
mergify[bot] Aug 24, 2021
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
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-cloudwatch/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,8 @@ The most important properties to set while creating an Alarms are:
- `evaluationPeriods`: how many consecutive periods the metric has to be
breaching the the threshold for the alarm to trigger.

To create a cross-account alarm, make sure you have enabled [cross-account functionality](https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/Cross-Account-Cross-Region.html) in CloudWatch. Then, set the `account` property in the `Metric` object either manually or via the `metric.attachTo()` function.
kaizencc marked this conversation as resolved.
Show resolved Hide resolved

### Alarm Actions

To add actions to an alarm, use the integration classes from the
Expand Down
11 changes: 6 additions & 5 deletions packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,10 @@ export class Alarm extends AlarmBase {
return dispatchMetric(metric, {
withStat(stat, conf) {
self.validateMetricStat(stat, metric);
if (conf.renderingProperties?.label == undefined) {
const canRenderAsLegacyMetric = conf.renderingProperties?.label == undefined &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what you mean by the term "legacy" here? This doesn't quite make sense to me, since the accountId is only set if this is true, which would imply that a cross-account alarm only works if the metric is in a "legacy" format of some kind. But that doesn't make sense, since cross-account alarms are a new feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "legacy" term refers to the fact that cloudwatch has two ways to provision metrics on alarms, and only one way is being updated with new features like cross-account alarms. We want to avoid causing unnecessary diffs, so we shepherd metrics that do not require these "new" features to the "legacy" properties - the new way to provision metrics is via the CfnAlarm.MetricDataQueryProperty.

Here accountId is only set if canRenderAsLegacyMetric is false, not true. So it is correctly named here. (what seems to be the confusion is that github is hiding the unchanged lines and makes it seem like accountId is inside the if (canRenderAsLegacyMetric) scope when it is actually not.)

Copy link
Contributor

Choose a reason for hiding this comment

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

aha, thank you for the explanation! The github diff was confusing me here as well :)

(stat.account == undefined || Stack.of(self).account == stat.account);
// Do this to disturb existing templates as little as possible
if (canRenderAsLegacyMetric) {
return dropUndefined({
dimensions: stat.dimensions,
namespace: stat.namespace,
Expand All @@ -283,6 +286,7 @@ export class Alarm extends AlarmBase {
unit: stat.unitFilter,
},
id: 'm1',
accountId: stat.account,
label: conf.renderingProperties?.label,
returnData: true,
} as CfnAlarm.MetricDataQueryProperty,
Expand Down Expand Up @@ -344,17 +348,14 @@ export class Alarm extends AlarmBase {
}

/**
* Validate that if a region and account are in the given stat config, they match the Alarm
* Validate that if a region is in the given stat config, they match the Alarm
*/
private validateMetricStat(stat: MetricStatConfig, metric: IMetric) {
const stack = Stack.of(this);

if (definitelyDifferent(stat.region, stack.region)) {
throw new Error(`Cannot create an Alarm in region '${stack.region}' based on metric '${metric}' in '${stat.region}'`);
}
if (definitelyDifferent(stat.account, stack.account)) {
throw new Error(`Cannot create an Alarm in account '${stack.account}' based on metric '${metric}' in '${stat.account}'`);
}
}
}

Expand Down
33 changes: 32 additions & 1 deletion packages/@aws-cdk/aws-cloudwatch/test/cross-environment.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ const a = new Metric({ namespace: 'Test', metricName: 'ACount' });

let stack1: Stack;
let stack2: Stack;
let stack3: Stack;
describe('cross environment', () => {
beforeEach(() => {
stack1 = new Stack(undefined, undefined, { env: { region: 'pluto', account: '1234' } });
stack2 = new Stack(undefined, undefined, { env: { region: 'mars', account: '5678' } });

stack3 = new Stack(undefined, undefined, { env: { region: 'pluto', account: '0000' } });
});

describe('in graphs', () => {
Expand Down Expand Up @@ -112,6 +113,36 @@ describe('cross environment', () => {


});

test('metric attached to stack3 will render in stack1 ', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a test that uses the method of setting the account property on the metric instead of using attachTo? There should be a unit test for each way that it is possible to use the feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added an additional test that uses the account property, but I do want to push back on this a bit (or ask for clarification). Under the hood, metric.AttachTo() simply updates the metric object with the account property, so I see it as a convenience method and not an additional way to use this feature. To me, it seems like the second test duplicates the first. Does this still warrant an additional unit test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it is somewhat of a grey area. But I still think the extra test is helpful. Your reasoning for the additional unit test being unnecessary is based on an implementation detail. What if that changes? In this particular scenario, it doesn't seem likely. But in general, unit tests should test the API in all ways that a customer could use it, and make sure that the correct behaviour/result happens in all of them. So that if someone accidentally breaks something by changing an implementation detail, then the unit tests will catch it.

//Cross-account metrics are supported in Alarms

// GIVEN
new Alarm(stack1, 'Alarm', {
threshold: 1,
evaluationPeriods: 1,
metric: a.attachTo(stack3),
});

// THEN
Template.fromStack(stack1).hasResourceProperties('AWS::CloudWatch::Alarm', {
Metrics: [
{
AccountId: '0000',
Id: 'm1',
MetricStat: {
Metric: {
MetricName: 'ACount',
Namespace: 'Test',
},
Period: 300,
Stat: 'Average',
},
ReturnData: true,
},
],
});
});
});
});

Expand Down