Merge pull request #607 from mikenairn/361_add_target_ref_missing_status

Update policy status on TargetNotFound

pkg/_internal/conditions/conditions.go

@@ -1,6 +1,7 @@
 package conditions
 
 import (
+	"errors"
 	"fmt"
 
 	k8smeta "k8s.io/apimachinery/pkg/api/meta"
@@ -20,8 +21,12 @@ const (
 	PolicyReasonInvalid    ConditionReason = "Invalid"
 	PolicyReasonUnknown    ConditionReason = "Unknown"
 	PolicyReasonConflicted ConditionReason = "Conflicted"
+
+	PolicyReasonTargetNotFound ConditionReason = "TargetNotFound"
 )
 
+var ErrTargetNotFound = errors.New("target not found")
+
 func BuildPolicyAffectedCondition(conditionType ConditionType, policyObject runtime.Object, targetRef metav1.Object, reason ConditionReason, err error) metav1.Condition {
 
 	condition := metav1.Condition{

pkg/controllers/dnspolicy/dnspolicy_controller.go

@@ -93,8 +93,11 @@ func (r *DNSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		if !markedForDeletion {
 			if apierrors.IsNotFound(err) {
 				log.V(3).Info("Network object not found. Cleaning up")
-				err := r.deleteResources(ctx, dnsPolicy, nil)
-				return ctrl.Result{}, err
+				delResErr := r.deleteResources(ctx, dnsPolicy, nil)
+				if delResErr == nil {
+					delResErr = err
+				}
+				return r.reconcileStatus(ctx, dnsPolicy, fmt.Errorf("%w : %w", conditions.ErrTargetNotFound, delResErr))
 			}
 			return ctrl.Result{}, err
 		}
@@ -126,25 +129,13 @@ func (r *DNSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	specErr := r.reconcileResources(ctx, dnsPolicy, targetNetworkObject)
 
-	newStatus := r.calculateStatus(dnsPolicy, specErr)
-	dnsPolicy.Status = *newStatus
-
-	if !equality.Semantic.DeepEqual(previous.Status, dnsPolicy.Status) {
-		updateErr := r.Client().Status().Update(ctx, dnsPolicy)
-		if updateErr != nil {
-			// Ignore conflicts, resource might just be outdated.
-			if apierrors.IsConflict(updateErr) {
-				return ctrl.Result{Requeue: true}, nil
-			}
-			return ctrl.Result{}, updateErr
-		}
-	}
+	statusResult, statusErr := r.reconcileStatus(ctx, dnsPolicy, specErr)
 
 	if specErr != nil {
 		return ctrl.Result{}, specErr
 	}
 
-	return ctrl.Result{}, nil
+	return statusResult, statusErr
 }
 
 func (r *DNSPolicyReconciler) reconcileResources(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, targetNetworkObject client.Object) error {
@@ -237,6 +228,28 @@ func (r *DNSPolicyReconciler) deleteResources(ctx context.Context, dnsPolicy *v1
 	return r.updateGatewayCondition(ctx, metav1.Condition{Type: string(DNSPolicyAffected)}, gatewayDiffObj)
 }
 
+func (r *DNSPolicyReconciler) reconcileStatus(ctx context.Context, dnsPolicy *v1alpha1.DNSPolicy, specErr error) (ctrl.Result, error) {
+	newStatus := r.calculateStatus(dnsPolicy, specErr)
+
+	if !equality.Semantic.DeepEqual(newStatus, dnsPolicy.Status) {
+		dnsPolicy.Status = *newStatus
+		updateErr := r.Client().Status().Update(ctx, dnsPolicy)
+		if updateErr != nil {
+			// Ignore conflicts, resource might just be outdated.
+			if apierrors.IsConflict(updateErr) {
+				return ctrl.Result{Requeue: true}, nil
+			}
+			return ctrl.Result{}, updateErr
+		}
+	}
+
+	if errors.Is(specErr, conditions.ErrTargetNotFound) {
+		return ctrl.Result{Requeue: true}, nil
+	}
+
+	return ctrl.Result{}, nil
+}
+
 func (r *DNSPolicyReconciler) calculateStatus(dnsPolicy *v1alpha1.DNSPolicy, specErr error) *v1alpha1.DNSPolicyStatus {
 	newStatus := dnsPolicy.Status.DeepCopy()
 	if specErr != nil {
@@ -257,8 +270,12 @@ func (r *DNSPolicyReconciler) readyCondition(targetNetworkObjectectKind string,
 
 	if specErr != nil {
 		cond.Status = metav1.ConditionFalse
-		cond.Reason = "ReconciliationError"
 		cond.Message = specErr.Error()
+		cond.Reason = "ReconciliationError"
+
+		if errors.Is(specErr, conditions.ErrTargetNotFound) {
+			cond.Reason = string(conditions.PolicyReasonTargetNotFound)
+		}
 	}
 
 	return cond

pkg/controllers/tlspolicy/tlspolicy_controller.go

@@ -96,10 +96,12 @@ func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		if !markedForDeletion {
 			if apierrors.IsNotFound(err) {
 				log.V(3).Info("Network object not found. Cleaning up")
-				err := r.deleteResources(ctx, tlsPolicy, nil)
-				if err != nil {
-					return ctrl.Result{}, err
+				delResErr := r.deleteResources(ctx, tlsPolicy, nil)
+				if delResErr == nil {
+					delResErr = err
 				}
+				return r.reconcileStatus(ctx, tlsPolicy, fmt.Errorf("%w: %w", conditions.ErrTargetNotFound, delResErr))
+
 			}
 			return ctrl.Result{}, err
 		}
@@ -128,25 +130,13 @@ func (r *TLSPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	specErr := r.reconcileResources(ctx, tlsPolicy, targetNetworkObject)
 
-	newStatus := r.calculateStatus(tlsPolicy, specErr)
-	tlsPolicy.Status = *newStatus
-
-	if !equality.Semantic.DeepEqual(previous.Status, tlsPolicy.Status) {
-		updateErr := r.Client().Status().Update(ctx, tlsPolicy)
-		if updateErr != nil {
-			// Ignore conflicts, resource might just be outdated.
-			if apierrors.IsConflict(updateErr) {
-				return ctrl.Result{Requeue: true}, nil
-			}
-			return ctrl.Result{}, updateErr
-		}
-	}
+	statusResult, statusErr := r.reconcileStatus(ctx, tlsPolicy, specErr)
 
 	if specErr != nil {
 		return ctrl.Result{}, specErr
 	}
 
-	return ctrl.Result{}, nil
+	return statusResult, statusErr
 }
 
 func (r *TLSPolicyReconciler) reconcileResources(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, targetNetworkObject client.Object) error {
@@ -226,6 +216,28 @@ func (r *TLSPolicyReconciler) deleteResources(ctx context.Context, tlsPolicy *v1
 	return r.updateGatewayCondition(ctx, metav1.Condition{Type: string(TLSPolicyAffected)}, gatewayDiffObj)
 }
 
+func (r *TLSPolicyReconciler) reconcileStatus(ctx context.Context, tlsPolicy *v1alpha1.TLSPolicy, specErr error) (ctrl.Result, error) {
+	newStatus := r.calculateStatus(tlsPolicy, specErr)
+
+	if !equality.Semantic.DeepEqual(newStatus, tlsPolicy.Status) {
+		tlsPolicy.Status = *newStatus
+		updateErr := r.Client().Status().Update(ctx, tlsPolicy)
+		if updateErr != nil {
+			// Ignore conflicts, resource might just be outdated.
+			if apierrors.IsConflict(updateErr) {
+				return ctrl.Result{Requeue: true}, nil
+			}
+			return ctrl.Result{}, updateErr
+		}
+	}
+
+	if errors.Is(specErr, conditions.ErrTargetNotFound) {
+		return ctrl.Result{Requeue: true}, nil
+	}
+
+	return ctrl.Result{}, nil
+}
+
 func (r *TLSPolicyReconciler) calculateStatus(tlsPolicy *v1alpha1.TLSPolicy, specErr error) *v1alpha1.TLSPolicyStatus {
 	newStatus := tlsPolicy.Status.DeepCopy()
 	if specErr != nil {
@@ -246,8 +258,12 @@ func (r *TLSPolicyReconciler) readyCondition(targetNetworkObjectectKind string,
 
 	if specErr != nil {
 		cond.Status = metav1.ConditionFalse
-		cond.Reason = "ReconciliationError"
 		cond.Message = specErr.Error()
+		cond.Reason = "ReconciliationError"
+
+		if errors.Is(specErr, conditions.ErrTargetNotFound) {
+			cond.Reason = string(conditions.PolicyReasonTargetNotFound)
+		}
 	}
 
 	return cond

test/integration/dnspolicy_controller_test.go

@@ -25,6 +25,7 @@ import (
 	. "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/dnspolicy"
 	mgcgateway "github.com/Kuadrant/multicluster-gateway-controller/pkg/controllers/gateway"
 	"github.com/Kuadrant/multicluster-gateway-controller/pkg/dns"
+	. "github.com/Kuadrant/multicluster-gateway-controller/test/util"
 )
 
 func testBuildManagedZone(domainName, ns string) *v1alpha1.ManagedZone {
@@ -194,6 +195,69 @@ var _ = Describe("DNSPolicy", Ordered, func() {
 		Expect(err).ToNot(HaveOccurred())
 	})
 
+	Context("invalid target", func() {
+		var gateway *gatewayv1beta1.Gateway
+		var dnsPolicy *v1alpha1.DNSPolicy
+		gwClassName := "istio"
+
+		BeforeEach(func() {
+			dnsPolicy = testBuildDNSPolicyWithHealthCheck("test-dns-policy", "test-gateway", testNamespace, nil)
+			Expect(k8sClient.Create(ctx, dnsPolicy)).To(BeNil())
+			Eventually(func() error { //dns policy exists
+				return k8sClient.Get(ctx, client.ObjectKey{Name: dnsPolicy.Name, Namespace: dnsPolicy.Namespace}, dnsPolicy)
+			}, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred())
+		})
+
+		AfterEach(func() {
+			err := k8sClient.Delete(ctx, dnsPolicy)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("should have ready condition with status false and correct reason", func() {
+			Eventually(func() error {
+				if err := k8sClient.Get(ctx, client.ObjectKey{Name: dnsPolicy.Name, Namespace: dnsPolicy.Namespace}, dnsPolicy); err != nil {
+					return err
+				}
+				readyCond := meta.FindStatusCondition(dnsPolicy.Status.Conditions, string(conditions.ConditionTypeReady))
+				if readyCond == nil {
+					return fmt.Errorf("expected dnsPolicy to have %s condition, got none",
+						string(conditions.ConditionTypeReady))
+				}
+				if readyCond.Status != metav1.ConditionFalse {
+					return fmt.Errorf("expected dnsPolicy %s condition to have status %s, got %s",
+						string(conditions.ConditionTypeReady), metav1.ConditionFalse, readyCond.Status)
+				}
+				if readyCond.Reason != string(conditions.PolicyReasonTargetNotFound) {
+					return fmt.Errorf("expected dnsPolicy %s condition to have reason %s, got %s",
+						string(conditions.ConditionTypeReady), string(conditions.PolicyReasonTargetNotFound), readyCond.Reason)
+				}
+				return nil
+			}, time.Second*15, time.Second).Should(BeNil())
+		})
+
+		It("should have ready condition with status true", func() {
+			By("creating a valid Gateway")
+
+			gateway = NewTestGateway("test-gateway", gwClassName, testNamespace).
+				WithHTTPListener("test.example.com").Gateway
+			Expect(k8sClient.Create(ctx, gateway)).To(BeNil())
+			Eventually(func() error { //gateway exists
+				return k8sClient.Get(ctx, client.ObjectKey{Name: gateway.Name, Namespace: gateway.Namespace}, gateway)
+			}, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred())
+
+			Eventually(func() error {
+				if err := k8sClient.Get(ctx, client.ObjectKey{Name: dnsPolicy.Name, Namespace: dnsPolicy.Namespace}, dnsPolicy); err != nil {
+					return err
+				}
+				if !meta.IsStatusConditionTrue(dnsPolicy.Status.Conditions, string(conditions.ConditionTypeReady)) {
+					return fmt.Errorf("expected dnsPolicy %s condition to have status %s ", string(conditions.ConditionTypeReady), metav1.ConditionTrue)
+				}
+				return nil
+			}, time.Second*15, time.Second).Should(BeNil())
+		})
+
+	})
+
 	Context("gateway placed", func() {
 		var gateway *gatewayv1beta1.Gateway
 		var lbHash, dnsRecordName, wildcardDNSRecordName string
@@ -1330,9 +1394,11 @@ var _ = Describe("DNSPolicy", Ordered, func() {
 				}
 
 				Eventually(func() error {
+					err = k8sClient.Get(ctx, client.ObjectKey{Name: gateway.Name, Namespace: gateway.Namespace}, gateway)
+					Expect(err).NotTo(HaveOccurred())
 					gateway.Spec.Listeners = append(gateway.Spec.Listeners, otherListener)
 					return k8sClient.Update(ctx, gateway)
-				}).Should(BeNil())
+				}, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred())
 
 				probeList := &v1alpha1.DNSHealthCheckProbeList{}
 				Eventually(func() error {

test/integration/tlspolicy_controller_test.go

@@ -12,6 +12,7 @@ import (
 	. "github.com/onsi/gomega"
 
 	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -73,6 +74,71 @@ var _ = Describe("TLSPolicy", Ordered, func() {
 		Expect(err).ToNot(HaveOccurred())
 	})
 
+	Context("invalid target", func() {
+		var gateway *gatewayv1beta1.Gateway
+		var tlsPolicy *v1alpha1.TLSPolicy
+		gwClassName := "istio"
+
+		BeforeEach(func() {
+			tlsPolicy = NewTestTLSPolicy("test-tls-policy", testNamespace).
+				WithTargetGateway("test-gateway").
+				WithIssuer("testissuer", certmanv1.IssuerKind, "cert-manager.io").TLSPolicy
+			Expect(k8sClient.Create(ctx, tlsPolicy)).To(BeNil())
+			Eventually(func() error { //tls policy exists
+				return k8sClient.Get(ctx, client.ObjectKey{Name: tlsPolicy.Name, Namespace: tlsPolicy.Namespace}, tlsPolicy)
+			}, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred())
+		})
+
+		AfterEach(func() {
+			err := k8sClient.Delete(ctx, tlsPolicy)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("should have ready condition with status false and correct reason", func() {
+			Eventually(func() error {
+				if err := k8sClient.Get(ctx, client.ObjectKey{Name: tlsPolicy.Name, Namespace: tlsPolicy.Namespace}, tlsPolicy); err != nil {
+					return err
+				}
+				readyCond := meta.FindStatusCondition(tlsPolicy.Status.Conditions, string(conditions.ConditionTypeReady))
+				if readyCond == nil {
+					return fmt.Errorf("expected tlsPolicy to have %s condition, got none",
+						string(conditions.ConditionTypeReady))
+				}
+				if readyCond.Status != metav1.ConditionFalse {
+					return fmt.Errorf("expected tlsPolicy %s condition to have status %s, got %s",
+						string(conditions.ConditionTypeReady), metav1.ConditionFalse, readyCond.Status)
+				}
+				if readyCond.Reason != string(conditions.PolicyReasonTargetNotFound) {
+					return fmt.Errorf("expected tlsPolicy %s condition to have reason %s, got %s",
+						string(conditions.ConditionTypeReady), string(conditions.PolicyReasonTargetNotFound), readyCond.Reason)
+				}
+				return nil
+			}, time.Second*15, time.Second).Should(BeNil())
+		})
+
+		It("should have ready condition with status true", func() {
+			By("creating a valid Gateway")
+
+			gateway = NewTestGateway("test-gateway", gwClassName, testNamespace).
+				WithHTTPListener("test.example.com").Gateway
+			Expect(k8sClient.Create(ctx, gateway)).To(BeNil())
+			Eventually(func() error { //gateway exists
+				return k8sClient.Get(ctx, client.ObjectKey{Name: gateway.Name, Namespace: gateway.Namespace}, gateway)
+			}, TestTimeoutMedium, TestRetryIntervalMedium).ShouldNot(HaveOccurred())
+
+			Eventually(func() error {
+				if err := k8sClient.Get(ctx, client.ObjectKey{Name: tlsPolicy.Name, Namespace: tlsPolicy.Namespace}, tlsPolicy); err != nil {
+					return err
+				}
+				if !meta.IsStatusConditionTrue(tlsPolicy.Status.Conditions, string(conditions.ConditionTypeReady)) {
+					return fmt.Errorf("expected tlsPolicy %s condition to have status %s ", string(conditions.ConditionTypeReady), metav1.ConditionTrue)
+				}
+				return nil
+			}, time.Second*15, time.Second).Should(BeNil())
+		})
+
+	})
+
 	Context("istio gateway", func() {
 		var gateway *gatewayv1beta1.Gateway
 		var tlsPolicy *v1alpha1.TLSPolicy