From 8ba1716201598929175d16c104e1103a7f9e7425 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 31 May 2023 11:55:58 -0600 Subject: [PATCH 01/17] Update edge controller to initialize route backends after module updates --- internal/controllers/httpsedge_controller.go | 53 +++++++++++++------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 658367d1..4b21b570 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -190,58 +190,73 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress // 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 - } - match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) var route *ngrok.HTTPSEdgeRoute if match == nil { - r.Log.Info("Creating new route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID) + r.Log.Info("Creating new route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) // This is a new route, so we need to create it 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) } 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 + // Otherwise, update the current route to disable its backend req := &ngrok.HTTPSEdgeRouteUpdate{ EdgeID: edge.Status.ID, ID: match.ID, Match: routeSpec.Match, MatchType: routeSpec.MatchType, - Backend: &ngrok.EndpointBackendMutate{ - BackendID: backend.ID, - }, } route, err = r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, req) } if err != nil { return err } + + // Update route status, sans backend routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ ID: route.ID, URI: route.URI, Match: route.Match, MatchType: route.MatchType, } + + // Attempt to apply module updates for a given route + r.Log.Info("Evaluating route module updates", "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 + } + + // Route modules successfully updated, so now we can update the route with the specified backend + backend, err := tunnelGroupReconciler.findOrCreate(ctx, routeSpec.Backend) + if err != nil { + return err + } + r.Log.Info("Updating route backend", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID) + + req := &ngrok.HTTPSEdgeRouteUpdate{ + EdgeID: edge.Status.ID, + ID: match.ID, + Match: routeSpec.Match, + MatchType: routeSpec.MatchType, + Backend: &ngrok.EndpointBackendMutate{ + BackendID: backend.ID, + }, + } + route, err = r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, req) + if err != nil { + return err + } + + // Update route status with backend ID 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 From b2d6c5899995d918d95b371ba61fb649ab87e367 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 31 May 2023 12:11:57 -0600 Subject: [PATCH 02/17] Simplify http edge routes var --- internal/controllers/httpsedge_controller.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 4b21b570..91a27969 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -188,6 +188,8 @@ 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 { match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) @@ -200,7 +202,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress Match: routeSpec.Match, MatchType: routeSpec.MatchType, } - route, err = r.NgrokClientset.HTTPSEdgeRoutes().Create(ctx, req) + route, err = edgeRoutes.Create(ctx, req) } else { // Otherwise, update the current route to disable its backend req := &ngrok.HTTPSEdgeRouteUpdate{ @@ -209,7 +211,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress Match: routeSpec.Match, MatchType: routeSpec.MatchType, } - route, err = r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, req) + route, err = edgeRoutes.Update(ctx, req) } if err != nil { return err @@ -246,7 +248,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress BackendID: backend.ID, }, } - route, err = r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, req) + route, err = edgeRoutes.Update(ctx, req) if err != nil { return err } @@ -270,7 +272,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 } } From cb0ca987e14edaabf240b2a84270aea1e410a322 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 31 May 2023 12:42:18 -0600 Subject: [PATCH 03/17] lint --- internal/controllers/httpsedge_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index e162b266..994457e7 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -194,7 +194,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress // TODO: clean this up. This is way too much nesting for i, routeSpec := range edge.Spec.Routes { - if routeSpec.IPRestriction != nil { + 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) @@ -204,7 +204,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress return err } } - + match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) var route *ngrok.HTTPSEdgeRoute if match == nil { From e7993a939f5a10dd5878bedf76a63b490ef1b7d2 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Fri, 2 Jun 2023 09:20:48 -0600 Subject: [PATCH 04/17] update log messages/comments --- internal/controllers/httpsedge_controller.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 994457e7..6b2367a9 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -207,9 +207,10 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) var route *ngrok.HTTPSEdgeRoute + // Search for the route by edge status. if match == nil { r.Log.Info("Creating new route", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) - // This is a new route, so we need to create it + // Route not found, so we need to create it. Note that we aren't setting the route's backend here yet. req := &ngrok.HTTPSEdgeRouteCreate{ EdgeID: edge.Status.ID, Match: routeSpec.Match, @@ -217,7 +218,8 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress } route, err = edgeRoutes.Create(ctx, req) } else { - // Otherwise, update the current route to disable its backend + r.Log.Info("Route found, disabling backend to prepare for module updates", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) + // If the route is found, update the it to disable its backend req := &ngrok.HTTPSEdgeRouteUpdate{ EdgeID: edge.Status.ID, ID: match.ID, @@ -238,19 +240,19 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress MatchType: route.MatchType, } - // Attempt to apply module updates for a given route - r.Log.Info("Evaluating route module updates", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) + // With the route now created and backend disabled, we now attempt to apply module updates for a given route + r.Log.Info("Updating 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 } - // Route modules successfully updated, so now we can update the route with the specified backend + // 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 backend", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID) + r.Log.Info("Setting route backend", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID) req := &ngrok.HTTPSEdgeRouteUpdate{ EdgeID: edge.Status.ID, From fc47ffd40fef9890ea3c948b98b73b2a212c3af1 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Fri, 2 Jun 2023 10:46:26 -0600 Subject: [PATCH 05/17] update comments --- internal/controllers/httpsedge_controller.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 6b2367a9..18479ed6 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -207,10 +207,13 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) var route *ngrok.HTTPSEdgeRoute - // Search for the route by edge status. + // Now we go ahead and create the route if it doesn't exist, or find the existing route. + // It's important to note here that we are intentionally ommiting the `route.Backend` for new routes, and + // removing it from existing routes. This is because we depend on the route modules being successfully applied + // before we apply the new route. This ensures that any route with a backend is considered to be successfully 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) - // Route not found, so we need to create it. Note that we aren't setting the route's backend here yet. req := &ngrok.HTTPSEdgeRouteCreate{ EdgeID: edge.Status.ID, Match: routeSpec.Match, @@ -219,7 +222,6 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress route, err = edgeRoutes.Create(ctx, req) } else { r.Log.Info("Route found, disabling backend to prepare for module updates", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) - // If the route is found, update the it to disable its backend req := &ngrok.HTTPSEdgeRouteUpdate{ EdgeID: edge.Status.ID, ID: match.ID, @@ -232,7 +234,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress return err } - // Update route status, sans backend + // With the route in hand, we now update its status routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ ID: route.ID, URI: route.URI, @@ -268,7 +270,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress return err } - // Update route status with backend ID + // 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, From e193b4772065c5a9809fefbf192163a68bf3eb61 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Fri, 2 Jun 2023 10:48:15 -0600 Subject: [PATCH 06/17] spacing --- internal/controllers/httpsedge_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 18479ed6..4ed5f8e4 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -210,7 +210,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress // Now we go ahead and create the route if it doesn't exist, or find the existing route. // It's important to note here that we are intentionally ommiting the `route.Backend` for new routes, and // removing it from existing routes. This is because we depend on the route modules being successfully applied - // before we apply the new route. This ensures that any route with a backend is considered to be successfully configured. + // before we apply the new route. This ensures that any route with a backend is considered to be successfully 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) From cf22df6d18e48b6779be9bbfe017999810ca9684 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Fri, 2 Jun 2023 10:50:21 -0600 Subject: [PATCH 07/17] more wording --- internal/controllers/httpsedge_controller.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 4ed5f8e4..9224ef11 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -234,7 +234,6 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress return err } - // With the route in hand, we now update its status routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ ID: route.ID, URI: route.URI, @@ -242,7 +241,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress MatchType: route.MatchType, } - // With the route now created and backend disabled, we now attempt to apply module updates for a given route + // With the route properly staged, we now attempt to apply its module updates r.Log.Info("Updating 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()) From bacaf316c62de486048e36d7c6acf7ffee03fc68 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Fri, 2 Jun 2023 11:22:44 -0600 Subject: [PATCH 08/17] more wording --- internal/controllers/httpsedge_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 9224ef11..b046aae4 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -209,8 +209,8 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress var route *ngrok.HTTPSEdgeRoute // Now we go ahead and create the route if it doesn't exist, or find the existing route. // It's important to note here that we are intentionally ommiting the `route.Backend` for new routes, and - // removing it from existing routes. This is because we depend on the route modules being successfully applied - // before we apply the new route. This ensures that any route with a backend is considered to be successfully configured. + // removing it from existing 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) From 9d6bc65918d53860b8b5235d3ef0aad434b0c64e Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Tue, 6 Jun 2023 12:56:23 -0600 Subject: [PATCH 09/17] No longer remove backend from existing routes; now raises new config error type that does not re-reconcile --- README.md | 2 +- internal/controllers/httpsedge_controller.go | 55 +++++++++++--------- internal/errors/errors.go | 18 +++++++ 3 files changed, 50 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index faafdc2e..fdb72fa5 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@

-> _*Warning*_: Currently this project has an issue being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/208) which can cause the controller to provide ingress to a service even if it failed to configure an authentication module like Oauth on a particular route due to a configuration or intermittent error. Additionally, even when there aren't errors, the controller first sets up ingress and then applies route modules like Oauth immediately after, but there is a brief period where ingress without authentication is provided. This issue is being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/219). Until this issue is resolved, it's not recommended to use this with security sensitive applications. +> _*Warning*_: The controller first sets up ingress and then applies route modules like Oauth immediately after, but there is a brief period where ingress without authentication is provided. This issue is being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/219). Until this issue is resolved, it's not recommended to use this with security sensitive applications. # ngrok Ingress Controller for Kubernetes diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index b046aae4..086167cb 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -117,7 +117,15 @@ 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) + switch { + // Configuration errors can't be resolved, so don't requeue + case errors.IsErrInvalidConfiguration(err): + return ctrl.Result{}, nil + default: + // No error or is a retryable error, so return as normal + return ctrl.Result{}, err + } } func (r *HTTPSEdgeReconciler) reconcileEdge(ctx context.Context, edge *ingressv1alpha1.HTTPSEdge) error { @@ -200,17 +208,16 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress r.Recorder.Eventf(edge, v1.EventTypeWarning, "FailedValidate", "Could not validate ip restriction: %v", err) continue } - - return err + return errors.NewErrInvalidConfiguration(err.Error()) } } match := r.getMatchingRouteFromEdgeStatus(edge, routeSpec) var route *ngrok.HTTPSEdgeRoute - // Now we go ahead and create the route if it doesn't exist, or find the existing route. - // It's important to note here that we are intentionally ommiting the `route.Backend` for new routes, and - // removing it from existing 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. + // 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) @@ -220,32 +227,31 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress MatchType: routeSpec.MatchType, } route, err = edgeRoutes.Create(ctx, req) + + // Update status for newly created route + routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ + ID: route.ID, + URI: route.URI, + Match: route.Match, + MatchType: route.MatchType, + } } else { - r.Log.Info("Route found, disabling backend to prepare for module updates", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) - req := &ngrok.HTTPSEdgeRouteUpdate{ - EdgeID: edge.Status.ID, - ID: match.ID, - Match: routeSpec.Match, - MatchType: routeSpec.MatchType, + req := &ngrok.EdgeRouteItem{ + ID: match.ID, + EdgeID: edge.Status.ID, } - route, err = edgeRoutes.Update(ctx, req) + route, err = edgeRoutes.Get(ctx, req) } if err != nil { return err } - 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 - r.Log.Info("Updating route modules", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) + // 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 + return errors.NewErrInvalidConfiguration(err.Error()) } // The route modules were successfully applied, so now we update the route with its specified backend @@ -253,8 +259,9 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress if err != nil { return err } - r.Log.Info("Setting route backend", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType, "backendID", backend.ID) + 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: match.ID, diff --git a/internal/errors/errors.go b/internal/errors/errors.go index f12e6f7a..df5eb80f 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -135,3 +135,21 @@ func IsErrMissingRequiredSecret(err error) bool { _, ok := err.(ErrMissingRequiredSecret) return ok } + +type ErrInvalidConfiguration struct { + message string +} + +func NewErrInvalidConfiguration(message string) ErrInvalidConfiguration { + return ErrInvalidConfiguration{message: message} +} + +func (e ErrInvalidConfiguration) Error() string { + return fmt.Sprintf("invalid configuration: %s", e.message) +} + +// IsErrInvalidConfiguration: Reflect: returns true if the error is a ErrInvalidConfiguration +func IsErrInvalidConfiguration(err error) bool { + _, ok := err.(ErrInvalidConfiguration) + return ok +} From cba91d65649357ff3338dcfc9327fe9c1241be21 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Tue, 6 Jun 2023 13:26:25 -0600 Subject: [PATCH 10/17] wrong ID when creating new route --- internal/controllers/httpsedge_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 086167cb..fc5b3a6a 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -122,8 +122,8 @@ func (r *HTTPSEdgeReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Configuration errors can't be resolved, so don't requeue case errors.IsErrInvalidConfiguration(err): return ctrl.Result{}, nil + // No error or is a retryable error, so return as normal default: - // No error or is a retryable error, so return as normal return ctrl.Result{}, err } } @@ -264,7 +264,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress // TODO: Do an entropy check here to avoid unnecessary updates req := &ngrok.HTTPSEdgeRouteUpdate{ EdgeID: edge.Status.ID, - ID: match.ID, + ID: route.ID, Match: routeSpec.Match, MatchType: routeSpec.MatchType, Backend: &ngrok.EndpointBackendMutate{ From b366e8df64b3f5821cfb6e5eaa72dd689fec2ae3 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 7 Jun 2023 09:23:53 -0600 Subject: [PATCH 11/17] undo README changes --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index fdb72fa5..faafdc2e 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,7 @@

-> _*Warning*_: The controller first sets up ingress and then applies route modules like Oauth immediately after, but there is a brief period where ingress without authentication is provided. This issue is being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/219). Until this issue is resolved, it's not recommended to use this with security sensitive applications. +> _*Warning*_: Currently this project has an issue being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/208) which can cause the controller to provide ingress to a service even if it failed to configure an authentication module like Oauth on a particular route due to a configuration or intermittent error. Additionally, even when there aren't errors, the controller first sets up ingress and then applies route modules like Oauth immediately after, but there is a brief period where ingress without authentication is provided. This issue is being tracked [here](https://github.com/ngrok/kubernetes-ingress-controller/issues/219). Until this issue is resolved, it's not recommended to use this with security sensitive applications. # ngrok Ingress Controller for Kubernetes From 0b96678b9dd149e8f97a40721d77f2d8c06d853d Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 7 Jun 2023 12:10:25 -0600 Subject: [PATCH 12/17] PR comments --- internal/controllers/httpsedge_controller.go | 22 ++++++++++---------- internal/errors/errors.go | 4 ++-- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index fc5b3a6a..f1170141 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -208,7 +208,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress r.Recorder.Eventf(edge, v1.EventTypeWarning, "FailedValidate", "Could not validate ip restriction: %v", err) continue } - return errors.NewErrInvalidConfiguration(err.Error()) + return err } } @@ -227,14 +227,6 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress MatchType: routeSpec.MatchType, } route, err = edgeRoutes.Create(ctx, req) - - // Update status for newly created route - routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ - ID: route.ID, - URI: route.URI, - Match: route.Match, - MatchType: route.MatchType, - } } else { req := &ngrok.EdgeRouteItem{ ID: match.ID, @@ -246,12 +238,20 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress 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 errors.NewErrInvalidConfiguration(err.Error()) + return err } // The route modules were successfully applied, so now we update the route with its specified backend @@ -735,7 +735,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOAuth(ctx context.Context, route *n } if module == nil { - return fmt.Errorf("no OAuth provider configured") + return errors.NewErrInvalidConfiguration("no OAuth provider found") } if reflect.DeepEqual(module, route.OAuth) { diff --git a/internal/errors/errors.go b/internal/errors/errors.go index df5eb80f..c3a92c7e 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -140,8 +140,8 @@ type ErrInvalidConfiguration struct { message string } -func NewErrInvalidConfiguration(message string) ErrInvalidConfiguration { - return ErrInvalidConfiguration{message: message} +func NewErrInvalidConfiguration(cause error) ErrInvalidConfiguration { + return ErrInvalidConfiguration{message: cause.Error()} } func (e ErrInvalidConfiguration) Error() string { From f1f432a794473f29c6909793f303c249f9d5bf48 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 7 Jun 2023 12:17:18 -0600 Subject: [PATCH 13/17] Only ignore 400 HTTP status codes as unretryable --- internal/controllers/httpsedge_controller.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index f1170141..20c78510 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -251,6 +251,17 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress 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()) + + codes := make([]int, 0, 100) + for i := 400; i <= 500; i++ { + codes = append(codes, i) + } + + // Ignore 400 error codes as unretryable + if ngrok.IsErrorCode(err, codes...) { + return errors.NewErrInvalidConfiguration(err) + } + return err } @@ -735,7 +746,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOAuth(ctx context.Context, route *n } if module == nil { - return errors.NewErrInvalidConfiguration("no OAuth provider found") + return errors.NewErrInvalidConfiguration(fmt.Errorf("no OAuth provider found")) } if reflect.DeepEqual(module, route.OAuth) { From c4886f64794d67c4be19292d08d66e288f32761d Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 7 Jun 2023 12:55:08 -0600 Subject: [PATCH 14/17] Add lib function to check for retryable errors --- internal/controllers/httpsedge_controller.go | 12 +++--------- internal/errors/errors.go | 15 +++++++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 20c78510..1ba5a0b6 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -252,17 +252,11 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress if err := routeModuleUpdater.updateModulesForRoute(ctx, route, &routeSpec); err != nil { r.Recorder.Event(edge, v1.EventTypeWarning, "RouteModuleUpdateFailed", err.Error()) - codes := make([]int, 0, 100) - for i := 400; i <= 500; i++ { - codes = append(codes, i) - } - - // Ignore 400 error codes as unretryable - if ngrok.IsErrorCode(err, codes...) { + if errors.IsRetryable(err) { + return err + } else { return errors.NewErrInvalidConfiguration(err) } - - return err } // The route modules were successfully applied, so now we update the route with its specified backend diff --git a/internal/errors/errors.go b/internal/errors/errors.go index c3a92c7e..3793584f 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -4,6 +4,7 @@ import ( "fmt" "strings" + "github.com/ngrok/ngrok-api-go/v5" netv1 "k8s.io/api/networking/v1" ) @@ -153,3 +154,17 @@ func IsErrInvalidConfiguration(err error) bool { _, ok := err.(ErrInvalidConfiguration) return ok } + +func IsRetryable(err error) bool { + if IsErrInvalidConfiguration(err) { + return false + } + + // Ignore 400 error codes as unretryable + codes := make([]int, 0, 100) + for i := 400; i <= 500; i++ { + codes = append(codes, i) + } + + return !ngrok.IsErrorCode(err, codes...) +} From 1f8d735d4e6bd0eecdb2c3c9669da602fc252ece Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Wed, 7 Jun 2023 13:03:22 -0600 Subject: [PATCH 15/17] Better reconciliaton error checking --- internal/controllers/httpsedge_controller.go | 18 +++++------------- internal/errors/errors.go | 6 +++++- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 1ba5a0b6..22146fec 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -118,13 +118,10 @@ func (r *HTTPSEdgeReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } err := r.reconcileEdge(ctx, edge) - switch { - // Configuration errors can't be resolved, so don't requeue - case errors.IsErrInvalidConfiguration(err): - return ctrl.Result{}, nil - // No error or is a retryable error, so return as normal - default: + if errors.IsErrorReconcilable(err) { return ctrl.Result{}, err + } else { + return ctrl.Result{}, nil } } @@ -251,12 +248,7 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress 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()) - - if errors.IsRetryable(err) { - return err - } else { - return errors.NewErrInvalidConfiguration(err) - } + return err } // The route modules were successfully applied, so now we update the route with its specified backend @@ -740,7 +732,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOAuth(ctx context.Context, route *n } if module == nil { - return errors.NewErrInvalidConfiguration(fmt.Errorf("no OAuth provider found")) + return errors.NewErrInvalidConfiguration(fmt.Errorf("no OAuth provider configured")) } if reflect.DeepEqual(module, route.OAuth) { diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 3793584f..05eab857 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -155,7 +155,11 @@ func IsErrInvalidConfiguration(err error) bool { return ok } -func IsRetryable(err error) bool { +func IsErrorReconcilable(err error) bool { + if err == nil { + return true + } + if IsErrInvalidConfiguration(err) { return false } From 2ad0ddc78b853c808cd4a871647f2adee90da101 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Thu, 8 Jun 2023 07:32:05 -0600 Subject: [PATCH 16/17] Better error casting --- internal/errors/errors.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 05eab857..0d2ebe70 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -1,6 +1,7 @@ package errors import ( + "errors" "fmt" "strings" @@ -149,26 +150,19 @@ func (e ErrInvalidConfiguration) Error() string { return fmt.Sprintf("invalid configuration: %s", e.message) } -// IsErrInvalidConfiguration: Reflect: returns true if the error is a ErrInvalidConfiguration -func IsErrInvalidConfiguration(err error) bool { - _, ok := err.(ErrInvalidConfiguration) - return ok -} - func IsErrorReconcilable(err error) bool { if err == nil { return true } - if IsErrInvalidConfiguration(err) { + if errors.As(err, &ErrInvalidConfiguration{}) { return false } - // Ignore 400 error codes as unretryable - codes := make([]int, 0, 100) - for i := 400; i <= 500; i++ { - codes = append(codes, i) + var nerr *ngrok.Error + if errors.As(err, &nerr) { + return nerr.StatusCode >= 500 } - return !ngrok.IsErrorCode(err, codes...) + return true } From 7b8ca27b6be0c9faa85f6896322ed79014d66da0 Mon Sep 17 00:00:00 2001 From: Carl Amko Date: Thu, 8 Jun 2023 07:34:25 -0600 Subject: [PATCH 17/17] Refactor invalid config error struct to look more like native error --- internal/errors/errors.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/internal/errors/errors.go b/internal/errors/errors.go index 0d2ebe70..a4da5b9a 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -139,15 +139,19 @@ func IsErrMissingRequiredSecret(err error) bool { } type ErrInvalidConfiguration struct { - message string + cause error } -func NewErrInvalidConfiguration(cause error) ErrInvalidConfiguration { - return ErrInvalidConfiguration{message: cause.Error()} +func NewErrInvalidConfiguration(cause error) error { + return ErrInvalidConfiguration{cause: cause} } func (e ErrInvalidConfiguration) Error() string { - return fmt.Sprintf("invalid configuration: %s", e.message) + return fmt.Sprintf("invalid configuration: %s", e.cause.Error()) +} + +func (e ErrInvalidConfiguration) Unwrap() error { + return e.cause } func IsErrorReconcilable(err error) bool {