Conversation
WalkthroughThis PR extends the AWS Identity Center module to support IdP-managed groups lookup, adds three new permission sets for root access and Terraform state backend operations, introduces conditional account map handling, and provides mixin files for flexible provider and version configuration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
…support New features: - Add TerraformPlanAccess permission set (read-only state and account access) - Add TerraformApplyAccess permission set (read/write state and admin access) - Add RootAccess permission set (centralized root access via sts:AssumeRoot) - Add provider-root.tf for root account assignments via profile - Add idp_groups variable to look up IdP-synced groups - Add group_ids output combining manual and IdP groups - Add account_map_enabled variable to toggle between remote state and static mapping - Add account_map variable for static account ID mapping Changes: - Update main.tf to conditionally use account_map module or static variable - Update main.tf to include new permission sets and idp_groups data source - Update remote-state.tf with conditional bypass for account_map - Update variables.tf with new variables - Update outputs.tf with group_ids output These changes support the account-map deprecation effort by enabling profile-based authentication and static account mappings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
757f7f6 to
e8326b3
Compare
The root provider with alias "root" is now defined in provider-root.tf as a simple profile-based provider. Remove the duplicate definition and iam_roles_root module from providers.tf to avoid conflict. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Revert providers.tf to include the root provider using account-map and move the profile-based providers to mixins/ for optional vendoring. This allows the component to work with account-map out of the box while still supporting profile-based authentication when using Atmos Auth by vendoring the mixins. Changes: - Restore root provider and iam_roles_root module to src/providers.tf - Delete src/provider-root.tf (profile-based version) - Add mixins/providers.tf with profile-based default provider - Add mixins/provider-root.tf with profile-based root provider - Update .pre-commit-config.yaml to exclude mixins/ from tflint 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The main providers.tf is handled by the centralized mixin from cloudposse-terraform-components/mixins. We only need component-specific mixins for unique situations like aliased providers (provider-root.tf). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Since module.account_map.outputs provides values from either remote state (when account_map_enabled is true) or from the static var.account_map defaults (when bypassed), we can always reference module.account_map.outputs directly without conditional logic. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
/terratest |
|
There are no real tests for this component. So we set terratest statuses to successful execution without running any tests |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/policy-TerraformAccess.tf (1)
24-45: Inline policy is partially redundant with AWS managed policy attachments.The inline policies (lines 30 and 42) explicitly grant S3 and role assumption permissions, while policy attachments add ReadOnlyAccess or AdministratorAccess. The inline policies provide more granular control (e.g., specific S3 bucket scope), but this combination may be confusing. Consider documenting the intent or simplifying if the AWS managed policies are sufficient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.pre-commit-config.yaml(1 hunks)mixins/.tflint.hcl(1 hunks)mixins/provider-root.tf(1 hunks)mixins/versions.tf(1 hunks)src/main.tf(3 hunks)src/outputs.tf(1 hunks)src/policy-RootAccess.tf(1 hunks)src/policy-TerraformAccess.tf(1 hunks)src/providers.tf(1 hunks)src/remote-state.tf(1 hunks)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 source of truth in src with these files present: main.tf, variables.tf, outputs.tf, providers.tf, versions.tf, context.tf
Files:
src/variables.tfsrc/providers.tfsrc/outputs.tfsrc/main.tf
src/**/*.tf
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.tf: Use 2-space indentation for all Terraform code
In Terraform, prefer 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 formatting (do not commit formatting violations)
Adhere to TFLint rules defined for the project (do not commit lint violations)
Files:
src/variables.tfsrc/providers.tfsrc/policy-RootAccess.tfsrc/outputs.tfsrc/policy-TerraformAccess.tfsrc/main.tfsrc/remote-state.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
🔇 Additional comments (12)
mixins/versions.tf (1)
1-13: LGTM!The mixin file correctly enforces Terraform >= 1.3.0 and AWS provider >= 4.0. The comment about precedence is helpful for users deploying with vendored mixins.
.pre-commit-config.yaml (1)
28-35: LGTM!The pre-commit hook exclusions for
mixins/are appropriate since mixins are partial Terraform files that shouldn't be independently formatted or linted.mixins/.tflint.hcl (1)
1-5: LGTM!Disabling linting by default for vendored mixin files is the correct approach since these are partial Terraform configurations meant to be merged into other components.
src/variables.tf (1)
80-88: LGTM!The
idp_groupsvariable is well-defined with a clear description. The default empty list is safe and allows gradual adoption of IdP group lookups.mixins/provider-root.tf (1)
8-12: Reconcile with past reviewer feedback on default implementation.A prior reviewer noted that "the default implementation should be without auth and with account-map". Since this mixin makes profile-based auth the default, verify that:
- Using this mixin is truly optional and doesn't break deployments when not used
- Documentation clearly explains when to use this mixin vs. the default account-map approach
- The default value
"core-root/terraform"aligns with your organization's actual profile namingsrc/main.tf (2)
78-90: Verify IdP group lookup error handling and existence guarantees.The data source uses
alternate_identifierwithDisplayNameto look up IdP groups. Terraform will error if a group invar.idp_groupsdoesn't exist in the Identity Store. Consider adding:
- Documentation warning users that IdP groups must be synced to AWS Identity Center before adding them to
var.idp_groups- Clear error messages if lookups fail during
terraform plan/apply
105-107: Permission set definitions are correctly defined and properly named.All three permission sets referenced at lines 105-107 in src/main.tf are correctly defined in src/policy-TerraformAccess.tf:
local.root_access_permission_set✓local.terraform_plan_access_permission_set(defined at line 24) ✓local.terraform_apply_access_permission_set(defined at line 36) ✓Variable names match exactly and follow lower_snake_case conventions.
src/policy-RootAccess.tf (1)
1-31: Root-task policy ARNs are valid and current.All five AWS managed root-task policies referenced in the condition are correct: IAMAuditRootUserCredentials, IAMCreateRootUserPassword, IAMDeleteRootUserCredentials, S3UnlockBucketPolicy, and SQSUnlockQueuePolicy. The implementation correctly restricts the permission set's
sts:AssumeRootaction to these officially supported task policies.src/providers.tf (1)
1-26: Theaccount_mapconditional mechanism is correctly implemented, but not via module count/for_each.The variables are properly used in
src/remote-state.tf: the module is always instantiated, but the remote-state module'sbypassanddefaultsparameters handle the conditional behavior. Whenaccount_map_enabled = false,bypass = trueanddefaults = var.account_mapwork together to return the static account mappings instead of performing a remote state lookup. Thevar.account_mapstructure correctly matches the expected outputs (full_account_map,root_account_account_name), and downstream references insrc/main.tfsafely consumemodule.account_map.outputswithout additional conditional logic since the module always provides the outputs.src/policy-TerraformAccess.tf (1)
1-46: Permission sets are already integrated into the main module.Both
terraform_plan_access_permission_setandterraform_apply_access_permission_setare concatenated into the module.permission_sets input in src/main.tf (lines 106–107).src/remote-state.tf (1)
16-18: Remove this review comment;module.iam_rolesis properly defined and available.The module is declared in
src/providers.tf(notsrc/context.tf) at line 70 and sources from the external CloudPosseaccount-map/modules/iam-rolesmodule. The outputsglobal_tenant_name,global_environment_name, andglobal_stage_nameare correctly referenced in lines 16–18 and used consistently throughout the codebase (e.g.,src/providers.tf,src/policy-TerraformUpdateAccess.tf). No action required.src/outputs.tf (1)
11-17: Data source and variable definitions are correctly configured; merge behavior is intentional.All components referenced in the output are properly defined:
data.aws_identitystore_group.idpqueries IdP-synced groups usingvar.idp_groups(a list of group names), and the merge operation correctly combines manual and IdP-synced groups. The second argument (IdP groups) takes precedence in a key collision, which is the intended design pattern—IdP-synced groups serve as the authoritative source. The code is correctly formatted with 2-space indentation and follows Terraform conventions.
|
These changes were released in v1.540.0. |
what
why
references
Summary by CodeRabbit
New Features
Configuration
✏️ Tip: You can customize this high-level summary in your review settings.