fix(cpo): diff SG permissions before AuthorizeSecurityGroupIngress

control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go

@@ -1016,7 +1016,7 @@ func (r *AWSEndpointServiceReconciler) reconcileAWSEndpointSecurityGroup(ctx con
 		machineCIDRs[i] = mNet.CIDR.String()
 	}
 	ingressPermissions := supportawsutil.VPCEndpointSecurityGroupRules(machineCIDRs, vpcEndpointPort(awsEndpointService))
-	missingPermissions := diffPermissions(sg.IpPermissions, ingressPermissions)
+	missingPermissions := supportawsutil.DiffPermissions(sg.IpPermissions, ingressPermissions)
 	if len(missingPermissions) > 0 {
 		if _, err = ec2Client.AuthorizeSecurityGroupIngress(ctx, &ec2.AuthorizeSecurityGroupIngressInput{
 			GroupId:       aws.String(sgID),
@@ -1328,37 +1328,3 @@ func (r *AWSEndpointServiceReconciler) deleteSecurityGroup(ctx context.Context,
 	return nil
 }
 
-func diffPermissions(actual, required []ec2types.IpPermission) []ec2types.IpPermission {
-	var result []ec2types.IpPermission
-	for _, req := range required {
-		if !isPermissionPresent(req, actual) {
-			result = append(result, req)
-		}
-	}
-	return result
-}
-
-func isPermissionPresent(perm ec2types.IpPermission, list []ec2types.IpPermission) bool {
-	for _, existing := range list {
-		if aws.ToInt32(existing.FromPort) == aws.ToInt32(perm.FromPort) &&
-			aws.ToInt32(existing.ToPort) == aws.ToInt32(perm.ToPort) &&
-			aws.ToString(existing.IpProtocol) == aws.ToString(perm.IpProtocol) &&
-			equalIPRanges(existing.IpRanges, perm.IpRanges) {
-			return true
-		}
-	}
-	return false
-}
-
-func equalIPRanges(a, b []ec2types.IpRange) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	for i := range a {
-		if aws.ToString(a[i].Description) != aws.ToString(b[i].Description) ||
-			aws.ToString(a[i].CidrIp) != aws.ToString(b[i].CidrIp) {
-			return false
-		}
-	}
-	return true
-}

control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller_test.go

@@ -12,6 +12,7 @@ import (
 
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
 	"github.com/openshift/hypershift/support/awsapi"
+	supportawsutil "github.com/openshift/hypershift/support/awsutil"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	ec2v2 "github.com/aws/aws-sdk-go-v2/service/ec2"
@@ -333,7 +334,7 @@ func TestDiffPermissions(t *testing.T) {
 	for i, test := range tests {
 		t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) {
 			g := NewGomegaWithT(t)
-			result := diffPermissions(test.actual, test.required)
+			result := supportawsutil.DiffPermissions(test.actual, test.required)
 			g.Expect(result).To(Equal(test.expected))
 		})
 	}

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -2787,15 +2787,20 @@ func createAWSDefaultSecurityGroup(ctx context.Context, ec2Client awsapi.EC2API,
 	}
 
 	ingressPermissions := supportawsutil.DefaultWorkerSGIngressRules(machineCIDRs, sgID, aws.ToString(sg.OwnerId))
-	_, err = ec2Client.AuthorizeSecurityGroupIngress(ctx, &ec2.AuthorizeSecurityGroupIngressInput{
-		GroupId:       aws.String(sgID),
-		IpPermissions: ingressPermissions,
-	})
-	if err != nil {
-		if supportawsutil.AWSErrorCode(err) != "InvalidPermission.Duplicate" {
-			return "", nil, fmt.Errorf("failed to set security group ingress rules, code: %s", supportawsutil.AWSErrorCode(err))
+	missingPermissions := supportawsutil.DiffPermissions(sg.IpPermissions, ingressPermissions)
+	if len(missingPermissions) > 0 {
+		_, err = ec2Client.AuthorizeSecurityGroupIngress(ctx, &ec2.AuthorizeSecurityGroupIngressInput{
+			GroupId:       aws.String(sgID),
+			IpPermissions: missingPermissions,
+		})
+		if err != nil {
+			if supportawsutil.AWSErrorCode(err) != "InvalidPermission.Duplicate" {
+				return "", nil, fmt.Errorf("failed to set security group ingress rules, code: %s", supportawsutil.AWSErrorCode(err))
+			}
+			logger.Info("WARNING: got duplicate permissions error when setting security group ingress permissions", "sgID", sgID)
 		}
-		logger.Info("WARNING: got duplicate permissions error when setting security group ingress permissions", "sgID", sgID)
+	} else {
+		logger.Info("security group already has required ingress permissions", "sgID", sgID)
 	}
 	return sgID, tags, nil
 }

support/awsutil/sg.go

@@ -290,3 +290,54 @@ func EC2TagsToMap(tags []ec2types.Tag) map[string]string {
 	}
 	return m
 }
+
+// DiffPermissions returns the permissions from required that are not present in actual.
+func DiffPermissions(actual, required []ec2types.IpPermission) []ec2types.IpPermission {
+	var result []ec2types.IpPermission
+	for _, req := range required {
+		if !isPermissionPresent(req, actual) {
+			result = append(result, req)
+		}
+	}
+	return result
+}
+
+func isPermissionPresent(perm ec2types.IpPermission, list []ec2types.IpPermission) bool {
+	for _, existing := range list {
+		if aws.ToInt32(existing.FromPort) == aws.ToInt32(perm.FromPort) &&
+			aws.ToInt32(existing.ToPort) == aws.ToInt32(perm.ToPort) &&
+			aws.ToString(existing.IpProtocol) == aws.ToString(perm.IpProtocol) &&
+			equalIPRanges(existing.IpRanges, perm.IpRanges) &&
+			equalUserIdGroupPairs(existing.UserIdGroupPairs, perm.UserIdGroupPairs) {
+			return true
+		}
+	}
+	return false
+}
+
+func equalIPRanges(a, b []ec2types.IpRange) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if aws.ToString(a[i].Description) != aws.ToString(b[i].Description) ||
+			aws.ToString(a[i].CidrIp) != aws.ToString(b[i].CidrIp) {
+			return false
+		}
+	}
+	return true
+}
+
+func equalUserIdGroupPairs(a, b []ec2types.UserIdGroupPair) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i := range a {
+		if aws.ToString(a[i].GroupId) != aws.ToString(b[i].GroupId) ||
+			aws.ToString(a[i].UserId) != aws.ToString(b[i].UserId) ||
+			aws.ToString(a[i].Description) != aws.ToString(b[i].Description) {
+			return false
+		}
+	}
+	return true
+}

