From 6e65298c83f69e44ab32b1dd76134fef42296f67 Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Wed, 27 Mar 2024 12:28:41 +0000 Subject: [PATCH 1/3] GH-36 dns record lifecycle --- .../conditions => api/v1alpha1}/conditions.go | 2 +- api/v1alpha1/dnsrecord_types.go | 13 +++ api/v1alpha1/zz_generated.deepcopy.go | 2 + bundle/manifests/kuadrant.io_dnsrecords.yaml | 20 ++++ cmd/main.go | 16 ++- config/crd/bases/kuadrant.io_dnsrecords.yaml | 20 ++++ internal/controller/dnsrecord_controller.go | 109 ++++++++++++------ .../controller/dnsrecord_controller_test.go | 5 +- internal/controller/managedzone_controller.go | 7 +- internal/controller/suite_test.go | 7 +- test/e2e/single_cluster_record_test.go | 9 +- 11 files changed, 157 insertions(+), 53 deletions(-) rename {internal/common/conditions => api/v1alpha1}/conditions.go (84%) diff --git a/internal/common/conditions/conditions.go b/api/v1alpha1/conditions.go similarity index 84% rename from internal/common/conditions/conditions.go rename to api/v1alpha1/conditions.go index 2a0b11d2..db246752 100644 --- a/internal/common/conditions/conditions.go +++ b/api/v1alpha1/conditions.go @@ -1,4 +1,4 @@ -package conditions +package v1alpha1 type ConditionType string type ConditionReason string diff --git a/api/v1alpha1/dnsrecord_types.go b/api/v1alpha1/dnsrecord_types.go index e07baf68..e57109d1 100644 --- a/api/v1alpha1/dnsrecord_types.go +++ b/api/v1alpha1/dnsrecord_types.go @@ -87,6 +87,19 @@ type DNSRecordStatus struct { // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // QueuedAt is a time when DNS record was received for the reconciliation + QueuedAt metav1.Time `json:"queuedAt,omitempty"` + + // QueuedFor is a time when we expect a DNS record to be reconciled again + QueuedFor metav1.Time `json:"queuedFor,omitempty"` + + // ValidFor indicates duration since the last reconciliation we consider data in the record to be valid + ValidFor string `json:"validFor,omitempty"` + + // WriteCounter represent a number of consecutive write attempts on the same generation of the record. + // It is being reset to 0 when the generation changes or there are no changes to write. + WriteCounter int64 `json:"writeCounter,omitempty"` + // endpoints are the last endpoints that were successfully published by the provider // // Provides a simple mechanism to store the current provider records in order to diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index a61edb73..0c53b82e 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -142,6 +142,8 @@ func (in *DNSRecordStatus) DeepCopyInto(out *DNSRecordStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + in.QueuedAt.DeepCopyInto(&out.QueuedAt) + in.QueuedFor.DeepCopyInto(&out.QueuedFor) if in.Endpoints != nil { in, out := &in.Endpoints, &out.Endpoints *out = make([]*endpoint.Endpoint, len(*in)) diff --git a/bundle/manifests/kuadrant.io_dnsrecords.yaml b/bundle/manifests/kuadrant.io_dnsrecords.yaml index 8129038e..6cf5b6a7 100644 --- a/bundle/manifests/kuadrant.io_dnsrecords.yaml +++ b/bundle/manifests/kuadrant.io_dnsrecords.yaml @@ -330,6 +330,26 @@ spec: it needs to retry the update for that specific zone. format: int64 type: integer + queuedAt: + description: QueuedAt is a time when DNS record was received for the + reconciliation + format: date-time + type: string + queuedFor: + description: QueuedFor is a time when we expect a DNS record to be + reconciled again + format: date-time + type: string + validFor: + description: ValidFor indicates duration since the last reconciliation + we consider data in the record to be valid + type: string + writeCounter: + description: WriteCounter represent a number of consecutive write + attempts on the same generation of the record. It is being reset + to 0 when the generation changes or there are no changes to write. + format: int64 + type: integer type: object type: object served: true diff --git a/cmd/main.go b/cmd/main.go index 208ed3bf..c5e1fcf3 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -19,6 +19,7 @@ package main import ( "flag" "os" + "time" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -43,6 +44,11 @@ var ( setupLog = ctrl.Log.WithName("setup") ) +const ( + RequeueDuration = time.Minute * 15 + ValidityDuration = time.Minute * 14 +) + func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) @@ -54,11 +60,19 @@ func main() { var metricsAddr string var enableLeaderElection bool var probeAddr string + var requeueTime time.Duration + var validFor time.Duration flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") flag.BoolVar(&enableLeaderElection, "leader-elect", false, "Enable leader election for controller manager. "+ "Enabling this will ensure there is only one active controller manager.") + flag.DurationVar(&requeueTime, "requeue-time", RequeueDuration, + "Duration for the validation reconciliation loop. "+ + "Controls how ofter record is reconciled") + flag.DurationVar(&validFor, "valid-for", ValidityDuration, + "Duration when the record is considered to hold valid information"+ + "Controls if we commit to the full reconcile loop") opts := zap.Options{ Development: true, } @@ -94,7 +108,7 @@ func main() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), ProviderFactory: providerFactory, - }).SetupWithManager(mgr); err != nil { + }).SetupWithManager(mgr, requeueTime, validFor); err != nil { setupLog.Error(err, "unable to create controller", "controller", "DNSRecord") os.Exit(1) } diff --git a/config/crd/bases/kuadrant.io_dnsrecords.yaml b/config/crd/bases/kuadrant.io_dnsrecords.yaml index 762b0bec..e186b2c1 100644 --- a/config/crd/bases/kuadrant.io_dnsrecords.yaml +++ b/config/crd/bases/kuadrant.io_dnsrecords.yaml @@ -330,6 +330,26 @@ spec: it needs to retry the update for that specific zone. format: int64 type: integer + queuedAt: + description: QueuedAt is a time when DNS record was received for the + reconciliation + format: date-time + type: string + queuedFor: + description: QueuedFor is a time when we expect a DNS record to be + reconciled again + format: date-time + type: string + validFor: + description: ValidFor indicates duration since the last reconciliation + we consider data in the record to be valid + type: string + writeCounter: + description: WriteCounter represent a number of consecutive write + attempts on the same generation of the record. It is being reset + to 0 when the generation changes or there are no changes to write. + format: int64 + type: integer type: object type: object served: true diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 6976511a..5c8e90a7 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -18,8 +18,10 @@ package controller import ( "context" + "errors" "fmt" "strings" + "time" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -36,7 +38,6 @@ import ( externaldnsprovider "sigs.k8s.io/external-dns/provider" "github.com/kuadrant/dns-operator/api/v1alpha1" - "github.com/kuadrant/dns-operator/internal/common/conditions" externaldnsplan "github.com/kuadrant/dns-operator/internal/external-dns/plan" "github.com/kuadrant/dns-operator/internal/provider" ) @@ -45,7 +46,13 @@ const ( DNSRecordFinalizer = "kuadrant.io/dns-record" ) -var Clock clock.Clock = clock.RealClock{} +var ( + defaultRequeueTime time.Duration + validationRequeueTime = time.Second * 5 + validFor time.Duration + reconcileStart = metav1.Time{} + Clock clock.Clock = clock.RealClock{} +) // DNSRecordReconciler reconciles a DNSRecord object type DNSRecordReconciler struct { @@ -61,19 +68,21 @@ type DNSRecordReconciler struct { func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { logger := log.FromContext(ctx) + reconcileStart = metav1.Now() previous := &v1alpha1.DNSRecord{} err := r.Client.Get(ctx, client.ObjectKey{Namespace: req.Namespace, Name: req.Name}, previous) if err != nil { - if err := client.IgnoreNotFound(err); err == nil { + if err = client.IgnoreNotFound(err); err == nil { return ctrl.Result{}, nil } else { return ctrl.Result{}, err } } dnsRecord := previous.DeepCopy() + dnsRecord.Status.QueuedAt = reconcileStart if dnsRecord.DeletionTimestamp != nil && !dnsRecord.DeletionTimestamp.IsZero() { - if err := r.deleteRecord(ctx, dnsRecord); err != nil { + if err = r.deleteRecord(ctx, dnsRecord); err != nil { logger.Error(err, "Failed to delete DNSRecord") return ctrl.Result{}, err } @@ -89,44 +98,46 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } if !controllerutil.ContainsFinalizer(dnsRecord, DNSRecordFinalizer) { + dnsRecord.Status.QueuedFor = metav1.NewTime(reconcileStart.Add(validationRequeueTime)) logger.Info("Adding Finalizer", "name", DNSRecordFinalizer) controllerutil.AddFinalizer(dnsRecord, DNSRecordFinalizer) err = r.Update(ctx, dnsRecord) if err != nil { return ctrl.Result{}, err } - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{RequeueAfter: validationRequeueTime}, nil } var reason, message string - status := metav1.ConditionTrue - reason = "ProviderSuccess" - message = "Provider ensured the dns record" err = dnsRecord.Validate() if err != nil { - status = metav1.ConditionFalse reason = "ValidationError" message = fmt.Sprintf("validation of DNSRecord failed: %v", err) - setDNSRecordCondition(dnsRecord, string(conditions.ConditionTypeReady), status, reason, message) - return r.updateStatus(ctx, previous, dnsRecord) + setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, reason, message) + return r.updateStatus(ctx, previous, dnsRecord, "0s") } + // Publish the record - err = r.publishRecord(ctx, dnsRecord) + requeueAfter, err := r.publishRecord(ctx, dnsRecord) if err != nil { - status = metav1.ConditionFalse reason = "ProviderError" message = fmt.Sprintf("The DNS provider failed to ensure the record: %v", provider.SanitizeError(err)) - setDNSRecordCondition(dnsRecord, string(conditions.ConditionTypeReady), status, reason, message) - return r.updateStatus(ctx, previous, dnsRecord) + setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, reason, message) + return r.updateStatus(ctx, previous, dnsRecord, "0s") } // success - setDNSRecordCondition(dnsRecord, string(conditions.ConditionTypeReady), status, reason, message) dnsRecord.Status.ObservedGeneration = dnsRecord.Generation dnsRecord.Status.Endpoints = dnsRecord.Spec.Endpoints - return r.updateStatus(ctx, previous, dnsRecord) + return r.updateStatus(ctx, previous, dnsRecord, requeueAfter) } -func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, current *v1alpha1.DNSRecord) (reconcile.Result, error) { +func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, current *v1alpha1.DNSRecord, requeueAfter string) (reconcile.Result, error) { + requeueDuration, err := time.ParseDuration(requeueAfter) + if err != nil { + return ctrl.Result{}, errors.New("error parsing duration while setting requeue time") + } + current.Status.QueuedFor = metav1.NewTime(reconcileStart.Add(requeueDuration)) + if !equality.Semantic.DeepEqual(previous.Status, current.Status) { updateError := r.Status().Update(ctx, current) if apierrors.IsConflict(updateError) { @@ -134,11 +145,14 @@ func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, curren } return ctrl.Result{}, updateError } - return ctrl.Result{}, nil + return ctrl.Result{RequeueAfter: requeueDuration}, nil } // SetupWithManager sets up the controller with the Manager. -func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { +func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager, requeueIn, valid time.Duration) error { + defaultRequeueTime = requeueIn + validFor = valid + return ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.DNSRecord{}). Complete(r) @@ -166,7 +180,7 @@ func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alp return fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name) } - err = r.applyChanges(ctx, dnsRecord, managedZone, true) + _, err = r.applyChanges(ctx, dnsRecord, managedZone, true) if err != nil { if strings.Contains(err.Error(), "was not found") || strings.Contains(err.Error(), "notFound") { logger.Info("Record not found in managed zone, continuing", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) @@ -184,7 +198,7 @@ func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alp // publishRecord publishes record(s) to the DNSPRovider(i.e. route53) configured by the ManagedZone assigned to this // DNSRecord (dnsRecord.Status.ParentManagedZone). -func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) error { +func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) (string, error) { logger := log.FromContext(ctx) managedZone := &v1alpha1.ManagedZone{ ObjectMeta: metav1.ObjectMeta{ @@ -194,26 +208,35 @@ func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1al } err := r.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone, &client.GetOptions{}) if err != nil { - return err + return "0s", err } managedZoneReady := meta.IsStatusConditionTrue(managedZone.Status.Conditions, "Ready") if !managedZoneReady { - return fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name) + return "0s", fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name) } - if dnsRecord.Generation == dnsRecord.Status.ObservedGeneration { + // cut off here for the short reconcile loop + expiryTime := metav1.NewTime(dnsRecord.Status.QueuedAt.Add(validFor)) + if !generationChanged(dnsRecord) && reconcileStart.Before(&expiryTime) { logger.V(3).Info("Skipping managed zone to which the DNS dnsRecord is already published", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) - return nil + return validFor.String(), nil + } + if generationChanged(dnsRecord) { + dnsRecord.Status.WriteCounter = 0 } - err = r.applyChanges(ctx, dnsRecord, managedZone, false) + requeueAfter, err := r.applyChanges(ctx, dnsRecord, managedZone, false) if err != nil { - return err + return "0s", err } logger.Info("Published DNSRecord to manage zone", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) - return nil + return requeueAfter, nil +} + +func generationChanged(record *v1alpha1.DNSRecord) bool { + return record.Generation != record.Status.ObservedGeneration } // setDNSRecordCondition adds or updates a given condition in the DNSRecord status.. @@ -228,7 +251,7 @@ func setDNSRecordCondition(dnsRecord *v1alpha1.DNSRecord, conditionType string, meta.SetStatusCondition(&dnsRecord.Status.Conditions, cond) } -func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alpha1.DNSRecord, managedZone *v1alpha1.ManagedZone, isDelete bool) error { +func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alpha1.DNSRecord, managedZone *v1alpha1.ManagedZone, isDelete bool) (string, error) { logger := log.FromContext(ctx) filterDomain, _ := strings.CutPrefix(managedZone.Spec.DomainName, v1alpha1.WildcardPrefix) if dnsRecord.Spec.RootHost != nil { @@ -244,7 +267,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp logger.V(3).Info("applyChanges", "zone", managedZone.Spec.DomainName, "rootDomainFilter", rootDomainFilter, "providerConfig", providerConfig) dnsProvider, err := r.ProviderFactory.ProviderFor(ctx, managedZone, providerConfig) if err != nil { - return err + return "0s", err } managedDNSRecordTypes := []string{externaldnsendpoint.RecordTypeA, externaldnsendpoint.RecordTypeAAAA, externaldnsendpoint.RecordTypeCNAME} @@ -252,13 +275,13 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp registry, err := dnsRecord.GetRegistry(dnsProvider, managedDNSRecordTypes, excludeDNSRecordTypes) if err != nil { - return err + return "0s", err } policyID := "sync" policy, exists := externaldnsplan.Policies[policyID] if !exists { - return fmt.Errorf("unknown policy: %s", policyID) + return "0s", fmt.Errorf("unknown policy: %s", policyID) } //If we are deleting set the expected endpoints to an empty array @@ -269,19 +292,19 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp //zoneEndpoints = Records in the current dns provider zone zoneEndpoints, err := registry.Records(ctx) if err != nil { - return err + return "0s", err } //specEndpoints = Records that this DNSRecord expects to exist specEndpoints, err := registry.AdjustEndpoints(dnsRecord.Spec.Endpoints) if err != nil { - return fmt.Errorf("adjusting specEndpoints: %w", err) + return "0s", fmt.Errorf("adjusting specEndpoints: %w", err) } //statusEndpoints = Records that were created/updated by this DNSRecord last statusEndpoints, err := registry.AdjustEndpoints(dnsRecord.Status.Endpoints) if err != nil { - return fmt.Errorf("adjusting statusEndpoints: %w", err) + return "0s", fmt.Errorf("adjusting statusEndpoints: %w", err) } //Note: All endpoint lists should be in the same provider specific format at this point @@ -302,15 +325,25 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp plan = plan.Calculate() + dnsRecord.Status.ValidFor = defaultRequeueTime.String() if plan.Changes.HasChanges() { + // generation has not changed but there are changes. + // implies that they were overridden - bump write counter + if !generationChanged(dnsRecord) { + dnsRecord.Status.WriteCounter++ + } + dnsRecord.Status.ValidFor = validationRequeueTime.String() + setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, "Awaiting validation", "Awaiting validation") logger.Info("Applying changes") err = registry.ApplyChanges(ctx, plan.Changes) if err != nil { - return err + return dnsRecord.Status.ValidFor, err } } else { logger.Info("All records are already up to date") + dnsRecord.Status.WriteCounter = 0 + setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionTrue, "ProviderSuccess", "Provider ensured the dns record") } - return nil + return dnsRecord.Status.ValidFor, nil } diff --git a/internal/controller/dnsrecord_controller_test.go b/internal/controller/dnsrecord_controller_test.go index df6c0e30..2505db25 100644 --- a/internal/controller/dnsrecord_controller_test.go +++ b/internal/controller/dnsrecord_controller_test.go @@ -31,7 +31,6 @@ import ( externaldnsendpoint "sigs.k8s.io/external-dns/endpoint" "github.com/kuadrant/dns-operator/api/v1alpha1" - "github.com/kuadrant/dns-operator/internal/common/conditions" ) var _ = Describe("DNSRecordReconciler", func() { @@ -89,7 +88,7 @@ var _ = Describe("DNSRecordReconciler", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), "Reason": Equal("ProviderSuccess"), "Message": Equal("Provider ensured the dns record"), @@ -108,7 +107,7 @@ var _ = Describe("DNSRecordReconciler", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), "Reason": Equal("ProviderSuccess"), "Message": Equal("Provider ensured the dns record"), diff --git a/internal/controller/managedzone_controller.go b/internal/controller/managedzone_controller.go index 673c18f3..f0ec0dfe 100644 --- a/internal/controller/managedzone_controller.go +++ b/internal/controller/managedzone_controller.go @@ -32,7 +32,6 @@ import ( externaldns "sigs.k8s.io/external-dns/endpoint" "github.com/kuadrant/dns-operator/api/v1alpha1" - "github.com/kuadrant/dns-operator/internal/common/conditions" "github.com/kuadrant/dns-operator/internal/provider" ) @@ -144,7 +143,7 @@ func (r *ManagedZoneReconciler) Reconcile(ctx context.Context, req ctrl.Request) } managedZone.Status.ObservedGeneration = managedZone.Generation - setManagedZoneCondition(managedZone, string(conditions.ConditionTypeReady), status, reason, message) + setManagedZoneCondition(managedZone, string(v1alpha1.ConditionTypeReady), status, reason, message) err = r.Status().Update(ctx, managedZone) if err != nil { return ctrl.Result{}, err @@ -192,7 +191,7 @@ func (r *ManagedZoneReconciler) deleteManagedZone(ctx context.Context, managedZo reason = "ProviderError" message = fmt.Sprintf("The DNS provider creation failed: %v", err) managedZone.Status.ObservedGeneration = managedZone.Generation - setManagedZoneCondition(managedZone, string(conditions.ConditionTypeReady), status, reason, message) + setManagedZoneCondition(managedZone, string(v1alpha1.ConditionTypeReady), status, reason, message) return err } err = dnsProvider.DeleteManagedZone(managedZone) @@ -336,7 +335,7 @@ func (r *ManagedZoneReconciler) parentZoneNSRecordReady(ctx context.Context, man } } - nsRecordReady := meta.IsStatusConditionTrue(nsRecord.Status.Conditions, string(conditions.ConditionTypeReady)) + nsRecordReady := meta.IsStatusConditionTrue(nsRecord.Status.Conditions, string(v1alpha1.ConditionTypeReady)) if !nsRecordReady { return fmt.Errorf("the ns record is not in a ready state : %s", nsRecord.Name) } diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index f4c34b64..c2814359 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -53,6 +53,11 @@ import ( // These tests use Ginkgo (BDD-style Go testing framework). Refer to // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. +const ( + RequeueDuration = time.Minute * 1 + ValidityDuration = time.Second * 30 +) + var cfg *rest.Config var k8sClient client.Client var testEnv *envtest.Environment @@ -132,7 +137,7 @@ var _ = BeforeSuite(func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), ProviderFactory: dnsProviderFactory, - }).SetupWithManager(mgr) + }).SetupWithManager(mgr, RequeueDuration, ValidityDuration) Expect(err).ToNot(HaveOccurred()) go func() { diff --git a/test/e2e/single_cluster_record_test.go b/test/e2e/single_cluster_record_test.go index 9506ad3b..5d3cdbca 100644 --- a/test/e2e/single_cluster_record_test.go +++ b/test/e2e/single_cluster_record_test.go @@ -17,7 +17,6 @@ import ( externaldnsendpoint "sigs.k8s.io/external-dns/endpoint" "github.com/kuadrant/dns-operator/api/v1alpha1" - "github.com/kuadrant/dns-operator/internal/common/conditions" ) var _ = Describe("Single Cluster Record Test", func() { @@ -86,7 +85,7 @@ var _ = Describe("Single Cluster Record Test", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), })), ) @@ -210,7 +209,7 @@ var _ = Describe("Single Cluster Record Test", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), })), ) @@ -362,7 +361,7 @@ var _ = Describe("Single Cluster Record Test", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), })), ) @@ -495,7 +494,7 @@ var _ = Describe("Single Cluster Record Test", func() { g.Expect(err).NotTo(HaveOccurred()) g.Expect(dnsRecord.Status.Conditions).To( ContainElement(MatchFields(IgnoreExtras, Fields{ - "Type": Equal(string(conditions.ConditionTypeReady)), + "Type": Equal(string(v1alpha1.ConditionTypeReady)), "Status": Equal(metav1.ConditionTrue), })), ) From 16839e4ee05a6697a03659e60f5c3233d52fcf74 Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Thu, 4 Apr 2024 11:40:16 +0100 Subject: [PATCH 2/3] GH-36 tests update --- internal/controller/dnsrecord_controller.go | 14 +++--- .../controller/dnsrecord_controller_test.go | 43 +++++++++++++------ internal/controller/helper_test.go | 35 +++++++++++++++ internal/controller/suite_test.go | 7 +-- test/e2e/single_cluster_record_test.go | 8 ++-- 5 files changed, 78 insertions(+), 29 deletions(-) diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 5c8e90a7..30077a13 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -79,7 +79,6 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } dnsRecord := previous.DeepCopy() - dnsRecord.Status.QueuedAt = reconcileStart if dnsRecord.DeletionTimestamp != nil && !dnsRecord.DeletionTimestamp.IsZero() { if err = r.deleteRecord(ctx, dnsRecord); err != nil { @@ -217,10 +216,14 @@ func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1al } // cut off here for the short reconcile loop - expiryTime := metav1.NewTime(dnsRecord.Status.QueuedAt.Add(validFor)) + requeueIn := validFor + if dnsRecord.Status.ValidFor != "" { + requeueIn, _ = time.ParseDuration(dnsRecord.Status.ValidFor) + } + expiryTime := metav1.NewTime(dnsRecord.Status.QueuedAt.Add(requeueIn)) if !generationChanged(dnsRecord) && reconcileStart.Before(&expiryTime) { - logger.V(3).Info("Skipping managed zone to which the DNS dnsRecord is already published", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) - return validFor.String(), nil + logger.V(3).Info("Skipping managed zone to which the DNS dnsRecord is already published and is still valid", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) + return requeueIn.String(), nil } if generationChanged(dnsRecord) { dnsRecord.Status.WriteCounter = 0 @@ -326,6 +329,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp plan = plan.Calculate() dnsRecord.Status.ValidFor = defaultRequeueTime.String() + dnsRecord.Status.QueuedAt = reconcileStart if plan.Changes.HasChanges() { // generation has not changed but there are changes. // implies that they were overridden - bump write counter @@ -333,7 +337,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp dnsRecord.Status.WriteCounter++ } dnsRecord.Status.ValidFor = validationRequeueTime.String() - setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, "Awaiting validation", "Awaiting validation") + setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, "AwaitingValidation", "Awaiting validation") logger.Info("Applying changes") err = registry.ApplyChanges(ctx, plan.Changes) if err != nil { diff --git a/internal/controller/dnsrecord_controller_test.go b/internal/controller/dnsrecord_controller_test.go index 2505db25..00dd1af7 100644 --- a/internal/controller/dnsrecord_controller_test.go +++ b/internal/controller/dnsrecord_controller_test.go @@ -28,7 +28,6 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" - externaldnsendpoint "sigs.k8s.io/external-dns/endpoint" "github.com/kuadrant/dns-operator/api/v1alpha1" ) @@ -53,19 +52,7 @@ var _ = Describe("DNSRecordReconciler", func() { ManagedZoneRef: &v1alpha1.ManagedZoneReference{ Name: managedZone.Name, }, - Endpoints: []*externaldnsendpoint.Endpoint{ - { - DNSName: "foo.example.com", - Targets: []string{ - "127.0.0.1", - }, - RecordType: "A", - SetIdentifier: "", - RecordTTL: 60, - Labels: nil, - ProviderSpecific: nil, - }, - }, + Endpoints: getTestEndpoints(), }, } }) @@ -96,6 +83,7 @@ var _ = Describe("DNSRecordReconciler", func() { })), ) g.Expect(dnsRecord.Finalizers).To(ContainElement(DNSRecordFinalizer)) + g.Expect(dnsRecord.Status.WriteCounter).To(BeZero()) }, TestTimeoutMedium, time.Second).Should(Succeed()) }) @@ -144,4 +132,31 @@ var _ = Describe("DNSRecordReconciler", func() { }) + It("should increase write counter if fail to publish record or record is overridden", func() { + dnsRecord.Spec.Endpoints = getTestNonExistingEndpoints() + Expect(k8sClient.Create(ctx, dnsRecord)).To(Succeed()) + + // should be requeue record for validation after the write attempt + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dnsRecord.Status.Conditions).To( + ContainElement(MatchFields(IgnoreExtras, Fields{ + "Type": Equal(string(v1alpha1.ConditionTypeReady)), + "Status": Equal(metav1.ConditionFalse), + "Reason": Equal("AwaitingValidation"), + "Message": Equal("Awaiting validation"), + "ObservedGeneration": Equal(dnsRecord.Generation), + })), + ) + }, TestTimeoutMedium, time.Second).Should(Succeed()) + + // should be increasing write counter + Eventually(func(g Gomega) { + err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord), dnsRecord) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(dnsRecord.Status.WriteCounter).To(BeNumerically(">", int64(0))) + }, TestTimeoutLong, time.Second).Should(Succeed()) + }) + }) diff --git a/internal/controller/helper_test.go b/internal/controller/helper_test.go index 4a7188c8..1754ef24 100644 --- a/internal/controller/helper_test.go +++ b/internal/controller/helper_test.go @@ -6,6 +6,7 @@ import ( "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + externaldnsendpoint "sigs.k8s.io/external-dns/endpoint" kuadrantdnsv1alpha1 "github.com/kuadrant/dns-operator/api/v1alpha1" ) @@ -14,6 +15,8 @@ const ( TestTimeoutMedium = time.Second * 10 TestTimeoutLong = time.Second * 30 TestRetryIntervalMedium = time.Millisecond * 250 + RequeueDuration = time.Second * 6 + ValidityDuration = time.Second * 3 defaultNS = "default" providerCredential = "secretname" ) @@ -34,3 +37,35 @@ func testBuildManagedZone(name, ns, domainName string) *kuadrantdnsv1alpha1.Mana }, } } + +func getTestEndpoints() []*externaldnsendpoint.Endpoint { + return []*externaldnsendpoint.Endpoint{ + { + DNSName: "foo.example.com", + Targets: []string{ + "127.0.0.1", + }, + RecordType: "A", + SetIdentifier: "", + RecordTTL: 60, + Labels: nil, + ProviderSpecific: nil, + }, + } +} + +func getTestNonExistingEndpoints() []*externaldnsendpoint.Endpoint { + return []*externaldnsendpoint.Endpoint{ + { + DNSName: "nope.example.com", + Targets: []string{ + "127.0.0.1", + }, + RecordType: "A", + SetIdentifier: "", + RecordTTL: 60, + Labels: nil, + ProviderSpecific: nil, + }, + } +} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index c2814359..61e4cbd3 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -53,11 +53,6 @@ import ( // These tests use Ginkgo (BDD-style Go testing framework). Refer to // http://onsi.github.io/ginkgo/ to learn more about Ginkgo. -const ( - RequeueDuration = time.Minute * 1 - ValidityDuration = time.Second * 30 -) - var cfg *rest.Config var k8sClient client.Client var testEnv *envtest.Environment @@ -67,7 +62,7 @@ var dnsProviderFactory = &providerFake.Factory{ ProviderForFunc: func(ctx context.Context, pa v1alpha1.ProviderAccessor, c provider.Config) (provider.Provider, error) { return &providerFake.Provider{ RecordsFunc: func(context.Context) ([]*externaldnsendpoint.Endpoint, error) { - return []*externaldnsendpoint.Endpoint{}, nil + return getTestEndpoints(), nil }, ApplyChangesFunc: func(context.Context, *externaldnsplan.Changes) error { return nil diff --git a/test/e2e/single_cluster_record_test.go b/test/e2e/single_cluster_record_test.go index 5d3cdbca..02216083 100644 --- a/test/e2e/single_cluster_record_test.go +++ b/test/e2e/single_cluster_record_test.go @@ -89,7 +89,7 @@ var _ = Describe("Single Cluster Record Test", func() { "Status": Equal(metav1.ConditionTrue), })), ) - }, 300*time.Second, 10*time.Second, ctx).Should(Succeed()) + }, 5*time.Minute, 10*time.Second, ctx).Should(Succeed()) By("ensuring the authoritative nameserver resolves the hostname") // speed up things by using the authoritative nameserver @@ -213,7 +213,7 @@ var _ = Describe("Single Cluster Record Test", func() { "Status": Equal(metav1.ConditionTrue), })), ) - }, 300*time.Second, 10*time.Second, ctx).Should(Succeed()) + }, 5*time.Minute, 10*time.Second, ctx).Should(Succeed()) By("ensuring the authoritative nameserver resolves the hostname") // speed up things by using the authoritative nameserver @@ -365,7 +365,7 @@ var _ = Describe("Single Cluster Record Test", func() { "Status": Equal(metav1.ConditionTrue), })), ) - }, 300*time.Second, 10*time.Second, ctx).Should(Succeed()) + }, 5*time.Minute, 10*time.Second, ctx).Should(Succeed()) By("ensuring the authoritative nameserver resolves the hostname") // speed up things by using the authoritative nameserver @@ -498,7 +498,7 @@ var _ = Describe("Single Cluster Record Test", func() { "Status": Equal(metav1.ConditionTrue), })), ) - }, 300*time.Second, 10*time.Second, ctx).Should(Succeed()) + }, 5*time.Minute, 10*time.Second, ctx).Should(Succeed()) By("ensuring the authoritative nameserver resolves the hostname") // speed up things by using the authoritative nameserver From 1bb751a578ddc0dcd475f693af70cfd6823c7d6a Mon Sep 17 00:00:00 2001 From: Maskym Vavilov Date: Thu, 11 Apr 2024 12:22:12 +0100 Subject: [PATCH 3/3] GH-36 use time.Duration instead of string --- internal/controller/dnsrecord_controller.go | 45 ++++++++++----------- 1 file changed, 21 insertions(+), 24 deletions(-) diff --git a/internal/controller/dnsrecord_controller.go b/internal/controller/dnsrecord_controller.go index 30077a13..48b60985 100644 --- a/internal/controller/dnsrecord_controller.go +++ b/internal/controller/dnsrecord_controller.go @@ -18,7 +18,6 @@ package controller import ( "context" - "errors" "fmt" "strings" "time" @@ -49,6 +48,7 @@ const ( var ( defaultRequeueTime time.Duration validationRequeueTime = time.Second * 5 + noRequeueDuration = time.Duration(0) validFor time.Duration reconcileStart = metav1.Time{} Clock clock.Clock = clock.RealClock{} @@ -113,7 +113,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( reason = "ValidationError" message = fmt.Sprintf("validation of DNSRecord failed: %v", err) setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, reason, message) - return r.updateStatus(ctx, previous, dnsRecord, "0s") + return r.updateStatus(ctx, previous, dnsRecord, noRequeueDuration) } // Publish the record @@ -122,7 +122,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( reason = "ProviderError" message = fmt.Sprintf("The DNS provider failed to ensure the record: %v", provider.SanitizeError(err)) setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, reason, message) - return r.updateStatus(ctx, previous, dnsRecord, "0s") + return r.updateStatus(ctx, previous, dnsRecord, noRequeueDuration) } // success dnsRecord.Status.ObservedGeneration = dnsRecord.Generation @@ -130,12 +130,8 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return r.updateStatus(ctx, previous, dnsRecord, requeueAfter) } -func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, current *v1alpha1.DNSRecord, requeueAfter string) (reconcile.Result, error) { - requeueDuration, err := time.ParseDuration(requeueAfter) - if err != nil { - return ctrl.Result{}, errors.New("error parsing duration while setting requeue time") - } - current.Status.QueuedFor = metav1.NewTime(reconcileStart.Add(requeueDuration)) +func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, current *v1alpha1.DNSRecord, requeueAfter time.Duration) (reconcile.Result, error) { + current.Status.QueuedFor = metav1.NewTime(reconcileStart.Add(requeueAfter)) if !equality.Semantic.DeepEqual(previous.Status, current.Status) { updateError := r.Status().Update(ctx, current) @@ -144,7 +140,7 @@ func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, curren } return ctrl.Result{}, updateError } - return ctrl.Result{RequeueAfter: requeueDuration}, nil + return ctrl.Result{RequeueAfter: requeueAfter}, nil } // SetupWithManager sets up the controller with the Manager. @@ -197,7 +193,7 @@ func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alp // publishRecord publishes record(s) to the DNSPRovider(i.e. route53) configured by the ManagedZone assigned to this // DNSRecord (dnsRecord.Status.ParentManagedZone). -func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) (string, error) { +func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) (time.Duration, error) { logger := log.FromContext(ctx) managedZone := &v1alpha1.ManagedZone{ ObjectMeta: metav1.ObjectMeta{ @@ -207,12 +203,12 @@ func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1al } err := r.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone, &client.GetOptions{}) if err != nil { - return "0s", err + return noRequeueDuration, err } managedZoneReady := meta.IsStatusConditionTrue(managedZone.Status.Conditions, "Ready") if !managedZoneReady { - return "0s", fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name) + return noRequeueDuration, fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name) } // cut off here for the short reconcile loop @@ -223,7 +219,7 @@ func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1al expiryTime := metav1.NewTime(dnsRecord.Status.QueuedAt.Add(requeueIn)) if !generationChanged(dnsRecord) && reconcileStart.Before(&expiryTime) { logger.V(3).Info("Skipping managed zone to which the DNS dnsRecord is already published and is still valid", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) - return requeueIn.String(), nil + return requeueIn, nil } if generationChanged(dnsRecord) { dnsRecord.Status.WriteCounter = 0 @@ -231,7 +227,7 @@ func (r *DNSRecordReconciler) publishRecord(ctx context.Context, dnsRecord *v1al requeueAfter, err := r.applyChanges(ctx, dnsRecord, managedZone, false) if err != nil { - return "0s", err + return noRequeueDuration, err } logger.Info("Published DNSRecord to manage zone", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name) @@ -254,7 +250,7 @@ func setDNSRecordCondition(dnsRecord *v1alpha1.DNSRecord, conditionType string, meta.SetStatusCondition(&dnsRecord.Status.Conditions, cond) } -func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alpha1.DNSRecord, managedZone *v1alpha1.ManagedZone, isDelete bool) (string, error) { +func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alpha1.DNSRecord, managedZone *v1alpha1.ManagedZone, isDelete bool) (time.Duration, error) { logger := log.FromContext(ctx) filterDomain, _ := strings.CutPrefix(managedZone.Spec.DomainName, v1alpha1.WildcardPrefix) if dnsRecord.Spec.RootHost != nil { @@ -270,7 +266,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp logger.V(3).Info("applyChanges", "zone", managedZone.Spec.DomainName, "rootDomainFilter", rootDomainFilter, "providerConfig", providerConfig) dnsProvider, err := r.ProviderFactory.ProviderFor(ctx, managedZone, providerConfig) if err != nil { - return "0s", err + return noRequeueDuration, err } managedDNSRecordTypes := []string{externaldnsendpoint.RecordTypeA, externaldnsendpoint.RecordTypeAAAA, externaldnsendpoint.RecordTypeCNAME} @@ -278,13 +274,13 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp registry, err := dnsRecord.GetRegistry(dnsProvider, managedDNSRecordTypes, excludeDNSRecordTypes) if err != nil { - return "0s", err + return noRequeueDuration, err } policyID := "sync" policy, exists := externaldnsplan.Policies[policyID] if !exists { - return "0s", fmt.Errorf("unknown policy: %s", policyID) + return noRequeueDuration, fmt.Errorf("unknown policy: %s", policyID) } //If we are deleting set the expected endpoints to an empty array @@ -295,19 +291,19 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp //zoneEndpoints = Records in the current dns provider zone zoneEndpoints, err := registry.Records(ctx) if err != nil { - return "0s", err + return noRequeueDuration, err } //specEndpoints = Records that this DNSRecord expects to exist specEndpoints, err := registry.AdjustEndpoints(dnsRecord.Spec.Endpoints) if err != nil { - return "0s", fmt.Errorf("adjusting specEndpoints: %w", err) + return noRequeueDuration, fmt.Errorf("adjusting specEndpoints: %w", err) } //statusEndpoints = Records that were created/updated by this DNSRecord last statusEndpoints, err := registry.AdjustEndpoints(dnsRecord.Status.Endpoints) if err != nil { - return "0s", fmt.Errorf("adjusting statusEndpoints: %w", err) + return noRequeueDuration, fmt.Errorf("adjusting statusEndpoints: %w", err) } //Note: All endpoint lists should be in the same provider specific format at this point @@ -335,13 +331,14 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp // implies that they were overridden - bump write counter if !generationChanged(dnsRecord) { dnsRecord.Status.WriteCounter++ + logger.V(3).Info("Changes needed on the same generation of record") } dnsRecord.Status.ValidFor = validationRequeueTime.String() setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, "AwaitingValidation", "Awaiting validation") logger.Info("Applying changes") err = registry.ApplyChanges(ctx, plan.Changes) if err != nil { - return dnsRecord.Status.ValidFor, err + return validationRequeueTime, err } } else { logger.Info("All records are already up to date") @@ -349,5 +346,5 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionTrue, "ProviderSuccess", "Provider ensured the dns record") } - return dnsRecord.Status.ValidFor, nil + return defaultRequeueTime, nil }