Fix reject action on Service traffic #2772

GraysonWu · 2021-09-13T23:23:02Z

Fixes issue #2699

1. Change reject response packetOut logic to below:

Pod-to-Pod traffic
- Locally: Directly output the packet to the destination OF port. Use src/dstPod's OF port as in/outPort.
- Remote to local: Directly output the packet to the destination OF port. Use gateway's OF port as inPort and dstPod's OF port as outPort.
- Local to remote: Resubmit packet to l3ForwardingTable(70). Use Pod's OF port as inPort.
Service traffic
- AntreaProxy enabled
  - Locally: Resubmit packet to conntrackTable(30). Use srcPod's OF port as inPort.
  - Remote to local: Use gateway's OF port as inPort.
  - Local to remote: Same as Pod-to-Pod traffic local to remote case.
- AntreaProxy disabled
  - Locally: Directly output the packet to gateway's OF port. Use srcPod's OF port as inPort and gateway's OF port as outPort.
  - Remote to local: Directly output the packet to gateway's OF port. Use tunnel's OF port as inPort and gateway's OF port as outPort.
  - Local to remote: Same as Pod-to-Pod traffic local to remote case.

2. To decided if it is Service traffic:

AntreaProxy enabled:
If EpSelectedRegMark is set in ServiceEPStateField, then it's Service traffic
AntreaProxy disabled:
For the destination of the reject response, if dstIP is on local Node, but the dstMAC is the gateway's MAC. We could say this response is heading to kube-proxy, which means it's Service traffic.

3. Removed reject bypass CT logic.

4. Added e2e to test reject action on Service traffic.

Reference Table of all cases when Antrea Proxy is disabled

Node	Policy	Traffic	Reject Packet	Type
Inter-Node	Ingress	Pod-to-Pod	srcMAC: srcPod MAC dstMAC: antrea-gw MAC srcIP: srcPodIP dstIP: dstPodIP(remote)	RejectLocalToRemote
Inter-Node	Ingress	Service	srcMAC: srcPod MAC dstMAC: antrea-gw MAC srcIP: srcPodIP dstIP: dstPodIP(remote)	RejectLocalToRemote
Inter-Node	Egress	Pod-to-Pod	srcMAC: antrea-gw MAC dstMAC: dstPod MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectPodRemoteToLocal
Inter-Node	Egress	Service	srcMAC: virtule MAC dstMAC: antrea-gw MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectNoAPServiceRemoteToLocal
Intra-Node	Ingress	Pod-to-Pod	srcMAC: srcPod MAC dstMAC: dstPod MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectPodLocal
Intra-Node	Ingress	Service	srcMAC: srcPod MAC dstMAC: antrea-gw MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectNoAPServiceLocal
Intra-Node	Egress	Pod-to-Pod	srcMAC: srcPod MAC dstMAC: dstPod MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectPodLocal
Intra-Node	Egress	Service	srcMAC: srcPod MAC dstMAC: antrea-gw MAC srcIP: srcPodIP dstIP: dstPodIP(local)	RejectNoAPServiceLocal

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

codecov-commenter · 2021-09-13T23:28:26Z

Codecov Report

Merging #2772 (be4ec20) into main (12f8395) will decrease coverage by 1.14%.
The diff coverage is 70.07%.

@@            Coverage Diff             @@
##             main    #2772      +/-   ##
==========================================
- Coverage   61.61%   60.46%   -1.15%     
==========================================
  Files         283      284       +1     
  Lines       23810    23878      +68     
==========================================
- Hits        14671    14439     -232     
- Misses       7558     7913     +355     
+ Partials     1581     1526      -55

Flag	Coverage Δ
kind-e2e-tests	`47.70% <68.61%> (-1.55%)`	⬇️
unit-tests	`40.43% <2.18%> (-0.19%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/controller/networkpolicy/packetin.go	`67.15% <ø> (-4.85%)`	⬇️
pkg/agent/openflow/pipeline.go	`77.83% <ø> (-0.58%)`	⬇️
pkg/agent/openflow/client.go	`58.44% <37.50%> (+0.11%)`	⬆️
pkg/agent/controller/networkpolicy/reject.go	`70.24% <70.24%> (ø)`
pkg/agent/controller/networkpolicy/fqdn.go	`75.97% <100.00%> (+0.12%)`	⬆️
...ntroller/networkpolicy/networkpolicy_controller.go	`71.03% <100.00%> (-0.22%)`	⬇️
pkg/ovs/openflow/ofctrl_packetout.go	`85.65% <100.00%> (+1.03%)`	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`19.54% <0.00%> (-55.18%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-51.73%)`	⬇️
... and 34 more

pkg/ovs/openflow/ofctrl_packetout.go

pkg/ovs/openflow/interfaces.go

pkg/agent/controller/networkpolicy/packetin.go

pkg/agent/openflow/client.go

wenyingd

LGTM, only two minor nits.

pkg/agent/controller/networkpolicy/packetin.go

Fixes issue antrea-io#2699. 1. Change reject response packetOut logic to below: * Pod-to-Pod traffic * Locally: Directly output the packet to the destination OF port. Use src/dstPod's OF port as in/outPort. * Remote to local: Directly output the packet to the destination OF port. Use gateway's OF port as inPort and dstPod's OF port as outPort. * Local to remote: Resubmit packet to l3ForwardingTable(70). Use Pod's OF port as inPort. * Service traffic * AntreaProxy enabled * Locally: Resubmit packet to conntrackTable(30). Use srcPod's OF port as inPort. * Remote to local: Use gateway's OF port as inPort. * Local to remote: Same as Pod-to-Pod traffic local to remote case. * AntreaProxy disabled * Locally: Directly output the packet to gateway's OF port. Use srcPod's OF port as inPort and gateway's OF port as outPort. * Remote to local: Directly output the packet to gateway's OF port. Use tunnel's OF port as inPort and gateway's OF port as outPort. * Local to remote: Same as Pod-to-Pod traffic local to remote case. 2. To decided if it is Service traffic: * AntreaProxy enabled: If EpSelectedRegMark is set in ServiceEPStateField, then it's Service traffic * AntreaProxy disabled: For the destination of the reject response, if dstIP is on local Node, but the dstMAC is the gateway's MAC. We could say this response is heading to kube-proxy, which means it's Service traffic. 3. Removed reject bypass CT logic. 4. Added e2e to test reject action on Service traffic. 5. Change packetOut outPort to uint32. Signed-off-by: wgrayson <wgrayson@vmware.com>

GraysonWu · 2021-10-27T06:11:34Z

/test-integration

GraysonWu · 2021-10-28T18:29:08Z

/test-all

wenyingd

LGTM overall, except for one comments, and no strong opinion to address it in this PR.

wenyingd · 2021-11-01T06:40:15Z

pkg/agent/openflow/client.go

-	if isReject {
-		packetOutBuilder = packetOutBuilder.AddLoadRegMark(CustomReasonRejectRegMark)
-	}
+	packetOutBuilder = addActionsToPacketOut(packetOutBuilder, packetOutType)


Is it possible to create a new function dedicated for "reject service traffic"? And you can call addActionsToPacketOut once in this new function, then append the "AddResubmitAction" in any flowbuilder which needs it. In this way, you don't need to call addActionsToPacketOut twice, and you don't need to inject this specific function in the general function "SendICMPPacketOut" and "SendTCPPacketOut" which don't require this specific action in other cases.

I do agree with Wenying that the current code is quite clunky. However, I don't think that these functions (SendTCPPacketOut / SendICMPPacketOut) should be responsible for doing anything which is specific to reject packets. I do not think that having binding.PacketOutType (which is specific to reject packets) as a parameter is a good idea.

Instead I think the last argument to these functions should be an arbitrary "PacketOutBuilder mutator":

func (c *client) SendICMPPacketOut( srcMAC string, dstMAC string, srcIP string, dstIP string, inPort uint32, outPort uint32, isIPv6 bool, icmpType uint8, icmpCode uint8, icmpData []byte, mutatePacket func(builder PacketOutBuilder) PacketOutBuilder) error {

Thanks for the review. An arbitrary mutator func sounds good to me. I understand what you mean and at the same time, I realized that I didn't express my thoughts clearly. Sorry for the confusion. I'd like to explain a little bit. Then you guys can see if it still doesn't make sense.
I don't want to make SendTCPPacketOut or SendICMPPacketOut specific to reject packet. I'd like to make PacketOutType as a generic type. SendTCPPacketOut or SendICMPPacketOut could add OF Actions to packetOutBuilder based on PacketOutType.
Currently, I only included cases for reject, so I name the type with a generic name PacketOutType and all current types have the prefix Reject. If needed, we could add more packetOutTypes in the future. And at the same time, add OF Actions that new packetOutType need in func addActionsToPacketOut.
Maybe add a normal type PacketOutType which won't do anything in func addActionsToPacketOut could help others understand?
Let me know if this makes sense to you guys. Thanks!

I think you have too many cases specific to the reject case (7) to make it a good candidate for a generic packet type concept. It would be a bad idea to have such a detailed list of cases that just keeps on growing. It's better to have a specialized "reject packet type" concept, and keep it local to the code dealing with reject packets.

Then you can have a SendRejectICMP function which is going to call c.SendICMPPacketOut with the right parameters (ICMP type / code, packet mutator).

pkg/agent/controller/networkpolicy/packetin.go

pkg/ovs/openflow/ofctrl_packetout.go

antoninbas · 2021-11-01T21:22:24Z

pkg/agent/openflow/client.go

-	if isReject {
-		packetOutBuilder = packetOutBuilder.AddLoadRegMark(CustomReasonRejectRegMark)
-	}
+	packetOutBuilder = addActionsToPacketOut(packetOutBuilder, packetOutType)


I do agree with Wenying that the current code is quite clunky. However, I don't think that these functions (SendTCPPacketOut / SendICMPPacketOut) should be responsible for doing anything which is specific to reject packets. I do not think that having binding.PacketOutType (which is specific to reject packets) as a parameter is a good idea.

Instead I think the last argument to these functions should be an arbitrary "PacketOutBuilder mutator":

func (c *client) SendICMPPacketOut( srcMAC string, dstMAC string, srcIP string, dstIP string, inPort uint32, outPort uint32, isIPv6 bool, icmpType uint8, icmpCode uint8, icmpData []byte, mutatePacket func(builder PacketOutBuilder) PacketOutBuilder) error {

wenyingd

LGTM

pkg/agent/controller/networkpolicy/reject.go

antoninbas · 2021-11-02T20:24:15Z

pkg/agent/controller/networkpolicy/reject.go

+	// 1. When AntreaProxy is enabled, EpSelectedRegMark is set in ServiceEPStateField.
+	// 2. When AntreaProxy is disabled, dstIP of reject response is on the local Node
+	//    and dstMAC of reject response is antrea-gw's MAC.
+	isServiceTraffic := func() bool {


my understanding is that isServiceTraffic only return true if we are at the source Node (reject action in an ingress policy). Is that correct? For Service traffic captured at the destination Node (reject action in a egress policy), how is the traffic classified?

Basically I don't understand how we can classify traffic a RejectNoAPServiceRemoteToLocal with this logic.

I think I understand a bit better after reading the PR description again. But it still seems to me that in some cases (for egress policies), isServiceTraffic will return false for Service traffic, so it may be worth emphasizing this here with a comment, and explaining again that for such traffic the logic is the same as for Pod-to-Pod traffic (unless I am mistaken).

Actually, it will only return true when the destination of the reject packet is on local Node. No matter in ingress/egress policy.
I made a table for AntreaProxy is disabled hope it could help you understand or discover what I missed:

Node Policy Traffic Reject Packet Type

Inter-Node Ingress Pod-to-Pod srcMAC: srcPod MAC
dstMAC: antrea-gw MAC
srcIP: srcPodIP
dstIP: dstPodIP(remote) RejectLocalToRemote

Inter-Node Ingress Service srcMAC: srcPod MAC
dstMAC: antrea-gw MAC
srcIP: srcPodIP
dstIP: dstPodIP(remote) RejectLocalToRemote

Inter-Node Egress Pod-to-Pod srcMAC: antrea-gw MAC
dstMAC: dstPod MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectPodRemoteToLocal

Inter-Node Egress Service srcMAC: virtule MAC
dstMAC: antrea-gw MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectNoAPServiceRemoteToLocal

Intra-Node Ingress Pod-to-Pod srcMAC: srcPod MAC
dstMAC: dstPod MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectPodLocal

Intra-Node Ingress Service srcMAC: srcPod MAC
dstMAC: antrea-gw MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectNoAPServiceLocal

Intra-Node Egress Pod-to-Pod srcMAC: srcPod MAC
dstMAC: dstPod MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectPodLocal

Intra-Node Egress Service srcMAC: srcPod MAC
dstMAC: antrea-gw MAC
srcIP: srcPodIP
dstIP: dstPodIP(local) RejectNoAPServiceLocal

Sorry didn't see that you have replied before I post the table above.

I think I understand a bit better after reading the PR description again. But it still seems to me that in some cases (for egress policies), isServiceTraffic will return false for Service traffic, so it may be worth emphasizing this here with a comment, and explaining again that for such traffic the logic is the same as for Pod-to-Pod traffic (unless I am mistaken).

Do you mean inter-Node ingress cases? Pod-to-Pod traffic and Service traffic have the same logic?

I added more comments here to explain this. @antoninbas you can take a look to see if it's clear. Thanks.

In my original comment, I switched "ingress" and "egress" so it must have been confusing :/

Actually, it will only return true when the destination of the reject packet is on local Node. No matter in ingress/egress policy.

I see that, but it is always the case for egress policies, and often not the case for ingress policies (inter-Node). Thanks for the table. I think you should copy the source code for the table to the PR description for future reference.

Good idea. Added to the PR description.

test/e2e/antreapolicy_test.go

test/e2e/k8s_util.go

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

GraysonWu · 2021-11-03T00:41:04Z

/test-all

This was introduced by merging antrea-io#2956 and antrea-io#2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

This was introduced by merging #2956 and #2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

This was introduced by merging antrea-io#2956 and antrea-io#2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

This was introduced by merging #2956 and #2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

GraysonWu force-pushed the fix-svc-reject branch 6 times, most recently from ed900d4 to 76fdde1 Compare September 25, 2021 06:57

GraysonWu changed the title ~~[WIP]Fix reject action on Service traffic~~ Fix reject action on Service traffic Sep 25, 2021

GraysonWu force-pushed the fix-svc-reject branch from 76fdde1 to a94cee2 Compare September 25, 2021 08:13

GraysonWu requested review from wenyingd and antoninbas September 25, 2021 20:31

wenyingd reviewed Sep 27, 2021

View reviewed changes

pkg/ovs/openflow/ofctrl_packetout.go Outdated Show resolved Hide resolved

pkg/ovs/openflow/interfaces.go Show resolved Hide resolved

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

GraysonWu force-pushed the fix-svc-reject branch from a94cee2 to f36786a Compare September 27, 2021 20:13

GraysonWu requested a review from wenyingd September 27, 2021 20:39

wenyingd reviewed Sep 29, 2021

View reviewed changes

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

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

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

GraysonWu force-pushed the fix-svc-reject branch 2 times, most recently from d1225e6 to 80be246 Compare September 29, 2021 20:16

GraysonWu requested a review from wenyingd September 29, 2021 21:14

wenyingd reviewed Oct 20, 2021

View reviewed changes

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

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

GraysonWu force-pushed the fix-svc-reject branch 3 times, most recently from 4a2b817 to ffaee3b Compare October 21, 2021 20:14

GraysonWu force-pushed the fix-svc-reject branch from ffaee3b to 1e918a1 Compare October 22, 2021 21:38

GraysonWu requested a review from wenyingd October 28, 2021 18:30

wenyingd reviewed Nov 1, 2021

View reviewed changes

antoninbas reviewed Nov 1, 2021

View reviewed changes

GraysonWu requested a review from antoninbas November 1, 2021 23:17

GraysonWu requested a review from wenyingd November 1, 2021 23:17

GraysonWu force-pushed the fix-svc-reject branch from 6dc9638 to 4f31d06 Compare November 2, 2021 00:59

wenyingd previously approved these changes Nov 2, 2021

View reviewed changes

antoninbas reviewed Nov 2, 2021

View reviewed changes

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

antoninbas reviewed Nov 2, 2021

View reviewed changes

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

GraysonWu dismissed wenyingd’s stale review via 3947798 November 2, 2021 22:07

GraysonWu force-pushed the fix-svc-reject branch from 4f31d06 to 3947798 Compare November 2, 2021 22:07

Address comments

be4ec20

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

GraysonWu force-pushed the fix-svc-reject branch from 3947798 to be4ec20 Compare November 2, 2021 22:36

GraysonWu requested a review from antoninbas November 2, 2021 22:38

antoninbas approved these changes Nov 3, 2021

View reviewed changes

tnqn merged commit 8ca720d into antrea-io:main Nov 3, 2021

tnqn added a commit to tnqn/antrea that referenced this pull request Nov 3, 2021

Fix missing parameter in tests

d0ff0e0

This was introduced by merging antrea-io#2956 and antrea-io#2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn mentioned this pull request Nov 3, 2021

Fix missing parameter in tests #2968

Merged

tnqn added a commit that referenced this pull request Nov 3, 2021

Fix missing parameter in tests (#2968)

a48f4db

This was introduced by merging #2956 and #2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn added a commit to tnqn/antrea that referenced this pull request Nov 3, 2021

Fix missing parameter in tests

774b2b3

This was introduced by merging antrea-io#2956 and antrea-io#2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

tnqn added a commit that referenced this pull request Nov 3, 2021

Fix missing parameter in tests (#2969)

58eacd9

This was introduced by merging #2956 and #2772 whose code conflict was not detected by git. Signed-off-by: Quan Tian <qtian@vmware.com>

GraysonWu mentioned this pull request Mar 31, 2022

antrea 1.6 agent crashing [signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0x1bb2f45] │ #3559

Closed

Fix reject action on Service traffic #2772

Fix reject action on Service traffic #2772

GraysonWu commented Sep 13, 2021 •

edited

codecov-commenter commented Sep 13, 2021 •

edited

wenyingd left a comment

GraysonWu commented Oct 27, 2021

GraysonWu commented Oct 28, 2021

wenyingd left a comment

wenyingd Nov 1, 2021

antoninbas Nov 1, 2021

GraysonWu Nov 1, 2021

antoninbas Nov 1, 2021

antoninbas Nov 1, 2021

wenyingd left a comment

antoninbas Nov 2, 2021

antoninbas Nov 2, 2021

GraysonWu Nov 2, 2021 •

edited

GraysonWu Nov 2, 2021 •

edited

GraysonWu Nov 2, 2021

antoninbas Nov 2, 2021

GraysonWu Nov 3, 2021

GraysonWu commented Nov 3, 2021

Fix reject action on Service traffic #2772

Fix reject action on Service traffic #2772

Conversation

GraysonWu commented Sep 13, 2021 • edited

codecov-commenter commented Sep 13, 2021 • edited

Codecov Report

wenyingd left a comment

Choose a reason for hiding this comment

GraysonWu commented Oct 27, 2021

GraysonWu commented Oct 28, 2021

wenyingd left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GraysonWu Nov 2, 2021 • edited

Choose a reason for hiding this comment

GraysonWu Nov 2, 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

GraysonWu commented Nov 3, 2021

GraysonWu commented Sep 13, 2021 •

edited

codecov-commenter commented Sep 13, 2021 •

edited

GraysonWu Nov 2, 2021 •

edited

GraysonWu Nov 2, 2021 •

edited