Repository navigation
DND-1537: export RDS error/slowquery logs to CloudWatch, with retention - #69
Conversation
A fleet audit for DND-1537 found EnabledCloudwatchLogsExports null on all 16 STSaaS Aurora clusters. The module never had the attribute, so there was no per-env knob to set — this was not an oversight per environment. That gap blocked the CUST-6816 post-mortem: agentro is granted rds:DescribeDBLogFiles but not rds:DownloadDBLogFilePortion, so the investigation could see that the reader's error log was 254 KB in the failure hour against a ~100 KB/hour baseline, and could not read a byte of it. Exporting to CloudWatch Logs closes that without widening the read-only role. Adds: rds_enabled_cloudwatch_logs_exports default ["error", "slowquery"] rds_log_retention_days default 90 The log groups are created explicitly rather than left to RDS. RDS auto-creates /aws/rds/cluster/<id>/<type> at "never expire" the instant an export is enabled — that is how the pre-rebuild zoox slowquery group came to hold 3.65 GB of dead data indefinitely, and it would leave 16 clusters' worth of groups unmanaged and needing import. depends_on makes the ordering explicit so the groups exist, with retention, before the export switches on. Measured cost across the whole fleet: 0.093 GB/day of raw log generation, so ~$1.39/month ingestion and ~$1.47/month with 90-day retention. Note "slowquery" produces nothing until slow_query_log=1 is also set via rds_cluster_parameters, which is off on 15 of 16 clusters today. It is enabled here anyway so the group exists and is retention-managed from the moment that parameter is turned on. Enabling the parameter itself is deliberately not part of this change: the slow query log records full SQL text including literals, so it needs a data-governance decision for the regulated single-tenant customers first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-ups on this PR. skip_destroy on the log groups. Dropping a type from rds_enabled_cloudwatch_logs_exports removes it from the for_each, which would otherwise destroy the group and every log in it — turning an export off must not delete the evidence already collected. The cost is that a real teardown leaves the groups behind, but they carry retention now and expire on their own, unlike the never-expire orphan DND-1537 had to clean up. Root-level validation on both variables. The inner module already validated them, so this is not a behaviour change; it surfaces a bad value at the env config rather than one layer deeper. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| resource "aws_cloudwatch_log_group" "rds_exported_logs" { | ||
| for_each = toset(var.rds_enabled_cloudwatch_logs_exports) | ||
|
|
||
| name = "/aws/rds/cluster/${local.rds_cluster_identifier}/${each.value}" | ||
| retention_in_days = var.rds_log_retention_days == 0 ? null : var.rds_log_retention_days | ||
|
|
||
| # Dropping a type from rds_enabled_cloudwatch_logs_exports removes it from the for_each, | ||
| # which would otherwise destroy the group and every log in it. Turning an export off must | ||
| # not delete the evidence already collected — that is the whole point of exporting. | ||
| # The cost is that a real teardown leaves the groups behind, but they carry retention now | ||
| # and expire on their own, unlike the never-expire orphan DND-1537 had to clean up. | ||
| skip_destroy = true |
There was a problem hiding this comment.
aws_cloudwatch_log_group.rds_exported_logs's for_each attempts to create /aws/rds/cluster/<id>/<type> groups that RDS may already own, so CreateLogGroup fails with ResourceAlreadyExistsException and blocks apply; with skip_destroy = true, removing and re-adding an export key repeats the conflict — should we add an adoption migration using exact terraform import addresses or make re-adoption lifecycle-safe?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
modules/comet_rds/main.tf around lines 80-91, the
`aws_cloudwatch_log_group.rds_exported_logs` `for_each` resource unconditionally tries
to create `/aws/rds/cluster/<id>/<type>` log groups, which fails with
`ResourceAlreadyExistsException` when RDS has already created them (as is the case for
already-enabled clusters like zoox), and also fails when an export is disabled then
re-enabled, since `skip_destroy = true` retains the AWS group while Terraform removes it
from state. Add a state-adoption/import mechanism for existing groups — such as
Terraform import blocks or documented `terraform import` commands using the exact
`for_each` addresses — so existing groups are adopted rather than recreated, while
still allowing normal creation for groups that don't exist yet. Ensure the fix supports
the disable/re-enable cycle without manual intervention and doesn't unconditionally
import groups that are genuinely missing.
There was a problem hiding this comment.
Commit b23500 addressed this comment by decoupling log-group management from export toggles, so disabling and re-enabling an export retains the resource in Terraform state and avoids recreation conflicts. It does not add adoption/import handling for groups that already exist outside Terraform.
| variable "rds_log_retention_days" { | ||
| description = "Retention for the RDS CloudWatch log groups, in days. 0 means keep forever — avoid: that is what RDS applies on its own when it auto-creates the groups, and the reason DND-1537 found an orphaned group holding 3.65 GB of dead data indefinitely." | ||
| type = number | ||
| default = 90 | ||
|
|
||
| validation { | ||
| condition = contains([0, 1, 3, 5, 7, 14, 30, 60, 90, 120, 150, 180, 365, 400, 545, 731, 1096, 1827, 2192, 2557, 2922, 3288, 3653], var.rds_log_retention_days) | ||
| error_message = "Must be a retention period CloudWatch Logs accepts (0, 1, 3, 5, 7, 14, 30, 60, 90, 120, 150, 180, 365, 400, 545, 731, 1096, 1827, 2192, 2557, 2922, 3288, 3653)." |
There was a problem hiding this comment.
rds_log_retention_days accepts 0, which modules/comet_rds/main.tf converts to retention_in_days = null, so enabled-by-default slowquery and other exported database logs lack a CloudWatch retention policy and can retain SQL text indefinitely — should we reject 0 for sensitive exports or require a finite, centrally approved retention period?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In `variables.tf`
around lines 1187-1194, update the `rds_log_retention_days` validation so `0` cannot be
configured when CloudWatch database log exports (especially `slowquery`) are enabled,
preferably by removing the zero-retention sentinel and requiring a finite supported
retention period, or by adding a conditional validation tied to
`rds_enabled_cloudwatch_logs_exports` with a clear error message. Align the variable
description and error message with this policy, and ensure the downstream conversion to
`retention_in_days = null` in `modules/comet_rds/main.tf` cannot produce indefinitely
retained logs for `error`, `slowquery`, or other exports.
There was a problem hiding this comment.
Commit b23500b addressed this comment by removing 0 from the allowed retention values and explicitly requiring a finite CloudWatch retention period. It also creates managed log groups with that retention before enabling RDS exports.
There was a problem hiding this comment.
Cloned the branch and ran terraform init / validate against hashicorp/aws v6.60.0 — the AWS provider, not a module tag — plus a schema and repo-convention check. That's what this repo's unbounded >= 6.52 floor resolves to today, and it matches what comet-devops (~> 6.50, != 6.57.0) resolves to on the Atlantis path, so it's the version migrated envs would actually apply this under.
Verdict: sound change, correct implementation, unusually well-justified — the measured cost figure and the live zoox verification make this easy to review. Two findings I'd like a response on before merge; neither is a correctness bug, but #2 gets baked into 16 environments if it ships as-is.
Verified
terraform validatepasses on the branch. The only warnings are the pre-existingname/regiondeprecations inside vendored upstream modules (terraform-aws-modules/eks) — nothing from this PR.skip_destroyis not deprecated onaws_cloudwatch_log_groupin AWS provider 6.60 (checkedterraform providers schemadirectly — I'd half-expected it to be). Safe against thehashicorp/aws >= 6.52floor inversions.tf./aws/rds/cluster/<id>/<type>is the correct Aurora MySQL group shape, and hoistingrds_cluster_identifierinto a local is the right fix for the name-drift risk. That's the detail most implementations of this get wrong.- The log-type validation set (
audit/error/general/slowquery) is correct for Aurora MySQL, with no Postgres false-reject risk in practice — the module is MySQL-only (hardcoded port 3306,aurora-mysql${version}parameter-group family). retention_in_days = 0 ? nullis right; the provider treats both as never-expire andnullis the cleaner spelling. Retention values list matches CloudWatch's accepted set exactly.depends_oncreates no cycle, since the group name derives from the local rather than from the cluster resource.- Root gating is fine —
count = var.enable_rds ? 1 : 0(main.tf:402) means RDS-less envs create no groups.
1. skip_destroy makes disabling an export a one-way door
modules/comet_rds/main.tf:91
Dropping a type from rds_enabled_cloudwatch_logs_exports removes it from the for_each — gone from state, still in AWS. Re-enabling it later fails on ResourceAlreadyExistsException. That's the same collision this PR exists to prevent, just deferred and self-inflicted.
The root cause is that one variable drives two lifecycles. Decoupling them fixes it cleanly: for_each the groups over a separate rds_managed_log_group_types (default ["error", "slowquery"]), and let rds_enabled_cloudwatch_logs_exports control only the cluster attribute. Then toggling an export off never removes a group from state, skip_destroy becomes belt-and-braces rather than load-bearing, and re-enabling is a no-op.
If you'd rather not add a variable, the minimum is documenting the re-enable import in the variable description alongside the disable caveat that's already there.
2. No kms_key_id on the log groups — the one I'd most want in this PR
The cluster takes rds_kms_key_id and Performance Insights takes rds_performance_insights_kms_key_id; the new groups have no equivalent knob, so they land on the CloudWatch service-managed key.
The PR's own argument is that the slow query log carries customer SQL text — workspace names, project names, user-supplied literals — and needs a data-governance decision for the regulated single-tenant customers. CloudWatch encryption is exactly where that decision lands. Adding rds_log_kms_key_id (default null, no behaviour change) now is cheap; retrofitting it after this reaches all 16 clusters means touching every env a second time, right at the moment DND-1537 flips slow_query_log=1.
Trivy will also flag this (AVD-AWS-0017). Report-only today, but ci.yml states the intent to make it blocking once the existing findings are triaged — no reason to add to that backlog.
3. No outputs for the new groups
modules/comet_rds/outputs.tf exports endpoints and cluster id but not the log group names or ARNs. The rollout explicitly needs those names for terraform import on every cluster enabled out-of-band, and alarms / metric filters will want the ARNs. mysql_log_group_names / mysql_log_group_arns would make the import command derivable rather than hand-assembled up to fifteen times.
4. skip_destroy survives a real teardown, and nothing tracks the orphans
Acknowledged in the description, and retention bounds the damage — unlike the never-expire orphan DND-1537 had to clean up. But STSaaS offboarding is a live workflow, so this is worth a line in the decommission checklist rather than only in a PR body.
Nits
- The README Inputs table isn't regenerated. It's terraform-docs-shaped but already covers 55 of 245 variables, and #63/#64/#65 didn't update it either — so this matches convention and isn't a blocker. Separately: there's no
terraform_docshook in.pre-commit-config.yaml, so that table drifts unchecked, which is arguably worse than having no table. - Root and module
validationblocks are byte-identical duplicates. Deliberate per the commit message and fine — just noting it as a drift point if CloudWatch ever adds a retention value. - Cost math holds: ingestion is 0.093 GB/day × 30 × $0.50 ≈ $1.40, and the $0.07 storage delta implies the compressed-archive billing basis. Right order of magnitude either way.
One addition to the rollout note
Enabling enabled_cloudwatch_logs_exports is an in-place cluster modify — no reboot, no downtime. With apply_immediately = true already set (modules/comet_rds/main.tf:123), envs adopting this take it live on apply. Worth stating explicitly so nobody schedules a maintenance window they don't need.
…+ outputs Review findings from @jms200 on #69. 1. skip_destroy made disabling an export a one-way door. Dropping a type from rds_enabled_cloudwatch_logs_exports removed it from the for_each — gone from state, still in AWS — so re-enabling it later would fail on ResourceAlreadyExistsException. That is the same collision this change exists to prevent, deferred and self-inflicted, and it was introduced by the skip_destroy fix earlier in this PR rather than being in the original design. Fixed as suggested, by splitting the two lifecycles: the log groups now for_each over a new rds_managed_log_group_types, and rds_enabled_cloudwatch_logs_exports controls only the cluster attribute. Toggling an export is now purely a cluster modify and never touches group state. A type managed but not exported yields an empty group with retention already applied, which is exactly the desired state for slowquery on the 15 clusters where slow_query_log=0. skip_destroy stays as belt-and-braces. 2. Added rds_log_kms_key_id (default null, no behaviour change). The cluster and Performance Insights both take a key and the groups did not. Cheap now; after this reaches 16 clusters it means touching every env a second time, at exactly the moment slow_query_log=1 starts putting customer SQL text in these groups. Also what trivy AVD-AWS-0017 asks for. 3. Added mysql_log_group_names / mysql_log_group_arns outputs, at module and root. The rollout needs the names to build terraform import addresses for every cluster enabled out-of-band, and alarms and metric filters will want the ARNs. Also, from the Baz review: rds_log_retention_days no longer accepts 0. Never-expire is what RDS applies on its own and the reason DND-1537 found a 3.65 GB orphan; a module whose purpose is preventing that should not offer it. These groups carry customer SQL text once slow_query_log is on. 3653 days remains available. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks — findings 1–3 addressed in 1.
|
|
Thanks for the detailed update and rationale. I can only save feedback to memory for specific code review findings, not general PR-level feedback. |
There was a problem hiding this comment.
Approving. Re-checked b23500b on a fresh clone rather than reading the summary: terraform fmt -check -recursive clean, terraform validate passes (only the pre-existing upstream name/region deprecations), all four CI jobs green.
All three findings are actually implemented, and the decoupling is the right shape:
- 1 — one-way door.
for_eachnow runs offrds_managed_log_group_types, the cluster attribute offrds_enabled_cloudwatch_logs_exports. Toggling an export is a pure cluster modify that never touches group state, so re-enabling is a no-op. Your point about the managed-but-not-exported case is the better half of this:slowqueryon the 15 clusters atslow_query_log=0now sits there as an empty group with retention already applied, which is exactly the state DND-1537's follow-up wants to walk into. - 2 — KMS.
rds_log_kms_key_id, defaultnull. No behaviour change, knob exists before it reaches 16 clusters. - 3 — outputs. Keyed by log type at both levels, root gated on
enable_rds. Import address is derivable now.
Good catch removing the dead == 0 ? null ternary along with 0 from the validation set — those had to move together, and it would have been easy to drop one and leave the other.
No objection to the two you pushed back on. Root-level validation failing at the env config rather than inside an Atlantis plan frame is worth the duplication, and hand-regenerating a terraform-docs table with no hook to keep it honest just drifts again — the missing hook is the real fix.
Two non-blocking notes for whoever picks up DND-1549, neither worth another round here:
Nothing enforces rds_enabled_cloudwatch_logs_exports ⊆ rds_managed_log_group_types. Both descriptions say "should be a superset" and both default to the same list, so it's correct out of the box — but setting exports to include, say, general without adding it to the managed list puts RDS back in charge of creating that group, at never-expire. That's this PR's own failure mode reintroduced through a new door, and by the standard you applied to retention_in_days = 0 it's the kind of thing worth making unreachable rather than documenting.
Cross-variable references in validation blocks need Terraform 1.9, above this repo's >= 1.5.7 floor. A lifecycle.precondition on aws_rds_cluster does the same job at the current floor:
lifecycle {
precondition {
condition = alltrue([
for t in var.rds_enabled_cloudwatch_logs_exports :
contains(var.rds_managed_log_group_types, t)
])
error_message = "Every exported log type must also be in rds_managed_log_group_types, or RDS creates its log group at never-expire."
}
}KMS key policy. When someone first sets rds_log_kms_key_id, the key policy has to grant logs.<region>.amazonaws.com — otherwise CreateLogGroup fails with InvalidParameterException, which reads like a terraform problem and isn't. Default null means nobody hits it today; worth a line in the variable description before the first regulated env turns it on.
| output "mysql_log_group_names" { | ||
| description = "CloudWatch log group names for the exported RDS logs, keyed by log type. Use when adopting a cluster whose export was enabled out-of-band: terraform import 'module.<path>.aws_cloudwatch_log_group.rds_exported_logs[\"error\"]' <name>" | ||
| value = { for t, lg in aws_cloudwatch_log_group.rds_exported_logs : t => lg.name } |
There was a problem hiding this comment.
mysql_log_group_names/mysql_log_group_arns derive from aws_cloudwatch_log_group.rds_exported_logs keyed by rds_managed_log_group_types, so groups like slowquery appear as active exports when slow_query_log is off. Should we filter them by rds_enabled_cloudwatch_logs_exports, or rename them as managed-group lists and expose a separate active-export list for metric filters, subscriptions, and alarms?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`modules/comet_rds/outputs.tf` around lines 35-42, `mysql_log_group_names` and
`mysql_log_group_arns` currently expose every `rds_managed_log_group_types` entry from
`aws_cloudwatch_log_group.rds_exported_logs`, including groups not enabled for export
(e.g. `slowquery` when `slow_query_log` is off). Either refactor these outputs to
build/filter an active-export map keyed by `rds_enabled_cloudwatch_logs_exports`, or
rename the `rds_exported_logs` resource/address and these outputs to managed-log-group
terminology (updating descriptions and any ARN output accordingly) and provide the
active export list as a separate output for metric filters, subscriptions, and alarms.
| } | ||
|
|
||
| variable "rds_managed_log_group_types" { | ||
| description = "MySQL log types whose CloudWatch log groups are created and retention-managed here. Kept separate from rds_enabled_cloudwatch_logs_exports so that disabling an export is not a one-way door: the group stays in state, and re-enabling later is a no-op instead of a ResourceAlreadyExistsException. Should be a superset of the export list." |
There was a problem hiding this comment.
Exported logs bypass retention management
rds_enabled_cloudwatch_logs_exports = ["audit"] is accepted even when rds_managed_log_group_types excludes audit, so RDS can create the group with Never Expire retention and bypass this module’s retention and KMS configuration — should we require every enabled export in rds_enabled_cloudwatch_logs_exports to be contained in var.rds_managed_log_group_types, including in the child module?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In `variables.tf`
around lines 1188-1194, update the
`rds_managed_log_group_types`/`rds_enabled_cloudwatch_logs_exports` validation logic to
require every enabled export to be present in the managed log-group list, preventing RDS
from auto-creating unmanaged groups. Apply the equivalent cross-variable validation in
the child module’s corresponding variable configuration, with a clear error message
identifying the missing managed log type.
There was a problem hiding this comment.
Commit 59e3a18 addressed this comment by adding a child-module precondition requiring every enabled export to appear in rds_managed_log_group_types, preventing unmanaged RDS-created log groups.
…policy Both non-blocking notes from @jms200's approving review. Cheap enough to take now rather than leave for DND-1549. Exporting a type with no managed log group hands group creation back to RDS, which makes it at never-expire — this module's own failure mode through a new door. Both variables default to the same list so it is correct out of the box, but by the same standard applied to retention 0, it should be unreachable rather than documented. Implemented as a lifecycle.precondition rather than a cross-variable validation block: the latter needs Terraform 1.9 and this repo's floor is >= 1.5.7. Also documents that a KMS key policy must grant logs.<region>.amazonaws.com before rds_log_kms_key_id can be set, since CreateLogGroup otherwise fails with InvalidParameterException, which reads like a terraform fault and is not. Default null means nobody hits this today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for the approval. Took both non-blocking notes anyway in Subset enforcement. You're right that documenting it was inconsistent with removing KMS key policy. Added to both variable descriptions. Worth having written down precisely because
Leaving the merge to you, since DND-1549 sequencing (migrate vs. enable-directly, and the zoox import) is the decision that determines when any of this actually reaches a cluster. |
…ypes Baz finding on b23500b, and a genuine defect I introduced: the outputs were added when the log groups were keyed by the export list, and their descriptions still said "for the exported RDS logs". Decoupling the two lifecycles made that false — they are keyed by rds_managed_log_group_types now, so `slowquery` appears in them on all 15 clusters where slow_query_log=0 and nothing is written. Keeping them keyed by managed types is correct for their primary purpose: import needs every group under management, including an empty one. So the fix is to say so, and to give consumers the filter they actually need — a metric filter or alarm attached to the slowquery group today would sit on a group nothing writes to. Adds mysql_exported_log_types (the cluster's live enabled_cloudwatch_logs_exports) at module and root, and rewords both existing outputs to state that they are a superset. Note a type can be exported and still produce nothing: slowquery stays empty until slow_query_log=1 is set via rds_cluster_parameters, which is DND-1549's problem 2. The output cannot express that, so both descriptions say it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Baz findings — statusFour raised, all now resolved. Two Baz confirmed itself, one is deliberate, one was a real defect I'd introduced.
3 — real defect, and mineBaz is right, and it was a regression from my own decoupling fix. The outputs were written when the groups were keyed by the export list, and their descriptions still said "for the exported RDS logs". Once Took the "rename + separate list" branch rather than filtering, because filtering would break the outputs' primary purpose —
Worth noting the residual sharp edge the outputs can't express: a type can be exported and still produce nothing. 1 — adoption stays deliberateBaz's follow-up is accurate: Only zoox is affected today, and only because it was enabled out-of-band this afternoon. CI green on |
|
@jms200 re-review requested — your approval was dismissed by two pushes after it, and that's my error. I checked Two commits landed after your approval, both narrow:
Diff since your approval if it's easier than a full re-read: |
| type = list(string) | ||
| default = ["error", "slowquery"] | ||
|
|
||
| validation { | ||
| condition = alltrue([for t in var.rds_enabled_cloudwatch_logs_exports : contains(["audit", "error", "general", "slowquery"], t)]) |
There was a problem hiding this comment.
Direct module null input crashes
var.rds_enabled_cloudwatch_logs_exports and var.rds_managed_log_group_types remain nullable, so callers can pass null and validation or resource expansion fails before the resource precondition emits a controlled diagnostic — should we set nullable = false or normalize both inputs to [] before validation and use?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
modules/comet_rds/variables.tf around lines 173-177, update
rds_enabled_cloudwatch_logs_exports so callers cannot pass null and trigger a failure
while its validation iterates the value. Apply the same nullable=false boundary to
rds_managed_log_group_types around lines 193-197, preventing its validation and
downstream toset/resource expansion from receiving null. Preserve the existing list
defaults and validation behavior, and update any relevant tests or documentation if
needed.
| variable "rds_managed_log_group_types" { | ||
| description = "MySQL log types whose CloudWatch log groups this module creates and manages retention for. Should be a superset of rds_enabled_cloudwatch_logs_exports — a type listed here but not exported simply yields an empty group with retention already applied, ready for when the export (or slow_query_log) is switched on. Removing a type here stops managing its group; it is NOT deleted, because of skip_destroy." | ||
| type = list(string) | ||
| default = ["error", "slowquery"] |
There was a problem hiding this comment.
Removing a type from rds_managed_log_group_types or setting enable_rds = false removes its for_each instance from Terraform state, while skip_destroy retains /aws/rds/cluster/<id>/<type> in AWS, so re-adding it fails on CreateLogGroup with ResourceAlreadyExistsException — should we provide an adoption/import path or stable resource addressing instead of relying on manual import?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`modules/comet_rds/variables.tf` around lines 191-194, and in the
`aws_cloudwatch_log_group.rds_exported_logs` resource that consumes
`rds_managed_log_group_types`, fix the lifecycle mismatch where removing (or re-adding)
a type, or setting `enable_rds = false`, drops the Terraform state entry while
`skip_destroy` leaves the AWS log group orphaned. Refactor so remove/re-add cycles are
safe—either retain the resource state with an explicit protected-removal workflow, use
a stable non-shrinking set of resource addresses, or require an explicit state/import
migration—then update the variable description and adoption guidance to match the
chosen behavior. Add or update coverage for remove/re-add and `enable_rds = false`
scenarios so they cannot fail with `ResourceAlreadyExistsException`.
| variable "rds_enabled_cloudwatch_logs_exports" { | ||
| description = "MySQL log types the CLUSTER exports to CloudWatch Logs. 'error' carries the entries that matter for post-mortems. 'slowquery' produces nothing unless slow_query_log=1 is also set via rds_cluster_parameters — it is enabled here so the log group exists and is retention-managed from the moment that parameter is turned on. Pass [] to stop exporting; the log groups are governed separately by rds_managed_log_group_types, so disabling an export never removes a group from state." | ||
| type = list(string) | ||
| default = ["error", "slowquery"] |
There was a problem hiding this comment.
Unapproved SQL export enabled by default
The default rds_enabled_cloudwatch_logs_exports enables slowquery for every RDS deployment, and arbitrary rds_cluster_parameters name/value pairs can make slow_query_log=1 effective, so customer SQL text reaches CloudWatch without any data-governance approval or authorized-consumer gate — should we require explicit approval for slow_query_log or drop slowquery from the default until that gate exists?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In `variables.tf`
around lines 1176-1179, update the `rds_enabled_cloudwatch_logs_exports` default so
`slowquery` is not enabled for every deployment. Because `rds_cluster_parameters` can
enable `slow_query_log` through arbitrary name/value pairs, require an explicit,
documented opt-in or approval gate before allowing `slowquery` export; otherwise default
to `['error']` and retain `slowquery` only when intentionally configured.
| variable "rds_log_retention_days" { | ||
| description = "Retention for the RDS CloudWatch log groups, in days. Must be finite — 'never expire' is intentionally not offered: that is what RDS applies when it creates the groups itself, and the reason DND-1537 found an orphan holding 3.65 GB of dead data indefinitely." | ||
| type = number | ||
| default = 90 |
There was a problem hiding this comment.
Historical RDS logs are deleted after adoption
The new root default applies retention_in_days = 90 to imported pre-existing /aws/rds/cluster/<id>/<type> groups, so CloudWatch Logs permanently deletes post-mortem events older than 90 days, including those predating adoption. Should the migration require an explicit retention/archive decision and document consumer/customer commitments before applying 90 days to existing groups?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In `variables.tf`
around lines 1204-1207, update the `rds_log_retention_days` configuration and its
downstream managed-log-group logic so existing CloudWatch log groups are not silently
changed to 90-day retention. Require an explicit retention/archive opt-in for applying
retention to pre-existing groups, and when unset, preserve their current retention
rather than issuing a destructive update; adjust validation and document the migration
decision required before enabling it.
jms200
left a comment
jms200
left a comment
There was a problem hiding this comment.
Re-approving. Checked 59e3a18 and 9adec86 on a fresh clone: fmt -check -recursive clean, validate passes with only the pre-existing upstream warnings, all four CI jobs green on 9adec86.
No concerns about the dismissal — that was the ruleset doing its job, and both commits are narrow.
59e3a18. Precondition sits in the existing lifecycle block on aws_rds_cluster and the condition is right. Worth noting the property that makes it the right mechanism rather than just an available one: it references only input variables, so Terraform evaluates it at plan time. A bad export list fails the plan outright instead of surfacing partway through an apply with some groups already created. That's the behaviour a cross-variable validation block would have given, without the 1.9 floor.
KMS caveat reads well in both descriptions.
9adec86. Baz was right and your diagnosis of it is right — after the decoupling, outputs keyed by rds_managed_log_group_types while describing themselves as "the exported RDS logs" were straightforwardly false, and an alarm wired off mysql_log_group_arns would have targeted a slowquery group nothing writes to on 15 clusters.
Keeping them keyed by managed types and adding the export list alongside is the correct branch. Filtering would have fixed the alarm case by breaking the import case, which is the outputs' primary reason to exist. And mysql_exported_log_types reading aws_rds_cluster.cometml-db-cluster.enabled_cloudwatch_logs_exports rather than the input variable is the detail that makes it trustworthy — it reflects what the cluster actually carries, so it stays honest if the two ever drift.
Calling out the exported-but-empty case explicitly in the description is the right call too. That one can't be expressed structurally — slowquery is exported on zoox today and its group doesn't exist — so documenting it is the only option, and it's the thing most likely to waste someone's afternoon.
Agreed on leaving finding 1 as deliberate: import blocks need a per-env cluster identifier, so it isn't module-shaped. Derivable addresses plus an acceptance criterion on DND-1549 is the right split.
Nothing further from me. Merge sequencing is yours — as you say, DND-1549 decides when this reaches a cluster, and the zoox import has to land with it.
2 participants
User description
Why
A fleet audit for DND-1537 found
EnabledCloudwatchLogsExportsnull on all 16 STSaaS Aurora clusters. The module never had the attribute at all, so there was no per-env knob to set — this was not a per-environment oversight.That gap blocked the CUST-6816 post-mortem.
agentrois grantedrds:DescribeDBLogFilesbut notrds:DownloadDBLogFilePortion, so the investigation could see that the reader's error log was 254 KB in the failure hour against a ~100 KB/hour baseline — and could not read a byte of it. Exporting to CloudWatch Logs closes that without widening the read-only IAM role, which was the alternative considered and rejected.What
rds_enabled_cloudwatch_logs_exports["error", "slowquery"]rds_log_retention_days90Wired into
aws_rds_cluster.enabled_cloudwatch_logs_exports, plus anaws_cloudwatch_log_groupper exported type.Why the log groups are created here rather than left to RDS
RDS auto-creates
/aws/rds/cluster/<id>/<type>at "never expire" the instant an export is enabled — verified live while enabling this on zoox. Two consequences:slowquerygroup came to hold 3.65 GB of dead data indefinitely (last event 2026-04-01, deleted under DND-1537).depends_onmakes the ordering explicit: groups exist, with retention, before the export switches on.Cost
Measured, not estimated —
describe-db-log-filesreports raw sizes, which is what CloudWatch bills ingestion on. Across all 30 instances over a 30-day retained window:Note on
slowqueryIt produces nothing until
slow_query_log=1is also set viards_cluster_parameters, which is off on 15 of 16 clusters today (bmw is the exception, atlong_query_time=5). The export is enabled anyway so the group exists and is retention-managed from the moment that parameter is turned on.Enabling the parameter is deliberately not in this PR: the slow query log records full SQL text including literals — workspace and project names, IDs, user-supplied strings — so it needs a data-governance decision for the regulated single-tenant customers before that text is shipped into CloudWatch. Tracked separately on DND-1537.
Testing
terraform fmt -check -recursive— cleanterraform validate— passes (remaining warnings are pre-existingname/regiondeprecations)Rollout note
Envs are pinned v1.20.6–v2.1.0 against
mainat v5.6.0, so this reaches new environments on the next release; existing clusters need either a module migration or direct enablement. zoox has already been enabled directly and is confirmed working —errorlog content is now readable, and RDS backfilled ~2 weeks of history on enablement.🤖 Generated with Claude Code
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Enable Aurora MySQL error and slow-query log exports to CloudWatch Logs through the RDS module, creating retention-managed log groups before RDS begins exporting. Expose configurable export types, retention, KMS encryption, cluster identifiers, and log group names/ARNs for operations and adoption.
Modified files (2)
Latest Contributors(2)
Modified files (4)
Latest Contributors(2)