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(cloudwatch): alarms with accountId fails in regions that don't support cross-account alarms #16875

Merged
merged 11 commits into from
Oct 8, 2021
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-cloudwatch/lib/alarm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -286,7 +286,7 @@ export class Alarm extends AlarmBase {
unit: stat.unitFilter,
},
id: 'm1',
accountId: stat.account,
accountId: (stat.account === Stack.of(self).account) ? undefined : stat.account,
eladb marked this conversation as resolved.
Show resolved Hide resolved
label: conf.renderingProperties?.label,
returnData: true,
} as CfnAlarm.MetricDataQueryProperty,
Expand Down Expand Up @@ -321,7 +321,7 @@ export class Alarm extends AlarmBase {
unit: stat.unitFilter,
},
id: entry.id || uniqueMetricId(),
accountId: stat.account,
accountId: (stat.account === Stack.of(self).account) ? undefined : stat.account,
label: conf.renderingProperties?.label,
returnData: entry.tag ? undefined : false, // entry.tag evaluates to true if the metric is the math expression the alarm is based on.
};
Expand Down
108 changes: 99 additions & 9 deletions packages/@aws-cdk/aws-cloudwatch/test/cross-environment.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Template } from '@aws-cdk/assertions';
import { Match, Template } from '@aws-cdk/assertions';
import { Duration, Stack } from '@aws-cdk/core';
import { Alarm, GraphWidget, IWidget, MathExpression, Metric } from '../lib';

Expand Down Expand Up @@ -124,12 +124,10 @@ describe('cross environment', () => {
Namespace: 'Test',
Period: 300,
});


});

test('metric attached to stack1 will throw in stack2', () => {
// Cross-region/cross-account metrics are supported in Dashboards but not in Alarms
// Cross-region metrics are supported in Dashboards but not in Alarms

// GIVEN
expect(() => {
Expand All @@ -139,8 +137,6 @@ describe('cross environment', () => {
metric: a.attachTo(stack1),
});
}).toThrow(/Cannot create an Alarm in region 'mars' based on metric 'ACount' in 'pluto'/);


});

test('metric attached to stack3 will render in stack1', () => {
Expand Down Expand Up @@ -207,12 +203,49 @@ describe('cross environment', () => {
});
});

test('metric from same account as stack will not have accountId', () => {
// GIVEN

// including label property will force Alarm configuration to "modern" config.
const b = new Metric({
namespace: 'Test',
metricName: 'ACount',
label: 'my-label',
});

new Alarm(stack1, 'Alarm', {
threshold: 1,
evaluationPeriods: 1,
metric: b,
});

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

test('math expression can render in a different account', () => {
// GIVEN
const b = new Metric({
namespace: 'Test',
metricName: 'ACount',
account: '1234',
account: '5678',
});

const c = new MathExpression({
Expand Down Expand Up @@ -248,7 +281,64 @@ describe('cross environment', () => {
ReturnData: false,
},
{
AccountId: '1234',
AccountId: '5678',
Id: 'b',
MetricStat: {
Metric: {
MetricName: 'ACount',
Namespace: 'Test',
},
Period: 60,
Stat: 'Average',
},
ReturnData: false,
},
],
});
});

test('math expression from same account as stack will not have accountId', () => {
// GIVEN
const b = new Metric({
namespace: 'Test',
metricName: 'ACount',
account: '1234',
});

const c = new MathExpression({
expression: 'a + b',
usingMetrics: { a: a.attachTo(stack1), b },
period: Duration.minutes(1),
});

new Alarm(stack1, 'Alarm', {
threshold: 1,
evaluationPeriods: 1,
metric: c,
});

// THEN
Template.fromStack(stack1).hasResourceProperties('AWS::CloudWatch::Alarm', {
Metrics: [
{
Expression: 'a + b',
Id: 'expr_1',
},
{
AccountId: Match.absent(),
Id: 'a',
MetricStat: {
Metric: {
MetricName: 'ACount',
Namespace: 'Test',
},
Period: 60,
Stat: 'Average',
},
ReturnData: false,
},
{
AccountId: Match.absent(),
Id: 'b',
MetricStat: {
Metric: {
Expand Down Expand Up @@ -289,7 +379,7 @@ describe('cross environment', () => {
}).toThrow(/Cannot create an Alarm based on a MathExpression which specifies a searchAccount or searchRegion/);
});

test('match expression with different searchRegion will throw', () => {
test('math expression with different searchRegion will throw', () => {
// GIVEN
const b = new Metric({
namespace: 'Test',
Expand Down