Skip to content

Conversation

@johncblandii
Copy link
Contributor

@johncblandii johncblandii commented Nov 12, 2025

what

Add KMS encryption support to the datadog-logs-archive component to address Drata compliance requirements. CloudTrail logs are now encrypted with SSE-KMS by default, with flexible configuration options to use an existing key or disable encryption if needed.

why

  • Created new kms.tf file with KMS key, policy, and alias resources
  • Added 5 new input variables for KMS configuration
  • Added 3 new outputs exposing KMS key information
  • Updated README with KMS encryption documentation

Summary by CodeRabbit

  • New Features

    • Added KMS encryption support for CloudTrail with flexible configuration options.
    • Option to use an existing KMS key or create a new one automatically.
    • Configurable KMS key deletion window and automatic key rotation.
  • Documentation

    • Updated documentation with CloudTrail KMS encryption configuration details and defaults.

… logs

This change addresses Drata compliance requirements by adding KMS encryption
to the CloudTrail logs created by the datadog-logs-archive component. The
implementation provides flexible configuration: encryption is enabled by
default with automatic key creation, but users can also bring their own KMS
key or disable encryption if needed. The KMS resources are organized in a
dedicated kms.tf file for better maintainability.

🤖 Generated with Claude Code

Co-Authored-By: Claude <[email protected]>
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 12, 2025

Walkthrough

This PR adds CloudTrail KMS encryption support: new Terraform variables and outputs, a conditional KMS key, alias and IAM policy (in kms.tf), local ARN resolution and module wiring in main.tf, and README/documentation updates.

Changes

Cohort / File(s) Summary
Configuration & Variables
src/variables.tf
Added five variables to control CloudTrail KMS encryption: cloudtrail_enable_kms_encryption, cloudtrail_kms_key_arn (nullable), cloudtrail_create_kms_key, cloudtrail_kms_key_deletion_window_in_days, and cloudtrail_kms_key_enable_rotation.
KMS Infrastructure
src/kms.tf
New IAM policy document data source (aws_iam_policy_document.cloudtrail_kms_key_policy), aws_kms_key.cloudtrail, and aws_kms_alias.cloudtrail. All are conditionally created when enabled, creating scoped policy statements for CloudTrail encryption/decryption and alias operations.
Module Integration & Locals
src/main.tf
Added local.cloudtrail_kms_key_arn to resolve ARN (explicit ARN, created key, or null). Passed kms_key_arn = local.cloudtrail_kms_key_arn into module.cloudtrail and extended depends_on to include aws_kms_key.cloudtrail.
Outputs
src/outputs.tf
Added outputs: cloudtrail_kms_key_arn (always from local), and conditional cloudtrail_kms_key_id and cloudtrail_kms_key_alias when a key is created by the module.
Documentation
src/README.md, README.yaml
Added "CloudTrail KMS Encryption" section describing defaults, new configurable variables, and note about generated key policy and authorized decryptors.
Provider pins / Prereqs
src/README.md
Updated provider entries to explicitly declare aws, datadog, and http with pinned versions in prerequisites/wiring notes.

Sequence Diagram

sequenceDiagram
    participant User
    participant Config as Input Variables
    participant Locals as Local Resolution
    participant KMS as KMS Resources
    participant CloudTrail as CloudTrail Module

    User->>Config: Provide encryption settings (enable, ARN, create, rotation)
    Config->>Locals: Provide values for local resolution

    rect rgb(230, 245, 230)
    note right of Locals: If enabled && create && no external ARN
    Locals->>KMS: Request creation (policy, key, alias)
    KMS->>Locals: Return created key ARN/ID/alias
    end

    alt External ARN provided
        Locals->>CloudTrail: Use provided `cloudtrail_kms_key_arn`
    else Created key used
        Locals->>CloudTrail: Use created key ARN
    end

    Locals->>CloudTrail: Pass `kms_key_arn`
    CloudTrail->>CloudTrail: Configure CloudTrail with KMS encryption
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Pay special attention to the IAM policy document in src/kms.tf (conditions: SourceArn, EncryptionContext, caller account restrictions).
  • Verify consistency of the composite condition across variables.tf, local resolution in main.tf, and count/creation logic in kms.tf.
  • Confirm depends_on change correctly enforces ordering and that kms_key_arn input matches the CloudTrail module's expected parameter.

Suggested reviewers

  • oycyc

Poem

🐇 I dug a tunnel, found a key so bright,
For CloudTrail logs to sleep safe at night.
A policy penned, an alias tied,
Hops of joy — the trail's now fortified! 🔐

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat: add KMS encryption support for CloudTrail logs' directly and clearly describes the primary change in the PR, which is adding KMS encryption capability for CloudTrail logs throughout the module.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 007435b and be81571.

