Skip to content

Commit

Permalink
Merge pull request #288 from ngrok/nikolay/283-error-handling
Browse files Browse the repository at this point in the history 
unified reconciling and error handling
  • Loading branch information
@nikolay-ngrok
nikolay-ngrok authored Aug 1, 2023
2 parents 1ccc930 + dea5595 commit c4387ea
Show file tree
Hide file tree
Showing 9 changed files with 379 additions and 395 deletions.
116 changes: 116 additions & 0 deletions internal/controllers/base_controller.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package controllers

import (
"context"
"errors"
"fmt"
"net/http"
"time"

"github.com/go-logr/logr"
"github.com/ngrok/ngrok-api-go/v5"
v1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
)

type baseControllerOp int

const (
createOp baseControllerOp = iota
updateOp
deleteOp
)

type baseController[T client.Object] struct {
Kube client.Client
Log logr.Logger
Recorder record.EventRecorder

kubeType string
statusID func(ct T) string
create func(ctx context.Context, cr T) error
update func(ctx context.Context, cr T) error
delete func(ctx context.Context, cr T) error
errResult func(op baseControllerOp, cr T, err error) (ctrl.Result, error)
}

func (r *baseController[T]) reconcile(ctx context.Context, req ctrl.Request, cr T) (ctrl.Result, error) {
log := r.Log.WithValues(r.kubeType, req.NamespacedName)
ctx = ctrl.LoggerInto(ctx, log)

if err := r.Kube.Get(ctx, req.NamespacedName, cr); err != nil {
return ctrl.Result{}, client.IgnoreNotFound(err)
}

crName := req.NamespacedName.String()
if isUpsert(cr) {
if err := registerAndSyncFinalizer(ctx, r.Kube, cr); err != nil {
return ctrl.Result{}, err
}

if r.statusID != nil && r.statusID(cr) == "" {
r.Recorder.Event(cr, v1.EventTypeNormal, "Creating", fmt.Sprintf("Creating %s: %s", r.kubeType, crName))
if err := r.create(ctx, cr); err != nil {
r.Recorder.Event(cr, v1.EventTypeWarning, "CreateError", fmt.Sprintf("Failed to create %s %s: %s", r.kubeType, crName, err.Error()))
if r.errResult != nil {
return r.errResult(createOp, cr, err)
}
return reconcileResultFromError(err)
}
r.Recorder.Event(cr, v1.EventTypeNormal, "Created", fmt.Sprintf("Created %s: %s", r.kubeType, crName))
} else {
r.Recorder.Event(cr, v1.EventTypeNormal, "Updating", fmt.Sprintf("Updating %s: %s", r.kubeType, crName))
if err := r.update(ctx, cr); err != nil {
r.Recorder.Event(cr, v1.EventTypeWarning, "UpdateError", fmt.Sprintf("Failed to update %s %s: %s", r.kubeType, crName, err.Error()))
if r.errResult != nil {
return r.errResult(updateOp, cr, err)
}
return reconcileResultFromError(err)
}
r.Recorder.Event(cr, v1.EventTypeNormal, "Updated", fmt.Sprintf("Updated %s: %s", r.kubeType, crName))
}
} else {
if hasFinalizer(cr) {
if r.statusID == nil || r.statusID(cr) != "" {
sid := r.statusID(cr)
r.Recorder.Event(cr, v1.EventTypeNormal, "Deleting", fmt.Sprintf("Deleting %s: %s", r.kubeType, crName))
if err := r.delete(ctx, cr); err != nil {
if !ngrok.IsNotFound(err) {
r.Recorder.Event(cr, v1.EventTypeWarning, "DeleteError", fmt.Sprintf("Failed to delete %s %s: %s", r.kubeType, crName, err.Error()))
if r.errResult != nil {
return r.errResult(deleteOp, cr, err)
}
return reconcileResultFromError(err)
}
log.Info(fmt.Sprintf("%s not found, assuming it was already deleted", r.kubeType), "ID", sid)
}
r.Recorder.Event(cr, v1.EventTypeNormal, "Deleted", fmt.Sprintf("Deleted %s: %s", r.kubeType, crName))
}

if err := removeAndSyncFinalizer(ctx, r.Kube, cr); err != nil {
return ctrl.Result{}, err
}
}
}

return ctrl.Result{}, nil
}

func reconcileResultFromError(err error) (ctrl.Result, error) {
var nerr *ngrok.Error
if errors.As(err, &nerr) {
switch {
case nerr.StatusCode >= 500:
return ctrl.Result{}, err
case nerr.StatusCode == http.StatusTooManyRequests:
return ctrl.Result{RequeueAfter: time.Minute}, nil
default:
// the rest are client errors, we don't retry by default
return ctrl.Result{}, nil
}
}

return ctrl.Result{}, err
}
115 changes: 37 additions & 78 deletions internal/controllers/domain_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ type DomainReconciler struct {
Scheme *runtime.Scheme
Recorder record.EventRecorder
DomainsClient *reserved_domains.Client

controller *baseController[*ingressv1alpha1.Domain]
}

// SetupWithManager sets up the controller with the Manager.
Expand All @@ -56,6 +58,18 @@ func (r *DomainReconciler) SetupWithManager(mgr ctrl.Manager) error {
return fmt.Errorf("DomainsClient must be set")
}

r.controller = &baseController[*ingressv1alpha1.Domain]{
Kube: r.Client,
Log: r.Log,
Recorder: r.Recorder,

kubeType: "v1alpha1.Domain",
statusID: func(cr *ingressv1alpha1.Domain) string { return cr.Status.ID },
create: r.create,
update: r.update,
delete: r.delete,
}

