Fix reject loop issue and add error handling #3569

GraysonWu · 2022-03-31T21:03:00Z

Fixes #3559
Skip the reject response generation when neither src nor dst are on
current Node.

Re-write the MAC address for RejectPodLocal reject type no matter
AntreaIPAM is on or not. And send the packetOut directly to the
dstPod instead of L3Forwarding table.

Signed-off-by: wgrayson wgrayson@vmware.com

codecov-commenter · 2022-03-31T21:19:18Z

Codecov Report

Merging #3569 (a5de58d) into main (ac50657) will decrease coverage by 18.63%.
The diff coverage is 34.83%.

❗ Current head a5de58d differs from pull request most recent head 29be1e2. Consider uploading reports for the commit 29be1e2 to get more accurate results

@@             Coverage Diff             @@
##             main    #3569       +/-   ##
===========================================
- Coverage   64.15%   45.52%   -18.64%     
===========================================
  Files         278      362       +84     
  Lines       27833    50754    +22921     
===========================================
+ Hits        17857    23105     +5248     
- Misses       8061    25439    +17378     
- Partials     1915     2210      +295

Flag	Coverage Δ
e2e-tests	`48.68% <34.83%> (?)`
integration-tests	`38.16% <ø> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

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

Impacted Files	Coverage Δ
pkg/agent/agent_linux.go	`5.55% <ø> (+1.23%)`	⬆️
pkg/agent/config/node_config.go	`88.88% <ø> (-11.12%)`	⬇️
pkg/agent/openflow/multicast.go	`0.00% <0.00%> (ø)`
pkg/controller/ipam/validate.go	`0.00% <0.00%> (-82.26%)`	⬇️
pkg/agent/openflow/pipeline.go	`68.88% <7.14%> (-5.29%)`	⬇️
pkg/features/antrea_features.go	`11.11% <10.00%> (-5.56%)`	⬇️
pkg/agent/proxy/proxier.go	`49.07% <34.78%> (-13.65%)`	⬇️
pkg/agent/openflow/service.go	`69.76% <42.30%> (-13.91%)`	⬇️
pkg/agent/openflow/egress.go	`69.44% <50.00%> (-14.77%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`76.47% <55.00%> (-11.44%)`	⬇️
... and 395 more

pkg/agent/controller/networkpolicy/reject.go

antoninbas

if there was a regression, we should add a test so that it doesn't happen again

GraysonWu · 2022-04-01T00:04:13Z

Do you have any good ideas about how to detect the loop in the test?

antoninbas · 2022-04-01T00:16:20Z

Do you have any good ideas about how to detect the loop in the test?

Isn't the reject loop causing the Reject functionality to break in some way?

If not, you could think about unit testing this function to make sure it generates the correct Reject packets for a few interesting input PacketIns. Based on the contents of that patch, it seems the loop is caused by a bug in the function and is not related to the OVS flows.

GraysonWu · 2022-04-01T01:20:03Z

Do you have any good ideas about how to detect the loop in the test?

Isn't the reject loop causing the Reject functionality to break in some way?

If not, you could think about unit testing this function to make sure it generates the correct Reject packets for a few interesting input PacketIns. Based on the contents of that patch, it seems the loop is caused by a bug in the function and is not related to the OVS flows.

My bad. I'm stupid. No matter if the loop will crash the agent or not. The test client won't receive the reject response in a loop situation. Then the test will fail.

GraysonWu · 2022-04-01T02:40:46Z

Added tests.

Fixes antrea-io#3559 Skip the reject response generation when neither src nor dst are on current Node. Re-write the MAC address for `RejectPodLocal` reject type no matter AntreaIPAM is on or not. And send the packetOut directly to the dstPod instead of L3Forwarding table. Signed-off-by: wgrayson <wgrayson@vmware.com>

tnqn

LGTM, one question

test/e2e/antreapolicy_test.go

pkg/agent/controller/networkpolicy/reject.go

test/e2e/antreapolicy_test.go

Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu · 2022-04-04T20:39:39Z

/test-all

GraysonWu · 2022-04-04T21:20:51Z

/test-integration

antoninbas

LGTM

pkg/agent/controller/networkpolicy/reject.go

antoninbas · 2022-04-05T18:45:15Z

/test-e2e

antoninbas · 2022-04-05T22:52:33Z

@GraysonWu looks like jenkins-e2e failed twice, maybe you need to take a look

GraysonWu · 2022-04-06T20:58:35Z

@GraysonWu looks like jenkins-e2e failed twice, maybe you need to take a look

Those tests failed in apply antrea phase. I think they shouldn't be introduced by this PR. And I'm unable to reproduce it in my env. Opened PR #3590 to just run those failed tests in jenkins. Will update here after test complete.

GraysonWu · 2022-04-06T22:52:34Z

Tests passed when running alone. One more retry here.
/test-e2e

GraysonWu · 2022-04-07T04:48:58Z

/test-e2e

antoninbas · 2022-04-07T23:08:26Z

@GraysonWu could you backport this to 1.6?

GraysonWu · 2022-04-08T01:00:57Z

Backported to 1.6 and 1.5

…andling (#3601) * Fix reject loop issue and add error handling Fixes #3559 Skip the reject response generation when neither src nor dst are on current Node. Re-write the MAC address for `RejectPodLocal` reject type no matter AntreaIPAM is on or not. And send the packetOut directly to the dstPod instead of L3Forwarding table. Signed-off-by: wgrayson <wgrayson@vmware.com> * Address comments Signed-off-by: wgrayson <wgrayson@vmware.com> * Change test function parameters matching release-1.5 format Signed-off-by: wgrayson <wgrayson@vmware.com>

…andling (#3600) Fixes #3559 Skip the reject response generation when neither src nor dst are on current Node. Re-write the MAC address for `RejectPodLocal` reject type no matter AntreaIPAM is on or not. And send the packetOut directly to the dstPod instead of L3Forwarding table. Signed-off-by: wgrayson <wgrayson@vmware.com>

Dyanngg reviewed Mar 31, 2022

View reviewed changes

pkg/agent/controller/networkpolicy/reject.go Outdated Show resolved Hide resolved

GraysonWu requested review from antoninbas, Dyanngg, tnqn and wenyingd March 31, 2022 22:41

antoninbas reviewed Mar 31, 2022

View reviewed changes

GraysonWu force-pushed the fix-reject-1.6 branch from 08f86ef to 7de9380 Compare April 1, 2022 02:39

GraysonWu force-pushed the fix-reject-1.6 branch from 7de9380 to 522199e Compare April 1, 2022 02:43

tnqn previously approved these changes Apr 1, 2022

View reviewed changes

test/e2e/antreapolicy_test.go Show resolved Hide resolved

antoninbas reviewed Apr 1, 2022

View reviewed changes

Address comments

29be1e2

Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu dismissed tnqn’s stale review via 29be1e2 April 4, 2022 05:32

GraysonWu requested review from antoninbas and tnqn April 4, 2022 21:21

antoninbas approved these changes Apr 5, 2022

View reviewed changes

pkg/agent/controller/networkpolicy/reject.go Outdated Show resolved Hide resolved

tnqn approved these changes Apr 7, 2022

View reviewed changes

tnqn added the kind/bug Categorizes issue or PR as related to a bug. label Apr 7, 2022

tnqn added action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Apr 7, 2022

tnqn merged commit 5e23ce9 into antrea-io:main Apr 7, 2022

antoninbas deleted the fix-reject-1.6 branch April 7, 2022 23:08

This was referenced Apr 8, 2022

Automated cherry pick of #3569: Fix reject loop issue and add error handling #3600

Merged

Automated cherry pick of #3569: Fix reject loop issue and add error handling #3601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reject loop issue and add error handling #3569

Fix reject loop issue and add error handling #3569

GraysonWu commented Mar 31, 2022

codecov-commenter commented Mar 31, 2022 •

edited

Loading

antoninbas left a comment

GraysonWu commented Apr 1, 2022

antoninbas commented Apr 1, 2022

GraysonWu commented Apr 1, 2022

GraysonWu commented Apr 1, 2022

tnqn left a comment

GraysonWu commented Apr 4, 2022

GraysonWu commented Apr 4, 2022

antoninbas left a comment

antoninbas commented Apr 5, 2022

antoninbas commented Apr 5, 2022

GraysonWu commented Apr 6, 2022 •

edited

Loading

GraysonWu commented Apr 6, 2022

GraysonWu commented Apr 7, 2022

antoninbas commented Apr 7, 2022

GraysonWu commented Apr 8, 2022

Fix reject loop issue and add error handling #3569

Fix reject loop issue and add error handling #3569

Conversation

GraysonWu commented Mar 31, 2022

codecov-commenter commented Mar 31, 2022 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

GraysonWu commented Apr 1, 2022

antoninbas commented Apr 1, 2022

GraysonWu commented Apr 1, 2022

GraysonWu commented Apr 1, 2022

tnqn left a comment

Choose a reason for hiding this comment

GraysonWu commented Apr 4, 2022

GraysonWu commented Apr 4, 2022

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Apr 5, 2022

antoninbas commented Apr 5, 2022

GraysonWu commented Apr 6, 2022 • edited Loading

GraysonWu commented Apr 6, 2022

GraysonWu commented Apr 7, 2022

antoninbas commented Apr 7, 2022

GraysonWu commented Apr 8, 2022

codecov-commenter commented Mar 31, 2022 •

edited

Loading

GraysonWu commented Apr 6, 2022 •

edited

Loading