Remove Service hairpin table ServiceHairpinTable and HairpinSNATTable #2842

hongliangl · 2021-09-26T10:32:54Z

Since a SNAT ct zone is added in PR #2599, hairpin Service
traffic can make use of the SNAT ct zone instead of current
stateless SNAT by modifying source and destination IPs.
By removing hairpin table ServiceHairpinTable and
HairpinSNATTable, the OVS pipeline can be simpler.

Pipeline modifications:

Remove table serviceHairpinTable 23.
Remove table HairpinSNATTable 108.
Add table hairpinMarkTable 81 after table
l2ForwardingCalcTable 80.

When a local Endpoint is referenced by a Service, then a
flow that matches the packet whose input and output
interfaces are the same and makes mark HairpinRegMark,
will be installed on table hairpinMarkTable. Packets with
mark HairpinRegMark will be performed SNAT with Antrea
gateway IP on table snatConntrackCommitTable.

Signed-off-by: Hongliang Liu lhongliang@vmware.com

codecov-commenter · 2021-09-26T10:38:32Z

Codecov Report

Merging #2842 (e879ea7) into main (5480b68) will decrease coverage by 0.15%.
The diff coverage is 93.00%.

@@            Coverage Diff             @@
##             main    #2842      +/-   ##
==========================================
- Coverage   61.42%   61.26%   -0.16%     
==========================================
  Files         284      284              
  Lines       23470    23483      +13     
==========================================
- Hits        14417    14388      -29     
- Misses       7496     7553      +57     
+ Partials     1557     1542      -15

Flag	Coverage Δ
kind-e2e-tests	`48.90% <93.00%> (-0.09%)`	⬇️
unit-tests	`40.86% <11.00%> (-0.08%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/openflow/client.go	`57.70% <66.66%> (+0.05%)`	⬆️
pkg/agent/openflow/pipeline.go	`76.12% <98.78%> (-0.01%)`	⬇️
pkg/apiserver/certificate/certificate.go	`69.86% <0.00%> (-6.85%)`	⬇️
pkg/agent/controller/traceflow/packetin.go	`59.49% <0.00%> (-5.49%)`	⬇️
...agent/controller/traceflow/traceflow_controller.go	`69.85% <0.00%> (-3.55%)`	⬇️
pkg/controller/traceflow/controller.go	`70.30% <0.00%> (-2.43%)`	⬇️
pkg/monitor/controller.go	`27.61% <0.00%> (-1.50%)`	⬇️
pkg/ovs/openflow/ofctrl_action.go	`68.33% <0.00%> (-1.25%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`55.33% <0.00%> (-1.13%)`	⬇️
... and 5 more

wenyingd · 2021-09-27T01:16:51Z

pkg/agent/flowexporter/connections/conntrack_windows.go

@@ -34,9 +34,9 @@ type connTrackOvsCtlWindows struct {
 func (ct *connTrackOvsCtlWindows) GetMaxConnections() (int, error) {
 	var zoneID int
 	if ct.serviceCIDRv4 != nil {
-		zoneID = openflow.CtZone
+		zoneID = openflow.DNATCtZone


Windows doesn't support IPv6 now, so it is not necessary to leverage DNATCtZoneV6 on Windows.

wenyingd · 2021-09-27T01:41:33Z

pkg/agent/openflow/pipeline.go

 					Cookie(c.cookieAllocator.Request(category).Raw()).
 					Done(),
-				connectionTrackCommitTable.BuildFlow(priorityLow).MatchProtocol(proto).
-					MatchCTStateTrk(true).
+				dnatConnectionTrackCommitTable.BuildFlow(priorityHigh).MatchProtocol(proto).
 					MatchCTMark(ServiceCTMark).


It looks you want all the DNATed connections to goto snatConntrackCommitTable? Is it better to redirect only the ones has HairpinRegMark for established connections and redirect all new DNATed connections?

wenyingd · 2021-09-27T01:48:56Z

pkg/agent/openflow/pipeline.go

+				// Service CT_MARK). Since the reply packets should be output to the port on which it was received and the
+				// CT_MARK value for hairpin will be lost, HairpinRegMark is used.
+				dnatConnectionTrackTable.BuildFlow(priorityHigh).MatchProtocol(proto).
+					MatchCTStateRpl(true).


Is it better to load the regMark in snatConnectionTrackTable or maybe you can a sequencing table after snatConnectionTrackTable to check the snat conntrack track state?

This flow is for reply packets. Without passing SNAT ct zone on table snatConnectionTrackTable, we cannot get the CT_MARK which is set on SNAT ct zone.

snatConnectionTrackTable is prior to dnatConnectionTrackTable, so the reply packet is possible to enter snatConnectionTrackTable first?

wenyingd · 2021-09-27T01:57:49Z

pkg/agent/openflow/pipeline.go

+// value set in SNAT ct zone will be lost after passing DNAT ct zone, before
+// passing DNAT ct zone, HairpinRegMark is set and is used to match the reply
+// packets.
+func (c *client) l2ForwardOutputServiceHairpinFlows() []binding.Flow {


Do you think it is better to use a single flow in L2ForwardingOutTable for Hairpin traffic. I guess you add a flow to match condition "ServiceHairpinCTMark" for the first traffic for such flows. Maybe you can load the regmark in the flow action when committing the connection to snatConntrackZone

It's not only for the first packet of hairpin connections. It's also for the third, fifth .. packets (requests packets except the first one). The request packets (except the first one) cannot get the value of CT_MARK when passing through SNAT ct zone on table snatConntrackTable (but reply packets can get). When passing through SNAT ct zone on table snatConntrackCommitTable, the value of CT_MARK can be got. However, the next table L2ForwardingOutTable. I think we'd better add a flow rather than a new table.

Actually I don't prefer to use ct mark in L2ForwardingOutTable, as I thought this table is possible to focus on L2 operations... Since the target ofport number is decided after L2FwdCalcTable, is it possible to set the regMark after that table?

antoninbas · 2021-09-27T21:53:35Z

Could you provide information about this change in the commit message & PR description? I have no context for this change and there is no linked issue.

hongliangl · 2021-09-28T10:49:17Z

Could you provide information about this change in the commit message & PR description? I have no context for this change and there is no linked issue.

Updated.

jianjuns

I think Antonin means to explain why we make the changes.

In the commit message:

flow that matches the packet whose input and output
interfaces are the same and makes mark HairpinRegMark,
will be installed on table hairpinMarkTable.
a flow will be installed into hairpinMarkTable, which sets HairpinRegMark for the packets whose input and output ports are the same.

mark HairpinRegMark will be performed SNAT with Antrea
Packets with HairpinRegMark set will be SNAT'd

gateway IP on table snatConntrackCommitTable.
in snatConntrackCommitTable

hongliangl · 2021-09-29T01:07:56Z

I think Antonin means to explain why we make the changes.

In the commit message:

flow that matches the packet whose input and output
interfaces are the same and makes mark HairpinRegMark,
will be installed on table hairpinMarkTable.
a flow will be installed into hairpinMarkTable, which sets HairpinRegMark for the packets whose input and output ports are the same.

mark HairpinRegMark will be performed SNAT with Antrea
Packets with HairpinRegMark set will be SNAT'd

gateway IP on table snatConntrackCommitTable.
in snatConntrackCommitTable

@jianjuns, Thanks. Since a SNAT ct zone is added in PR #2599, hairpin Service traffic can make use of the SNAT ct zone instead of current stateless SNAT by modifying source and destination IPs. By removing hairpin table ServiceHairpinTable and HairpinSNATTable, the OVS pipeline can be simpler.

Since a SNAT ct zone is added in PR antrea-io#2599, hairpin Service traffic can make use of the SNAT ct zone instead of current stateless SNAT by modifying source and destination IPs. By removing hairpin table ServiceHairpinTable and HairpinSNATTable, the OVS pipeline can be simpler. Pipeline modifications: - Remove table serviceHairpinTable #23. - Remove table HairpinSNATTable #108. - Add table hairpinMarkTable #81 after table l2ForwardingCalcTable #80. When a local Endpoint is referenced by a Service, then a flow that matches the packet whose input and output interfaces are the same and makes mark HairpinRegMark, will be installed on table hairpinMarkTable. Packets with mark HairpinRegMark will be performed SNAT with Antrea gateway IP on table snatConntrackCommitTable. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

github-actions · 2021-12-30T00:24:04Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

hongliangl requested review from wenyingd, tnqn, jianjuns and antoninbas September 26, 2021 10:33

hongliangl force-pushed the remove-hairpin branch from 1c9c3a9 to d14680a Compare September 26, 2021 15:22

wenyingd reviewed Sep 27, 2021

View reviewed changes

hongliangl force-pushed the remove-hairpin branch from d14680a to 4d7338f Compare September 27, 2021 10:34

antoninbas added area/ovs/openflow Issues or PRs related to Open vSwitch Open Flow. area/proxy Issues or PRs related to proxy functions in Antrea review-manager-test labels Sep 27, 2021

github-actions bot requested review from GraysonWu and wenyingd September 27, 2021 19:03

hongliangl force-pushed the remove-hairpin branch 3 times, most recently from 60fca2b to 6ee76a7 Compare September 28, 2021 10:48

hongliangl force-pushed the remove-hairpin branch from 6ee76a7 to ef0dfff Compare September 28, 2021 15:16

jianjuns reviewed Sep 28, 2021

View reviewed changes

hongliangl force-pushed the remove-hairpin branch from ef0dfff to c458f01 Compare September 29, 2021 01:05

github-actions bot requested a review from jianjuns September 29, 2021 01:05

hongliangl force-pushed the remove-hairpin branch 3 times, most recently from 2d515eb to 0d032f8 Compare September 29, 2021 10:57

hongliangl force-pushed the remove-hairpin branch from 0d032f8 to e879ea7 Compare September 29, 2021 12:03

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 30, 2021

hongliangl closed this Jan 11, 2022

hongliangl deleted the remove-hairpin branch April 21, 2022 01:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove Service hairpin table ServiceHairpinTable and HairpinSNATTable #2842

Remove Service hairpin table ServiceHairpinTable and HairpinSNATTable #2842

hongliangl commented Sep 26, 2021 •

edited

Loading

codecov-commenter commented Sep 26, 2021 •

edited

Loading

wenyingd Sep 27, 2021

wenyingd Sep 27, 2021

wenyingd Sep 27, 2021

hongliangl Sep 27, 2021

wenyingd Sep 27, 2021

wenyingd Sep 27, 2021

hongliangl Sep 27, 2021

wenyingd Sep 27, 2021

antoninbas commented Sep 27, 2021

hongliangl commented Sep 28, 2021

jianjuns left a comment •

edited

Loading

hongliangl commented Sep 29, 2021

github-actions bot commented Dec 30, 2021

Remove Service hairpin table ServiceHairpinTable and HairpinSNATTable #2842

Remove Service hairpin table ServiceHairpinTable and HairpinSNATTable #2842

Conversation

hongliangl commented Sep 26, 2021 • edited Loading

codecov-commenter commented Sep 26, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented Sep 27, 2021

hongliangl commented Sep 28, 2021

jianjuns left a comment • edited Loading

Choose a reason for hiding this comment

hongliangl commented Sep 29, 2021

github-actions bot commented Dec 30, 2021

hongliangl commented Sep 26, 2021 •

edited

Loading

codecov-commenter commented Sep 26, 2021 •

edited

Loading

jianjuns left a comment •

edited

Loading