Initialize route backends after module updates

internal/controllers/httpsedge_controller.go

@@ -117,7 +117,12 @@ func (r *HTTPSEdgeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		return ctrl.Result{}, nil
 	}
 
-	return ctrl.Result{}, r.reconcileEdge(ctx, edge)
+	err := r.reconcileEdge(ctx, edge)
+	if errors.IsErrorReconcilable(err) {
+		return ctrl.Result{}, err
+	} else {
+		return ctrl.Result{}, nil
+	}
 }
 
 func (r *HTTPSEdgeReconciler) reconcileEdge(ctx context.Context, edge *ingressv1alpha1.HTTPSEdge) error {
@@ -189,71 +194,91 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress
 		secretResolver:   secretResolver{r.Client},
 	}
 
+	edgeRoutes := r.NgrokClientset.HTTPSEdgeRoutes()
+
 	// TODO: clean this up. This is way too much nesting
 	for i, routeSpec := range edge.Spec.Routes {
-		backend, err := tunnelGroupReconciler.findOrCreate(ctx, routeSpec.Backend)
-		if err != nil {
-			return err
-		}
 
 		if routeSpec.IPRestriction != nil {
 			if err := routeModuleUpdater.ipPolicyResolver.validateIPPolicyNames(ctx, edge.Namespace, routeSpec.IPRestriction.IPPolicies); err != nil {
 				if apierrors.IsNotFound(err) {
 					r.Recorder.Eventf(edge, v1.EventTypeWarning, "FailedValidate", "Could not validate ip restriction: %v", err)
 					continue
 				}
-
 				return err
 			}
 		}
 
 		match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec)
 		var route *ngrok.HTTPSEdgeRoute
+		// Now we go ahead and create the route if it doesn't exist.
+		// It's important to note here that we are intentionally ommiting the `route.Backend` for new routes.
+		//  The success or failure of applying a route's modules is then strongly linked the state of its backend.
+		//  Thus, any route with a backend is considered properly configured.
+		//  See https://github.com/ngrok/kubernetes-ingress-controller/issues/208 for additional context.
 		if match == nil {
-			r.Log.Info("Creating new route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID)
-			// This is a new route, so we need to create it
+			r.Log.Info("Creating new route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType)
 			req := &ngrok.HTTPSEdgeRouteCreate{
 				EdgeID:    edge.Status.ID,
 				Match:     routeSpec.Match,
 				MatchType: routeSpec.MatchType,
-				Backend: &ngrok.EndpointBackendMutate{
-					BackendID: backend.ID,
-				},
 			}
-			route, err = r.NgrokClientset.HTTPSEdgeRoutes().Create(ctx, req)
+			route, err = edgeRoutes.Create(ctx, req)
 		} else {
-			r.Log.Info("Updating route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID)
-			// This is an existing route, so we need to update it
-			req := &ngrok.HTTPSEdgeRouteUpdate{
-				EdgeID:    edge.Status.ID,
-				ID:        match.ID,
-				Match:     routeSpec.Match,
-				MatchType: routeSpec.MatchType,
-				Backend: &ngrok.EndpointBackendMutate{
-					BackendID: backend.ID,
-				},
+			req := &ngrok.EdgeRouteItem{
+				ID:     match.ID,
+				EdgeID: edge.Status.ID,
 			}
-			route, err = r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, req)
+			route, err = edgeRoutes.Get(ctx, req)
 		}
 		if err != nil {
 			return err
 		}
+
+		// Update status for newly created route
 		routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{
 			ID:        route.ID,
 			URI:       route.URI,
 			Match:     route.Match,
 			MatchType: route.MatchType,
 		}
+
+		// With the route properly staged, we now attempt to apply its module updates
+		// TODO: Check if there are no updates to apply here to skip any unnecessary disruption
+		r.Log.Info("Applying route modules", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType)
+		if err := routeModuleUpdater.updateModulesForRoute(ctx, route, &routeSpec); err != nil {
+			r.Recorder.Event(edge, v1.EventTypeWarning, "RouteModuleUpdateFailed", err.Error())
+			return err
+		}
+
+		// The route modules were successfully applied, so now we update the route with its specified backend
+		backend, err := tunnelGroupReconciler.findOrCreate(ctx, routeSpec.Backend)
+		if err != nil {
+			return err
+		}
+		r.Log.Info("Updating route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID)
+
+		// TODO: Do an entropy check here to avoid unnecessary updates
+		req := &ngrok.HTTPSEdgeRouteUpdate{
+			EdgeID:    edge.Status.ID,
+			ID:        route.ID,
+			Match:     routeSpec.Match,
+			MatchType: routeSpec.MatchType,
+			Backend: &ngrok.EndpointBackendMutate{
+				BackendID: backend.ID,
+			},
+		}
+		route, err = edgeRoutes.Update(ctx, req)
+		if err != nil {
+			return err
+		}
+
+		// With the route modules successfully applied and the edge updated, we now update the route's backend status
 		if route.Backend != nil {
 			routeStatuses[i].Backend = ingressv1alpha1.TunnelGroupBackendStatus{
 				ID: route.Backend.Backend.ID,
 			}
 		}
-
-		// Update all route modules for a given route
-		if err := routeModuleUpdater.updateModulesForRoute(ctx, route, &routeSpec); err != nil {
-			return err
-		}
 	}
 
 	// Delete any routes that are no longer in the spec
@@ -267,7 +292,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress
 		}
 		if !found {
 			r.Log.Info("Deleting route", "edgeID", edge.Status.ID, "routeID", remoteRoute.ID)
-			if err := r.NgrokClientset.HTTPSEdgeRoutes().Delete(ctx, &ngrok.EdgeRouteItem{EdgeID: edge.Status.ID, ID: remoteRoute.ID}); err != nil {
+			if err := edgeRoutes.Delete(ctx, &ngrok.EdgeRouteItem{EdgeID: edge.Status.ID, ID: remoteRoute.ID}); err != nil {
 				return err
 			}
 		}
@@ -707,7 +732,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOAuth(ctx context.Context, route *n
 	}
 
 	if module == nil {
-		return fmt.Errorf("no OAuth provider configured")
+		return errors.NewErrInvalidConfiguration(fmt.Errorf("no OAuth provider configured"))
 	}
 
 	if reflect.DeepEqual(module, route.OAuth) {

internal/errors/errors.go

@@ -1,9 +1,11 @@
 package errors
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
+	"github.com/ngrok/ngrok-api-go/v5"
 	netv1 "k8s.io/api/networking/v1"
 )
 
@@ -135,3 +137,36 @@ func IsErrMissingRequiredSecret(err error) bool {
 	_, ok := err.(ErrMissingRequiredSecret)
 	return ok
 }
+
+type ErrInvalidConfiguration struct {
+	cause error
+}
+
+func NewErrInvalidConfiguration(cause error) error {
+	return ErrInvalidConfiguration{cause: cause}
+}
+
+func (e ErrInvalidConfiguration) Error() string {
+	return fmt.Sprintf("invalid configuration: %s", e.cause.Error())
+}
+
+func (e ErrInvalidConfiguration) Unwrap() error {
+	return e.cause
+}
+
+func IsErrorReconcilable(err error) bool {
+	if err == nil {
+		return true
+	}
+
+	if errors.As(err, &ErrInvalidConfiguration{}) {
+		return false
+	}
+
+	var nerr *ngrok.Error
+	if errors.As(err, &nerr) {
+		return nerr.StatusCode >= 500
+	}
+
+	return true
+}