diff --git a/pkg/controllers/dnspolicy/dns_helper.go b/pkg/controllers/dnspolicy/dns_helper.go index fc5e45566..68c4d38ff 100644 --- a/pkg/controllers/dnspolicy/dns_helper.go +++ b/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,21 +341,21 @@ 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 } @@ -347,7 +363,7 @@ func (r *dnsHelper) createDNSRecordForListener(ctx context.Context, gateway *gat 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{ @@ -355,7 +371,7 @@ func (r *dnsHelper) deleteDNSRecordForListener(ctx context.Context, owner metav1 Namespace: owner.GetNamespace(), }, } - return r.Delete(ctx, &dnsRecord, &client.DeleteOptions{}) + return dh.Delete(ctx, &dnsRecord, &client.DeleteOptions{}) } func isWildCardListener(l gatewayv1beta1.Listener) bool { diff --git a/pkg/controllers/dnspolicy/dnspolicy_controller.go b/pkg/controllers/dnspolicy/dnspolicy_controller.go index bd60af3aa..b03a1f69a 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_controller.go +++ b/pkg/controllers/dnspolicy/dnspolicy_controller.go @@ -159,7 +159,7 @@ func (r *DNSPolicyReconciler) reconcileResources(ctx context.Context, dnsPolicy return errors.Join(fmt.Errorf("reconcile DNSRecords error %w", err), updateErr) } - if err = r.reconcileHealthChecks(ctx, dnsPolicy, gatewayDiffObj); err != nil { + if err = r.reconcileHealthCheckProbes(ctx, dnsPolicy, gatewayDiffObj); err != nil { gatewayCondition = conditions.BuildPolicyAffectedCondition(DNSPolicyAffected, dnsPolicy, targetNetworkObject, conditions.PolicyReasonInvalid, err) updateErr := r.updateGatewayCondition(ctx, gatewayCondition, gatewayDiffObj) return errors.Join(fmt.Errorf("reconcile HealthChecks error %w", err), updateErr) @@ -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.deleteHealthCheckProbes(ctx, dnsPolicy); err != nil { return err } diff --git a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go b/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go index 0dd2487f0..53d09025c 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go +++ b/pkg/controllers/dnspolicy/dnspolicy_dnsrecords.go @@ -23,23 +23,21 @@ import ( func (r *DNSPolicyReconciler) reconcileDNSRecords(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, gwDiffObj *reconcilers.GatewayDiff) error { log := crlog.FromContext(ctx) + log.V(3).Info("reconciling dns records") 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) - if err != nil { - return err + if err := r.deleteGatewayDNSRecords(ctx, gw.Gateway, dnsPolicy); err != nil { + return fmt.Errorf("error deleting dns records for gw %v: %w", gw.Gateway.Name, 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()) - err := r.reconcileGatewayDNSRecords(ctx, gw.Gateway, dnsPolicy) - if err != nil { - return err + log.V(1).Info("reconcileDNSRecords: gateway with valid or missing policy ref", "key", gw.Key()) + if err := r.reconcileGatewayDNSRecords(ctx, gw.Gateway, dnsPolicy); err != nil { + return fmt.Errorf("error reconciling dns records for gateway %v: %w", gw.Gateway.Name, err) } } - return nil } @@ -124,9 +122,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)), dnsPolicy.Namespace) +} + +func (r *DNSPolicyReconciler) deleteDNSRecords(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy) error { + return r.deleteDNSRecordsWithLabels(ctx, policyDNSRecordLabels(client.ObjectKeyFromObject(dnsPolicy)), dnsPolicy.Namespace) +} + +func (r *DNSPolicyReconciler) deleteDNSRecordsWithLabels(ctx context.Context, lbls map[string]string, namespace 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), Namespace: namespace} recordsList := &v1alpha1.DNSRecordList{} if err := r.Client().List(ctx, recordsList, listOptions); err != nil { return err diff --git a/pkg/controllers/dnspolicy/dnspolicy_healthchecks.go b/pkg/controllers/dnspolicy/dnspolicy_healthchecks.go index ebccd4ed1..fa05bcea9 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_healthchecks.go +++ b/pkg/controllers/dnspolicy/dnspolicy_healthchecks.go @@ -11,6 +11,7 @@ import ( "k8s.io/apimachinery/pkg/labels" "sigs.k8s.io/controller-runtime/pkg/client" crlog "sigs.k8s.io/controller-runtime/pkg/log" + gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/kuadrant/kuadrant-operator/pkg/common" "github.com/kuadrant/kuadrant-operator/pkg/reconcilers" @@ -19,33 +20,33 @@ import ( "github.com/Kuadrant/multicluster-gateway-controller/pkg/apis/v1alpha1" ) -func (r *DNSPolicyReconciler) reconcileHealthChecks(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, gwDiffObj *reconcilers.GatewayDiff) error { +func (r *DNSPolicyReconciler) reconcileHealthCheckProbes(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, gwDiffObj *reconcilers.GatewayDiff) error { log := crlog.FromContext(ctx) log.V(3).Info("reconciling health checks") + for _, gw := range gwDiffObj.GatewaysWithInvalidPolicyRef { + log.V(1).Info("reconcileHealthCheckProbes: gateway with invalid policy ref", "key", gw.Key()) + if err := r.deleteGatewayHealthCheckProbes(ctx, gw.Gateway, dnsPolicy); err != nil { + return fmt.Errorf("error deleting probes for gw %v: %w", gw.Gateway.Name, err) + } + } + + // Reconcile DNSHealthCheckProbes for each gateway directly referred by the policy (existing and new) for _, gw := range append(gwDiffObj.GatewaysWithValidPolicyRef, gwDiffObj.GatewaysMissingPolicyRef...) { log.V(3).Info("reconciling probes", "gateway", gw.Name) - expectedProbes := r.expectedProbesForGateway(ctx, gw, dnsPolicy) - 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) + expectedProbes := r.expectedHealthCheckProbesForGateway(ctx, gw, dnsPolicy) + if err := r.createOrUpdateHealthCheckProbes(ctx, expectedProbes); err != nil { + return fmt.Errorf("error creating or updating expected probes for gateway %v: %w", gw.Gateway.Name, err) } - if err := r.deleteUnexpectedGatewayProbes(ctx, expectedProbes, gw, dnsPolicy); err != nil { + if err := r.deleteUnexpectedGatewayHealthCheckProbes(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 { - return fmt.Errorf("error deleting probes for gw %v: %w", gw.Gateway.Name, err) - } - } - return nil } -func (r *DNSPolicyReconciler) createOrUpdateProbes(ctx context.Context, expectedProbes []*v1alpha1.DNSHealthCheckProbe) error { +func (r *DNSPolicyReconciler) createOrUpdateHealthCheckProbes(ctx context.Context, expectedProbes []*v1alpha1.DNSHealthCheckProbe) error { //create or update all expected probes for _, hcProbe := range expectedProbes { p := &v1alpha1.DNSHealthCheckProbe{} @@ -66,29 +67,49 @@ 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) deleteGatewayHealthCheckProbes(ctx context.Context, gateway *gatewayv1beta1.Gateway, dnsPolicy *v1alpha1.DNSPolicy) error { + return r.deleteHealthCheckProbesWithLabels(ctx, commonDNSRecordLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(dnsPolicy)), dnsPolicy.Namespace) +} + +func (r *DNSPolicyReconciler) deleteHealthCheckProbes(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy) error { + return r.deleteHealthCheckProbesWithLabels(ctx, policyDNSRecordLabels(client.ObjectKeyFromObject(dnsPolicy)), dnsPolicy.Namespace) +} + +func (r *DNSPolicyReconciler) deleteHealthCheckProbesWithLabels(ctx context.Context, lbls map[string]string, namespace string) error { + probes := &v1alpha1.DNSHealthCheckProbeList{} + listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls), Namespace: namespace} + 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) deleteUnexpectedGatewayHealthCheckProbes(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 } -func (r *DNSPolicyReconciler) expectedProbesForGateway(ctx context.Context, gw common.GatewayWrapper, dnsPolicy *v1alpha1.DNSPolicy) []*v1alpha1.DNSHealthCheckProbe { +func (r *DNSPolicyReconciler) expectedHealthCheckProbesForGateway(ctx context.Context, gw common.GatewayWrapper, dnsPolicy *v1alpha1.DNSPolicy) []*v1alpha1.DNSHealthCheckProbe { log := crlog.FromContext(ctx) var healthChecks []*v1alpha1.DNSHealthCheckProbe if dnsPolicy.Spec.HealthCheck == nil { @@ -126,7 +147,7 @@ func (r *DNSPolicyReconciler) expectedProbesForGateway(ctx context.Context, gw c } else { protocol = string(*dnsPolicy.Spec.HealthCheck.Protocol) } - log.V(1).Info("reconcileHealthChecks: adding health check for target", "target", address.Value) + log.V(1).Info("reconcileHealthCheckProbes: adding health check for target", "target", address.Value) healthCheck := &v1alpha1.DNSHealthCheckProbe{ ObjectMeta: metav1.ObjectMeta{ Name: dnsHealthCheckProbeName(matches[1], gw.Name, string(listener.Name)), diff --git a/pkg/controllers/dnspolicy/dnspolicy_healthchecks_test.go b/pkg/controllers/dnspolicy/dnspolicy_healthchecks_test.go index 3c676785e..8a27ee885 100644 --- a/pkg/controllers/dnspolicy/dnspolicy_healthchecks_test.go +++ b/pkg/controllers/dnspolicy/dnspolicy_healthchecks_test.go @@ -343,9 +343,9 @@ func TestDNSPolicyReconciler_expectedProbesForGateway(t *testing.T) { DNSProvider: tt.fields.DNSProvider, dnsHelper: tt.fields.dnsHelper, } - got := r.expectedProbesForGateway(tt.args.ctx, tt.args.gw, tt.args.dnsPolicy) + got := r.expectedHealthCheckProbesForGateway(tt.args.ctx, tt.args.gw, tt.args.dnsPolicy) if !reflect.DeepEqual(got, tt.want) { - t.Errorf("expectedProbesForGateway() got = %v, want %v", got, tt.want) + t.Errorf("expectedHealthCheckProbesForGateway() got = %v, want %v", got, tt.want) } }) } diff --git a/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go b/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go index c57dfcb59..5fe84d762 100644 --- a/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go +++ b/pkg/controllers/tlspolicy/tlspolicy_certmanager_certificates.go @@ -8,7 +8,7 @@ import ( certmanv1 "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" + k8serror "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/util/validation/field" @@ -25,70 +25,88 @@ import ( func (r *TLSPolicyReconciler) reconcileCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, gwDiffObj *reconcilers.GatewayDiff) error { log := crlog.FromContext(ctx) + log.V(3).Info("reconciling certificates") 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 { - return err + return fmt.Errorf("error deleting certificates for gw %v: %w", gw.Gateway.Name, 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()) - if err := r.reconcileGatewayCertificates(ctx, gw.Gateway, tlsPolicy); err != nil { - return err + log.V(1).Info("reconcileCertificates: gateway with valid or missing policy ref", "key", gw.Key()) + expectedCertificates := r.expectedCertificatesForGateway(ctx, gw.Gateway, tlsPolicy) + if err := r.createOrUpdateGatewayCertificates(ctx, expectedCertificates); err != nil { + return fmt.Errorf("error creating and updating expected certificates for gateway %v: %w", gw.Gateway.Name, err) + } + if err := r.deleteUnexpectedCertificates(ctx, expectedCertificates, gw.Gateway, tlsPolicy); err != nil { + return fmt.Errorf("error removing unexpected certificate for gateway %v: %w", gw.Gateway.Name, err) } } - return nil } -func (r *TLSPolicyReconciler) reconcileGatewayCertificates(ctx context.Context, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { - log := crlog.FromContext(ctx) - - log.V(1).Info("reconcileGatewayCertificates", "tlsPolicy", tlsPolicy) - - expectedCerts := r.expectedCertificatesForGateway(ctx, gateway, tlsPolicy) - - if err := r.deleteUnexpectedGatewayCertificates(ctx, expectedCerts, gateway, tlsPolicy); err != nil { - return err - } - - for _, cert := range expectedCerts { - err := r.ReconcileResource(ctx, &certmanv1.Certificate{}, cert, alwaysUpdateCertificate) - if err != nil && !apierrors.IsAlreadyExists(err) { - log.Error(err, "failed to reconcile Certificate resource") +func (r *TLSPolicyReconciler) createOrUpdateGatewayCertificates(ctx context.Context, expectedCertificates []*certmanv1.Certificate) error { + //create or update all expected Certificates + for _, cert := range expectedCertificates { + p := &certmanv1.Certificate{} + if err := r.Client().Get(ctx, client.ObjectKeyFromObject(cert), p); k8serror.IsNotFound(err) { + if err := r.Client().Create(ctx, cert); err != nil { + return err + } + } else if client.IgnoreNotFound(err) == nil { + p.Spec = cert.Spec + if err := r.Client().Update(ctx, p); err != nil { + return err + } + } else { return err } } - return nil } func (r *TLSPolicyReconciler) deleteGatewayCertificates(ctx context.Context, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { - return r.deleteUnexpectedGatewayCertificates(ctx, []*certmanv1.Certificate{}, gateway, tlsPolicy) + return r.deleteCertificatesWithLabels(ctx, commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)), tlsPolicy.Namespace) } -func (r *TLSPolicyReconciler) deleteUnexpectedGatewayCertificates(ctx context.Context, expectedCerts []*certmanv1.Certificate, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { - log := crlog.FromContext(ctx) +func (r *TLSPolicyReconciler) deleteCertificates(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy) error { + return r.deleteCertificatesWithLabels(ctx, policyTLSCertificateLabels(client.ObjectKeyFromObject(tlsPolicy)), tlsPolicy.Namespace) +} - listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(tlsCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)))} +func (r *TLSPolicyReconciler) deleteCertificatesWithLabels(ctx context.Context, lbls map[string]string, namespace string) error { + listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(lbls), Namespace: namespace} certList := &certmanv1.CertificateList{} if err := r.Client().List(ctx, certList, listOptions); err != nil { return err } - for _, cert := range certList.Items { - if !slice.Contains(expectedCerts, func(expectedCert *certmanv1.Certificate) bool { - return expectedCert.Name == cert.Name && expectedCert.Namespace == cert.Namespace + for _, c := range certList.Items { + if err := r.Client().Delete(ctx, &c); err != nil { + return err + } + } + return nil +} + +func (r *TLSPolicyReconciler) deleteUnexpectedCertificates(ctx context.Context, expectedCertificates []*certmanv1.Certificate, gateway *gatewayv1beta1.Gateway, tlsPolicy *v1alpha1.TLSPolicy) error { + // remove any certificates for this gateway and TLSPolicy that are no longer expected + existingCertificates := &certmanv1.CertificateList{} + dnsLabels := commonTLSCertificateLabels(client.ObjectKeyFromObject(gateway), client.ObjectKeyFromObject(tlsPolicy)) + listOptions := &client.ListOptions{LabelSelector: labels.SelectorFromSet(dnsLabels)} + if err := r.Client().List(ctx, existingCertificates, listOptions); client.IgnoreNotFound(err) != nil { + return err + } + for _, p := range existingCertificates.Items { + if !slice.Contains(expectedCertificates, func(expectedCertificate *certmanv1.Certificate) bool { + return expectedCertificate.Name == p.Name && expectedCertificate.Namespace == p.Namespace }) { - if err := r.DeleteResource(ctx, &cert); client.IgnoreNotFound(err) != nil { - log.Error(err, "failed to delete Certificate resource") + if err := r.Client().Delete(ctx, &p); err != nil { return err } } } - return nil } @@ -126,7 +144,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 +166,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..b6fb8f055 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" @@ -285,6 +286,41 @@ var _ = Describe("DNSPolicy", Ordered, func() { }, time.Second*15, time.Second).Should(BeNil()) }) + It("should not have any health check records created", func() { + // create a health check with the labels for the dnspolicy and the gateway name and namespace that would be expected in a valid target scenario + // this one should get deleted if the gateway is invalid policy ref + probe := &v1alpha1.DNSHealthCheckProbe{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s-%s-%s", TestAttachedRouteAddressTwo, TestPlacedGatewayName, TestAttachedRouteName), + Namespace: testNamespace, + Labels: map[string]string{ + DNSPolicyBackRefAnnotation: "test-dns-policy", + fmt.Sprintf("%s-namespace", DNSPolicyBackRefAnnotation): testNamespace, + LabelGatewayNSRef: testNamespace, + LabelGatewayReference: "test-gateway", + }, + }, + } + Expect(k8sClient.Create(ctx, probe)).To(BeNil()) + + Eventually(func() error { // probe should be present + getCreatedProbe := &v1alpha1.DNSHealthCheckProbe{} + err := k8sClient.Get(ctx, client.ObjectKey{Name: probe.Name, Namespace: probe.Namespace}, getCreatedProbe) + return err + }, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred()) + + Eventually(func() bool { // probe should be removed + getDeletedProbe := &v1alpha1.DNSHealthCheckProbe{} + err := k8sClient.Get(ctx, client.ObjectKey{Name: probe.Name, Namespace: probe.Namespace}, getDeletedProbe) + if err != nil { + if k8serrors.IsNotFound(err) { + return true + } + } + return false + }, TestTimeoutMedium, TestRetryIntervalMedium).Should(BeTrue()) + }) + }) Context("gateway placed", func() { @@ -340,7 +376,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 +710,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 {