Repository navigation
PLT-1776 : modules support log retention - #624
jasonvinson wants to merge 24 commits into
Conversation
- new shared naming convention for resources shared by multiple apps - use modules/cloudwatch_log_group - remove var.app as this pipeline is cdap managed only - remove references to prod. We want to firm this up as much as possible in test first - add cdap-test-log-retention KMS key references to 900-github-actions-role/config/cdap yml files
- also move tftesting function to numbered service - update 701-log-retention/main.tf to properly parse cloudwatch control messages
| @@ -1,5 +1,27 @@ | |||
| locals { | |||
| cdap_env = contains(["prod", "sandbox"], var.env) ? "prod" : "test" | |||
There was a problem hiding this comment.
is it fair to assume that inheriting the platform module here would be overload?
There was a problem hiding this comment.
Yeah the platform module is a bit heavy in terms of data lookups, so I think it would be overkill here.
| kms_key_id = var.platform.kms_alias_primary.target_key_arn | ||
| skip_destroy = strcontains(local.env, "prod") ? true : false | ||
| retention_in_days = var.log_retention_days | ||
| module "function_logs" { |
There was a problem hiding this comment.
in some ways, this naming is nice because it's descriptive. if you want to keep it consistent, though, so you could do a quick find and replace across all modules, might work to do "log_group" or something or match the module name directly
There was a problem hiding this comment.
Partially agree, though a single uniform name won't work since the service module has two instances (app_logs, datadog_logs).
| service_name = coalesce(var.service_name_override, var.platform.service) | ||
| service_name_full = "${var.platform.app}-${var.platform.env}-${local.service_name}" | ||
|
|
||
| app_log_group_name = "/aws/ecs/fargate/${var.platform.app}-${var.platform.env}/${local.service_name}" |
There was a problem hiding this comment.
Be sure to verify this works with "parent_env" for ephemeral environments, assuming their logs get split up for analysis.
There was a problem hiding this comment.
${var.platform.env} already splits log groups per ephemeral env, so this line should be good. But this surfaced three issues which are now fixed in this PR: the cloudwatch_log_group env validation rejected ephemeral names, the log-retention SSM lookups needed a parent-env suffix match, and the GHA-role log ARN patterns didn't match ephemeral names.
mianava
left a comment
mianava
left a comment
There was a problem hiding this comment.
Some small items to validate, otherwise looks good to me!
This branch has not been deployed
2 participants
🎫 Ticket
https://jira.cms.gov/browse/PLT-1776
🛠 Changes
ℹ️ Context
We are moving to a shared long term log retention strategy to ensure regulatory compliance.
🧪 Validation
Changes were tested by deploying test Function, Cluster, and Service instances to test environment and log delivery to retention bucket was verified.