Skip to content

BAU: use well-known for cribl kinesis arns#1842

Merged
whi-tw merged 1 commit into
mainfrom
whi-tw/use_well_known_more
Nov 26, 2025
Merged

BAU: use well-known for cribl kinesis arns#1842
whi-tw merged 1 commit into
mainfrom
whi-tw/use_well_known_more

Conversation

@whi-tw
Copy link
Copy Markdown
Contributor

@whi-tw whi-tw commented Nov 12, 2025

What problem does this pull request solve?

  • BAU: use well-known module for Cribl Kinesis dest

Things to consider when reviewing

  • Ensure that you consider the wider context.
  • Does it work when run on your machine?
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Are there all the unit tests needed?
  • Has all relevant documentation been updated?

Reminders

If you've made changes to the deployer role (files in modules/deployer-access):

  • Remember to run make <environment> forms/account apply on the relevant environments (dev, staging, user-research, and/or prod)
  • Check the #govuk-forms-deployment-notifications Slack channel to ensure the apply-forms-terraform-<environment> pipelines have run successfully

@whi-tw whi-tw changed the base branch from main to whi-tw/s3-to-cribl November 12, 2025 13:12
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 12, 2025

💰 Infracost report

Monthly estimate generated

Estimate details (includes details of unsupported resources and skipped projects due to errors)
──────────────────────────────────
Project: forms-health-dev
Module path: infra/deployments/forms/health
Errors:
  Error loading Terraform modules:
    could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
      /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
        Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)
  Diff baseline error:
    Error loading Terraform modules:
      could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
        /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
          Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)

──────────────────────────────────
Project: forms-health-production
Module path: infra/deployments/forms/health
Errors:
  Error loading Terraform modules:
    could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
      /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
        Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)
  Diff baseline error:
    Error loading Terraform modules:
      could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
        /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
          Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)

──────────────────────────────────
Project: forms-health-staging
Module path: infra/deployments/forms/health
Errors:
  Error loading Terraform modules:
    could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
      /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
        Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)
  Diff baseline error:
    Error loading Terraform modules:
      could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
        /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
          Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)

──────────────────────────────────
Project: forms-health-user-research
Module path: infra/deployments/forms/health
Errors:
  Error loading Terraform modules:
    could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
      /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
        Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)
  Diff baseline error:
    Error loading Terraform modules:
      could not load modules for path /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health failed to parse file /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf diag:
        /home/runner/work/forms-deploy/forms-deploy/infra/deployments/forms/health/alerts/variables.tf:32,90-91:
          Missing argument separator; A comma is required to separate each function argument from the next., and 1 other diagnostic(s)

──────────────────────────────────
──────────────────────────────────
53 projects have no cost estimate changes.
Run the following command to see their breakdown: infracost breakdown --path=/path/to/code

──────────────────────────────────
3650 cloud resources were detected:
∙ 673 were estimated
∙ 2952 were free
∙ 25 are not supported yet, see https://infracost.io/requested-resources:
  ∙ 21 x aws_codepipeline
  ∙ 4 x aws_guardduty_malware_protection_plan
This comment will be updated when code changes.

@whi-tw whi-tw force-pushed the whi-tw/s3-to-cribl branch from b5b9ff3 to 226680d Compare November 12, 2025 13:13
@whi-tw whi-tw force-pushed the whi-tw/use_well_known_more branch from e80e616 to b4abcf6 Compare November 12, 2025 13:14
@whi-tw whi-tw force-pushed the whi-tw/s3-to-cribl branch from 226680d to 2209f2e Compare November 12, 2025 13:15
@whi-tw whi-tw force-pushed the whi-tw/use_well_known_more branch 2 times, most recently from 21dbff8 to a321e98 Compare November 12, 2025 13:25
@whi-tw whi-tw requested a review from Copilot November 13, 2025 12:02
@whi-tw whi-tw marked this pull request as ready for review November 13, 2025 12:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request refactors the Cribl Kinesis logging infrastructure to use a well-known module for managing Kinesis destination ARNs. The change centralizes the destination ARN definitions and removes the need to pass them as variables through multiple layers of the infrastructure code.

Key changes:

  • Created a new well-known/cribl module that provides Kinesis destination names and ARNs for both eu-west-2 and us-east-1 regions
  • Simplified variable interfaces by replacing the log_to_splunk_settings object with a single kinesis_subscription_role_arn string variable
  • Removed hardcoded Kinesis destination ARNs from tfvars files across all environments

