Skip to content

Commit

Permalink
OCPBUGS-16728: Require Service Deletion for LB Type Updates
Browse files Browse the repository at this point in the history 
Due to the AWS CCM leaking resources when the load balancer type
is changed on a service, the cloud team is now blocking updates
to the load balancer type. As a result, the Ingress Operator
will require the service to be deleted and recreated when the
Ingress Controller's load balancer type changes.

This change introduces a Progressing=True status message when
the load balancer type is modified, instructing the user on how to
effectuate the change. Additionally, the
`service.beta.kubernetes.io/aws-load-balancer-type` annotation is
now unmanaged.
  • Loading branch information
@gcs278
gcs278 committed Sep 10, 2024
1 parent 59a477e commit 65eea90
Show file tree
Hide file tree
Showing 4 changed files with 185 additions and 68 deletions.
62 changes: 54 additions & 8 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,19 +33,19 @@ const (
// Key=Value pairs that are additionally recorded on
// load balancer resources and security groups.
//
// https://kubernetes.io/docs/concepts/services-networking/service/#aws-load-balancer-additional-resource-tags
// https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-additional-resource-tags
awsLBAdditionalResourceTags = "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags"

// awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any
// AWS load balancer services created.
//
// https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws
// https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-proxy-protocol
awsLBProxyProtocolAnnotation = "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol"

// AWSLBTypeAnnotation is a Service annotation used to specify an AWS load
// balancer type. See the following for additional details:
//
// https://kubernetes.io/docs/concepts/services-networking/service/#aws-nlb-support
// https://kubernetes.io/docs/reference/labels-annotations-taints/#service-beta-kubernetes-io-aws-load-balancer-type
AWSLBTypeAnnotation = "service.beta.kubernetes.io/aws-load-balancer-type"

// AWSNLBAnnotation is the annotation value of an AWS Network Load Balancer (NLB).
Expand Down Expand Up @@ -242,8 +242,6 @@ var (
// local-with-fallback annotation for kube-proxy (see
// <https://bugzilla.redhat.com/show_bug.cgi?id=1960284>).
localWithFallbackAnnotation,
// AWS load balancer type annotation to set either CLB/ELB or NLB
AWSLBTypeAnnotation,
// awsLBProxyProtocolAnnotation is used to enable the PROXY protocol on any
// AWS load balancer services created.
//
Expand Down Expand Up @@ -599,7 +597,7 @@ func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options
// indicating whether the service was updated, and an error value.
func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, platform *configv1.PlatformStatus, autoDeleteLB bool) (bool, error) {
if shouldRecreateLB, reason := shouldRecreateLoadBalancer(current, desired, platform); shouldRecreateLB && autoDeleteLB {
log.Info("deleting and recreating the load balancer because "+reason, "namespace", desired.Namespace, "name", desired.Name)
log.Info("deleting and recreating the load balancer", "reason", reason, "namespace", desired.Namespace, "name", desired.Name)
foreground := metav1.DeletePropagationForeground
deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground}
if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil {
Expand Down Expand Up @@ -661,6 +659,9 @@ func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *conf
if platform.Type == configv1.AWSPlatformType && !serviceEIPAllocationsEqual(current, desired) {
return true, "its eipAllocations changed"
}
if platform.Type == configv1.AWSPlatformType && !serviceAWSLBTypeEqual(current, desired) {
return true, "its load balancer type changed"
}
return false, ""
}

Expand Down Expand Up @@ -799,12 +800,24 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
errs = append(errs, err)
}

if platform.Type == configv1.AWSPlatformType {
wantLBType := getAWSLoadBalancerTypeInSpec(ic)
haveLBType := getAWSLBTypeFromServiceAnnotation(service)

if wantLBType != haveLBType {
changedMsg := fmt.Errorf("The IngressController load balancer type was changed from %q to %q.", haveLBType, wantLBType)
ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"type\":\"LoadBalancerService\",\"loadBalancer\":{\"providerParameters\":{\"type\":\"AWS\",\"aws\":{\"type\":\"%[2]s\"}}}}}}'", ic.Name, wantLBType)
err := fmt.Errorf("%[1]s To effectuate this change, you must delete the service: `oc -n %[2]s delete svc/%[3]s`; the service load-balancer will then be deprovisioned and a new one created. This will most likely cause the new load-balancer to have a different host name and IP address and cause disruption. To return to the previous state, you can revert the change to the IngressController: `%[4]s`", changedMsg, service.Namespace, service.Name, ocPatchRevertCmd)
errs = append(errs, err)
}
}

