Skip to content

Commit

Permalink
Handle special case for changing auth types that causes an error duri… (
Browse files Browse the repository at this point in the history 
#259)

* Handle special case for changing auth types that causes an error during state transition

* remove comment

* slight refactor

* change if statement format
  • Loading branch information
@alex-bezek
alex-bezek authored Jun 28, 2023
1 parent 1e031d1 commit 8bc2331
Show file tree
Hide file tree
Showing 2 changed files with 120 additions and 10 deletions.
92 changes: 82 additions & 10 deletions internal/controllers/httpsedge_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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)
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
38 changes: 38 additions & 0 deletions internal/controllers/httpsedge_controller_test.go
View file
Original file line number Diff line number Diff line change
@@ -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),
)
})

0 comments on commit 8bc2331

Please sign in to comment.