diff --git a/pkg/virtualrouter/routes_manager.go b/pkg/virtualrouter/routes_manager.go index 03a221e7..65693990 100644 --- a/pkg/virtualrouter/routes_manager.go +++ b/pkg/virtualrouter/routes_manager.go @@ -295,13 +295,13 @@ func taintedSDKRouteRefs(routes []appmesh.Route, sdkVR *appmeshsdk.VirtualRouter for _, name := range matchedNameSet.List() { route := routeByName[name] - if route.TCPRoute != nil && route.TCPRoute.Match != nil && sdkListenerByPort[aws.Int64Value(route.TCPRoute.Match.Port)] != appmesh.PortProtocolTCP { + 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 && sdkListenerByPort[aws.Int64Value(route.GRPCRoute.Match.Port)] != appmesh.PortProtocolGRPC { + } 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 && sdkListenerByPort[aws.Int64Value(route.HTTP2Route.Match.Port)] != appmesh.PortProtocolHTTP2 { + } 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 && sdkListenerByPort[aws.Int64Value(route.HTTPRoute.Match.Port)] != appmesh.PortProtocolHTTP { + } else if route.HTTPRoute != nil && route.HTTPRoute.Match.Port != nil && sdkListenerByPort[aws.Int64Value(route.HTTPRoute.Match.Port)] != appmesh.PortProtocolHTTP { unmatchedSDKRouteRefNameSet.Insert(route.Name) } } diff --git a/pkg/virtualrouter/routes_manager_test.go b/pkg/virtualrouter/routes_manager_test.go index ab943075..d958dcb3 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,462 @@ 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 + sdkRoutes []*appmeshsdk.RouteData + args args + wantDeleteRoutes []*appmeshsdk.DeleteRouteInput + }{ + { + name: "preserve existing unchanged routes", + sdkRoutes: []*appmeshsdk.RouteData{ + { + RouteName: aws.String("route-1"), + Spec: &appmeshsdk.RouteSpec{ + TcpRoute: &appmeshsdk.TcpRoute{ + Match: &appmeshsdk.TcpRouteMatch{ + Port: aws.Int64(8000), + }, + }, + }, + }, + }, + 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: "tainted routes are removed", + sdkRoutes: []*appmeshsdk.RouteData{ + { + RouteName: aws.String("route-1"), + Metadata: &appmeshsdk.ResourceMetadata{ + MeshOwner: aws.String("111111111111"), + }, + MeshName: aws.String("mesh-1"), + VirtualRouterName: aws.String("virtual-router-1"), + Spec: &appmeshsdk.RouteSpec{ + TcpRoute: &appmeshsdk.TcpRoute{ + Match: &appmeshsdk.TcpRouteMatch{ + Port: aws.Int64(8000), + }, + }, + }, + }, + }, + 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", + HTTPRoute: &appmesh.HTTPRoute{ + Match: appmesh.HTTPRouteMatch{ + Port: aws.Int64(8000), + }, + }, + }, + }, + }, + }, + }, + wantDeleteRoutes: []*appmeshsdk.DeleteRouteInput{ + { + MeshName: aws.String("mesh-1"), + MeshOwner: aws.String("111111111111"), + RouteName: aws.String("route-1"), + VirtualRouterName: aws.String("virtual-router-1"), + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := &fakeAppMesh{ + existingRoutes: tt.sdkRoutes, + } + 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 + + existingRoutes []*appmeshsdk.RouteData + deletedRoutes []*appmeshsdk.DeleteRouteInput +} + +func (f *fakeAppMesh) ListRoutesPagesWithContext(_ aws.Context, _ *appmeshsdk.ListRoutesInput, callback func(*appmeshsdk.ListRoutesOutput, bool) bool, _ ...request.Option) error { + if len(f.existingRoutes) > 0 { + routes := make([]*appmeshsdk.RouteRef, len(f.existingRoutes)) + for i, route := range f.existingRoutes { + routes[i] = &appmeshsdk.RouteRef{ + RouteName: route.RouteName, + } + } + callback(&appmeshsdk.ListRoutesOutput{ + Routes: routes, + }, false) + } + return nil +} + +func (f *fakeAppMesh) DescribeRouteWithContext(_ aws.Context, params *appmeshsdk.DescribeRouteInput, _ ...request.Option) (*appmeshsdk.DescribeRouteOutput, error) { + for _, route := range f.existingRoutes { + if aws.StringValue(route.RouteName) != aws.StringValue(params.RouteName) { + continue + } + return &appmeshsdk.DescribeRouteOutput{ + Route: route, + }, nil + } + return nil, fmt.Errorf("not found") +} + +func (f *fakeAppMesh) DeleteRouteWithContext(_ aws.Context, params *appmeshsdk.DeleteRouteInput, _ ...request.Option) (*appmeshsdk.DeleteRouteOutput, error) { + f.deletedRoutes = append(f.deletedRoutes, params) + for _, route := range f.existingRoutes { + if aws.StringValue(route.RouteName) != aws.StringValue(params.RouteName) { + continue + } + return &appmeshsdk.DeleteRouteOutput{ + Route: route, + }, nil + } + return nil, fmt.Errorf("not found") +}