From e9b281c7022c9f7d412b07dc3ff020e0fc40de6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Thu, 18 May 2023 16:11:30 +0200 Subject: [PATCH 1/5] feat: allow updating entities' IDs while keeping their names --- diff/diff.go | 73 ++++++++- diff/order.go | 39 ++++- diff/order_test.go | 41 +++++ tests/integration/sync_test.go | 144 +++++++++++++++++- tests/integration/test_utils.go | 1 + .../020-same-names-altered-ids/1-before.yaml | 26 ++++ .../020-same-names-altered-ids/2-before.yaml | 26 ++++ .../020-same-names-altered-ids/3-before.yaml | 26 ++++ .../020-same-names-altered-ids/desired.yaml | 26 ++++ types/consumer.go | 41 +++++ types/core.go | 6 + types/route.go | 40 +++++ types/service.go | 96 +++++++++++- 13 files changed, 571 insertions(+), 14 deletions(-) create mode 100644 tests/integration/testdata/sync/020-same-names-altered-ids/1-before.yaml create mode 100644 tests/integration/testdata/sync/020-same-names-altered-ids/2-before.yaml create mode 100644 tests/integration/testdata/sync/020-same-names-altered-ids/3-before.yaml create mode 100644 tests/integration/testdata/sync/020-same-names-altered-ids/desired.yaml 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 62a0496c3..6de27dcd3 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,140 @@ func Test_Sync_SkipConsumers(t *testing.T) { }) } } + +// test scope: +// - 3.0.0+ +// - konnect +func Test_Sync_ChangingIDsWhileKeepingNames(t *testing.T) { + client, err := getTestClient() + if err != nil { + t.Errorf(err.Error()) + } + + runWhenKongOrKonnect(t, ">=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/test_utils.go b/tests/integration/test_utils.go index 228db1e28..b21dd5c50 100644 --- a/tests/integration/test_utils.go +++ b/tests/integration/test_utils.go @@ -10,6 +10,7 @@ import ( "github.com/fatih/color" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/kong/deck/cmd" deckDump "github.com/kong/deck/dump" "github.com/kong/deck/utils" 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..6af6b2f8a 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 consumers 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..b672fae75 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 routes 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..6c063b844 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,91 @@ 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 { + if err := d.currentState.Plugins.Delete(*plugin.ID); err != nil { + return nil, fmt.Errorf("error deleting plugin %q for route %q: %w", + *route.Name, *plugin.Name, err) + } + // REVIEW: Should we generate a delete event for the plugin? It will always result in a DELETE + // call for this plugin while it could be just a local state change as we already know it's going + // to be deleted by the cascading delete of the route. + // + // It's also problematic when syncing with Konnect as Koko returns 404 when trying to delete a plugin + // that doesn't exist. It's not an issue when syncing with Kong as Kong returns 204. + // + //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 +} From 9a47903114351d13ee1dd9be384fd26a0e88dbf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Wed, 24 May 2023 19:31:05 +0200 Subject: [PATCH 2/5] move implicit cascade deletes of plugins to postActions --- tests/integration/test_utils.go | 1 - types/postProcess.go | 45 ++++++++++++++++++++++++++++++--- types/service.go | 31 ----------------------- 3 files changed, 42 insertions(+), 35 deletions(-) diff --git a/tests/integration/test_utils.go b/tests/integration/test_utils.go index b21dd5c50..228db1e28 100644 --- a/tests/integration/test_utils.go +++ b/tests/integration/test_utils.go @@ -10,7 +10,6 @@ import ( "github.com/fatih/color" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" - "github.com/kong/deck/cmd" deckDump "github.com/kong/deck/dump" "github.com/kong/deck/utils" diff --git a/types/postProcess.go b/types/postProcess.go index 2ec5e9f6e..453205973 100644 --- a/types/postProcess.go +++ b/types/postProcess.go @@ -2,6 +2,7 @@ package types import ( "context" + "fmt" "github.com/kong/deck/crud" "github.com/kong/deck/state" @@ -16,7 +17,20 @@ func (crud *servicePostAction) Create(_ context.Context, args ...crud.Arg) (crud } func (crud *servicePostAction) Delete(_ context.Context, args ...crud.Arg) (crud.Arg, error) { - return nil, crud.currentState.Services.Delete(*((args[0].(*state.Service)).ID)) + serviceID := *args[0].(*state.Service).ID + + // Delete all plugins associated with this service as that's the implicit behavior of Kong (cascade delete). + plugins, err := crud.currentState.Plugins.GetAllByServiceID(serviceID) + if err != nil { + return nil, fmt.Errorf("error looking up plugins for service '%v': %v", serviceID, err) + } + for _, plugin := range plugins { + err = crud.currentState.Plugins.Delete(*plugin.ID) + if err != nil { + return nil, fmt.Errorf("error deleting plugin '%v' for service '%v': %v", *plugin.ID, serviceID, err) + } + } + return nil, crud.currentState.Services.Delete(serviceID) } func (crud *servicePostAction) Update(_ context.Context, args ...crud.Arg) (crud.Arg, error) { @@ -32,7 +46,20 @@ func (crud *routePostAction) Create(_ context.Context, args ...crud.Arg) (crud.A } func (crud *routePostAction) Delete(_ context.Context, args ...crud.Arg) (crud.Arg, error) { - return nil, crud.currentState.Routes.Delete(*((args[0].(*state.Route)).ID)) + routeID := *args[0].(*state.Route).ID + + // Delete all plugins associated with this route as that's the implicit behavior of Kong (cascade delete). + plugins, err := crud.currentState.Plugins.GetAllByRouteID(routeID) + if err != nil { + return nil, fmt.Errorf("error looking up plugins for route '%v': %v", routeID, err) + } + for _, plugin := range plugins { + err = crud.currentState.Plugins.Delete(*plugin.ID) + if err != nil { + return nil, fmt.Errorf("error deleting plugin '%v' for route '%v': %v", *plugin.ID, routeID, err) + } + } + return nil, crud.currentState.Routes.Delete(routeID) } func (crud *routePostAction) Update(_ context.Context, args ...crud.Arg) (crud.Arg, error) { @@ -146,7 +173,19 @@ func (crud *consumerPostAction) Create(_ context.Context, args ...crud.Arg) (cru } func (crud *consumerPostAction) Delete(_ context.Context, args ...crud.Arg) (crud.Arg, error) { - return nil, crud.currentState.Consumers.Delete(*((args[0].(*state.Consumer)).ID)) + consumerID := *args[0].(*state.Consumer).ID + + // Delete all plugins associated with this consumer as that's the implicit behavior of Kong (cascade delete). + plugins, err := crud.currentState.Plugins.GetAllByConsumerID(consumerID) + if err != nil { + return nil, fmt.Errorf("error looking up plugins for consumer '%v': %v", consumerID, err) + } + for _, plugin := range plugins { + if err := crud.currentState.Plugins.Delete(*plugin.ID); err != nil { + return nil, fmt.Errorf("error deleting plugin '%v' for consumer '%v': %v", *plugin.ID, consumerID, err) + } + } + return nil, crud.currentState.Consumers.Delete(consumerID) } func (crud *consumerPostAction) Update(_ context.Context, args ...crud.Arg) (crud.Arg, error) { diff --git a/types/service.go b/types/service.go index 6c063b844..71f27f241 100644 --- a/types/service.go +++ b/types/service.go @@ -186,7 +186,6 @@ func (d *serviceDiffer) deleteDuplicateService(targetService *state.Service) ([] } 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 @@ -199,36 +198,6 @@ func (d *serviceDiffer) deleteDuplicateService(targetService *state.Service) ([] } 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 { - if err := d.currentState.Plugins.Delete(*plugin.ID); err != nil { - return nil, fmt.Errorf("error deleting plugin %q for route %q: %w", - *route.Name, *plugin.Name, err) - } - // REVIEW: Should we generate a delete event for the plugin? It will always result in a DELETE - // call for this plugin while it could be just a local state change as we already know it's going - // to be deleted by the cascading delete of the route. - // - // It's also problematic when syncing with Konnect as Koko returns 404 when trying to delete a plugin - // that doesn't exist. It's not an issue when syncing with Kong as Kong returns 204. - // - //events = append(events, crud.Event{ - // Op: crud.Delete, - // Kind: "plugin", - // Obj: plugin, - //}) - } - events = append(events, crud.Event{ Op: crud.Delete, Kind: "route", From 84182192e9406931dff491c30a7ed97e04677421 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Thu, 25 May 2023 23:29:36 +0200 Subject: [PATCH 3/5] cover the regression with test and fix it --- tests/integration/sync_test.go | 128 ++++++++++++------ .../021-update-with-explicit-ids/after.yaml | 18 +++ .../021-update-with-explicit-ids/before.yaml | 15 ++ types/service.go | 8 +- 4 files changed, 127 insertions(+), 42 deletions(-) create mode 100644 tests/integration/testdata/sync/021-update-with-explicit-ids/after.yaml create mode 100644 tests/integration/testdata/sync/021-update-with-explicit-ids/before.yaml diff --git a/tests/integration/sync_test.go b/tests/integration/sync_test.go index 6de27dcd3..3a3f5f085 100644 --- a/tests/integration/sync_test.go +++ b/tests/integration/sync_test.go @@ -3102,17 +3102,52 @@ func Test_Sync_SkipConsumers(t *testing.T) { } } +// In the tests we're concerned only with the IDs and names of the entities we'll ignore other fields when comparing states. +var ignoreFieldsIrrelevantForIDsTests = []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", + ), +} + // test scope: // - 3.0.0+ // - konnect func Test_Sync_ChangingIDsWhileKeepingNames(t *testing.T) { + runWhenKongOrKonnect(t, ">=3.0.0") + client, err := getTestClient() if err != nil { t.Errorf(err.Error()) } - runWhenKongOrKonnect(t, ">=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") @@ -3162,41 +3197,6 @@ func Test_Sync_ChangingIDsWhileKeepingNames(t *testing.T) { } ) - // 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 @@ -3234,7 +3234,59 @@ func Test_Sync_ChangingIDsWhileKeepingNames(t *testing.T) { Routes: []*kong.Route{expectedRoute}, Consumers: []*kong.Consumer{expectedConsumer}, Plugins: expectedPlugins, - }, ignoreTestIrrelevantFields) + }, ignoreFieldsIrrelevantForIDsTests) }) } } + +// test scope: +// - 3.0.0+ +// - konnect +func Test_Sync_UpdateWithExplicitIDs(t *testing.T) { + runWhenKongOrKonnect(t, ">=3.0.0") + + client, err := getTestClient() + if err != nil { + t.Errorf(err.Error()) + } + + const ( + beforeConfig = "testdata/sync/021-update-with-explicit-ids/before.yaml" + afterConfig = "testdata/sync/021-update-with-explicit-ids/after.yaml" + ) + + // First, create entities with IDs assigned explicitly. + err = sync(beforeConfig) + require.NoError(t, err) + + // Then, sync again, adding tags to every entity just to trigger an update. + err = sync(afterConfig) + require.NoError(t, err) + + // Finally, verify that the update was successful. + testKongState(t, client, false, utils.KongRawState{ + Services: []*kong.Service{ + { + Name: kong.String("s1"), + ID: kong.String("c75a775b-3a32-4b73-8e05-f68169c23941"), + Tags: kong.StringSlice("after"), + }, + }, + Routes: []*kong.Route{ + { + Name: kong.String("r1"), + ID: kong.String("97b6a97e-f3f7-4c47-857a-7464cb9e202b"), + Tags: kong.StringSlice("after"), + Service: &kong.Service{ + ID: kong.String("c75a775b-3a32-4b73-8e05-f68169c23941"), + }, + }, + }, + Consumers: []*kong.Consumer{ + { + Username: kong.String("c1"), + Tags: kong.StringSlice("after"), + }, + }, + }, ignoreFieldsIrrelevantForIDsTests) +} diff --git a/tests/integration/testdata/sync/021-update-with-explicit-ids/after.yaml b/tests/integration/testdata/sync/021-update-with-explicit-ids/after.yaml new file mode 100644 index 000000000..b0b70589a --- /dev/null +++ b/tests/integration/testdata/sync/021-update-with-explicit-ids/after.yaml @@ -0,0 +1,18 @@ +_format_version: "3.0" +services: + - enabled: true + host: mockbin.org + id: c75a775b-3a32-4b73-8e05-f68169c23941 # Leaving ID + name: s1 # and name unchanged. + port: 80 + tags: [after] + routes: + - id: 97b6a97e-f3f7-4c47-857a-7464cb9e202b # Leaving ID + name: r1 # and name unchanged. + paths: + - /r1 + tags: [after] +consumers: + - id: 9a1e49a8-2536-41fa-a4e9-605bf218a4fa # Leaving ID + username: c1 # and username unchanged. + tags: [after] diff --git a/tests/integration/testdata/sync/021-update-with-explicit-ids/before.yaml b/tests/integration/testdata/sync/021-update-with-explicit-ids/before.yaml new file mode 100644 index 000000000..6ed10ebac --- /dev/null +++ b/tests/integration/testdata/sync/021-update-with-explicit-ids/before.yaml @@ -0,0 +1,15 @@ +_format_version: "3.0" +services: + - enabled: true + host: mockbin.org + id: c75a775b-3a32-4b73-8e05-f68169c23941 # Leaving ID + name: s1 # and name unchanged. + port: 80 + routes: + - id: 97b6a97e-f3f7-4c47-857a-7464cb9e202b # Leaving ID + name: r1 # and name unchanged. + paths: + - /r1 +consumers: + - id: 9a1e49a8-2536-41fa-a4e9-605bf218a4fa # Leaving ID + username: c1 # and username unchanged. diff --git a/types/service.go b/types/service.go index 71f27f241..b44c35553 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.Update(ctx, &service.Service) + updatedService, err := s.client.Services.Create(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 { - event, err := d.createUpdateService(service) + n, err := d.createUpdateService(service) if err != nil { return err } - if event != nil { - err = handler(*event) + if n != nil { + err = handler(*n) if err != nil { return err } From 8e5b4d41430a46d7865a87f39ecd7fcbf3257fb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Tue, 30 May 2023 11:55:17 +0200 Subject: [PATCH 4/5] address review comments --- types/postProcess.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/types/postProcess.go b/types/postProcess.go index 453205973..9b9b9c2f7 100644 --- a/types/postProcess.go +++ b/types/postProcess.go @@ -22,12 +22,12 @@ func (crud *servicePostAction) Delete(_ context.Context, args ...crud.Arg) (crud // Delete all plugins associated with this service as that's the implicit behavior of Kong (cascade delete). plugins, err := crud.currentState.Plugins.GetAllByServiceID(serviceID) if err != nil { - return nil, fmt.Errorf("error looking up plugins for service '%v': %v", serviceID, err) + return nil, fmt.Errorf("error looking up plugins for service '%v': %w", serviceID, err) } for _, plugin := range plugins { err = crud.currentState.Plugins.Delete(*plugin.ID) if err != nil { - return nil, fmt.Errorf("error deleting plugin '%v' for service '%v': %v", *plugin.ID, serviceID, err) + return nil, fmt.Errorf("error deleting plugin '%v' for service '%v': %w", *plugin.ID, serviceID, err) } } return nil, crud.currentState.Services.Delete(serviceID) @@ -51,12 +51,12 @@ func (crud *routePostAction) Delete(_ context.Context, args ...crud.Arg) (crud.A // Delete all plugins associated with this route as that's the implicit behavior of Kong (cascade delete). plugins, err := crud.currentState.Plugins.GetAllByRouteID(routeID) if err != nil { - return nil, fmt.Errorf("error looking up plugins for route '%v': %v", routeID, err) + return nil, fmt.Errorf("error looking up plugins for route '%v': %w", routeID, err) } for _, plugin := range plugins { err = crud.currentState.Plugins.Delete(*plugin.ID) if err != nil { - return nil, fmt.Errorf("error deleting plugin '%v' for route '%v': %v", *plugin.ID, routeID, err) + return nil, fmt.Errorf("error deleting plugin '%v' for route '%v': %w", *plugin.ID, routeID, err) } } return nil, crud.currentState.Routes.Delete(routeID) @@ -178,11 +178,11 @@ func (crud *consumerPostAction) Delete(_ context.Context, args ...crud.Arg) (cru // Delete all plugins associated with this consumer as that's the implicit behavior of Kong (cascade delete). plugins, err := crud.currentState.Plugins.GetAllByConsumerID(consumerID) if err != nil { - return nil, fmt.Errorf("error looking up plugins for consumer '%v': %v", consumerID, err) + return nil, fmt.Errorf("error looking up plugins for consumer '%v': %w", consumerID, err) } for _, plugin := range plugins { if err := crud.currentState.Plugins.Delete(*plugin.ID); err != nil { - return nil, fmt.Errorf("error deleting plugin '%v' for consumer '%v': %v", *plugin.ID, consumerID, err) + return nil, fmt.Errorf("error deleting plugin '%v' for consumer '%v': %w", *plugin.ID, consumerID, err) } } return nil, crud.currentState.Consumers.Delete(consumerID) From 55a168c86ba43f232162dd0d423e398881743c29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Grzegorz=20Burzy=C5=84ski?= Date: Tue, 30 May 2023 12:02:34 +0200 Subject: [PATCH 5/5] add changelog entry --- CHANGELOG.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4056ac68b..a435582f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,16 @@ - [v0.2.0](#v020) - [v0.1.0](#v010) +## [v1.21.0] + +> Release date: TBD + +### Add + +- Add support for updating Services, Routes, and Consumers by changing their IDs, + but retaining their names. + [#918](https://github.com/Kong/deck/pull/918) + ## [v1.20.0] > Release date: 2023/04/24 @@ -64,7 +74,6 @@ - Add the license type to the file package. - ## [v1.19.1] > Release date: 2023/03/21