fix(hosting): place CloudFront 5xx alarm in us-east-1 (#481) - #488
fix(hosting): place CloudFront 5xx alarm in us-east-1 (#481)#488sarayev wants to merge 4 commits into
Conversation
AWS/CloudFront metrics are published only in us-east-1, and a CloudWatch
alarm can only evaluate a metric in its own region (confirmed by the
CloudWatch docs "Cross-Region functionality is not supported for alarms"
and rejected by aws-cdk-lib at synth). For any hosting stack outside
us-east-1 the CloudFront5xxRate alarm never received a datapoint and,
with treatMissingData: NOT_BREACHING, sat at OK forever instead of
alarming — a monitoring alarm that silently never fires.
Fix (two-topic design): off-region, defer the CloudFront alarm to a
dedicated hosting-owned us-east-1 support stack that owns its own SNS
topic. Its topic ARN is surfaced as MonitoringTopicArnUsEast1 (an output
of the support stack) for the operator to subscribe to, alongside the
regional MonitoringTopicArn. The regional alarms (SSR/image/DLQ) are
unchanged — their metrics are regional. In-region (us-east-1) behavior
is unchanged: single stack, alarm created locally.
New prop monitoring.cloudFrontAlarm: 'usEast1Stack' (default) | 'skip'.
The default requires env:{account,region} off-region (a cross-region
stack needs a concrete account); env-agnostic off-region synth throws
MonitoringErrorRequiredError with guidance. 'skip' emits a synth warning
and creates no second stack.
Notes:
- The us-east-1 alarm references the regional distribution id, which CDK
bridges with its standard cross-region export reader/writer custom
resources (added to both stacks automatically) — not a runtime
forwarder.
- Breaking-ish: the default now synthesizes a second CloudFormation stack
off-region and requires env:{account,region}.
Tests: MonitoringConstruct deferral unit tests; new
hosting_construct.cf_alarm_region.test.ts covering off-region two-stack
synth, env-required throw, skip warning, and in-region single-stack.
All 853 hosting tests pass.
🦋 Changeset detectedLatest commit: c0b3ab8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… handling, core cloudFrontAlarm prop, split MonitoringStageRequiredError, lint
…Alarm passthrough, document defensive stage guard
There was a problem hiding this comment.
Nice catch on the root cause. Off-region, the old CloudFront5xxRate alarm sat at OK forever (metric only in us-east-1 + treatMissingData: NOT_BREACHING) — a monitoring alarm that silently never fired. The deferral to a hosting-owned us-east-1 stack with its own topic is a clean approach, region/account gating reads correctly, and alarm-config parity with the original is exact.
I built the branch (Node 22, aws-cdk-lib 2.257.0) and ran hosting_construct.cf_alarm_region.test.ts — build green, 4/4 pass. Worth noting for future readers: the us-east-1 alarm references this.distribution.distributionId (an unresolved cross-stack token), and CDK auto-bridges that with CrossRegionExportWriter/ExportsReader custom resources on this CDK version without crossRegionReferences: true. That works — but it means the code comments claiming "plain string / no cross-region reference" are inaccurate (see inline).
The one thing I'd like a maintainer call on before merge: is breaking-by-default the behavior we want? The new 'usEast1Stack' default makes a previously-synthesizing env-agnostic off-region stack now throw MonitoringEnvRequiredError. The minor changeset is the correct label for that (0.x → minor = breaking), so semver is fine — this is purely a "do we want the default to break, or default to 'skip'?" product decision. Please also make the changeset state plainly that this is a breaking change requiring env off-region.
Remaining items are all non-blocking suggestions/nits inline. Recommend an off-region E2E destroy→deploy pass to confirm the new cross-region custom resources don't hit the known "cannot delete export in use" / CREATE-vs---revert-drift issues.
| export type UsEast1MonitoringStackProps = StackProps & { | ||
| /** | ||
| * CloudFront distribution id to alarm on. Passed as a plain string so | ||
| * this us-east-1 stack takes no cross-region CDK reference on the |
There was a problem hiding this comment.
The doc says distributionId is 'passed as a plain string so this us-east-1 stack takes no cross-region CDK reference.' That's not what happens on the default path: the caller passes this.distribution.distributionId, an unresolved cross-stack token, and CDK does create cross-region export machinery (CrossRegionExportWriter in the regional stack, ExportsReader Fn::GetAtt here). Please reword to say this creates a cross-region reference that CDK bridges via export writer/reader custom resources. The 'plain string' escape hatch is only real when a caller supplies a concrete id.
| }); | ||
| } | ||
| // The us-east-1 CloudFront alarm must reference the regional | ||
| // distribution's id; CDK bridges that with its standard |
There was a problem hiding this comment.
This comment ('CDK bridges that with its standard cross-region export reader/writer custom resources ... automatically') is correct, but it contradicts the distributionId prop doc in us_east_1_monitoring_stack.ts ('no cross-region reference'). Please reconcile the two so the same story is told in both places.
| // re-output here, to keep the wiring one-directional. | ||
| new UsEast1MonitoringStack( | ||
| stage, | ||
| `${hostingStack.stackName}-CfMonitoring`, |
There was a problem hiding this comment.
${hostingStack.stackName}-CfMonitoring collides if two HostingConstructs are instantiated in the same stack (duplicate construct id under the same Stage). Since HostingConstruct is an embeddable L3, consider folding the construct's node id/addr into the support-stack id to keep it unique.
| supportTemplate.resourcePropertiesCountIs( | ||
| 'AWS::CloudWatch::Alarm', | ||
| Match.objectLike({ | ||
| Namespace: 'AWS/CloudFront', |
There was a problem hiding this comment.
This asserts only Namespace/MetricName/Threshold/TreatMissingData. Since the point is that the re-homed alarm is identical to the regional one, please also assert dimensionsMap (Region: 'Global' + DistributionId), ComparisonOperator, Period, Statistic, and EvaluationPeriods to lock parity against future drift.
| // The us-east-1 topic ARN is surfaced as an output of the support stack. | ||
| supportTemplate.hasOutput( | ||
| '*', | ||
| Match.objectLike({ Description: Match.stringLikeRegexp('us-east-1') }), |
There was a problem hiding this comment.
hasOutput('*', { Description: /us-east-1/ }) matches on the description, but docs and operators depend on the logical id MonitoringTopicArnUsEast1. Assert hasOutput('MonitoringTopicArnUsEast1', ...) so a rename can't silently pass.
| monitoring: props.monitoring, | ||
| monitoring: props.monitoring | ||
| ? { | ||
| ...props.monitoring, |
There was a problem hiding this comment.
Minor: { ...props.monitoring } is a functional no-op here — cloudFrontAlarm already flowed through the previous monitoring: props.monitoring. Fine to keep for the anchoring comment, but the @aws-blocks/core: patch changeset shouldn't imply core behavior changed; it's just surfacing the type.
| owns its own encrypted SNS topic and exposes a `MonitoringTopicArnUsEast1` | ||
| CloudFormation output for operator subscriptions. | ||
|
|
||
| Off-region deployments now defer the CloudFront 5xx alarm to a synthesized |
There was a problem hiding this comment.
Two things: (1) the off-region explanation is repeated in two adjacent paragraphs — collapse to one. (2) Please state explicitly that this is a breaking change (new default synthesizes a second stack off-region and throws MonitoringEnvRequiredError for env-agnostic off-region stacks) so it reads that way in the changelog. The minor bump is the right label for a breaking change at 0.x.
svidgen
left a comment
svidgen
left a comment
There was a problem hiding this comment.
Small design with API BR review please. Details in slack.
3 participants
Fixes #481.
Root cause
AWS/CloudFrontmetrics are published only in us-east-1, and a CloudWatch alarm can only evaluate a metric in its own region — confirmed by the CloudWatch docs ("Cross-Region functionality is not supported for alarms, so you cannot create an alarm in one Region that watches a metric in a different Region") and rejected by aws-cdk-lib at synth.new Metric({ region: 'us-east-1' })is honored for dashboards but throws for alarms.MonitoringConstructcreated theCloudFront5xxRatealarm in the hosting stack's region. For any stack outside us-east-1 the alarm never received a datapoint and, withtreatMissingData: NOT_BREACHING, sat atOKforever instead of alarming — a monitoring alarm that silently never fires.monitoring.enableddefaults totrue, so every off-region hosting stack was affected. (waf_construct.tsalready handles the same CloudFront/us-east-1 constraint; the alarm had no equivalent.)Fix — two-topic design
Off-region, the CloudFront alarm is deferred to a dedicated, hosting-owned us-east-1 support stack (
UsEast1MonitoringStack) that owns its own SNS topic. That topic's ARN is surfaced asMonitoringTopicArnUsEast1(an output of the support stack) for the operator to subscribe to, alongside the regionalMonitoringTopicArn. No forwarder Lambda and no runtime message forwarding.New prop
monitoring.cloudFrontAlarm: 'usEast1Stack' (default) | 'skip''usEast1Stack': create the support stack. Requiresenv: { account, region }off-region (a cross-region stack needs a concrete account). Env-agnostic off-region synth throwsHostingError('MonitoringEnvRequiredError')with resolution guidance.'skip': emit a synth warning, create no second stack (works env-agnostic).Breaking-ish notes for reviewers
env: { account, region }.Tests
monitoring_construct.test.ts: deferral unit tests (createCloudFrontAlarmLocally: false→ no CF alarm +cloudFrontAlarmDeferred, regional alarms still created).hosting_construct.cf_alarm_region.test.ts: off-region two-stack synth (CF alarm in the us-east-1 stack +MonitoringTopicArnUsEast1output, one SNS topic, no forwarder subscription); env-agnostic off-region →MonitoringEnvRequiredError;'skip'mode warning + no second stack; in-region single-stack with local alarm.Do not merge — opened for review.