BUG 2054200: Fix removing custom created service in openshift-ingress with same name

pkg/operator/controller/ingress/load_balancer_service.go

@@ -261,10 +261,16 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 		return false, nil, err
 	}
 
+	// BZ2054200: Don't modify/delete services that are not directly owned by this controller.
+	ownLBS := isServiceOwnedByIngressController(currentLBService, ci)
+
 	switch {
 	case !wantLBS && !haveLBS:
 		return false, nil, nil
 	case !wantLBS && haveLBS:
+		if !ownLBS {
+			return false, nil, fmt.Errorf("a conflicting load balancer service exists that is not owned by the ingress controller: %s", controller.LoadBalancerServiceName(ci))
+		}
 		if err := r.deleteLoadBalancerService(currentLBService, &crclient.DeleteOptions{}); err != nil {
 			return true, currentLBService, err
 		}
@@ -275,6 +281,9 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 		}
 		return r.currentLoadBalancerService(ci)
 	case wantLBS && haveLBS:
+		if !ownLBS {
+			return false, nil, fmt.Errorf("a conflicting load balancer service exists that is not owned by the ingress controller: %s", controller.LoadBalancerServiceName(ci))
+		}
 		if updated, err := r.normalizeLoadBalancerServiceAnnotations(currentLBService); err != nil {
 			return true, currentLBService, fmt.Errorf("failed to normalize annotations for load balancer service: %w", err)
 		} else if updated {
@@ -296,6 +305,14 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 	return true, currentLBService, nil
 }
 
+// isServiceOwnedByIngressController determines whether a service is owned by an ingress controller.
+func isServiceOwnedByIngressController(service *corev1.Service, ic *operatorv1.IngressController) bool {
+	if service != nil && service.Labels[manifests.OwningIngressControllerLabel] == ic.Name {
+		return true
+	}
+	return false
+}
+
 // desiredLoadBalancerService returns the desired LB service for a
 // 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

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -8,6 +8,7 @@ import (
 
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -736,3 +737,66 @@ func TestLoadBalancerServiceChanged(t *testing.T) {
 		})
 	}
 }
+
+func TestServiceIngressOwner(t *testing.T) {
+	testCases := []struct {
+		description string
+		service     *corev1.Service
+		ingressName string
+		expect      bool
+	}{
+		{
+			description: "if owner is set correctly",
+			service: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						manifests.OwningIngressControllerLabel: "foo",
+					},
+				},
+			},
+			ingressName: "foo",
+			expect:      true,
+		},
+		{
+			description: "if owner is not set correctly",
+			service: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						manifests.OwningIngressControllerLabel: "foo",
+					},
+				},
+			},
+			ingressName: "bar",
+			expect:      false,
+		},
+		{
+			description: "if owner label is not set at all",
+			service: &corev1.Service{
+				ObjectMeta: metav1.ObjectMeta{},
+			},
+			ingressName: "bar",
+			expect:      false,
+		},
+		{
+			description: "if service is nil",
+			service:     nil,
+			ingressName: "bar",
+			expect:      false,
+		},
+	}
+
+	for _, tc := range testCases {
+		ic := &operatorv1.IngressController{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: tc.ingressName,
+			},
+			Spec:   operatorv1.IngressControllerSpec{},
+			Status: operatorv1.IngressControllerStatus{},
+		}
+
+		if actual := isServiceOwnedByIngressController(tc.service, ic); actual != tc.expect {
+			t.Errorf("expected ownership %t got %t", tc.expect, actual)
+		}
+	}
+
+}

pkg/operator/controller/ingress/nodeport_service.go

@@ -52,10 +52,16 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 		return false, nil, err
 	}
 