📒 Files selected for processing (3)
  • README.yaml (1 hunks)
  • src/README.md (5 hunks)
  • src/kms.tf (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for all Terraform files
In Terraform, use lower_snake_case for variables and locals; keep resource/data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and adhere to TFLint rules defined in .tflint.hcl; do not commit formatting or lint violations

Files:

  • src/kms.tf
src/README.md

📄 CodeRabbit inference engine (AGENTS.md)

Regenerate src/README.md using atmos docs generate readme-simple; do not edit src/README.md manually

Files:

  • src/README.md
**/*.{yaml,yml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

  • src/README.md
  • README.yaml
README.{yaml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Treat README.yaml as the authoritative source and regenerate README.md from it (do not edit README.md directly)

Files:

  • README.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (5)
src/README.md (1)

30-40: **README.md must be regenerated, not manually edited. **

The CloudTrail KMS Encryption section was added manually to the auto-generated README. Per coding guidelines, src/README.md must be regenerated using atmos docs generate readme-simple rather than edited directly. The source file README.yaml has been correctly updated (lines 25–35 in README.yaml), so regenerating the README should include this content automatically.

Action: Run atmos docs generate readme-simple to regenerate src/README.md from README.yaml, then commit only the regenerated output (do not manually edit README.md).

src/kms.tf (3)

1-100: KMS policy structure is correct and follows CloudTrail best practices.

The policy statements are well-structured and appropriately scoped:

  • Statement 1 grants root account full KMS permissions (standard pattern).
  • Statements 2–3 allow CloudTrail service to generate data keys and describe the key, constrained by SourceArn and EncryptionContext conditions.
  • Statement 4 permits account principals to decrypt logs, restricted by CallerAccount and EncryptionContext conditions.
  • Statement 5 allows alias creation for root with CallerAccount scoping, without overly restrictive ViaService conditions.

The removal of the ViaService condition from alias creation (flagged in the previous review) has been correctly applied, allowing Terraform to create the alias via IAM credentials.


102-129: Resource configuration properly wired to variables and count condition.

The KMS key and alias resources use the correct composite count condition and reference configuration variables (deletion_window_in_days, enable_key_rotation). Tagging and policy wiring are consistent.


1-129: Cross-file integration verified — all requirements satisfied.

All three requirements have been confirmed in the codebase:

  1. src/main.tf correctly wires local.cloudtrail_kms_key_arn to the CloudTrail module's kms_key_arn parameter
  2. src/main.tf declares the dependency on aws_kms_key.cloudtrail in the depends_on list
  3. src/outputs.tf exposes the three KMS outputs (cloudtrail_kms_key_arn, cloudtrail_kms_key_id, cloudtrail_kms_key_alias) with proper conditional logic

The local.cloudtrail_kms_key_arn is defined in the main.tf locals block with correct conditional branching to handle all scenarios (KMS disabled, external ARN provided, create KMS key, or null).

README.yaml (1)

25-35: CloudTrail KMS Encryption documentation is well-structured and complete.

The new section accurately documents the five KMS-related variables, their defaults, and the generated key policy. The content aligns with the variables introduced in the PR and provides clear context for users. Once src/README.md is regenerated from this source file using atmos docs generate readme-simple, the documentation will be properly included.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@mergify mergify bot requested review from a team November 12, 2025 20:01
@mergify
Copy link

mergify bot commented Nov 12, 2025

Important

Do not edit the README.md directly. It's auto-generated from the README.yaml

Please update the README.yaml file instead.

Could you fix it @johncblandii? 🙏

@mergify mergify bot added triage Needs triage needs-test Needs testing labels Nov 12, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/variables.tf (1)

111-115: Clarify variable description to reflect full conditional logic.

The description for cloudtrail_create_kms_key (line 113) states it's "Only used if cloudtrail_kms_key_arn is not provided and cloudtrail_enable_kms_encryption is true," but it omits a key dependency: the variable is also ignored if cloudtrail_enable_kms_encryption is false. The description should clarify this cascading condition.

Suggested revision:

- description = "Create a new KMS key for CloudTrail encryption. Only used if cloudtrail_kms_key_arn is not provided and cloudtrail_enable_kms_encryption is true"
+ description = "Create a new KMS key for CloudTrail encryption. Only used if cloudtrail_enable_kms_encryption is true and cloudtrail_kms_key_arn is not provided"
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e9d2ef7 and 007435b.

📒 Files selected for processing (5)
  • src/README.md (1 hunks)
  • src/kms.tf (1 hunks)
  • src/main.tf (3 hunks)
  • src/outputs.tf (1 hunks)
  • src/variables.tf (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/README.md

📄 CodeRabbit inference engine (AGENTS.md)

Regenerate src/README.md using atmos docs generate readme-simple; do not edit src/README.md manually

Files:

  • src/README.md
**/*.{yaml,yml,md}

📄 CodeRabbit inference engine (AGENTS.md)

Use 2-space indentation for YAML and Markdown files

Files:

  • src/README.md
src/{main,variables,outputs,providers,versions,context}.tf

📄 CodeRabbit inference engine (AGENTS.md)

Keep the Terraform component’s source of truth in src/ with canonical files: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, and context.tf

Files:

  • src/variables.tf
  • src/main.tf
  • src/outputs.tf
**/*.tf

📄 CodeRabbit inference engine (AGENTS.md)

**/*.tf: Use 2-space indentation for all Terraform files
In Terraform, use lower_snake_case for variables and locals; keep resource/data source names descriptive and aligned with Cloud Posse null-label patterns
Run terraform fmt and adhere to TFLint rules defined in .tflint.hcl; do not commit formatting or lint violations

Files:

  • src/variables.tf
  • src/main.tf
  • src/outputs.tf
  • src/kms.tf
🪛 markdownlint-cli2 (0.18.1)
src/README.md

19-19: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3

(MD001, heading-increment)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary
🔇 Additional comments (5)
src/outputs.tf (1)

46-59: Outputs correctly guard against null references and match resource creation conditions.

The three new outputs are well-structured:

  • Line 47: cloudtrail_kms_key_arn exposes the derived or provided ARN without additional conditioning, returning null when encryption is disabled—appropriate for downstream modules.
  • Lines 52 & 57: cloudtrail_kms_key_id and cloudtrail_kms_key_alias use a consistent 4-part AND condition that precisely matches the condition in kms.tf (lines 3, 109, 130), ensuring outputs only expose created resources.

Indentation and naming are correct throughout.

src/main.tf (2)

34-40: KMS ARN resolution logic is sound.

The ternary chain correctly handles all paths:

  1. Encryption disabled → null
  2. Explicit ARN provided → use it
  3. Create key && module enabled → resolve created key ARN via try()
  4. Otherwise → null

The use of try() at line 38 is the correct pattern for safely referencing a resource that may not exist due to conditional count.


315-315: Verify Terraform dependency behavior with conditional resource count.

At line 315, aws_kms_key.cloudtrail is added to depends_on, but this resource uses count and may not be created (count = 0 when encryption disabled or using external key).

Terraform should gracefully handle this—dependencies to non-existent resources are typically ignored. However, please verify this behavior in your environment by running terraform plan and confirming that:

  1. When encryption is disabled (cloudtrail_enable_kms_encryption = false), no KMS key is created and CloudTrail still provisions without errors.
  2. The depends_on to the non-existent KMS key does not cause a validation error.

Also applies to: 325-325

src/kms.tf (2)

108-126: KMS key resource is well-configured.

The aws_kms_key resource properly:

  • Uses user-provided variables for deletion window (line 112) and rotation (line 113).
  • Includes a descriptive key policy (line 114).
  • Applies appropriate tags and naming conventions (lines 116–125).

The conditional count ensures the key is created only when encryption is enabled, no external ARN is provided, and creation is requested.


2-41: KMS policy statements for CloudTrail encryption are comprehensive.

The policy document includes all required permissions:

  • Account root can manage the key and create aliases (statements 1 & 5).
  • CloudTrail service can encrypt logs and describe the key (statements 2 & 3).
  • Account principals can decrypt logs for access (statement 4).

EncryptionContext and SourceArn conditions properly scope access. However, review the ViaService condition in statement 5 (line 102) as noted separately.

johncblandii and others added 2 commits November 12, 2025 14:14
Signed-off-by: John C. Bland II <[email protected]>
The ViaService condition restricting alias creation to EC2 would prevent
Terraform from creating the KMS alias. Keeping only the CallerAccount
condition allows principals in the account to create aliases while
maintaining proper account scoping.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
@milldr
Copy link
Contributor

milldr commented Nov 12, 2025

/terratest

@mergify mergify bot removed the triage Needs triage label Nov 12, 2025
@mergify
Copy link

mergify bot commented Nov 13, 2025

Thanks @johncblandii for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@milldr milldr enabled auto-merge November 14, 2025 15:46
@milldr milldr added this pull request to the merge queue Nov 14, 2025
Merged via the queue into cloudposse-terraform-components:main with commit 2ecefc5 Nov 14, 2025
19 checks passed
@github-actions
Copy link
Contributor

These changes were released in v1.537.2.

@johncblandii johncblandii deleted the feat/PP-51-datadog-logs-archive branch November 14, 2025 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-test Needs testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants