From 7f5adda705251bcce65248456c01b444d9f8b58d Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Wed, 6 Nov 2024 12:41:01 +0000 Subject: [PATCH] decouple healthy from ready conditions Signed-off-by: Maskym Vavilov --- internal/controller/dnsrecord_controller.go | 9 +- .../controller/dnsrecord_healthchecks_test.go | 125 +++++++++++++++++- 2 files changed, 128 insertions(+), 6 deletions(-) diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 42808fe..a10a83b 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -442,17 +442,23 @@ func setStatusConditions(record *v1alpha1.DNSRecord, hadChanges bool, notHealthy setDNSRecordCondition(record, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, string(v1alpha1.ConditionReasonAwaitingValidation), "Awaiting validation") return } + setDNSRecordCondition(record, string(v1alpha1.ConditionTypeReady), metav1.ConditionTrue, string(v1alpha1.ConditionReasonProviderSuccess), "Provider ensured the dns record") // probes are disabled or not defined, or this is a wildcard record if record.Spec.HealthCheck == nil || strings.HasPrefix(record.Spec.RootHost, v1alpha1.WildcardPrefix) { + meta.RemoveStatusCondition(&record.Status.Conditions, string(v1alpha1.ConditionTypeHealthy)) return } + // if we haven't published because of the health failure, we won't have changes but the spec endpoints will be empty + if record.Status.Endpoints == nil || len(record.Status.Endpoints) == 0 { + setDNSRecordCondition(record, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, string(v1alpha1.ConditionReasonUnhealthy), "Not publishing unhealthy records") + } + // we don't have probes yet if cap(notHealthyProbes) == 0 { setDNSRecordCondition(record, string(v1alpha1.ConditionTypeHealthy), metav1.ConditionFalse, string(v1alpha1.ConditionReasonUnhealthy), "Probes are creating") - setDNSRecordCondition(record, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, string(v1alpha1.ConditionReasonUnhealthy), "Probes are creating") return } @@ -470,7 +476,6 @@ func setStatusConditions(record *v1alpha1.DNSRecord, hadChanges bool, notHealthy } // none of the probes is healthy (change ready condition to false) setDNSRecordCondition(record, string(v1alpha1.ConditionTypeHealthy), metav1.ConditionFalse, string(v1alpha1.ConditionReasonUnhealthy), fmt.Sprintf("Not healthy addresses: %s", notHealthyProbes)) - setDNSRecordCondition(record, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, string(v1alpha1.ConditionReasonUnhealthy), "None of the healthchecks succeeded") } diff --git a/internal/controller/dnsrecord_healthchecks_test.go b/internal/controller/dnsrecord_healthchecks_test.go index f9c0bb6..72c8b40 100644 --- a/internal/controller/dnsrecord_healthchecks_test.go +++ b/internal/controller/dnsrecord_healthchecks_test.go @@ -180,7 +180,7 @@ var _ = Describe("DNSRecordReconciler_HealthChecks", func() { "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionFalse), "Reason": Equal(string(v1alpha1.ConditionReasonUnhealthy)), - "Message": Equal("None of the healthchecks succeeded"), + "Message": Equal("Not publishing unhealthy records"), "ObservedGeneration": Equal(dnsRecord.Generation), })), ) @@ -345,12 +345,129 @@ var _ = Describe("DNSRecordReconciler_HealthChecks", func() { }), MatchFields(IgnoreExtras, Fields{ "Type": Equal(string(v1alpha1.ConditionTypeReady)), - "Status": Equal(metav1.ConditionFalse), - "Reason": Equal(string(v1alpha1.ConditionReasonUnhealthy)), - "Message": Equal("None of the healthchecks succeeded"), + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal(string(v1alpha1.ConditionReasonProviderSuccess)), + "Message": Equal("Provider ensured the dns record"), + }), + )) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + }) + + It("Should remove healthy condition when healthchecks removed", func() { + //Create default test dnsrecord + Expect(k8sClient.Create(ctx, dnsRecord)).To(Succeed()) + + By("Marking all probes as healthy") + Eventually(func(g Gomega) { + probes := &v1alpha1.DNSHealthCheckProbeList{} + + g.Expect(k8sClient.List(ctx, probes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), + }), + Namespace: dnsRecord.Namespace, + })).To(Succeed()) + g.Expect(len(probes.Items)).To(Equal(2)) + + // make probes healthy + for _, probe := range probes.Items { + probe.Status.Healthy = ptr.To(true) + probe.Status.LastCheckedAt = metav1.Now() + probe.Status.ConsecutiveFailures = 0 + g.Expect(k8sClient.Status().Update(ctx, &probe)).To(Succeed()) + } + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // make sure we published endpoint + Eventually(func(g Gomega) { + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)).To(Succeed()) + g.Expect(dnsRecord.Status.Endpoints).To(ConsistOf( + PointTo(MatchFields(IgnoreExtras, Fields{ + "DNSName": Equal(testHostname), + "Targets": ConsistOf("172.32.200.1", "172.32.200.2"), + })), + )) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + By("Making one of the probes unhealthy") + Eventually(func(g Gomega) { + probes := &v1alpha1.DNSHealthCheckProbeList{} + + g.Expect(k8sClient.List(ctx, probes, &client.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + ProbeOwnerLabel: BuildOwnerLabelValue(dnsRecord), }), + Namespace: dnsRecord.Namespace, + })).To(Succeed()) + + var updated bool + // make one of the probes unhealthy + for _, probe := range probes.Items { + if probe.Spec.Address == "172.32.200.1" { + probe.Status.Healthy = ptr.To(false) + probe.Status.LastCheckedAt = metav1.Now() + probe.Status.ConsecutiveFailures = dnsRecord.Spec.HealthCheck.FailureThreshold + 1 + g.Expect(k8sClient.Status().Update(ctx, &probe)).To(Succeed()) + updated = true + } + } + // expect to have updated one of the probes + g.Expect(updated).To(BeTrue()) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + By("Ensure unhealthy endpoints are gone and status is updated") + Eventually(func(g Gomega) { + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)).To(Succeed()) + + g.Expect(dnsRecord.Status.Endpoints).To(ConsistOf( + PointTo(MatchFields(IgnoreExtras, Fields{ + "DNSName": Equal(testHostname), + "Targets": ConsistOf("172.32.200.2"), + })), )) + g.Expect(dnsRecord.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeHealthy)), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal(string(v1alpha1.ConditionReasonPartiallyHealthy)), + "Message": Equal("Not healthy addresses: [172.32.200.1]"), + })), + ) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + By("Remove health spec") + Eventually(func(g Gomega) { + dnsRecord.Spec.HealthCheck = nil + g.Expect(k8sClient.Update(ctx, dnsRecord)).To(Succeed()) }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // we don't remove EPs if this leads to empty EPs + By("Ensure endpoints are published and a health condition is gone") + Eventually(func(g Gomega) { + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord)).To(Succeed()) + g.Expect(dnsRecord.Status.Endpoints).To(ConsistOf( + PointTo(MatchFields(IgnoreExtras, Fields{ + "DNSName": Equal(testHostname), + "Targets": ConsistOf("172.32.200.1", "172.32.200.2"), + })), + )) + g.Expect(dnsRecord.Status.Conditions).To( + And( + ContainElement( + MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeReady)), + "Status": Equal(metav1.ConditionTrue), + "Reason": Equal(string(v1alpha1.ConditionReasonProviderSuccess)), + "Message": Equal("Provider ensured the dns record"), + }), + ), + Not(ContainElement( + MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeHealthy)), + }), + )), + )) + }, time.Hour, time.Second).Should(Succeed()) }) })