From e424420051790cb4ab7e7c2e71aeba02c8e91abe Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 21 Oct 2024 16:36:59 -0400 Subject: [PATCH] [WILL SQUASH] Move inactive LB parameters clearing to status.go Previously, the fix for OCPBUGS-38217 cleared inactive LB parameters in the status in setDefaultPublishingStrategy. This was valid at the time, as the desired LB type would always become the actual LB type. However, with the introduction of pending LB type changes, setDefaultPublishingStrategy was now prematurely clearing parameters for the current (but soon-to-be inactive) LB type. This caused issues because certain LB parameters, such as classicLoadBalancer.subnets or networkLoadBalancer.eipAllocations, reflect the actual state. As a result, setDefaultPublishingStrategy was clearing them too early, without knowing a type change was pending. The solution is to move the clearing logic to syncIngressControllerStatus in status.go, where it can detect pending LB type changes and avoid clearing parameters for the still-active LB type. This means that during a pending type change, both networkLoadBalancer and classicLoadBalancer parameters are present. This commit will be squashed into the previous one, as both are dependent on each other. I've kept them separate for easier review. --- pkg/operator/controller/ingress/controller.go | 2 - .../controller/ingress/controller_test.go | 14 ------ pkg/operator/controller/ingress/status.go | 48 +++++++++++++++++-- .../controller/ingress/status_test.go | 38 +++++++++++++++ 4 files changed, 81 insertions(+), 21 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 229926fb4..71f4ac4bd 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -581,7 +581,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat changed = true } if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer { - statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = nil if statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} } @@ -603,7 +602,6 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStat } } if statusLB.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer { - statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = nil if statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters == nil { statusLB.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} } diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index 84374d962..30f460843 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -658,20 +658,6 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())), domainMatchesBaseDomain: true, }, - { - name: "loadbalancer type changed from ELB to NLB, with old ELB parameters removal", - ic: makeIC(spec(nlb()), status(elbWithNullParameters())), - expectedResult: true, - expectedIC: makeIC(spec(nlb()), status(nlbWithNullParameters())), - domainMatchesBaseDomain: true, - }, - { - name: "loadbalancer type changed from NLB to ELB, with old NLB parameters removal", - ic: makeIC(spec(elb()), status(nlbWithNullParameters())), - expectedResult: true, - expectedIC: makeIC(spec(elb()), status(elbWithNullParameters())), - domainMatchesBaseDomain: true, - }, { name: "loadbalancer type changed from NLB to unset, with Classic LB as default", ic: makeIC(spec(eps(lbs(operatorv1.ExternalLoadBalancer, &managedDNS))), status(nlb())), diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index a258c8192..3d99c1f13 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -71,6 +71,8 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle updated.Status.Selector = selector.String() updated.Status.TLSProfile = computeIngressTLSProfile(ic.Status.TLSProfile, deployment) + clearIngressControllerInactiveAWSLBTypeParametersStatus(platformStatus, updated, service) + if updated.Status.EndpointPublishingStrategy != nil && updated.Status.EndpointPublishingStrategy.LoadBalancer != nil { updated.Status.EndpointPublishingStrategy.LoadBalancer.AllowedSourceRanges = computeAllowedSourceRanges(service) } @@ -810,18 +812,28 @@ func IngressStatusesEqual(a, b operatorv1.IngressControllerStatus) bool { } if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil { - if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters != nil { - if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.Subnets) { + nlbParamsA := a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters + nlbParamsB := b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters + if nlbParamsA != nil && nlbParamsB != nil { + if !awsSubnetsEqual(nlbParamsA.Subnets, nlbParamsB.Subnets) { return false } - if !awsEIPAllocationsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters.EIPAllocations) { + if !awsEIPAllocationsEqual(nlbParamsA.EIPAllocations, nlbParamsB.EIPAllocations) { return false } + } else if nlbParamsA != nlbParamsB { + // One is nil, the other is not. + return false } - if a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil && b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil { - if !awsSubnetsEqual(a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets, b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters.Subnets) { + clbParamsA := a.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters + clbParamsB := b.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters + if clbParamsA != nil && clbParamsB != nil { + if !awsSubnetsEqual(clbParamsA.Subnets, clbParamsB.Subnets) { return false } + } else if clbParamsA != clbParamsB { + // One is nil, the other is not. + return false } } if getOpenStackFloatingIPInEPS(a.EndpointPublishingStrategy) != getOpenStackFloatingIPInEPS(b.EndpointPublishingStrategy) { @@ -949,6 +961,32 @@ func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, serv } } +// clearIngressControllerInactiveAWSLBTypeParametersStatus clears AWS parameters for the inactive load balancer type, +// unless there is a load balancer type change is pending. When a change is pending, setDefaultPublishingStrategy +// sets the desired parameters for the new LB type. To avoid removing the desired state, we allow +// parameters for both classicLoadBalancer and networkLoadBalancer to exist during the transition. +func clearIngressControllerInactiveAWSLBTypeParametersStatus(platformStatus *configv1.PlatformStatus, ic *operatorv1.IngressController, service *corev1.Service) { + if service == nil { + return + } + if platformStatus.Type == configv1.AWSPlatformType { + haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service) + wantLBType := getAWSLoadBalancerTypeInStatus(ic) + if haveLBType == wantLBType { + switch haveLBType { + case operatorv1.AWSNetworkLoadBalancer: + if getAWSClassicLoadBalancerParametersInStatus(ic) != nil { + ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = nil + } + case operatorv1.AWSClassicLoadBalancer: + if getAWSNetworkLoadBalancerParametersInStatus(ic) != nil { + ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = nil + } + } + } + } +} + // updateIngressControllerAWSSubnetStatus mutates the provided IngressController object to // sync its status to the effective subnets on the LoadBalancer-type service. func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, service *corev1.Service) { diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 88f1d3196..991678373 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -2324,6 +2324,44 @@ func Test_IngressStatusesEqual(t *testing.T) { []operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"}, ), }, + { + description: "classicLoadBalancer parameters cleared", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSClassicLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{}, + }, + }, + }, + }, + }, + { + description: "networkLoadBalancer parameters cleared", + expected: false, + a: icStatusWithSubnetsOrEIPAllocations( + operatorv1.AWSNetworkLoadBalancer, + nil, + []operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx"}, + ), + b: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{}, + }, + }, + }, + }, + }, } for _, tc := range testCases {