perf: Performance enhancement for adding many rules to Linux IPtables chain #1644
Conversation
fgschwan
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1644 +/- ##
==========================================
+ Coverage 57.74% 58.11% +0.37%
==========================================
Files 495 293 -202
Lines 40579 23848 -16731
==========================================
- Hits 23431 13859 -9572
+ Misses 14693 8858 -5835
+ Partials 2455 1131 -1324
|
…ules into linux netfilter (iptables) Signed-off-by: Filip Gschwandtner <filip.gschwandtner@pantheon.tech>
| if err != nil { | ||
| d.log.Errorf("Error by appending iptables rule: %v", err) | ||
| break | ||
| if len(rch.Rules) > 0 { |
There was a problem hiding this comment.
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 invoking iptables 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 to IPTablesAPI 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 to IPTablesAPI that configures this from descriptor and might possibly be ignored by some handler implementations.
DISCLAIMER:
I have not gone through entire IPTablesAPI or iptables command usage and there might quite possibly be other methods that are tightly coupled with iptables 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/nftables
There was a problem hiding this comment.
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 the handler: 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.
Signed-off-by: Filip Gschwandtner <filip.gschwandtner@pantheon.tech>
|
Kudos, SonarCloud Quality Gate passed!
|
The performance enhancement is based on usage of
iptables-save/iptables-restore. It can speed up insertion of rules into chain significantly, but it is different approach then use of more genericiptablestool.Due to changing
iptables-save-exported data that are in not well documented format (not an API in classic sense), i made this performance enhancement optional and by default turned off.The usage of this method is regulated by rule count that we want to insert into chain. So inserting few rules that can be quick also with generic
iptablestool can benefit from stability ofiptablestool API. Inserting many rules will switch to performance method and can be done much more quickly, but with possibility of future incompatibility break. The data export format can change without notice, but i don't think that it will break anytime soon, because:a) this format seems to quite mature and stable (didn't find signs of changes in last years)
b) i don't do massive data parsing - i just insert data into one place