Fix VirtualRouter route deletion sequence

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)