Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: Recording strategy #102

Closed
wants to merge 1 commit into from

Conversation

babychm
Copy link

@babychm babychm commented Mar 20, 2024

what

Added ability to conditionally determine the recording strategy, record all types of resources, include or exclude resources from the recording group by the provided list.

module "aws_config" {
  source = "../../../../terraform-aws-config"
 
  enabled = true

  s3_bucket_id  = local.s3_bucket.config_bucket_id
  s3_bucket_arn = local.s3_bucket.config_bucket_arn
  create_iam_role  = local.create_iam_role
  iam_role_arn     = local.config_iam_role_arn
  create_sns_topic = true
  managed_rules    = local.enabled_rules
  global_resource_collector_region   = var.global_resource_collector_region

  strategy_resource_types = ["AWS::EC2::EIP", "AWS::EC2::Instance"]
  enable_exclusion = true
#  enable_inclusion = true

  context = module.this.context
}

why

  • Missing feature: recording strategy for the configuration recorder not been presented before in the module
  • More clean inputs: when you need to record in AWS Config e.g 80 of 100 resources, all you need is enable exclusion strategy and exclude not needed 20 resources

references

@babychm babychm requested review from a team as code owners March 20, 2024 13:55
@babychm babychm requested review from kevcube and Gowiem March 20, 2024 13:55
@mergify mergify bot added the triage Needs triage label Mar 20, 2024
recording_group {
all_supported = true
all_supported = var.enable_exclusion == true || var.enable_inclusion == true ? false : true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified to

Suggested change
all_supported = var.enable_exclusion == true || var.enable_inclusion == true ? false : true
all_supported = !var.enable_exclusion && !var.enable_inclusion

@@ -295,7 +310,7 @@ locals {
enabled = module.this.enabled && !contains(var.disabled_aggregation_regions, data.aws_region.this.name)

is_central_account = var.central_resource_collector_account == data.aws_caller_identity.this.account_id
is_global_recorder_region = var.global_resource_collector_region == data.aws_region.this.name
is_global_recorder_region = var.global_resource_collector_region == data.aws_region.this.name && !(length(var.strategy_resource_types) > 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
is_global_recorder_region = var.global_resource_collector_region == data.aws_region.this.name && !(length(var.strategy_resource_types) > 0)
is_global_recorder_region = var.global_resource_collector_region == data.aws_region.this.name && length(var.strategy_resource_types) == 0

}

# Only if inclusion enabled
resource_types = var.enable_inclusion == true && length(var.strategy_resource_types) > 0 ? var.strategy_resource_types : []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this needs to be in recording_mode block? based on this example

also can be simplified

Suggested change
resource_types = var.enable_inclusion == true && length(var.strategy_resource_types) > 0 ? var.strategy_resource_types : []
resource_types = var.enable_inclusion && length(var.strategy_resource_types) > 0 ? var.strategy_resource_types : []

Comment on lines +28 to +32
for_each = var.enable_exclusion ? [1] : []
content {
resource_types = length(var.strategy_resource_types) > 0 ? var.strategy_resource_types : []
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for_each = var.enable_exclusion ? [1] : []
content {
resource_types = length(var.strategy_resource_types) > 0 ? var.strategy_resource_types : []
}
}
for_each = var.enable_exclusion && length(var.strategy_resource_types) > 0 ? [1] : []
content {
resource_types = var.strategy_resource_types
}
}

all_supported = var.enable_exclusion == true || var.enable_inclusion == true ? false : true

recording_strategy {
use_only = var.enable_exclusion == true ? "EXCLUSION_BY_RESOURCE_TYPES" : (var.enable_inclusion == true ? "INCLUSION_BY_RESOURCE_TYPES" : "ALL_SUPPORTED_RESOURCE_TYPES")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use_only = var.enable_exclusion == true ? "EXCLUSION_BY_RESOURCE_TYPES" : (var.enable_inclusion == true ? "INCLUSION_BY_RESOURCE_TYPES" : "ALL_SUPPORTED_RESOURCE_TYPES")
use_only = var.enable_exclusion ? "EXCLUSION_BY_RESOURCE_TYPES" : (var.enable_inclusion ? "INCLUSION_BY_RESOURCE_TYPES" : "ALL_SUPPORTED_RESOURCE_TYPES")

Copy link

mergify bot commented Oct 31, 2024

💥 This pull request now has conflicts. Could you fix it @babychm? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Oct 31, 2024
@mergify mergify bot closed this Nov 7, 2024
Copy link

mergify bot commented Nov 7, 2024

This PR was closed due to inactivity and merge conflicts. 😭
Please resolve the conflicts and reopen if necessary.

@mergify mergify bot removed conflict This PR has conflicts triage Needs triage labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants