From 5d80b8fbe265a0a4f9a3753945c0b8c6d1c5c8c3 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Fri, 31 Jan 2025 14:31:11 +0100
Subject: [PATCH 01/13] Make routes order independent

In the past the routes were not order independent. Whenever the API
would return a different order than specified, the routes would be
deleted and recreated. The `getMatchingRoute` code also returned the
first route that would have a matching field like the
VPCPeeringConnectionId. This resulted in routes not being stable.

Now the `customPreCompare` handles routes properly, i.e. is order
independent. Also the `syncRoutes` uses the computed diff, to create or
delete the according routes.
---
 generator.yaml                    |   2 +
 pkg/resource/route_table/hooks.go | 126 +++++++++++-------------------
 2 files changed, 46 insertions(+), 82 deletions(-)

diff --git a/generator.yaml b/generator.yaml
index 884024d6..6b279605 100644
--- a/generator.yaml
+++ b/generator.yaml
@@ -535,6 +535,8 @@ resources:
       Routes:
         custom_field:
           list_of: CreateRouteInput
+        compare:
+          is_ignored: true
       RouteTableID:
         print:
           path: Status.RouteTableID
diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index bacf8f0e..c5176c04 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -15,6 +15,7 @@ package route_table
 
 import (
 	"context"
+	"reflect"
 	"strings"
 
 	svcapitypes "github.com/aws-controllers-k8s/ec2-controller/apis/v1alpha1"
@@ -32,7 +33,7 @@ func (rm *resourceManager) createRoutes(
 	ctx context.Context,
 	r *resource,
 ) error {
-	if err := rm.syncRoutes(ctx, r, nil); err != nil {
+	if err := rm.syncRoutes(ctx, r, nil, nil); err != nil {
 		return err
 	}
 	return nil
@@ -42,6 +43,7 @@ func (rm *resourceManager) syncRoutes(
 	ctx context.Context,
 	desired *resource,
 	latest *resource,
+	delta *ackcompare.Delta,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncRoutes")
@@ -62,34 +64,17 @@ func (rm *resourceManager) syncRoutes(
 		}
 	}
 
-	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)
-		}
-	}
-	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)
+	switch {
+	case delta == nil:
+		toAdd = desired.ko.Spec.Routes
+	case delta.DifferentAt("Spec.Routes"):
+		for _, diff := range delta.Differences {
+			if diff.Path.Contains("Spec.Routes") {
+				toAdd = diff.A.([]*svcapitypes.CreateRouteInput)
+				toDelete = diff.B.([]*svcapitypes.CreateRouteInput)
 			}
 		}
+	default: // nothing to do
 	}
 
 	for _, route := range toDelete {
@@ -108,60 +93,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,
@@ -226,7 +157,7 @@ func (rm *resourceManager) customUpdateRouteTable(
 	}
 
 	if delta.DifferentAt("Spec.Routes") {
-		if err := rm.syncRoutes(ctx, desired, latest); err != nil {
+		if err := rm.syncRoutes(ctx, desired, latest, delta); err != nil {
 			return nil, err
 		}
 		// A ReadOne call is made to refresh Status.RouteStatuses
@@ -277,6 +208,37 @@ func customPreCompare(
 ) {
 	a.ko.Spec.Routes = removeLocalRoute(a.ko.Spec.Routes)
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
+
+	toAdd := []*svcapitypes.CreateRouteInput{}
+	toDelete := make([]*svcapitypes.CreateRouteInput, len(b.ko.Spec.Routes))
+	copy(toDelete, b.ko.Spec.Routes) // Copy of the routes from b, that will be reduced while iterating
+
+	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 not in latest, need to be added.
+	// Routes that are in latest, but not desired, need to be deleted.
+	// Everything else, we don't touch.
+	for _, routeA := range a.ko.Spec.Routes {
+		found := false
+		for idx, routeB := range toDelete {
+			if found = reflect.DeepEqual(routeA, routeB); found {
+				toDelete = remove(toDelete, idx)
+				break
+			}
+		}
+		if !found {
+			toAdd = append(toAdd, routeA)
+		}
+	}
+
+	if len(toAdd) > 0 || len(toDelete) > 0 {
+		delta.Add("Spec.Routes", toAdd, toDelete)
+	}
 }
 
 // removeLocalRoute will filter out any routes that have a gateway ID that

From a5e0f0efbabab182eb7674615ac4a93558ce57ad Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Fri, 31 Jan 2025 15:31:08 +0100
Subject: [PATCH 02/13] Add test for CustomerPreCompare

---
 go.mod                                 |   2 +
 pkg/resource/route_table/hooks_test.go | 101 +++++++++++++++++++++++++
 2 files changed, 103 insertions(+)
 create mode 100644 pkg/resource/route_table/hooks_test.go

diff --git a/go.mod b/go.mod
index 020bd92a..0c1cb92e 100644
--- a/go.mod
+++ b/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
diff --git a/pkg/resource/route_table/hooks_test.go b/pkg/resource/route_table/hooks_test.go
new file mode 100644
index 00000000..361b46f2
--- /dev/null
+++ b/pkg/resource/route_table/hooks_test.go
@@ -0,0 +1,101 @@
+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"
+	"github.com/stretchr/testify/require"
+)
+
+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,
+				},
+			},
+		}
+	}
+
+	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"))
+				require.Equal(t, len(tti.toAdd), len(diffA))
+				require.Equal(t, len(tti.toDelete), len(diffB))
+				for i := range tti.toAdd {
+					assert.Equal(t, tti.toAdd[i], diffA[i])
+				}
+				for i := range tti.toDelete {
+					assert.Equal(t, tti.toDelete[i], diffB[i])
+				}
+			}
+		})
+	}
+}

