Skip to content

Commit

Permalink
[WILL SQUASH] Move inactive LB parameters clearing to status.go
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@gcs278
gcs278 committed Dec 9, 2024
1 parent 7549e74 commit e424420
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 21 deletions.
2 changes: 0 additions & 2 deletions pkg/operator/controller/ingress/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
}
Expand All @@ -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{}
}
Expand Down
14 changes: 0 additions & 14 deletions pkg/operator/controller/ingress/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())),
Expand Down
48 changes: 43 additions & 5 deletions pkg/operator/controller/ingress/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
38 changes: 38 additions & 0 deletions pkg/operator/controller/ingress/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit e424420

Please sign in to comment.