diff --git a/pkg/controllers/dnspolicy/dns_helper.go b/pkg/controllers/dnspolicy/dns_helper.go index fc5e45566..7e29a3747 100644 --- a/pkg/controllers/dnspolicy/dns_helper.go +++ b/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, } } diff --git a/pkg/controllers/dnspolicy/dnspolicy_controller.go b/pkg/controllers/dnspolicy/dnspolicy_controller.go index bd60af3aa..e496ad2e4 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_controller.go +++ b/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 } diff --git a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go b/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go index 0dd2487f0..1288403e3 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go +++ b/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go @@ -25,7 +25,7 @@ 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 } @@ -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 @@ -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 diff --git a/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go b/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go index c57dfcb59..4f6747fe8 100644 --- a/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go +++ b/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, } } diff --git a/pkg/controllers/tlspolicy/tlspolicy_controller.go b/pkg/controllers/tlspolicy/tlspolicy_controller.go index f21471e7c..eda4ffc19 100644 --- a/pkg/controllers/tlspolicy/tlspolicy_controller.go +++ b/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 } diff --git a/test/integration/dnspolicy_controller_test.go b/test/integration/dnspolicy_controller_test.go index f8d7fc803..858c3649f 100644 --- a/test/integration/dnspolicy_controller_test.go +++ b/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() { diff --git a/test/integration/tlspolicy_controller_test.go b/test/integration/tlspolicy_controller_test.go index 45b021d21..fdd07e41e 100644 --- a/test/integration/tlspolicy_controller_test.go +++ b/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 {