Support Organization-wide aggregator across all regions #28
Conversation
osterman
requested review from
mcalhoun
and removed request for
nitrocode and
woz5999
| } | ||
|
|
||
| data "aws_iam_policy" "aws_config_built_in_role_for_organizations" { | ||
| arn = "arn:aws:iam::aws:policy/service-role/AWSConfigRoleForOrganizations" |
There was a problem hiding this comment.
Consider using the aws_partition data source to look up the partition rather than hard-coding aws here.
| dynamic "account_aggregation_source" { | ||
| for_each = var.aggregate_organization_wide ? [] : [1] | ||
| content { | ||
| account_ids = local.child_resource_collector_accounts | ||
| all_regions = true | ||
| } | ||
| } | ||
| dynamic "organization_aggregation_source" { | ||
| for_each = var.aggregate_organization_wide ? [1] : [] | ||
| content { | ||
| all_regions = true | ||
| role_arn = local.create_iam_role ? module.iam_role_organization_aggregator[0].arn : var.iam_role_organization_aggregator_arn | ||
| } | ||
| } |
There was a problem hiding this comment.
As this is a generic module, I'd look for a way to disable aggregation all together if the consumer doesn't want to aggregate. The way your change is currently structured, it requires aggregation, which way not be what is desired.
| module "iam_role_organization_aggregator" { | ||
| count = module.this.enabled && var.aggregate_organization_wide && local.create_iam_role ? 1 : 0 | ||
| source = "cloudposse/iam-role/aws" | ||
| version = "0.9.3" | ||
|
|
||
| principals = { | ||
| "Service" = ["config.amazonaws.com"] | ||
| } | ||
|
|
||
| policy_document_count = 0 | ||
| policy_description = "AWS Config IAM policy for Organization Aggregation" | ||
| role_description = "AWS Config IAM role for Organization Aggregation" | ||
|
|
||
| name = "aggregator" | ||
| use_fullname = true | ||
|
|
||
| attributes = ["config"] | ||
|
|
||
| context = module.this.context | ||
| } | ||
|
|
||
| resource "aws_iam_role_policy_attachment" "config_organization_aggregator_policy_attachment" { | ||
| count = module.this.enabled && var.aggregate_organization_wide && local.create_iam_role ? 1 : 0 | ||
|
|
||
| role = module.iam_role_organization_aggregator[0].name | ||
| policy_arn = data.aws_iam_policy.aws_config_built_in_role_for_organizations.arn | ||
| } |
There was a problem hiding this comment.
I would suggest decoupling the create_iam_role variable to allow the consumer to specify an existing role for either the standard config role or the aggregation role (but have the module create the other).
| description = <<-DOC | ||
| IAM Role the Aggregator uses to read organization data | ||
| DOC |
There was a problem hiding this comment.
When you have a short one-line description, consider just using quotes rather than a HEREDOC.
|
This pull request is now in conflict. Could you fix it @rdkls? 🙏 |
|
@rdkls do you plan to address the comments so that the PR can be merged into master? |
|
Fixed by #85 |
what
why
references