Skip to content

Commit

Permalink
Merge pull request #256 from containeroo/cleanup
Browse files Browse the repository at this point in the history 
refactor: use more idiomatic error handling
  • Loading branch information
@rxbn
rxbn committed Aug 14, 2023
2 parents 17d2c9f + 1e564bb commit 1cb94c0
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 149 deletions.
44 changes: 15 additions & 29 deletions controllers/account_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
log := ctrllog.FromContext(ctx)

instance := &cfv1.Account{}
err := r.Get(ctx, req.NamespacedName, instance)
if err != nil {
if err := r.Get(ctx, req.NamespacedName, instance); err != nil {
if errors.IsNotFound(err) {
log.Info("Account resource not found. Ignoring since object must be deleted.")
return ctrl.Result{}, nil
Expand All @@ -68,8 +67,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

if !controllerutil.ContainsFinalizer(instance, cloudflareOperatorFinalizer) {
controllerutil.AddFinalizer(instance, cloudflareOperatorFinalizer)
err := r.Update(ctx, instance)
if err != nil {
if err := r.Update(ctx, instance); err != nil {
log.Error(err, "Failed to update Account finalizer")
return ctrl.Result{}, err
}
Expand All @@ -81,8 +79,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

controllerutil.RemoveFinalizer(instance, cloudflareOperatorFinalizer)
err := r.Update(ctx, instance)
if err != nil {
if err := r.Update(ctx, instance); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, nil
Expand All @@ -91,10 +88,8 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
accountFailureCounter.WithLabelValues(instance.Name).Set(0)

secret := &corev1.Secret{}
err = r.Get(ctx, client.ObjectKey{Namespace: instance.Spec.ApiToken.SecretRef.Namespace, Name: instance.Spec.ApiToken.SecretRef.Name}, secret)
if err != nil {
err := r.markFailed(instance, ctx, "Failed to get secret")
if err != nil {
if err := r.Get(ctx, client.ObjectKey{Namespace: instance.Spec.ApiToken.SecretRef.Namespace, Name: instance.Spec.ApiToken.SecretRef.Name}, secret); err != nil {
if err := r.markFailed(instance, ctx, "Failed to get secret"); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
Expand All @@ -103,18 +98,16 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

cfApiToken := string(secret.Data["apiToken"])
if cfApiToken == "" {
err := r.markFailed(instance, ctx, "Secret has no 'apiToken' key")
if err != nil {
if err := r.markFailed(instance, ctx, "Secret has no 'apiToken' key"); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: time.Second * 30}, err
return ctrl.Result{RequeueAfter: time.Second * 30}, nil
}

cf, err := cloudflare.NewWithAPIToken(cfApiToken)
if err != nil {
err := r.markFailed(instance, ctx, err.Error())
if err != nil {
if err := r.markFailed(instance, ctx, err.Error()); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
Expand All @@ -124,8 +117,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

cfZones, err := r.Cf.ListZones(ctx)
if err != nil {
err := r.markFailed(instance, ctx, err.Error())
if err != nil {
if err := r.markFailed(instance, ctx, err.Error()); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
Expand All @@ -148,10 +140,8 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

zones := &cfv1.ZoneList{}
err = r.List(ctx, zones)
if err != nil {
err := r.markFailed(instance, ctx, "Failed to list zones")
if err != nil {
if err := r.List(ctx, zones); err != nil {
if err := r.markFailed(instance, ctx, "Failed to list zones"); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
Expand Down Expand Up @@ -191,8 +181,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
Interval: instance.Spec.Interval,
},
}
err = r.Create(ctx, z)
if err != nil {
if err := r.Create(ctx, z); err != nil {
log.Error(err, "Failed to create Zone resource", "Zone.Name", operatorManagedZone.Name, "Zone.ID", operatorManagedZone.ID)
continue
}
Expand All @@ -202,17 +191,15 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

if !reflect.DeepEqual(instance.Status.Zones, operatorManagedZones) {
instance.Status.Zones = operatorManagedZones
err := r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
}

for _, z := range zones.Items {
if _, found := operatorManagedZoneMap[z.Spec.ID]; !found {
err = r.Delete(ctx, &z)
if err != nil {
if err := r.Delete(ctx, &z); err != nil {
log.Error(err, "Failed to delete Zone resource", "Zone.Name", z.Name)
return ctrl.Result{}, err
}
Expand All @@ -227,8 +214,7 @@ func (r *AccountReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
Message: "Account is ready",
ObservedGeneration: instance.Generation,
})
err = r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update Account status")
return ctrl.Result{}, err
}
Expand Down
62 changes: 22 additions & 40 deletions controllers/dnsrecord_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
log := ctrllog.FromContext(ctx)

instance := &cfv1.DNSRecord{}
err := r.Get(ctx, req.NamespacedName, instance)
if err != nil {
if err := r.Get(ctx, req.NamespacedName, instance); err != nil {
if errors.IsNotFound(err) {
log.Info("DNSRecord resource not found. Ignoring since object must be deleted.")
return ctrl.Result{}, nil
Expand All @@ -74,23 +73,20 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
Message: "Cloudflare account is not yet ready",
ObservedGeneration: instance.Generation,
})
err := r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: time.Second * 5}, err
return ctrl.Result{RequeueAfter: time.Second * 5}, nil
}

zoneName, _ := publicsuffix.EffectiveTLDPlusOne(instance.Spec.Name)
zoneName = strings.ReplaceAll(zoneName, ".", "-")

zone := &cfv1.Zone{}
err = r.Get(ctx, client.ObjectKey{Name: zoneName}, zone)
if err != nil {
if err := r.Get(ctx, client.ObjectKey{Name: zoneName}, zone); err != nil {
if errors.IsNotFound(err) {
err := r.markFailed(instance, ctx, "Zone not found")
if err != nil {
if err := r.markFailed(instance, ctx, "Zone not found"); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -108,8 +104,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
Message: "Zone is not yet ready",
ObservedGeneration: instance.Generation,
})
err := r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -118,8 +113,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

if !controllerutil.ContainsFinalizer(instance, cloudflareOperatorFinalizer) {
controllerutil.AddFinalizer(instance, cloudflareOperatorFinalizer)
err := r.Update(ctx, instance)
if err != nil {
if err := r.Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord finalizer")
return ctrl.Result{}, err
}
Expand All @@ -132,8 +126,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}

controllerutil.RemoveFinalizer(instance, cloudflareOperatorFinalizer)
err := r.Update(ctx, instance)
if err != nil {
if err := r.Update(ctx, instance); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, nil
Expand All @@ -144,6 +137,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
var existingRecord cloudflare.DNSRecord

if instance.Status.RecordID != "" {
var err error
existingRecord, err = r.Cf.GetDNSRecord(ctx, cloudflare.ZoneIdentifier(zone.Spec.ID), instance.Status.RecordID)
if err != nil && err.Error() != "Record does not exist. (81044)" {
log.Error(err, "Failed to get DNS record from Cloudflare")
Expand All @@ -153,28 +147,24 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (

if (instance.Spec.Type == "A" || instance.Spec.Type == "AAAA") && instance.Spec.IPRef.Name != "" {
ip := &cfv1.IP{}
err := r.Get(ctx, client.ObjectKey{Name: instance.Spec.IPRef.Name}, ip)
if err != nil {
err := r.markFailed(instance, ctx, "IP object not found")
if err != nil {
if err := r.Get(ctx, client.ObjectKey{Name: instance.Spec.IPRef.Name}, ip); err != nil {
if err := r.markFailed(instance, ctx, "IP object not found"); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: time.Second * 30}, err
}
if ip.Spec.Address != instance.Spec.Content {
instance.Spec.Content = ip.Spec.Address
err = r.Update(ctx, instance)
if err != nil {
if err := r.Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord resource")
return ctrl.Result{}, err
}
}
}

if *instance.Spec.Proxied && instance.Spec.TTL != 1 {
err := r.markFailed(instance, ctx, "TTL must be 1 when proxied")
if err != nil {
if err := r.markFailed(instance, ctx, "TTL must be 1 when proxied"); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -192,8 +182,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
Data: instance.Spec.Data,
})
if err != nil {
err := r.markFailed(instance, ctx, err.Error())
if err != nil {
if err := r.markFailed(instance, ctx, err.Error()); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -207,16 +196,15 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
ObservedGeneration: instance.Generation,
})
instance.Status.RecordID = newDNSRecord.ID
err = r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: instance.Spec.Interval.Duration}, nil
}

if !compareDNSRecord(instance.Spec, existingRecord) {
_, err := r.Cf.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier(zone.Spec.ID), cloudflare.UpdateDNSRecordParams{
if _, err := r.Cf.UpdateDNSRecord(ctx, cloudflare.ZoneIdentifier(zone.Spec.ID), cloudflare.UpdateDNSRecordParams{
ID: existingRecord.ID,
Name: instance.Spec.Name,
Type: instance.Spec.Type,
Expand All @@ -225,10 +213,8 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
Proxied: instance.Spec.Proxied,
Priority: instance.Spec.Priority,
Data: instance.Spec.Data,
})
if err != nil {
err := r.markFailed(instance, ctx, err.Error())
if err != nil {
}); err != nil {
if err := r.markFailed(instance, ctx, err.Error()); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -242,8 +228,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
ObservedGeneration: instance.Generation,
})
instance.Status.RecordID = existingRecord.ID
err = r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -259,8 +244,7 @@ func (r *DNSRecordReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
ObservedGeneration: instance.Generation,
})
instance.Status.RecordID = existingRecord.ID
err = r.Status().Update(ctx, instance)
if err != nil {
if err := r.Status().Update(ctx, instance); err != nil {
log.Error(err, "Failed to update DNSRecord status")
return ctrl.Result{}, err
}
Expand All @@ -277,8 +261,7 @@ func (r *DNSRecordReconciler) SetupWithManager(mgr ctrl.Manager) error {

// finalizeDNSRecord deletes the DNS record from cloudflare
func (r *DNSRecordReconciler) finalizeDNSRecord(ctx context.Context, dnsRecordZoneId string, log logr.Logger, d *cfv1.DNSRecord) {
err := r.Cf.DeleteDNSRecord(ctx, cloudflare.ZoneIdentifier(dnsRecordZoneId), d.Status.RecordID)
if err != nil {
if err := r.Cf.DeleteDNSRecord(ctx, cloudflare.ZoneIdentifier(dnsRecordZoneId), d.Status.RecordID); err != nil {
log.Error(err, "Failed to delete DNS record in Cloudflare. Record may still exist in Cloudflare")
}
}
Expand Down Expand Up @@ -319,8 +302,7 @@ func compareData(a interface{}, b *apiextensionsv1.JSON) bool {
return false
}
var bb interface{}
err := json.Unmarshal(b.Raw, &bb)
if err != nil {
if err := json.Unmarshal(b.Raw, &bb); err != nil {
return false
}
return reflect.DeepEqual(a, bb)
Expand Down
26 changes: 8 additions & 18 deletions controllers/ingress_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
log := ctrllog.FromContext(ctx)

instance := &networkingv1.Ingress{}
err := r.Get(ctx, req.NamespacedName, instance)
if err != nil {
if err := r.Get(ctx, req.NamespacedName, instance); err != nil {
if errors.IsNotFound(err) {
log.Info("Ingress resource not found. Ignoring since object must be deleted.")
return ctrl.Result{}, nil
Expand All @@ -58,27 +57,21 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

dnsRecords := &cfv1.DNSRecordList{}
err = r.List(
ctx,
dnsRecords,
client.InNamespace(instance.Namespace),
client.MatchingFields{"metadata.ownerReferences.uid": string(instance.UID)})
if err != nil {
if err := r.List(ctx, dnsRecords, client.InNamespace(instance.Namespace), client.MatchingFields{"metadata.ownerReferences.uid": string(instance.UID)}); err != nil {
log.Error(err, "Failed to fetch DNSRecord")
return ctrl.Result{RequeueAfter: time.Second * 30}, err
}

if instance.Annotations["cloudflare-operator.io/ignore"] == "true" {
if len(dnsRecords.Items) > 0 {
for _, dnsRecord := range dnsRecords.Items {
err := r.Delete(ctx, &dnsRecord)
if err != nil {
if err := r.Delete(ctx, &dnsRecord); err != nil {
log.Error(err, "Failed to delete DNSRecord")
return ctrl.Result{RequeueAfter: time.Second * 30}, err
}
log.Info("Deleted DNSRecord, because it was owned by an Ingress that is being ignored", "DNSRecord", dnsRecord.Name)
}
return ctrl.Result{}, err
return ctrl.Result{}, nil
}
log.Info("Ingress has ignore annotation, skipping reconciliation", "ingress", instance.Name)
return ctrl.Result{}, nil
Expand Down Expand Up @@ -107,7 +100,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
dnsRecordSpec.Proxied = newFalse()
default:
dnsRecordSpec.Proxied = newTrue()
log.Error(err, "Failed to parse proxied annotation, defaulting to true", "proxied", proxied, "ingress", instance.Name)
log.Error(nil, "Failed to parse proxied annotation, defaulting to true", "proxied", proxied, "ingress", instance.Name)
}
}
if dnsRecordSpec.Proxied == nil {
Expand Down Expand Up @@ -185,8 +178,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
}

log.Info("Creating DNSRecord", "name", dnsRecord.Name)
err = r.Create(ctx, dnsRecord)
if err != nil {
if err := r.Create(ctx, dnsRecord); err != nil {
log.Error(err, "Failed to create DNSRecord")
return ctrl.Result{}, err
}
Expand All @@ -205,8 +197,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct

log.Info("Updating DNSRecord", "name", dnsRecord.Name)
dnsRecord.Spec = dnsRecordSpec
err := r.Update(ctx, &dnsRecord)
if err != nil {
if err := r.Update(ctx, &dnsRecord); err != nil {
log.Error(err, "Failed to update DNSRecord")
return ctrl.Result{}, err
}
Expand All @@ -217,8 +208,7 @@ func (r *IngressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
continue
}
log.Info("Deleting DNSRecord", "name", dnsRecord.Name)
err := r.Delete(ctx, &dnsRecord)
if err != nil {
if err := r.Delete(ctx, &dnsRecord); err != nil {
log.Error(err, "Failed to delete DNSRecord")
}
}
Expand Down
Loading

0 comments on commit 1cb94c0

Please sign in to comment.