gh-628 fix deletion of dnsrecord and certificates on deletion of gat…

…eway target for dnspolicy and tlspolicy

pkg/controllers/dnspolicy/dns_helper.go

@@ -80,11 +80,27 @@ func findMatchingManagedZone(originalHost, host string, zones []v1alpha1.Managed
 }
 
 func commonDNSRecordLabels(gwKey, apKey client.ObjectKey) map[string]string {
+	common := map[string]string{}
+	for k, v := range policyDNSRecordLabels(apKey) {
+		common[k] = v
+	}
+	for k, v := range gatewayDNSRecordLabels(gwKey) {
+		common[k] = v
+	}
+	return common
+}
+
+func policyDNSRecordLabels(apKey client.ObjectKey) map[string]string {
 	return map[string]string{
 		DNSPolicyBackRefAnnotation:                              apKey.Name,
 		fmt.Sprintf("%s-namespace", DNSPolicyBackRefAnnotation): apKey.Namespace,
-		LabelGatewayNSRef:                                       gwKey.Namespace,
-		LabelGatewayReference:                                   gwKey.Name,
+	}
+}
+
+func gatewayDNSRecordLabels(gwKey client.ObjectKey) map[string]string {
+	return map[string]string{
+		LabelGatewayNSRef:     gwKey.Namespace,
+		LabelGatewayReference: gwKey.Name,
 	}
 }
 
@@ -282,13 +298,13 @@ func createOrUpdateEndpoint(dnsName string, targets v1alpha1.Targets, recordType
 }
 
 // removeDNSForDeletedListeners remove any DNSRecords that are associated with listeners that no longer exist in this gateway
