diff --git a/diff/diff.go b/diff/diff.go index c9bc97e66..214945ffd 100644 --- a/diff/diff.go +++ b/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 --git a/diff/order.go b/diff/order.go index 8e1bff6b0..064ed5d1e 100644 --- a/diff/order.go +++ b/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 --git a/diff/order_test.go b/diff/order_test.go index b555b72f5..a92398150 100644 --- a/diff/order_test.go +++ b/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) +} diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index 0fef8b49a..4d5b69907 100644 --- a/tests/integration/sync_test.go +++ b/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) + }) + } +} diff --git a/tests/integration/testdata/sync/020-same-names-altered-ids/1-before.yaml b/tests/integration/testdata/sync/020-same-names-altered-ids/1-before.yaml new file mode 100644 index 000000000..22a367bf4 --- /dev/null +++ b/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 diff --git a/tests/integration/testdata/sync/020-same-names-altered-ids/2-before.yaml b/tests/integration/testdata/sync/020-same-names-altered-ids/2-before.yaml new file mode 100644 index 000000000..7bbcd6b14 --- /dev/null +++ b/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 diff --git a/tests/integration/testdata/sync/020-same-names-altered-ids/3-before.yaml b/tests/integration/testdata/sync/020-same-names-altered-ids/3-before.yaml new file mode 100644 index 000000000..577dbd5b2 --- /dev/null +++ b/tests/integration/testdata/sync/020-same-names-altered-ids/3-before.yaml @@ -0,0 +1,26 @@ +_format_version: "3.0" +services: + - id: 98076db2-28b6-423b-ba39-a797193017f7 + name: s1 + 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 diff --git a/tests/integration/testdata/sync/020-same-names-altered-ids/desired.yaml b/tests/integration/testdata/sync/020-same-names-altered-ids/desired.yaml new file mode 100644 index 000000000..25bb0f055 --- /dev/null +++ b/tests/integration/testdata/sync/020-same-names-altered-ids/desired.yaml @@ -0,0 +1,26 @@ +_format_version: "3.0" +services: + - id: 98076db2-28b6-423b-ba39-a797193017f7 + name: s1 + host: "mockbin.org" + routes: + - id: 97b6a97e-f3f7-4c47-857a-7464cb9e202b + name: r1 + paths: + - /r1 +consumers: + - id: 9a1e49a8-2536-41fa-a4e9-605bf218a4fa + username: c1 +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 diff --git a/types/consumer.go b/types/consumer.go index cf1c7406f..9e2e52266 100644 --- a/types/consumer.go +++ b/types/consumer.go @@ -158,3 +158,44 @@ func (d *consumerDiffer) createUpdateConsumer(consumer *state.Consumer) (*crud.E } return nil, nil } + +func (d *consumerDiffer) DuplicatesDeletes() ([]crud.Event, error) { + targetConsumers, err := d.targetState.Consumers.GetAll() + if err != nil { + return nil, fmt.Errorf("error fetching services from state: %w", err) + } + + var events []crud.Event + for _, targetConsumer := range targetConsumers { + event, err := d.deleteDuplicateConsumer(targetConsumer) + if err != nil { + return nil, err + } + if event != nil { + events = append(events, *event) + } + } + + return events, nil +} + +func (d *consumerDiffer) deleteDuplicateConsumer(targetConsumer *state.Consumer) (*crud.Event, error) { + currentConsumer, err := d.currentState.Consumers.Get(*targetConsumer.Username) + if err == state.ErrNotFound { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("error looking up consumer %q: %w", + *targetConsumer.Username, err) + } + + if *currentConsumer.ID != *targetConsumer.ID { + return &crud.Event{ + Op: crud.Delete, + Kind: "consumer", + Obj: currentConsumer, + }, nil + } + + return nil, nil +} diff --git a/types/core.go b/types/core.go index 82bfa199d..b914673bd 100644 --- a/types/core.go +++ b/types/core.go @@ -14,6 +14,12 @@ type Differ interface { CreateAndUpdates(func(crud.Event) error) error } +type DuplicatesDeleter interface { + // DuplicatesDeletes returns delete events for entities that have duplicates in the current and target state. + // A duplicate is defined as an entity with the same name but different ID. + DuplicatesDeletes() ([]crud.Event, error) +} + type Entity interface { Type() EntityType CRUDActions() crud.Actions diff --git a/types/route.go b/types/route.go index 946153e9e..8477508f6 100644 --- a/types/route.go +++ b/types/route.go @@ -165,3 +165,43 @@ func (d *routeDiffer) createUpdateRoute(route *state.Route) (*crud.Event, error) } return nil, nil } + +func (d *routeDiffer) DuplicatesDeletes() ([]crud.Event, error) { + targetRoutes, err := d.targetState.Routes.GetAll() + if err != nil { + return nil, fmt.Errorf("error fetching services from state: %w", err) + } + + var events []crud.Event + for _, route := range targetRoutes { + event, err := d.deleteDuplicateRoute(route) + if err != nil { + return nil, err + } + if event != nil { + events = append(events, *event) + } + } + + return events, nil +} + +func (d *routeDiffer) deleteDuplicateRoute(targetRoute *state.Route) (*crud.Event, error) { + currentRoute, err := d.currentState.Routes.Get(*targetRoute.Name) + if err == state.ErrNotFound { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("error looking up route %q: %w", *targetRoute.Name, err) + } + + if *currentRoute.ID != *targetRoute.ID { + return &crud.Event{ + Op: crud.Delete, + Kind: "route", + Obj: currentRoute, + }, nil + } + + return nil, nil +} diff --git a/types/service.go b/types/service.go index 3bfee19f3..fb3618813 100644 --- a/types/service.go +++ b/types/service.go @@ -58,7 +58,7 @@ func (s *serviceCRUD) Update(ctx context.Context, arg ...crud.Arg) (crud.Arg, er event := crud.EventFromArg(arg[0]) service := serviceFromStruct(event) - updatedService, err := s.client.Services.Create(ctx, &service.Service) + updatedService, err := s.client.Services.Update(ctx, &service.Service) if err != nil { return nil, err } @@ -116,12 +116,12 @@ func (d *serviceDiffer) CreateAndUpdates(handler func(crud.Event) error) error { } for _, service := range targetServices { - n, err := d.createUpdateService(service) + event, err := d.createUpdateService(service) if err != nil { return err } - if n != nil { - err = handler(*n) + if event != nil { + err = handler(*event) if err != nil { return err } @@ -157,3 +157,80 @@ func (d *serviceDiffer) createUpdateService(service *state.Service) (*crud.Event } return nil, nil } + +func (d *serviceDiffer) DuplicatesDeletes() ([]crud.Event, error) { + targetServices, err := d.targetState.Services.GetAll() + if err != nil { + return nil, fmt.Errorf("error fetching services from state: %w", err) + } + var events []crud.Event + for _, service := range targetServices { + serviceEvents, err := d.deleteDuplicateService(service) + if err != nil { + return nil, err + } + events = append(events, serviceEvents...) + } + + return events, nil +} + +func (d *serviceDiffer) deleteDuplicateService(targetService *state.Service) ([]crud.Event, error) { + currentService, err := d.currentState.Services.Get(*targetService.Name) + if err == state.ErrNotFound { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("error looking up service %q: %w", + *targetService.Name, err) + } + + if *currentService.ID != *targetService.ID { + // Found a duplicate, delete it along with all routes and plugins associated with it. + var events []crud.Event + + // We have to delete all routes beforehand as otherwise we will get a foreign key error when deleting the service + // as routes are not deleted by the cascading delete of the service. + // See https://github.com/Kong/kong/discussions/7314 for more details. + routesToDelete, err := d.currentState.Routes.GetAllByServiceID(*currentService.ID) + if err != nil { + return nil, fmt.Errorf("error looking up routes for service %q: %w", + *currentService.Name, err) + } + + for _, route := range routesToDelete { + // We have to delete all plugins associated with the route to make sure they'll be recreated eventually. + // Plugins are deleted by the cascading delete of the route and without us generating a delete event manually, + // they could not be later recreated in createUpdates stage of the diff. + // By generating a delete event for each plugin, we make sure that the implicit deletion of plugins is handled + // in the local state and createUpdate stage can recreate them. + pluginsToDelete, err := d.currentState.Plugins.GetAllByRouteID(*route.ID) + if err != nil { + return nil, fmt.Errorf("error looking up plugins for route %q: %w", + *route.Name, err) + } + + for _, plugin := range pluginsToDelete { + events = append(events, crud.Event{ + Op: crud.Delete, + Kind: "plugin", + Obj: plugin, + }) + } + + events = append(events, crud.Event{ + Op: crud.Delete, + Kind: "route", + Obj: route, + }) + } + + return append(events, crud.Event{ + Op: crud.Delete, + Kind: "service", + Obj: currentService, + }), nil + } + + return nil, nil +}