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") +}