[IPv6] Bug fix in adding iptable rules #2469

lzhecheng · 2021-07-26T09:09:35Z

bug fix in EnsureRule()
refactor iptables methods

Signed-off-by: Zhecheng Li lzhecheng@vmware.com

lzhecheng · 2021-07-26T09:10:11Z

/test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e

codecov-commenter · 2021-07-26T09:14:47Z

Codecov Report

Merging #2469 (15437d1) into main (cdc8453) will increase coverage by 5.12%.
The diff coverage is 37.03%.

@@            Coverage Diff             @@
##             main    #2469      +/-   ##
==========================================
+ Coverage   59.82%   64.95%   +5.12%     
==========================================
  Files         284      281       -3     
  Lines       22168    25534    +3366     
==========================================
+ Hits        13263    16586    +3323     
+ Misses       7483     7403      -80     
- Partials     1422     1545     +123

Flag	Coverage Δ
e2e-tests	`55.83% <37.03%> (?)`
kind-e2e-tests	`47.09% <26.15%> (+0.01%)`	⬆️
unit-tests	`42.22% <0.00%> (+0.20%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/nodeportlocal/portcache/port_table.go	`64.94% <0.00%> (+3.22%)`	⬆️
pkg/agent/nodeportlocal/rules/rules.go	`100.00% <ø> (ø)`
pkg/agent/route/route_linux.go	`50.59% <33.33%> (+5.91%)`	⬆️
pkg/agent/util/iptables/iptables.go	`43.55% <35.59%> (+6.15%)`	⬆️
pkg/agent/nodeportlocal/rules/iptable_rule.go	`59.72% <47.05%> (+7.17%)`	⬆️
pkg/agent/agent_linux.go	`80.00% <0.00%> (-20.00%)`	⬇️
pkg/controller/egress/ipallocator/allocator.go	`67.82% <0.00%> (-15.16%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`77.64% <0.00%> (-13.79%)`	⬇️
pkg/apis/controlplane/v1beta1/conversion.go	`72.44% <0.00%> (-11.89%)`	⬇️
pkg/util/ip/ip.go	`68.67% <0.00%> (-11.63%)`	⬇️
... and 273 more

pkg/agent/util/iptables/iptables.go

lzhecheng · 2021-07-28T03:03:26Z

/test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e /test-ipv6-only-e2e

lzhecheng · 2021-07-28T05:35:06Z

@antoninbas @tnqn I refactored some iptables methods. PTAL.

test/e2e/flowaggregator_test.go

test/e2e/networkpolicy_test.go

pkg/agent/route/route_linux.go

pkg/agent/util/iptables/iptables.go

lzhecheng · 2021-07-29T08:53:27Z

/test-e2e /test-conformance /test-networkpolicy /test-ipv6-e2e /test-ipv6-only-e2e

antoninbas

latest changes look good to me overall

pkg/agent/nodeportlocal/rules/iptable_rule.go

lzhecheng · 2021-07-30T01:22:44Z

/test-all /test-ipv6-all /test-ipv6-only-all

antoninbas · 2021-07-30T18:48:18Z

@tnqn PTAL

@lzhecheng please cherry-pick this as needed (at least v1.2, but potentially all the way back to v0.13 if appropriate)

pkg/agent/nodeportlocal/rules/iptable_rule.go

pkg/agent/util/iptables/iptables.go

tnqn · 2021-08-02T04:58:22Z

pkg/agent/util/iptables/iptables.go

-	for idx := range c.ipts {
-		err := c.ipts[idx].ClearChain(table, chain)
+// DeleteMatchingChains deletes all rules from chains in all tables and then delete the chains.
+func (c *Client) DeleteMatchingChains(table string, chain string) error {


ditto.

The comment about "delete all rules from chains in all tables" is incorrect.

I think if we specify "protocol" like EnsureChain, AppendRule and InsertRule, it will make same to name the delete method DeleteChain and DeleteRule? @antoninbas

Comment is updated.

@antoninbas please take a look at @tnqn 's suggestion.

yes if we pass the protocol like for the insertion functions, we can use the same naming scheme I think

pkg/agent/route/route_linux.go

lzhecheng · 2021-08-04T00:04:25Z

/test-e2e /test-conformance /test-networkpolicy /test-ipv6-all /test-ipv6-only-all

pkg/agent/util/iptables/iptables.go

lzhecheng · 2021-08-04T12:17:14Z

/test-all /test-ipv6-all /test-ipv6-only-all

antoninbas

LGTM, save for the typo

antoninbas · 2021-08-04T17:43:56Z

pkg/agent/route/route_linux.go

@@ -771,5 +772,5 @@ func (c *Client) DeleteSNATRule(mark uint32) error {
 	}
 	c.markToSNATIP.Delete(mark)
 	snatIP := value.(net.IP)
-	return c.ipt.DeleteRule(iptables.NATTable, antreaPostRoutingChain, c.snatRuleSpec(snatIP, mark))
+	return c.ipt.DeleteRules(iptables.ProtocolDual, iptables.NATTable, antreaPostRoutingChain, c.snatRuleSpec(snatIP, mark))


this should still be DeleteRule?

Thank you for catching this. Updated.

* bug fix in EnsureRule() * refactor iptables methods Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

lzhecheng · 2021-08-05T00:52:14Z

/test-all /test-ipv6-all /test-ipv6-only-all

tnqn · 2021-08-05T09:12:01Z

pkg/agent/nodeportlocal/rules/iptable_rule.go

-	if !exists {
-		return nil
+	if err := ipt.table.DeleteChain(iptables.ProtocolIPv4, iptables.NATTable, NodePortLocalChain); err != nil {
+		return err


I did a test, deleting a chain which was still referenced by another rule always fails with "Too many links" error, so this would introduce a regression. Is this method covered by any test?

Did you do it manually? "Too many links" is expected in this way.

Anyway, it seems DeleteChain is only used in DeleteAllRules and DeleteAllRules is used by no one. I am going to delete them.

I don't think it's expected. The original code deletes the chain at last so it ensures the chain is not referenced by any rule when deleting it. This patch changes to delete the chain in the beginning so it would introduce a regression. I'm not why there there is an orphaned method here, maybe it's left for historical reason, or maybe it's planed for future usage. Anyway it's beyond the scope of this PR. I would sugges to keep it as is if you want to merge this earlier. Otherwise I would invite the feature owner to comment whether deleting it is expected. If you keep the method, I would expect it wouldn't introduce a bug by this PR.

DeleteAllRules and ChainExists are reverted.

Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

tnqn

LGTM

tnqn · 2021-08-05T12:17:05Z

/test-all

* [IPv6] Bug fix in adding iptable rules * bug fix in EnsureRule() * refactor iptables methods Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

* Bug fix in adding iptable rules * bug fix in EnsureRule() * refactor iptables methods Signed-off-by: Zhecheng Li <lzhecheng@vmware.com> * Revert ChainExists() and DeleteAllRules() Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

lzhecheng requested review from jianjuns, tnqn and wenyingd July 26, 2021 11:11

antoninbas reviewed Jul 26, 2021

View reviewed changes

pkg/agent/util/iptables/iptables.go Show resolved Hide resolved

lzhecheng force-pushed the e2e-fix-ds-upgrade branch 6 times, most recently from b6105a8 to 789e384 Compare July 28, 2021 02:41

lzhecheng requested a review from antoninbas July 28, 2021 05:34

antoninbas reviewed Jul 28, 2021

View reviewed changes

test/e2e/flowaggregator_test.go Outdated Show resolved Hide resolved

test/e2e/networkpolicy_test.go Outdated Show resolved Hide resolved

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

pkg/agent/util/iptables/iptables.go Show resolved Hide resolved

lzhecheng force-pushed the e2e-fix-ds-upgrade branch 8 times, most recently from 9cb186a to a2b8df9 Compare July 29, 2021 08:31

antoninbas reviewed Jul 29, 2021

View reviewed changes

pkg/agent/nodeportlocal/rules/iptable_rule.go Outdated Show resolved Hide resolved

pkg/agent/nodeportlocal/rules/iptable_rule.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the e2e-fix-ds-upgrade branch 3 times, most recently from b0c0e9b to cc394bb Compare July 30, 2021 01:12

tnqn reviewed Aug 2, 2021

View reviewed changes

pkg/agent/route/route_linux.go Outdated Show resolved Hide resolved

lzhecheng dismissed antoninbas’s stale review via 376914f August 2, 2021 06:54

lzhecheng force-pushed the e2e-fix-ds-upgrade branch 3 times, most recently from e3cb226 to ef30704 Compare August 4, 2021 00:03

tnqn reviewed Aug 4, 2021

View reviewed changes

lzhecheng force-pushed the e2e-fix-ds-upgrade branch from ef30704 to cc9130c Compare August 4, 2021 12:15

antoninbas reviewed Aug 4, 2021

View reviewed changes

[IPv6] Bug fix in adding iptable rules

a4e7873

* bug fix in EnsureRule() * refactor iptables methods Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

lzhecheng force-pushed the e2e-fix-ds-upgrade branch from cc9130c to a4e7873 Compare August 4, 2021 23:38

lzhecheng requested review from antoninbas and tnqn August 5, 2021 03:51

tnqn reviewed Aug 5, 2021

View reviewed changes

lzhecheng force-pushed the e2e-fix-ds-upgrade branch 2 times, most recently from 8a9aabf to 896c590 Compare August 5, 2021 10:47

Revert ChainExists() and DeleteAllRules()

15437d1

Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>

lzhecheng force-pushed the e2e-fix-ds-upgrade branch from 896c590 to 15437d1 Compare August 5, 2021 10:51

tnqn approved these changes Aug 5, 2021

View reviewed changes

tnqn merged commit f0a44d5 into antrea-io:main Aug 5, 2021

antoninbas mentioned this pull request Aug 5, 2021

Automated cherry pick of #2469: Bug fix in adding iptable rules * bug fix in #2540

Merged

lzhecheng mentioned this pull request Aug 6, 2021

Automated cherry pick of #2469: Bug fix in adding iptable rules * bug fix in #2553

Merged

lzhecheng deleted the e2e-fix-ds-upgrade branch October 26, 2021 02:53

[IPv6] Bug fix in adding iptable rules #2469

[IPv6] Bug fix in adding iptable rules #2469

lzhecheng commented Jul 26, 2021 •

edited

lzhecheng commented Jul 26, 2021

codecov-commenter commented Jul 26, 2021 •

edited

lzhecheng commented Jul 28, 2021

lzhecheng commented Jul 28, 2021

lzhecheng commented Jul 29, 2021

antoninbas left a comment

lzhecheng commented Jul 30, 2021

antoninbas commented Jul 30, 2021

tnqn Aug 2, 2021

lzhecheng Aug 2, 2021

lzhecheng Aug 3, 2021

antoninbas Aug 3, 2021

lzhecheng Aug 4, 2021

lzhecheng commented Aug 4, 2021

lzhecheng commented Aug 4, 2021

antoninbas left a comment

antoninbas Aug 4, 2021

lzhecheng Aug 4, 2021

lzhecheng commented Aug 5, 2021 •

edited

tnqn Aug 5, 2021

lzhecheng Aug 5, 2021

lzhecheng Aug 5, 2021

tnqn Aug 5, 2021

lzhecheng Aug 5, 2021

tnqn left a comment

tnqn commented Aug 5, 2021

[IPv6] Bug fix in adding iptable rules #2469

[IPv6] Bug fix in adding iptable rules #2469

Conversation

lzhecheng commented Jul 26, 2021 • edited

lzhecheng commented Jul 26, 2021

codecov-commenter commented Jul 26, 2021 • edited

Codecov Report

lzhecheng commented Jul 28, 2021

lzhecheng commented Jul 28, 2021

lzhecheng commented Jul 29, 2021

antoninbas left a comment

Choose a reason for hiding this comment

lzhecheng commented Jul 30, 2021

antoninbas commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Aug 4, 2021

lzhecheng commented Aug 4, 2021

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Aug 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 5, 2021

lzhecheng commented Jul 26, 2021 •

edited

codecov-commenter commented Jul 26, 2021 •

edited

lzhecheng commented Aug 5, 2021 •

edited