feat: allow updating entities' IDs while keeping their names

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