parser: check for route duplicates (LP: #2003061) - #320
Merged
Conversation
We currently check if the number of routes we have in the list is the same as the number of entries in the yaml structure. This cause problems when routes come from different files, see LP#2003061. In this case, the second route is not added to the netdef. This patch changes the logic to actually check if the route being processed is already in the list. For the uniqueness check, it compares the route table ID, metric, from, to and via fields.
slyon
suggested changes
Feb 1, 2023
slyon
left a comment
slyon
left a comment
Contributor
There was a problem hiding this comment.
Thanks, I like this approach. But I think we need to work a bit more on the route matching logic. Everything else are just small suggestions inline.
While on it, we might consider refactoring some of this code, by introducing a reset_route helper (similar to reset_ip_rule). Also, what about handle_ip_roules in general? Wouldn't the routing-policy handling have the same problem of duplicates?
Comment on lines
+881
to
+882
| * XXX: in the future we could add a route "key" to a hash set so this verification could | ||
| * be done faster. |
added 2 commits
February 3, 2023 15:56
Take into account that the destination could be 'default' or '0.0.0.0/0' (or '::/0' for IPv6). In this case we should consider them the same. Add the family to the matching criteria as well.
slyon
changed the title
slyon
approved these changes
Feb 6, 2023
slyon
left a comment
slyon
left a comment
Contributor
There was a problem hiding this comment.
Thanks, lgtm. The refactoring and potential improvement of ip_rules can be done at later point in time. Let's keep this fix isolated.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
We currently check if the number of routes we have in the list is the same as the number of entries in the yaml structure. This cause problems when routes come from different files, see LP#2003061. In this case, the second route is not added to the netdef.
This patch changes the logic to actually check if the route being processed is already in the list. For the uniqueness check, it compares the route IP family, table ID, metric, from, to and via fields.
Description
Checklist
make checksuccessfully.make check-coverage).