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 added to the `managedAtServiceCreationLBServiceAnnotations`
map along with other annotations that require service deletion.
  • Loading branch information
@gcs278
gcs278 committed Sep 12, 2024
1 parent 59a477e commit 9d3a208
Show file tree
Hide file tree
Showing 4 changed files with 277 additions and 167 deletions.
207 changes: 121 additions & 86 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import (
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kerrors "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/sets"

crclient "sigs.k8s.io/controller-runtime/pkg/client"
)

Expand All @@ -33,19 +31,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 @@ -218,9 +216,11 @@ var (
configv1.GCPPlatformType: {},
}

// managedLoadBalancerServiceAnnotations is a set of annotation keys for
// annotations that the operator manages for LoadBalancer-type services.
// The operator preserves all other annotations.
// managedLBServiceAnnotations is a set of annotation keys for
// annotations that the operator fully manages for LoadBalancer-type services.
// The operator preserves all other annotations. Any desired updates to
// these annotations are immediately applied to the service without
// requiring service deletion.
//
// Be careful when adding annotation keys to this set. If a new release
// of the operator starts managing an annotation that it previously
Expand All @@ -229,8 +229,8 @@ var (
// <https://bugzilla.redhat.com/show_bug.cgi?id=1905490>). In order to
// avoid problems, make sure the previous release blocks upgrades when
// the user has modified an annotation that the new release manages.
managedLoadBalancerServiceAnnotations = func() sets.String {
result := sets.NewString(
managedLBServiceAnnotations = func() []string {
result := []string{
// AWS LB health check interval annotation (see
// <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
awsLBHealthCheckIntervalAnnotation,
Expand All @@ -242,19 +242,17 @@ 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.
//
// https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws
awsLBProxyProtocolAnnotation,
// iksLBEnableFeaturesAnnotation annotation used on a service to enable features
// on the load balancer.
//
// https://cloud.ibm.com/docs/containers?topic=containers-vpc-lbaas
iksLBEnableFeaturesAnnotation,
)
// 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
awsLBProxyProtocolAnnotation,
}

// Azure and GCP support switching between internal and external
// scope by changing the annotation, so the operator manages the
Expand All @@ -265,15 +263,63 @@ var (
// <https://issues.redhat.com/browse/NE-621>.
for platform := range platformsWithMutableScope {
for name := range InternalLBAnnotations[platform] {
result.Insert(name)
result = append(result, name)
}
for name := range externalLBAnnotations[platform] {
result.Insert(name)
result = append(result, name)
}
}

return result
}()

// managedAtServiceCreationLBServiceAnnotations maps platform to annotation keys
// that the operator manages, but only during service creation. This means that
// the operator will not apply a desired annotation update right away.
// Instead, it will set a Progressing=True status condition with a message explaining
// how to apply the change manually (e.g., by deleting the service so that it can be
// recreated).
//
// Note that the ingress.operator.openshift.io/auto-delete-load-balancer annotation enables
// these annotations to be applied immediately by automatically deleting and recreating the
// service.
managedAtServiceCreationLBServiceAnnotations = func() map[configv1.PlatformType][]string {
result := map[configv1.PlatformType][]string{
configv1.AWSPlatformType: {
// Annotation to set either CLB/ELB or NLB.
// This annotation was previously fully managed (https://issues.redhat.com/browse/NE-865),
// but now required service recreation due to issues with AWS leaking resources when updating
// LB Type without recreating the service (see https://issues.redhat.com/browse/OCPBUGS-16728).
AWSLBTypeAnnotation,
// Annotation for configuring load balancer subnets.
// This requires service recreation because NLBs do not support updates to subnets,
// the operator cannot detect errors if the subnets are invalid, and to prevent
// compatibility issues during upgrades since this annotation was previously unmanaged.
awsLBSubnetsAnnotation,
// Annotation for configuring load balancer EIPs.
// This requires service recreation to prevent compatibility issues during upgrades since
// this annotation was previously unmanaged.
awsEIPAllocationsAnnotation,
},
}

// Add the annotations that do NOT support mutable scope.
for platform, annotation := range InternalLBAnnotations {
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotation {
result[platform] = append(result[platform], name)
}
}
}
for platform, annotation := range externalLBAnnotations {
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotation {
result[platform] = append(result[platform], name)
}
}
}
return result
}()
)

// ensureLoadBalancerService creates an LB service if one is desired but absent.
Expand Down Expand Up @@ -598,8 +644,8 @@ func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options
// updateLoadBalancerService updates a load balancer service. Returns a Boolean
// 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)
if shouldRecreateLB, changedAnnotations, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedAtServiceCreationLBServiceAnnotations[platform.Type]); shouldRecreateLB && autoDeleteLB {
log.Info("deleting and recreating the load balancer", "annotations", changedAnnotations, "namespace", desired.Namespace, "name", desired.Name)
foreground := metav1.DeletePropagationForeground
deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground}
if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil {
Expand All @@ -624,46 +670,6 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service,
return true, nil
}

