Support changing ingresscontroller load balancer scope

hack/run-local.sh

@@ -11,4 +11,4 @@ RELEASE_VERSION=$(oc get clusterversion/version -o json | jq -r '.status.desired
 echo "Image: ${IMAGE}"
 echo "Release version: ${RELEASE_VERSION}"
 
-IMAGE="${IMAGE}" RELEASE_VERSION="${RELEASE_VERSION}" ${DELVE:-} ./ingress-operator start $@
+IMAGE="${IMAGE}" RELEASE_VERSION="${RELEASE_VERSION}" ${DELVE:-} ./ingress-operator start --image $IMAGE --release-version $RELEASE_VERSION --namespace openshift-ingress-operator --shutdown-file "" $@

pkg/operator/controller/ingress/controller.go

@@ -333,6 +333,12 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig
 		ic.Status.EndpointPublishingStrategy = effectiveStrategy
 		return true
 	}
+	// Detect changes to LB scope, which is something we can safely roll out.
+	specLB := ic.Status.EndpointPublishingStrategy.LoadBalancer
+	statusLB := effectiveStrategy.LoadBalancer
+	if specLB != nil && statusLB != nil && specLB.Scope != statusLB.Scope {
+		ic.Status.EndpointPublishingStrategy.LoadBalancer.Scope = effectiveStrategy.LoadBalancer.Scope
+	}
 	return false
 }
 

pkg/operator/controller/ingress/load_balancer_service.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
+
 	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
 	"github.com/openshift/cluster-ingress-operator/pkg/util/slice"
@@ -15,6 +16,8 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
@@ -66,23 +69,58 @@ var (
 // 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, infraConfig *configv1.Infrastructure) (*corev1.Service, error) {
-	desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig)
+	desired, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig)
 	if err != nil {
 		return nil, err
 	}
-
-	currentLBService, err := r.currentLoadBalancerService(ci)
+	current, err := r.currentLoadBalancerService(ci)
 	if err != nil {
 		return nil, err
 	}
-	if desiredLBService != nil && currentLBService == nil {
-		if err := r.client.Create(context.TODO(), desiredLBService); err != nil {
-			return nil, fmt.Errorf("failed to create load balancer service %s/%s: %v", desiredLBService.Namespace, desiredLBService.Name, err)
+
+	switch {
+	case desired != nil && current == nil:
+		if err := r.createLoadBalancerService(desired); err != nil {
+			return nil, err
 		}
-		log.Info("created load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name)
-		return desiredLBService, nil
+	case desired != nil && current != nil:
+		// If switching from internal/external, delete and recreate the service.
+		if IsServiceInternal(desired) != IsServiceInternal(current) {
+			if _, err := r.finalizeLoadBalancerService(ci); err != nil {
+				return nil, err
+			}
+			foreground := metav1.DeletePropagationForeground
+			if err := r.client.Delete(context.TODO(), current, &crclient.DeleteOptions{PropagationPolicy: &foreground}); err != nil {
+				return nil, err
+			}
+			if err := r.createLoadBalancerService(desired); err != nil {
+				return nil, err
+			}
+		}
+	}
+
+	return r.currentLoadBalancerService(ci)
+}
+
+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
+}
+
+func (r *reconciler) createLoadBalancerService(service *corev1.Service) error {
+	if err := r.client.Create(context.TODO(), service); err != nil {
+		return fmt.Errorf("failed to create load balancer service %s/%s: %v", service.Namespace, service.Name, err)
 	}
-	return currentLBService, nil
+	log.Info("created load balancer service", "namespace", service.Namespace, "name", service.Name)
+	return nil
 }
 
 // desiredLoadBalancerService returns the desired LB service for a

test/e2e/operator_test.go

@@ -580,6 +580,109 @@ func TestInternalLoadBalancer(t *testing.T) {
 	}
 }
 
+func TestScopeChange(t *testing.T) {
+	platform := infraConfig.Status.Platform
+
+	supportedPlatforms := map[configv1.PlatformType]struct{}{
+		configv1.AWSPlatformType:      {},
+		configv1.AzurePlatformType:    {},
+		configv1.IBMCloudPlatformType: {},
+	}
+	if _, supported := supportedPlatforms[platform]; !supported {
+		t.Skip(fmt.Sprintf("test skipped on platform %q", platform))
+	}
+
+	name := types.NamespacedName{Namespace: operatorNamespace, Name: "test"}
+	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(kclient, 5*time.Minute, name, defaultAvailableConditions...); 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)
+	}
+
+	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
+			} else {
+				return false, err
+			}
+		}
+		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")
+	}
+
+	if err := waitForIngressControllerCondition(kclient, 5*time.Minute, name, defaultAvailableConditions...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %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)
+	}
+
+	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
+			} else {
+				return false, err
+			}
+		}
+		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(kclient, 5*time.Minute, name, defaultAvailableConditions...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+}
+
 // TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller
 // with the "NodePortService" endpoint publishing strategy type and verifies
 // that the operator creates a router and that the router becomes available.