Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor Windows SNAT flows for SNAT policy implementation #1892

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Refactor Windows SNAT flows for SNAT policy implementation #1892

merged 1 commit into from
Mar 17, 2021

Conversation

jianjuns
Copy link
Contributor

@jianjuns jianjuns commented Feb 22, 2021 •
edited
Loading

Separate common flows and Windows only flows for SNAT.
Add a new snatTable(71) between l3ForwardingTable and l3DecTTLTable(72)
for looking up SNAT IPs of the external traffic.

Issue: #667

@codecov-io
Copy link

codecov-io commented Feb 22, 2021 •
edited
Loading

Codecov Report

Merging #1892 (1702046) into main (d77f8ff) will increase coverage by 0.95%.
The diff coverage is 26.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1892      +/-   ##
==========================================
+ Coverage   64.47%   65.43%   +0.95%     
==========================================
  Files         193      195       +2     
  Lines       16893    16872      -21     
==========================================
+ Hits        10892    11040     +148     
+ Misses       4837     4666     -171     
- Partials     1164     1166       +2
Flag Coverage Δ
e2e-tests 32.98% <26.47%> (?)
kind-e2e-tests 56.05% <26.47%> (+0.68%) ⬆️
unit-tests 41.89% <2.94%> (+0.52%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/agent.go 48.92% <0.00%> (+1.19%) ⬆️
pkg/agent/agent_linux.go 100.00% <ø> (ø)
pkg/agent/openflow/client_other.go 0.00% <0.00%> (ø)
pkg/agent/openflow/pipeline.go 73.33% <14.28%> (+6.31%) ⬆️
pkg/agent/openflow/client.go 65.33% <66.66%> (+4.57%) ⬆️
pkg/agent/openflow/pipeline_other.go 66.66% <66.66%> (ø)
...ntroller/networkpolicy/networkpolicy_controller.go 71.71% <0.00%> (-0.29%) ⬇️
pkg/agent/controller/networkpolicy/reconciler.go 76.81% <0.00%> (-0.08%) ⬇️
pkg/querier/querier.go 57.14% <0.00%> (ø)
pkg/agent/types/networkpolicy.go 83.33% <0.00%> (ø)
... and 19 more

@jianjuns jianjuns force-pushed the snat-flow branch 3 times, most recently from bc0318c to 09a97d9 Compare February 22, 2021 22:42
@jianjuns
Copy link
Contributor Author

/test-windows-conformance

@jianjuns jianjuns force-pushed the snat-flow branch 2 times, most recently from 6a380b4 to 18ada71 Compare February 24, 2021 07:10
@jianjuns jianjuns marked this pull request as ready for review February 24, 2021 07:10
@jianjuns
Copy link
Contributor Author

The OVS flow design is described in #667 (comment). This PR just implements the common flows and the SNAT flows on Windows for the default SNAT IP.

tnqn
tnqn reviewed Feb 24, 2021
View reviewed changes
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated
// It is used on Windows Nodes only.
func (c *client) snatFlows(nodeIP net.IP, localSubnet net.IPNet, category cookie.Category) []binding.Flow {
snatIPRange := &binding.IPRange{StartIP: nodeIP, EndIP: nodeIP}
// snatCommonFlows installs flows to perform SNAT for traffic to the external
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it complete? It doesn't seem the flows below perform SNAT

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

snatCommonFlows is for the default common flows on Linux and Windows, which do not really perform SNAT. On Linux only SNAT policies will add flows to look up SNAT IP, mark packets, etc.. Let me revise the comments.

@jianjuns
Copy link
Contributor Author

/test-windows-conformance

@jianjuns
Copy link
Contributor Author

/test-windows-conformance

@jianjuns jianjuns force-pushed the snat-flow branch 2 times, most recently from 147623f to 769ec64 Compare February 25, 2021 05:34
@jianjuns
Copy link
Contributor Author

/test-windows-conformance

@jianjuns jianjuns force-pushed the snat-flow branch 3 times, most recently from 3c42745 to 29eb6ce Compare February 28, 2021 22:49
tnqn
tnqn reviewed Mar 9, 2021
View reviewed changes
pkg/agent/openflow/client.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
pkg/agent/openflow/pipeline.go Outdated Show resolved Hide resolved
@jianjuns jianjuns force-pushed the snat-flow branch 3 times, most recently from 5cc23ee to 00df2de Compare March 12, 2021 06:09
tnqn
tnqn reviewed Mar 16, 2021
View reviewed changes
pkg/agent/openflow/pipeline_windows.go Outdated
MatchCTMark(snatCTMark, nil).
MatchRegRange(int(marksReg), markTrafficFromUplink, binding.Range{0, 15}).
// return packets to remote Pods will be handled by L3Forwarding flows.
MatchDstIPNet(localSubnet).
Copy link
Member

@tnqn tnqn Mar 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I notice this condition is new, can a packet to remote Pods get to L3Forwarding flows? It looks like the following flow with normal priority will output the packet to bridge port?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Humm.. yes, such pakcets will be output to br-int, which we would not want to avoid asymmetric return path.

In this case, probably no harm to set macRewriteMark for all packets, though it is not needed for packets to remote Nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the flow.

tnqn
tnqn reviewed Mar 16, 2021
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, just a question about the impact of the new condition in a snat flow.

@jianjuns
Refactor Windows SNAT flows for SNAT policy implementation
1702046 
Separate common flows and Windows only flows for SNAT.
Add a new snatTable(71) between l3ForwardingTable and l3DecTTLTable(72)
for looking up SNAT IPs of the external traffic.
tnqn
tnqn approved these changes Mar 16, 2021
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jianjuns
Copy link
Contributor Author

/test-all
/test-windows-conformance
/test-windows-networkpolicy

@jianjuns
Copy link
Contributor Author

/test-e2e

1 similar comment
@jianjuns
Copy link
Contributor Author

/test-e2e

@jianjuns
Copy link
Contributor Author

/skip-all
The PR once passed all the tests. The e2e test failures are due to #1956.

@jianjuns jianjuns merged commit e984a6e into antrea-io:main Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruicao93 ruicao93 ruicao93 left review comments

@tnqn tnqn tnqn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jianjuns @codecov-io @ruicao93 @tnqn @vmwclabot