Fix cloudtrail s3 bucket name

[goruha]

what

• Use own the CloudTrail's S3 bucket name null/label module
• Added query_override variable

why

• Have a single source of truth S3 bucket name module. We can not rely on the S3 bucket output, as there is a dependency loop that has to be solved.
• Allow to specify a custom query

references

• Deploy Fails - Error: putting S3 Bucket ... Policy has invalid resource #71

Summary by CodeRabbit

• New Features

  • Optional query override to customize the Datadog archive query; additional query tags now configurable.

• Chores

  • Introduced a dedicated label/context for CloudTrail resources and updated bucket, policy, and archive references to use it for consistent naming and policies.
  • Archive query selection now uses a local default-or-override mechanism.

[coderabbitai]

Walkthrough

Adds a new cloudtrail_label module, rebinds CloudTrail S3 bucket context and IAM policy ARNs to use the label module's context and id, introduces query_override and additional_query_tags variable defaults, and replaces an inline Datadog archive query with local.query (default + override logic).

Changes

Cohort / File(s)	Summary
CloudTrail label & module wiring src/main.tf	Added module "cloudtrail_label" (source = "cloudposse/label/null", version = "0.25.0", name = "datadog-logs-archive-cloudtrail", context = module.this.context). Updated module "cloudtrail_s3_bucket" and module "cloudtrail" usages to use context = module.cloudtrail_label.context and removed the name argument from the bucket module.
IAM policy ARN updates src/main.tf	Replaced S3 ARN references in data "aws_iam_policy_document" "default" from arn:${local.aws_partition}:s3:::${module.this.id}-cloudtrail and ...-cloudtrail/* to arn:${local.aws_partition}:s3:::${module.cloudtrail_label.id} and .../* (multiple occurrences).
Datadog logs archive & locals src/main.tf	Replaced inline datadog_logs_archive query with local.query. Added locals default_query (composed from stage, account, additional tags) and query (uses var.query_override if set, else local.default_query).
Variables src/variables.tf	Added variable "query_override" (type = string, nullable = true, default = null). Updated variable "additional_query_tags" to include default = [].

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Root as Root Module
    participant This as module.this
    participant Label as module.cloudtrail_label
    participant Bucket as module.cloudtrail_s3_bucket
    participant IAM as aws_iam_policy_document
    participant DD as datadog_logs_archive
    participant Loc as locals

    Note over Root,Label: New label module created
    Root->>Label: create label (name="datadog-logs-archive-cloudtrail", context=module.this.context)

    Note over Root,Bucket: Bucket/context wiring changed
    Root->>Bucket: create bucket (context=module.cloudtrail_label.context)

    Note over IAM,Label: IAM ARNs now reference label id
    IAM->>Label: read id -> build ARN arn:...:s3:::${module.cloudtrail_label.id}/*

    Note over Loc,DD: Query selection (default vs override)
    Loc->>Loc: compute default_query using stage/account/additional_query_tags
    Loc->>DD: provide local.query (var.query_override ? override : default_query)
    DD->>Root: create datadog_logs_archive (query=local.query)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

needs-test, needs-review

Poem

🐰 I hopped in code with gentle cheer,
A label stitched where names appear,
Buckets and ARNs now find their way,
Queries choose what they will say,
Small hops, neat trails—springtime near.

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 "Fix cloudtrail s3 bucket name" is directly aligned with the primary objective stated in the PR description and clearly reflects the main architectural change in this changeset. The core modification involves introducing a new cloudtrail_label module as the single source of truth for the CloudTrail S3 bucket name, replacing the previous dependency on module.this.id, which required updating multiple IAM policy ARN references and the cloudtrail_s3_bucket module configuration. While the PR also includes secondary changes like adding a query_override variable and query logic updates, these are ancillary to the primary purpose of fixing the bucket naming issue. The title is concise, specific enough for team members scanning history to understand the change, and accurately summarizes the main intent without excessive detail.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-cloudtrail-bucket-name

---

📜 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 341d75c and d0d08de.

📒 Files selected for processing (2)

• src/main.tf (7 hunks)
• src/variables.tf (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/variables.tf

🧰 Additional context used

📓 Path-based instructions (2)

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/main.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/main.tf

🪛 Checkov (3.2.334)

src/main.tf

[medium] 223-229: Ensure Terraform module sources use a commit hash

(CKV_TF_1)

⏰ 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). (3)

• GitHub Check: component / terraform / Lint (./src)
• GitHub Check: component / terraform / bats
• GitHub Check: Summary

🔇 Additional comments (5)

src/main.tf (5)

68-68: ✓ ARN replacements align with PR objective.

All three IAM policy resource ARNs (lines 68, 87, 122) correctly migrate from module.this.id to module.cloudtrail_label.id, establishing the label module as the single source of truth for the CloudTrail S3 bucket name and resolving the dependency loop.

Also applies to: 87-87, 122-122

---

297-297: ✓ CloudTrail bucket context properly bound to label module.

Line 297 correctly binds module.cloudtrail_s3_bucket context to module.cloudtrail_label.context, ensuring consistent naming and labeling across the CloudTrail infrastructure.

---

345-345: ✓ Query logic migrated to locals for override flexibility.

Line 345 replaces the inline query composition with local.query, which implements the override pattern (default + query_override fallback) as intended in the PR. This maintains backward compatibility while allowing custom queries.

---

223-229: No changes needed; code follows established project versioning standards.

The project's .tflint.hcl explicitly enables terraform_module_version (requiring versions for registry modules) and terraform_module_pinned_source (which applies only to git/mercurial repositories, not registry sources). All cloudposse registry modules in src/ consistently use version constraints rather than commit hashes. The code at lines 223–229 is compliant with these guidelines.

Likely an incorrect or invalid review comment.

---

31-32: Variables query_override and additional_query_tags are correctly defined.

Verification confirms both variables exist in src/variables.tf with appropriate configurations:

• query_override: nullable string with clear description
• additional_query_tags: list with default empty array and clear description

The variable definitions align with their usage in lines 31–32 of src/main.tf and meet Terraform documentation standards.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/main.tf (1)

219-225: Consider pinning module source by commit hash for enhanced reproducibility.

The module uses semantic versioning; Checkov recommends explicit commit hashing for version control determinism. This ensures the exact module code is used across deployments, mitigating risks from minor version changes.

Apply this diff to pin by commit hash (verify the current commit hash for v0.25.0 from the registry):

 module "cloudtrail_label" {
-  source  = "cloudposse/label/null"
-  version = "0.25.0" # requires Terraform >= 0.13.0
+  source = "git::https://github.com/cloudposse/terraform-null-label.git?ref=0.25.0"

Alternatively, you can look up the commit hash for tag 0.25.0 from the module's repository if you prefer explicit commit pinning.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9767707 and 7cda741.

📒 Files selected for processing (1)

• src/main.tf (5 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

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/main.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/main.tf

🪛 Checkov (3.2.334)

src/main.tf

[medium] 219-225: Ensure Terraform module sources use a commit hash

(CKV_TF_1)

⏰ 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 (2)

src/main.tf (2)

64-64: IAM policy ARN updates establish correct bucket reference.

The ARN updates correctly reference module.cloudtrail_label.id instead of manual composition. This ensures the policy grants permissions to the exact bucket provisioned by the cloudtrail_s3_bucket module, establishing a single authoritative source for the bucket name.

Also applies to: 83-83, 118-118

---

293-293: Context change correctly establishes input-driven naming with no issues.

Verification confirms the shift to module.cloudtrail_label.context successfully resolves the circular dependency. The s3-bucket v4.10.0 module derives its bucket name from the context using Cloud Posse's null-label logic (same as cloudtrail_label), producing a bucket ID matching module.cloudtrail_label.id already referenced in the policy ARNs (lines 64, 83, 118). Naming is now upstream-dependent on the label module rather than downstream-dependent on bucket outputs.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7cda741 and 341d75c.

📒 Files selected for processing (1)

• src/variables.tf (1 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

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

**/*.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