return ctrl.NewControllerManagedBy(mgr).
For(&ingressv1alpha1.Domain{}).
WithEventFilter(commonPredicateFilters).
Expand All @@ -72,72 +86,10 @@ func (r *DomainReconciler) SetupWithManager(mgr ctrl.Manager) error {
// For more details, check Reconcile and its Result here:
// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.13.1/pkg/reconcile
func (r *DomainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
log := r.Log.WithValues("V1Alpha1Domain", req.NamespacedName)

domain := new(ingressv1alpha1.Domain)
if err := r.Get(ctx, req.NamespacedName, domain); err != nil {
return ctrl.Result{}, client.IgnoreNotFound(err)
}

if domain.ObjectMeta.DeletionTimestamp.IsZero() {
if err := registerAndSyncFinalizer(ctx, r.Client, domain); err != nil {
return ctrl.Result{}, err
}
} else {
// The object is being deleted
if hasFinalizer(domain) {
if domain.Status.ID != "" {
log.Info("Deleting reserved domain", "ID", domain.Status.ID)
r.Recorder.Event(domain, v1.EventTypeNormal, "Deleting", fmt.Sprintf("Deleting Domain %s", domain.Name))
// Question: Do we actually want to delete the reserved domains for real? Or maybe just delete the resource and have the user delete the reserved domain from
// the ngrok dashboard manually?
if err := r.DomainsClient.Delete(ctx, domain.Status.ID); err != nil {
if !ngrok.IsNotFound(err) {
r.Recorder.Event(domain, v1.EventTypeWarning, "FailedDelete", fmt.Sprintf("Failed to delete Domain %s: %s", domain.Name, err.Error()))
return ctrl.Result{}, err
}
log.Info("Domain not found, assuming it was already deleted", "ID", domain.Status.ID)
}
domain.Status.ID = ""
}

if err := removeAndSyncFinalizer(ctx, r.Client, domain); err != nil {
return ctrl.Result{}, err
}
}

r.Recorder.Event(domain, v1.EventTypeNormal, "Deleted", fmt.Sprintf("Deleted Domain %s", domain.Name))

// Stop reconciliation as the item is being deleted
return ctrl.Result{}, nil
}

if domain.Status.ID != "" {
if err := r.updateExternalResources(ctx, domain); err != nil {
r.Recorder.Event(domain, v1.EventTypeWarning, "UpdateFailed", fmt.Sprintf("Failed to update Domain %s: %s", domain.Name, err.Error()))
return ctrl.Result{}, err
}
} else {
// Create
if err := r.createExternalResources(ctx, domain); err != nil {
r.Recorder.Event(domain, v1.EventTypeWarning, "CreateFailed", fmt.Sprintf("Failed to create Domain %s: %s", domain.Name, err.Error()))
return ctrl.Result{}, err
}

r.Recorder.Event(domain, v1.EventTypeNormal, "Created", fmt.Sprintf("Created Domain %s", domain.Name))
}

return ctrl.Result{}, nil
return r.controller.reconcile(ctx, req, new(ingressv1alpha1.Domain))
}

// Deletes the external resources associated with the ReservedDomain. This is just the reserved domain itself.
//
//nolint:unused
func (r *DomainReconciler) deleteExternalResources(ctx context.Context, domain *ingressv1alpha1.Domain) error {
return r.DomainsClient.Delete(ctx, domain.Status.ID)
}

func (r *DomainReconciler) createExternalResources(ctx context.Context, domain *ingressv1alpha1.Domain) error {
func (r *DomainReconciler) create(ctx context.Context, domain *ingressv1alpha1.Domain) error {
// First check if the reserved domain already exists. The API is sometimes returning dangling CNAME records
// errors right now, so we'll check if the domain already exists before trying to create it.
resp, err := r.findReservedDomainByHostname(ctx, domain.Spec.Domain)
Expand All @@ -162,27 +114,34 @@ func (r *DomainReconciler) createExternalResources(ctx context.Context, domain *
return r.updateStatus(ctx, domain, resp)
}

func (r *DomainReconciler) updateExternalResources(ctx context.Context, domain *ingressv1alpha1.Domain) error {
func (r *DomainReconciler) update(ctx context.Context, domain *ingressv1alpha1.Domain) error {
resp, err := r.DomainsClient.Get(ctx, domain.Status.ID)
if err != nil {
return err
}

if !domain.Equal(resp) {
r.Log.Info("Updating reserved domain", "ID", domain.Status.ID)
req := &ngrok.ReservedDomainUpdate{
ID: domain.Status.ID,
Description: &domain.Spec.Description,
Metadata: &domain.Spec.Metadata,
}
resp, err = r.DomainsClient.Update(ctx, req)
if err != nil {
return err
}
return r.updateStatus(ctx, domain, resp)
if domain.Equal(resp) {
return nil
}

return nil
req := &ngrok.ReservedDomainUpdate{
ID: domain.Status.ID,
Description: &domain.Spec.Description,
Metadata: &domain.Spec.Metadata,
}
resp, err = r.DomainsClient.Update(ctx, req)
if err != nil {
return err
}
return r.updateStatus(ctx, domain, resp)
}

func (r *DomainReconciler) delete(ctx context.Context, domain *ingressv1alpha1.Domain) error {
err := r.DomainsClient.Delete(ctx, domain.Status.ID)
if err == nil || ngrok.IsNotFound(err) {
domain.Status.ID = ""
}
return err
}

// finds the reserved domain by the hostname. If it doesn't exist, returns nil
Expand Down
Loading

0 comments on commit c4387ea

Please sign in to comment.