support/awsutil/sg_test.go

@@ -0,0 +1,153 @@
+package awsutil
+
+import (
+	"testing"
+
+	"github.com/aws/aws-sdk-go-v2/aws"
+	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
+)
+
+func TestDiffPermissions(t *testing.T) {
+	ipRange := func(desc, cidr string) ec2types.IpRange {
+		return ec2types.IpRange{
+			Description: aws.String(desc),
+			CidrIp:      aws.String(cidr),
+		}
+	}
+
+	groupPair := func(groupID, userID, desc string) ec2types.UserIdGroupPair {
+		return ec2types.UserIdGroupPair{
+			GroupId:     aws.String(groupID),
+			UserId:     aws.String(userID),
+			Description: aws.String(desc),
+		}
+	}
+
+	ipRangePerm := func(from, to int32, protocol string, ranges ...ec2types.IpRange) ec2types.IpPermission {
+		return ec2types.IpPermission{
+			FromPort:   aws.Int32(from),
+			ToPort:     aws.Int32(to),
+			IpProtocol: aws.String(protocol),
+			IpRanges:   ranges,
+		}
+	}
+
+	groupPairPerm := func(from, to int32, protocol string, pairs ...ec2types.UserIdGroupPair) ec2types.IpPermission {
+		return ec2types.IpPermission{
+			FromPort:         aws.Int32(from),
+			ToPort:           aws.Int32(to),
+			IpProtocol:       aws.String(protocol),
+			UserIdGroupPairs: pairs,
+		}
+	}
+
+	tests := []struct {
+		name     string
+		actual   []ec2types.IpPermission
+		required []ec2types.IpPermission
+		expected []ec2types.IpPermission
+	}{
+		{
+			name:   "When actual is empty it should return all required permissions",
+			actual: nil,
+			required: []ec2types.IpPermission{
+				ipRangePerm(443, 443, "tcp", ipRange("Control plane service", "10.0.0.0/16")),
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+			},
+			expected: []ec2types.IpPermission{
+				ipRangePerm(443, 443, "tcp", ipRange("Control plane service", "10.0.0.0/16")),
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+			},
+		},
+		{
+			name: "When all IpRange permissions already exist it should return none",
+			actual: []ec2types.IpPermission{
+				ipRangePerm(443, 443, "tcp", ipRange("Control plane service", "10.0.0.0/16")),
+				ipRangePerm(6443, 6443, "tcp", ipRange("Control plane service", "10.0.0.0/16")),
+			},
+			required: []ec2types.IpPermission{
+				ipRangePerm(443, 443, "tcp", ipRange("Control plane service", "10.0.0.0/16")),
+			},
+			expected: nil,
+		},
+		{
+			name: "When all UserIdGroupPair permissions already exist it should return none",
+			actual: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+			},
+			required: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+			},
+			expected: nil,
+		},
+		{
+			name: "When some UserIdGroupPair permissions are missing it should return only the missing ones",
+			actual: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+			},
+			required: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+				groupPairPerm(10250, 10250, "tcp", groupPair("sg-123", "111111111111", "Kubelet")),
+			},
+			expected: []ec2types.IpPermission{
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+				groupPairPerm(10250, 10250, "tcp", groupPair("sg-123", "111111111111", "Kubelet")),
+			},
+		},
+		{
+			name: "When mixed IpRange and UserIdGroupPair permissions exist it should correctly diff both types",
+			actual: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+				ipRangePerm(22, 22, "tcp", ipRange("SSH", "10.0.0.0/16")),
+			},
+			required: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+				ipRangePerm(22, 22, "tcp", ipRange("SSH", "10.0.0.0/16")),
+				ipRangePerm(-1, -1, "icmp", ipRange("ICMP", "10.0.0.0/16")),
+			},
+			expected: []ec2types.IpPermission{
+				groupPairPerm(6081, 6081, "udp", groupPair("sg-123", "111111111111", "GENEVE Protocol")),
+				ipRangePerm(-1, -1, "icmp", ipRange("ICMP", "10.0.0.0/16")),
+			},
+		},
+		{
+			name: "When UserIdGroupPair has different group ID it should detect as missing",
+			actual: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-999", "111111111111", "VXLAN Packets")),
+			},
+			required: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+			},
+			expected: []ec2types.IpPermission{
+				groupPairPerm(4789, 4789, "udp", groupPair("sg-123", "111111111111", "VXLAN Packets")),
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := DiffPermissions(tt.actual, tt.required)
+			if len(tt.expected) == 0 && len(result) == 0 {
+				return
+			}
+			if len(result) != len(tt.expected) {
+				t.Fatalf("expected %d missing permissions, got %d", len(tt.expected), len(result))
+			}
+			for i := range tt.expected {
+				if aws.ToInt32(result[i].FromPort) != aws.ToInt32(tt.expected[i].FromPort) ||
+					aws.ToInt32(result[i].ToPort) != aws.ToInt32(tt.expected[i].ToPort) ||
+					aws.ToString(result[i].IpProtocol) != aws.ToString(tt.expected[i].IpProtocol) {
+					t.Errorf("permission %d mismatch: got port %d-%d/%s, want port %d-%d/%s",
+						i,
+						aws.ToInt32(result[i].FromPort), aws.ToInt32(result[i].ToPort), aws.ToString(result[i].IpProtocol),
+						aws.ToInt32(tt.expected[i].FromPort), aws.ToInt32(tt.expected[i].ToPort), aws.ToString(tt.expected[i].IpProtocol),
+					)
+				}
+			}
+		})
+	}
+}