diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 43984ec1a4..5da03a1172 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -463,7 +463,12 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat } // Set provider parameters based on the cluster ingress config. - setDefaultProviderParameters(effectiveStrategy.LoadBalancer, ingressConfig, alreadyAdmitted) + var currentLBStrategy *operatorv1.LoadBalancerStrategy + if ic.Status.EndpointPublishingStrategy != nil { + currentLBStrategy = ic.Status.EndpointPublishingStrategy.LoadBalancer + } + effectiveLBStrategy := effectiveStrategy.LoadBalancer + setDefaultProviderParameters(effectiveLBStrategy, currentLBStrategy, ingressConfig, alreadyAdmitted) case operatorv1.NodePortServiceStrategyType: if effectiveStrategy.NodePort == nil { @@ -678,13 +683,14 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat return false } -// setDefaultProviderParameters mutates the given LoadBalancerStrategy by +// setDefaultProviderParameters mutates the given effective LoadBalancerStrategy by // defaulting its ProviderParameters field based on the defaults in the provided -// ingress config object. -func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress, alreadyAdmitted bool) { +// ingress config object if it hasn't been admitted yet. If already admitted, it +// bases the effective LoadBalancerStrategy on the current LoadBalancerStrategy. +func setDefaultProviderParameters(lbsEffective, lbsCurrent *operatorv1.LoadBalancerStrategy, ingressConfig *configv1.Ingress, alreadyAdmitted bool) { var provider operatorv1.LoadBalancerProviderType - if lbs.ProviderParameters != nil { - provider = lbs.ProviderParameters.Type + if lbsEffective.ProviderParameters != nil { + provider = lbsEffective.ProviderParameters.Type } if len(provider) == 0 && !alreadyAdmitted { // Infer the LB type from the cluster ingress config, but only @@ -696,48 +702,48 @@ func setDefaultProviderParameters(lbs *operatorv1.LoadBalancerStrategy, ingressC } switch provider { case operatorv1.AWSLoadBalancerProvider: - if lbs.ProviderParameters == nil { - lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + if lbsEffective.ProviderParameters == nil { + lbsEffective.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} } - lbs.ProviderParameters.Type = provider + lbsEffective.ProviderParameters.Type = provider defaultLBType := operatorv1.AWSClassicLoadBalancer - if p := ingressConfig.Spec.LoadBalancer.Platform; !alreadyAdmitted && p.Type == configv1.AWSPlatformType && p.AWS != nil { - if p.AWS.Type == configv1.NLB { - defaultLBType = operatorv1.AWSNetworkLoadBalancer - } + if alreadyAdmitted { + defaultLBType = lbsCurrent.ProviderParameters.AWS.Type + } else if p := ingressConfig.Spec.LoadBalancer.Platform; p.Type == configv1.AWSPlatformType && p.AWS != nil && p.AWS.Type == configv1.NLB { + defaultLBType = operatorv1.AWSNetworkLoadBalancer } - if lbs.ProviderParameters.AWS == nil { - lbs.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{} + if lbsEffective.ProviderParameters.AWS == nil { + lbsEffective.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{} } - if len(lbs.ProviderParameters.AWS.Type) == 0 { - lbs.ProviderParameters.AWS.Type = defaultLBType + if len(lbsEffective.ProviderParameters.AWS.Type) == 0 { + lbsEffective.ProviderParameters.AWS.Type = defaultLBType } - switch lbs.ProviderParameters.AWS.Type { + switch lbsEffective.ProviderParameters.AWS.Type { case operatorv1.AWSClassicLoadBalancer: - if lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { - lbs.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} + if lbsEffective.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { + lbsEffective.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} } case operatorv1.AWSNetworkLoadBalancer: - if lbs.ProviderParameters.AWS.NetworkLoadBalancerParameters == nil { - lbs.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} + if lbsEffective.ProviderParameters.AWS.NetworkLoadBalancerParameters == nil { + lbsEffective.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} } } case operatorv1.GCPLoadBalancerProvider: - if lbs.ProviderParameters == nil { - lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + if lbsEffective.ProviderParameters == nil { + lbsEffective.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} } - lbs.ProviderParameters.Type = provider - if lbs.ProviderParameters.GCP == nil { - lbs.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{} + lbsEffective.ProviderParameters.Type = provider + if lbsEffective.ProviderParameters.GCP == nil { + lbsEffective.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{} } case operatorv1.IBMLoadBalancerProvider: - if lbs.ProviderParameters == nil { - lbs.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} + if lbsEffective.ProviderParameters == nil { + lbsEffective.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} } - lbs.ProviderParameters.Type = provider - if lbs.ProviderParameters.IBM == nil { - lbs.ProviderParameters.IBM = &operatorv1.IBMLoadBalancerParameters{} + lbsEffective.ProviderParameters.Type = provider + if lbsEffective.ProviderParameters.IBM == nil { + lbsEffective.ProviderParameters.IBM = &operatorv1.IBMLoadBalancerParameters{} } } } diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index 530de30715..5bd4d41756 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -413,6 +413,14 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { } return lbs } + // providerParameters.type is set, but providerParameters. is not. + lbsWithEmptyPlatformParameters = func(providerType operatorv1.LoadBalancerProviderType) *operatorv1.LoadBalancerStrategy { + lbsWithEmptyPP := lbs(operatorv1.ExternalLoadBalancer, &managedDNS) + lbsWithEmptyPP.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ + Type: providerType, + } + return lbsWithEmptyPP + } eps = func(lbs *operatorv1.LoadBalancerStrategy) *operatorv1.EndpointPublishingStrategy { return &operatorv1.EndpointPublishingStrategy{ Type: operatorv1.LoadBalancerServiceStrategyType, @@ -662,6 +670,14 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), domainMatchesBaseDomain: true, }, + { + name: "loadbalancer type with empty platform parameters changed from NLB to unset, with NLB as default (OCPBUGS-43692)", + ic: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.AWSLoadBalancerProvider))), status(nlb())), + ingressConfig: ingressConfigWithDefaultNLB, + expectedResult: false, + expectedIC: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.AWSLoadBalancerProvider))), status(nlbWithNullParameters())), + domainMatchesBaseDomain: true, + }, { name: "loadbalancer ELB connection idle timeout changed from unset with null provider parameters to 2m", ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithNullParameters())), @@ -704,6 +720,13 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(gcpLB(operatorv1.GCPLocalAccess)), status(gcpLB(operatorv1.GCPLocalAccess))), domainMatchesBaseDomain: true, }, + { + name: "loadbalancer GCP Global Access with empty platform provider parameters changed from global to unset (OCPBUGS-43692)", + ic: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.GCPLoadBalancerProvider))), status(gcpLB(operatorv1.GCPGlobalAccess))), + expectedResult: true, + expectedIC: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.GCPLoadBalancerProvider))), status(gcpLB(""))), + domainMatchesBaseDomain: true, + }, { name: "loadbalancer IBM Protocol changed from unset to PROXY", ic: makeIC(spec(ibmLB(operatorv1.ProxyProtocol)), status(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS)))), @@ -725,6 +748,13 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(ibmLB(operatorv1.TCPProtocol)), status(ibmLB(operatorv1.TCPProtocol))), domainMatchesBaseDomain: true, }, + { + name: "loadbalancer IBM Protocol with empty platlform parameters changed from PROXY to TCP", + ic: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.IBMLoadBalancerProvider))), status(ibmLB(operatorv1.ProxyProtocol))), + expectedResult: true, + expectedIC: makeIC(spec(eps(lbsWithEmptyPlatformParameters(operatorv1.IBMLoadBalancerProvider))), status(ibmLB(""))), + domainMatchesBaseDomain: true, + }, { // https://bugzilla.redhat.com/show_bug.cgi?id=1997226 name: "nodeport protocol changed to PROXY with null status.endpointPublishingStrategy.nodePort",