Skip to content

Commit

Permalink
OCPBUGS-43692: Fix LB Type Defaulting with ProviderParameters Set
Browse files Browse the repository at this point in the history 
Approach: pass in LoadBalanacerStrategy status so that LB Type can be
defaulted to the current value when already admitted.
  • Loading branch information
@gcs278
gcs278 committed Oct 23, 2024
1 parent 8bf1b36 commit 82e4b34
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 32 deletions.
70 changes: 38 additions & 32 deletions pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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{}
}
}
}
Expand Down
30 changes: 30 additions & 0 deletions pkg/operator/controller/ingress/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,14 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) {
}
return lbs
}
// providerParameters.type is set, but providerParameters.<platform> 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,
Expand Down Expand Up @@ -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())),
Expand Down Expand Up @@ -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)))),
Expand All @@ -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",
Expand Down

0 comments on commit 82e4b34

Please sign in to comment.