+	// BZ2054200: Don't modify/delete services that are not directly owned by this controller.
+	ownLBS := isServiceOwnedByIngressController(current, ic)
+
 	switch {
 	case !wantService && !haveService:
 		return false, nil, nil
 	case !wantService && haveService:
+		if !ownLBS {
+			return false, nil, fmt.Errorf("a conflicting nodeport service exists that is not owned by the ingress controller: %s", controller.LoadBalancerServiceName(ic))
+		}
 		if err := r.client.Delete(context.TODO(), current); err != nil {
 			if !errors.IsNotFound(err) {
 				return true, current, fmt.Errorf("failed to delete NodePort service: %v", err)
@@ -71,6 +77,9 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep
 		log.Info("created NodePort service", "service", desired)
 		return r.currentNodePortService(ic)
 	case wantService && haveService:
+		if !ownLBS {
+			return false, nil, fmt.Errorf("a conflicting nodeport service exists that is not owned by the ingress controller: %s", controller.LoadBalancerServiceName(ic))
+		}
 		if updated, err := r.updateNodePortService(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update NodePort service: %v", err)
 		} else if updated {

test/e2e/operator_test.go

@@ -2723,6 +2723,89 @@ func TestTunableRouterKubeletProbesForCustomIngressController(t *testing.T) {
 	}
 }
 
+// TestIngressControllerServiceNameCollision validates BZ2054200: Don't delete services that are not directly owned by this controller.
+// It creates a service with the same naming convention as the ingress controller creates its own load balancing services.
+// Then it triggers a reconcilation of the ingress operator to see if it will delete our service.
+func TestIngressControllerServiceNameCollision(t *testing.T) {
+	// Create the new private controller that we will later create a service to collide with the naming scheme of this.
+	icName := types.NamespacedName{
+		Namespace: operatorNamespace,
+		Name:      "e2e-name-collision-test",
+	}
+	domain := icName.Name + "." + dnsConfig.Spec.BaseDomain
+	ic := newPrivateController(icName, domain)
+	if err := kclient.Create(context.TODO(), ic); err != nil {
+		t.Fatalf("failed to create ingresscontroller %s: %v", icName, err)
+	}
+
+	// Clean up our new Ingress Controller after we are done.
+	defer assertIngressControllerDeleted(t, kclient, ic)
+
+	// Wait for new ingress controller to come online.
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, icName, availableConditionsForPrivateIngressController...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
+	// Create services that could collide with the new ingress controller's naming convention for loadbalancer and nodeport.
+	// TRICKY: Our private ingress controller will reconcile and delete extra loadBalancerServices, even though the
+	//         ingress controller isn't a loadbalancer type itself.
+	conflictingLoadBalancerServiceName := types.NamespacedName{
+		Name:      "router-" + icName.Name,
+		Namespace: "openshift-ingress",
+	}
+	conflictingLoadBalancerService := buildEchoService(conflictingLoadBalancerServiceName.Name, conflictingLoadBalancerServiceName.Namespace, nil)
+	if err := kclient.Create(context.TODO(), conflictingLoadBalancerService); err != nil {
+		t.Fatalf("failed to create service %s: %v", conflictingLoadBalancerServiceName, err)
+	}
+	defer func() {
+		if err := kclient.Delete(context.TODO(), conflictingLoadBalancerService); err != nil {
+			t.Fatalf("failed to delete service %s: %v", conflictingLoadBalancerServiceName, err)
+		}
+	}()
+
+	conflictingNodeportServiceName := types.NamespacedName{
+		Name:      "router-nodeport-" + icName.Name,
+		Namespace: "openshift-ingress",
+	}
+	conflictingNodeportService := buildEchoService(conflictingNodeportServiceName.Name, conflictingNodeportServiceName.Namespace, nil)
+	if err := kclient.Create(context.TODO(), conflictingNodeportService); err != nil {
+		t.Fatalf("failed to create service %s: %v", conflictingNodeportServiceName, err)
+	}
+	defer func() {
+		if err := kclient.Delete(context.TODO(), conflictingNodeportService); err != nil {
+			t.Fatalf("failed to delete service %s: %v", conflictingNodeportServiceName, err)
+		}
+	}()
+
+	ic, err := getIngressController(t, kclient, icName, 1*time.Minute)
+	if err != nil {
+		t.Fatalf("failed to get ingress controller: %v", err)
+	}
+
+	// Annotate the ingress operator, to trigger a reconcilation to determine if our service is deleted.
+	// This may not be needed, but it ensures a reconcilation occurs ASAP.
+	if ic.Annotations == nil {
+		ic.Annotations = map[string]string{}
+	}
+	ic.Annotations["ingress.operator.openshift.io/e2e-name-collision-test"] = ""
+
+	if err := kclient.Update(context.TODO(), ic); err != nil {
+		t.Fatal(err)
+	}
+
+	// Wait to see if our service gets deleted by the operator due to name collision.
+	oldLoadBalancerUID := conflictingLoadBalancerService.UID
+	oldNodePortUID := conflictingNodeportService.UID
+	wait.PollImmediate(1*time.Second, 1*time.Minute, func() (bool, error) {
+		// Check if LoadBalancer and Nodeport Service don't get deleted for the entire duration of this loop.
+		// Will throw fatal error if deleted or marked for deletion and this loop stops.
+		assertServiceNotDeleted(t, conflictingLoadBalancerServiceName, oldLoadBalancerUID)
+		assertServiceNotDeleted(t, conflictingNodeportServiceName, oldNodePortUID)
+
+		return false, nil
+	})
+}
+
 func newLoadBalancerController(name types.NamespacedName, domain string) *operatorv1.IngressController {
 	repl := int32(1)
 	return &operatorv1.IngressController{
@@ -2987,6 +3070,25 @@ func deleteIngressController(t *testing.T, cl client.Client, ic *operatorv1.Ingr
 	return nil
 }
 
+// assertServiceNotDeleted asserts that a provide service wasn't deleted.
+func assertServiceNotDeleted(t *testing.T, serviceName types.NamespacedName, oldUid types.UID) {
+	t.Helper()
+
+	// First check our LoadBalancer Service.
+	service := &corev1.Service{}
+	if err := kclient.Get(context.TODO(), serviceName, service); err != nil {
+		t.Fatalf("expected %s to be present: %v", serviceName, err)
+	}
+	// If there is a DeletionTimestamp, it has been marked for deletion.
+	if service.DeletionTimestamp != nil {
+		t.Fatalf("expected service %s to not be marked for deletion: %v", serviceName, service.DeletionTimestamp)
+	}
+	// If the UID has changed, then the service has been recreated.
+	if service.UID != oldUid {
+		t.Fatalf("expected service %s to have UID %v, got %v", serviceName, oldUid, service.UID)
+	}
+}
+
 func createDefaultCertTestSecret(cl client.Client, name string) (*corev1.Secret, error) {
 	defaultCert := `-----BEGIN CERTIFICATE-----
 MIIDIjCCAgqgAwIBAgIBBjANBgkqhkiG9w0BAQUFADCBoTELMAkGA1UEBhMCVVMx