BCSS-23565 create terraform module WAF#55
Conversation
nhs-oliverslater
added
enhancement
nhs-oliverslater
left a comment
nhs-oliverslater
left a comment
There was a problem hiding this comment.
Thanks for your work on this. As per below comments, theres no need to maintain strict compatibility or even consider the existing legacy code (except for ensuring the module can support the same rule types etc).
If you can please refactor to remove any reference to legacy modules, remove explicit rules in the module, ensure that consumer level rules can be passed in and theres no conflict with rule ids etc.
As opposed to building raw resources directly, can you instead wrap the recommended module with appropriate defaults and overrides, allowing consumer level input to pass in rules/rulegroups. @Pira-nhs has some good examples on what sort of things should be included and there is also a Copilot skill setup called /new-terraform-module that should get you started.
With respect of outside resources i.e. Log groups, SNS topics etc - ideally, these should not be created by THIS module but instead by their respective module and either (give users both options) to create within this module but making a cross module reference, or built at the consumer layer and passed in as an ARN.
Additionally, ensure you store locals in locals.tf, data sources in data.tf etc and have README.md with documentation - examples of this can be found in the acm module for a simple example, moving up to the VPC module (on branch at present) for a more complex example.
There was a problem hiding this comment.
Ruleset should be passed in by consumer, not defined in module
| "aws-waf-logs-${local.derived_name_prefix}" | ||
| ) | ||
|
|
||
| enable_legacy_bcss_mode = var.enable_legacy_bcss_mode != null ? var.enable_legacy_bcss_mode : anytrue([ |
There was a problem hiding this comment.
This isnt needed - we're creating resources afresh - feel free to drop this and write elsewhere to keep this aligned with upstream module inputs only
| enable_splunk_logging_subscription = var.enable_splunk_logging_subscription != null ? var.enable_splunk_logging_subscription : local.enable_legacy_bcss_mode | ||
| enable_shield_ddos_alarming = var.enable_shield_ddos_alarming != null ? var.enable_shield_ddos_alarming : local.enable_legacy_bcss_mode | ||
|
|
||
| waf_ips_secret_name = coalesce( |
There was a problem hiding this comment.
This isnt needed - we're creating resources afresh - feel free to drop this and write elsewhere to keep this aligned with upstream module inputs only
2 participants
Description
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.