Reviewed Changes

Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
infra/modules/well-known/cribl/kinesis.tf New module defining well-known Kinesis destination names and ARN construction logic
infra/modules/ecs-service/logging.tf Updated to use well-known module for Kinesis destination ARN
infra/modules/cloudfront_waf_protection/waf.tf Updated to use well-known module for Kinesis destination ARN (us-east-1)
infra/modules/alb_waf_protection/waf.tf Updated to use well-known module for Kinesis destination ARN
infra/modules/forms-runner/variables.tf Simplified variable from object to string for kinesis_subscription_role_arn
infra/modules/forms-product-page/variables.tf Simplified variable from object to string for kinesis_subscription_role_arn
infra/modules/forms-admin/variables.tf Simplified variable from object to string for kinesis_subscription_role_arn
infra/modules/environment/variables.tf Simplified variable from object to string for kinesis_subscription_role_arn
infra/modules/cloudfront/variables.tf Simplified variable from object to string for kinesis_subscription_role_arn
infra/deployments/integration/account/cloudwatch-log-to-splunk.tf Converted counted resources to single resources with moved blocks, uses well-known module
infra/deployments/integration/account/outputs.tf Removed kinesis_destination_arn outputs (but contains bug with [0] index)
infra/deployments/integration/account/inputs.tf Removed kinesis_destination_arn input variables
infra/deployments/integration/tfvars/integration.tfvars Removed hardcoded Kinesis destination ARNs
infra/deployments/integration/review/logging.tf Updated to use well-known module for Kinesis destination ARN
infra/deployments/integration/review/cloudfront.tf Updated module call to pass kinesis_subscription_role_arn directly
infra/deployments/integration/review/alb/waf.tf Updated module call to pass kinesis_subscription_role_arn directly
infra/deployments/forms/account/cloudwatch-log-to-splunk.tf Converted counted resources to single resources with moved blocks, uses well-known module
infra/deployments/forms/account/outputs.tf Removed kinesis_destination_arn outputs (but contains bug with [0] index)
infra/deployments/forms/account/inputs.tf Removed kinesis_destination_arn input variables
infra/deployments/forms/account/tfvars/*.tfvars Removed hardcoded Kinesis destination ARNs from all environment tfvars files
infra/deployments/forms/environment/main.tf Updated module call to pass kinesis_subscription_role_arn directly
infra/deployments/forms/forms-admin/main.tf Updated module call to pass kinesis_subscription_role_arn directly
infra/deployments/forms/forms-product-page/main.tf Updated module call with simplified variable
infra/deployments/forms/forms-runner/main.tf Updated module call with simplified variable
infra/deployments/deploy/tools/log_to_splunk/outputs.tf Removed destination_arn outputs
infra/deployments/deploy/tools/log_to_splunk/kinesis-stream.tf Updated to use well-known module for destination names

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread infra/deployments/forms/account/outputs.tf Outdated
Comment thread infra/deployments/integration/account/outputs.tf Outdated
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 41 out of 41 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/terraform-ci.yml
@whi-tw whi-tw force-pushed the whi-tw/use_well_known_more branch from d0e36a8 to 0e5ba91 Compare November 13, 2025 14:26
@whi-tw whi-tw force-pushed the whi-tw/s3-to-cribl branch from 2209f2e to 762f235 Compare November 13, 2025 14:45
Base automatically changed from whi-tw/s3-to-cribl to main November 13, 2025 14:47
@whi-tw whi-tw force-pushed the whi-tw/use_well_known_more branch from 0e5ba91 to ad427c0 Compare November 17, 2025 09:58
We were passing around a lot of ARNs for the CloudWatch Log Destinations
as tfvars, which were all the same string. Instead, use a well-known
module to create and reference these resources.
@whi-tw whi-tw force-pushed the whi-tw/use_well_known_more branch from ad427c0 to 0b346b0 Compare November 17, 2025 11:05
Copy link
Copy Markdown
Contributor

@sarahseewhy sarahseewhy left a comment

Choose a reason for hiding this comment

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

I'm a bit late to the well-known module so I appreciate I may lack some context.

I'd like to propose a name change to the module to align it with the existing module naming convention (service-oriented nouns): canonical-identifiers or shared-identifiers.

For example: module.canonical-identifiers.kinesis_destination_names["eu-west-2"]

As I understand it, based on the helpful README (thank you!), the purpose of the well-known module is a central catalogue of canonical identifiers (names/ARNs/IDs) that are shared across environments, with lookup via outputs instead of repeated in each environment’s tfvars.

I know a request for this kind of change is a pain and I do apologise -- I'm also happy to chat about it if that would help. Maybe this is a drop in the bucket for having a forms-deploy style guide.

@whi-tw whi-tw enabled auto-merge November 26, 2025 10:31
@whi-tw whi-tw requested a review from sarahseewhy November 26, 2025 10:38
@sarahseewhy
Copy link
Copy Markdown
Contributor

I'm a bit late to the well-known module so I appreciate I may lack some context.

I'd like to propose a name change to the module to align it with the existing module naming convention (service-oriented nouns): canonical-identifiers or shared-identifiers.

For example: module.canonical-identifiers.kinesis_destination_names["eu-west-2"]

As I understand it, based on the helpful README (thank you!), the purpose of the well-known module is a central catalogue of canonical identifiers (names/ARNs/IDs) that are shared across environments, with lookup via outputs instead of repeated in each environment’s tfvars.

I know a request for this kind of change is a pain and I do apologise -- I'm also happy to chat about it if that would help. Maybe this is a drop in the bucket for having a forms-deploy style guide.

We had a really good chat in stand-up about the naming and I retract my proposal. Instead, I'm going to update the directory README to include a section on why we named it well-known in a separate PR. I'll approve this PR now.

Copy link
Copy Markdown
Contributor

@sarahseewhy sarahseewhy left a comment

Choose a reason for hiding this comment

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

Thanks for the extra context in stand-up!

@whi-tw whi-tw added this pull request to the merge queue Nov 26, 2025
Merged via the queue into main with commit b488fdd Nov 26, 2025
29 checks passed
@whi-tw whi-tw deleted the whi-tw/use_well_known_more branch November 26, 2025 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants