From dba837a8b5314a3baa8167554f951bbb6f458c1e Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Thu, 21 Apr 2022 12:36:15 -0700 Subject: [PATCH] BUG 2054200: Fix removing custom created service in openshift-ingress with same naming convention --- .../ingress/load_balancer_service.go | 17 +++ .../ingress/load_balancer_service_test.go | 64 +++++++++++ .../controller/ingress/nodeport_service.go | 9 ++ test/e2e/operator_test.go | 102 ++++++++++++++++++ 4 files changed, 192 insertions(+) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index c453d6652f..98182981db 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/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 diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 9caf0ccf34..c94fe9fa21 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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) + } + } + +} diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 755fd924ea..1897566f1c 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/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 { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 3359a0652b..e5e4542d8a 100644 --- a/test/e2e/operator_test.go +++ b/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