Periodically verify that all required iptables rules are present in the Node

[antoninbas]

Describe what you are trying to solve
This is similar to #627 in spirit, but for iptables rules instead of gw0 routes.

Describe the solution you have in mind
We should verify that all iptables rules required by Antrea are present, in case some other process removes them (or a rule is removed manually by mistake). kube-proxy does something similar: https://github.com/kubernetes/kubernetes/blob/v1.18.2/pkg/proxy/iptables/proxier.go#L503

Describe how your solution impacts user flows
Nothing changes for the user, Antrea becomes more robust.

Describe the main design/architecture of your solution
A new go routine in RouteClient to sync-up iptables rules periodically.

Test plan
A new e2e test can be added to check that an iptables rule "owned" by Antrea gets added back within a reasonable amount of time when it is deleted manually.

[agiannif]

I can take a shot at this!

[antoninbas]

Awesome @agiannif! Looking forward to your contribution.

[siddhant94]

hey @antoninbas , is this still in pipeline for implementation?
If yes and if @agiannif has not already started on this, I would like to try work on this.

[agiannif]

Hey @siddhant94, I had to drop this. Feel free to take it up!

[siddhant94]

hey @antoninbas ,apologies for the delay. For the implementation part, I kept track of all rules that are applied in RouteClient while initialization. As part of initialization,I have added a sync loop, a goroutine which given a specified interval runs a check for every rule we have bookmarked in init iptables phase.
The above check is run using go-iptables' Exists() method]. But this is returning incorrect results for some rules (Refer).
So, I logged which rules were not present according to exists fn, and checked them manually to confirm if they were present in the tables.
I'll look into it further to see if I am doing something wrong or missing some caveat. Alternative way is, was can use the List() fn call for table/chain and write a check/match logic to compare if rulespec was found in the list or not.

[antoninbas]

@siddhant94 I don't think you want to use Exists() for every single rule, especially if each call takes the iptables lock. I would recommend the same approach as the kube-proxy iptables proxier instead: https://github.com/kubernetes/kubernetes/blob/v1.18.2/pkg/proxy/iptables/proxier.go#L1543. I believe they re-build the set of rules and use one iptables restore call.

[siddhant94]

@siddhant94 I don't think you want to use Exists() for every single rule, especially if each call takes the iptables lock. I would recommend the same approach as the kube-proxy iptables proxier instead: https://github.com/kubernetes/kubernetes/blob/v1.18.2/pkg/proxy/iptables/proxier.go#L1543. I believe they re-build the set of rules and use one iptables restore call.

@antoninbas Should iptables restore be called everytime sync runs? Or we do it only if we find a rule missing? And also, what do you think should be the sync period?

fyi, kube-proxy does it via iptables restore in fn syncProxyRules()(as you mentioned), whereas kubelet checks/ensures every rule individually via fn syncNetworkUtil

[antoninbas]

I wasn't aware of the kubelet code. I think for a small number of rules, either approach can work fine. It seems that the current implementation of initIPTables in Antrea uses a mix of iptables-restore (whenever possible) and EnsureRule (as a fallback for link Antrea-specific chains with default chains, and avoid overriding rules not managed by Antrea).
Based on existing comments, it seems there is a preference for using iptables-restore when possible:

https://github.com/vmware-tanzu/antrea/blob/fd10274e9fcae90847d750e9be9d698ec9fc6479/pkg/agent/route/route_linux.go#L234-L237

BTW, it seems that the simplest solution would be to simply call initIPTables periodically in a go routine (maybe a rename of the function would be appropriate). The code does say that the function is idempotent and can be called multiple times, so I would suggest trying this approach first.

[siddhant94]

Sure @antoninbas . So I am already using the initIPTables method, it's just that I have added complexity of looping through existing rules and then making a decision.I'll remove this part, so we would just set a sync-period and the go routine would at every sync-period run Antrea's current initIPTables(with name changed).
I'll raise the PR.

[siddhant94]

Linked the PR @antoninbas . For test, I have extended the TestInitialize in route_test.go (agent). For now I am testing with just one testcase. I'll wait for review comments, then will update collectively more cases if needed.