✨ add preventive guardrails

[daxingplay]

No description provided.

[AlibabaCloudLandingZone]

Terraform Review — Preventive Guardrails

Reviewed the Terraform changes in this PR against: provider version compatibility, state management correctness, naming conventions, security policy compliance, and resource dependency ordering.

Bugs

1. target attribute is dead — custom targets never work (main.tf:24)

target_id = can(each.value.target_id) ? each.value.target_id : local.resource_directory_root_folder_id

The variable object declares the attribute as target (see variables.tf), but this reads each.value.target_id. target_id does not exist on the object type, so can() always returns false and every policy attaches to the root folder — the target input is silently ignored. Verified empirically: can(var.preventive_guardrails[0].target_id) evaluates to false.

Fix:

target_id = coalesce(each.value.target, local.resource_directory_root_folder_id)

(keep the attribute name target, or rename the attribute to target_id consistently across variables.tf and the example tfvars).

2. can() is always true for rule_description → description becomes null (main.tf:22)

description = can(each.value.rule_description) ? each.value.rule_description : ""

rule_description is an optional() attribute — it always exists (as null when unset), so can() is always true, the "" fallback is never taken, and description receives null instead of "". Verified empirically: can(...) -> true, value -> null.

Fix:

description = coalesce(each.value.rule_description, "")

State management

3. Module rename breaks existing state (examples/common/main.tf)

module "detective_guardrails" is renamed to module "guardrails" with no moved block. Existing deployments will lose track of the detective guardrail resources (Terraform sees a "new" module and plans create/destroy). Add a moved block:

moved {
  from = module.detective_guardrails
  to   = module.guardrails
}

4. Fragile index .directories.0 (main.tf)

data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id errors with an unclear message if the data source returns an empty list. Add a length guard or wrap with try().

Provider version

5. required_version too low (versions.tf)

Still >= 0.14, but the code now uses optional() (stable since Terraform 1.3.0) — the experiments = [module_variable_optional_attrs] block was commented out in favor of the stable form. Bump required_version to >= 1.3.0.

6. Confirm provider version for the new resources (versions.tf)

alicloud_resource_manager_control_policy and alicloud_resource_manager_control_policy_attachment require a provider version that supports them. Please confirm >= 1.145.0 is sufficient, or bump accordingly.

Correct / good

• Resource dependency ordering (modules/control_policies/main.tf): the attachment references alicloud_resource_manager_control_policy.policy.id, producing a correct implicit dependency (policy created before attachment). The data.alicloud_resource_manager_resource_directories data source is read at plan time before local.resource_directory_root_folder_id is consumed by the module. Correct.
• Security policy: the DenyCreateRamRole policy denies ram:CreateRole except for resourcedirectoryaccountaccessrole — a sound least-privilege preventive control. No secrets introduced.

Suggestions

• Validate policy_document with jsonencode() or a validation block so a malformed policy fails at plan time rather than being applied silently.
• effect_scope = "RAM" is hardcoded in the control_policies module — consider exposing it as a variable for flexibility.
• Pre-existing typo config_aggreator_name -> config_aggregator_name (not introduced by this PR, but worth fixing while touching this module).

[AlibabaCloudLandingZone]

Terraform review of the preventive guardrails addition, checked for provider version compatibility, state management correctness, naming conventions, security policy compliance, and resource dependency ordering. Overall the structure is sound: control policy -> attachment (implicit dependency via policy_id), for_each keyed by rule_name so add/remove of a rule won't recompute the others. The headline issue is a target/target_id field mismatch that silently ignores the per-rule target.

Provider / Terraform version

• alicloud_resource_manager_control_policy / _attachment and the resource_directories data source are all available since provider v1.120.0, so the existing >= 1.145.0 constraint covers them. OK.
• The experiment block is removed and optional() is now used in both preventive_guardrails and detective_guardrails. optional() is GA only since Terraform 1.3, but versions.tf still pins required_version = ">= 0.14". Bump to >= 1.3.0 so pre-1.3 users get a clear constraint error instead of a confusing syntax error, and delete the commented-out experiment block.

State management

• module "control_policies" for_each keyed by rule_name -- good.
• Example module renamed detective_guardrails -> guardrails; existing users who copy the example need terraform state mv (example-only impact, noted inline).
• data.alicloud_resource_manager_resource_directories.default.directories.0... assumes a non-empty list; add a length/try guard.

Naming

• config_aggreator_name is a pre-existing misspelling of "aggregator" (not introduced here) -- drive-by fix welcome.

Security policy

• The example Deny ram:CreateRole attached at the RD root folder propagates to all member accounts and only exempts resourcedirectoryaccountaccessrole -- guardrail self-lockout risk for the IaC deployer. Noted inline.

README

• README.md still says "this module only contains detective guardrails" and only documents detective_guardrails. Update it to document preventive_guardrails, effect_scope, and the control policy resources.

