CCM-15020: Add reporting target configuration to send digital letters events to reporting#121
CCM-15020: Add reporting target configuration to send digital letters events to reporting#121gareth-allan wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configuration in the events Terraform component to optionally forward Digital Letters events to a Reporting destination, along with generated docs updates.
Changes:
- Extends
event_target_arnsto include an optionalreportingdestination and addsreporting_data_cross_account_targetinput. - Introduces a new data-plane EventBridge rule/target for
detail.typeprefixed withuk.nhs.notify.digital.letters.. - Updates component Terraform docs and recommends the Terraform VS Code extension in the workspace.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| project.code-workspace | Recommends the HashiCorp Terraform VS Code extension. |
| infrastructure/terraform/components/events/variables.tf | Adds reporting target ARN key and reporting_data_cross_account_target input. |
| infrastructure/terraform/components/events/README.md | Regenerates Terraform docs to reflect new inputs. |
| infrastructure/terraform/components/events/cloudwatch_event_rule_digital_letters_reporting.tf | Adds EventBridge rule/target + IAM role/policy for forwarding Digital Letters events to reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Action = "sns:Publish" | ||
| Resource = var.event_target_arns["reporting"] | ||
| }, | ||
| { | ||
| Action = [ | ||
| "kms:GenerateDataKey", | ||
| "kms:Encrypt", | ||
| "kms:DescribeKey", | ||
| "kms:Decrypt" | ||
| ], | ||
| Effect = "Allow", | ||
| Resource = "arn:aws:kms:${var.region}:${var.reporting_data_cross_account_target.account_id}:key/*" | ||
| Condition = { | ||
| "ForAnyValue:StringEquals" = { | ||
| "kms:ResourceAliases" = "alias/nhs-${var.reporting_data_cross_account_target.environment}-reporting" | ||
| } | ||
| } |
There was a problem hiding this comment.
The IAM policy attached to this EventBridge target allows sns:Publish, but this target ARN is described/used like an EventBridge forwarding target (consistent with other cross-account EventBridge targets in this component). If the reporting destination is an EventBridge event bus, this should instead grant events:PutEvents on the reporting bus ARN (and the SNS/KMS permissions should be removed). If the destination really is an SNS topic, then the PR description and variable naming/docs should be updated to reflect that it’s SNS publishing rather than EventBridge PutEvents.
| Action = "sns:Publish" | |
| Resource = var.event_target_arns["reporting"] | |
| }, | |
| { | |
| Action = [ | |
| "kms:GenerateDataKey", | |
| "kms:Encrypt", | |
| "kms:DescribeKey", | |
| "kms:Decrypt" | |
| ], | |
| Effect = "Allow", | |
| Resource = "arn:aws:kms:${var.region}:${var.reporting_data_cross_account_target.account_id}:key/*" | |
| Condition = { | |
| "ForAnyValue:StringEquals" = { | |
| "kms:ResourceAliases" = "alias/nhs-${var.reporting_data_cross_account_target.environment}-reporting" | |
| } | |
| } | |
| Action = "events:PutEvents" | |
| Resource = var.event_target_arns["reporting"] |
| count = var.event_target_arns["reporting"] != null ? 1 : 0 | ||
|
|
||
| rule = aws_cloudwatch_event_rule.digital_letters_reporting.name | ||
| arn = var.event_target_arns["reporting"] | ||
| target_id = "digital-letters-reporting" | ||
| event_bus_name = aws_cloudwatch_event_bus.data_plane.name | ||
| role_arn = aws_iam_role.digital_letters_reporting[0].arn | ||
| } |
There was a problem hiding this comment.
var.reporting_data_cross_account_target is dereferenced here, but the resources in this file are only count-guarded by var.event_target_arns["reporting"] != null. If event_target_arns.reporting is set without also setting reporting_data_cross_account_target, Terraform will fail with a null dereference. Consider updating the count conditions to also require var.reporting_data_cross_account_target != null (or using try(...)/validation to ensure the object is set whenever the reporting target is enabled).
| environment = optional(string, null) | ||
| account_id = optional(string, null) | ||
| }) | ||
| default = null |
There was a problem hiding this comment.
This variable’s attributes are declared as optional(..., null), but the new reporting rule/policy construction assumes environment and account_id are present (they’re interpolated into ARNs/aliases). To prevent invalid ARNs like ...::null:... at plan/apply time, consider making these fields required (non-optional) or adding a validation block that enforces both are set when reporting_data_cross_account_target is non-null.
| default = null | |
| default = null | |
| validation { | |
| condition = var.reporting_data_cross_account_target == null || ( | |
| var.reporting_data_cross_account_target.environment != null && | |
| var.reporting_data_cross_account_target.account_id != null | |
| ) | |
| error_message = "When reporting_data_cross_account_target is set, both environment and account_id must be provided." | |
| } |
Description
Implemented AI-assisted Terraform updates in the Events component to support Digital Letters reporting forwarding. Added an optional reporting key to event_target_arns, created a new data-plane EventBridge rule and optional target for detail.type prefix uk.nhs.notify.digital.letters., and added count-guarded IAM role/policy to allow events:PutEvents to the reporting target ARN. Updated generated Terraform docs for the component README. Validated with terraform fmt/validate and pre-commit; only Docker-dependent formatting hooks failed due local runtime issues, while Terraform lint/docs and other checks passed.
Context
Type of changes
Checklist
Sensitive Information Declaration
To ensure the utmost confidentiality and protect your and others privacy, we kindly ask you to NOT including PII (Personal Identifiable Information) / PID (Personal Identifiable Data) or any other sensitive data in this PR (Pull Request) and the codebase changes. We will remove any PR that do contain any sensitive information. We really appreciate your cooperation in this matter.