added recording_mode{} attribute

[AdamTylerLynch]

what

Added recording_mode block.

Requesting maintainer guidance on properly defining the inputs as a practitioner would expect. The way it is defined now feels odd, requiring a variable assignment and then a list for recording_mode_override.

Example:

##---------------------------------------------------
## AWS Config to monitor compliance
##---------------------------------------------------
module "config" {
  source    = "cloudposse/terraform-aws-config/aws"
  name      = "${local.name}-config-${data.aws_caller_identity.current.account_id}"
  namespace = local.namespace

  s3_bucket_id                     = module.log_storage.bucket_id
  s3_bucket_arn                    = module.log_storage.bucket_arn
  global_resource_collector_region = data.aws_region.current.name

  create_iam_role = true

  recording_mode = {
    recording_frequency = "DAILY"
    recording_mode_override = {
      description         = "Override for specific resource types"
      recording_frequency = "CONTINUOUS"
      resource_types      = ["AWS::EC2::Instance"]
    }
  }
}

why

This feature allows for cost optimization. Adds the ability to leverage Periodic recording VS continious.

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/config_configuration_recorder#periodic-recording
• https://aws.amazon.com/about-aws/whats-new/2023/11/aws-config-periodic-recording/
• https://github.com/hashicorp/terraform-provider-aws/releases/tag/v5.38.0

[Gowiem]

Looking good, but I think we should change that type def unless I'm mistaken.

One other thing: We'll need to require the latest provider to support this, so we'll need to change to version = ">= 5.38" here.

Check out those updates and let me know your thoughts! Once you've got it into a solid spot where it's ready for another review, please do the following locally, adding + committing the result, and push to your branch:

make init
make readme

Thanks!

[Gowiem]

Provider docs seem to uppercase these, so just in case that is an issue..

Suggested change

	- Continuous
	- Daily
	- CONTINUOUS
	- DAILY

[Gowiem]

AFAICT, it seems there can only be one recording_mode_override block, yeah? Considering that, I'd just make this a single option object like so:

Suggested change

	recording_mode_override = optional(list(object({
	description = string
	recording_frequency = string
	resource_types = list(string)
	})))
	recording_mode_override = optional(object({
	description = string
	recording_frequency = string
	resource_types = list(string)
	}))

This way we don't need to worry about list access and can avoid that data type. Or am I missing something on that?

[AdamTylerLynch]

The AWS API has a list, which means it is meant for future expansion. Whomever designed the Terraform resource did not realize that and just coded a structure.

We can align to the Terraform resource, knowing that in the future this could be an issue.

[Gowiem]

We could create a validation block for these values in this variable, but not required.

[AdamTylerLynch]

Adding validation client side means we need to publish new versions as the API changes. My preference is to provide clear examples, optional attributes, and let the APIs do the validation. But, I am open to adding it if you feel it's necessary.

[AdamTylerLynch]

Output post apply:

Terraform will perform the following actions:

  # module.config.aws_config_configuration_recorder.recorder[0] will be updated in-place
  ~ resource "aws_config_configuration_recorder" "recorder" {
        id       = "ga-account-guardian-config-123456789123-config"
        name     = "ga-account-guardian-config-123456789123-config"
        # (1 unchanged attribute hidden)

      ~ recording_mode {
            # (1 unchanged attribute hidden)

          + recording_mode_override {
              + description         = "Override for specific resource types"
              + recording_frequency = "CONTINUOUS"
              + resource_types      = [
                  + "AWS::EC2::Instance",
                ]
            }
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
module.config.aws_config_configuration_recorder.recorder[0]: Modifying... [id=ga-account-guardian-config-123456789123-config]
module.config.aws_config_configuration_recorder.recorder[0]: Modifications complete after 0s [id=ga-account-guardian-config-123456789123-config]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

Outputs:

[Gowiem]

/terratest

[Gowiem]

@AdamTylerLynch Tests are failing elsewhere (#86), so we're working on that internally and will circle back once they're fixed 👍 Thanks for the patience!

[Nuru]

/terratest

[Nuru]

LGTM, thank you! There remains a lot of cruft in this module to clean up around testing and examples, but that is beyond the scope of this PR.

• Supersedes and closes Update README.md and docs #86
• Supersedes and closes Optional recording #79
• Supersedes and closes Add Filtering of Recorded ResourceTypes #65
• Supersedes and closes add ability to choose specific resources in config #49
• Supersedes and closes Fix: Allow recorders to define including global resources #36

Requested changes have been made

[Nuru]

/terratest

[Nuru]

@AdamTylerLynch Thank you very much for this PR. As you can see, it elegantly resolved a number of piecemeal attempts at adding this feature and complaints about missing support.

@Gowiem Thank you for all your help. Your guidance was excellent, so by the time I got to look at this PR, all my concerns about the original PR had been addressed.

FYI, regarding list vs. not a list inputs, this is a complex issue about which I'm going to write a more detailed article. As a general rule, if the Terraform resource block is documented in the Terraform resource to be a list (see, for example, aws_route_table.route), we model it as a list, even if it is documented as a list of max length 1. If it is documented as an optional block, as here with recording_mode, we model it as an optional object.

Now if the variable in question is involved with determining the number of resources to create, a number which must be known at plan time, we always use a list or map. But in this case where, even if the API is documented as a list, the Terraform resource is not documented as being a block list, we leave it an object.

Regarding validation blocks, we typically use them as enhanced documentation for the case where the input type does not match the resource attribute type. The primary case being these variables that control the number of resources. We may take an optional value as a list of length zero or one, even though the resource takes a plain value, so there we add a validation rejecting lists longer than length one and explaining the situation. Other situation for custom validation are where there is some value (typically one or more of a set of enums) we do not support, or where an input is removed but we want to provide guidance on how to migrate to its replacement (in which case anything but the default value triggers an error message).

I agree with @AdamTylerLynch that duplicating validation done by the API adds maintenance burden and slows the adoption of new features without providing enough benefit to be worth it.