Minor: description = can(each.value.rule_description) ? each.value.rule_description : "" (main.tf:22) is ineffective for an optional attribute -- can() returns true even when it's null, so the "" fallback never fires (null is acceptable for description, so harmless, but coalesce(each.value.rule_description, "") would express the intent more clearly).

[AlibabaCloudLandingZone]

Bug: the variable defines target = optional(string) (variables.tf), but here you read each.value.target_id. Since target_id is not an attribute of the object type, can() is always false and the fallback to root_folder_id always wins -- the per-rule target is silently ignored. Rename the variable field to target_id and use coalesce(each.value.target_id, local.resource_directory_root_folder_id), or read each.value.target here. The documented 'If target is not specified, it will be set to root_folder_id' can't currently be honored.

[AlibabaCloudLandingZone]

This field is named target, but main.tf reads each.value.target_id, so it's never consumed. Rename to target_id to match the usage (or change main.tf to use target). See the inline comment on main.tf.

[AlibabaCloudLandingZone]

directories.0.root_folder_id will fail at plan time if the resource directory isn't enabled (empty list -> index out of range). This module already assumes RD exists (the detective side uses alicloud_resource_manager_accounts), but consider a length()/try() guard for a clearer error. Minor: the "${...}" interpolation wrapper is redundant -- = data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id is enough.

[AlibabaCloudLandingZone]

effect_scope is hardcoded to "RAM" (valid values: All | RAM; ForceNew). The detective guardrails are fully variable, so for consistency consider exposing effect_scope on the submodule so non-RAM-scoped policies are possible. Note: switching scope later forces replacement of the policy.

[AlibabaCloudLandingZone]

Security caveat: this Deny ram:CreateRole policy is attached at the RD root folder and propagates to all member accounts, but the only exempt principal is resourcedirectoryaccountaccessrole. That means the IaC/landing-zone deployer itself can be blocked from creating roles -- a classic guardrail self-lockout. Consider also exempting the deployment principal / a break-glass role in the Condition, or applying this to a narrower target_id than root.

[AlibabaCloudLandingZone]

Renaming module "detective_guardrails" -> module "guardrails" changes the state address for anyone using this example. Existing deployments that follow it will need terraform state mv module.detective_guardrails module.guardrails to avoid destroy/recreate. (Example-only impact, low severity.)

[AlibabaCloudLandingZone]

The experiment block is commented out instead of removed. Since optional() is now used in the variable definitions (GA since Terraform 1.3), remove this block and set required_version = ">= 1.3.0" in versions.tf (currently >= 0.14, which lets pre-1.3 inits fail with a confusing error).

[AlibabaCloudLandingZone]

Terraform Code Review — PR #81: Preventive Guardrails

Reviewed for: provider version compatibility, state management correctness, naming conventions, security policy compliance, and resource dependency ordering.

---

1. Provider Version Compatibility

CRITICAL — required_version is incorrect in versions.tf

versions.tf specifies required_version = ">= 0.14", but the code uses the optional() type modifier (in variables.tf for both preventive_guardrails and detective_guardrails), which requires Terraform >= 1.3.0. The module_variable_optional_attrs experiment was commented out but required_version was not updated. The can() function also requires >= 0.15.

Fix: Update versions.tf to required_version = ">= 1.3.0".

OK: alicloud_resource_manager_control_policy (available since v1.120.0) and alicloud_resource_manager_control_policy_attachment (available since v1.120.0) are both covered by the existing alicloud >= 1.145.0 constraint.

---

2. State Management Correctness

HIGH — Breaking module rename in example (examples/common/main.tf)

Renaming module "detective_guardrails" to module "guardrails" is a breaking state change. Any user who has already applied this example will have orphaned resources in state — Terraform will plan to destroy and recreate all detective guardrail resources. If this rename is intentional, document the required terraform state mv migration steps.

MEDIUM — No prevent_destroy lifecycle on security-critical resources

Control policies are security guardrails. Consider adding lifecycle { prevent_destroy = true } to alicloud_resource_manager_control_policy in modules/control_policies/main.tf to prevent accidental deletion.

OK: The for_each key uses rule.rule_name — stable and predictable. Renaming a rule will destroy + recreate, which is expected behavior.

---

3. Naming Conventions

CRITICAL — Variable attribute name mismatch (bug) in main.tf

variables.tf defines the attribute as target, but main.tf references each.value.target_id (which does not exist in the object type). can(each.value.target_id) will always evaluate to false, so target_id always falls back to local.resource_directory_root_folder_id. Any user-supplied target value is silently ignored.

Fix: Change each.value.target_id to each.value.target in main.tf line 24.

MEDIUM — can() misuse for optional attributes

can(each.value.rule_description) — since rule_description is optional(string), when omitted it is null, and can(null) returns true. The ternary then passes null as the description to the provider.

