Merge pull request #1207 from mtulio/fix-1206-update-sg

fix/lb SG rule update failures when service port is changed

Author: k8s-ci-robot

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")
+		}
+	})
+}