Skip to content

fix: handle nil AWS ALB entries #758

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

Open
cyn8 wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix: handle nil AWS ALB entries #758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 pkg/providers/aws/alb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func (ep *elbV2Provider) listELBV2Resources(albClient *elbv2.ELBV2, ec2Client *e
}

for _, lb := range loadBalancers {
if lb.DNSName == nil || lb.LoadBalancerName == nil {
if lb == nil || lb.DNSName == nil || lb.LoadBalancerName == nil {

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject empty DNS values, not only nil pointers.

The guard on Line 77 only checks whether DNSName is non-nil. aws.String("") is non-nil. If LoadBalancerName is populated, the loop emits a resource with an empty DNSName.

Check the pointed-to DNS value before constructing the resource.

Proposed fix 
-		if lb == nil || lb.DNSName == nil || lb.LoadBalancerName == nil {
+		if lb == nil || lb.DNSName == nil || *lb.DNSName == "" || lb.LoadBalancerName == nil {

As per coding guidelines, each emitted resource must have at least one of IP or DNS populated; empty resources are invalid.

📝 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
if lb == nil || lb.DNSName == nil || lb.LoadBalancerName == nil {
if lb == nil || lb.DNSName == nil || *lb.DNSName == "" || lb.LoadBalancerName == nil {
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/providers/aws/alb.go` at line 77, Update the guard in the load balancer
processing loop to reject both nil and empty pointed-to DNSName values before
constructing the resource. Preserve the existing LoadBalancerName validation and
ensure emitted resources never contain an empty DNSName.

Source: Coding guidelines

continue
}
albDNS := *lb.DNSName
Expand Down
7 changes: 4 additions & 3 deletions pkg/providers/aws/alb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (
func processLoadBalancersForTest(lbs []*elbv2.LoadBalancer) *schema.Resources {
list := schema.NewResources()
for _, lb := range lbs {
if lb.DNSName == nil || lb.LoadBalancerName == nil {
if lb == nil || lb.DNSName == nil || lb.LoadBalancerName == nil {

@coderabbitai coderabbitai Bot Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Exercise the production listing path.

processLoadBalancersForTest duplicates the filtering logic, and TestListELBV2Resources_NilLoadBalancer invokes that helper instead of listELBV2Resources. The test can pass even if the production method still dereferences a nil lb.

Use a mocked ELBv2 client to call the production method, or extract the shared filtering logic into a production helper. Express the nil and incomplete cases as table-driven inputs.

As per coding guidelines, provider tests should prefer mocked cloud provider clients and use table-driven tests for resource parsing.

Also applies to: 44-48

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/providers/aws/alb_test.go` at line 18, Update
TestListELBV2Resources_NilLoadBalancer to exercise listELBV2Resources through a
mocked ELBv2 client instead of processLoadBalancersForTest, so production nil
handling is validated. Convert the nil and incomplete load-balancer scenarios
into table-driven inputs while preserving coverage for nil entries and missing
DNSName or LoadBalancerName.

Source: Coding guidelines

continue
}
list.Append(&schema.Resource{
Expand All @@ -41,10 +41,11 @@ func processTargetsForTest(targets []*elbv2.TargetHealthDescription) []string {
return ids
}

func TestListELBV2Resources_NilDNSName(t *testing.T) {
func TestListELBV2Resources_NilLoadBalancer(t *testing.T) {
t.Parallel()

lbs := []*elbv2.LoadBalancer{
nil,
{
DNSName: nil,
LoadBalancerName: aws.String("internal-lb"),
Expand All @@ -58,7 +59,7 @@ func TestListELBV2Resources_NilDNSName(t *testing.T) {

require.NotPanics(t, func() {
resources := processLoadBalancersForTest(lbs)
assert.Equal(t, 0, len(resources.Items), "nil DNSName or LoadBalancerName LBs must be skipped")
assert.Equal(t, 0, len(resources.Items), "nil or incomplete load balancers must be skipped")
})
}

Expand Down