From 2386870d500c9abeecbffc7e38ae31fc00499bf8 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Tue, 4 Feb 2025 11:47:12 +0100
Subject: [PATCH 03/13] Remove local routes when creating a route table

This is the issue that made the e2e tests fail.
---
 pkg/resource/route_table/hooks.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index c5176c04..a539cbc8 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -66,7 +66,7 @@ func (rm *resourceManager) syncRoutes(
 
 	switch {
 	case delta == nil:
-		toAdd = desired.ko.Spec.Routes
+		toAdd = removeLocalRoute(desired.ko.Spec.Routes)
 	case delta.DifferentAt("Spec.Routes"):
 		for _, diff := range delta.Differences {
 			if diff.Path.Contains("Spec.Routes") {

From 6b8268faa0cd9950ccd177ee9324919ee6219bc7 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Thu, 20 Feb 2025 11:56:03 +0100
Subject: [PATCH 04/13] Add comments to syncRoutes function

This should help to make obvious what the different steps are doing.
Also added information on the route related diffs in the delta.
---
 pkg/resource/route_table/hooks.go | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index a539cbc8..f97f2296 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -48,6 +48,9 @@ func (rm *resourceManager) syncRoutes(
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncRoutes")
 	defer func(err error) { exit(err) }(err)
+
+	// To determine the required updates to the route table, the routes to be
+	// added and deleted will be collected first.
 	toAdd := []*svcapitypes.CreateRouteInput{}
 	toDelete := []*svcapitypes.CreateRouteInput{}
 
@@ -65,18 +68,27 @@ func (rm *resourceManager) syncRoutes(
 	}
 
 	switch {
+	// If the route table is created all routes need to be added.
 	case delta == nil:
 		toAdd = removeLocalRoute(desired.ko.Spec.Routes)
+	// If there are changes to the routes in the delta ...
 	case delta.DifferentAt("Spec.Routes"):
+		// ... iterate over all the differences ...
 		for _, diff := range delta.Differences {
+			// ... and if the current one is regarding the routes ...
 			if diff.Path.Contains("Spec.Routes") {
+				// ... take the routes to add from the left side of the diff ...
 				toAdd = diff.A.([]*svcapitypes.CreateRouteInput)
+				// ... and the routes to delete from the right side (see the
+				// customPreCompare function for information on the diff
+				// structure).
 				toDelete = diff.B.([]*svcapitypes.CreateRouteInput)
 			}
 		}
 	default: // nothing to do
 	}
 
+	// Finally delete and add the routes that were collected.
 	for _, route := range toDelete {
 		rlog.Debug("deleting route from route table")
 		if err = rm.deleteRoute(ctx, latest, *route); err != nil {
@@ -201,6 +213,8 @@ 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 to add, the
+// right side (`B`) the routes that must be deleted.
 func customPreCompare(
 	delta *ackcompare.Delta,
 	a *resource,

From 9bd0e21b5d28986a121e29e1da7017560968a0be Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Thu, 20 Feb 2025 11:38:00 +0100
Subject: [PATCH 05/13] Update generated code using code-generator v0.43.2

---
 apis/v1alpha1/ack-generate-metadata.yaml | 6 +++---
 apis/v1alpha1/generator.yaml             | 2 ++
 pkg/resource/route_table/delta.go        | 7 -------
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml
index f6c9f2ce..1fc5256a 100755
--- a/apis/v1alpha1/ack-generate-metadata.yaml
+++ b/apis/v1alpha1/ack-generate-metadata.yaml
@@ -1,13 +1,13 @@
 ack_generate_info:
-  build_date: "2025-02-20T17:55:49Z"
+  build_date: "2025-02-24T08:32:50Z"
   build_hash: a326346bd3a6973254d247c9ab2dc76790c36241
-  go_version: go1.24.0
+  go_version: go1.23.5
   version: v0.43.2
 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
diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml
index 884024d6..6b279605 100644
--- a/apis/v1alpha1/generator.yaml
+++ b/apis/v1alpha1/generator.yaml
@@ -535,6 +535,8 @@ resources:
       Routes:
         custom_field:
           list_of: CreateRouteInput
+        compare:
+          is_ignored: true
       RouteTableID:
         print:
           path: Status.RouteTableID
diff --git a/pkg/resource/route_table/delta.go b/pkg/resource/route_table/delta.go
index 7c01adcf..ed6ae41e 100644
--- a/pkg/resource/route_table/delta.go
+++ b/pkg/resource/route_table/delta.go
@@ -44,13 +44,6 @@ func newResourceDelta(
 	}
 	customPreCompare(delta, a, b)
 
-	if len(a.ko.Spec.Routes) != len(b.ko.Spec.Routes) {
-		delta.Add("Spec.Routes", a.ko.Spec.Routes, b.ko.Spec.Routes)
-	} else if len(a.ko.Spec.Routes) > 0 {
-		if !reflect.DeepEqual(a.ko.Spec.Routes, b.ko.Spec.Routes) {
-			delta.Add("Spec.Routes", a.ko.Spec.Routes, b.ko.Spec.Routes)
-		}
-	}
 	if !ackcompare.MapStringStringEqual(ToACKTags(a.ko.Spec.Tags), ToACKTags(b.ko.Spec.Tags)) {
 		delta.Add("Spec.Tags", a.ko.Spec.Tags, b.ko.Spec.Tags)
 	}

From 5cce44555737398754bcc60ccdc5b27a686d0552 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Mon, 24 Feb 2025 09:23:13 +0100
Subject: [PATCH 06/13] Create deep copies of route inputs

This is not strictly necessary, but as these are already copied, not an
issue either.
---
 pkg/resource/route_table/hooks.go | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index f97f2296..206b9da3 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -224,8 +224,7 @@ func customPreCompare(
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
 
 	toAdd := []*svcapitypes.CreateRouteInput{}
-	toDelete := make([]*svcapitypes.CreateRouteInput, len(b.ko.Spec.Routes))
-	copy(toDelete, b.ko.Spec.Routes) // Copy of the routes from b, that will be reduced while iterating
+	toDelete := b.ko.Spec.DeepCopy().Routes
 
 	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
@@ -246,7 +245,7 @@ func customPreCompare(
 			}
 		}
 		if !found {
-			toAdd = append(toAdd, routeA)
+			toAdd = append(toAdd, routeA.DeepCopy())
 		}
 	}
 

From 86e6f18f86cf0e39251e44365a3f0e306a70b9ca Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Mon, 24 Feb 2025 11:49:12 +0100
Subject: [PATCH 07/13] Rename variables to make intent more obvious

The delta contains the routes that are different between the "desired"
and the "latest" state. The identical ones are ignored.
---
 pkg/resource/route_table/hooks.go | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index 206b9da3..16f46168 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -213,8 +213,9 @@ 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 to add, the
-// right side (`B`) the routes that must be deleted.
+// 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,
@@ -223,8 +224,8 @@ func customPreCompare(
 	a.ko.Spec.Routes = removeLocalRoute(a.ko.Spec.Routes)
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
 
-	toAdd := []*svcapitypes.CreateRouteInput{}
-	toDelete := b.ko.Spec.DeepCopy().Routes
+	desired := []*svcapitypes.CreateRouteInput{}
+	latest := b.ko.Spec.DeepCopy().Routes
 
 	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
@@ -233,24 +234,22 @@ func customPreCompare(
 		return s[:len(s)-1]
 	}
 
-	// Routes that are desired, but not in latest, need to be added.
-	// Routes that are in latest, but not desired, need to be deleted.
-	// Everything else, we don't touch.
+	// Routes that are desired, but already exist in latest, can be ignored.
 	for _, routeA := range a.ko.Spec.Routes {
 		found := false
-		for idx, routeB := range toDelete {
+		for idx, routeB := range latest {
 			if found = reflect.DeepEqual(routeA, routeB); found {
-				toDelete = remove(toDelete, idx)
+				latest = remove(latest, idx)
 				break
 			}
 		}
 		if !found {
-			toAdd = append(toAdd, routeA.DeepCopy())
+			desired = append(desired, routeA.DeepCopy())
 		}
 	}
 
-	if len(toAdd) > 0 || len(toDelete) > 0 {
-		delta.Add("Spec.Routes", toAdd, toDelete)
+	if len(desired) > 0 || len(latest) > 0 {
+		delta.Add("Spec.Routes", desired, latest)
 	}
 }
 

From 2ac08c24a308d07f61f579b8a56537c92e0fe9cc Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Tue, 25 Feb 2025 07:57:12 +0100
Subject: [PATCH 08/13] Adopt ACK style delta

The differences for complex types (e.g. the lists of tags or routes) are
given as the entire complex type, but only the changed parts. This is
the way it currently is, so stick to that.
---
 pkg/resource/route_table/hooks.go | 51 +++++++++++++++----------------
 1 file changed, 24 insertions(+), 27 deletions(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index 16f46168..b8d7f0b6 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -49,11 +49,6 @@ func (rm *resourceManager) syncRoutes(
 	exit := rlog.Trace("rm.syncRoutes")
 	defer func(err error) { exit(err) }(err)
 
-	// To determine the required updates to the route table, the routes to be
-	// added and deleted will be collected first.
-	toAdd := []*svcapitypes.CreateRouteInput{}
-	toDelete := []*svcapitypes.CreateRouteInput{}
-
 	if latest != nil {
 		latest.ko.Spec.Routes, err = rm.excludeAWSRoute(ctx, latest.ko.Spec.Routes)
 		if err != nil {
@@ -67,24 +62,15 @@ func (rm *resourceManager) syncRoutes(
 		}
 	}
 
+	var toAdd, toDelete []*svcapitypes.CreateRouteInput
+
 	switch {
 	// If the route table is created all routes need to be added.
 	case delta == nil:
 		toAdd = removeLocalRoute(desired.ko.Spec.Routes)
 	// If there are changes to the routes in the delta ...
 	case delta.DifferentAt("Spec.Routes"):
-		// ... iterate over all the differences ...
-		for _, diff := range delta.Differences {
-			// ... and if the current one is regarding the routes ...
-			if diff.Path.Contains("Spec.Routes") {
-				// ... take the routes to add from the left side of the diff ...
-				toAdd = diff.A.([]*svcapitypes.CreateRouteInput)
-				// ... and the routes to delete from the right side (see the
-				// customPreCompare function for information on the diff
-				// structure).
-				toDelete = diff.B.([]*svcapitypes.CreateRouteInput)
-			}
-		}
+		toAdd, toDelete = filterDifferentRoutes(desired.ko.Spec.Routes, latest.ko.Spec.Routes)
 	default: // nothing to do
 	}
 
@@ -224,8 +210,18 @@ func customPreCompare(
 	a.ko.Spec.Routes = removeLocalRoute(a.ko.Spec.Routes)
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
 
-	desired := []*svcapitypes.CreateRouteInput{}
-	latest := b.ko.Spec.DeepCopy().Routes
+	desired, latest := filterDifferentRoutes(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)
+	}
+}
+
+// filterDifferentRoutes compares the desired and latest routes. It returns the
+// routes that are different and must be added or deleted.
+func filterDifferentRoutes(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
@@ -234,23 +230,24 @@ func customPreCompare(
 		return s[:len(s)-1]
 	}
 
-	// Routes that are desired, but already exist in latest, can be ignored.
-	for _, routeA := range a.ko.Spec.Routes {
+	// 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 latest {
+		for idx, routeB := range toDelete {
 			if found = reflect.DeepEqual(routeA, routeB); found {
-				latest = remove(latest, idx)
+				toDelete = remove(toDelete, idx)
 				break
 			}
 		}
 		if !found {
-			desired = append(desired, routeA.DeepCopy())
+			toAdd = append(toAdd, routeA.DeepCopy())
 		}
 	}
 
-	if len(desired) > 0 || len(latest) > 0 {
-		delta.Add("Spec.Routes", desired, latest)
-	}
+	return toAdd, toDelete
 }
 
 // removeLocalRoute will filter out any routes that have a gateway ID that

From 9d6f263e20c78529cf88d6061f56cec0fd36ead7 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Tue, 25 Feb 2025 08:45:00 +0100
Subject: [PATCH 09/13] Fix tests

---
 pkg/resource/route_table/hooks_test.go | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/pkg/resource/route_table/hooks_test.go b/pkg/resource/route_table/hooks_test.go
index 361b46f2..79aa32e4 100644
--- a/pkg/resource/route_table/hooks_test.go
+++ b/pkg/resource/route_table/hooks_test.go
@@ -7,7 +7,6 @@ import (
 	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestCustomerPreCompare(t *testing.T) {
@@ -30,6 +29,13 @@ func TestCustomerPreCompare(t *testing.T) {
 		}
 	}
 
+	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
@@ -87,14 +93,13 @@ func TestCustomerPreCompare(t *testing.T) {
 				diffA := diff.A.([]*svcapitypes.CreateRouteInput)
 				diffB := diff.B.([]*svcapitypes.CreateRouteInput)
 				assert.True(t, diff.Path.Contains("Spec.Routes"))
-				require.Equal(t, len(tti.toAdd), len(diffA))
-				require.Equal(t, len(tti.toDelete), len(diffB))
-				for i := range tti.toAdd {
-					assert.Equal(t, tti.toAdd[i], diffA[i])
-				}
-				for i := range tti.toDelete {
-					assert.Equal(t, tti.toDelete[i], diffB[i])
-				}
+				assert.ElementsMatch(t, tti.desiredRoutes, diffA)
+				assert.ElementsMatch(t, tti.latestRoutes, diffB)
+
+				// Check the different routes are identified correctly
+				toAdd, toDelete := filterDifferentRoutes(tti.desiredRoutes, tti.latestRoutes)
+				assertRoutesIdentical(t, tti.toAdd, toAdd)
+				assertRoutesIdentical(t, tti.toDelete, toDelete)
 			}
 		})
 	}

From 7d7fb6c056f01791be19d04e47ca877bea79bede Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Wed, 26 Feb 2025 08:41:24 +0100
Subject: [PATCH 10/13] Add review feedback

- Rename function
- Removed `delta` from `syncRoutes` signature and thereby got rid of a
  lot of additional code. This is possible now that we recompute the
  differences, instead of using the information from the delta.
---
 pkg/resource/route_table/hooks.go      | 28 +++++++++-----------------
 pkg/resource/route_table/hooks_test.go |  2 +-
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index b8d7f0b6..5ddbde49 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -33,7 +33,7 @@ func (rm *resourceManager) createRoutes(
 	ctx context.Context,
 	r *resource,
 ) error {
-	if err := rm.syncRoutes(ctx, r, nil, nil); err != nil {
+	if err := rm.syncRoutes(ctx, r, nil); err != nil {
 		return err
 	}
 	return nil
@@ -43,7 +43,6 @@ func (rm *resourceManager) syncRoutes(
 	ctx context.Context,
 	desired *resource,
 	latest *resource,
-	delta *ackcompare.Delta,
 ) (err error) {
 	rlog := ackrtlog.FromContext(ctx)
 	exit := rlog.Trace("rm.syncRoutes")
@@ -62,19 +61,12 @@ func (rm *resourceManager) syncRoutes(
 		}
 	}
 
-	var toAdd, toDelete []*svcapitypes.CreateRouteInput
-
-	switch {
-	// If the route table is created all routes need to be added.
-	case delta == nil:
-		toAdd = removeLocalRoute(desired.ko.Spec.Routes)
-	// If there are changes to the routes in the delta ...
-	case delta.DifferentAt("Spec.Routes"):
-		toAdd, toDelete = filterDifferentRoutes(desired.ko.Spec.Routes, latest.ko.Spec.Routes)
-	default: // nothing to do
-	}
+	// Get the routes that need to be added and deleted, by checking for
+	// differences between desired and latest.
+	toAdd, toDelete := getRoutesDifference(desired.ko.Spec.Routes, latest.ko.Spec.Routes)
 
-	// Finally delete and add the routes that were collected.
+	// 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 {
@@ -155,7 +147,7 @@ func (rm *resourceManager) customUpdateRouteTable(
 	}
 
 	if delta.DifferentAt("Spec.Routes") {
-		if err := rm.syncRoutes(ctx, desired, latest, delta); err != nil {
+		if err := rm.syncRoutes(ctx, desired, latest); err != nil {
 			return nil, err
 		}
 		// A ReadOne call is made to refresh Status.RouteStatuses
@@ -210,16 +202,16 @@ func customPreCompare(
 	a.ko.Spec.Routes = removeLocalRoute(a.ko.Spec.Routes)
 	b.ko.Spec.Routes = removeLocalRoute(b.ko.Spec.Routes)
 
-	desired, latest := filterDifferentRoutes(a.ko.Spec.Routes, 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)
 	}
 }
 
-// filterDifferentRoutes compares the desired and latest routes. It returns the
+// getRoutesDifference compares the desired and latest routes. It returns the
 // routes that are different and must be added or deleted.
-func filterDifferentRoutes(desired, latest []*svcapitypes.CreateRouteInput) (toAdd, toDelete []*svcapitypes.CreateRouteInput) {
+func getRoutesDifference(desired, latest []*svcapitypes.CreateRouteInput) (toAdd, toDelete []*svcapitypes.CreateRouteInput) {
 	toDelete = make([]*svcapitypes.CreateRouteInput, len(latest))
 	copy(toDelete, latest)
 
diff --git a/pkg/resource/route_table/hooks_test.go b/pkg/resource/route_table/hooks_test.go
index 79aa32e4..79c78f0c 100644
--- a/pkg/resource/route_table/hooks_test.go
+++ b/pkg/resource/route_table/hooks_test.go
@@ -97,7 +97,7 @@ func TestCustomerPreCompare(t *testing.T) {
 				assert.ElementsMatch(t, tti.latestRoutes, diffB)
 
 				// Check the different routes are identified correctly
-				toAdd, toDelete := filterDifferentRoutes(tti.desiredRoutes, tti.latestRoutes)
+				toAdd, toDelete := getRoutesDifference(tti.desiredRoutes, tti.latestRoutes)
 				assertRoutesIdentical(t, tti.toAdd, toAdd)
 				assertRoutesIdentical(t, tti.toDelete, toDelete)
 			}

From 13385d6d547c7cabb6c83abaf6b582a13af5e0f5 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Fri, 28 Feb 2025 08:08:12 +0100
Subject: [PATCH 11/13] Fix route comparison

The CreateRouteInput type supports a lot of references to different
types. Those will not be set in latest. So if any references are used,
the DeepEqual will always fail. The dedicated compareCreateRouteInput
method will only look at the final types.
---
 pkg/resource/route_table/hooks.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index 5ddbde49..fb828cd7 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -15,7 +15,6 @@ package route_table
 
 import (
 	"context"
-	"reflect"
 	"strings"
 
 	svcapitypes "github.com/aws-controllers-k8s/ec2-controller/apis/v1alpha1"
@@ -229,8 +228,9 @@ func getRoutesDifference(desired, latest []*svcapitypes.CreateRouteInput) (toAdd
 	for _, routeA := range desired {
 		found := false
 		for idx, routeB := range toDelete {
-			if found = reflect.DeepEqual(routeA, routeB); found {
+			if delta := compareCreateRouteInput(routeA, routeB); len(delta.Differences) == 0 {
 				toDelete = remove(toDelete, idx)
+				found = true
 				break
 			}
 		}

From 9843ec38f02871847688e27c3f2daaea579cd3e7 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Fri, 28 Feb 2025 10:19:38 +0100
Subject: [PATCH 12/13] Fix issue with NPE

`latest` is nil if the syncRoutes method is called from createRoutes.
---
 pkg/resource/route_table/hooks.go | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index fb828cd7..531a32d9 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -62,7 +62,14 @@ func (rm *resourceManager) syncRoutes(
 
 	// Get the routes that need to be added and deleted, by checking for
 	// differences between desired and latest.
-	toAdd, toDelete := getRoutesDifference(desired.ko.Spec.Routes, latest.ko.Spec.Routes)
+	var toAdd, toDelete []*svcapitypes.CreateRouteInput
+	if latest != nil {
+		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.

From ece9faab594de8c62fbb41c9d0b4251c5c33f940 Mon Sep 17 00:00:00 2001
From: Gereon Frey <gfrey@adobe.com>
Date: Mon, 3 Mar 2025 11:07:58 +0100
Subject: [PATCH 13/13] Remove local routes

This got lost somewhere on the way, so re-add it now.
---
 pkg/resource/route_table/hooks.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go
index 531a32d9..a1ffcfcd 100644
--- a/pkg/resource/route_table/hooks.go
+++ b/pkg/resource/route_table/hooks.go
@@ -48,12 +48,14 @@ func (rm *resourceManager) syncRoutes(
 	defer func(err error) { exit(err) }(err)
 
 	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