From b27545bc94870c4cbd664b0d29579af0b4c46c12 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 | 113 ++++++++++++++++++ 4 files changed, 156 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..b82a5e102 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "github.com/stretchr/testify/assert" "math/big" "reflect" "strings" @@ -2324,6 +2325,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 { @@ -3415,3 +3454,77 @@ func Test_computeAllowedSourceRanges(t *testing.T) { }) } } + +func Test_clearIngressControllerInactiveAWSLBTypeParametersStatus(t *testing.T) { + awsPlatformStatus := &configv1.PlatformStatus{ + Type: configv1.AWSPlatformType, + } + lbServiceWithNLB := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } + lbServiceWithCLB := &corev1.Service{} + icStatusWithAWSLBParams := func(lbType operatorv1.AWSLoadBalancerType, nlbParams, clbParams bool) operatorv1.IngressControllerStatus { + icStatus := operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: lbType, + }, + }, + }, + }, + } + if nlbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} + } + if clbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} + } + return icStatus + } + testCases := []struct { + name string + service *corev1.Service + icStatus operatorv1.IngressControllerStatus + expectICStatus operatorv1.IngressControllerStatus + }{ + { + name: "service is nil", + service: nil, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + { + name: "clear clb params when lb type change is not pending", + service: lbServiceWithNLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, false), + }, + { + name: "clear nlb params when lb type change is not pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, false, true), + }, + { + name: "don't clear any params when lb type change is pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ic := &operatorv1.IngressController{ + Status: tc.icStatus, + } + clearIngressControllerInactiveAWSLBTypeParametersStatus(awsPlatformStatus, ic, tc.service) + assert.Equal(t, tc.expectICStatus, ic.Status) + }) + } +}