feat: allow updating entities' IDs while keeping their names

This adds an additional step to the diff.Syncer's diff producer -
deleteDuplicates. Its purpose is to clean up the entities in the current state
that have duplicates with the same names in the target state before proceeding
with updates/creates and regular deletes. Without that additional step, it's
not possible to perform sync in which the IDs of entities are changed while
their names are kept between syncs.

An actual use-case that is the motivation for this change is Kong Ingress
Controller where we recently started assigning entities' IDs explicitly.
Without this change, existing setups break when KIC is upgraded from the
version not setting the IDs to the new one because of breaking the uniqueness
constraints of Services, Routes, and Consumers.

The added test cases represent the complete use case with all the affected
entities: Services, Routes, and Consumers. We change their IDs, keeping the
names and we expect the final state on the Kong side to be using the new IDs.

diff/diff.go

@@ -166,15 +166,74 @@ func (sc *Syncer) init() error {
 }
 
 func (sc *Syncer) diff() error {
-	var err error
-	err = sc.createUpdate()
-	if err != nil {
-		return err
+	for _, operation := range []func() error{
+		sc.deleteDuplicates,
+		sc.createUpdate,
+		sc.delete,
+	} {
+		err := operation()
+		if err != nil {
+			return err
+		}
 	}
-	err = sc.delete()
-	if err != nil {
-		return err
+	return nil
+}
+
+func (sc *Syncer) deleteDuplicates() error {
+	var events []crud.Event
+	for _, ts := range reverseOrder() {
+		for _, entityType := range ts {
+			entityDiffer, ok := sc.entityDiffers[entityType].(types.DuplicatesDeleter)
+			if !ok {
+				continue
+			}
+			entityEvents, err := entityDiffer.DuplicatesDeletes()
+			if err != nil {
+				return err
+			}
+			events = append(events, entityEvents...)
+		}
 	}
+
+	return sc.processDeleteDuplicates(eventsInOrder(events, reverseOrder()))
+}
+
+func (sc *Syncer) processDeleteDuplicates(eventsByLevel [][]crud.Event) error {
+	// All entities implement this interface. We'll use it to index delete events by (kind, identifier) tuple to prevent
+	// deleting a single object twice.
+	type identifier interface {
+		Identifier() string
+	}
+	var (
+		alreadyDeleted = map[string]struct{}{}
+		keyForEvent    = func(event crud.Event) (string, error) {
+			obj, ok := event.Obj.(identifier)
+			if !ok {
+				return "", fmt.Errorf("unexpected type %T in event", event.Obj)
+			}
+			return fmt.Sprintf("%s-%s", event.Kind, obj.Identifier()), nil
+		}
+	)
+
+	for _, events := range eventsByLevel {
+		for _, event := range events {
+			key, err := keyForEvent(event)
+			if err != nil {
+				return err
+			}
+			if _, ok := alreadyDeleted[key]; ok {
+				continue
+			}
+			if err := sc.queueEvent(event); err != nil {
+				return err
+			}
+			alreadyDeleted[key] = struct{}{}
+		}
+
+		// Wait for all the deletes to finish before moving to the next level to avoid conflicts.
+		sc.wait()
+	}
+
 	return nil
 }
 

diff/order.go

@@ -1,6 +1,9 @@
 package diff
 
-import "github.com/kong/deck/types"
+import (
+	"github.com/kong/deck/crud"
+	"github.com/kong/deck/types"
+)
 
 /*
                                        Root
@@ -92,3 +95,37 @@ func deepCopy(src [][]types.EntityType) [][]types.EntityType {
 	}
 	return res
 }
+
+func eventsInOrder(events []crud.Event, order [][]types.EntityType) [][]crud.Event {
+	// kindToLevel maps a Kind to its level in the order to avoid repeated lookups.
+	kindToLevel := make(map[crud.Kind]int)
+
+	// eventsByLevel is a slice of slices of events, where each slice of events is at the same level and can be
+	// processed concurrently.
+	eventsByLevel := make([][]crud.Event, len(order))
+
+	for _, event := range events {
+		level, ok := kindToLevel[event.Kind]
+		if !ok {
+			level = levelForEvent(event, order)
+			kindToLevel[event.Kind] = level
+		}
+
+		eventsByLevel[level] = append(eventsByLevel[level], event)
+	}
+
+	return eventsByLevel
+}
+
+func levelForEvent(event crud.Event, order [][]types.EntityType) int {
+	for i, level := range order {
+		for _, entityType := range level {
+			if event.Kind == crud.Kind(entityType) {
+				return i
+			}
+		}
+	}
+
+	// This should never happen.
+	return -1
+}

diff/order_test.go

@@ -4,7 +4,9 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/kong/deck/crud"
 	"github.com/kong/deck/types"
+	"github.com/stretchr/testify/require"
 )
 
 func Test_reverse(t *testing.T) {
@@ -48,3 +50,42 @@ func Test_reverse(t *testing.T) {
 		})
 	}
 }
+
+func TestEventsInOrder(t *testing.T) {
+	e := func(entityType types.EntityType) crud.Event {
+		return crud.Event{Kind: crud.Kind(entityType)}
+	}
+
+	eventsOutOfOrder := []crud.Event{
+		e(types.Consumer),
+		e(types.Service),
+		e(types.KeyAuth),
+		e(types.Route),
+		e(types.ServicePackage),
+		e(types.ConsumerGroup),
+		e(types.ServiceVersion),
+		e(types.Plugin),
+	}
+
+	order := reverseOrder()
+	result := eventsInOrder(eventsOutOfOrder, order)
+
+	require.Equal(t, [][]crud.Event{
+		{
+			e(types.Plugin),
+		},
+		{
+			e(types.Route),
+			e(types.ServiceVersion),
+		},
+		{
+			e(types.Service),
+			e(types.KeyAuth),
+			e(types.ConsumerGroup),
+		},
+		{
+			e(types.Consumer),
+			e(types.ServicePackage),
+		},
+	}, result)
+}

tests/integration/sync_test.go

@@ -12,9 +12,12 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	"github.com/kong/deck/utils"
-	"github.com/kong/go-kong/kong"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/kong/go-kong/kong"
+
+	"github.com/kong/deck/utils"
 )
 
 var (
@@ -3098,3 +3101,139 @@ func Test_Sync_SkipConsumers(t *testing.T) {
 		})
 	}
 }
+
+// test scope:
+//   - 3.0.0+
+func Test_Sync_ChangingIDsWhileKeepingNames(t *testing.T) {
+	client, err := getTestClient()
+	if err != nil {
+		t.Errorf(err.Error())
+	}
+
+	runWhen(t, "kong", ">=3.0.0")
+
+	// These are the IDs that should be present in Kong after the second sync in all cases.
+	var (
+		expectedServiceID  = kong.String("98076db2-28b6-423b-ba39-a797193017f7")
+		expectedRouteID    = kong.String("97b6a97e-f3f7-4c47-857a-7464cb9e202b")
+		expectedConsumerID = kong.String("9a1e49a8-2536-41fa-a4e9-605bf218a4fa")
+	)
+
+	// These are the entities that should be present in Kong after the second sync in all cases.
+	var (
+		expectedService = &kong.Service{
+			Name: kong.String("s1"),
+			ID:   expectedServiceID,
+		}
+
+		expectedRoute = &kong.Route{
+			Name: kong.String("r1"),
+			ID:   expectedRouteID,
+			Service: &kong.Service{
+				ID: expectedServiceID,
+			},
+		}
+
+		expectedConsumer = &kong.Consumer{
+			Username: kong.String("c1"),
+			ID:       expectedConsumerID,
+		}
+
+		expectedPlugins = []*kong.Plugin{
+			{
+				Name: kong.String("rate-limiting"),
+				Route: &kong.Route{
+					ID: expectedRouteID,
+				},
+			},
+			{
+				Name: kong.String("rate-limiting"),
+				Service: &kong.Service{
+					ID: expectedServiceID,
+				},
+			},
+			{
+				Name: kong.String("rate-limiting"),
+				Consumer: &kong.Consumer{
+					ID: expectedConsumerID,
+				},
+			},
+		}
+	)
+
+	// In this test we're concerned only with the IDs and names of the entities.
+	ignoreTestIrrelevantFields := []cmp.Option{
+		cmpopts.IgnoreFields(
+			kong.Plugin{},
+			"Config",
+			"Protocols",
+			"Enabled",
+		),
+		cmpopts.IgnoreFields(
+			kong.Service{},
+			"ConnectTimeout",
+			"Enabled",
+			"Host",
+			"Port",
+			"Protocol",
+			"ReadTimeout",
+			"WriteTimeout",
+			"Retries",
+		),
+		cmpopts.IgnoreFields(
+			kong.Route{},
+			"Paths",
+			"PathHandling",
+			"PreserveHost",
+			"Protocols",
+			"RegexPriority",
+			"StripPath",
+			"HTTPSRedirectStatusCode",
+			"Sources",
+			"Destinations",
+			"RequestBuffering",
+			"ResponseBuffering",
+		),
+	}
+
+	testCases := []struct {
+		name         string
+		beforeConfig string
+	}{
+		{
+			name:         "all entities have the same names, but different IDs",
+			beforeConfig: "testdata/sync/020-same-names-altered-ids/1-before.yaml",
+		},
+		{
+			name:         "service and consumer changed IDs, route did not",
+			beforeConfig: "testdata/sync/020-same-names-altered-ids/2-before.yaml",
+		},
+		{
+			name:         "route and consumer changed IDs, service did not",
+			beforeConfig: "testdata/sync/020-same-names-altered-ids/3-before.yaml",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			teardown := setup(t)
+			defer teardown(t)
+
+			// First, create the entities with the original IDs.
+			err = sync(tc.beforeConfig)
+			require.NoError(t, err)
+
+			// Then, sync again with the same names, but different IDs.
+			err = sync("testdata/sync/020-same-names-altered-ids/desired.yaml")
+			require.NoError(t, err)
+
+			// Finally, check that the all entities exist and have the expected IDs.
+			testKongState(t, client, false, utils.KongRawState{
+				Services:  []*kong.Service{expectedService},
+				Routes:    []*kong.Route{expectedRoute},
+				Consumers: []*kong.Consumer{expectedConsumer},
+				Plugins:   expectedPlugins,
+			}, ignoreTestIrrelevantFields)
+		})
+	}
+}

tests/integration/testdata/sync/020-same-names-altered-ids/1-before.yaml

@@ -0,0 +1,26 @@
+_format_version: "3.0"
+services:
+  - id: 18076db2-28b6-423b-ba39-a797193017f7     # Changing ID,
+    name: s1                                     # leaving the same name.
+    host: "mockbin.org"
+    routes:
+      - id: 17b6a97e-f3f7-4c47-857a-7464cb9e202b # Changing ID,
+        name: r1                                 # leaving the same name.
+        paths:
+          - /r1
+consumers:
+  - id: 5a1e49a8-2536-41fa-a4e9-605bf218a4fa     # Changing ID,
+    username: c1                                 # leaving the same name.
+plugins:
+  - name: rate-limiting
+    config:
+      second: 1
+    service: s1
+  - name: rate-limiting
+    config:
+      second: 1
+    route: r1
+  - name: rate-limiting
+    config:
+      second: 1
+    consumer: c1

tests/integration/testdata/sync/020-same-names-altered-ids/2-before.yaml

@@ -0,0 +1,26 @@
+_format_version: "3.0"
+services:
+  - id: 18076db2-28b6-423b-ba39-a797193017f7     # Changing ID,
+    name: s1                                     # leaving the same name.
+    host: "mockbin.org"
+    routes:
+      - id: 97b6a97e-f3f7-4c47-857a-7464cb9e202b
+        name: r1
+        paths:
+          - /r1
+consumers:
+  - id: 5a1e49a8-2536-41fa-a4e9-605bf218a4fa     # Changing ID,
+    username: c1                                 # leaving the same name.
+plugins:
+  - name: rate-limiting
+    config:
+      second: 1
+    service: s1
+  - name: rate-limiting
+    config:
+      second: 1
+    route: r1
+  - name: rate-limiting
+    config:
+      second: 1
+    consumer: c1