fix: module/roles-to-principals has long trust policies #17
Conversation
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟠 Require terratestWaiting checks:
|
WalkthroughThis PR makes ARN prefix for SSO permission-set role matching configurable, deprecates an overridable-team flag, unconditionally sources accounts from AWS Organizations, replaces stack-describe dynamic-role logic with direct principal→account access maps, adjusts IAM-role caller identity handling to detect SSO permission sets, updates module paths/inputs, removes several generated README doc blocks, and tightens a provider version constraint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TF as Terraform config
participant Orgs as AWS Organizations
participant Caller as aws_caller_identity
participant Locals as local computations
participant Outputs as module outputs
Note over TF,Orgs: Account sourcing (now unconditional)
TF->>Orgs: data.aws_organizations_organization.organization.accounts
Orgs-->>TF: account list
TF->>Locals: build local.full_account_map (exclude SUSPENDED)
Caller->>TF: aws_caller_identity
TF->>Locals: current_user_account ← caller.account_id
TF->>Locals: parse permission_set from role ARN (regex) or use role ARN
Note over Locals: Dynamic roles logic replaced
TF->>Locals: construct account_auths (principals per account)
Locals->>Locals: aggregate principal_terraform_access_map (apply overrides plan)
Locals->>Outputs: terraform_access_map ← principal_terraform_access_map (if dynamic enabled)
Outputs-->>TF: exported map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/modules/roles-to-principals/variables.tf (1)
69-79: New variable for flexible ARN prefix configuration.
The introduction ofoverridable_permission_set_arn_like_role_prefixis well documented and supports the flexible construction of the ARN. However, please note a minor typo in the description on line 73: “disinguish” should be corrected to “distinguish.” Additionally, ensure that the inclusion of a wildcard (*) in the default value is thoroughly tested for your intended glob use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/modules/roles-to-principals/main.tf(1 hunks)src/modules/roles-to-principals/variables.tf(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (2)
src/modules/roles-to-principals/main.tf (1)
49-57: Clarify ARN format string and double-formatting usage.
The new format string on line 55 uses a double percentage (%%s) to leave a literal%sfor later substitution by Terraform’sformatlistfunction. Please verify that the substitution order (usinglocal.aws_partition, thenvar.overridable_permission_set_arn_like_role_prefix, followed bymodule.account_map.outputs.full_account_map[acct]) is correct and produces the intended ARN structure for AWS SSO Permission Sets.src/modules/roles-to-principals/variables.tf (1)
53-67: Deprecation update for permission sets feature.
The variableoverridable_team_permission_sets_enabledhas been correctly marked as deprecated with the default value changed fromtruetofalse, and the description now directs users to usetrusted_identity_permission_setsinaws-team-roles. Consider adding a brief note about migration steps for existing configurations.
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
|
Heads up! This pull request looks stale. It will be closed soon, if there are no new commits. ⏳ |
mergify
bot
added
stale
overridable_team_permission_sets_enabled
cloudpossebot
force-pushed
the
fix/roles-to-principals/reduce-trust-policy-for-sso
branch
from
f927ab5 to
68b6a15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/dynamic-roles--stacks.tf (3)
51-53: Use jsondecode for utils_describe_stacks output (JSON, not YAML).Replace yamldecode with jsondecode to avoid subtle parsing surprises.
- teams_stacks = local.dynamic_role_enabled ? { - for k, v in yamldecode(data.utils_describe_stacks.teams[0].output) : k => v if !local.stack_has_namespace || try(split(module.this.delimiter, k)[local.stack_namespace_index] == module.this.namespace, false) - } : local.empty + teams_stacks = local.dynamic_role_enabled ? { + for k, v in jsondecode(data.utils_describe_stacks.teams[0].output) : k => v if !local.stack_has_namespace || try(split(module.this.delimiter, k)[local.stack_namespace_index] == module.this.namespace, false) + } : local.empty
56-58: Dash-in-key traversal is invalid; fix attribute access and guard empty teams_vars.HCL can’t traverse dashed keys with dots; use ["aws-teams"]. Also prevent index-out-of-range when no stacks found.
- teams_vars = { for k, v in local.teams_stacks : k => v.components.terraform.aws-teams.vars if try(v.components.terraform.aws-teams.vars, null) != null } - # Extract components.terraform.aws-teams.vars.teams_config, drop the stack name. - teams_config = local.dynamic_role_enabled ? values(local.teams_vars)[0].teams_config : local.empty + teams_vars = { for k, v in local.teams_stacks : k => v.components.terraform["aws-teams"].vars if try(v.components.terraform["aws-teams"].vars, null) != null } + # Extract components.terraform.aws-teams.vars.teams_config, drop the stack name. + teams_config = local.dynamic_role_enabled ? try(values(local.teams_vars)[0].teams_config, {}) : local.empty
68-70: Use jsondecode for team_roles output.- team_roles_stacks = local.dynamic_role_enabled ? { - for k, v in yamldecode(data.utils_describe_stacks.team_roles[0].output) : k => v if !local.stack_has_namespace || try(split(module.this.delimiter, k)[local.stack_namespace_index] == module.this.namespace, false) - } : local.empty + team_roles_stacks = local.dynamic_role_enabled ? { + for k, v in jsondecode(data.utils_describe_stacks.team_roles[0].output) : k => v if !local.stack_has_namespace || try(split(module.this.delimiter, k)[local.stack_namespace_index] == module.this.namespace, false) + } : local.empty
🧹 Nitpick comments (5)
src/dynamic-roles--stacks.tf (5)
45-46: Potentially unused local.stack_account_map is defined but not used in this file. If unused across the module, drop it.
2-3: Docs: use plural “aws-teams”.“aws-team” → “aws-teams” for consistency with component name.
-# this files reads in all the `aws-team` and `aws-team-roles` stacks +# this file reads in all the `aws-teams` and `aws-team-roles` stacks
23-24: Docs: fix self-reference typo.Reference stack_has_namespace (not duplicate stack_namespace_index).
-# the same `namespace` as `module.this.namespace`. See `local.stack_namespace_index` and `local.stack_namespace_index` +# the same `namespace` as `module.this.namespace`. See `local.stack_namespace_index` and `local.stack_has_namespace`
63-64: Docs: map vs list wording.team_arns is a map (team_name => ARN), not a list.
-# Now we have local.team_arns which is a list of IAM role ARNs for each team. +# Now we have local.team_arns which is a map of team_name => IAM role ARN.
99-105: Module source pinning is OK; Checkov CKV_TF_1 is a false positive for registry sources.You’re using the Terraform Registry plus an explicit version, which is sufficient. If Checkov blocks the pipeline, add a scoped skip with rationale.
#checkov:skip=CKV_TF_1 This is a Terraform Registry module pinned by version; commit hashes apply to VCS sources. module "atmos" { source = "cloudposse/label/null" version = "0.25.0" # ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHANGELOG.md(1 hunks)src/README.md(2 hunks)src/dynamic-roles--stacks.tf(1 hunks)src/dynamic-roles.tf(1 hunks)src/main.tf(1 hunks)src/modules/iam-roles/README.md(0 hunks)src/modules/iam-roles/main.tf(2 hunks)src/modules/iam-roles/outputs.tf(1 hunks)src/modules/roles-to-principals/README.md(0 hunks)src/modules/roles-to-principals/main.tf(1 hunks)src/modules/roles-to-principals/variables.tf(1 hunks)src/modules/team-assume-role-policy/README.md(3 hunks)src/modules/team-assume-role-policy/main.tf(2 hunks)src/modules/team-assume-role-policy/variables.tf(1 hunks)src/outputs.tf(1 hunks)src/variables.tf(0 hunks)src/versions.tf(1 hunks)
💤 Files with no reviewable changes (3)
- src/variables.tf
- src/modules/iam-roles/README.md
- src/modules/roles-to-principals/README.md
🚧 Files skipped from review as they are similar to previous changes (12)
- src/modules/iam-roles/outputs.tf
- src/modules/team-assume-role-policy/variables.tf
- src/outputs.tf
- src/main.tf
- src/dynamic-roles.tf
- src/modules/team-assume-role-policy/README.md
- src/modules/roles-to-principals/variables.tf
- src/modules/iam-roles/main.tf
- src/modules/roles-to-principals/main.tf
- src/modules/team-assume-role-policy/main.tf
- src/README.md
- CHANGELOG.md
🧰 Additional context used
🪛 Checkov (3.2.334)
src/dynamic-roles--stacks.tf
[medium] 99-123: Ensure Terraform module sources use a commit hash
(CKV_TF_1)
🔇 Additional comments (4)
src/dynamic-roles--stacks.tf (2)
62-63: Confirm presence of local.iam_role_arn_templates and local.account_role_map.identity.These locals aren’t defined in this file; make sure they exist and the format string matches one %s placeholder.
75-78: Confirm merge precedence.merge(teams_vars, team_roles_vars) makes team_roles_vars win on key collisions. Confirm this is intended.
src/versions.tf (2)
15-15: LGTM on constraint style.Using
~> 1.26keeps you on the 1.x line while avoiding drift; aligns with semver expectations.Confirm README/module docs and any example stacks also reflect
~> 1.26.
14-16: cloudposse/utils ~>1.26 — verified: no Terraform Core floor; refresh consumer lockscloudposse/utils v1.26 does not declare a Terraform Core required_version; this repo’s versions.tf files currently set required_version = ">= 1.2.0". AWS provider v5 does not universally force TF >1.2. Do not bump required_version by default. Actions: test the bump on TF 1.2 in CI, run
terraform init -upgradein consumers and commit updated.terraform.lock.hcl, and update any modules/providers pinned < 1.26.Location: src/versions.tf (lines 14–16).
Likely an incorrect or invalid review comment.
| team_roles_vars = { for k, v in local.team_roles_stacks : k => v.components.terraform.aws-team-roles.vars if try(v.components.terraform.aws-team-roles.vars, null) != null } | ||
|
|
There was a problem hiding this comment.
Dash-in-key traversal is invalid; fix aws-team-roles access.
- team_roles_vars = { for k, v in local.team_roles_stacks : k => v.components.terraform.aws-team-roles.vars if try(v.components.terraform.aws-team-roles.vars, null) != null }
+ team_roles_vars = { for k, v in local.team_roles_stacks : k => v.components.terraform["aws-team-roles"].vars if try(v.components.terraform["aws-team-roles"].vars, null) != null }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| team_roles_vars = { for k, v in local.team_roles_stacks : k => v.components.terraform.aws-team-roles.vars if try(v.components.terraform.aws-team-roles.vars, null) != null } | |
| team_roles_vars = { for k, v in local.team_roles_stacks : k => v.components.terraform["aws-team-roles"].vars if try(v.components.terraform["aws-team-roles"].vars, null) != null } |
🤖 Prompt for AI Agents
In src/dynamic-roles--stacks.tf around lines 73 to 74, the code attempts to
access a map key containing hyphens using dot notation
(v.components.terraform.aws-team-roles.vars), which is invalid in Terraform;
change the access to bracket notation with a quoted key (e.g.,
v.components.terraform["aws-team-roles"].vars) and apply the same bracket syntax
inside the try() call so the expression becomes k =>
v.components.terraform["aws-team-roles"].vars if
try(v.components.terraform["aws-team-roles"].vars, null) != null.
what
overridable_team_permission_sets_enabledoverridable_permission_set_arn_like_role_prefixwith compressed/globdefault
why
Permission sets to surpass this limit if they are fully written out
aws-team-rolescomponentSummary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation