Remove unused ingress and services during reconcile (#674)

Co-authored-by: Jan Høydahl <janhoy@users.noreply.github.com>
Co-authored-by: Jason Gerlowski <gerlowskija@apache.org>
Co-authored-by: Houston Putman <houston@apache.org>

Author: 3 people

controllers/controller_utils_test.go

@@ -241,7 +241,7 @@ func expectService(ctx context.Context, parentResource client.Object, serviceNam
 func expectServiceWithChecks(ctx context.Context, parentResource client.Object, serviceName string, selectorLables map[string]string, isHeadless bool, additionalChecks func(Gomega, *corev1.Service), additionalOffset ...int) *corev1.Service {
 	service := &corev1.Service{}
 	EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
-		Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
+		g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
 
 		g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.")
 
@@ -275,7 +275,7 @@ func expectServiceWithChecks(ctx context.Context, parentResource client.Object,
 func expectServiceWithConsistentChecks(ctx context.Context, parentResource client.Object, serviceName string, selectorLables map[string]string, isHeadless bool, additionalChecks func(Gomega, *corev1.Service), additionalOffset ...int) *corev1.Service {
 	service := &corev1.Service{}
 	ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
-		Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
+		g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), service)).To(Succeed(), "Expected Service does not exist")
 
 		g.Expect(service.Spec.Selector).To(Equal(selectorLables), "Service is not pointing to the correct Pods.")
 
@@ -299,10 +299,23 @@ func expectNoService(ctx context.Context, parentResource client.Object, serviceN
 	}).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not")
 }
 
+func eventuallyExpectNoService(ctx context.Context, parentResource client.Object, serviceName string, message string, additionalOffset ...int) {
+	EventuallyWithOffset(resolveOffset(additionalOffset), func() error {
+		return k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})
+	}).Should(MatchError("services \""+serviceName+"\" not found"), message, "Service exists when it should not")
+}
+
 func expectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) {
 	ConsistentlyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
 		for _, serviceName := range serviceNames {
-			g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message)
+			g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName)
+		}
+	}).Should(Succeed())
+}
+func eventuallyExpectNoServices(ctx context.Context, parentResource client.Object, message string, serviceNames []string, additionalOffset ...int) {
+	EventuallyWithOffset(resolveOffset(additionalOffset), func(g Gomega) {
+		for _, serviceName := range serviceNames {
+			g.Expect(k8sClient.Get(ctx, resourceKey(parentResource, serviceName), &corev1.Service{})).To(MatchError("services \""+serviceName+"\" not found"), message, "service", serviceName)
 		}
 	}).Should(Succeed())
 }
@@ -356,6 +369,12 @@ func expectNoIngress(ctx context.Context, parentResource client.Object, ingressN
 	}).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not")
 }
 
+func eventuallyExpectNoIngress(ctx context.Context, parentResource client.Object, ingressName string, additionalOffset ...int) {
+	EventuallyWithOffset(resolveOffset(additionalOffset), func() error {
+		return k8sClient.Get(ctx, resourceKey(parentResource, ingressName), &netv1.Ingress{})
+	}).Should(MatchError("ingresses.networking.k8s.io \""+ingressName+"\" not found"), "Ingress exists when it should not")
+}
+
 func expectPodDisruptionBudget(ctx context.Context, parentResource client.Object, podDisruptionBudgetName string, selector *metav1.LabelSelector, maxUnavailable intstr.IntOrString, additionalOffset ...int) *policyv1.PodDisruptionBudget {
 	return expectPodDisruptionBudgetWithChecks(ctx, parentResource, podDisruptionBudgetName, selector, maxUnavailable, nil, resolveOffset(additionalOffset))
 }

controllers/solrcloud_controller.go

@@ -326,7 +326,6 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	}
 
 	var statefulSet *appsv1.StatefulSet
-
 	if !blockReconciliationOfStatefulSet {
 		// Generate StatefulSet that should exist
 		expectedStatefulSet := util.GenerateStatefulSet(instance, &newStatus, hostNameIpMap, reconcileConfigInfo, tls, security)
@@ -380,12 +379,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		}
 	} else {
 		// If we are blocking the reconciliation of the statefulSet, we still want to find information about it.
-		err = r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet)
-		if err != nil {
-			if !errors.IsNotFound(err) {
-				return requeueOrNot, err
-			} else {
+		statefulSet = &appsv1.StatefulSet{}
+		if e := r.Get(ctx, types.NamespacedName{Name: instance.StatefulSetName(), Namespace: instance.Namespace}, statefulSet); e != nil {
+			if errors.IsNotFound(e) {
 				statefulSet = nil
+			} else {
+				err = e
 			}
 		}
 	}
@@ -424,6 +423,17 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		if err != nil {
 			return requeueOrNot, err
 		}
+	} else {
+		// If ingress exists, delete it
+		foundIngress := &netv1.Ingress{}
+		err = r.Get(ctx, types.NamespacedName{Name: instance.CommonIngressName(), Namespace: instance.GetNamespace()}, foundIngress)
+		if err == nil {
+			err = r.Delete(ctx, foundIngress)
+			if err != nil {
+				return requeueOrNot, err
+			}
+			logger.Info("Deleted Ingress")
+		}
 	}
 
 	// *********************************************************