// scopeEqual returns true if the scope is the same between the two given
// services and false if the scope is different.
func scopeEqual(a, b *corev1.Service, platform *configv1.PlatformStatus) bool {
aAnnotations := a.Annotations
if aAnnotations == nil {
aAnnotations = map[string]string{}
}
bAnnotations := b.Annotations
if bAnnotations == nil {
bAnnotations = map[string]string{}
}
for name := range InternalLBAnnotations[platform.Type] {
if aAnnotations[name] != bAnnotations[name] {
return false
}
}
for name := range externalLBAnnotations[platform.Type] {
if aAnnotations[name] != bAnnotations[name] {
return false
}
}
return true
}

// shouldRecreateLoadBalancer determines whether a load balancer needs to be
// recreated and returns the reason for its recreation.
func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, string) {
_, platformHasMutableScope := platformsWithMutableScope[platform.Type]
if !platformHasMutableScope && !scopeEqual(current, desired, platform) {
return true, "its scope changed"
}
if platform.Type == configv1.AWSPlatformType && !serviceSubnetsEqual(current, desired) {
return true, "its subnets changed"
}
if platform.Type == configv1.AWSPlatformType && !serviceEIPAllocationsEqual(current, desired) {
return true, "its eipAllocations changed"
}
return false, ""
}

// loadBalancerServiceChanged checks if the current load balancer service
// matches the expected and if not returns an updated one.
func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev1.Service) {
Expand All @@ -675,7 +681,7 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev
// avoid problems, make sure the previous release blocks upgrades when
// the user has modified an annotation or spec field that the new
// release manages.
changed, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedLoadBalancerServiceAnnotations)
changed, _, updated := loadBalancerServiceAnnotationsChanged(current, expected, managedLBServiceAnnotations)

// If spec.loadBalancerSourceRanges is nonempty on the service, that
// means that allowedSourceRanges is nonempty on the ingresscontroller,
Expand Down Expand Up @@ -703,18 +709,18 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev

// loadBalancerServiceAnnotationsChanged checks if the annotations on the expected Service
// match the ones on the current Service.
func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations sets.String) (bool, *corev1.Service) {
changed := false
for annotation := range annotations {
func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations []string) (bool, []string, *corev1.Service) {
var changedAnnotations []string
for _, annotation := range annotations {
currentVal, have := current.Annotations[annotation]
expectedVal, want := expected.Annotations[annotation]
if (want && (!have || currentVal != expectedVal)) || (have && !want) {
changed = true
changedAnnotations = append(changedAnnotations, annotation)
break
}
}
if !changed {
return false, nil
if len(changedAnnotations) == 0 {
return false, nil, nil
}

updated := current.DeepCopy()
Expand All @@ -723,7 +729,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an
updated.Annotations = map[string]string{}
}

for annotation := range annotations {
for _, annotation := range annotations {
currentVal, have := current.Annotations[annotation]
expectedVal, want := expected.Annotations[annotation]
if want && (!have || currentVal != expectedVal) {
Expand All @@ -733,7 +739,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an
}
}

return true, updated
return true, changedAnnotations, updated
}

// IsServiceInternal returns a Boolean indicating whether the provided service
Expand All @@ -752,8 +758,8 @@ func IsServiceInternal(service *corev1.Service) bool {
}

// loadBalancerServiceTagsModified verifies that none of the managedAnnotations have been changed and also the AWS tags annotation
func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, *corev1.Service) {
ignoredAnnotations := managedLoadBalancerServiceAnnotations.Union(sets.NewString(awsLBAdditionalResourceTags))
func loadBalancerServiceTagsModified(current, expected *corev1.Service) (bool, []string, *corev1.Service) {
ignoredAnnotations := append(managedLBServiceAnnotations, awsLBAdditionalResourceTags)
return loadBalancerServiceAnnotationsChanged(current, expected, ignoredAnnotations)
}

Expand All @@ -773,7 +779,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
return nil
}

changed, updated := loadBalancerServiceTagsModified(current, desired)
changed, _, updated := loadBalancerServiceTagsModified(current, desired)
if changed {
diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
return fmt.Errorf("load balancer service has been modified; changes must be reverted before upgrading: %s", diff)
Expand All @@ -799,12 +805,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, haveLBType)
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 +853,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 +984,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 @@ -1025,16 +1058,6 @@ func getEIPAllocationsFromServiceAnnotation(service *corev1.Service) []operatorv
return awsEIPAllocations
}

// serviceSubnetsEqual compares the subnet annotations on two services to determine if they are equivalent,
// ignoring the order of the subnets.
func serviceSubnetsEqual(a, b *corev1.Service) bool {
return awsSubnetsEqual(getSubnetsFromServiceAnnotation(a), getSubnetsFromServiceAnnotation(b))
}

func serviceEIPAllocationsEqual(a, b *corev1.Service) bool {
return awsEIPAllocationsEqual(getEIPAllocationsFromServiceAnnotation(a), getEIPAllocationsFromServiceAnnotation(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 +1203,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
Loading

0 comments on commit 9d3a208

Please sign in to comment.