From 0d1295218a8d069d6f26efbdedd0497128634089 Mon Sep 17 00:00:00 2001 From: "D. Ryan Hild" Date: Tue, 16 Apr 2024 18:06:41 -0700 Subject: [PATCH] Fix VirtualRouter route deletion sequence (#768) * Remove routes before the matching listener This addresses a few rough edges with updating a VirtualRouter. After this change, it is possible to update the listener protocol on the CRD in one step. It is also possible to remove the listener at the same time as the routes which use that listener. * Fix a nil panic error When running this against a test cluster, discovered that the logic must filter to only names with SDK routes before adding them to a deletion list. * Add unit test coverage Adds unit test coverage for the new functions. A possible nil pointer dereference is fixed, which could occur if port matching is not utilized. * Remove unneeded API calls The API call to describe the route is not needed, as the route ref contains all the information needed to delete routes. --- pkg/virtualrouter/resource_manager.go | 4 + pkg/virtualrouter/routes_manager.go | 75 ++++ pkg/virtualrouter/routes_manager_test.go | 442 +++++++++++++++++++++++ 3 files changed, 521 insertions(+) diff --git a/pkg/virtualrouter/resource_manager.go b/pkg/virtualrouter/resource_manager.go index 7d4580a0..0bdd690e 100644 --- a/pkg/virtualrouter/resource_manager.go +++ b/pkg/virtualrouter/resource_manager.go @@ -86,6 +86,10 @@ func (m *defaultResourceManager) Reconcile(ctx context.Context, vr *appmesh.Virt return err } } else { + err = m.routesManager.remove(ctx, ms, sdkVR, vr) + if err != nil { + return err + } sdkVR, err = m.updateSDKVirtualRouter(ctx, sdkVR, vr) if err != nil { return err diff --git a/pkg/virtualrouter/routes_manager.go b/pkg/virtualrouter/routes_manager.go index e1495885..d4fd0022 100644 --- a/pkg/virtualrouter/routes_manager.go +++ b/pkg/virtualrouter/routes_manager.go @@ -24,6 +24,8 @@ import ( type routesManager interface { // create will create routes on AppMesh virtualRouter to match k8s virtualRouter spec. create(ctx context.Context, ms *appmesh.Mesh, vr *appmesh.VirtualRouter, vnByRefHash map[types.NamespacedName]*appmesh.VirtualNode) (map[string]*appmeshsdk.RouteData, error) + // remove will remove old routes on AppMesh virtualRouter to match k8s virtualRouter spec. + remove(ctx context.Context, ms *appmesh.Mesh, sdkVR *appmeshsdk.VirtualRouterData, vr *appmesh.VirtualRouter) error // update will update routes on AppMesh virtualRouter to match k8s virtualRouter spec. update(ctx context.Context, ms *appmesh.Mesh, vr *appmesh.VirtualRouter, vnByRefHash map[types.NamespacedName]*appmesh.VirtualNode) (map[string]*appmeshsdk.RouteData, error) // cleanup will cleanup routes on AppMesh virtualRouter @@ -47,6 +49,21 @@ func (m *defaultRoutesManager) create(ctx context.Context, ms *appmesh.Mesh, vr return m.reconcile(ctx, ms, vr, vnByKey, vr.Spec.Routes, nil) } +func (m *defaultRoutesManager) remove(ctx context.Context, ms *appmesh.Mesh, sdkVR *appmeshsdk.VirtualRouterData, vr *appmesh.VirtualRouter) error { + sdkRouteRefs, err := m.listSDKRouteRefs(ctx, ms, vr) + if err != nil { + return err + } + // Only reconcile routes which need to be removed before we remove the corresponding listener + taintedRefs := taintedSDKRouteRefs(vr.Spec.Routes, sdkVR, sdkRouteRefs) + for _, sdkRouteRef := range taintedRefs { + if err = m.deleteSDKRouteByRef(ctx, sdkRouteRef); err != nil { + return err + } + } + return err +} + func (m *defaultRoutesManager) update(ctx context.Context, ms *appmesh.Mesh, vr *appmesh.VirtualRouter, vnByKey map[types.NamespacedName]*appmesh.VirtualNode) (map[string]*appmeshsdk.RouteData, error) { sdkRouteRefs, err := m.listSDKRouteRefs(ctx, ms, vr) if err != nil { @@ -206,6 +223,23 @@ func (m *defaultRoutesManager) deleteSDKRoute(ctx context.Context, sdkRoute *app return nil } +func (m *defaultRoutesManager) deleteSDKRouteByRef(ctx context.Context, sdkRouteRef *appmeshsdk.RouteRef) error { + _, err := m.appMeshSDK.DeleteRouteWithContext(ctx, &appmeshsdk.DeleteRouteInput{ + MeshName: sdkRouteRef.MeshName, + MeshOwner: sdkRouteRef.MeshOwner, + VirtualRouterName: sdkRouteRef.VirtualRouterName, + RouteName: sdkRouteRef.RouteName, + }) + if err != nil { + var awsErr awserr.Error + if ok := errors.As(err, &awsErr); ok && awsErr.Code() == "NotFoundException" { + return nil + } + return err + } + return nil +} + type routeAndSDKRouteRef struct { route appmesh.Route sdkRouteRef *appmeshsdk.RouteRef @@ -249,6 +283,47 @@ func matchRoutesAgainstSDKRouteRefs(routes []appmesh.Route, sdkRouteRefs []*appm return matchedRouteAndSDKRouteRef, unmatchedRoutes, unmatchedSDKRouteRefs } +// taintedSDKRouteRefs returns the routes which need to be deleted before the corresponding listener can be updated or deleted. +// This includes both routes which are no longer defined by the CRD and routes where the protocol has changed but not the port. +func taintedSDKRouteRefs(routes []appmesh.Route, sdkVR *appmeshsdk.VirtualRouterData, sdkRouteRefs []*appmeshsdk.RouteRef) []*appmeshsdk.RouteRef { + routeByName := make(map[string]appmesh.Route, len(routes)) + sdkRouteRefByName := make(map[string]*appmeshsdk.RouteRef, len(sdkRouteRefs)) + sdkListenerByPort := make(map[int64]appmesh.PortProtocol, len(sdkVR.Spec.Listeners)) + for _, route := range routes { + routeByName[route.Name] = route + } + for _, sdkRouteRef := range sdkRouteRefs { + sdkRouteRefByName[aws.StringValue(sdkRouteRef.RouteName)] = sdkRouteRef + } + for _, sdkListener := range sdkVR.Spec.Listeners { + sdkListenerByPort[aws.Int64Value(sdkListener.PortMapping.Port)] = appmesh.PortProtocol(aws.StringValue(sdkListener.PortMapping.Protocol)) + } + routeNameSet := sets.StringKeySet(routeByName) + sdkRouteRefNameSet := sets.StringKeySet(sdkRouteRefByName) + matchedNameSet := routeNameSet.Intersection(sdkRouteRefNameSet) + unmatchedSDKRouteRefNameSet := sdkRouteRefNameSet.Difference(routeNameSet) + + for _, name := range matchedNameSet.List() { + route := routeByName[name] + if route.TCPRoute != nil && route.TCPRoute.Match != nil && route.TCPRoute.Match.Port != nil && sdkListenerByPort[aws.Int64Value(route.TCPRoute.Match.Port)] != appmesh.PortProtocolTCP { + unmatchedSDKRouteRefNameSet.Insert(route.Name) + } else if route.GRPCRoute != nil && route.GRPCRoute.Match.Port != nil && sdkListenerByPort[aws.Int64Value(route.GRPCRoute.Match.Port)] != appmesh.PortProtocolGRPC { + unmatchedSDKRouteRefNameSet.Insert(route.Name) + } else if route.HTTP2Route != nil && route.HTTP2Route.Match.Port != nil && sdkListenerByPort[aws.Int64Value(route.HTTP2Route.Match.Port)] != appmesh.PortProtocolHTTP2 { + unmatchedSDKRouteRefNameSet.Insert(route.Name) + } else if route.HTTPRoute != nil && route.HTTPRoute.Match.Port != nil && sdkListenerByPort[aws.Int64Value(route.HTTPRoute.Match.Port)] != appmesh.PortProtocolHTTP { + unmatchedSDKRouteRefNameSet.Insert(route.Name) + } + } + + unmatchedSDKRouteRefs := make([]*appmeshsdk.RouteRef, 0, len(unmatchedSDKRouteRefNameSet)) + for _, name := range unmatchedSDKRouteRefNameSet.List() { + unmatchedSDKRouteRefs = append(unmatchedSDKRouteRefs, sdkRouteRefByName[name]) + } + + return unmatchedSDKRouteRefs +} + func BuildSDKRouteSpec(vr *appmesh.VirtualRouter, route appmesh.Route, vnByKey map[types.NamespacedName]*appmesh.VirtualNode) (*appmeshsdk.RouteSpec, error) { sdkVNRefConvertFunc := references.BuildSDKVirtualNodeReferenceConvertFunc(vr, vnByKey) converter := conversion.NewConverter(conversion.DefaultNameFunc) diff --git a/pkg/virtualrouter/routes_manager_test.go b/pkg/virtualrouter/routes_manager_test.go index ab943075..de309e17 100644 --- a/pkg/virtualrouter/routes_manager_test.go +++ b/pkg/virtualrouter/routes_manager_test.go @@ -1,11 +1,16 @@ package virtualrouter import ( + "context" + "fmt" "testing" appmesh "github.com/aws/aws-app-mesh-controller-for-k8s/apis/appmesh/v1beta2" + "github.com/aws/aws-app-mesh-controller-for-k8s/pkg/aws/services" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/request" appmeshsdk "github.com/aws/aws-sdk-go/service/appmesh" + "github.com/go-logr/logr" "github.com/stretchr/testify/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -773,3 +778,440 @@ func Test_BuildSDKRouteSpec(t *testing.T) { }) } } + +// taintedSDKRouteRefs returns the routes which need to be deleted before the corresponding listener can be updated or deleted. +// This includes both routes which are no longer defined by the CRD and routes where the protocol has changed but not the port. +func Test_taintedSDKRouteRefs(t *testing.T) { + type args struct { + routes []appmesh.Route + sdkVR *appmeshsdk.VirtualRouterData + sdkRouteRefs []*appmeshsdk.RouteRef + } + tests := []struct { + name string + args args + wantTaintedRouteRefs []*appmeshsdk.RouteRef + }{ + { + name: "routes without port mapping do not get removed", + args: args{ + routes: []appmesh.Route{ + { + Name: "route-1", + }, + { + Name: "route-2", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Prefix: aws.String("/"), + }, + }, + }, + { + Name: "route-3", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Method: aws.String("POST"), + }, + }, + }, + { + Name: "route-4", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Headers: []appmesh.HTTPRouteHeader{ + { + Name: "X-Backend", + Match: &appmesh.HeaderMatchMethod{ + Exact: aws.String("widgets"), + }, + }, + }, + }, + }, + }, + { + Name: "route-5", + }, + }, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8080), + Protocol: aws.String("HTTP"), + }, + }, + }, + }, + }, + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + { + RouteName: aws.String("route-3"), + }, + { + RouteName: aws.String("route-4"), + }, + }, + }, + wantTaintedRouteRefs: []*appmeshsdk.RouteRef{}, + }, + { + name: "routes removed from CRD are removed", + args: args{ + routes: []appmesh.Route{ + { + Name: "route-1", + }, + }, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8080), + Protocol: aws.String("HTTP"), + }, + }, + }, + }, + }, + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + }, + }, + wantTaintedRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-2"), + }, + }, + }, + { + name: "routes with port change are removed", + args: args{ + routes: []appmesh.Route{ + { + Name: "route-1", + HTTP2Route: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(9000), + }, + }, + }, + { + Name: "route-2", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(4000), + }, + }, + }, + }, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(9000), + Protocol: aws.String("http2"), + }, + }, + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(5000), + Protocol: aws.String("http"), + }, + }, + }, + }, + }, + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + }, + }, + wantTaintedRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-2"), + }, + }, + }, + { + name: "routes with protocol changed are removed", + args: args{ + routes: []appmesh.Route{ + { + Name: "route-1", + HTTP2Route: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(8001), + }, + }, + }, + { + Name: "route-2", + GRPCRoute: &appmesh.GRPCRoute{ + Match: appmesh.GRPCRouteMatch{ + Port: aws.Int64(8002), + }, + }, + }, + { + Name: "route-3", + TCPRoute: &appmesh.TCPRoute{ + Match: &appmesh.TCPRouteMatch{ + Port: aws.Int64(8003), + }, + }, + }, + { + Name: "route-4", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(8004), + }, + }, + }, + }, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8001), + Protocol: aws.String("tcp"), + }, + }, + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8002), + Protocol: aws.String("http"), + }, + }, + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8003), + Protocol: aws.String("http2"), + }, + }, + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8004), + Protocol: aws.String("grpc"), + }, + }, + }, + }, + }, + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + { + RouteName: aws.String("route-3"), + }, + { + RouteName: aws.String("route-4"), + }, + }, + }, + wantTaintedRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + { + RouteName: aws.String("route-3"), + }, + { + RouteName: aws.String("route-4"), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotTaintedRouteRefs := taintedSDKRouteRefs(tt.args.routes, tt.args.sdkVR, tt.args.sdkRouteRefs) + assert.Equal(t, tt.wantTaintedRouteRefs, gotTaintedRouteRefs) + }) + } +} + +func Test_defaultRoutesManager_remove(t *testing.T) { + type args struct { + ms *appmesh.Mesh + sdkVR *appmeshsdk.VirtualRouterData + vr *appmesh.VirtualRouter + } + tests := []struct { + name string + sdkRouteRefs []*appmeshsdk.RouteRef + args args + wantDeleteRoutes []*appmeshsdk.DeleteRouteInput + }{ + { + name: "preserve existing matching routes", + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + }, + args: args{ + ms: &appmesh.Mesh{}, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8000), + Protocol: aws.String("tcp"), + }, + }, + }, + }, + }, + vr: &appmesh.VirtualRouter{ + Spec: appmesh.VirtualRouterSpec{ + Routes: []appmesh.Route{ + { + Name: "route-1", + TCPRoute: &appmesh.TCPRoute{ + Match: &appmesh.TCPRouteMatch{ + Port: aws.Int64(8000), + }, + }, + }, + }, + }, + }, + }, + wantDeleteRoutes: []*appmeshsdk.DeleteRouteInput{}, + }, + { + name: "routes with changes are removed", + sdkRouteRefs: []*appmeshsdk.RouteRef{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + }, + args: args{ + ms: &appmesh.Mesh{}, + sdkVR: &appmeshsdk.VirtualRouterData{ + Spec: &appmeshsdk.VirtualRouterSpec{ + Listeners: []*appmeshsdk.VirtualRouterListener{ + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(8000), + Protocol: aws.String("tcp"), + }, + }, + { + PortMapping: &appmeshsdk.PortMapping{ + Port: aws.Int64(9000), + Protocol: aws.String("http"), + }, + }, + }, + }, + }, + vr: &appmesh.VirtualRouter{ + Spec: appmesh.VirtualRouterSpec{ + Routes: []appmesh.Route{ + { + Name: "route-1", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(8000), + }, + }, + }, + { + Name: "route-2", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(8080), + }, + }, + }, + }, + }, + }, + }, + wantDeleteRoutes: []*appmeshsdk.DeleteRouteInput{ + { + RouteName: aws.String("route-1"), + }, + { + RouteName: aws.String("route-2"), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &fakeAppMesh{ + existingRouteRefs: tt.sdkRouteRefs, + } + m := &defaultRoutesManager{ + appMeshSDK: f, + log: logr.Discard(), + } + + err := m.remove(context.Background(), tt.args.ms, tt.args.sdkVR, tt.args.vr) + + assert.NoError(t, err) + assert.ElementsMatch(t, tt.wantDeleteRoutes, f.deletedRoutes) + }) + } +} + +type fakeAppMesh struct { + services.AppMesh + + existingRouteRefs []*appmeshsdk.RouteRef + deletedRoutes []*appmeshsdk.DeleteRouteInput +} + +func (f *fakeAppMesh) ListRoutesPagesWithContext(_ aws.Context, _ *appmeshsdk.ListRoutesInput, callback func(*appmeshsdk.ListRoutesOutput, bool) bool, _ ...request.Option) error { + if len(f.existingRouteRefs) > 0 { + callback(&appmeshsdk.ListRoutesOutput{ + Routes: f.existingRouteRefs, + }, false) + } + return nil +} + +func (f *fakeAppMesh) DeleteRouteWithContext(_ aws.Context, params *appmeshsdk.DeleteRouteInput, _ ...request.Option) (*appmeshsdk.DeleteRouteOutput, error) { + f.deletedRoutes = append(f.deletedRoutes, params) + for _, ref := range f.existingRouteRefs { + if aws.StringValue(ref.RouteName) != aws.StringValue(params.RouteName) { + continue + } + return &appmeshsdk.DeleteRouteOutput{}, nil + } + return nil, fmt.Errorf("not found") +}