Fix VirtualRouter route deletion sequence #768
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This addresses a few rough edges with updating a VirtualRouter. After this change, it is possible to update the listener protocol on the CRD in one step. It is also possible to remove the listener at the same time as the routes which use that listener.
When running this against a test cluster, discovered that the logic must filter to only names with SDK routes before adding them to a deletion list.
dhild
changed the title
|
|
||
| return unmatchedSDKRouteRefs | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we get unit tests covering remove and this function?
There was a problem hiding this comment.
Sure, I'll get some test coverage written up for those.
Adds unit test coverage for the new functions. A possible nil pointer dereference is fixed, which could occur if port matching is not utilized.
The API call to describe the route is not needed, as the route ref contains all the information needed to delete routes.
BennettJames
approved these changes
Apr 17, 2024
|
Merged. It might be a bit before we release this as the default, but we can try to put up a version you can use in the meantime with a helm override if that'd be useful. |
|
Great, thanks! I would appreciate having an official image to run these changes in test environments, if it’s not too much trouble. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses a few rough edges with updating a
VirtualRouter, by removing routes which will be deleted before updating the listeners. After this change, it is possible to update the listener protocol on the CRD in one step. It is also possible to remove the listener at the same time as the routes which use that listener.Issue #, if available: #767
Description of changes:
Adds a step to the
VirtualRouterreconciliation sequence which identifies the routes to remove and deletes them before the corresponding listeners are updated. This allows a greater range of valid API changes to be successfully reconciled.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.