Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: allow eks addons to use pod identity associations #244

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feat: allow eks addons to use pod identity associations #244

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
29af931
feat: add pod_identity_association
nitrocode Nov 17, 2024
ee8d211
feat: add pod_identity_association
nitrocode Nov 17, 2024
dc44e10
feat: allow pod identity config block in eks addons
nitrocode Nov 17, 2024
d510b58
feat: bump to v5.75.0
nitrocode Nov 17, 2024
2989569
feat: bump to v5.75.0
nitrocode Nov 17, 2024
f422d4d
Apply suggestions from code review
nitrocode Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/complete/versions.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = ">= 5.74"
version = ">= 5.75"
nitrocode marked this conversation as resolved.
Show resolved Hide resolved
}
kubernetes = {
source = "hashicorp/kubernetes"
Expand Down
11 changes: 11 additions & 0 deletions main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,17 @@
resolve_conflicts_on_update = lookup(each.value, "resolve_conflicts_on_update", lookup(each.value, "resolve_conflicts", null))
service_account_role_arn = lookup(each.value, "service_account_role_arn", null)

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)

Check warning on line 177 in main.tf

View workflow job for this annotation

GitHub Actions / terraform-module / CI / Lint (.) 

[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated

Raw Output:
main.tf:177:29: warning: Lookup with 2 arguments is deprecated ()
service_account = try(lookup(each.value.pod_identity_association, "service_account"), null)

Check warning on line 178 in main.tf

View workflow job for this annotation

GitHub Actions / terraform-module / CI / Lint (.) 

[tflint] reported by reviewdog 🐶
Lookup with 2 arguments is deprecated

Raw Output:
main.tf:178:29: warning: Lookup with 2 arguments is deprecated ()
}
}
Comment on lines +173 to +180
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor to address deprecated lookup usage and simplify conditions.

The implementation works but can be improved:

  1. The lookup function with two arguments is deprecated.
  2. 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 lookup usage
  • Simplify attribute access using dot notation
  • Keep the try function 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.

Suggested change
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 ()

Comment on lines +171 to +180
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. Ensure the AWS provider version (>= 5.75) supports this feature
  2. 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 ()


tags = module.label.tags

depends_on = [
Expand Down
5 changes: 4 additions & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
# tflint-ignore: terraform_unused_declarations
variable "region" {

type = string
description = "OBSOLETE (not needed): AWS Region"
default = null
Expand Down Expand Up @@ -175,6 +174,10 @@ variable "addons" {
create_timeout = optional(string, null)
update_timeout = optional(string, null)
delete_timeout = optional(string, null)
pod_identity_association = optional(object({
role_arn = string
service_account = string
}))
Comment on lines +177 to +180
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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:

  1. Add validation for the role_arn format to ensure it's a valid IAM role ARN
  2. 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.
 EOT

Committable suggestion skipped: line range outside the PR's diff.

}))
description = <<-EOT
Manages [`aws_eks_addon`](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_addon) resources.
Expand Down
2 changes: 1 addition & 1 deletion versions.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ terraform {
required_providers {
aws = {
source = "hashicorp/aws"
version = ">= 5.74.0"
version = ">= 5.75.0"
nitrocode marked this conversation as resolved.
Show resolved Hide resolved
}
tls = {
source = "hashicorp/tls"
Expand Down
Loading