From 5b7f63470cb7a786206705dcbba1976e53a3732c Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Thu, 8 Jan 2026 16:00:14 -0800 Subject: [PATCH 1/3] Fix NLB IP version selection to respect service ipFamilies When ipFamilyPolicy is PreferDualStack, the getLbListenerBackendSetIpVersion function was incorrectly returning [IPv4, IPv6] based solely on subnet capabilities, ignoring the service's ipFamilies field. This caused issues when: - Service has ipFamilyPolicy: PreferDualStack - Service has ipFamilies: [IPv4] (explicitly IPv4-only) - Subnet supports dual-stack The CCM would create IPv6 listeners even though the service explicitly specifies IPv4-only via ipFamilies. The ipFamilies field is the authoritative specification of what IP families a service uses. The ipFamilyPolicy only influences how ipFamilies gets populated when not explicitly set. This fix ensures the CCM respects ipFamilies by only including IP versions that are both specified in ipFamilies AND supported by the subnet. --- .../providers/oci/load_balancer.go | 23 ++++++++++--------- .../providers/oci/load_balancer_test.go | 8 +++---- 2 files changed, 16 insertions(+), 15 deletions(-) diff --git a/pkg/cloudprovider/providers/oci/load_balancer.go b/pkg/cloudprovider/providers/oci/load_balancer.go index 4eba9aa4ae..a385afc9b7 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer.go +++ b/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 } diff --git a/pkg/cloudprovider/providers/oci/load_balancer_test.go b/pkg/cloudprovider/providers/oci/load_balancer_test.go index a2879062ff..3ac29dee2d 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_test.go +++ b/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, }, } From 66047b022257969a821f17e915f73b01531477db Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Sat, 6 Jun 2026 15:51:47 -0700 Subject: [PATCH 2/3] Deduplicate OCI security list rules --- .../oci/load_balancer_security_lists.go | 130 ++++++++++++++++++ .../oci/load_balancer_security_lists_test.go | 44 ++++++ 2 files changed, 174 insertions(+) diff --git a/pkg/cloudprovider/providers/oci/load_balancer_security_lists.go b/pkg/cloudprovider/providers/oci/load_balancer_security_lists.go index ca5fe277d5..7073a6b424 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_security_lists.go +++ b/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 "" + } + return fmt.Sprintf("code=%s,type=%s", intPtrKey(options.Code), intPtrKey(options.Type)) +} + +func tcpOptionsKey(options *core.TcpOptions) string { + if options == nil { + return "" + } + return fmt.Sprintf("source=%s,destination=%s", portRangeKey(options.SourcePortRange), portRangeKey(options.DestinationPortRange)) +} + +func udpOptionsKey(options *core.UdpOptions) string { + if options == nil { + return "" + } + return fmt.Sprintf("source=%s,destination=%s", portRangeKey(options.SourcePortRange), portRangeKey(options.DestinationPortRange)) +} + +func portRangeKey(portRange *core.PortRange) string { + if portRange == nil { + return "" + } + return fmt.Sprintf("min=%s,max=%s", intPtrKey(portRange.Min), intPtrKey(portRange.Max)) +} + +func stringPtrKey(value *string) string { + if value == nil { + return "" + } + 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 "" + } + return fmt.Sprintf("%t", *value) +} + +func intPtrKey(value *int) string { + if value == nil { + return "" + } + return fmt.Sprintf("%d", *value) +} + func portRangeMatchesSpec(r core.PortRange, ports *portSpec) bool { if ports == nil { return false diff --git a/pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go b/pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go index fe59ae1ab4..c406acd815 100644 --- a/pkg/cloudprovider/providers/oci/load_balancer_security_lists_test.go +++ b/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) { } From 8ebd110e39acc6b12062b4f759c5a020e49ae010 Mon Sep 17 00:00:00 2001 From: Kabir Kwatra Date: Sat, 6 Jun 2026 22:09:16 -0700 Subject: [PATCH 3/3] Publish fix-nlb-ipfamilies image --- .../workflows/publish-fix-nlb-ipfamilies.yaml | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 .github/workflows/publish-fix-nlb-ipfamilies.yaml diff --git a/.github/workflows/publish-fix-nlb-ipfamilies.yaml b/.github/workflows/publish-fix-nlb-ipfamilies.yaml new file mode 100644 index 0000000000..ae7d43fa84 --- /dev/null +++ b/.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 }}