diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index 80adc1a4bb..84d3695846 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -430,6 +430,10 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig return true } + // Detect changes to LB scope. + if specLB != nil && statusLB != nil && specLB.Scope != statusLB.Scope { + ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope = effectiveStrategy.LoadBalancer.Scope + } return false } @@ -803,7 +807,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err)) } - errs = append(errs, r.syncIngressControllerStatus(ci, deployment, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig)) + errs = append(errs, r.syncIngressControllerStatus(ci, deployment, pods.Items, lbService, operandEvents.Items, wildcardRecord, dnsConfig, infraConfig)) return retryable.NewMaybeRetryableAggregate(errs) } diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 1aeb1da6b2..2c161c68d2 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -124,10 +124,17 @@ const ( // alibabaCloudLBAddressTypeIntranet is the service annotation value used to specify an Aliyun SLB // IP is exposed to the intranet (private) alibabaCloudLBAddressTypeIntranet = "intranet" + + // autoDeleteLoadBalancerAnnotation is an annotation that can be set on + // an IngressController to indicate that the operator should + // automatically delete any associated service load-balancer when its + // scope changes if changing scope requires deleting service + // load-balancers on the current platform. + autoDeleteLoadBalancerAnnotation = "ingress.operator.openshift.io/auto-delete-load-balancer" ) var ( - // internalLBAnnotations maps platform to the annotation name and value + // InternalLBAnnotations maps platform to the annotation name and value // that tell the cloud provider that is associated with the platform // that the load balancer is internal. // @@ -169,6 +176,71 @@ var ( alibabaCloudLBAddressTypeAnnotation: alibabaCloudLBAddressTypeIntranet, }, } + + // externalLBAnnotations maps platform to the annotation name and value + // that tell the cloud provider that is associated with the platform + // that the load balancer is external. This is the default for most + // platforms; only platforms for which it is not the default need + // entries in this map. + externalLBAnnotations = map[configv1.PlatformType]map[string]string{ + configv1.IBMCloudPlatformType: { + iksLBScopeAnnotation: iksLBScopePublic, + }, + configv1.PowerVSPlatformType: { + iksLBScopeAnnotation: iksLBScopePublic, + }, + } + + // platformsWithMutableScope is the set of platforms that support + // mutating load-balancer scope without deleting and recreating a + // service load-balancer. + platformsWithMutableScope = map[configv1.PlatformType]struct{}{ + configv1.AzurePlatformType: {}, + 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. + // + // Be careful when adding annotation keys to this set. If a new release + // of the operator starts managing an annotation that it previously + // ignored, it could stomp annotations that the user has set when the + // user upgrades the operator to the new release (see + // ). 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( + // AWS LB health check interval annotation (see + // ). + awsLBHealthCheckIntervalAnnotation, + // GCP Global Access internal Load Balancer annotation + // (see ). + GCPGlobalAccessAnnotation, + // local-with-fallback annotation for kube-proxy (see + // ). + localWithFallbackAnnotation, + ) + + // Azure and GCP support switching between internal and external + // scope by changing the annotation, so the operator manages the + // corresponding load-balancer scope annotations for these + // platforms. Other platforms require deleting and recreating + // the service, so the operator doesn't update the annotations + // that specify load-balancer scope for those platforms. See + // . + for platform := range platformsWithMutableScope { + for name := range InternalLBAnnotations[platform] { + result.Insert(name) + } + for name := range externalLBAnnotations[platform] { + result.Insert(name) + } + } + + return result + }() ) // ensureLoadBalancerService creates an LB service if one is desired but absent. @@ -223,7 +295,11 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, return haveLBS, currentLBService, err } } - if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform); err != nil { + deleteIfScopeChanged := false + if _, ok := ci.Annotations[autoDeleteLoadBalancerAnnotation]; ok { + deleteIfScopeChanged = true + } + if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform, deleteIfScopeChanged); err != nil { return true, currentLBService, fmt.Errorf("failed to update load balancer service: %v", err) } else if updated { return r.currentLoadBalancerService(ci) @@ -281,6 +357,11 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations[GCPGlobalAccessAnnotation] = strconv.FormatBool(globalAccessEnabled) } } + } else { + annotation := externalLBAnnotations[platform.Type] + for name, value := range annotation { + service.Annotations[name] = value + } } switch platform.Type { case configv1.AWSPlatformType: @@ -313,9 +394,6 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef service.Annotations[awsLBHealthCheckUnhealthyThresholdAnnotation] = awsLBHealthCheckUnhealthyThresholdDefault service.Annotations[awsLBHealthCheckHealthyThresholdAnnotation] = awsLBHealthCheckHealthyThresholdDefault case configv1.IBMCloudPlatformType, configv1.PowerVSPlatformType: - if !isInternal { - service.Annotations[iksLBScopeAnnotation] = iksLBScopePublic - } // Set ExternalTrafficPolicy to type Cluster - IBM's LoadBalancer impl is created within the cluster. // LB places VIP on one of the worker nodes, using keepalived to maintain the VIP and ensuring redundancy // LB relies on iptable rules kube-proxy puts in to send traffic from the VIP node to the cluster @@ -448,7 +526,21 @@ 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) (bool, error) { +func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, platform *configv1.PlatformStatus, deleteIfScopeChanged bool) (bool, error) { + _, platformHasMutableScope := platformsWithMutableScope[platform.Type] + if !platformHasMutableScope && deleteIfScopeChanged && !scopeEqual(current, desired, platform) { + log.Info("deleting and recreating the load balancer because its scope changed", "namespace", desired.Namespace, "name", desired.Name) + foreground := metav1.DeletePropagationForeground + deleteOptions := crclient.DeleteOptions{PropagationPolicy: &foreground} + if err := r.deleteLoadBalancerService(current, &deleteOptions); err != nil { + return false, err + } + if err := r.createLoadBalancerService(desired); err != nil { + return false, err + } + return true, nil + } + changed, updated := loadBalancerServiceChanged(current, desired) if !changed { return false, nil @@ -462,17 +554,41 @@ func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, return true, nil } -// managedLoadBalancerServiceAnnotations is a set of annotation keys for -// annotations that the operator manages for LoadBalancer-type services. -var managedLoadBalancerServiceAnnotations = sets.NewString( - awsLBHealthCheckIntervalAnnotation, - GCPGlobalAccessAnnotation, - localWithFallbackAnnotation, -) +// 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 +} // 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) { + // Preserve most fields and annotations. If a new release of the + // operator starts managing an annotation or spec field that it + // previously ignored, it could stomp user changes when the user + // upgrades the operator to the new release (see + // ). In order to + // avoid problems, make sure the previous release blocks upgrades when + // the user has modified an annotation or spec field that the new + // release manages. annotationCmpOpts := []cmp.Option{ cmpopts.IgnoreMapEntries(func(k, _ string) bool { return !managedLoadBalancerServiceAnnotations.Has(k) @@ -484,13 +600,6 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev updated := current.DeepCopy() - // Preserve everything but the AWS LB health check interval annotation, - // GCP Global Access internal Load Balancer annotation, and - // local-with-fallback annotation - // (see ). - // Updating annotations and spec fields cannot be done unless the - // previous release blocks upgrades when the user has modified those - // fields (see ). if updated.Annotations == nil { updated.Annotations = map[string]string{} } @@ -507,3 +616,18 @@ func loadBalancerServiceChanged(current, expected *corev1.Service) (bool, *corev return true, updated } + +// IsServiceInternal returns a Boolean indicating whether the provided service +// is annotated to request an internal load balancer. +func IsServiceInternal(service *corev1.Service) bool { + for dk, dv := range service.Annotations { + for _, annotations := range InternalLBAnnotations { + for ik, iv := range annotations { + if dk == ik && dv == iv { + return true + } + } + } + } + return false +} diff --git a/pkg/operator/controller/ingress/status.go b/pkg/operator/controller/ingress/status.go index b54f42dc33..658a4c68ef 100644 --- a/pkg/operator/controller/ingress/status.go +++ b/pkg/operator/controller/ingress/status.go @@ -44,12 +44,17 @@ type expectedCondition struct { // syncIngressControllerStatus computes the current status of ic and // updates status upon any changes since last sync. -func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS) error { +func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressController, deployment *appsv1.Deployment, pods []corev1.Pod, service *corev1.Service, operandEvents []corev1.Event, wildcardRecord *iov1.DNSRecord, dnsConfig *configv1.DNS, infraConfig *configv1.Infrastructure) error { selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) if err != nil { return fmt.Errorf("deployment has invalid spec.selector: %v", err) } + platform, err := oputil.GetPlatformStatus(r.client, infraConfig) + if err != nil { + return fmt.Errorf("failed to determine infrastructure platform status for ingresscontroller %s/%s: %w", ic.Namespace, ic.Name, err) + } + var errs []error updated := ic.DeepCopy() @@ -65,6 +70,7 @@ func (r *reconciler) syncIngressControllerStatus(ic *operatorv1.IngressControlle updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressAvailableCondition(updated.Status.Conditions)) degradedCondition, err := computeIngressDegradedCondition(updated.Status.Conditions, updated.Name) errs = append(errs, err) + updated.Status.Conditions = MergeConditions(updated.Status.Conditions, computeIngressProgressingCondition(updated.Status.Conditions, ic, service, platform)) updated.Status.Conditions = MergeConditions(updated.Status.Conditions, degradedCondition) updated.Status.Conditions = PruneConditions(updated.Status.Conditions) @@ -540,6 +546,45 @@ func formatConditions(conditions []*operatorv1.OperatorCondition) string { return formatted } +// computeIngressProgressingCondition computes the ingresscontroller's +// "Progressing" status condition, which indicates if the ingresscontroller's +// current state matches its desired state. +func computeIngressProgressingCondition(conditions []operatorv1.OperatorCondition, ic *operatorv1.IngressController, service *corev1.Service, platform *configv1.PlatformStatus) operatorv1.OperatorCondition { + condition := operatorv1.OperatorCondition{ + Type: operatorv1.OperatorStatusTypeProgressing, + Status: operatorv1.ConditionFalse, + } + if ic.Status.EndpointPublishingStrategy.Type == operatorv1.LoadBalancerServiceStrategyType { + switch { + case ic.Status.EndpointPublishingStrategy.LoadBalancer == nil: + condition.Message = "status.endpointPublishingStrategy.loadBalancer is not set." + condition.Reason = "IncompleteStatus" + condition.Status = operatorv1.ConditionUnknown + case service == nil: + condition.Message = "LoadBalancer Service not created." + condition.Reason = "NoService" + condition.Status = operatorv1.ConditionTrue + default: + wantScope := ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope + haveScope := operatorv1.ExternalLoadBalancer + if IsServiceInternal(service) { + haveScope = operatorv1.InternalLoadBalancer + } + if wantScope != haveScope { + message := fmt.Sprintf("The IngressController scope was changed from %q to %q.", haveScope, wantScope) + switch platform.Type { + case configv1.AWSPlatformType, configv1.IBMCloudPlatformType: + message = 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 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\"}}}}'", message, service.Namespace, service.Name, ic.Name, haveScope) + } + condition.Reason = "ScopeChanged" + condition.Message = message + condition.Status = operatorv1.ConditionTrue + } + } + } + return condition +} + // IngressStatusesEqual compares two IngressControllerStatus values. Returns true // if the provided values should be considered equal for the purpose of determining // whether an update is necessary, false otherwise. diff --git a/pkg/operator/controller/ingress/status_test.go b/pkg/operator/controller/ingress/status_test.go index 4ad75817ef..f39bd00b0c 100644 --- a/pkg/operator/controller/ingress/status_test.go +++ b/pkg/operator/controller/ingress/status_test.go @@ -1,6 +1,7 @@ package ingress import ( + "strings" "testing" "time" @@ -490,6 +491,151 @@ func TestComputeIngressDegradedCondition(t *testing.T) { } } +// TestComputeIngressProgressCondition verifies that +// computeIngressProgressingCondition returns the expected status condition. +func TestComputeIngressProgressCondition(t *testing.T) { + hostNetworkIngressController := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.HostNetworkStrategyType, + }, + }, + } + loadBalancerIngressController := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + }, + }, + } + loadBalancerIngressControllerWithInternalScope := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.InternalLoadBalancer, + }, + }, + }, + } + loadBalancerIngressControllerWithExternalScope := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.LoadBalancerServiceStrategyType, + LoadBalancer: &operatorv1.LoadBalancerStrategy{ + Scope: operatorv1.ExternalLoadBalancer, + }, + }, + }, + } + nodePortIngressController := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.NodePortServiceStrategyType, + }, + }, + } + privateIngressController := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: operatorv1.PrivateStrategyType, + }, + }, + } + lbService := &corev1.Service{} + lbServiceWithInternalScopeOnAWS := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + awsInternalLBAnnotation: "true", + }, + }, + } + awsPlatformStatus := &configv1.PlatformStatus{ + Type: configv1.AWSPlatformType, + } + azurePlatformStatus := &configv1.PlatformStatus{ + Type: configv1.AzurePlatformType, + } + tests := []struct { + name string + conditions []operatorv1.OperatorCondition + ic *operatorv1.IngressController + service *corev1.Service + platformStatus *configv1.PlatformStatus + expectStatus operatorv1.ConditionStatus + expectMessageContains string + expectMessageDoesNotContain string + }{ + { + name: "Private", + ic: &privateIngressController, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "NodePortService", + ic: &nodePortIngressController, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "HostNetwork", + ic: &hostNetworkIngressController, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "LoadBalancerService, no service", + ic: &loadBalancerIngressControllerWithExternalScope, + expectStatus: operatorv1.ConditionTrue, + }, + { + name: "LoadBalancerService, no status", + ic: &loadBalancerIngressController, + expectStatus: operatorv1.ConditionUnknown, + }, + { + name: "LoadBalancerService, inconsistent scope on AWS", + ic: &loadBalancerIngressControllerWithInternalScope, + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionTrue, + expectMessageContains: "delete", + }, + { + name: "LoadBalancerService, inconsistent scope on Azure", + ic: &loadBalancerIngressControllerWithInternalScope, + service: lbService, + platformStatus: azurePlatformStatus, + expectStatus: operatorv1.ConditionTrue, + expectMessageDoesNotContain: "delete", + }, + { + name: "LoadBalancerService, internal scope", + ic: &loadBalancerIngressControllerWithInternalScope, + service: lbServiceWithInternalScopeOnAWS, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + { + name: "LoadBalancerService, external scope", + ic: &loadBalancerIngressControllerWithExternalScope, + service: lbService, + platformStatus: awsPlatformStatus, + expectStatus: operatorv1.ConditionFalse, + }, + } + for _, test := range tests { + actual := computeIngressProgressingCondition(test.conditions, test.ic, test.service, test.platformStatus) + if actual.Status != test.expectStatus { + t.Errorf("%q: expected status to be %s, got %s", test.name, test.expectStatus, actual.Status) + } + if len(test.expectMessageContains) != 0 && !strings.Contains(actual.Message, test.expectMessageContains) { + t.Errorf("%q: expected message to include %q, got %q", test.name, test.expectMessageContains, actual.Message) + } + if len(test.expectMessageDoesNotContain) != 0 && strings.Contains(actual.Message, test.expectMessageDoesNotContain) { + t.Errorf("%q: expected message not to include %q, got %q", test.name, test.expectMessageDoesNotContain, actual.Message) + } + } +} + func TestComputeDeploymentAvailableCondition(t *testing.T) { tests := []struct { name string diff --git a/pkg/operator/controller/status/controller.go b/pkg/operator/controller/status/controller.go index aebe830411..e958887cd2 100644 --- a/pkg/operator/controller/status/controller.go +++ b/pkg/operator/controller/status/controller.go @@ -151,6 +151,7 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( co.Status.Conditions = mergeConditions(co.Status.Conditions, computeOperatorAvailableCondition(state.IngressControllers), computeOperatorProgressingCondition( + state.IngressControllers, allIngressesAvailable, oldStatus.Versions, co.Status.Versions, @@ -333,10 +334,7 @@ func computeOperatorDegradedCondition(ingresses []operatorv1.IngressController) } // computeOperatorProgressingCondition computes the operator's current Progressing status state. -func computeOperatorProgressingCondition(allIngressesAvailable bool, oldVersions, curVersions []configv1.OperandVersion, operatorReleaseVersion, ingressControllerImage string, canaryImage string) configv1.ClusterOperatorStatusCondition { - // TODO: Update progressingCondition when an ingresscontroller - // progressing condition is created. The Operator's condition - // should be derived from the ingresscontroller's condition. +func computeOperatorProgressingCondition(ingresscontrollers []operatorv1.IngressController, allIngressesAvailable bool, oldVersions, curVersions []configv1.OperandVersion, operatorReleaseVersion, ingressControllerImage string, canaryImage string) configv1.ClusterOperatorStatusCondition { progressingCondition := configv1.ClusterOperatorStatusCondition{ Type: configv1.OperatorProgressing, } @@ -344,6 +342,17 @@ func computeOperatorProgressingCondition(allIngressesAvailable bool, oldVersions progressing := false var messages []string + + for _, ic := range ingresscontrollers { + for _, c := range ic.Status.Conditions { + if c.Type == operatorv1.OperatorStatusTypeProgressing && c.Status == operatorv1.ConditionTrue { + msg := fmt.Sprintf("ingresscontroller %q is progressing: %s: %s.", ic.Name, c.Reason, c.Message) + messages = append(messages, msg) + progressing = true + } + } + } + if !allIngressesAvailable { messages = append(messages, "Not all ingress controllers are available.") progressing = true diff --git a/pkg/operator/controller/status/controller_test.go b/pkg/operator/controller/status/controller_test.go index 857f0c7dd3..aa31f32f41 100644 --- a/pkg/operator/controller/status/controller_test.go +++ b/pkg/operator/controller/status/controller_test.go @@ -6,6 +6,8 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + operatorv1 "github.com/openshift/api/operator/v1" + configv1 "github.com/openshift/api/config/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -16,19 +18,26 @@ func TestComputeOperatorProgressingCondition(t *testing.T) { } testCases := []struct { - description string - noNamespace bool - allIngressesAvailable bool - reportedVersions versions - oldVersions versions - curVersions versions - expectProgressing configv1.ConditionStatus + description string + noNamespace bool + allIngressesAvailable bool + someIngressProgressing bool + reportedVersions versions + oldVersions versions + curVersions versions + expectProgressing configv1.ConditionStatus }{ { description: "all ingress controllers are available", allIngressesAvailable: true, expectProgressing: configv1.ConditionFalse, }, + { + description: "some ingress controller is progressing", + allIngressesAvailable: true, + someIngressProgressing: true, + expectProgressing: configv1.ConditionTrue, + }, { description: "all ingress controllers are not available", expectProgressing: configv1.ConditionTrue, @@ -134,7 +143,21 @@ func TestComputeOperatorProgressingCondition(t *testing.T) { Status: tc.expectProgressing, } - actual := computeOperatorProgressingCondition(tc.allIngressesAvailable, oldVersions, reportedVersions, tc.curVersions.operator, tc.curVersions.operand1, tc.curVersions.operand2) + var ingresscontrollers []operatorv1.IngressController + ic := operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + Conditions: []operatorv1.OperatorCondition{{ + Type: operatorv1.OperatorStatusTypeProgressing, + Status: operatorv1.ConditionFalse, + }}, + }, + } + ingresscontrollers = append(ingresscontrollers, ic) + if tc.someIngressProgressing { + ingresscontrollers[0].Status.Conditions[0].Status = operatorv1.ConditionTrue + } + + actual := computeOperatorProgressingCondition(ingresscontrollers, tc.allIngressesAvailable, oldVersions, reportedVersions, tc.curVersions.operator, tc.curVersions.operand1, tc.curVersions.operand2) conditionsCmpOpts := []cmp.Option{ cmpopts.IgnoreFields(configv1.ClusterOperatorStatusCondition{}, "LastTransitionTime", "Reason", "Message"), } diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 5b55212fc5..2d08fede1f 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -884,6 +884,179 @@ func TestInternalLoadBalancerGlobalAccessGCP(t *testing.T) { } } +// TestScopeChange creates an ingresscontroller with the "LoadBalancerService" +// endpoint publishing strategy type and verifies that the operator behaves +// correctly when the ingresscontroller's scope is mutated. The correct +// behavior depends on the platform on which the test is running. On AWS and +// IBM Cloud, the operator should set the Progressing=True status condition to +// prompt the administrator to delete the old LoadBalancer service. On Azure +// and GCP, the operator should update the LoadBalancer service's annotations. +// +// As a special case, if the ingresscontroller's scope has been changed, the +// "ingress.operator.openshift.io/auto-delete-load-balancer" annotation is set +// on the ingresscontroller, and the current platform requires deleting and +// recreating the LoadBalancer service to change its scope, then the operator +// should delete and recreate the service automatically. +func TestScopeChange(t *testing.T) { + platform := infraConfig.Status.Platform + supportedPlatforms := map[configv1.PlatformType]struct{}{ + configv1.AlibabaCloudPlatformType: {}, + configv1.AWSPlatformType: {}, + configv1.AzurePlatformType: {}, + configv1.GCPPlatformType: {}, + configv1.IBMCloudPlatformType: {}, + configv1.PowerVSPlatformType: {}, + } + if _, supported := supportedPlatforms[platform]; !supported { + t.Skipf("test skipped on platform %q", platform) + } + + name := types.NamespacedName{Namespace: operatorNamespace, Name: "scope"} + 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.Fatalf("failed to create ingresscontroller: %v", err) + } + defer assertIngressControllerDeleted(t, kclient, ic) + + // Wait for the load balancer and DNS to be ready. + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { + 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 ingresscontroller.IsServiceInternal(lbService) { + t.Fatalf("load balancer is internal but should be external") + } + + // Change the scope to internal and wait for everything to come back to + // normal. + if err := kclient.Get(context.TODO(), name, ic); err != nil { + t.Fatalf("failed to get ingresscontroller %s: %v", name, err) + } + ic.Spec.EndpointPublishingStrategy.LoadBalancer.Scope = operatorv1.InternalLoadBalancer + + if err := kclient.Update(context.TODO(), ic); err != nil { + t.Fatal(err) + } + + switch platform { + case configv1.AlibabaCloudPlatformType, configv1.AWSPlatformType, configv1.IBMCloudPlatformType, configv1.PowerVSPlatformType: + progressingTrue := operatorv1.OperatorCondition{ + Type: operatorv1.OperatorStatusTypeProgressing, + Status: operatorv1.ConditionTrue, + } + if err := waitForIngressControllerCondition(t, kclient, 1*time.Minute, name, progressingTrue); err != nil { + t.Fatalf("failed to observe the ingresscontroller report Progressing=True: %v", err) + } + case configv1.AzurePlatformType, configv1.GCPPlatformType: + 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 { + if errors.IsNotFound(err) { + return false, nil + } + t.Logf("failed to get service %s: %v", controller.LoadBalancerServiceName(ic), err) + return false, nil + } + if ingresscontroller.IsServiceInternal(service) { + return true, nil + } + t.Logf("service is still external: %#v\n", service) + return false, nil + }) + if err != nil { + t.Fatalf("expected load balancer to become internal: %v", err) + } + } + + // Change the scope back to external and wait for everything to come + // back to normal. + if err := kclient.Get(context.TODO(), name, ic); err != nil { + t.Fatalf("failed to get ingresscontroller %s: %v", name, err) + } + ic.Spec.EndpointPublishingStrategy.LoadBalancer.Scope = operatorv1.ExternalLoadBalancer + + if err := kclient.Update(context.TODO(), ic); err != nil { + t.Fatal(err) + } + + switch platform { + case configv1.AWSPlatformType, configv1.IBMCloudPlatformType: + progressingFalse := operatorv1.OperatorCondition{ + Type: operatorv1.OperatorStatusTypeProgressing, + Status: operatorv1.ConditionFalse, + } + if err := waitForIngressControllerCondition(t, kclient, 1*time.Minute, name, progressingFalse); err != nil { + t.Fatalf("failed to observe the ingresscontroller report Progressing=True: %v", err) + } + case configv1.AzurePlatformType, configv1.GCPPlatformType: + 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 { + if errors.IsNotFound(err) { + return false, nil + } + t.Logf("failed to get ingresscontroller %s: %v", name.Name, err) + return false, nil + } + if ingresscontroller.IsServiceInternal(service) { + t.Logf("service is still internal: %#v\n", service) + return false, nil + } + return true, nil + }) + if err != nil { + t.Fatalf("expected load balancer to become external: %v", err) + } + } + + if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableConditionsForIngressControllerWithLoadBalancer...); err != nil { + t.Fatalf("failed to observe expected conditions: %v", err) + } + + // Annotate the ingresscontroller to tell the operator to automatically + // delete and recreate the LoadBalancer service if necessary when the + // scope changes, and change the scope. + if err := kclient.Get(context.TODO(), name, ic); err != nil { + t.Fatalf("failed to get ingresscontroller %s: %v", name, err) + } + if ic.Annotations == nil { + ic.Annotations = map[string]string{} + } + ic.Annotations["ingress.operator.openshift.io/auto-delete-load-balancer"] = "" + ic.Spec.EndpointPublishingStrategy.LoadBalancer.Scope = operatorv1.InternalLoadBalancer + + if err := kclient.Update(context.TODO(), ic); err != nil { + t.Fatal(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 { + if errors.IsNotFound(err) { + return false, nil + } + t.Logf("failed to get service %s: %v", controller.LoadBalancerServiceName(ic), err) + return false, nil + } + if ingresscontroller.IsServiceInternal(service) { + return true, nil + } + t.Logf("service is still external: %#v\n", service) + return false, nil + }) + if err != nil { + t.Fatalf("expected load balancer to become internal: %v", err) + } +} + // TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller // with the "NodePortService" endpoint publishing strategy type and verifies // that the operator creates a router, that the router becomes available, and