Issue 2090/route table order dependence (aws-controllers-k8s#236)

Fixes issue [#2090](aws-controllers-k8s/community#2090)

Description of changes:
- Configure generator for RouteTable to ignore the Routes from the generated delta code.
- Extend the `customPreCompare` code to take care of the routes (computing the delta ignoring the order of routes).
- Added unit test for the custom code.
- Change the `syncRoutes` code to use the delta from the compare phase.

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

Author: gfrey

apis/v1alpha1/ack-generate-metadata.yaml

@@ -7,7 +7,7 @@ api_directory_checksum: b31faecf6092fab9677498f3624e624fee4cbaed
 api_version: v1alpha1
 aws_sdk_go_version: v1.32.6
 generator_config_info:
-  file_checksum: b3d6924c2a4a2252ea9a7fa775eb86bd1660eeef
+  file_checksum: 4f9d21bd7c46b495cd9374e454a3a0e1e6de5d08
   original_file_name: generator.yaml
 last_modification:
   reason: API generation

apis/v1alpha1/generator.yaml

@@ -535,6 +535,8 @@ resources:
       Routes:
         custom_field:
           list_of: CreateRouteInput
+        compare:
+          is_ignored: true
       RouteTableID:
         print:
           path: Status.RouteTableID

generator.yaml

@@ -535,6 +535,8 @@ resources:
       Routes:
         custom_field:
           list_of: CreateRouteInput
+        compare:
+          is_ignored: true
       RouteTableID:
         print:
           path: Status.RouteTableID

go.mod

@@ -13,6 +13,7 @@ require (
 	github.com/go-logr/logr v1.4.2
 	github.com/samber/lo v1.37.0
 	github.com/spf13/pflag v1.0.5
+	github.com/stretchr/testify v1.9.0
 	k8s.io/api v0.31.0
 	k8s.io/apimachinery v0.31.0
 	k8s.io/client-go v0.31.0
@@ -61,6 +62,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
 	github.com/prometheus/client_golang v1.19.1 // indirect
 	github.com/prometheus/client_model v0.6.1 // indirect
 	github.com/prometheus/common v0.55.0 // indirect

pkg/resource/route_table/delta.go

@@ -46,52 +46,35 @@ func (rm *resourceManager) syncRoutes(
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncRoutes")
 	defer func(err error) { exit(err) }(err)
-	toAdd := []*svcapitypes.CreateRouteInput{}
-	toDelete := []*svcapitypes.CreateRouteInput{}
 
 	if latest != nil {
+		latest.ko.Spec.Routes = removeLocalRoute(latest.ko.Spec.Routes)
 		latest.ko.Spec.Routes, err = rm.excludeAWSRoute(ctx, latest.ko.Spec.Routes)
 		if err != nil {
 			return err
 		}
 	}
 	if desired != nil {
+		desired.ko.Spec.Routes = removeLocalRoute(desired.ko.Spec.Routes)
 		desired.ko.Spec.Routes, err = rm.excludeAWSRoute(ctx, desired.ko.Spec.Routes)
 		if err != nil {
 			return err
 		}
 	}
 
-	for _, desiredRoute := range desired.ko.Spec.Routes {
-		if (*desiredRoute).GatewayID != nil && *desiredRoute.GatewayID == LocalRouteGateway {
-			// no-op for default route
-			continue
-		}
-		if latestRoute := getMatchingRoute(desiredRoute, latest); latestRoute != nil {
-			delta := compareCreateRouteInput(desiredRoute, latestRoute)
-			if len(delta.Differences) > 0 {
-				// "update" route by deleting old route and adding the new route
-				toDelete = append(toDelete, latestRoute)
-				toAdd = append(toAdd, desiredRoute)
-			}
-		} else {
-			// a desired route is not in latest; therefore, create
-			toAdd = append(toAdd, desiredRoute)
-		}
-	}
+	// Get the routes that need to be added and deleted, by checking for
+	// differences between desired and latest.
+	var toAdd, toDelete []*svcapitypes.CreateRouteInput
 	if latest != nil {
-		for _, latestRoute := range latest.ko.Spec.Routes {
-			if (*latestRoute).GatewayID != nil && *latestRoute.GatewayID == LocalRouteGateway {
-				// no-op for default route
-				continue
-			}
-			if desiredRoute := getMatchingRoute(latestRoute, desired); desiredRoute == nil {
-				// latest has a route that is not desired; therefore, delete
-				toDelete = append(toDelete, latestRoute)
-			}
-		}
+		toAdd, toDelete = getRoutesDifference(desired.ko.Spec.Routes, latest.ko.Spec.Routes)
+	} else {
+		// If method is called from the createRoutes method, then latest is nil
+		// and all routes must be added, as non exist yet.
+		toAdd = desired.ko.Spec.Routes
 	}
 
+	// Delete and add the routes that were found to be different between desired
+	// and latest.
 	for _, route := range toDelete {
 		rlog.Debug("deleting route from route table")
 		if err = rm.deleteRoute(ctx, latest, *route); err != nil {
@@ -108,60 +91,6 @@ func (rm *resourceManager) syncRoutes(
 	return nil
 }
 
-func getMatchingRoute(
-	routeToMatch *svcapitypes.CreateRouteInput,
-	resource *resource,
-) *svcapitypes.CreateRouteInput {
-	if resource == nil {
-		return nil
-	}
-
-	for _, route := range resource.ko.Spec.Routes {
-		delta := compareCreateRouteInput(routeToMatch, route)
-		if len(delta.Differences) == 0 {
-			return route
-		} else {
-			if routeToMatch.CarrierGatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.CarrierGatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.EgressOnlyInternetGatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.EgressOnlyInternetGatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.GatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.GatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.LocalGatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.LocalGatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.NATGatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.NATGatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.TransitGatewayID != nil {
-				if !delta.DifferentAt("CreateRouteInput.TransitGatewayID") {
-					return route
-				}
-			}
-			if routeToMatch.VPCPeeringConnectionID != nil {
-				if !delta.DifferentAt("CreateRouteInput.VPCPeeringConnectionID") {
-					return route
-				}
-			}
-		}
-	}
-
-	return nil
-}
-
 func (rm *resourceManager) createRoute(
 	ctx context.Context,
 	r *resource,
@@ -270,13 +199,56 @@ var computeTagsDelta = tags.ComputeTagsDelta
 
 // customPreCompare ensures that default values of types are initialised and
 // server side defaults are excluded from the delta.
+// The left side (`A`) of any `Spec.Routes` diff contains the routes that are
+// desired, but do not exist. Analogously, the right side (`B`) contains the
+// routes that exist, but are not desired.
 func customPreCompare(
 	delta *ackcompare.Delta,
 	a *resource,
 	b *resource,
 ) {
 	a.ko.Spec.Routes = removeLocalRoute(a.ko.Spec.Routes)
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
+
+	desired, latest := getRoutesDifference(a.ko.Spec.Routes, b.ko.Spec.Routes)
+
+	if len(desired) > 0 || len(latest) > 0 {
+		delta.Add("Spec.Routes", a.ko.Spec.Routes, b.ko.Spec.Routes)
+	}
+}
+
+// getRoutesDifference compares the desired and latest routes. It returns the
+// routes that are different and must be added or deleted.
+func getRoutesDifference(desired, latest []*svcapitypes.CreateRouteInput) (toAdd, toDelete []*svcapitypes.CreateRouteInput) {
+	toDelete = make([]*svcapitypes.CreateRouteInput, len(latest))
+	copy(toDelete, latest)
+
+	remove := func(s []*svcapitypes.CreateRouteInput, i int) []*svcapitypes.CreateRouteInput {
+		if i < len(s)-1 { // if not last element just copy the last element to where the removed element was
+			s[i] = s[len(s)-1]
+		}
+		return s[:len(s)-1]
+	}
+
+	// Routes that are desired, but already exist in latest, can be ignored. The
+	// toDelete slice is a copy of latest and will be slowly modified so that at
+	// the end it only contains routes that exist in latest, but are not
+	// desired.
+	for _, routeA := range desired {
+		found := false
+		for idx, routeB := range toDelete {
+			if delta := compareCreateRouteInput(routeA, routeB); len(delta.Differences) == 0 {
+				toDelete = remove(toDelete, idx)
+				found = true
+				break
+			}
+		}
+		if !found {
+			toAdd = append(toAdd, routeA.DeepCopy())
+		}
+	}
+
+	return toAdd, toDelete
 }
 
 // removeLocalRoute will filter out any routes that have a gateway ID that

pkg/resource/route_table/hooks.go

@@ -0,0 +1,106 @@
+package route_table
+
+import (
+	"testing"
+
+	svcapitypes "github.com/aws-controllers-k8s/ec2-controller/apis/v1alpha1"
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestCustomerPreCompare(t *testing.T) {
+	type Routes []*svcapitypes.CreateRouteInput
+
+	peeringRoute := func(pcxID string, cidr string) *svcapitypes.CreateRouteInput {
+		return &svcapitypes.CreateRouteInput{
+			DestinationCIDRBlock:   aws.String(cidr),
+			VPCPeeringConnectionID: aws.String(pcxID),
+		}
+	}
+
+	createRouteTableTestResource := func(routes []*svcapitypes.CreateRouteInput) *resource {
+		return &resource{
+			ko: &svcapitypes.RouteTable{
+				Spec: svcapitypes.RouteTableSpec{
+					Routes: routes,
+				},
+			},
+		}
+	}
+
+	assertRoutesIdentical := func(t *testing.T, a, b []*svcapitypes.CreateRouteInput) {
+		assert.Len(t, a, len(b))
+		for i := range a {
+			assert.EqualValues(t, a[i], b[i])
+		}
+	}
+
+	tt := []struct {
+		id            string
+		desiredRoutes Routes
+		latestRoutes  Routes
+		toAdd         Routes
+		toDelete      Routes
+	}{
+		{"all identical",
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			nil, nil,
+		},
+		{"add route",
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			nil,
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			nil,
+		},
+		{"delete route",
+			nil,
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			nil,
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+		},
+		{"keep one delete one",
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24"), peeringRoute("pcx-123", "172.30.2.0/24")},
+			nil,
+			Routes{peeringRoute("pcx-123", "172.30.2.0/24")},
+		},
+		{"keep one add one",
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24"), peeringRoute("pcx-123", "172.30.2.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.2.0/24")},
+			nil,
+		},
+		{"keep one add one delete one",
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24"), peeringRoute("pcx-123", "172.30.2.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.1.0/24"), peeringRoute("pcx-123", "172.30.3.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.2.0/24")},
+			Routes{peeringRoute("pcx-123", "172.30.3.0/24")},
+		},
+	}
+
+	for _, tti := range tt {
+		t.Run(tti.id, func(t *testing.T) {
+			delta := ackcompare.NewDelta()
+			a := createRouteTableTestResource(tti.desiredRoutes)
+			b := createRouteTableTestResource(tti.latestRoutes)
+			customPreCompare(delta, a, b)
+			if len(tti.toAdd) == 0 && len(tti.toDelete) == 0 {
+				assert.Equal(t, 0, len(delta.Differences))
+			} else {
+				diff := delta.Differences[0]
+				diffA := diff.A.([]*svcapitypes.CreateRouteInput)
+				diffB := diff.B.([]*svcapitypes.CreateRouteInput)
+				assert.True(t, diff.Path.Contains("Spec.Routes"))
+				assert.ElementsMatch(t, tti.desiredRoutes, diffA)
+				assert.ElementsMatch(t, tti.latestRoutes, diffB)
+
+				// Check the different routes are identified correctly
+				toAdd, toDelete := getRoutesDifference(tti.desiredRoutes, tti.latestRoutes)
+				assertRoutesIdentical(t, tti.toAdd, toAdd)
+				assertRoutesIdentical(t, tti.toDelete, toDelete)
+			}
+		})
+	}
+}