@@ -638,6 +648,12 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		}
 	}
 
+	// Remove unused services if necessary
+	err = r.cleanupUnconfiguredServices(ctx, instance, podList, logger)
+	if err != nil && !errors.IsNotFound(err) {
+		return requeueOrNot, err
+	}
+
 	if !reflect.DeepEqual(instance.Status, newStatus) {
 		logger.Info("Updating SolrCloud Status", "status", newStatus)
 		oldInstance := instance.DeepCopy()
@@ -651,7 +667,76 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	return requeueOrNot, err
 }
 
-// InitializePods Ensure that all SolrCloud Pods are initialized
+// cleanupUnconfiguredServices remove services that are no longer defined by the SolrCloud resource, and no longer in use by pods.
+// This does not currently include removing per-node services that are no longer in use because the SolrCloud has been scaled down.
+func (r *SolrCloudReconciler) cleanupUnconfiguredServices(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, podList []corev1.Pod, logger logr.Logger) (err error) {
+	var onlyServiceTypeInUse string
+	if solrCloud.UsesHeadlessService() {
+		onlyServiceTypeInUse = util.HeadlessServiceType
+	} else if solrCloud.UsesIndividualNodeServices() {
+		onlyServiceTypeInUse = util.PerNodeServiceType
+	} else {
+		// This should never happen, there are only 2 options
+		return
+	}
+	for _, pod := range podList {
+		if pod.Annotations == nil && pod.Annotations[util.ServiceTypeAnnotation] != "" {
+			if onlyServiceTypeInUse != pod.Annotations[util.ServiceTypeAnnotation] {
+				// Only remove services if all pods are using the same, and configured, type of service.
+				// Otherwise, we are in transition between service types and need to wait to delete anything.
+				return
+			}
+		} else {
+			// If we have a pod missing this annotation, then it has not been fully updated to a supported Operator version.
+			// We don't have the information, so assume both serviceTypes are in use, and don't remove anything.
+			return
+		}
+	}
+
+	// If we are at this point, then we can assume we are completely transitioned and can delete the unused services
+	if solrCloud.UsesHeadlessService() {
+		err = r.deleteServicesOfType(ctx, solrCloud, util.PerNodeServiceType, logger)
+	} else if solrCloud.UsesIndividualNodeServices() {
+		err = r.deleteServicesOfType(ctx, solrCloud, util.HeadlessServiceType, logger)
+	}
+	return
+}
+
+func (r *SolrCloudReconciler) deleteServicesOfType(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, serviceType string, logger logr.Logger) (err error) {
+	foundServices := &corev1.ServiceList{}
+	selectorLabels := solrCloud.SharedLabels()
+	selectorLabels[util.ServiceTypeAnnotation] = serviceType
+
+	serviceSelector := labels.SelectorFromSet(selectorLabels)
+	listOps := &client.ListOptions{
+		Namespace:     solrCloud.Namespace,
+		LabelSelector: serviceSelector,
+	}
+
+	if err = r.List(ctx, foundServices, listOps); err != nil {
+		logger.Error(err, "Error listing services for SolrCloud", "serviceType", serviceType)
+		return
+	}
+
+	for _, headlessService := range foundServices.Items {
+		if e := r.deleteService(ctx, &headlessService, logger); e != nil {
+			// Don't break, just add the error for later
+			err = e
+		}
+	}
+	return
+}
+
+func (r *SolrCloudReconciler) deleteService(ctx context.Context, service *corev1.Service, logger logr.Logger) (err error) {
+	logger.Info("Deleting Service for SolrCloud", "Service", service.Name)
+	err = r.Client.Delete(ctx, service)
+	if err != nil && !errors.IsNotFound(err) {
+		logger.Error(err, "Error deleting unused Service for SolrCloud", "Service", service.Name)
+	}
+	return
+}
+
+// initializePods Ensure that all SolrCloud Pods are initialized
 func (r *SolrCloudReconciler) initializePods(ctx context.Context, solrCloud *solrv1beta1.SolrCloud, statefulSet *appsv1.StatefulSet, logger logr.Logger) (podSelector labels.Selector, podList []corev1.Pod, err error) {
 	foundPods := &corev1.PodList{}
 	selectorLabels := solrCloud.SharedLabels()

controllers/solrcloud_controller_backup_test.go

@@ -98,6 +98,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() {
 			Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(testPodAnnotations, map[string]string{
 				"solr.apache.org/solrXmlMd5":          solrXmlMd5,
 				util.SolrBackupRepositoriesAnnotation: "test-repo",
+				util.ServiceTypeAnnotation:            util.HeadlessServiceType,
 			})), "Incorrect pod annotations")
 
 			// Env Variable Tests
