feat: allow eks addons to use pod identity associations #244
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to the Terraform configuration files for an Amazon EKS cluster. The AWS provider version requirement is increased from ">= 5.74" to ">= 5.75". A new optional field for Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (4)
examples/complete/versions.tf(1 hunks)main.tf(1 hunks)variables.tf(1 hunks)versions.tf(1 hunks)
🧰 Additional context used
🪛 GitHub Check: terraform-module / CI / Lint (.)
main.tf
[warning] 177-177:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:177:29: warning: Lookup with 2 arguments is deprecated ()
[warning] 178-178:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:178:29: warning: Lookup with 2 arguments is deprecated ()
🔇 Additional comments (3)
versions.tf (1)
7-7: Verify minimum version requirement for pod identity associations
The AWS provider version bump appears to be related to supporting pod identity associations in EKS addons.
Let's verify if this is the minimum required version:
examples/complete/versions.tf (1)
7-7: Verify minimum required version for pod identity associations feature.
The AWS provider version bump from 5.74 to 5.75 looks reasonable for a feature addition.
Let's verify if 5.75 is indeed the minimum required version for pod identity associations:
main.tf (1)
171-171: LGTM: Pod identity association attribute added correctly.
The attribute is added with proper null handling for backward compatibility.
| dynamic "pod_identity_association" { | ||
| for_each = try(lookup(each.value, "pod_identity_association", null), null) != null ? [true] : [] | ||
|
|
||
| content { | ||
| role_arn = try(lookup(each.value.pod_identity_association, "role_arn"), null) | ||
| service_account = try(lookup(each.value.pod_identity_association, "service_account"), null) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to address deprecated lookup usage and simplify conditions.
The implementation works but can be improved:
- The
lookupfunction with two arguments is deprecated. - The condition and value access can be simplified.
Apply this diff to improve the code:
dynamic "pod_identity_association" {
- for_each = try(lookup(each.value, "pod_identity_association", null), null) != null ? [true] : []
+ for_each = try(each.value.pod_identity_association, null) != null ? [true] : []
content {
- role_arn = try(lookup(each.value.pod_identity_association, "role_arn"), null)
- service_account = try(lookup(each.value.pod_identity_association, "service_account"), null)
+ role_arn = try(each.value.pod_identity_association.role_arn, null)
+ service_account = try(each.value.pod_identity_association.service_account, null)
}
}The changes:
- Remove deprecated two-argument
lookupusage - Simplify attribute access using dot notation
- Keep the
tryfunction for safe access to potentially undefined values
📝 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.
| dynamic "pod_identity_association" { | |
| for_each = try(lookup(each.value, "pod_identity_association", null), null) != null ? [true] : [] | |
| content { | |
| role_arn = try(lookup(each.value.pod_identity_association, "role_arn"), null) | |
| service_account = try(lookup(each.value.pod_identity_association, "service_account"), null) | |
| } | |
| } | |
| dynamic "pod_identity_association" { | |
| for_each = try(each.value.pod_identity_association, null) != null ? [true] : [] | |
| content { | |
| role_arn = try(each.value.pod_identity_association.role_arn, null) | |
| service_account = try(each.value.pod_identity_association.service_account, null) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: terraform-module / CI / Lint (.)
[warning] 177-177:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:177:29: warning: Lookup with 2 arguments is deprecated ()
[warning] 178-178:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:178:29: warning: Lookup with 2 arguments is deprecated ()
| pod_identity_association = lookup(each.value, "pod_identity_association", null) | ||
|
|
||
| dynamic "pod_identity_association" { | ||
| for_each = try(lookup(each.value, "pod_identity_association", null), null) != null ? [true] : [] | ||
|
|
||
| content { | ||
| role_arn = try(lookup(each.value.pod_identity_association, "role_arn"), null) | ||
| service_account = try(lookup(each.value.pod_identity_association, "service_account"), null) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the results, I can now generate the final response:
Documentation needed for pod identity association feature
The implementation is functionally correct with proper AWS provider version (>= 5.75.0) support, but requires documentation updates:
- The pod identity association feature is not documented in the README.md
- The feature should be documented with:
- Description of the feature and its purpose
- Example usage showing the pod_identity_association configuration
- Any prerequisites or limitations
🔗 Analysis chain
Implementation aligns with PR objectives.
The changes successfully add support for pod identity associations in EKS addons while maintaining backward compatibility. This provides users with an alternative to IRSA as intended.
A few points to verify:
- Ensure the AWS provider version (>= 5.75) supports this feature
- Consider documenting the new feature in the module's README
Let me check the README content to ensure we have proper documentation for this feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify AWS provider version and documentation
# Check if README exists and contains pod identity documentation
fd -t f "README.md" -x grep -l "pod_identity_association"
# Check provider version constraints
fd -t f "versions.tf" -x grep -A 5 "aws"
Length of output: 606
Script:
#!/bin/bash
# Get full content of README.md to check for documentation
fd -t f "README.md" -x cat {}
# Also check for any existing examples that might need updates
fd -t f -e tf "examples" -x grep -l "addon"
Length of output: 54903
🧰 Tools
🪛 GitHub Check: terraform-module / CI / Lint (.)
[warning] 177-177:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:177:29: warning: Lookup with 2 arguments is deprecated ()
[warning] 178-178:
[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated
Raw Output:
main.tf:178:29: warning: Lookup with 2 arguments is deprecated ()
| pod_identity_association = optional(object({ | ||
| role_arn = string | ||
| service_account = string | ||
| })) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding validation and improving documentation.
The implementation of pod_identity_association is well-structured and maintains backward compatibility. A few suggestions for enhancement:
- Add validation for the
role_arnformat to ensure it's a valid IAM role ARN - Enhance the variable description to include details about pod identity associations
Consider adding these enhancements:
pod_identity_association = optional(object({
role_arn = string
service_account = string
-}))
+}), {
+ validation {
+ condition = can(regex("^arn:aws:iam::[0-9]{12}:role/.+", var.role_arn))
+ error_message = "The role_arn value must be a valid IAM role ARN."
+ }
+})Update the description field to include:
description = <<-EOT
Manages [`aws_eks_addon`](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_addon) resources.
Note: `resolve_conflicts` is deprecated. If `resolve_conflicts` is set and
`resolve_conflicts_on_create` or `resolve_conflicts_on_update` is not set,
`resolve_conflicts` will be used instead. If `resolve_conflicts_on_create` is
not set and `resolve_conflicts` is `PRESERVE`, `resolve_conflicts_on_create`
will be set to `NONE`.
+
+ The `pod_identity_association` block allows you to configure pod identity associations
+ for EKS addons as an alternative to IAM Roles for Service Accounts (IRSA).
+ This enables more flexible identity management for pods running EKS addons.
EOTCommittable suggestion skipped: line range outside the PR's diff.
|
hashicorp/terraform-provider-aws#30645 Linked issue can block migration of existing addons from IRSA to Pod Identity. I support this PR, but migration path, that it opens, might not be usable by end users as of now. Either documentation is needed or wait for until mentioned issue is resolved. |
|
I changed it to a draft for now. I figured if the new optional argument defaulted to null then it wouldn't cause issues with existing addons, so new addons would be able to take advantage. |
Co-authored-by: Ihor Urazov <[email protected]>
what
why
references
Summary by CodeRabbit
New Features
Updates
Deprecation
regionvariable as obsolete, indicating it is no longer needed for future configurations.