Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+562 −233
Open

OCPBUGS-16728: Require Service Deletion for LB Type Updates #1142

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c25c6c1
Update Broken Annotation Documentation Links
gcs278 Oct 18, 2024
3dcff76
Introduce `managedAtServiceCreationLBServiceAnnotations` Map
gcs278 Oct 18, 2024
0d249ce
OCPBUGS-16728: Require Service Deletion for LB Type Updates
gcs278 Sep 9, 2024
5862310
[WILL SQUASH] Refactor IsProxyProtocolNeeded
gcs278 Sep 18, 2024
1a57afd
[WILL SQUASH] Move inactive LB parameters clearing to status.go
gcs278 Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 74 additions & 66 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"encoding/json"
"fmt"
"k8s.io/apimachinery/pkg/util/sets"
"reflect"
"strconv"
"strings"
Expand All @@ -23,8 +24,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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This import got erroneously moved.


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

Expand Down Expand Up @@ -203,9 +202,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 @@ -214,8 +215,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() sets.Set[string] {
result := sets.New[string](
// AWS LB health check interval annotation (see
// <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>).
awsLBHealthCheckIntervalAnnotation,
Expand Down Expand Up @@ -264,6 +265,55 @@ var (

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]sets.Set[string] {
result := map[configv1.PlatformType]sets.Set[string]{
configv1.AWSPlatformType: sets.New[string](
// 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 {
Comment on lines +298 to +300
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for platform, annotation := range InternalLBAnnotations {
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotation {
for platform, annotations := range InternalLBAnnotations {
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotations {

if result[platform] == nil {
result[platform] = sets.New[string]()
}
result[platform].Insert(name)
}
}
Comment on lines +299 to +306
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little easier to read:

Suggested change
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotation {
if result[platform] == nil {
result[platform] = sets.New[string]()
}
result[platform].Insert(name)
}
}
if _, ok := platformsWithMutableScope[platform]; ok {
continue
}
for name := range annotation {
if result[platform] == nil {
result[platform] = sets.New[string]()
}
result[platform].Insert(name)
}

}
for platform, annotation := range externalLBAnnotations {
if _, ok := platformsWithMutableScope[platform]; !ok {
for name := range annotation {
if result[platform] == nil {
result[platform] = sets.New[string]()
}
result[platform].Insert(name)
}
}
}
return result
}()
)

// ensureLoadBalancerService creates an LB service if one is desired but absent.
Expand Down Expand Up @@ -598,8 +648,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, changedFields := shouldRecreateLoadBalancer(current, desired, platform); shouldRecreateLB && autoDeleteLB {
log.Info("deleting and recreating the load balancer", "annotations", changedAnnotations, "fields", changedFields, "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,47 +674,16 @@ 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"
}
// recreated and returns the changed annotations and fields that require recreation.
func shouldRecreateLoadBalancer(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, []string, []string) {
shouldRecreateLB, changedAnnotations, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedAtServiceCreationLBServiceAnnotations[platform.Type])
var changedFields []string
if platform.Type == configv1.OpenStackPlatformType && current.Spec.LoadBalancerIP != desired.Spec.LoadBalancerIP {
return true, "its load balancer IP changed"
shouldRecreateLB = true
changedFields = append(changedFields, "spec.loadBalancerIP")
}
return false, ""
return shouldRecreateLB, changedAnnotations, changedFields
}

// loadBalancerServiceChanged checks if the current load balancer service
Expand All @@ -678,7 +697,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 @@ -706,18 +725,17 @@ 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
func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, annotations sets.Set[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
break
changedAnnotations = append(changedAnnotations, annotation)
}
}
if !changed {
return false, nil
if len(changedAnnotations) == 0 {
return false, nil, nil
}

updated := current.DeepCopy()
Expand All @@ -736,7 +754,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 Down Expand Up @@ -774,7 +792,7 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme
// Since the status logic runs after the controller sets the annotations, it checks for
// any discrepancy (in case modified) between the desired annotation values of the controller
// and the current annotation values.
changed, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLoadBalancerServiceAnnotations)
changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
changed, updated, _ := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)
changed, _, updated := loadBalancerServiceAnnotationsChanged(current, desired, managedLBServiceAnnotations)

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 Down Expand Up @@ -1047,16 +1065,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
Loading