Fix deletion of svclb DaemonSet when Service is deleted

87e1806 removed the OwnerReferences
field from the DaemonSet, which makes sense since the Service may now be
in a different namespace than the DaemonSet and cross-namespace owner
references are not supported.  Unfortunately, we were relying on
garbage collection to delete the DameonSet, so this started leaving
orphaned DaemonSets when Services were deleted.

We don't want to add an a Service OnRemove handler, since this will add
finalizers to all Services, not just LoadBalancers services, causing
conformance tests to fail. Instead, manage our own finalizers, and
restore the DaemonSet removal Event that was removed by the same commit.

Signed-off-by: Brad Davidson <[email protected]>

pkg/servicelb/controller.go

@@ -33,6 +33,7 @@ import (
 )
 
 var (
+	finalizerName          = "svccontroller." + version.Program + ".cattle.io/lbservice"
 	svcNameLabel           = "svccontroller." + version.Program + ".cattle.io/svcname"
 	svcNamespaceLabel      = "svccontroller." + version.Program + ".cattle.io/svcnamespace"
 	daemonsetNodeLabel     = "svccontroller." + version.Program + ".cattle.io/enablelb"
@@ -189,8 +190,8 @@ func (h *handler) onChangeNode(key string, node *core.Node) (*core.Node, error)
 // updateService ensures that the Service ingress IP address list is in sync
 // with the Nodes actually running pods for this service.
 func (h *handler) updateService(svc *core.Service) (runtime.Object, error) {
-	if !h.enabled || svc.Spec.Type != core.ServiceTypeLoadBalancer {
-		return svc, nil
+	if !h.enabled || svc.DeletionTimestamp != nil || svc.Spec.Type != core.ServiceTypeLoadBalancer {
+		return h.removeFinalizer(svc)
 	}
 
 	pods, err := h.podCache.List(h.klipperLBNamespace, labels.SelectorFromSet(map[string]string{
@@ -216,14 +217,19 @@ func (h *handler) updateService(svc *core.Service) (runtime.Object, error) {
 	}
 
 	svc = svc.DeepCopy()
+	svc, err = h.addFinalizer(svc)
+	if err != nil {
+		return svc, err
+	}
+
 	svc.Status.LoadBalancer.Ingress = nil
 	for _, ip := range expectedIPs {
 		svc.Status.LoadBalancer.Ingress = append(svc.Status.LoadBalancer.Ingress, core.LoadBalancerIngress{
 			IP: ip,
 		})
 	}
 
-	h.recorder.Eventf(svc, core.EventTypeNormal, "UpdatedIngressIP", "LoadBalancer Ingress IP addresses updated: %s", strings.Join(expectedIPs, ", "))
+	defer h.recorder.Eventf(svc, core.EventTypeNormal, "UpdatedIngressIP", "LoadBalancer Ingress IP addresses updated: %s", strings.Join(expectedIPs, ", "))
 	return h.services.Services(svc.Namespace).UpdateStatus(context.TODO(), svc, meta.UpdateOptions{})
 }
 
@@ -358,26 +364,40 @@ func (h *handler) deployPod(svc *core.Service) error {
 	if err := h.deleteOldDeployments(svc); err != nil {
 		return err
 	}
-	objs := objectset.NewObjectSet()
-	if !h.enabled || svc.Spec.Type != core.ServiceTypeLoadBalancer || svc.Spec.ClusterIP == "" || svc.Spec.ClusterIP == "None" {
-		return h.processor.WithOwner(svc).Apply(objs)
+
+	if !h.enabled || svc.DeletionTimestamp != nil || svc.Spec.Type != core.ServiceTypeLoadBalancer || svc.Spec.ClusterIP == "" || svc.Spec.ClusterIP == "None" {
+		return h.deletePod(svc)
 	}
 
 	ds, err := h.newDaemonSet(svc)
 	if err != nil {
 		return err
 	}
+
+	objs := objectset.NewObjectSet()
 	if ds != nil {
 		objs.Add(ds)
-		h.recorder.Eventf(svc, core.EventTypeNormal, "AppliedDaemonSet", "Applied LoadBalancer DaemonSet %s/%s", ds.Namespace, ds.Name)
+		defer h.recorder.Eventf(svc, core.EventTypeNormal, "AppliedDaemonSet", "Applied LoadBalancer DaemonSet %s/%s", ds.Namespace, ds.Name)
 	}
 	return h.processor.WithOwner(svc).Apply(objs)
 }
 
+func (h *handler) deletePod(svc *core.Service) error {
+	name := generateName(svc)
+	if err := h.daemonsets.DaemonSets(h.klipperLBNamespace).Delete(context.TODO(), name, meta.DeleteOptions{}); err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+	defer h.recorder.Eventf(svc, core.EventTypeNormal, "DeletedDaemonSet", "Deleted LoadBalancer DaemonSet %s/%s", h.klipperLBNamespace, name)
+	return nil
+}
+
 // newDaemonSet creates a DaemonSet to ensure that ServiceLB pods are run on
 // each eligible node.
 func (h *handler) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
-	name := fmt.Sprintf("svclb-%s-%s", svc.Name, svc.UID[:8])
+	name := generateName(svc)
 	oneInt := intstr.FromInt(1)
 
 	// If ipv6 is present, we must enable ipv6 forwarding in the manifest
@@ -394,6 +414,8 @@ func (h *handler) newDaemonSet(svc *core.Service) (*apps.DaemonSet, error) {
 			Namespace: h.klipperLBNamespace,
 			Labels: map[string]string{
 				nodeSelectorLabel: "false",
+				svcNameLabel:      svc.Name,
+				svcNamespaceLabel: svc.Namespace,
 			},
 		},
 		TypeMeta: meta.TypeMeta{
@@ -562,3 +584,39 @@ func (h *handler) deleteOldDeployments(svc *core.Service) error {
 	}
 	return h.deployments.Deployments(svc.Namespace).Delete(context.TODO(), name, meta.DeleteOptions{})
 }
+
+func (h *handler) addFinalizer(svc *core.Service) (*core.Service, error) {
+	if !h.hasFinalizer(svc) {
+		svc.Finalizers = append(svc.Finalizers, finalizerName)
+		return h.services.Services(svc.Namespace).Update(context.TODO(), svc, meta.UpdateOptions{})
+	}
+	return svc, nil
+}
+
+func (h *handler) removeFinalizer(svc *core.Service) (*core.Service, error) {
+	if !h.hasFinalizer(svc) {
+		return svc, nil
+	}
+
+	for k, v := range svc.Finalizers {
+		if v != finalizerName {
+			continue
+		}
+		svc.Finalizers = append(svc.Finalizers[:k], svc.Finalizers[k+1:]...)
+	}
+	return h.services.Services(svc.Namespace).Update(context.TODO(), svc, meta.UpdateOptions{})
+}
+
+func (h *handler) hasFinalizer(svc *core.Service) bool {
+	for _, finalizer := range svc.Finalizers {
+		if finalizer == finalizerName {
+			return true
+		}
+	}
+	return false
+}
+
+// generateName generates a distinct name for the DaemonSet based on the service name and UID
+func generateName(svc *core.Service) string {
+	return fmt.Sprintf("svclb-%s-%s", svc.Name, svc.UID[:8])
+}