⏰ 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

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 341d75c and 4acc711.

📒 Files selected for processing (2)

• src/main.tf (7 hunks)
• src/variables.tf (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/variables.tf

🧰 Additional context used

📓 Path-based instructions (2)

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/main.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/main.tf

🪛 Checkov (3.2.334)

src/main.tf

[medium] 224-230: Ensure Terraform module sources use a commit hash

(CKV_TF_1)

🪛 GitHub Actions: Branch

src/main.tf

[error] 1-1: Command 'git diff --exit-code' failed with exit code 1. CI pipeline halted due to uncommitted changes or non-zero diff check.

⏰ 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 (3)

src/main.tf (3)

224-230: Context propagation verified and dependency loop resolved.

All assertions have been confirmed:

• ✓ Lines 69, 88, 123 correctly use module.cloudtrail_label.id in ARNs
• ✓ Line 298 correctly uses module.cloudtrail_label.context for S3 bucket context
• ✓ Module receives context from module.this.context (line 228)
• ✓ No remaining references to module.this.id or module.this.context in cloudtrail-related ARNs

The implementation correctly isolates cloudtrail bucket naming through the dedicated label module, breaking the circular dependency mentioned in issue #71.

---

30-33: All verification points confirmed—locals logic and query override implementation are correct.

✓ The concat() in line 31 produces the expected Datadog query format: "env:{stage} OR account:{aws_account_id} OR {additional_query_tags}"

✓ The query_override variable in variables.tf is typed optional(string), correctly matching the null check in line 33

✓ The logs_archive resource (line 346) correctly references local.query; the catchall_archive resource intentionally uses hardcoded "*" (by design). All inline query construction has been migrated to locals.

---

224-230: Review comment is incorrect and should be disregarded.

The module cloudposse/label/null is a Terraform Registry module and is already correctly pinned to semantic version 0.25.0. Registry modules require semantic version tags; Terraform Registry publishes releases by semantic-version tags (vMAJOR.MINOR.PATCH), and non-semver tags are ignored.

Commit hashes apply only to git/mercurial sources (pinned via ?ref=), not registry modules. The codebase already follows the correct pattern: registry modules use version constraints while git sources use ?ref= with commit references (e.g., ?ref=v1.536.0 in src/providers.tf). The .tflint.hcl enforces this with terraform_module_version (enabled), and the README.md explicitly advises "pinning each module to the exact version you're using."

CKV_TF_1 is intended for unversioned git/mercurial sources, not registry modules. The code is compliant with organizational standards.

Likely an incorrect or invalid review comment.

[github-actions]

These changes were released in v1.537.0.