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

@@ -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

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)