if platform.Type == configv1.AWSPlatformType && subnetsAWSEnabled {
var (
wantSubnets, haveSubnets *operatorv1.AWSSubnets
paramsFieldName string
)
switch getAWSLoadBalancerTypeInStatus(ic) {
switch getAWSLBTypeFromServiceAnnotation(service) {
case operatorv1.AWSNetworkLoadBalancer:
if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil {
wantSubnets = nlbParams.Subnets
Expand Down Expand Up @@ -835,7 +848,7 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service
}
}

if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer {
if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLBTypeFromServiceAnnotation(service) == operatorv1.AWSNetworkLoadBalancer {
var (
wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation
)
Expand Down Expand Up @@ -966,6 +979,21 @@ func loadBalancerSourceRangesMatch(ic *operatorv1.IngressController, current *co
return fmt.Errorf("You have manually edited an operator-managed object. You must revert your modifications by removing the Spec.LoadBalancerSourceRanges field of LoadBalancer-typed service %q. You can use the new AllowedSourceRanges API field on the ingresscontroller to configure this setting instead.", current.Name)
}

// getAWSLBTypeFromServiceAnnotation gets the effective load balancer type by looking at the
// service.beta.kubernetes.io/aws-load-balancer-type annotation of the LoadBalancer-type Service.
// If the annotation isn't specified, the function returns the default of Classic.
func getAWSLBTypeFromServiceAnnotation(service *corev1.Service) operatorv1.AWSLoadBalancerType {
if service == nil {
return ""
}

if a, ok := service.Annotations[AWSLBTypeAnnotation]; ok && a == AWSNLBAnnotation {
return operatorv1.AWSNetworkLoadBalancer
}

return operatorv1.AWSClassicLoadBalancer
}

// getSubnetsFromServiceAnnotation gets the effective subnets by looking at the
// service.beta.kubernetes.io/aws-load-balancer-subnets annotation of the LoadBalancer-type Service.
// If no subnets are specified in the annotation, this function returns nil.
Expand Down Expand Up @@ -1035,6 +1063,12 @@ func serviceEIPAllocationsEqual(a, b *corev1.Service) bool {
return awsEIPAllocationsEqual(getEIPAllocationsFromServiceAnnotation(a), getEIPAllocationsFromServiceAnnotation(b))
}

// serviceAWSLBTypeEqual compares the AWS load balancer type annotations
// on the two services to determine if they are equivalent.
func serviceAWSLBTypeEqual(a, b *corev1.Service) bool {
return getAWSLBTypeFromServiceAnnotation(a) == getAWSLBTypeFromServiceAnnotation(b)
}

// awsEIPAllocationsEqual compares two AWSEIPAllocation slices and returns a boolean
// whether they are equal are not. The order of the EIP Allocations are ignored.
func awsEIPAllocationsEqual(eipAllocations1, eipAllocations2 []operatorv1.EIPAllocation) bool {
Expand Down Expand Up @@ -1180,6 +1214,18 @@ func JoinAWSEIPAllocations(eipAllocations []operatorv1.EIPAllocation, sep string
return buffer.String()
}

// getAWSLoadBalancerTypeInSpec gets the AWS Load Balancer Type reported in the status.
// If nothing is configured, then it returns the default of Classic.
func getAWSLoadBalancerTypeInSpec(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType {
if ic.Spec.EndpointPublishingStrategy != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil &&
ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil {
return ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type
}
return operatorv1.AWSClassicLoadBalancer
}

// getAWSLoadBalancerTypeInStatus gets the AWS Load Balancer Type reported in the status.
func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1.AWSLoadBalancerType {
if ic.Status.EndpointPublishingStrategy != nil &&
Expand Down
9 changes: 8 additions & 1 deletion pkg/operator/controller/ingress/load_balancer_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1170,7 +1170,14 @@ func Test_loadBalancerServiceChanged(t *testing.T) {
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-subnets"] = "foo-subnet"
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB"
},
expect: true,
expect: false,
},
{
description: "if the service.beta.kubernetes.io/aws-load-balancer-type is added",
mutate: func(svc *corev1.Service) {
svc.Annotations["service.beta.kubernetes.io/aws-load-balancer-type"] = "NLB"
},
expect: false,
},
{
description: "if the service.beta.kubernetes.io/aws-load-balancer-eip-allocations annotation added",
Expand Down
70 changes: 51 additions & 19 deletions pkg/operator/controller/ingress/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -655,8 +655,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
},
},
}

loadBalancerIngressControllerWithAWSSubnets := func(lbType operatorv1.AWSLoadBalancerType, subnetSpec *operatorv1.AWSSubnets, subnetStatus *operatorv1.AWSSubnets) *operatorv1.IngressController {
loadBalancerIngressControllerWithLBType := func(lbType operatorv1.AWSLoadBalancerType) *operatorv1.IngressController {
eps := &operatorv1.EndpointPublishingStrategy{
Type: operatorv1.LoadBalancerServiceStrategyType,
LoadBalancer: &operatorv1.LoadBalancerStrategy{
Expand All @@ -677,6 +676,11 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
EndpointPublishingStrategy: eps.DeepCopy(),
},
}
return ic
}
loadBalancerIngressControllerWithAWSSubnets := func(lbType operatorv1.AWSLoadBalancerType, subnetSpec *operatorv1.AWSSubnets, subnetStatus *operatorv1.AWSSubnets) *operatorv1.IngressController {
ic := loadBalancerIngressControllerWithLBType(lbType)

switch lbType {
case operatorv1.AWSNetworkLoadBalancer:
ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{
Expand Down Expand Up @@ -763,6 +767,13 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
},
}
lbService := &corev1.Service{}
lbServiceWithNLB := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
AWSLBTypeAnnotation: AWSNLBAnnotation,
},
},
}
lbServiceWithInternalScopeOnAWS := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
Expand Down Expand Up @@ -884,7 +895,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
nil,
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -896,7 +907,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
nil,
&operatorv1.AWSSubnets{},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -910,7 +921,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
},
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: false,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -924,7 +935,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
},
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand All @@ -938,7 +949,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
IDs: []operatorv1.AWSSubnetID{"subnet-12345"},
},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand All @@ -956,7 +967,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
Names: []operatorv1.AWSSubnetName{"name-12345"},
},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand Down Expand Up @@ -992,7 +1003,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
Names: []operatorv1.AWSSubnetName{"name-67890", "name-12345"},
},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1010,7 +1021,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
Names: []operatorv1.AWSSubnetName{"name-67890", "name-12345", "name-54321"},
},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsSubnetsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand Down Expand Up @@ -1145,7 +1156,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
nil,
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1156,7 +1167,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
nil,
[]operatorv1.EIPAllocation{},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1167,7 +1178,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: false,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1178,7 +1189,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
nil,
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand All @@ -1189,7 +1200,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
nil,
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand All @@ -1200,7 +1211,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1211,7 +1222,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
[]operatorv1.EIPAllocation{"eipalloc-aaaaaaaaaaaaaaaaa", "eipalloc-bbbbbbbbbbbbbbbbb"},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
Expand All @@ -1222,7 +1233,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy"},
[]operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
Expand All @@ -1233,11 +1244,32 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) {
[]operatorv1.EIPAllocation{"eipalloc-xxxxxxxxxxxxxxxxx", "eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-zzzzzzzzzzzzz"},
[]operatorv1.EIPAllocation{"eipalloc-yyyyyyyyyyyyyyyyy", "eipalloc-xxxxxxxxxxxxxxxxx"},
),
service: &corev1.Service{},
service: lbServiceWithNLB,
awsEIPAllocationsEnabled: true,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
},
{
name: "LBType Empty LoadBalancerService (default Classic), AWS LBType Classic",
ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer),
service: lbService,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionFalse,
},
{
name: "LBType Classic LoadBalancerService, AWS LBType NLB",
ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSNetworkLoadBalancer),
service: lbService,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
},
{
name: "LBType NLB LoadBalancerService, AWS LBType Classic",
ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer),
service: lbServiceWithNLB,
platformStatus: awsPlatformStatus,
expectStatus: operatorv1.ConditionTrue,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
Expand Down
Loading

0 comments on commit 65eea90

Please sign in to comment.