Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow updating entities' IDs while keeping their names #918

Merged
merged 5 commits into from
May 30, 2023

Conversation

czeslavo
Copy link
Contributor

@czeslavo czeslavo commented May 22, 2023

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.

The problematic part of the implementation is how Services are handled - we cannot safely delete them as they might be referred by Routes. When they do, deletes fail as there's no cascade delete for them configured and foreign key constraint is violated. Because of that, in the Service's deleteDuplicateService implementation, we have to manually generate delete events for the associated Routes and, in consequence, their Plugins. It is to make sure they'll be recreated in the later stage of the diff command - createUpdate.

That gives us more headache as that means we generate events that shouldn't be handled in parallel in the case of Service. Because of that, higher level events rearranging needs to happen to make sure they are all handled in the expected reverseOrder, just as the regular delete events.

It's a prerequisite for solving Kong/kubernetes-ingress-controller#4025.

@CLAassistant
Copy link

CLAassistant commented May 22, 2023

CLA assistant check
All committers have signed the CLA.

@czeslavo czeslavo force-pushed the delete-duplicates-step branch 2 times, most recently from 6daf13e to fa38b2b Compare May 22, 2023 23:55
@czeslavo czeslavo changed the title feat: allow updating entities' ids while keeping their names feat: allow updating entities' IDs while keeping their names May 22, 2023
@czeslavo czeslavo force-pushed the delete-duplicates-step branch 2 times, most recently from 35dadc6 to b8abb2c Compare May 23, 2023 00:09
@Kong Kong deleted a comment from CLAassistant May 23, 2023
@czeslavo czeslavo self-assigned this May 23, 2023
@czeslavo
Copy link
Contributor Author

It's ready for initial review - I'd like to validate whether the path I'm following here is sane and if there's no simpler way to achieve the possibility of changing entities' IDs without changing their names.

I'd much appreciate input from @hbagdi who authored most of the code I was touching here. I was doing my best to not break existing scenarios after reading your comment on ec56add commit saying:

Another change is create and update operations are now performed before
delete operations.
This is to cover the following case:

  • assume service s1 with route r1 is present in kong
  • an update needs to perform: delete s1, create s2, associate r1 to s2

This operation will fail if delete is first performed since s1 can't be
deleted as it has r1 associated with it.

But if s2 is created first, then, r1 is moved from s1 to s2. then s1 can
be deleted safely.

@czeslavo czeslavo added feature New feature or request status/wip Work in progress to address the issue labels May 23, 2023
@czeslavo czeslavo marked this pull request as ready for review May 23, 2023 00:30
@czeslavo czeslavo requested a review from a team as a code owner May 23, 2023 00:30
@czeslavo czeslavo requested a review from a team May 23, 2023 00:32
types/consumer.go Outdated Show resolved Hide resolved
types/route.go Outdated Show resolved Hide resolved
@czeslavo czeslavo force-pushed the delete-duplicates-step branch 4 times, most recently from 8954f6f to c1594cb Compare May 24, 2023 18:23
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Patch coverage: 30.13% and project coverage change: +0.01 🎉

Comparison is base (bc3e247) 35.57% compared to head (8418219) 35.59%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #918      +/-   ##
==========================================
+ Coverage   35.57%   35.59%   +0.01%     
==========================================
  Files          92       92              
  Lines       11279    11343      +64     
==========================================
+ Hits         4013     4038      +25     
- Misses       6873     6912      +39     
  Partials      393      393              
Impacted Files Coverage Δ
diff/diff.go 0.00% <0.00%> (ø)
diff/order.go 93.61% <95.65%> (+14.45%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@aboudreault
Copy link
Contributor

@czeslavo I am currently testing this impl. Looks like there is a regression (sad that no test catched that too). It seems that updating an entity (with a definition that contains the id) is failing now.
Config file:

_format_version: "3.0"
_konnect:
  runtime_group_name: default
services:
- connect_timeout: 60000
  enabled: true
  host: mockbin.org
  id: c75a775b-3a32-4b73-8e05-f68169c23941
  name: s1
  port: 80
  protocol: http
  read_timeout: 60000
  retries: 5
  write_timeout: 60000

deck sync .... then modify a field and try to sync again. you should get:

Error: 1 errors occurred:
        while processing event: {Update} service s1 failed: HTTP status 404 (message: "Cannot PATCH 

Note that I am testing on konnect and that patch is not supported. However the main branch is doing a put properly... so there is probably something to double check.

@czeslavo
Copy link
Contributor Author

@aboudreault Cool! It's great you were able to catch it in manual tests. Indeed I made this change unnecessarily:

+       updatedService, err := s.client.Services.Update(ctx, &service.Service)
-       updatedService, err := s.client.Services.Create(ctx, &service.Service)

I added a test case that covers updates of the entities with explicitly assigned IDs and fixed the regression.

@czeslavo czeslavo removed HOLD/DO NOT MERGE status/wip Work in progress to address the issue labels May 26, 2023
@aboudreault
Copy link
Contributor

thanks @czeslavo! I will test this again on Monday.

Copy link
Contributor

@aboudreault aboudreault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job here @czeslavo! I spend some time to review this change, did some additional testing and overall it seems to work well now. One thing I noticed is that this "bug" also exists with all other entities that have more than the ID as unique constraint (e.g. Upstream). We should create a follow-up ticket for these ones.

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor

@mmorel-35 mmorel-35 May 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi !
Adding errorlint linter to golangci-lint could help you ensure that accross the project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, will look to add this one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref: #923

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

@czeslavo
Copy link
Contributor Author

@aboudreault I addressed all your suggestions and added a changelog entry, also created a tracking issue to cover the other entities (#922).

@aboudreault aboudreault merged commit b5d2c99 into main May 30, 2023
33 checks passed
@aboudreault aboudreault deleted the delete-duplicates-step branch May 30, 2023 10:49
AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants