diff --git a/internal/controllers/httpsedge_controller.go b/internal/controllers/httpsedge_controller.go index 22146fec..27a08ed6 100644 --- a/internal/controllers/httpsedge_controller.go +++ b/internal/controllers/httpsedge_controller.go @@ -235,6 +235,14 @@ func (r *HTTPSEdgeReconciler) reconcileRoutes(ctx context.Context, edge *ingress return err } + if isMigratingAuthProviders(route, &routeSpec) { + r.Log.Info("Route is migrating auth types. Taking offline before updating", "edgeID", edge.Status.ID, "match", routeSpec.Match, "matchType", routeSpec.MatchType) + if err := r.takeOfflineWithoutAuth(ctx, route); err != nil { + r.Recorder.Event(edge, v1.EventTypeWarning, "RouteTakeOfflineFailed", err.Error()) + return err + } + } + // Update status for newly created route routeStatuses[i] = ingressv1alpha1.HTTPSEdgeRouteStatus{ ID: route.ID, @@ -498,7 +506,7 @@ func (u *edgeRouteModuleUpdater) updateModulesForRoute(ctx context.Context, rout return nil } -func (u *edgeRouteModuleUpdater) edgeRouteItem(route *ngrok.HTTPSEdgeRoute) *ngrok.EdgeRouteItem { +func edgeRouteItem(route *ngrok.HTTPSEdgeRoute) *ngrok.EdgeRouteItem { return &ngrok.EdgeRouteItem{ EdgeID: route.EdgeID, ID: route.ID, @@ -517,7 +525,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteCircuitBreaker(ctx context.Context, return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } module := ngrok.EndpointCircuitBreaker{ @@ -553,7 +561,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteCompression(ctx context.Context, ro return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } _, err := client.Replace(ctx, &ngrok.EdgeRouteCompressionReplace{ @@ -576,7 +584,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteIPRestriction(ctx context.Context, return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } policyIds, err := u.ipPolicyResolver.resolveIPPolicyNamesorIds(ctx, u.edge.Namespace, ipRestriction.IPPolicies) @@ -622,7 +630,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteRequestHeaders(ctx context.Context, return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } module := ngrok.EndpointRequestHeaders{} @@ -659,7 +667,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteResponseHeaders(ctx context.Context return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } module := ngrok.EndpointResponseHeaders{} @@ -693,7 +701,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOAuth(ctx context.Context, route *n return nil } - return oauthClient.Delete(ctx, u.edgeRouteItem(route)) + return oauthClient.Delete(ctx, edgeRouteItem(route)) } var module *ngrok.EndpointOAuth @@ -758,7 +766,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteOIDC(ctx context.Context, route *ng return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } clientSecret, err := u.getSecret(ctx, oidc.ClientSecret) @@ -803,7 +811,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteSAML(ctx context.Context, route *ng return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } module := ngrok.EndpointSAMLMutate{ @@ -842,7 +850,7 @@ func (u *edgeRouteModuleUpdater) setEdgeRouteWebhookVerification(ctx context.Con return nil } - return client.Delete(ctx, u.edgeRouteItem(route)) + return client.Delete(ctx, edgeRouteItem(route)) } // Some WebhookVerification providers don't require a secret, @@ -888,3 +896,67 @@ type OAuthProvider interface { ClientSecretKeyRef() *ingressv1alpha1.SecretKeyRef ToNgrok(*string) *ngrok.EndpointOAuth } + +// isMigratingAuthProviders returns true if the auth provider is changing +// It takes in the current ngrok.HTTPSEdgeRoute and the desired ingressv1alpha1.HTTPSEdgeRouteSpec +// if the current and desired have different auth types (OAuth, OIDC, SAML), it returns true +func isMigratingAuthProviders(current *ngrok.HTTPSEdgeRoute, desired *ingressv1alpha1.HTTPSEdgeRouteSpec) bool { + modifiedAuthTypes := 0 + if (current.OAuth == nil && desired.OAuth != nil) || (current.OAuth != nil && desired.OAuth == nil) { + modifiedAuthTypes += 1 + } + if (current.OIDC == nil && desired.OIDC != nil) || (current.OIDC != nil && desired.OIDC == nil) { + modifiedAuthTypes += 1 + } + if (current.SAML == nil && desired.SAML != nil) || (current.SAML != nil && desired.SAML == nil) { + modifiedAuthTypes += 1 + } + + // Each check above tells if that auth type is being added or removed in some way. + // If it happens 0 times, no modifications happened. If it happens only once, then its + // just being added or removed, so there is no chance of conflict. But if multiple are triggered + // then it must be moving from 1 type to another, so we can return true. + return modifiedAuthTypes > 1 +} + +// takeOfflineWithoutAuth takes an ngrok.HTTPSEdgeRoute and will remove the backed first. +// It will save the route without the backend so its offline. Then for each of the auth types (OAuth, OIDC, SAML) +// it will try to remove them if nil. If removed, it will set the route to nil for that auth type. +func (r *HTTPSEdgeReconciler) takeOfflineWithoutAuth(ctx context.Context, route *ngrok.HTTPSEdgeRoute) error { + routeUpdate := &ngrok.HTTPSEdgeRouteUpdate{ + EdgeID: route.EdgeID, + ID: route.ID, + Match: route.Match, + MatchType: route.MatchType, + Backend: nil, + } + routeClientSet := r.NgrokClientset.EdgeModules().HTTPS().Routes() + + route, err := r.NgrokClientset.HTTPSEdgeRoutes().Update(ctx, routeUpdate) + if err != nil { + return err + } + + if route.OAuth != nil { + if err := routeClientSet.OAuth().Delete(ctx, edgeRouteItem(route)); err != nil { + return err + } + route.OAuth = nil + } + + if route.OIDC != nil { + if err := routeClientSet.OIDC().Delete(ctx, edgeRouteItem(route)); err != nil { + return err + } + route.OIDC = nil + } + + if route.SAML != nil { + if err := routeClientSet.SAML().Delete(ctx, edgeRouteItem(route)); err != nil { + return err + } + route.SAML = nil + } + + return nil +} diff --git a/internal/controllers/httpsedge_controller_test.go b/internal/controllers/httpsedge_controller_test.go new file mode 100644 index 00000000..fe9ddb6f --- /dev/null +++ b/internal/controllers/httpsedge_controller_test.go @@ -0,0 +1,38 @@ +package controllers + +import ( + "testing" + + ingressv1alpha1 "github.com/ngrok/kubernetes-ingress-controller/api/v1alpha1" + "github.com/ngrok/ngrok-api-go/v5" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestControllers(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Controllers package Test Suite") +} + +var _ = Describe("HTTPSEdgeController", func() { + DescribeTable("isMigratingAuthProviders", func(current *ngrok.HTTPSEdgeRoute, desired *ingressv1alpha1.HTTPSEdgeRouteSpec, expected bool) { + Expect(isMigratingAuthProviders(current, desired)).To(Equal(expected)) + }, + Entry("no auth", &ngrok.HTTPSEdgeRoute{}, &ingressv1alpha1.HTTPSEdgeRouteSpec{}, false), + Entry("Same Auth: Oauth", &ngrok.HTTPSEdgeRoute{OAuth: &ngrok.EndpointOAuth{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OAuth: &ingressv1alpha1.EndpointOAuth{}}, false), + Entry("Same Auth: SAML", &ngrok.HTTPSEdgeRoute{SAML: &ngrok.EndpointSAML{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{SAML: &ingressv1alpha1.EndpointSAML{}}, false), + Entry("Same Auth: OIDC", &ngrok.HTTPSEdgeRoute{OIDC: &ngrok.EndpointOIDC{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OIDC: &ingressv1alpha1.EndpointOIDC{}}, false), + Entry("Added Oauth", &ngrok.HTTPSEdgeRoute{}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OAuth: &ingressv1alpha1.EndpointOAuth{}}, false), + Entry("Added SAML", &ngrok.HTTPSEdgeRoute{}, &ingressv1alpha1.HTTPSEdgeRouteSpec{SAML: &ingressv1alpha1.EndpointSAML{}}, false), + Entry("Added OIDC", &ngrok.HTTPSEdgeRoute{}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OIDC: &ingressv1alpha1.EndpointOIDC{}}, false), + Entry("Removed Oauth", &ngrok.HTTPSEdgeRoute{OAuth: &ngrok.EndpointOAuth{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{}, false), + Entry("Removed SAML", &ngrok.HTTPSEdgeRoute{SAML: &ngrok.EndpointSAML{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{}, false), + Entry("Removed OIDC", &ngrok.HTTPSEdgeRoute{OIDC: &ngrok.EndpointOIDC{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{}, false), + Entry("Removed Oauth and Added Saml", &ngrok.HTTPSEdgeRoute{OAuth: &ngrok.EndpointOAuth{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{SAML: &ingressv1alpha1.EndpointSAML{}}, true), + Entry("Removed Oauth and Added OIDC", &ngrok.HTTPSEdgeRoute{OAuth: &ngrok.EndpointOAuth{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OIDC: &ingressv1alpha1.EndpointOIDC{}}, true), + Entry("Removed SAML and Added Oauth", &ngrok.HTTPSEdgeRoute{SAML: &ngrok.EndpointSAML{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OAuth: &ingressv1alpha1.EndpointOAuth{}}, true), + Entry("Removed SAML and Added OIDC", &ngrok.HTTPSEdgeRoute{SAML: &ngrok.EndpointSAML{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OIDC: &ingressv1alpha1.EndpointOIDC{}}, true), + Entry("Removed OIDC and Added Oauth", &ngrok.HTTPSEdgeRoute{OIDC: &ngrok.EndpointOIDC{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{OAuth: &ingressv1alpha1.EndpointOAuth{}}, true), + Entry("Removed OIDC and Added SAML", &ngrok.HTTPSEdgeRoute{OIDC: &ngrok.EndpointOIDC{}}, &ingressv1alpha1.HTTPSEdgeRouteSpec{SAML: &ingressv1alpha1.EndpointSAML{}}, true), + ) +})