Fix: Use coalesce(each.value.rule_description, "") or each.value.rule_description != null ? each.value.rule_description : "".

LOW — Unnecessary string interpolation

"${data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id}" — the ${...} wrapper is redundant in modern Terraform. Use a direct reference.

LOW — Missing trailing newlines

common.tfvars and modules/control_policies/variables.tf are missing a newline at end of file.

LOW — Pre-existing typo

config_aggreator_name should be config_aggregator_name (double-g vs. single-g). Not introduced by this PR but worth noting.

---

4. Security Policy Compliance

MEDIUM — effect_scope hardcoded to "RAM"

The alicloud provider supports "All" and "RAM" for effect_scope. Hardcoding limits flexibility. Make this a variable with default "RAM" in modules/control_policies/variables.tf.

LOW — No JSON validation on policy_document

policy_document is typed as string but the provider expects valid JSON (JsonString). Consider adding validation on the variable or using jsonencode() in the example.

LOW — No tags on control policies

tags is available since provider v1.260.1. Consider adding optional tags for resource management and cost allocation.

OK: The example DenyCreateRamRole policy correctly uses Deny effect with StringNotLike condition to allow only resourcedirectoryaccountaccessrole to create RAM roles — good security practice.

---

5. Resource Dependency Ordering

MEDIUM — Unsafe index access on data source

data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id — if the resource directory is not enabled, .directories will be empty and .0 will cause a runtime error. Consider adding a guard check or a meaningful error message.

OK: alicloud_resource_manager_control_policy_attachment has correct implicit dependency on alicloud_resource_manager_control_policy.policy.id. Data sources resolve before resources — ordering is correct.

---

Additional Finding

MEDIUM — No outputs for preventive guardrails

output.tf only exports detective guardrail outputs (config_aggregator, config_rules, config_compliance_pack). Add outputs for control policies (e.g., control_policy_ids, control_policy_attachment_ids) so consumers can reference them.

---

Summary

Area	Severity	Count
Provider Version	CRITICAL	1
State Management	HIGH	1
State Management	MEDIUM	1
Naming / Bugs	CRITICAL	1
Naming / Bugs	MEDIUM	1
Naming	LOW	3
Security	MEDIUM	1
Security	LOW	2
Dependencies	MEDIUM	1
Outputs	MEDIUM	1

Recommendation: Address the 2 CRITICAL issues (wrong required_version, target/target_id mismatch) and the 1 HIGH issue (module rename migration) before merge. The MEDIUM issues should be addressed in a follow-up if not in this PR.

[AlibabaCloudLandingZone]

CRITICAL BUG — target_id attribute does not exist

The variable preventive_guardrails in variables.tf defines the attribute as target (not target_id). can(each.value.target_id) will always return false because target_id is not a defined attribute on the object type, so this expression always falls back to local.resource_directory_root_folder_id.

Any user-supplied target value is silently ignored.

Fix:

target_id = coalesce(each.value.target, local.resource_directory_root_folder_id)

[AlibabaCloudLandingZone]

MEDIUM — can() misuse for optional attributes

rule_description is optional(string). When omitted, it defaults to null. can(null) returns true, so the ternary evaluates each.value.rule_description which is null — passing null as the description.

Fix:

description = coalesce(each.value.rule_description, "")

[AlibabaCloudLandingZone]

MEDIUM — Unsafe index access + unnecessary interpolation

1. data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id — if the resource directory is not enabled, .directories will be empty and .0 will cause a runtime error. Add a guard or meaningful error.

2. The "${...}" interpolation wrapper is unnecessary in modern Terraform. Use:

resource_directory_root_folder_id = data.alicloud_resource_manager_resource_directories.default.directories.0.root_folder_id

[AlibabaCloudLandingZone]

MEDIUM — effect_scope hardcoded

The provider supports "All" and "RAM" for effect_scope. Consider making this a variable with default "RAM" to allow flexibility for future policies that need to apply to Alibaba Cloud accounts as well.

Also: Consider adding lifecycle { prevent_destroy = true } to this resource — control policies are security-critical and accidental deletion could expose the organization to risk.

[AlibabaCloudLandingZone]

Note: This attribute is named target, but main.tf line 24 references each.value.target_id (which doesn't exist in the object type). See the inline comment on main.tf for details. Either rename this to target_id or fix the reference in main.tf.

[AlibabaCloudLandingZone]

HIGH — Breaking module rename

Renaming module "detective_guardrails" to module "guardrails" is a breaking state change. Existing users who have applied this example will have orphaned resources in state — Terraform will plan to destroy and recreate all detective guardrail resources.

If this rename is intentional, document the required terraform state mv migration:

terraform state mv module.detective_guardrails module.guardrails

Or keep the old name to avoid the breaking change.