Enhance RouteTable CRD sync routes synchronisation (#199)

- Update `syncRoutes` method to handle errors from `excludeAWSRoute`
- Reorder route deletion and addition in `syncRoutes` for proper sync
- Modify `excludeAWSRoute` to return an error and rename variables for clarity
- Move tags sync before routes sync in `customUpdateRouteTable`
- Update `sdkCreate` to use a copy of the RouteTable for creating routes
- Minor formatting and comment updates in template files

The changes in this commit enhance the synchronization of routes for the
RouteTable Custom Resource Definition (CRD). The `syncRoutes` method now handles
errors returned from the `excludeAWSRoute` function and reorders the deletion
and addition of routes for proper synchronization. The `excludeAWSRoute`
function is modified to return an error and has some variable renaming for
better clarity...

In the `customUpdateRouteTable` method the tags synchronization is moved before
the routes synchronization to ensure tagss are updated before route changes.

The `sdkCreate` method now uses a deep copy of the RouteTable when creating
routes to avoid modifying the original desired state

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Author: a-hilaly

apis/v1alpha1/ack-generate-metadata.yaml

@@ -1,7 +1,7 @@
 ack_generate_info:
-  build_date: "2024-06-04T06:39:17Z"
+  build_date: "2024-06-19T06:55:08Z"
   build_hash: 14cef51778d471698018b6c38b604181a6948248
-  go_version: go1.22.3
+  go_version: go1.22.4
   version: v0.34.0
 api_directory_checksum: 7fd395ceb7d5d8e35906991c7348d3498f384741
 api_version: v1alpha1

pkg/resource/route_table/hooks.go

@@ -44,15 +44,21 @@ func (rm *resourceManager) syncRoutes(
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncRoutes")
-	defer exit(err)
+	defer func(err error) { exit(err) }(err)
 	toAdd := []*svcapitypes.CreateRouteInput{}
 	toDelete := []*svcapitypes.CreateRouteInput{}
 
 	if latest != nil {
-		latest.ko.Spec.Routes = rm.excludeAwsRoute(latest.ko.Spec.Routes)
+		latest.ko.Spec.Routes, err = rm.excludeAWSRoute(latest.ko.Spec.Routes)
+		if err != nil {
+			return err
+		}
 	}
 	if desired != nil {
-		desired.ko.Spec.Routes = rm.excludeAwsRoute(desired.ko.Spec.Routes)
+		desired.ko.Spec.Routes, err = rm.excludeAWSRoute(desired.ko.Spec.Routes)
+		if err != nil {
+			return err
+		}
 	}
 
 	for _, desiredRoute := range desired.ko.Spec.Routes {
@@ -85,18 +91,18 @@ func (rm *resourceManager) syncRoutes(
 		}
 	}
 
-	for _, route := range toAdd {
-		rlog.Debug("adding route to route table")
-		if err = rm.createRoute(ctx, desired, *route); err != nil {
-			return err
-		}
-	}
 	for _, route := range toDelete {
 		rlog.Debug("deleting route from route table")
 		if err = rm.deleteRoute(ctx, latest, *route); err != nil {
 			return err
 		}
 	}
+	for _, route := range toAdd {
+		rlog.Debug("adding route to route table")
+		if err = rm.createRoute(ctx, desired, *route); err != nil {
+			return err
+		}
+	}
 
 	return nil
 }
@@ -162,7 +168,7 @@ func (rm *resourceManager) createRoute(
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.createRoute")
-	defer exit(err)
+	defer func(err error) { exit(err) }(err)
 
 	input := rm.newCreateRouteInput(c)
 	// Routes should only be configurable for the
@@ -180,7 +186,7 @@ func (rm *resourceManager) deleteRoute(
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.deleteRoute")
-	defer exit(err)
+	defer func(err error) { exit(err) }(err)
 
 	input := rm.newDeleteRouteInput(c)
 	// Routes should only be configurable for the
@@ -199,7 +205,7 @@ func (rm *resourceManager) customUpdateRouteTable(
 ) (updated *resource, err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.customUpdateRouteTable")
-	defer exit(err)
+	defer func(err error) { exit(err) }(err)
 
 	// Default `updated` to `desired` because it is likely
 	// EC2 `modify` APIs do NOT return output, only errors.
@@ -208,6 +214,16 @@ func (rm *resourceManager) customUpdateRouteTable(
 	// (now updated.Spec) reflects the latest resource state.
 	updated = rm.concreteResource(desired.DeepCopy())
 
+	if delta.DifferentAt("Spec.Tags") {
+		if err := tags.Sync(
+			ctx, rm.sdkapi, rm.metrics,
+			*latest.ko.Status.RouteTableID,
+			desired.ko.Spec.Tags, latest.ko.Spec.Tags,
+		); err != nil {
+			return nil, err
+		}
+	}
+
 	if delta.DifferentAt("Spec.Routes") {
 		if err := rm.syncRoutes(ctx, desired, latest); err != nil {
 			return nil, err
@@ -220,15 +236,6 @@ func (rm *resourceManager) customUpdateRouteTable(
 		}
 	}
 
-	if delta.DifferentAt("Spec.Tags") {
-		if err := tags.Sync(
-			ctx, rm.sdkapi, rm.metrics, *latest.ko.Status.RouteTableID,
-			desired.ko.Spec.Tags, latest.ko.Spec.Tags,
-		); err != nil {
-			return nil, err
-		}
-	}
-
 	return updated, nil
 }
 
@@ -287,9 +294,9 @@ func removeLocalRoute(
 	return ret
 }
 
-func (rm *resourceManager) excludeAwsRoute(
+func (rm *resourceManager) excludeAWSRoute(
 	routes []*svcapitypes.CreateRouteInput,
-) (ret []*svcapitypes.CreateRouteInput) {
+) (ret []*svcapitypes.CreateRouteInput, err error) {
 	ret = make([]*svcapitypes.CreateRouteInput, 0)
 	var prefixListIds []*string
 
@@ -300,20 +307,21 @@ func (rm *resourceManager) excludeAwsRoute(
 		return route.DestinationPrefixListID != nil
 	})
 
-	prefix_list_routes := lo.Filter(routes, func(route *svcapitypes.CreateRouteInput, index int) bool {
+	prefixListRoutes := lo.Filter(routes, func(route *svcapitypes.CreateRouteInput, index int) bool {
 		return route.DestinationPrefixListID != nil
 	})
 
-	prefixListIds = lo.Map(prefix_list_routes, func(route *svcapitypes.CreateRouteInput, _ int) *string {
+	prefixListIds = lo.Map(prefixListRoutes, func(route *svcapitypes.CreateRouteInput, _ int) *string {
 		return route.DestinationPrefixListID
-
 	})
 
-	var resp *svcsdk.DescribeManagedPrefixListsOutput
 	input := &svcsdk.DescribeManagedPrefixListsInput{}
 	input.PrefixListIds = prefixListIds
-	resp, _ = rm.sdkapi.DescribeManagedPrefixLists(input)
+	resp, err := rm.sdkapi.DescribeManagedPrefixLists(input)
 	rm.metrics.RecordAPICall("READ_MANY", "DescribeManagedPrefixLists", nil)
+	if err != nil {
+		return ret, nil
+	}
 
 	m := lo.FilterMap(resp.PrefixLists, func(mpl *svcsdk.ManagedPrefixList, _ int) (string, bool) {
 		if strings.EqualFold(*mpl.OwnerId, "AWS") {
@@ -322,7 +330,7 @@ func (rm *resourceManager) excludeAwsRoute(
 		return "", false
 	})
 
-	filtered_routes := lo.FilterMap(prefix_list_routes, func(route *svcapitypes.CreateRouteInput, _ int) (*svcapitypes.CreateRouteInput, bool) {
+	filtered_routes := lo.FilterMap(prefixListRoutes, func(route *svcapitypes.CreateRouteInput, _ int) (*svcapitypes.CreateRouteInput, bool) {
 		found := lo.IndexOf(m, *route.DestinationPrefixListID)
 		if found == -1 {
 			return route, true
@@ -331,7 +339,7 @@ func (rm *resourceManager) excludeAwsRoute(
 	})
 	ret = append(ret, filtered_routes...)
 
-	return ret
+	return ret, nil
 }
 
 // updateTagSpecificationsInCreateRequest adds

pkg/resource/route_table/sdk.go

@@ -7,7 +7,8 @@
 	if len(desired.ko.Spec.Routes) > 0 {
 		//desired routes are overwritten by RouteTable's default route
 		ko.Spec.Routes = append(ko.Spec.Routes, desired.ko.Spec.Routes...)
-		if err := rm.createRoutes(ctx, &resource{ko}); err != nil {
+		copy := ko.DeepCopy()
+		if err := rm.createRoutes(ctx, &resource{copy}); err != nil {
 			return nil, err
 		}
 	}
@@ -17,4 +18,4 @@
 		// if desired tags and response tags are equal,
 		// then assign desired tags to maintain tag order
 		ko.Spec.Tags = desired.ko.Spec.Tags
-	}
+	}

templates/hooks/route_table/sdk_create_post_set_output.go.tpl

@@ -7,5 +7,4 @@
 		// if resource's initial tags and response tags are equal,
 		// then assign resource's tags to maintain tag order
 		ko.Spec.Tags = r.ko.Spec.Tags
-	}
-    
+	}