Fix NLB IP version selection to respect service ipFamilies

.github/workflows/publish-fix-nlb-ipfamilies.yaml

@@ -0,0 +1,55 @@
+name: Publish fix-nlb-ipfamilies image
+
+on:
+  workflow_dispatch: {}
+  push:
+    branches:
+      - fix-nlb-ipfamilies
+    paths:
+      - ".github/workflows/publish-fix-nlb-ipfamilies.yaml"
+      - "cmd/**"
+      - "image/**"
+      - "pkg/**"
+      - "scripts/**"
+      - "Dockerfile_arm_all"
+      - "go.mod"
+      - "go.sum"
+
+permissions:
+  contents: read
+  packages: write
+
+jobs:
+  publish-arm64:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      - name: Set up QEMU
+        uses: docker/setup-qemu-action@v3
+        with:
+          platforms: arm64
+
+      - name: Set up Docker Buildx
+        uses: docker/setup-buildx-action@v3
+
+      - name: Log in to GHCR
+        uses: docker/login-action@v3
+        with:
+          registry: ghcr.io
+          username: ${{ github.actor }}
+          password: ${{ secrets.GITHUB_TOKEN }}
+
+      - name: Build and push arm64 image
+        uses: docker/build-push-action@v6
+        with:
+          context: .
+          file: Dockerfile_arm_all
+          platforms: linux/arm64
+          push: true
+          build-args: |
+            COMPONENT=oci-cloud-controller-manager oci-volume-provisioner oci-flexvolume-driver oci-csi-controller-driver oci-csi-node-driver
+          tags: |
+            ghcr.io/kab1r/oci-cloud-controller-manager:fix-nlb-ipfamilies
+            ghcr.io/kab1r/oci-cloud-controller-manager:fix-nlb-ipfamilies-${{ github.sha }}

pkg/cloudprovider/providers/oci/load_balancer.go

@@ -2244,19 +2244,20 @@ func (cp *CloudProvider) getLbListenerBackendSetIpVersion(ipFamilies []string, i
 		}
 		return []string{IPv4, IPv6}, nil
 	case string(v1.IPFamilyPolicyPreferDualStack):
-		if errIPv6Subnet != nil && errIPv4Subnet != nil {
-			// should never happen
-			return nil, errors.New("subnet does not have IPv4 or IPv6 cidr, can't create loadbalancer")
+		// ipFamilies is the authoritative source of what IP families the service uses.
+		// Only include IP versions that are in ipFamilies AND supported by the subnet.
+		var result []string
+		for _, ipFamily := range ipFamilies {
+			if ipFamily == IPv4 && errIPv4Subnet == nil {
+				result = append(result, IPv4)
+			} else if ipFamily == IPv6 && errIPv6Subnet == nil {
+				result = append(result, IPv6)
+			}
 		}
-		if errIPv6Subnet != nil {
-			cp.logger.Warn("subnet provided does not have IPv6 subnet CIDR block, creating listeners and backends of ip-version IPv4")
-			return []string{IPv4}, nil
+		if len(result) == 0 {
+			return nil, errors.New("no compatible IP families between service spec and subnet capabilities")
 		}
-		if errIPv4Subnet != nil {
-			cp.logger.Warn("subnet provided does not have IPV4 subnet CIDR block, creating listeners and backends of ip-version IPv6")
-			return []string{IPv6}, nil
-		}
-		return []string{IPv4, IPv6}, nil
+		return result, nil
 	}
 	return []string{IPv4}, nil
 }

pkg/cloudprovider/providers/oci/load_balancer_security_lists.go

