Modernization of SG Account Starter #3
Conversation
wesleykirklandsg
requested review from
rchildress87,
DJKaswa,
jason-drouhard,
sgtursun and
StratusGrid-StephenColeman
| ## Centralized Logging | ||
|
|
||
| This repo is fully configured to allow for centralized logging with S3 and it's controlled via a few variables. To enable centralized logging set the following variables `log_archive_retention`, `aws_org_id`, `s3_destination_bucket_name`, `logging_account_id` to their required values and uncomment this block in `s3-bucket-logging.tf`. | ||
| If the apply file you're doing is for the log archive account these vars should be modified `enable_centralized_logging`, `log_archive_account` in addition to the prior variables with the proper values set. |
There was a problem hiding this comment.
What typo? I can't find anything.
|
|
||
| ## StratusGrid Standards we assume | ||
|
|
||
| - All resource names and name tags shall use `_` and not `-`s |
There was a problem hiding this comment.
Are you sure you want to use underscores in name tags? I believe AWS API uses hyphens or camel case when generating these sorts of things and no value is provided . I've typically used underscores for all HCL and then used hyphens for the names and name tags.
There was a problem hiding this comment.
This doc states that AWS will basically let you do anything in the tag. From what I've learned over the years though the _ is the common replacement character for spaces. Most Tags would be PascalCase.
https://docs.aws.amazon.com/mediaconnect/latest/ug/tagging-restrictions.html
| /* source = "StratusGrid/cloudtrail/aws" | ||
| version = "~> 3.0" */ | ||
|
|
||
| source = "github.com/StratusGrid/terraform-aws-cloudtrail.git?ref=aws4.x" |
There was a problem hiding this comment.
Why are you using github instead of a Registry release?
There was a problem hiding this comment.
Because I screwed up See here
| providers = { | ||
| aws = aws.us-east-1 | ||
| } | ||
|
|
||
| # Uncomment the below if you wish to enable centralized logging to S3 |
There was a problem hiding this comment.
Have you considered using conditional expressions with null instead of comment?
There was a problem hiding this comment.
I did and we talked about this. The problem with it is the mass amount of code duplication doesn't make sense, and modules don't take dynamics. I'm welcome to other ideas though.
There was a problem hiding this comment.
Ok I see what you mean now, I tried it and it won't work due to a count statement on the backend.
iam_role_s3_replication_arn needs to evaluate and is indexed and TF is trying to validate the index even though it won't be used. This is a product limitation.
enable_centralized_logging = var.enable_centralized_logging == true ? true : false
s3_destination_bucket_name = var.enable_centralized_logging == true ? var.s3_destination_bucket_name : ""
iam_role_s3_replication_arn = var.enable_centralized_logging == false ? "" : module.iam_role_s3[0].iam_role_arn
logging_account_id = var.enable_centralized_logging == true ? var.logging_account_id : ""
See https://stratusgrid.atlassian.net/browse/SSA-42