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 }} 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_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) { } 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, }, }