@@ -116,6 +116,7 @@ func (s *baseSecurityListManager) updateBackendRules(ctx context.Context, lbSubn
 		logger := s.logger.With("securityListID", *secList.Id)
 
 		ingressRules := getNodeIngressRules(logger, secList.IngressSecurityRules, lbSubnets, actualPorts, desiredPorts, s.serviceLister, sourceCIDRs, isPreserveSource, ipFamilies)
+		ingressRules = dedupeIngressSecurityRules(logger, ingressRules)
 
 		if !securityListRulesChanged(secList, ingressRules, secList.EgressSecurityRules) {
 			logger.Debug("No changes for node subnet security list")
@@ -155,11 +156,13 @@ func (s *baseSecurityListManager) updateLoadBalancerRules(ctx context.Context, l
 
 		lbEgressRules := getLoadBalancerEgressRules(logger, secList.EgressSecurityRules, nodeSubnets, currentBackEndPort, desiredPorts.BackendPort, s.serviceLister, ipFamilies)
 		lbEgressRules = getLoadBalancerEgressRules(logger, lbEgressRules, nodeSubnets, currentHealthCheck, desiredPorts.HealthCheckerPort, s.serviceLister, ipFamilies)
+		lbEgressRules = dedupeEgressSecurityRules(logger, lbEgressRules)
 
 		lbIngressRules := secList.IngressSecurityRules
 		if desiredPorts.ListenerPort != 0 {
 			lbIngressRules = getLoadBalancerIngressRules(logger, lbIngressRules, sourceCIDRs, desiredPorts.ListenerPort, s.serviceLister)
 		}
+		lbIngressRules = dedupeIngressSecurityRules(logger, lbIngressRules)
 
 		if !securityListRulesChanged(secList, lbIngressRules, lbEgressRules) {
 			logger.Debug("No changes for load balancer subnet security list")
@@ -329,6 +332,133 @@ func securityListRulesChanged(securityList *core.SecurityList, ingressRules []co
 	return false
 }
 
+func dedupeIngressSecurityRules(logger *zap.SugaredLogger, rules []core.IngressSecurityRule) []core.IngressSecurityRule {
+	seen := map[string]struct{}{}
+	deduped := make([]core.IngressSecurityRule, 0, len(rules))
+	for _, rule := range rules {
+		key := ingressSecurityRuleKey(rule)
+		if _, ok := seen[key]; ok {
+			logger.With(
+				"source", stringPtrKey(rule.Source),
+				"sourceType", ingressSourceTypeKey(rule.SourceType),
+				"protocol", stringPtrKey(rule.Protocol),
+				"tcpOptions", tcpOptionsKey(rule.TcpOptions),
+				"udpOptions", udpOptionsKey(rule.UdpOptions),
+				"icmpOptions", icmpOptionsKey(rule.IcmpOptions),
+			).Debug("Dropping duplicate ingress security rule")
+			continue
+		}
+		seen[key] = struct{}{}
+		deduped = append(deduped, rule)
+	}
+	return deduped
+}
+
+func dedupeEgressSecurityRules(logger *zap.SugaredLogger, rules []core.EgressSecurityRule) []core.EgressSecurityRule {
+	seen := map[string]struct{}{}
+	deduped := make([]core.EgressSecurityRule, 0, len(rules))
+	for _, rule := range rules {
+		key := egressSecurityRuleKey(rule)
+		if _, ok := seen[key]; ok {
+			logger.With(
+				"destination", stringPtrKey(rule.Destination),
+				"destinationType", egressDestinationTypeKey(rule.DestinationType),
+				"protocol", stringPtrKey(rule.Protocol),
+				"tcpOptions", tcpOptionsKey(rule.TcpOptions),
+				"udpOptions", udpOptionsKey(rule.UdpOptions),
+				"icmpOptions", icmpOptionsKey(rule.IcmpOptions),
+			).Debug("Dropping duplicate egress security rule")
+			continue
+		}
+		seen[key] = struct{}{}
+		deduped = append(deduped, rule)
+	}
+	return deduped
+}
+
+func ingressSecurityRuleKey(rule core.IngressSecurityRule) string {
+	return fmt.Sprintf(
+		"stateless=%s|protocol=%s|source=%s|sourceType=%s|icmp=%s|tcp=%s|udp=%s",
+		boolPtrKey(rule.IsStateless),
+		stringPtrKey(rule.Protocol),
+		stringPtrKey(rule.Source),
+		ingressSourceTypeKey(rule.SourceType),
+		icmpOptionsKey(rule.IcmpOptions),
+		tcpOptionsKey(rule.TcpOptions),
+		udpOptionsKey(rule.UdpOptions),
+	)
+}
+
+func egressSecurityRuleKey(rule core.EgressSecurityRule) string {
+	return fmt.Sprintf(
+		"stateless=%s|protocol=%s|destination=%s|destinationType=%s|icmp=%s|tcp=%s|udp=%s",
+		boolPtrKey(rule.IsStateless),
+		stringPtrKey(rule.Protocol),
+		stringPtrKey(rule.Destination),
+		egressDestinationTypeKey(rule.DestinationType),
+		icmpOptionsKey(rule.IcmpOptions),
+		tcpOptionsKey(rule.TcpOptions),
+		udpOptionsKey(rule.UdpOptions),
+	)
+}
+
+func icmpOptionsKey(options *core.IcmpOptions) string {
+	if options == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("code=%s,type=%s", intPtrKey(options.Code), intPtrKey(options.Type))
+}
+
+func tcpOptionsKey(options *core.TcpOptions) string {
+	if options == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("source=%s,destination=%s", portRangeKey(options.SourcePortRange), portRangeKey(options.DestinationPortRange))
+}
+
+func udpOptionsKey(options *core.UdpOptions) string {
+	if options == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("source=%s,destination=%s", portRangeKey(options.SourcePortRange), portRangeKey(options.DestinationPortRange))
+}
+
+func portRangeKey(portRange *core.PortRange) string {
+	if portRange == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("min=%s,max=%s", intPtrKey(portRange.Min), intPtrKey(portRange.Max))
+}
+
+func stringPtrKey(value *string) string {
+	if value == nil {
+		return "<nil>"
+	}
+	return *value
+}
+
+func ingressSourceTypeKey(value core.IngressSecurityRuleSourceTypeEnum) string {
+	return string(value)
+}
+
+func egressDestinationTypeKey(value core.EgressSecurityRuleDestinationTypeEnum) string {
+	return string(value)
+}
+
+func boolPtrKey(value *bool) string {
+	if value == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("%t", *value)
+}
+
+func intPtrKey(value *int) string {
+	if value == nil {
+		return "<nil>"
+	}
+	return fmt.Sprintf("%d", *value)
+}
+
 func portRangeMatchesSpec(r core.PortRange, ports *portSpec) bool {
 	if ports == nil {
 		return false

pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go

@@ -1318,6 +1318,50 @@ func TestSecurityListRulesChanged(t *testing.T) {
 	}
 }
 
+func TestDedupeIngressSecurityRules(t *testing.T) {
+	rule := makeIngressSecurityRule("1", 80)
+	ruleWithDescription := makeIngressSecurityRule("1", 80)
+	ruleWithDescription.Description = common.String("generated by old reconciliation")
+	otherRule := makeIngressSecurityRule("2", 81)
+
+	result := dedupeIngressSecurityRules(zap.S(), []core.IngressSecurityRule{
+		rule,
+		rule,
+		ruleWithDescription,
+		otherRule,
+	})
+
+	expected := []core.IngressSecurityRule{
+		rule,
+		otherRule,
+	}
+	if !reflect.DeepEqual(result, expected) {
+		t.Errorf("expected deduped ingress rules\n%+v\nbut got\n%+v", expected, result)
+	}
+}
+
+func TestDedupeEgressSecurityRules(t *testing.T) {
+	rule := makeEgressSecurityRule("1", 80)
+	ruleWithDescription := makeEgressSecurityRule("1", 80)
+	ruleWithDescription.Description = common.String("generated by old reconciliation")
+	otherRule := makeEgressSecurityRule("2", 81)
+
+	result := dedupeEgressSecurityRules(zap.S(), []core.EgressSecurityRule{
+		rule,
+		rule,
+		ruleWithDescription,
+		otherRule,
+	})
+
+	expected := []core.EgressSecurityRule{
+		rule,
+		otherRule,
+	}
+	if !reflect.DeepEqual(result, expected) {
+		t.Errorf("expected deduped egress rules\n%+v\nbut got\n%+v", expected, result)
+	}
+}
+
 func TestUpdate(t *testing.T) {
 
 }

pkg/cloudprovider/providers/oci/load_balancer_test.go

@@ -1781,7 +1781,7 @@ func Test_getLbListenerBackendSetIpVersion(t *testing.T) {
 			listenerBackendSetIpVersions: []string{IPv4},
 			wantErr:                      nil,
 		},
-		"PreferDualStack IPv4": {
+		"PreferDualStack IPv4 - ipFamilies takes precedence over subnet capabilities": {
 			ipFamilies:     []string{IPv4},
 			ipFamilyPolicy: string(v1.IPFamilyPolicyPreferDualStack),
 			nodeSubnets: []*core.Subnet{
@@ -1791,10 +1791,10 @@ func Test_getLbListenerBackendSetIpVersion(t *testing.T) {
 					Ipv6CidrBlocks: []string{"2001:0000:130F:0000:0000:09C0:876A:130B"},
 				},
 			},
-			listenerBackendSetIpVersions: []string{IPv4, IPv6},
+			listenerBackendSetIpVersions: []string{IPv4},
 			wantErr:                      nil,
 		},
-		"PreferDualStack IPv4 multiple subnets": {
+		"PreferDualStack IPv4 multiple subnets - ipFamilies takes precedence": {
 			ipFamilies:     []string{IPv4},
 			ipFamilyPolicy: string(v1.IPFamilyPolicyPreferDualStack),
 			nodeSubnets: []*core.Subnet{
@@ -1806,7 +1806,7 @@ func Test_getLbListenerBackendSetIpVersion(t *testing.T) {
 					Ipv6CidrBlocks: []string{"2001:0000:130F:0000:0000:09C0:876A:130B"},
 				},
 			},
-			listenerBackendSetIpVersions: []string{IPv4, IPv6},
+			listenerBackendSetIpVersions: []string{IPv4},
 			wantErr:                      nil,
 		},
 	}