-
Notifications
You must be signed in to change notification settings - Fork 125
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
perf: Performance enhancement for adding many rules to Linux IPtables chain #1644
Merged
Merged
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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 file contains 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 file contains 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 file contains 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 file contains 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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this implementation of optimized rule appending really need to be in descriptor? From descriptor's perspective it should not really matter how the rules are added.
The handler API should hide implementation details like this behind abstracted interface, so it can be swapped out with different handler implementation in the future without affecting higher-level code.
EXAMPLE:
Someone adds new handler implementation for
IPTablesAPI
which instead of invokingiptables
command to configure IP tables, rather calls relevant syscalls in kernel.PROBLEM:
Descriptor for IP tables rulechain has to be changed as well because it concatenates raw arguments to
iptables
command.SOLUTION:
Add
AppendRules
method toIPTablesAPI
handler which accepts list of rules and the handler implementation will decide how to append those rules.If you are worried about the config option (
minRuleCountForPerfRuleAddition
), you can easily add another method toIPTablesAPI
that configures this from descriptor and might possibly be ignored by some handler implementations.DISCLAIMER:
I have not gone through entire
IPTablesAPI
oriptables
command usage and there might quite possibly be other methods that are tightly coupled withiptables
command. Either way we should start abstracting those parts away so we could possibly start using this pure Go iptables successor: https://github.com/google/nftablesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this should be hidden, so i changed that in 2102934 .
However, I didn't wanted to polute API with one method setting
minRuleCountForPerfRuleAddition
so i added config into handler initialization so it can grab it from there. Unfortunately there has been a dependency cycle so i split the config structure into 2 structures to prevent that. Hence thehandler:
in plugin config. Despite the config change, this seem to me cleaner than to change the API as you suggested.Btw. the google/nftables uses iptables successor in linux kernel. That mean other syntax (currently the model of iptablesplugin has raw arguments tied to iptables tool as you mentioned), rename of all "iptables" strings in the code and so on. So switch to nftables will be big change break anyway. It more feels like reimplementing the whole plugin.