From 39cf30c134455b828f66ad472dc9cf097d9aeda2 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 3cbd8050b..bfdd70b34 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -567,7 +567,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{} } @@ -589,7 +588,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 2cecd6ccf..aa12d0cf0 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -624,20 +624,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 407665050..e34f34782 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) } @@ -807,18 +809,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 } } } @@ -943,6 +955,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 7cc2118dd..9b2acfe90 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -2234,6 +2234,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 {