@@ -321,6 +322,7 @@ var _ = FDescribe("SolrCloud controller - Backup Repositories", func() {
 			Expect(statefulSet.Spec.Template.Annotations).To(Equal(map[string]string{
 				"solr.apache.org/solrXmlMd5":          fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"]))),
 				util.SolrBackupRepositoriesAnnotation: "another,test-repo",
+				util.ServiceTypeAnnotation:            util.HeadlessServiceType,
 			}), "Incorrect pod annotations")
 		})
 	})

controllers/solrcloud_controller_ingress_test.go

@@ -163,6 +163,92 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() {
 		})
 	})
 
+	FContext("Full Ingress - Switch off after creation", func() {
+		BeforeEach(func() {
+			solrCloud.Spec.SolrAddressability = solrv1beta1.SolrAddressabilityOptions{
+				External: &solrv1beta1.ExternalAddressability{
+					Method:             solrv1beta1.Ingress,
+					UseExternalAddress: true,
+					DomainName:         testDomain,
+					NodePortOverride:   100,
+				},
+				PodPort:           3000,
+				CommonServicePort: 4000,
+			}
+		})
+		FIt("has the correct resources", func(ctx context.Context) {
+			By("testing the Solr StatefulSet")
+			statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName())
+			// Pod Annotations test
+			Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.PerNodeServiceType), "Since external address is used for advertising, the perNode service should be specified in the pod annotations.")
+
+			By("testing the Solr Common Service")
+			commonService := expectService(ctx, solrCloud, solrCloud.CommonServiceName(), statefulSet.Spec.Selector.MatchLabels, false)
+			Expect(commonService.Annotations).To(Equal(testCommonServiceAnnotations), "Incorrect common service annotations")
+			Expect(commonService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
+			Expect(commonService.Spec.Ports[0].Port).To(Equal(int32(4000)), "Wrong port on common Service")
+			Expect(commonService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on common Service")
+
+			By("ensuring the Solr Headless Service does not exist")
+			expectNoService(ctx, solrCloud, solrCloud.HeadlessServiceName(), "Headless service shouldn't exist, but it does.")
+
+			By("making sure the individual Solr Node Services exist and route correctly")
+			nodeNames := solrCloud.GetAllSolrPodNames()
+			Expect(nodeNames).To(HaveLen(replicas), "SolrCloud has incorrect number of nodeNames.")
+			for _, nodeName := range nodeNames {
+				service := expectService(ctx, solrCloud, nodeName, util.MergeLabelsOrAnnotations(statefulSet.Spec.Selector.MatchLabels, map[string]string{"statefulset.kubernetes.io/pod-name": nodeName}), false)
+				expectedNodeServiceLabels := util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), map[string]string{"service-type": "external"})
+				Expect(service.Labels).To(Equal(util.MergeLabelsOrAnnotations(expectedNodeServiceLabels, testNodeServiceLabels)), "Incorrect node '"+nodeName+"' service labels")
+				Expect(service.Annotations).To(Equal(testNodeServiceAnnotations), "Incorrect node '"+nodeName+"' service annotations")
+				Expect(service.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
+				Expect(service.Spec.Ports[0].Port).To(Equal(int32(100)), "Wrong port on node Service")
+				Expect(service.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on node Service")
+			}
+
+			By("making sure Ingress was created correctly")
+			ingress := expectIngress(ctx, solrCloud, solrCloud.CommonIngressName())
+			Expect(ingress.Labels).To(Equal(util.MergeLabelsOrAnnotations(solrCloud.SharedLabelsWith(solrCloud.Labels), testIngressLabels)), "Incorrect ingress labels")
+			Expect(ingress.Annotations).To(Equal(ingressLabelsWithDefaults(testIngressAnnotations)), "Incorrect ingress annotations")
+			Expect(ingress.Spec.IngressClassName).To(Not(BeNil()), "Ingress class name should not be nil")
+			Expect(*ingress.Spec.IngressClassName).To(Equal(testIngressClass), "Incorrect ingress class name")
+			testIngressRules(solrCloud, ingress, true, replicas, 4000, 100, testDomain)
+
+			By("making sure the node addresses in the Status are correct")
+			expectSolrCloudStatusWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloudStatus) {
+				g.Expect(found.InternalCommonAddress).To(Equal("http://"+solrCloud.CommonServiceName()+"."+solrCloud.Namespace+":4000"), "Wrong internal common address in status")
+				g.Expect(found.ExternalCommonAddress).To(Not(BeNil()), "External common address in status should not be nil")
+				g.Expect(*found.ExternalCommonAddress).To(Equal("http://"+solrCloud.Namespace+"-"+solrCloud.Name+"-solrcloud"+"."+testDomain), "Wrong external common address in status")
+			})
+
+			By("Turning off node external addressability and making sure the node services are deleted")
+			expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) {
+				found.Spec.SolrAddressability.External.UseExternalAddress = false
+				found.Spec.SolrAddressability.External.HideNodes = true
+				g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to not advertise the nodes externally.")
+			})
+
+			// Since node external addressability is off, but common external addressability is on, the ingress should exist, but the node services should not
+			eventuallyExpectNoServices(ctx, solrCloud, "Node services shouldn't exist after the update, but they do.", nodeNames)
+
+			expectIngress(ctx, solrCloud, solrCloud.CommonIngressName())
+
+			headlessService := expectService(ctx, solrCloud, solrCloud.HeadlessServiceName(), statefulSet.Spec.Selector.MatchLabels, true)
+			Expect(headlessService.Annotations).To(Equal(testHeadlessServiceAnnotations), "Incorrect headless service annotations")
+			Expect(headlessService.Spec.Ports[0].Name).To(Equal("solr-client"), "Wrong port name on common Service")
+			Expect(headlessService.Spec.Ports[0].Port).To(Equal(int32(3000)), "Wrong port on headless Service")
+			Expect(headlessService.Spec.Ports[0].TargetPort.StrVal).To(Equal("solr-client"), "Wrong podPort name on headless Service")
+			Expect(headlessService.Spec.Ports[0].Protocol).To(Equal(corev1.ProtocolTCP), "Wrong protocol on headless Service")
+			Expect(headlessService.Spec.Ports[0].AppProtocol).To(BeNil(), "AppProtocol on headless Service should be nil when not running with TLS")
+
+			By("Turning off common external addressability and making sure the ingress is deleted")
+			expectSolrCloudWithChecks(ctx, solrCloud, func(g Gomega, found *solrv1beta1.SolrCloud) {
+				found.Spec.SolrAddressability.External = nil
+				g.Expect(k8sClient.Update(ctx, found)).To(Succeed(), "Couldn't update the solrCloud to remove external addressability")
+			})
+			eventuallyExpectNoIngress(ctx, solrCloud, solrCloud.CommonIngressName())
+		})
+	})
+
 	FContext("Hide Nodes from external connections - Using default ingress class", func() {
 		ingressClass := &netv1.IngressClass{
 			ObjectMeta: metav1.ObjectMeta{
@@ -206,6 +292,7 @@ var _ = FDescribe("SolrCloud controller - Ingress", func() {
 
 			By("testing the Solr StatefulSet")
 			statefulSet := expectStatefulSet(ctx, solrCloud, solrCloud.StatefulSetName())
+			Expect(statefulSet.Spec.Template.Annotations).To(HaveKeyWithValue(util.ServiceTypeAnnotation, util.HeadlessServiceType), "Since external address is not used for advertising, the headless service should be specified in the pod annotations.")
 
 			Expect(statefulSet.Spec.Template.Spec.Containers).To(HaveLen(1), "Solr StatefulSet requires a container.")
 

controllers/solrcloud_controller_test.go

@@ -280,7 +280,7 @@ var _ = FDescribe("SolrCloud controller - General", func() {
 			Expect(statefulSet.Spec.Template.ObjectMeta.Annotations).To(HaveKey(util.SolrScheduledRestartAnnotation), "Pod Template does not have scheduled restart annotation when it should")
 			// Remove the annotation when we know that it exists, we don't know the exact value so we can't check it below.
 			delete(statefulSet.Spec.Template.Annotations, util.SolrScheduledRestartAnnotation)
-			Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(map[string]string{"solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"])))}, testPodAnnotations)), "Incorrect pod annotations")
+			Expect(statefulSet.Spec.Template.Annotations).To(Equal(util.MergeLabelsOrAnnotations(map[string]string{util.ServiceTypeAnnotation: util.HeadlessServiceType, "solr.apache.org/solrXmlMd5": fmt.Sprintf("%x", md5.Sum([]byte(configMap.Data["solr.xml"])))}, testPodAnnotations)), "Incorrect pod annotations")
 			Expect(statefulSet.Spec.Template.Spec.NodeSelector).To(Equal(testNodeSelectors), "Incorrect pod node selectors")
 
 			Expect(statefulSet.Spec.Template.Spec.Containers[0].LivenessProbe, testProbeLivenessNonDefaults, "Incorrect Liveness Probe")