From 9b665ce50968b188881f8a6db2d93dc545ae7071 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 18 Oct 2024 18:05:13 -0400 Subject: [PATCH 1/5] Update Broken Annotation Documentation Links Some links in `load_balancer_service.go` were invalid due to K8S documentation URL changes. --- pkg/operator/controller/ingress/load_balancer_service.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 562068fa7..1989226e5 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -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). From ee99c15d4799f1e424baa833a93391041284a980 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Fri, 18 Oct 2024 17:53:12 -0400 Subject: [PATCH 2/5] Introduce `managedAtServiceCreationLBServiceAnnotations` Map 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 existing logic in `shouldRecreateLoadBalancer`, allowing the removal of the `scopeEqual` function as well. --- .../ingress/load_balancer_service.go | 140 +++++++++--------- .../ingress/load_balancer_service_test.go | 89 ++++++----- 2 files changed, 130 insertions(+), 99 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 1989226e5..d17a5419e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "k8s.io/apimachinery/pkg/util/sets" "reflect" "strconv" "strings" @@ -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" ) @@ -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 @@ -229,8 +230,8 @@ var ( // ). 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 // ). awsLBHealthCheckIntervalAnnotation, @@ -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. @@ -618,8 +668,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 { @@ -644,47 +694,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 @@ -698,7 +717,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, @@ -726,18 +745,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() @@ -756,7 +774,7 @@ func loadBalancerServiceAnnotationsChanged(current, expected *corev1.Service, an } } - return true, updated + return true, changedAnnotations, updated } // IsServiceInternal returns a Boolean indicating whether the provided service @@ -794,7 +812,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) @@ -1067,16 +1085,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 { diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 44e779861..d81d17d93 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -5,6 +5,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -1290,32 +1292,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 sets.Set[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: sets.New[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: sets.New[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: sets.New[string]("foo"), expect: false, }, { @@ -1327,7 +1330,7 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { "foo": "bar", "baz": "quux", }, - managedAnnotations: sets.NewString("foo"), + managedAnnotations: sets.New[string]("foo"), expect: false, }, { @@ -1336,8 +1339,9 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "bar", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: sets.New[string]("foo"), + expect: true, + expectChangedAnnotations: []string{"foo"}, }, { description: "if a managed annotation is updated", @@ -1347,17 +1351,19 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { expectedAnnotations: map[string]string{ "foo": "baz", }, - managedAnnotations: sets.NewString("foo"), - expect: true, + managedAnnotations: sets.New[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: sets.New[string]("foo"), + expect: true, + expectChangedAnnotations: []string{"foo"}, }, } @@ -1373,13 +1379,14 @@ func Test_loadBalancerServiceAnnotationsChanged(t *testing.T) { Annotations: tc.expectedAnnotations, }, } - if changed, updated := loadBalancerServiceAnnotationsChanged(¤t, &expected, tc.managedAnnotations); changed != tc.expect { + if changed, changedAnnotations, updated := loadBalancerServiceAnnotationsChanged(¤t, &expected, tc.managedAnnotations); changed != tc.expect { t.Errorf("expected loadBalancerServiceAnnotationsChanged to be %t, got %t", tc.expect, changed) } else if changed { - if updatedChanged, _ := loadBalancerServiceAnnotationsChanged(¤t, updated, tc.managedAnnotations); !updatedChanged { + assert.Equal(t, tc.expectChangedAnnotations, changedAnnotations) + if updatedChanged, _, _ := loadBalancerServiceAnnotationsChanged(¤t, 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") } } @@ -1643,12 +1650,13 @@ func TestLoadBalancerServiceChangedEmptyAnnotations(t *testing.T) { // recreated if the annotations or spec of the service have changed. func Test_shouldRecreateLoadBalancer(t *testing.T) { testCases := []struct { - description string - current *corev1.Service - desired *corev1.Service - platform *configv1.PlatformStatus - expect bool - reason string + description string + current *corev1.Service + desired *corev1.Service + platform *configv1.PlatformStatus + expect bool + expectChangedAnnotations []string + expectChangedFields []string }{ { description: "AWS platform with different subnets", @@ -1670,7 +1678,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its subnets changed", + expectChangedAnnotations: []string{ + awsLBSubnetsAnnotation, + }, }, { description: "AWS platform with different EIP allocations", @@ -1692,7 +1702,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its eipAllocations changed", + expectChangedAnnotations: []string{ + awsEIPAllocationsAnnotation, + }, }, { description: "OpenStack platform with different LoadBalancerIP", @@ -1710,7 +1722,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.OpenStackPlatformType, }, expect: true, - reason: "its load balancer IP changed", + expectChangedFields: []string{ + "spec.loadBalancerIP", + }, }, { description: "Platform with mutable scope and same scope", @@ -1753,7 +1767,9 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { Type: configv1.AWSPlatformType, }, expect: true, - reason: "its scope changed", + expectChangedAnnotations: []string{ + awsInternalLBAnnotation, + }, }, { description: "Platform with same configuration", @@ -1780,12 +1796,19 @@ func Test_shouldRecreateLoadBalancer(t *testing.T) { for _, tc := range testCases { t.Run(tc.description, func(t *testing.T) { - changed, reason := shouldRecreateLoadBalancer(tc.current, tc.desired, tc.platform) + changed, changedAnnotations, changedFields := shouldRecreateLoadBalancer(tc.current, tc.desired, tc.platform) if changed != tc.expect { t.Errorf("expected %t, got %t", tc.expect, changed) } - if reason != tc.reason { - t.Errorf("expected reason %s, got %s", tc.reason, reason) + sliceCmpOpts := []cmp.Option{ + cmpopts.EquateEmpty(), + cmpopts.SortSlices(func(a, b string) bool { return a < b }), + } + if !cmp.Equal(changedAnnotations, tc.expectChangedAnnotations, sliceCmpOpts...) { + t.Errorf("expected reason %s, got %s", tc.expectChangedAnnotations, changedAnnotations) + } + if !cmp.Equal(changedFields, tc.expectChangedFields, sliceCmpOpts...) { + t.Errorf("expected reason %s, got %s", tc.expectChangedFields, changedFields) } }) } From e8aeefc4f3dab46b84f81b48f74b2fac47140471 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 9 Sep 2024 18:55:43 -0400 Subject: [PATCH 3/5] OCPBUGS-16728: Require Service Deletion for LB Type Updates 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. This commit does not alter the function of the LB type in the status, which has always (incorrectly) represented the desired state. However, with this change, a new scenario is introduced where the desired LB type may differ from the actual state. Consequently, the LB type status may no longer reliably reflect the actual state as it previously did, potentially surprising users who mistakenly assumed it always indicated the current state. --- pkg/operator/controller/ingress/controller.go | 4 + .../ingress/load_balancer_service.go | 76 ++++++++++-- .../ingress/load_balancer_service_test.go | 9 +- pkg/operator/controller/ingress/status.go | 4 +- .../controller/ingress/status_test.go | 72 ++++++++---- test/e2e/operator_test.go | 111 ++++++++++++------ 6 files changed, 203 insertions(+), 73 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 9e67518f2..83d877e2c 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -440,6 +440,10 @@ func setDefaultDomain(ic *operatorv1.IngressController, ingressConfig *configv1. } func setDefaultPublishingStrategy(ic *operatorv1.IngressController, platformStatus *configv1.PlatformStatus, domainMatchesBaseDomain bool, ingressConfig *configv1.Ingress, alreadyAdmitted bool) bool { + // WARNING: setDefaultPublishingStrategy follows an outdated pattern of using spec and status. It uses the status + // to represent the *desired* state (spec + defaulting), but per Kubernetes standards, the status should reflect + // the *actual* state. As a result, this function is considered legacy. For new API fields, use spec as the desired + // state and syncIngressControllerStatus to populate the status with the actual state. effectiveStrategy := ic.Spec.EndpointPublishingStrategy.DeepCopy() if effectiveStrategy == nil { var strategyType operatorv1.EndpointPublishingStrategyType diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index d17a5419e..2f10ee871 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -243,8 +243,6 @@ var ( // local-with-fallback annotation for kube-proxy (see // ). 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. // @@ -294,6 +292,11 @@ var ( managedAtServiceCreationLBServiceAnnotations = func() map[configv1.PlatformType]sets.Set[string] { result := map[configv1.PlatformType]sets.Set[string]{ configv1.AWSPlatformType: sets.New[string]( + // 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 @@ -821,29 +824,61 @@ func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deployme return nil } +// effectuateMessage returns a message describing how to effectuate a +// change that requires the service to be deleted. +func effectuateMessage(changedMsg, ocPatchRevertCmd string, service *corev1.Service) string { + return fmt.Sprintf( + "%[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`. "+ + "Direct updates to the service annotations are not supported.", + changedMsg, service.Namespace, service.Name, ocPatchRevertCmd, + ) +} + // loadBalancerServiceIsProgressing returns an error value indicating if the // load balancer service is in progressing status. +// Tricky: Status fields have mixed functions. Older status fields often represent the +// *desired* state, while some newer fields reflect the *actual* state of the object. func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { var errs []error + // Tricky: Scope in the status indicates the *desired* scope (user desire + defaulting). wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope haveScope := operatorv1.ExternalLoadBalancer if IsServiceInternal(service) { haveScope = operatorv1.InternalLoadBalancer } if wantScope != haveScope { - err := fmt.Errorf("The IngressController scope was changed from %q to %q.", haveScope, wantScope) + changedMsg := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope) + err := fmt.Errorf(changedMsg) if _, ok := platformsWithMutableScope[platform.Type]; !ok { - 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 from the old one's. Alternatively, you can revert the change to the IngressController: `oc -n openshift-ingress-operator patch ingresscontrollers/%[4]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[5]s\"}}}}'`", err.Error(), service.Namespace, service.Name, ic.Name, haveScope) + ocPatchRevertCmd := fmt.Sprintf("oc -n openshift-ingress-operator patch ingresscontrollers/%[1]s --type=merge --patch='{\"spec\":{\"endpointPublishingStrategy\":{\"loadBalancer\":{\"scope\":\"%[2]s\"}}}}'", ic.Name, haveScope) + err = fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) } errs = append(errs, err) } + haveLBType := getAWSLoadBalancerTypeFromServiceAnnotation(service) + if platform.Type == configv1.AWSPlatformType { + // Tricky: LB Type in the status indicates the *desired* scope (user desire + defaulting). + wantLBType := getAWSLoadBalancerTypeInStatus(ic) + + if wantLBType != haveLBType { + changedMsg := fmt.Sprintf("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(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) + errs = append(errs, err) + } + } + if platform.Type == configv1.AWSPlatformType && subnetsAWSEnabled { var ( + // Tricky: Subnets in the status indicates the *actual* scope. wantSubnets, haveSubnets *operatorv1.AWSSubnets paramsFieldName string ) - switch getAWSLoadBalancerTypeInStatus(ic) { + switch haveLBType { case operatorv1.AWSNetworkLoadBalancer: if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { wantSubnets = nlbParams.Subnets @@ -868,14 +903,15 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service haveSubnetsPrettyJson := convertAWSSubnetListToPatchJson(haveSubnets, "{}", "[]") wantSubnetsPrettyJson := convertAWSSubnetListToPatchJson(wantSubnets, "{}", "[]") changedMsg := fmt.Sprintf("The IngressController subnets were changed from %q to %q.", haveSubnetsPrettyJson, wantSubnetsPrettyJson) - 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\",\"%[3]s\":{\"subnets\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), paramsFieldName, haveSubnetsPatchJson) - 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) + 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\",\"%[3]s\":{\"subnets\":%[4]s}}}}}}}'", ic.Name, haveLBType, paramsFieldName, haveSubnetsPatchJson) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } - if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && getAWSLoadBalancerTypeInStatus(ic) == operatorv1.AWSNetworkLoadBalancer { + if platform.Type == configv1.AWSPlatformType && eipAllocationsAWSEnabled && haveLBType == operatorv1.AWSNetworkLoadBalancer { var ( + // Tricky: EIP Allocations in the status indicate the *actual* scope. wantEIPAllocations, haveEIPAllocations []operatorv1.EIPAllocation ) if nlbParams := getAWSNetworkLoadBalancerParametersInSpec(ic); nlbParams != nil { @@ -891,20 +927,21 @@ func loadBalancerServiceIsProgressing(ic *operatorv1.IngressController, service haveEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(haveEIPAllocations, "[]") wantEIPAllocationsPrettyJson := convertAWSEIPAllocationsListToPatchJson(wantEIPAllocations, "[]") changedMsg := fmt.Sprintf("The IngressController eipAllocations were changed from %q to %q.", haveEIPAllocationsPrettyJson, wantEIPAllocationsPrettyJson) - 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\",\"%[3]s\":{\"eipAllocations\":%[4]s}}}}}}}'", ic.Name, getAWSLoadBalancerTypeInStatus(ic), "networkLoadBalancer", haveEIPAllocationsPatchJson) - 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) + 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\",\"%[3]s\":{\"eipAllocations\":%[4]s}}}}}}}'", ic.Name, haveLBType, "networkLoadBalancer", haveEIPAllocationsPatchJson) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } if platform.Type == configv1.OpenStackPlatformType { + // Tricky: FloatingIP in the status indicate the *actual* scope. wantFloatingIP := getOpenStackFloatingIPInSpec(ic) haveFloatingIP := getOpenStackFloatingIPInStatus(ic) // OpenStack CCM does not support updating Service.Spec.LoadBalancerIP after creation, the load balancer will never be updated. if wantFloatingIP != haveFloatingIP { - changeMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP) + changedMsg := fmt.Sprintf("The IngressController floatingIP was changed from %q to %q.", haveFloatingIP, wantFloatingIP) ocPatchRevertCmd := fmt.Sprintf(`oc -n %[1]s patch ingresscontrollers/%[2]s --type=merge --patch='{"spec":{"endpointPublishingStrategy":{"type":"LoadBalancerService","loadBalancer":{"providerParameters":{"type":"OpenStack","openstack":{"floatingIP":"%[3]s"}}}}}}'`, ic.Namespace, ic.Name, haveFloatingIP) - 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`", changeMsg, service.Namespace, service.Name, ocPatchRevertCmd) + err := fmt.Errorf(effectuateMessage(changedMsg, ocPatchRevertCmd, service)) errs = append(errs, err) } } @@ -1017,6 +1054,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) } +// getAWSLoadBalancerTypeFromServiceAnnotation 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 getAWSLoadBalancerTypeFromServiceAnnotation(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. diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index d81d17d93..d65d3bd3d 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -1222,7 +1222,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", diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index e0df9564d..a258c8192 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -954,7 +954,7 @@ func computeLoadBalancerProgressingStatus(ic *operatorv1.IngressController, serv func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, service *corev1.Service) { // Set the subnets status based on the actual service annotation and the load balancer type, // as NLBs and CLBs have separate subnet configuration fields. - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLoadBalancerTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: // NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy // when an IngressController is admitted, so we don't need to initialize here. @@ -974,7 +974,7 @@ func updateIngressControllerAWSSubnetStatus(ic *operatorv1.IngressController, se // sync its status to the effective eipAllocations on the LoadBalancer-type service. func updateIngressControllerAWSEIPAllocationStatus(ic *operatorv1.IngressController, service *corev1.Service) { // Set the eipAllocations status based on the actual service annotation and on the load balancer type `NLB`. - switch getAWSLoadBalancerTypeInStatus(ic) { + switch getAWSLoadBalancerTypeFromServiceAnnotation(service) { case operatorv1.AWSNetworkLoadBalancer: // NetworkLoadBalancerParameters should be initialized by setDefaultPublishingStrategy // when an IngressController is admitted, so we don't need to initialize here. diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index bd2a0ca15..c22c3e80b 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -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{ @@ -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{ @@ -790,6 +794,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{ @@ -935,7 +946,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -947,7 +958,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, &operatorv1.AWSSubnets{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -961,7 +972,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: false, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -975,7 +986,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { }, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -989,7 +1000,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { IDs: []operatorv1.AWSSubnetID{"subnet-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1007,7 +1018,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-12345"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1025,7 +1036,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { Names: []operatorv1.AWSSubnetName{"name-67890"}, }, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsSubnetsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionTrue, @@ -1043,7 +1054,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, @@ -1061,7 +1072,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, @@ -1196,7 +1207,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, nil, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1207,7 +1218,7 @@ func Test_computeLoadBalancerProgressingStatus(t *testing.T) { nil, []operatorv1.EIPAllocation{}, ), - service: &corev1.Service{}, + service: lbServiceWithNLB, awsEIPAllocationsEnabled: true, platformStatus: awsPlatformStatus, expectStatus: operatorv1.ConditionFalse, @@ -1218,7 +1229,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, @@ -1229,7 +1240,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, @@ -1240,7 +1251,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, @@ -1251,7 +1262,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, @@ -1262,7 +1273,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, @@ -1273,7 +1284,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, @@ -1284,11 +1295,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), IC Status LBType Classic", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSClassicLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "LBType Classic LoadBalancerService, IC Status LBType NLB", + ic: loadBalancerIngressControllerWithLBType(operatorv1.AWSNetworkLoadBalancer), + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "LBType NLB LoadBalancerService, IC Status 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) { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index b9fb0d818..62c31b09b 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -1371,19 +1371,23 @@ func TestAWSResourceTagsChanged(t *testing.T) { assertLoadBalancerServiceAnnotationWithPollImmediate(t, kclient, defaultIC, awsLBAdditionalResourceTags, expectedTags) } +// TestAWSLBTypeChange verifies that process of changing the IngressController's +// load balancer type, ensuring that it requires the associated service to be deleted +// and recreated for the change to take effect. func TestAWSLBTypeChange(t *testing.T) { t.Parallel() - if infraConfig.Status.Platform != "AWS" { - t.Skipf("test skipped on platform %q", infraConfig.Status.Platform) + if infraConfig.Status.PlatformStatus.Type != configv1.AWSPlatformType { + t.Skipf("test skipped on platform %q", infraConfig.Status.PlatformStatus.Type) } - name := types.NamespacedName{Namespace: operatorNamespace, Name: "awslb"} + name := types.NamespacedName{Namespace: operatorNamespace, Name: "aws-lb-type-change"} ic := newLoadBalancerController(name, name.Name+"."+dnsConfig.Spec.BaseDomain) ic.Spec.EndpointPublishingStrategy.LoadBalancer = &operatorv1.LoadBalancerStrategy{ Scope: operatorv1.ExternalLoadBalancer, } - if err := kclient.Create(context.TODO(), ic); err != nil { + t.Logf("creating ingresscontroller %q without specifying LB type", ic.Name) + if err := kclient.Create(context.Background(), ic); err != nil { t.Fatalf("failed to create ingresscontroller: %v", err) } defer assertIngressControllerDeleted(t, kclient, ic) @@ -1393,50 +1397,81 @@ func TestAWSLBTypeChange(t *testing.T) { t.Fatalf("failed to observe expected conditions: %v", err) } - lbService := &corev1.Service{} - if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), lbService); err != nil { - t.Fatalf("failed to get LoadBalancer service: %v", err) - } - if v := lbService.Annotations[ingresscontroller.AWSLBTypeAnnotation]; len(v) != 0 { - t.Fatalf("load balancer service has unexpected %s=%s annotation", ingresscontroller.AWSLBTypeAnnotation, v) - } + // LB Annotation should be empty by default (meaning CLB). + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") - pp := &operatorv1.ProviderLoadBalancerParameters{ - Type: operatorv1.AWSLoadBalancerProvider, - AWS: &operatorv1.AWSLoadBalancerParameters{ - Type: operatorv1.AWSNetworkLoadBalancer, - }, + // Update IngressController to use NLB. + t.Logf("updating ingresscontroller %q to change the LB type to NLB", ic.Name) + if err := updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ + Type: operatorv1.AWSLoadBalancerProvider, + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: operatorv1.AWSNetworkLoadBalancer, + }, + } + }); err != nil { + t.Fatalf("failed to update ingresscontroller: %v", err) } - if err := updateIngressControllerWithRetryOnConflict(t, name, timeout, func(ic *operatorv1.IngressController) { - ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = pp + // Effectuate the LB Type change. + effectuateIngressControllerLBType(t, ic, operatorv1.AWSNetworkLoadBalancer, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...) + + // Now, update the IngressController switch back to CLB, but let's use the + // auto-delete-load-balancer annotation, so we don't have to manually delete the service. + t.Logf("updating ingresscontroller %q to use CLB while using the auto-delete-load-balancer annotation", ic.Name) + if err := updateIngressControllerWithRetryOnConflict(t, name, 5*time.Minute, func(ic *operatorv1.IngressController) { + if ic.Annotations == nil { + ic.Annotations = map[string]string{} + } + ic.Annotations["ingress.operator.openshift.io/auto-delete-load-balancer"] = "" + ic.Spec.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type = operatorv1.AWSClassicLoadBalancer }); err != nil { t.Fatalf("failed to update ingresscontroller: %v", err) } - // Wait for the load balancer and DNS to be ready. - if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { + // Verify the LB type annotation is removed on the service (default to CLB). + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") + + // Expect the load balancer to provision successfully. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil { t.Fatalf("failed to observe expected conditions: %v", err) } +} - err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) { - service := &corev1.Service{} - if err := kclient.Get(context.TODO(), controller.LoadBalancerServiceName(ic), service); err != nil { - t.Logf("failed to get service %s: %v", controller.LoadBalancerServiceName(ic), err) - return false, nil - } - if actual, ok := service.Annotations[ingresscontroller.AWSLBTypeAnnotation]; !ok { - t.Logf("load balancer has no %q annotation: %v", ingresscontroller.AWSLBTypeAnnotation, service.Annotations) - return false, nil - } else if actual != ingresscontroller.AWSNLBAnnotation { - t.Logf("expected %s=%s, found %s=%s", ingresscontroller.AWSLBTypeAnnotation, ingresscontroller.AWSNLBAnnotation, - ingresscontroller.AWSLBTypeAnnotation, actual) - return false, nil - } - return true, nil - }) - if err != nil { - t.Fatalf("timed out waiting for the service LB type annotation to be updated: %v", err) +// effectuateIngressControllerLBType manually effectuates updated IngressController LB type by +// confirming IngressController is in a progressing state, deleting the service, and waiting for +// the expected LB type annotation to appear on the service. It waits for the provided operator +// conditions after the LB type has been effectuated. +func effectuateIngressControllerLBType(t *testing.T, ic *operatorv1.IngressController, expectedLBType operatorv1.AWSLoadBalancerType, expectedOperatorConditions ...operatorv1.OperatorCondition) { + t.Helper() + t.Logf("effectuating LB type for IngressController %s", ic.Name) + icName := types.NamespacedName{Name: ic.Name, Namespace: ic.Namespace} + progressingTrue := operatorv1.OperatorCondition{ + Type: ingresscontroller.IngressControllerLoadBalancerProgressingConditionType, + Status: operatorv1.ConditionTrue, + } + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, progressingTrue); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Delete and recreate the IngressController service to effectuate. + t.Logf("recreating the service to effectuate the LB type: %s/%s", controller.LoadBalancerServiceName(ic).Namespace, controller.LoadBalancerServiceName(ic).Name) + if err := recreateIngressControllerService(t, ic); err != nil { + t.Fatalf("failed to delete and recreate service: %v", err) + } + + // Verify we get the expected LB type annotation on the service. + if expectedLBType == operatorv1.AWSClassicLoadBalancer { + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, false, "") + } else if expectedLBType == operatorv1.AWSNetworkLoadBalancer { + waitForLBAnnotation(t, ic, ingresscontroller.AWSLBTypeAnnotation, true, ingresscontroller.AWSNLBAnnotation) + } else { + t.Fatalf("unsupported LB type: %s", expectedLBType) + } + + // Expect the load balancer to provision successfully with the new LB type. + if err := waitForIngressControllerCondition(t, kclient, 10*time.Minute, icName, expectedOperatorConditions...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) } } From 52ab987232aef9f74642ec76d4e2236c2fd9c061 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Wed, 18 Sep 2024 17:22:51 -0400 Subject: [PATCH 4/5] [WILL SQUASH] Refactor IsProxyProtocolNeeded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IsProxyProtocolNeeded must get the LB Type from the service, as the status now may not accurately reflect the actual LB Type during an update. However, we can't rely only on the service because it doesn’t exist during the initial bootstrap (and potentially when it gets deleted). In that case, it's safe to use the status to determine the anticipated LB Type. Without this commit results in a misalignment of the proxy protocol value between the load balancer and the router deployment when an LB type change is pending. This commit will be squashed into the previous one, as both are dependent on each other. I've kept them separate for easier review. --- pkg/operator/controller/ingress/controller.go | 35 +++++++++++++------ .../controller/ingress/controller_test.go | 25 ++++++++++++- pkg/operator/controller/ingress/deployment.go | 6 +--- .../controller/ingress/deployment_test.go | 12 +++---- .../ingress/load_balancer_service.go | 19 +++++----- .../ingress/load_balancer_service_test.go | 8 ++--- .../controller/ingress/status_test.go | 4 +-- 7 files changed, 69 insertions(+), 40 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 83d877e2c..229926fb4 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -1110,7 +1110,19 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d haveClientCAConfigmap = true } - haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig) + _, currentLBService, err := r.currentLoadBalancerService(ci) + if err != nil { + errs = append(errs, fmt.Errorf("failed to get load balancer service for %s: %v", ci.Name, err)) + return utilerrors.NewAggregate(errs) + } + + proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus, currentLBService) + if err != nil { + errs = append(errs, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err)) + return utilerrors.NewAggregate(errs) + } + + haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig, haveClientCAConfigmap, clientCAConfigmap, platformStatus, clusterProxyConfig, proxyNeeded) if err != nil { errs = append(errs, fmt.Errorf("failed to ensure deployment: %v", err)) return utilerrors.NewAggregate(errs) @@ -1129,7 +1141,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d } var wildcardRecord *iov1.DNSRecord - haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus) + haveLB, lbService, err := r.ensureLoadBalancerService(ci, deploymentRef, platformStatus, proxyNeeded) if err != nil { errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err)) } else { @@ -1224,23 +1236,26 @@ func IsStatusDomainSet(ingress *operatorv1.IngressController) bool { // IsProxyProtocolNeeded checks whether proxy protocol is needed based // upon the given ic and platform. -func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus) (bool, error) { +func IsProxyProtocolNeeded(ic *operatorv1.IngressController, platform *configv1.PlatformStatus, service *corev1.Service) (bool, error) { if platform == nil { return false, fmt.Errorf("platform status is missing; failed to determine if proxy protocol is needed for %s/%s", ic.Namespace, ic.Name) } switch ic.Status.EndpointPublishingStrategy.Type { case operatorv1.LoadBalancerServiceStrategyType: - // This can really be done for for any external [cloud] LBs that support the proxy protocol. + // This can really be done for any external [cloud] LBs that support the proxy protocol. switch platform.Type { case configv1.AWSPlatformType: - if ic.Status.EndpointPublishingStrategy.LoadBalancer == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS == nil || - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider && - ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type == operatorv1.AWSClassicLoadBalancer { - return true, nil + // TRICKY: If the service exists, use the LB Type annotation from the service, + // as status will be inaccurate if a LB Type update is pending. + // If the service does not exist, the status WILL be what determines the next LB Type. + var lbType operatorv1.AWSLoadBalancerType + if service != nil { + lbType = getAWSLoadBalancerTypeFromServiceAnnotation(service) + } else { + lbType = getAWSLoadBalancerTypeInStatus(ic) } + return lbType == operatorv1.AWSClassicLoadBalancer, nil case configv1.IBMCloudPlatformType: if ic.Status.EndpointPublishingStrategy.LoadBalancer != nil && ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index de931a014..84374d962 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -1471,11 +1471,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { Protocol: operatorv1.ProxyProtocol, }, } + serviceWithELB = corev1.Service{} + serviceWithNLB = corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } ) testCases := []struct { description string strategy *operatorv1.EndpointPublishingStrategy platform *configv1.PlatformStatus + service *corev1.Service expect bool expectError bool }{ @@ -1521,6 +1530,20 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { platform: &awsPlatform, expect: false, }, + { + description: "loadbalancer strategy with NLB, but a service with ELB should use PROXY", + strategy: &loadBalancerStrategyWithNLB, + platform: &awsPlatform, + service: &serviceWithELB, + expect: true, + }, + { + description: "loadbalancer strategy with ELB, but a service with NLB shouldn't use PROXY", + strategy: &loadBalancerStrategyWithNLB, + platform: &awsPlatform, + service: &serviceWithNLB, + expect: false, + }, { description: "loadbalancer strategy shouldn't use PROXY on Azure", strategy: &loadBalancerStrategy, @@ -1602,7 +1625,7 @@ func Test_IsProxyProtocolNeeded(t *testing.T) { EndpointPublishingStrategy: tc.strategy, }, } - switch actual, err := IsProxyProtocolNeeded(ic, tc.platform); { + switch actual, err := IsProxyProtocolNeeded(ic, tc.platform, tc.service); { case tc.expectError && err == nil: t.Error("expected error, got nil") case !tc.expectError && err != nil: diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go index 9bfcdf729..d5a238d2e 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -117,15 +117,11 @@ const ( // ensureRouterDeployment ensures the router deployment exists for a given // ingresscontroller. -func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy) (bool, *appsv1.Deployment, error) { +func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network, haveClientCAConfigmap bool, clientCAConfigmap *corev1.ConfigMap, platformStatus *configv1.PlatformStatus, clusterProxyConfig *configv1.Proxy, proxyNeeded bool) (bool, *appsv1.Deployment, error) { haveDepl, current, err := r.currentRouterDeployment(ci) if err != nil { return false, nil, err } - proxyNeeded, err := IsProxyProtocolNeeded(ci, platformStatus) - if err != nil { - return false, nil, fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ci.Namespace, ci.Name, err) - } desired, err := desiredRouterDeployment(ci, r.config.IngressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, haveClientCAConfigmap, clientCAConfigmap, clusterProxyConfig, r.config.RouteExternalCertificateEnabled, r.config.IngressControllerDCMEnabled) if err != nil { return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err) diff --git a/pkg/operator/controller/ingress/deployment_test.go b/pkg/operator/controller/ingress/deployment_test.go index c192bab92..ba3245e3b 100644 --- a/pkg/operator/controller/ingress/deployment_test.go +++ b/pkg/operator/controller/ingress/deployment_test.go @@ -379,7 +379,7 @@ func getRouterDeploymentComponents(t *testing.T) (*operatorv1.IngressController, }, }, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -683,7 +683,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { ic.Status.Domain = "example.com" ic.Status.EndpointPublishingStrategy.Type = operatorv1.LoadBalancerServiceStrategyType - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -781,7 +781,7 @@ func TestDesiredRouterDeploymentSpecAndNetwork(t *testing.T) { }, }, } - proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err = IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -891,7 +891,7 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) { networkConfig.Status.ClusterNetwork = []configv1.ClusterNetworkEntry{ {CIDR: "2620:0:2d0:200::7/32"}, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) if err != nil { t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) } @@ -1009,10 +1009,6 @@ func TestDesiredRouterDeploymentVariety(t *testing.T) { func TestDesiredRouterDeploymentHostNetworkNil(t *testing.T) { ic, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, clusterProxyConfig := getRouterDeploymentComponents(t) ic.Status.EndpointPublishingStrategy.Type = operatorv1.HostNetworkStrategyType - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) - if err != nil { - t.Fatal(err) - } deployment, err := desiredRouterDeployment(ic, ingressControllerImage, ingressConfig, infraConfig, apiConfig, networkConfig, proxyNeeded, false, nil, clusterProxyConfig, false, false) if err != nil { t.Fatal(err) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 2f10ee871..a41f0d40a 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -337,8 +337,8 @@ var ( // ensureLoadBalancerService creates an LB service if one is desired but absent. // Always returns the current LB service if one exists (whether it already // existed or was created during the course of the function). -func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus) (bool, *corev1.Service, error) { - wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled) +func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platformStatus *configv1.PlatformStatus, proxyNeeded bool) (bool, *corev1.Service, error) { + wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, platformStatus, r.config.IngressControllerLBSubnetsAWSEnabled, r.config.IngressControllerEIPAllocationsAWSEnabled, proxyNeeded) if err != nil { return false, nil, err } @@ -404,7 +404,7 @@ func isServiceOwnedByIngressController(service *corev1.Service, ic *operatorv1.I // ingresscontroller, or nil if an LB service isn't desired. An LB service is // desired if the high availability type is Cloud. An LB service will declare an // owner reference to the given deployment. -func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) (bool, *corev1.Service, error) { +func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool, proxyNeeded bool) (bool, *corev1.Service, error) { if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType { return false, nil, nil } @@ -434,11 +434,6 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations = map[string]string{} } - proxyNeeded, err := IsProxyProtocolNeeded(ci, platform) - if err != nil { - return false, nil, fmt.Errorf("failed to determine if proxy protocol is proxyNeeded for ingresscontroller %q: %v", ci.Name, err) - } - if platform != nil { if isInternal { annotation := InternalLBAnnotations[platform.Type] @@ -802,7 +797,11 @@ func IsServiceInternal(service *corev1.Service) bool { // the service, then the return value is a non-nil error indicating that the // modification must be reverted before upgrading is allowed. func loadBalancerServiceIsUpgradeable(ic *operatorv1.IngressController, deploymentRef metav1.OwnerReference, current *corev1.Service, platform *configv1.PlatformStatus, subnetsAWSEnabled bool, eipAllocationsAWSEnabled bool) error { - want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled) + proxyNeeded, err := IsProxyProtocolNeeded(ic, platform, current) + if err != nil { + return fmt.Errorf("failed to determine if proxy protocol is needed for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) + } + want, desired, err := desiredLoadBalancerService(ic, deploymentRef, platform, subnetsAWSEnabled, eipAllocationsAWSEnabled, proxyNeeded) if err != nil { return err } @@ -1290,7 +1289,7 @@ func getAWSLoadBalancerTypeInStatus(ic *operatorv1.IngressController) operatorv1 ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil { return ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type } - return "" + return operatorv1.AWSClassicLoadBalancer } // getAWSClassicLoadBalancerParametersInSpec gets the ClassicLoadBalancerParameter struct diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index d65d3bd3d..0abd8f49e 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/pkg/operator/controller/ingress/load_balancer_service_test.go @@ -769,7 +769,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { }, } - proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus) + proxyNeeded, err := IsProxyProtocolNeeded(ic, infraConfig.Status.PlatformStatus, nil) switch { case err != nil: t.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %v", ic.Namespace, ic.Name, err) @@ -777,7 +777,7 @@ func Test_desiredLoadBalancerService(t *testing.T) { t.Errorf("expected IsProxyProtocolNeeded to return %v, got %v", tc.proxyNeeded, proxyNeeded) } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, tc.subnetsAWSFeatureEnabled, tc.eipAllocationsAWSFeatureEnabled, proxyNeeded) switch { case err != nil: t.Error(err) @@ -962,7 +962,7 @@ func TestDesiredLoadBalancerServiceAWSIdleTimeout(t *testing.T) { }, }, } - haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) + haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true, false) if err != nil { t.Fatal(err) } @@ -1588,7 +1588,7 @@ func TestUpdateLoadBalancerServiceSourceRanges(t *testing.T) { }, }, } - wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true) + wantSvc, desired, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig.Status.PlatformStatus, true, true, false) if err != nil { t.Fatal(err) } diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index c22c3e80b..88f1d3196 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -3207,7 +3207,7 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) { }, }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true, true) if err != nil { t.Errorf("unexpected error from desiredLoadBalancerService: %v", err) return @@ -3317,7 +3317,7 @@ func Test_computeIngressEvaluationConditionsDetectedCondition(t *testing.T) { }, } - wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true) + wantSvc, service, err := desiredLoadBalancerService(ic, deploymentRef, platformStatus, true, true, true) if err != nil { t.Fatalf("unexpected error from desiredLoadBalancerService: %v", err) } From b27545bc94870c4cbd664b0d29579af0b4c46c12 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Mon, 21 Oct 2024 16:36:59 -0400 Subject: [PATCH 5/5] [WILL SQUASH] Move inactive LB parameters clearing to status.go 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. --- pkg/operator/controller/ingress/controller.go | 2 - .../controller/ingress/controller_test.go | 14 --- pkg/operator/controller/ingress/status.go | 48 +++++++- .../controller/ingress/status_test.go | 113 ++++++++++++++++++ 4 files changed, 156 insertions(+), 21 deletions(-) diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 229926fb4..71f4ac4bd 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -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{} } @@ -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{} } diff --git a/pkg/operator/controller/ingress/controller_test.go b/pkg/operator/controller/ingress/controller_test.go index 84374d962..30f460843 100644 --- a/pkg/operator/controller/ingress/controller_test.go +++ b/pkg/operator/controller/ingress/controller_test.go @@ -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())), diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index a258c8192..3d99c1f13 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -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) } @@ -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) { @@ -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) { diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 88f1d3196..b82a5e102 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -6,6 +6,7 @@ import ( "crypto/x509" "crypto/x509/pkix" "encoding/pem" + "github.com/stretchr/testify/assert" "math/big" "reflect" "strings" @@ -2324,6 +2325,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 { @@ -3415,3 +3454,77 @@ func Test_computeAllowedSourceRanges(t *testing.T) { }) } } + +func Test_clearIngressControllerInactiveAWSLBTypeParametersStatus(t *testing.T) { + awsPlatformStatus := &configv1.PlatformStatus{ + Type: configv1.AWSPlatformType, + } + lbServiceWithNLB := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + AWSLBTypeAnnotation: AWSNLBAnnotation, + }, + }, + } + lbServiceWithCLB := &corev1.Service{} + icStatusWithAWSLBParams := func(lbType operatorv1.AWSLoadBalancerType, nlbParams, clbParams bool) operatorv1.IngressControllerStatus { + icStatus := operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + ProviderParameters: &operatorv1.ProviderLoadBalancerParameters{ + AWS: &operatorv1.AWSLoadBalancerParameters{ + Type: lbType, + }, + }, + }, + }, + } + if nlbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.NetworkLoadBalancerParameters = &operatorv1.AWSNetworkLoadBalancerParameters{} + } + if clbParams { + icStatus.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} + } + return icStatus + } + testCases := []struct { + name string + service *corev1.Service + icStatus operatorv1.IngressControllerStatus + expectICStatus operatorv1.IngressControllerStatus + }{ + { + name: "service is nil", + service: nil, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + { + name: "clear clb params when lb type change is not pending", + service: lbServiceWithNLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, false), + }, + { + name: "clear nlb params when lb type change is not pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSClassicLoadBalancer, false, true), + }, + { + name: "don't clear any params when lb type change is pending", + service: lbServiceWithCLB, + icStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + expectICStatus: icStatusWithAWSLBParams(operatorv1.AWSNetworkLoadBalancer, true, true), + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ic := &operatorv1.IngressController{ + Status: tc.icStatus, + } + clearIngressControllerInactiveAWSLBTypeParametersStatus(awsPlatformStatus, ic, tc.service) + assert.Equal(t, tc.expectICStatus, ic.Status) + }) + } +}