Skip to content

Commit

Permalink
Introduce managedAtServiceCreationLBServiceAnnotations Map
Browse files Browse the repository at this point in the history 
Refactor to introduce a new map,
`managedAtServiceCreationLBServiceAnnotations`, which tracks
annotations that are managed by the operator but are only
applied to the load balancer at the time of service creation.
These annotations are also responsible for triggering the
`Progressing: True` status when modified.

This addition not only improves documentation of these annotations,
but also simplifies the code. The `loadBalancerServiceAnnotationsChanged`
function has been adapted to replace `shouldRecreateLoadBalancer`,
allowing the removal of the `scopeEqual` function as well.
  • Loading branch information
@gcs278
gcs278 committed Nov 28, 2024
1 parent 72d16d3 commit 2a26973
Show file tree
Hide file tree
Showing 2 changed files with 92 additions and 89 deletions.
135 changes: 67 additions & 68 deletions pkg/operator/controller/ingress/load_balancer_service.go
View file
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"

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

Expand Down Expand Up @@ -218,9 +217,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 +230,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 @@ -279,6 +280,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 {
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 @@ -603,8 +653,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 @@ -629,46 +679,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 @@ -680,7 +690,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 @@ -708,18 +718,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 @@ -738,7 +747,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 @@ -776,7 +785,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)
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 @@ -1028,16 +1037,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
46 changes: 25 additions & 21 deletions pkg/operator/controller/ingress/load_balancer_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/sets"
)

func Test_desiredLoadBalancerService(t *testing.T) {
Expand Down Expand Up @@ -1240,32 +1239,33 @@ func Test_loadBalancerServiceChanged(t *testing.T) {
// loadBalancerServiceAnnotationsChanged behaves correctly.
func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) {
testCases := []struct {
description string
mutate func(*corev1.Service)
currentAnnotations map[string]string
expectedAnnotations map[string]string
managedAnnotations sets.String
expect bool
description string
mutate func(*corev1.Service)
currentAnnotations map[string]string
expectedAnnotations map[string]string
managedAnnotations []string
expect bool
expectChangedAnnotations []string
}{
{
description: "if current and expected annotations are both empty",
currentAnnotations: map[string]string{},
expectedAnnotations: map[string]string{},
managedAnnotations: sets.NewString("foo"),
managedAnnotations: []string{"foo"},
expect: false,
},
{
description: "if current annotations is nil and expected annotations is empty",
currentAnnotations: nil,
expectedAnnotations: map[string]string{},
managedAnnotations: sets.NewString("foo"),
managedAnnotations: []string{"foo"},
expect: false,
},
{
description: "if current annotations is empty and expected annotations is nil",
currentAnnotations: map[string]string{},
expectedAnnotations: nil,
managedAnnotations: sets.NewString("foo"),
managedAnnotations: []string{"foo"},
expect: false,
},
{
Expand All @@ -1277,7 +1277,7 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) {
"foo": "bar",
"baz": "quux",
},
managedAnnotations: sets.NewString("foo"),
managedAnnotations: []string{"foo"},
expect: false,
},
{
Expand All @@ -1286,8 +1286,9 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) {
expectedAnnotations: map[string]string{
"foo": "bar",
},
managedAnnotations: sets.NewString("foo"),
expect: true,
managedAnnotations: []string{"foo"},
expect: true,
expectChangedAnnotations: []string{"foo"},
},
{
description: "if a managed annotation is updated",
Expand All @@ -1297,17 +1298,19 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) {
expectedAnnotations: map[string]string{
"foo": "baz",
},
managedAnnotations: sets.NewString("foo"),
expect: true,
managedAnnotations: []string{"foo"},
expect: true,
expectChangedAnnotations: []string{"foo"},
},
{
description: "if a managed annotation is deleted",
currentAnnotations: map[string]string{
"foo": "bar",
},
expectedAnnotations: map[string]string{},
managedAnnotations: sets.NewString("foo"),
expect: true,
expectedAnnotations: map[string]string{},
managedAnnotations: []string{"foo"},
expect: true,
expectChangedAnnotations: []string{"foo"},
},
}

Expand All @@ -1323,13 +1326,14 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) {
Annotations: tc.expectedAnnotations,
},
}
if changed, updated := loadBalancerServiceAnnotationsChanged(&current, &expected, tc.managedAnnotations); changed != tc.expect {
if changed, changedAnnotations, updated := loadBalancerServiceAnnotationsChanged(&current, &expected, tc.managedAnnotations); changed != tc.expect {
t.Errorf("expected loadBalancerServiceAnnotationsChanged to be %t, got %t", tc.expect, changed)
} else if changed {
if updatedChanged, _ := loadBalancerServiceAnnotationsChanged(&current, updated, tc.managedAnnotations); !updatedChanged {
assert.Equal(t, tc.expectChangedAnnotations, changedAnnotations)
if updatedChanged, _, _ := loadBalancerServiceAnnotationsChanged(&current, updated, tc.managedAnnotations); !updatedChanged {
t.Error("loadBalancerServiceAnnotationsChanged reported changes but did not make any update")
}
if changedAgain, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain {
if changedAgain, _, _ := loadBalancerServiceAnnotationsChanged(&expected, updated, tc.managedAnnotations); changedAgain {
t.Error("loadBalancerServiceAnnotationsChanged does not behave as a fixed point function")
}
}
Expand Down

0 comments on commit 2a26973

Please sign in to comment.