Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-38 validatie record on deletion #72

Merged
merged 1 commit into from
Apr 16, 2024
Merged

GH-38 validatie record on deletion #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions internal/controller/dnsrecord_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,16 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
dnsRecord := previous.DeepCopy()

if dnsRecord.DeletionTimestamp != nil && !dnsRecord.DeletionTimestamp.IsZero() {
if err = r.deleteRecord(ctx, dnsRecord); err != nil {
requeueTime, err := r.deleteRecord(ctx, dnsRecord)
if err != nil {
logger.Error(err, "Failed to delete DNSRecord")
return ctrl.Result{}, err
return ctrl.Result{RequeueAfter: requeueTime}, err
}
// if requeueTime returned is the same as validationRequeueTime - the deleteRecord has successfully applied changes
// in this case we need to queue for validation to ensure DNS Provider retained changes
// before removing finalizer and deleting the DNS Record CR
if requeueTime == validationRequeueTime {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so delete was successful but we need to revalidate. Hmm this took me a while to figure out. Could we add a comment explaining that is what is happening here and why we do this comparison?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added more detailed comment

return ctrl.Result{RequeueAfter: requeueTime}, nil
}
logger.Info("Removing Finalizer", "name", DNSRecordFinalizer)
controllerutil.RemoveFinalizer(dnsRecord, DNSRecordFinalizer)
Expand Down Expand Up @@ -138,7 +145,7 @@ func (r *DNSRecordReconciler) updateStatus(ctx context.Context, previous, curren
if apierrors.IsConflict(updateError) {
return ctrl.Result{Requeue: true}, nil
}
return ctrl.Result{}, updateError
return ctrl.Result{RequeueAfter: requeueAfter}, updateError
}
return ctrl.Result{RequeueAfter: requeueAfter}, nil
}
Expand All @@ -155,7 +162,7 @@ func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager, requeueIn, vali

// deleteRecord deletes record(s) in the DNSPRovider(i.e. route53) configured by the ManagedZone assigned to this
// DNSRecord (dnsRecord.Status.ParentManagedZone).
func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) error {
func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alpha1.DNSRecord) (time.Duration, error) {
logger := log.FromContext(ctx)

managedZone := &v1alpha1.ManagedZone{
Expand All @@ -167,28 +174,28 @@ func (r *DNSRecordReconciler) deleteRecord(ctx context.Context, dnsRecord *v1alp
err := r.Get(ctx, client.ObjectKeyFromObject(managedZone), managedZone, &client.GetOptions{})
if err != nil {
// If the Managed Zone isn't found, just continue
return client.IgnoreNotFound(err)
return noRequeueDuration, client.IgnoreNotFound(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 validationRequeueTime, fmt.Errorf("the managed zone is not in a ready state : %s", managedZone.Name)
}

_, err = r.applyChanges(ctx, dnsRecord, managedZone, true)
requeueTime, 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)
return nil
return noRequeueDuration, nil
} else if strings.Contains(err.Error(), "no endpoints") {
logger.Info("DNS record had no endpoint, continuing", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name)
return nil
return noRequeueDuration, nil
}
return err
return noRequeueDuration, err
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does noRequeueDuration have the effect as returning ctrl.Result{}, updateError?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. The way it is interpreted: if err != nil it will ignore ctrl.Result{} entirely and will requeue immediately. If err == nil then it will look at instructions for requeue.
This makes me think that I might made it more confusing than it should be. There was a suggestion to just roam with the boolean flag saying if there were changes published or not and we decide on how to requeue in the very last step. I'm planning on having that implemented after the code freeze (but if that will make more sense could have it ready by the end of a day)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no leave as is for now and can follow up with improvements

}
logger.Info("Deleted DNSRecord in manage zone", "dnsRecord", dnsRecord.Name, "managedZone", managedZone.Name)

return nil
return requeueTime, nil
}

// publishRecord publishes record(s) to the DNSPRovider(i.e. route53) configured by the ManagedZone assigned to this
Expand Down
Loading