fix: rule difference for nested objects

The Difference method calculation are returning wrong values on service
update calculating the security group rules.

The core issue is caused by it is always comparing the pointer address,
not the values, always generating difference between lists.

Example:
given s1:{a1, a2} and s2:{a2,a3,a4}
s2.Difference(s1) was returning {a2, a3, a4}, instead {a3, a4}

Author: mtulio

pkg/providers/v1/sets_ippermissions.go

@@ -141,18 +141,24 @@ func (s IPPermissionSet) Equal(s2 IPPermissionSet) bool {
 	return len(s) == len(s2) && s.IsSuperset(s2)
 }
 
-// Difference returns a set of objects that are not in s2
+// Difference returns a set of objects that are not in s2.
 // For example:
 // s1 = {a1, a2, a3}
 // s2 = {a1, a2, a4, a5}
 // s1.Difference(s2) = {a3}
 // s2.Difference(s1) = {a4, a5}
 func (s IPPermissionSet) Difference(s2 IPPermissionSet) IPPermissionSet {
 	result := NewIPPermissionSet()
-	for k, v := range s {
-		_, found := s2[k]
+	for _, desired := range s.List() {
+		found := false
+		for _, existing := range s2.List() {
+			if ipPermissionExists(&desired, &existing, false) {
+				found = true
+				break
+			}
+		}
 		if !found {
-			result[k] = v
+			result.Insert(desired)
 		}
 	}
 	return result

pkg/providers/v1/sets_ippermissions_test.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestUngroup(t *testing.T) {
@@ -156,3 +157,264 @@ func TestUngroup(t *testing.T) {
 		})
 	}
 }