-func (r *dnsHelper) removeDNSForDeletedListeners(ctx context.Context, upstreamGateway *gatewayv1beta1.Gateway) error {
+func (dh *dnsHelper) removeDNSForDeletedListeners(ctx context.Context, upstreamGateway *gatewayv1beta1.Gateway) error {
 	dnsList := &v1alpha1.DNSRecordList{}
 	//List all dns records that belong to this gateway
 	labelSelector := &client.MatchingLabels{
 		LabelGatewayReference: upstreamGateway.Name,
 	}
-	if err := r.List(ctx, dnsList, labelSelector, &client.ListOptions{Namespace: upstreamGateway.Namespace}); err != nil {
+	if err := dh.List(ctx, dnsList, labelSelector, &client.ListOptions{Namespace: upstreamGateway.Namespace}); err != nil {
 		return err
 	}
 
@@ -301,7 +317,7 @@ func (r *dnsHelper) removeDNSForDeletedListeners(ctx context.Context, upstreamGa
 			}
 		}
 		if !listenerExists {
-			if err := r.Delete(ctx, &dns, &client.DeleteOptions{}); client.IgnoreNotFound(err) != nil {
+			if err := dh.Delete(ctx, &dns, &client.DeleteOptions{}); client.IgnoreNotFound(err) != nil {
 				return err
 			}
 		}
@@ -310,9 +326,9 @@ func (r *dnsHelper) removeDNSForDeletedListeners(ctx context.Context, upstreamGa
 
 }
 
-func (r *dnsHelper) getManagedZoneForListener(ctx context.Context, ns string, listener gatewayv1beta1.Listener) (*v1alpha1.ManagedZone, error) {
+func (dh *dnsHelper) getManagedZoneForListener(ctx context.Context, ns string, listener gatewayv1beta1.Listener) (*v1alpha1.ManagedZone, error) {
 	var managedZones v1alpha1.ManagedZoneList
-	if err := r.List(ctx, &managedZones, client.InNamespace(ns)); err != nil {
+	if err := dh.List(ctx, &managedZones, client.InNamespace(ns)); err != nil {
 		log.FromContext(ctx).Error(err, "unable to list managed zones for gateway ", "in ns", ns)
 		return nil, err
 	}
@@ -325,37 +341,37 @@ func dnsRecordName(gatewayName, listenerName string) string {
 	return fmt.Sprintf("%s-%s", gatewayName, listenerName)
 }
 
-func (r *dnsHelper) createDNSRecordForListener(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy, mz *v1alpha1.ManagedZone, listener gatewayv1beta1.Listener) (*v1alpha1.DNSRecord, error) {
+func (dh *dnsHelper) createDNSRecordForListener(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy, mz *v1alpha1.ManagedZone, listener gatewayv1beta1.Listener) (*v1alpha1.DNSRecord, error) {
 
 	log := log.FromContext(ctx)
 	log.Info("creating dns for gateway listener", "listener", listener.Name)
-	dnsRecord := r.buildDNSRecordForListener(gateway, dnsPolicy, listener, mz)
-	if err := controllerutil.SetControllerReference(mz, dnsRecord, r.Scheme()); err != nil {
+	dnsRecord := dh.buildDNSRecordForListener(gateway, dnsPolicy, listener, mz)
+	if err := controllerutil.SetControllerReference(mz, dnsRecord, dh.Scheme()); err != nil {
 		return dnsRecord, err
 	}
 
-	err := r.Create(ctx, dnsRecord, &client.CreateOptions{})
+	err := dh.Create(ctx, dnsRecord, &client.CreateOptions{})
 	if err != nil && !k8serrors.IsAlreadyExists(err) {
 		return dnsRecord, err
 	}
 	if err != nil && k8serrors.IsAlreadyExists(err) {
-		err = r.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)
+		err = dh.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)
 		if err != nil {
 			return dnsRecord, err
 		}
 	}
 	return dnsRecord, nil
 }
 
-func (r *dnsHelper) deleteDNSRecordForListener(ctx context.Context, owner metav1.Object, listener gatewayv1beta1.Listener) error {
+func (dh *dnsHelper) deleteDNSRecordForListener(ctx context.Context, owner metav1.Object, listener gatewayv1beta1.Listener) error {
 	recordName := dnsRecordName(owner.GetName(), string(listener.Name))
 	dnsRecord := v1alpha1.DNSRecord{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      recordName,
 			Namespace: owner.GetNamespace(),
 		},
 	}
-	return r.Delete(ctx, &dnsRecord, &client.DeleteOptions{})
+	return dh.Delete(ctx, &dnsRecord, &client.DeleteOptions{})
 }
 
 func isWildCardListener(l gatewayv1beta1.Listener) bool {

pkg/controllers/dnspolicy/dnspolicy_controller.go

@@ -195,13 +195,12 @@ func (r *DNSPolicyReconciler) deleteResources(ctx context.Context, dnsPolicy *v1
 	if err != nil {
 		return err
 	}
-
-	if err := r.reconcileDNSRecords(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+	if err = r.deleteDNSRecords(ctx, dnsPolicy); err != nil {
 		log.V(3).Info("error reconciling DNS records from delete, returning", "error", err)
 		return err
 	}
 
-	if err := r.reconcileHealthChecks(ctx, dnsPolicy, gatewayDiffObj); err != nil {
+	if err := r.deleteProbes(ctx, dnsPolicy); err != nil {
 		return err
 	}
 

pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go

@@ -33,7 +33,7 @@ func (r *DNSPolicyReconciler) reconcileDNSRecords(ctx context.Context, dnsPolicy
 
 	// Reconcile DNSRecords for each gateway directly referred by the policy (existing and new)
 	for _, gw := range append(gwDiffObj.GatewaysWithValidPolicyRef, gwDiffObj.GatewaysMissingPolicyRef...) {
-		log.V(1).Info("reconcileDNSRecords: gateway with valid and missing policy ref", "key", gw.Key())
+		log.V(1).Info("reconcileDNSRecords: gateway with valid or missing policy ref", "key", gw.Key())
 		err := r.reconcileGatewayDNSRecords(ctx, gw.Gateway, dnsPolicy)
 		if err != nil {
 			return err
@@ -124,9 +124,17 @@ func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, ga
 }
 
 func (r *DNSPolicyReconciler) deleteGatewayDNSRecords(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
+	return r.deleteDNSRecordsWithLabels(ctx, commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)))
+}
+
+func (r *DNSPolicyReconciler) deleteDNSRecords(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy) error {
+	return r.deleteDNSRecordsWithLabels(ctx, (policyDNSRecordLabels(client.ObjectKeyFromObject(dnsPolicy))))
+}
+
+func (r *DNSPolicyReconciler) deleteDNSRecordsWithLabels(ctx context.Context, lbls map[string]string) error {
 	log := crlog.FromContext(ctx)
 
-	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)))}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls)}
 	recordsList := &v1alpha1.DNSRecordList{}
 	if err := r.Client().List(ctx, recordsList, listOptions); err != nil {
 		return err

pkg/controllers/dnspolicy/dnspolicy_healthchecks.go

@@ -3,6 +3,8 @@ package dnspolicy
 import (
 	"context"
 	"fmt"
+	"github.com/Kuadrant/multicluster-gateway-controller/pkg/_internal/slice"
+	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 	"strings"
 	"time"
 
@@ -15,7 +17,6 @@ import (
 	"github.com/kuadrant/kuadrant-operator/pkg/common"
 	"github.com/kuadrant/kuadrant-operator/pkg/reconcilers"
 
-	"github.com/Kuadrant/multicluster-gateway-controller/pkg/_internal/slice"
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/apis/v1alpha1"
 )
 
@@ -29,15 +30,15 @@ func (r *DNSPolicyReconciler) reconcileHealthChecks(ctx context.Context, dnsPoli
 		if err := r.createOrUpdateProbes(ctx, expectedProbes); err != nil {
 			return fmt.Errorf("error creating and updating expected proves for gateway %v: %w", gw.Gateway.Name, err)
 		}
-		if err := r.deleteUnexpectedGatewayProbes(ctx, expectedProbes, gw, dnsPolicy); err != nil {
+		if err := r.deleteUnexpectedGatewayProbes(ctx, expectedProbes, gw.Gateway, dnsPolicy); err != nil {
 			return fmt.Errorf("error removing unexpected probes for gateway %v: %w", gw.Gateway.Name, err)
 		}
 
 	}
 
 	for _, gw := range gwDiffObj.GatewaysWithInvalidPolicyRef {
 		log.V(3).Info("deleting probes", "gateway", gw.Gateway.Name)
-		if err := r.deleteUnexpectedGatewayProbes(ctx, []*v1alpha1.DNSHealthCheckProbe{}, gw, dnsPolicy); err != nil {
+		if err := r.deleteGatewayProbes(ctx, gw.Gateway, dnsPolicy); err != nil {
 			return fmt.Errorf("error deleting probes for gw %v: %w", gw.Gateway.Name, err)
 		}
 	}
@@ -66,25 +67,45 @@ func (r *DNSPolicyReconciler) createOrUpdateProbes(ctx context.Context, expected
 	return nil
 }
 
-func (r *DNSPolicyReconciler) deleteUnexpectedGatewayProbes(ctx context.Context, expectedProbes []*v1alpha1.DNSHealthCheckProbe, gw common.GatewayWrapper, dnsPolicy *v1alpha1.DNSPolicy) error {
+func (r *DNSPolicyReconciler) deleteGatewayProbes(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
+	return r.deleteProbesWithLabels(ctx, commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)))
+}
+
+func (r *DNSPolicyReconciler) deleteProbes(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy) error {
+	return r.deleteProbesWithLabels(ctx, policyDNSRecordLabels(client.ObjectKeyFromObject(dnsPolicy)))
+}
+
+func (r *DNSPolicyReconciler) deleteProbesWithLabels(ctx context.Context, lbls map[string]string) error {
+	probes := &v1alpha1.DNSHealthCheckProbeList{}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls)}
+	if err := r.Client().List(ctx, probes, listOptions); client.IgnoreNotFound(err) != nil {
+		return err
+	}
+	for _, p := range probes.Items {
+		if err := r.Client().Delete(ctx, &p); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func (r *DNSPolicyReconciler) deleteUnexpectedGatewayProbes(ctx context.Context, expectedProbes []*v1alpha1.DNSHealthCheckProbe, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
 	// remove any probes for this gateway and DNS Policy that are no longer expected
 	existingProbes := &v1alpha1.DNSHealthCheckProbeList{}
-	dnsLabels := commonDNSRecordLabels(client.ObjectKeyFromObject(gw), client.ObjectKeyFromObject(dnsPolicy))
+	dnsLabels := commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy))
 	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(dnsLabels)}
 	if err := r.Client().List(ctx, existingProbes, listOptions); client.IgnoreNotFound(err) != nil {
 		return err
-	} else {
-		for _, p := range existingProbes.Items {
-			if !slice.Contains(expectedProbes, func(expectedProbe *v1alpha1.DNSHealthCheckProbe) bool {
-				return expectedProbe.Name == p.Name && expectedProbe.Namespace == p.Namespace
-			}) {
-				if err := r.Client().Delete(ctx, &p); err != nil {
-					return err
-				}
+	}
+	for _, p := range existingProbes.Items {
+		if !slice.Contains(expectedProbes, func(expectedProbe *v1alpha1.DNSHealthCheckProbe) bool {
+			return expectedProbe.Name == p.Name && expectedProbe.Namespace == p.Namespace
+		}) {
+			if err := r.Client().Delete(ctx, &p); err != nil {
+				return err
 			}
 		}
 	}
-
 	return nil
 }
 

pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go

@@ -27,14 +27,14 @@ func (r *TLSPolicyReconciler) reconcileCertificates(ctx context.Context, tlsPoli
 
 	for _, gw := range gwDiffObj.GatewaysWithInvalidPolicyRef {
 		log.V(1).Info("reconcileCertificates: gateway with invalid policy ref", "key", gw.Key())
-		if err := r.deleteGatewayCertificates(ctx, gw.Gateway, tlsPolicy); err != nil {
+		if err := r.deleteGatewayCertificates(ctx, []*certmanv1.Certificate{}, gw.Gateway, tlsPolicy); err != nil {
 			return err
 		}
 	}
 
 	// Reconcile Certificates for each gateway directly referred by the policy (existing and new)
 	for _, gw := range append(gwDiffObj.GatewaysWithValidPolicyRef, gwDiffObj.GatewaysMissingPolicyRef...) {
-		log.V(1).Info("reconcileCertificates: gateway with valid and missing policy ref", "key", gw.Key())
+		log.V(1).Info("reconcileCertificates: gateway with valid or missing policy ref", "key", gw.Key())
 		if err := r.reconcileGatewayCertificates(ctx, gw.Gateway, tlsPolicy); err != nil {
 			return err
 		}
@@ -50,7 +50,7 @@ func (r *TLSPolicyReconciler) reconcileGatewayCertificates(ctx context.Context,
 
 	expectedCerts := r.expectedCertificatesForGateway(ctx, gateway, tlsPolicy)
 
-	if err := r.deleteUnexpectedGatewayCertificates(ctx, expectedCerts, gateway, tlsPolicy); err != nil {
+	if err := r.deleteGatewayCertificates(ctx, expectedCerts, gateway, tlsPolicy); err != nil {
 		return err
 	}
 
@@ -65,14 +65,18 @@ func (r *TLSPolicyReconciler) reconcileGatewayCertificates(ctx context.Context,
 	return nil
 }
 
-func (r *TLSPolicyReconciler) deleteGatewayCertificates(ctx context.Context, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error {
-	return r.deleteUnexpectedGatewayCertificates(ctx, []*certmanv1.Certificate{}, gateway, tlsPolicy)
+func (r *TLSPolicyReconciler) deleteGatewayCertificates(ctx context.Context, expectedCerts []*certmanv1.Certificate, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error {
+	return r.deleteCertificatesWithLabels(ctx, expectedCerts, commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)))
 }
 
-func (r *TLSPolicyReconciler) deleteUnexpectedGatewayCertificates(ctx context.Context, expectedCerts []*certmanv1.Certificate, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error {
+func (r *TLSPolicyReconciler) deleteCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy) error {
+	return r.deleteCertificatesWithLabels(ctx, []*certmanv1.Certificate{}, policyTLSCertificateLabels(client.ObjectKeyFromObject(tlsPolicy)))
+}
+
+func (r *TLSPolicyReconciler) deleteCertificatesWithLabels(ctx context.Context, expectedCerts []*certmanv1.Certificate, lbls map[string]string) error {
 	log := crlog.FromContext(ctx)
 
-	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(tlsCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)))}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls)}
 	certList := &certmanv1.CertificateList{}
 	if err := r.Client().List(ctx, certList, listOptions); err != nil {
 		return err
@@ -126,7 +130,7 @@ func (r *TLSPolicyReconciler) expectedCertificatesForGateway(ctx context.Context
 }
 
 func (r *TLSPolicyReconciler) buildCertManagerCertificate(gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy, secretRef corev1.ObjectReference, hosts []string) *certmanv1.Certificate {
-	tlsCertLabels := tlsCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy))
+	tlsCertLabels := commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy))
 
 	crt := &certmanv1.Certificate{
 		ObjectMeta: metav1.ObjectMeta{
@@ -148,12 +152,28 @@ func (r *TLSPolicyReconciler) buildCertManagerCertificate(gateway *gatewayv1beta
 	return crt
 }
 
-func tlsCertificateLabels(gwKey, apKey client.ObjectKey) map[string]string {
+func commonTLSCertificateLabels(gwKey, apKey client.ObjectKey) map[string]string {
+	common := map[string]string{}
+	for k, v := range policyTLSCertificateLabels(apKey) {
+		common[k] = v
+	}
+	for k, v := range gatewayTLSCertificateLabels(gwKey) {
+		common[k] = v
+	}
+	return common
+}
+
+func policyTLSCertificateLabels(apKey client.ObjectKey) map[string]string {
 	return map[string]string{
 		TLSPolicyBackRefAnnotation:                              apKey.Name,
 		fmt.Sprintf("%s-namespace", TLSPolicyBackRefAnnotation): apKey.Namespace,
-		"gateway-namespace":                                     gwKey.Namespace,
-		"gateway":                                               gwKey.Name,
+	}
+}
+
+func gatewayTLSCertificateLabels(gwKey client.ObjectKey) map[string]string {
+	return map[string]string{
+		"gateway-namespace": gwKey.Namespace,
+		"gateway":           gwKey.Name,
 	}
 }
 

pkg/controllers/tlspolicy/tlspolicy_controller.go

@@ -196,7 +196,7 @@ func (r *TLSPolicyReconciler) deleteResources(ctx context.Context, tlsPolicy *v1
 		return err
 	}
 
-	if err := r.reconcileCertificates(ctx, tlsPolicy, gatewayDiffObj); err != nil {
+	if err := r.deleteCertificates(ctx, tlsPolicy); err != nil {
 		return err
 	}
 

test/integration/dnspolicy_controller_test.go

@@ -4,6 +4,7 @@ package integration
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"time"
 
@@ -340,7 +341,8 @@ var _ = Describe("DNSPolicy", Ordered, func() {
 			Expect(err).ToNot(HaveOccurred())
 
 			for _, record := range dnsRecordList.Items {
-				Expect(k8sClient.Delete(ctx, &record)).ToNot(HaveOccurred())
+				err := k8sClient.Delete(ctx, &record)
+				Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 			}
 		})
 
@@ -673,6 +675,34 @@ var _ = Describe("DNSPolicy", Ordered, func() {
 					return nil
 				}, time.Second*5, time.Second).Should(BeNil())
 			})
+
+			It("should remove dns record reference on policy deletion even if gateway is removed", func() {
+				createdDNSRecord := &v1alpha1.DNSRecord{}
+				Eventually(func() error { // DNS record exists
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: dnsRecordName, Namespace: testNamespace}, createdDNSRecord); err != nil {
+						return err
+					}
+					return nil
+				}, TestTimeoutMedium, TestRetryIntervalMedium).Should(BeNil())
+
+				err := k8sClient.Delete(ctx, gateway)
+				Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
+
+				dnsPolicy = testBuildDNSPolicyWithHealthCheck("test-dns-policy", TestPlacedGatewayName, testNamespace, nil)
+				err = k8sClient.Delete(ctx, dnsPolicy)
+				Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
+
+				Eventually(func() error { // DNS record removed
+					if err := k8sClient.Get(ctx, client.ObjectKey{Name: dnsRecordName, Namespace: testNamespace}, createdDNSRecord); err != nil {
+						if k8serrors.IsNotFound(err) {
+							return nil
+						}
+						return err
+					}
+					return errors.New("found dnsrecord when it should be deleted")
+				}, TestTimeoutMedium, TestRetryIntervalMedium).Should(BeNil())
+			})
+
 		})
 
 		Context("geo dnspolicy", func() {