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

…way target for dnspolicy and tlspolicy

pkg/controllers/dnspolicy/dns_helper.go

@@ -78,13 +78,28 @@ func findMatchingManagedZone(originalHost, host string, zones []v1alpha1.Managed
 	return findMatchingManagedZone(originalHost, parentDomain, zones)
 
 }
-
 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,
 	}
 }
 

pkg/controllers/dnspolicy/dnspolicy_controller.go

@@ -182,6 +182,7 @@ func (r *DNSPolicyReconciler) reconcileResources(ctx context.Context, dnsPolicy
 	// set gateway policy affected condition status - should be the last step, only when all the reconciliation steps succeed
 	updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj)
 	if updateErr != nil {
+		// getting an update err here because the gateway has been deleted
 		return fmt.Errorf("failed to update gateway conditions %w ", updateErr)
 	}
 
@@ -195,8 +196,7 @@ 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
 	}

pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go

@@ -25,15 +25,15 @@ func (r *DNSPolicyReconciler) reconcileDNSRecords(ctx context.Context, dnsPolicy
 
 	for _, gw := range gwDiffObj.GatewaysWithInvalidPolicyRef {
 		log.V(1).Info("reconcileDNSRecords: gateway with invalid policy ref", "key", gw.Key())
-		err := r.deleteGatewayDNSRecords(ctx, gw.Gateway, dnsPolicy)
+		err := r.deleteDNSRecords(ctx, dnsPolicy)
 		if err != nil {
 			return err
 		}
 	}
 
 	// 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
@@ -123,10 +123,10 @@ func (r *DNSPolicyReconciler) reconcileGatewayDNSRecords(ctx context.Context, ga
 	return nil
 }
 
-func (r *DNSPolicyReconciler) deleteGatewayDNSRecords(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error {
+func (r *DNSPolicyReconciler) deleteDNSRecords(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy) error {
 	log := crlog.FromContext(ctx)
 
-	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)))}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(policyDNSRecordLabels(client.ObjectKeyFromObject(dnsPolicy)))}
 	recordsList := &v1alpha1.DNSRecordList{}
 	if err := r.Client().List(ctx, recordsList, listOptions); err != nil {
 		return err

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.deleteCertificates(ctx, 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.deleteUnexpectedCertificates(ctx, expectedCerts, tlsPolicy); err != nil {
 		return err
 	}
 
@@ -65,14 +65,14 @@ 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) deleteCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy) error {
+	return r.deleteUnexpectedCertificates(ctx, []*certmanv1.Certificate{}, tlsPolicy)
 }
 
-func (r *TLSPolicyReconciler) deleteUnexpectedGatewayCertificates(ctx context.Context, expectedCerts []*certmanv1.Certificate, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error {
+func (r *TLSPolicyReconciler) deleteUnexpectedCertificates(ctx context.Context, expectedCerts []*certmanv1.Certificate, tlsPolicy *v1alpha1.TLSPolicy) error {
 	log := crlog.FromContext(ctx)
 
-	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(tlsCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)))}
+	listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(policyTLSCertificateLabels(client.ObjectKeyFromObject(tlsPolicy)))}
 	certList := &certmanv1.CertificateList{}
 	if err := r.Client().List(ctx, certList, listOptions); err != nil {
 		return err
@@ -126,7 +126,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 +148,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() {

test/integration/tlspolicy_controller_test.go

@@ -56,23 +56,27 @@ var _ = Describe("TLSPolicy", Ordered, func() {
 		gatewayList := &gatewayv1beta1.GatewayList{}
 		Expect(k8sClient.List(ctx, gatewayList)).To(BeNil())
 		for _, gw := range gatewayList.Items {
-			k8sClient.Delete(ctx, &gw)
+			err := k8sClient.Delete(ctx, &gw)
+			Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 		}
 		policyList := v1alpha1.TLSPolicyList{}
 		Expect(k8sClient.List(ctx, &policyList)).To(BeNil())
 		for _, policy := range policyList.Items {
-			k8sClient.Delete(ctx, &policy)
+			err := k8sClient.Delete(ctx, &policy)
+			Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 		}
 		issuerList := certmanv1.IssuerList{}
 		Expect(k8sClient.List(ctx, &issuerList)).To(BeNil())
 		for _, issuer := range issuerList.Items {
-			k8sClient.Delete(ctx, &issuer)
+			err := k8sClient.Delete(ctx, &issuer)
+			Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
 		}
 	})
 
 	AfterAll(func() {
 		err := k8sClient.Delete(ctx, gatewayClass)
-		Expect(err).ToNot(HaveOccurred())
+		Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
+
 	})
 
 	Context("invalid target", func() {
@@ -522,7 +526,7 @@ var _ = Describe("TLSPolicy", Ordered, func() {
 					return nil
 				}, time.Second*120, time.Second).Should(BeNil())
 			})
-			It("should delete all tls certificates when tls policy is removed", func() {
+			It("should delete all tls certificates when tls policy is removed even if gateway is already removed", func() {
 				//confirm all expected certificates are present
 				Eventually(func() error {
 					certificateList := &certmanv1.CertificateList{}
@@ -533,8 +537,11 @@ var _ = Describe("TLSPolicy", Ordered, func() {
 					return nil
 				}, time.Second*10, time.Second).Should(BeNil())
 
+				// delete the gateway
+				Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, gateway))).ToNot(HaveOccurred())
+
 				//delete the tls policy
-				Expect(k8sClient.Delete(ctx, tlsPolicy)).To(BeNil())
+				Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, tlsPolicy))).ToNot(HaveOccurred())
 
 				//confirm all certificates have been deleted
 				Eventually(func() error {