+
+func TestIPPermissionSetDifferenceCriticalScenarios(t *testing.T) {
+	t.Run("real_world_nlb_security_group_scenario", func(t *testing.T) {
+		// Scenario:
+		// Desired: tcp:80, tcp:81, icmp:3-4
+		// Actual: tcp:80, icmp:3-4
+		// Expected: add tcp:81 only, remove nothing
+
+		desired := NewIPPermissionSet(
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(80),
+				ToPort:     aws.Int32(80),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(81),
+				ToPort:     aws.Int32(81),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+			ec2types.IpPermission{
+				IpProtocol: aws.String("icmp"),
+				FromPort:   aws.Int32(3),
+				ToPort:     aws.Int32(4),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+		)
+
+		actual := NewIPPermissionSet(
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(80),
+				ToPort:     aws.Int32(80),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+			ec2types.IpPermission{
+				IpProtocol: aws.String("icmp"),
+				FromPort:   aws.Int32(3),
+				ToPort:     aws.Int32(4),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+		)
+
+		// Calculate what should be added and removed
+		add := desired.Difference(actual)
+		remove := actual.Difference(desired)
+
+		// Verify correct results
+		assert.Equal(t, 1, add.Len(), "Should add exactly one permission (tcp:81)")
+		assert.Equal(t, 0, remove.Len(), "Should remove no permissions")
+
+		// Verify the added permission is tcp:81
+		addList := add.List()
+		if len(addList) > 0 {
+			perm := addList[0]
+			assert.Equal(t, "tcp", aws.ToString(perm.IpProtocol))
+			assert.Equal(t, int32(81), aws.ToInt32(perm.FromPort))
+			assert.Equal(t, int32(81), aws.ToInt32(perm.ToPort))
+		}
+	})
+
+	t.Run("empty_sets_and_edge_cases", func(t *testing.T) {
+		// Test edge cases with empty sets and nil scenarios
+
+		emptySet := NewIPPermissionSet()
+		nonEmptySet := NewIPPermissionSet(
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(443),
+				ToPort:     aws.Int32(443),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("10.0.0.0/8")},
+				},
+			},
+		)
+
+		// Empty - NonEmpty should return empty
+		diff1 := emptySet.Difference(nonEmptySet)
+		assert.Equal(t, 0, diff1.Len(), "Empty set difference with non-empty should be empty")
+
+		// NonEmpty - Empty should return all from NonEmpty
+		diff2 := nonEmptySet.Difference(emptySet)
+		assert.Equal(t, 1, diff2.Len(), "Non-empty set difference with empty should return all permissions")
+
+		// Empty - Empty should return empty
+		diff3 := emptySet.Difference(emptySet)
+		assert.Equal(t, 0, diff3.Len(), "Empty set difference with empty should be empty")
+	})
+
+	t.Run("initialization_issue_prevention", func(t *testing.T) {
+		// Test that demonstrates the importance of proper initialization
+		// This prevents the bug where variables were declared as `var add IPPermissionSet`
+
+		sourceSet := NewIPPermissionSet(
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(80),
+				ToPort:     aws.Int32(80),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+		)
+
+		// Test with properly initialized empty set
+		emptySet := NewIPPermissionSet()
+		diff := sourceSet.Difference(emptySet)
+		assert.Equal(t, 1, diff.Len(), "Difference with properly initialized empty set should work")
+
+		// Test that uninitialized set doesn't cause panic in Difference operation
+		var uninitializedSet IPPermissionSet
+		defer func() {
+			if r := recover(); r != nil {
+				t.Errorf("Difference operation should not panic with uninitialized set: %v", r)
+			}
+		}()
+
+		// This should not panic (though behavior may be undefined)
+		_ = sourceSet.Difference(uninitializedSet)
+	})
+
+	t.Run("multiple_ip_ranges_scenario", func(t *testing.T) {
+		// Test complex permissions with multiple IP ranges to ensure
+		// the Difference function handles them correctly
+
+		desired := NewIPPermissionSet(
+			// Permission with multiple IP ranges
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(443),
+				ToPort:     aws.Int32(443),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("10.0.0.0/8")},
+					{CidrIp: aws.String("172.16.0.0/12")},
+					{CidrIp: aws.String("192.168.0.0/16")},
+				},
+			},
+			// Single IP range permission
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(80),
+				ToPort:     aws.Int32(80),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("0.0.0.0/0")},
+				},
+			},
+		)
+
+		actual := NewIPPermissionSet(
+			// Same permission with multiple IP ranges (should match)
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(443),
+				ToPort:     aws.Int32(443),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("10.0.0.0/8")},
+					{CidrIp: aws.String("172.16.0.0/12")},
+					{CidrIp: aws.String("192.168.0.0/16")},
+				},
+			},
+			// Different permission with multiple IP ranges
+			ec2types.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int32(8080),
+				ToPort:     aws.Int32(8080),
+				IpRanges: []ec2types.IpRange{
+					{CidrIp: aws.String("10.0.0.0/8")},
+					{CidrIp: aws.String("172.16.0.0/12")},
+				},
+			},
+		)
+
+		// Calculate differences
+		add := desired.Difference(actual)
+		remove := actual.Difference(desired)
+
+		// Should add tcp:80 (not present in actual)
+		assert.Equal(t, 1, add.Len(), "Should add exactly one permission (tcp:80)")
+
+		// Should remove tcp:8080 (not present in desired)
+		assert.Equal(t, 1, remove.Len(), "Should remove exactly one permission (tcp:8080)")
+
+		// Verify what's being added
+		addList := add.List()
+		if len(addList) > 0 {
+			perm := addList[0]
+			assert.Equal(t, "tcp", aws.ToString(perm.IpProtocol))
+			assert.Equal(t, int32(80), aws.ToInt32(perm.FromPort))
+			assert.Equal(t, int32(80), aws.ToInt32(perm.ToPort))
+			assert.Equal(t, 1, len(perm.IpRanges), "Should have one IP range")
+			assert.Equal(t, "0.0.0.0/0", aws.ToString(perm.IpRanges[0].CidrIp))
+		}
+
+		// Verify what's being removed
+		removeList := remove.List()
+		if len(removeList) > 0 {
+			perm := removeList[0]
+			assert.Equal(t, "tcp", aws.ToString(perm.IpProtocol))
+			assert.Equal(t, int32(8080), aws.ToInt32(perm.FromPort))
+			assert.Equal(t, int32(8080), aws.ToInt32(perm.ToPort))
+			assert.Equal(t, 2, len(perm.IpRanges), "Should have two IP ranges")
+		}
+	})
+
+	t.Run("identical_permissions_different_ip_range_order", func(t *testing.T) {
+		// Test that permissions with same IP ranges but in different order
+		// are treated as identical (this tests the robustness of the key generation)
+
+		perm1 := ec2types.IpPermission{
+			IpProtocol: aws.String("tcp"),
+			FromPort:   aws.Int32(443),
+			ToPort:     aws.Int32(443),
+			IpRanges: []ec2types.IpRange{
+				{CidrIp: aws.String("10.0.0.0/8")},
+				{CidrIp: aws.String("172.16.0.0/12")},
+				{CidrIp: aws.String("192.168.0.0/16")},
+			},
+		}
+
+		perm2 := ec2types.IpPermission{
+			IpProtocol: aws.String("tcp"),
+			FromPort:   aws.Int32(443),
+			ToPort:     aws.Int32(443),
+			IpRanges: []ec2types.IpRange{
+				{CidrIp: aws.String("192.168.0.0/16")}, // Different order
+				{CidrIp: aws.String("10.0.0.0/8")},
+				{CidrIp: aws.String("172.16.0.0/12")},
+			},
+		}
+
+		set1 := NewIPPermissionSet(perm1)
+		set2 := NewIPPermissionSet(perm2)
+
+		// These should be different due to different order in JSON marshaling
+		// (This tests the current behavior - if this fails, it indicates the key generation
+		// doesn't account for order, which might be the root cause of issues)
+		diff := set1.Difference(set2)
+
+		// Log the result to understand current behavior
+		t.Logf("Difference between permissions with same IP ranges in different order: %d", diff.Len())
+
+		// The current implementation might treat these as different due to JSON marshaling
+		// This test documents the current behavior and will help identify if this is the issue
+		if diff.Len() == 0 {
+			t.Log("Permissions with different IP range order are treated as identical")
+		} else {
+			t.Log("Permissions with different IP range order are treated as different")
+		}
+	})
+}