diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 2b8435fa..e410b320 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/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 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/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/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/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) } diff --git a/pkg/resource/route_table/hooks.go b/pkg/resource/route_table/hooks.go index bacf8f0e..a1ffcfcd 100644 --- a/pkg/resource/route_table/hooks.go +++ b/pkg/resource/route_table/hooks.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,6 +199,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 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, @@ -277,6 +209,46 @@ func customPreCompare( ) { 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 diff --git a/pkg/resource/route_table/hooks_test.go b/pkg/resource/route_table/hooks_test.go new file mode 100644 index 00000000..79c78f0c --- /dev/null +++ b/pkg/resource/route_table/hooks_test.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) + } + }) + } +}