Skip to content

Conversation

@milldr
Copy link
Contributor

@milldr milldr commented Sep 18, 2025

what

  • Exclude Name from tags
  • Resolve tflint warnings

why

Services like echo-server were using k8s-common ALB instead of the provisioned alb-controller-ingress-group ALB, despite having ingress_type: "alb" configured.

The root cause was tag conflicts: the ALB Controller's defaultTags (from module.this.tags) applied Name: acme-plat-euc1-dev, while the ALB controller ingress group applied Name: acme-plat-euc1-dev-alb-controller-ingress-group. These conflicting Name tags prevented the AWS Load Balancer Controller from reconciling ingresses into the target group.

Ingresses should manage their own tags rather than inheriting the tags from this component.

references

Summary by CodeRabbit

  • Refactor
    • Updated Terraform to use consistent indexing for namespaces and annotations.
    • Adjusted tag generation to exclude the “Name” tag from Kubernetes and AWS resources.
  • Documentation
    • Added inline comments clarifying tag exclusions and indexing choices.
  • Chores
    • Added account-map/ to .gitignore to prevent committing local files.

@coderabbitai
Copy link

coderabbitai bot commented Sep 18, 2025

Walkthrough

Updated .gitignore to ignore the account-map/ directory. In Terraform, adjusted splat/indexing syntax for Kubernetes namespace and metadata lookups, and modified tag construction to exclude the "Name" tag. Added inline comments explaining these Terraform expression changes.

Changes

Cohort / File(s) Summary of Changes
VCS ignore updates
\.gitignore
Added account-map/ to ignored paths.
Terraform expression and tag filtering tweaks
src/main.tf
Switched default.*.id to default[*].id; changed metadata access to metadata[0].annotations; rebuilt kube_tags to exclude "Name"; added comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I thump my paws—ignore that map,
Tags trimmed neat: no “Name” to wrap.
Arrays aligned with brackets bright,
Annotations click just right.
In Terraform fields I hop with cheer,
Clean paths ahead—no clutter here! 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: Exclude Name tag to prevent ALB conflicts" succinctly and accurately describes the primary change—removing the Name tag to avoid AWS Load Balancer Controller/ALB conflicts—and aligns with the PR objectives and diffs. It is concise, uses the conventional commit prefix "fix:" to indicate intent, and avoids noisy details, so a teammate scanning PR history will understand the main purpose. Therefore the title meets the stated criteria.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/default-ingress-group

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@milldr
Copy link
Contributor Author

milldr commented Sep 18, 2025

/terratest

@mergify mergify bot requested review from a team September 18, 2025 18:10
@mergify mergify bot added the triage Needs triage label Sep 18, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (5)
.gitignore (1)

79-79: Ignore rule LGTM; consider anchoring to repo root and mirroring in .dockerignore

account-map/ will ignore any directory named account-map at any depth. If you intend only the repo‑root dir, prefer /account-map/. Also add the same to .dockerignore if applicable.

src/main.tf (4)

6-6: Use one(...) instead of join("", list) for single-element extraction

For consistency with later usage (one(data.aws_lb.default[*].name)), this reads clearer and asserts cardinality.

-  kubernetes_namespace = local.create_namespace ? join("", kubernetes_namespace.default[*].id) : var.kubernetes_namespace
+  kubernetes_namespace = local.create_namespace ? one(kubernetes_namespace.default[*].id) : var.kubernetes_namespace

27-30: Exclude “Name” tag: simplify comprehension and align formatting

Logic is correct. You can simplify the map/list comp and keep behavior identical. Optional: guard against accidental case variants if your tag source ever varies.

-  kube_tags = join(",", [for k, v in { for key, value in module.this.tags : key => value if key != "Name" } : "${k}=${v}"])
+  # Build "k=v,k2=v2" string from tags excluding the AWS-reserved/conflicting Name tag
+  kube_tags = join(
+    ",",
+    [for k, v in module.this.tags : "${k}=${v}" if k != "Name"]
+  )
+  # If your tag keys could be case-variant, consider: if lower(k) != "name"

Please verify none of the tag values contain commas or equal signs; ALB tag annotation parsing is CSV-like and will break on those.


201-204: Nit: Prefer one(...) over join("", ...) for LB ID as well

Not a blocker; improves clarity and parity with one(...) elsewhere.

-    endpoint_id                    = join("", data.aws_lb.default[*].id)
+    endpoint_id                    = one(data.aws_lb.default[*].id)

51-52: Side note: Kubernetes label constraints vs. module tags

Unchanged in this PR, but labels = module.this.tags may include keys like Name that aren’t valid Kubernetes label keys (must be DNS‑1123 compliant). If you haven’t already, consider a sanitized projection for labels.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5fb68ad and cdbe404.

📒 Files selected for processing (2)
  • .gitignore (1 hunks)
  • src/main.tf (2 hunks)
⏰ 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 (1)
src/main.tf (1)

36-41: Bracket indexing for metadata access LGTM

Switch to metadata[0].annotations resolves tflint/HCL indexing warnings and is robust behind try(...).

@milldr milldr added minor New features that do not break anything and removed triage Needs triage labels Sep 18, 2025
@milldr milldr added this pull request to the merge queue Sep 18, 2025
Merged via the queue into main with commit 6e6663a Sep 18, 2025
19 checks passed
@milldr milldr deleted the fix/default-ingress-group branch September 18, 2025 19:34
@github-actions
Copy link

These changes were released in v1.537.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor New features that do not break anything

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants