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.

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

Add OVS flows for implementing Egress #1969

Merged
merged 1 commit into from
Mar 26, 2021
Merged

Conversation

jianjuns
Copy link
Contributor

Add flows that set pkt_mark for egress traffic that should be SNAT'd
using a SNAT IP, including egress traffic from a local Pod to which the
Egress is applied, and traffic from a remote Node that is tunnelled to
the egress Node with the SNAT IP.
Each SNAT IP on the Node will be allocated with a unique integer ID,
which is set to the pkt_mark, and so the SNAT implementation can look
up the right SNAT IP from the pkt_mark. On Linux, SNAT will be
implemented by iptables SNAT rules; on Windows, SNAT is implemented
by OVS NAT.

#1924

@jianjuns jianjuns force-pushed the egress-flow branch 2 times, most recently from e9716c6 to 495dba8 Compare March 18, 2021 04:46
@tnqn tnqn mentioned this pull request Mar 18, 2021
5 tasks
@codecov-io
Copy link

codecov-io commented Mar 18, 2021

Codecov Report

Merging #1969 (4fa781e) into main (fcd5070) will increase coverage by 0.15%.
The diff coverage is 19.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1969      +/-   ##
==========================================
+ Coverage   65.05%   65.21%   +0.15%     
==========================================
  Files         195      197       +2     
  Lines       16882    17275     +393     
==========================================
+ Hits        10983    11266     +283     
- Misses       4729     4836     +107     
- Partials     1170     1173       +3     
Flag Coverage Δ
kind-e2e-tests 56.34% <19.29%> (+0.40%) ⬆️
unit-tests 41.70% <5.26%> (-0.15%) ⬇️

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

Impacted Files Coverage Δ
pkg/agent/openflow/pipeline_other.go 50.00% <0.00%> (-16.67%) ⬇️
pkg/ovs/openflow/ofctrl_action.go 47.63% <0.00%> (-0.21%) ⬇️
pkg/ovs/openflow/ofctrl_builder.go 56.57% <0.00%> (-1.14%) ⬇️
pkg/agent/openflow/pipeline.go 70.64% <9.09%> (-2.13%) ⬇️
pkg/agent/openflow/client.go 64.16% <50.00%> (-1.18%) ⬇️
pkg/apis/controlplane/sets.go 33.33% <0.00%> (-12.83%) ⬇️
pkg/ovs/openflow/ofctrl_packetout.go 88.78% <0.00%> (-3.14%) ⬇️
pkg/controller/networkpolicy/crd_utils.go 88.60% <0.00%> (-1.61%) ⬇️
pkg/controller/networkpolicy/external_entity.go
... and 16 more

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.

Code logic LGTM. CI detects some syntax errors.

func (c *client) InstallPodSNATFlows(ofPort uint32, snatMark uint32, isIPv6 bool) error {
c.replayMutex.RLock()
defer c.replayMutex.RUnlock()
flows := make([]binding.Flow, 0, 2)
Copy link
Member

Choose a reason for hiding this comment

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

why make it 2? I see only one flow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it.
Originally I thought we might need to add a flow for both v4 and v6 in the dual stack case.

@@ -39,13 +38,13 @@ func (c *client) InstallLoadBalancerServiceFromOutsideFlows(svcIP net.IP, svcPor
defer c.replayMutex.RUnlock()
var flows []binding.Flow
flows = append(flows, c.loadBalancerServiceFromOutsideFlow(svcIP, svcPort, protocol))
cacheKey := fmt.Sprintf("L%s%s%x", svcIP, protocol, svcPort)
cacheKey := fmt.Sprintf("LoadBalancerService_%s_%d_%s", svcIP, svcPort, protocol)
Copy link
Member

Choose a reason for hiding this comment

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

Unintended update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one and another error were introduced by rebasing. Thanks for pointing out.

@jianjuns jianjuns force-pushed the egress-flow branch 7 times, most recently from d4527a7 to 57615a9 Compare March 19, 2021 02:49
// which set the SNAT IP mark on the packets from the ofPort to external.
// As of now, a Pod can be configured with a single SNAT IP in a single
// address family (IPv4 or IPv6).
InstallPodSNATFlows(ofPort uint32, snatMark uint32, isIPv6 bool) error
Copy link
Member

Choose a reason for hiding this comment

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

@jianjuns should there be another interface to forward traffic of Pods whose SNAT IPs are not local to tunnel port with specific tun dest IP specified?
cc @ceclinux

Copy link
Contributor Author

@jianjuns jianjuns Mar 25, 2021

Choose a reason for hiding this comment

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

You are right! I missed the flow. Added it.

@jianjuns jianjuns force-pushed the egress-flow branch 3 times, most recently from 680eae2 to 93b50a9 Compare March 25, 2021 00:27
Add flows that set pkt_mark for egress traffic that should be SNAT'd
using a SNAT IP, including egress traffic from a local Pod to which the
Egress is applied, and traffic from a remote Node that is tunnelled to
the egress Node with the SNAT IP; and flows that tunnel egress traffic
to the remote Node, when the SNAT IP for the traffic is on the local
Node.
Each SNAT IP on the Node will be allocated with a unique integer ID,
which is set to the pkt_mark, and so the SNAT implementation can look
up the right SNAT IP from the pkt_mark. On Linux, SNAT will be
implemented by iptables SNAT rules; on Windows, SNAT is implemented
by OVS NAT.
@jianjuns jianjuns force-pushed the egress-flow branch 2 times, most recently from b42e93f to 4fa781e Compare March 25, 2021 00:38
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

@jianjuns jianjuns merged commit 2cb2310 into antrea-io:main Mar 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants