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 WireGuard support for tunnel traffic encryption #2297

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

xliuxu
Copy link
Contributor

@xliuxu xliuxu commented Jun 23, 2021

Add WireGuard client for tunnel traffic encryption. This PR implements #2243.

Signed-off-by: Xu Liu [email protected]

@xliuxu xliuxu changed the title Add WireGuard client for tunnel traffic encryption. This PR is a part Add WireGuard client for tunnel traffic encryption. Jun 23, 2021
@xliuxu xliuxu changed the title Add WireGuard client for tunnel traffic encryption. Add WireGuard client for tunnel traffic encryption Jun 23, 2021
@xliuxu xliuxu force-pushed the wireguard_client branch 4 times, most recently from b823ec3 to bd9fa04 Compare June 23, 2021 02:34
@codecov-commenter
Copy link

codecov-commenter commented Jun 23, 2021

Codecov Report

Merging #2297 (b054878) into main (f425a87) will increase coverage by 5.31%.
The diff coverage is 70.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2297      +/-   ##
==========================================
+ Coverage   60.34%   65.66%   +5.31%     
==========================================
  Files         283      285       +2     
  Lines       22866    26335    +3469     
==========================================
+ Hits        13798    17292    +3494     
+ Misses       7585     7427     -158     
- Partials     1483     1616     +133     
Flag Coverage Δ
e2e-tests 57.15% <58.76%> (?)
kind-e2e-tests 48.35% <55.67%> (+0.29%) ⬆️
unit-tests 41.07% <38.06%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/config/traffic_encap_mode.go 94.73% <ø> (-5.27%) ⬇️
...gent/controller/noderoute/node_route_controller.go 62.23% <65.78%> (+7.02%) ⬆️
pkg/agent/wireguard/client_linux.go 66.96% <66.96%> (ø)
pkg/agent/agent.go 59.09% <72.72%> (+8.67%) ⬆️
pkg/agent/openflow/client.go 71.71% <72.72%> (+14.11%) ⬆️
pkg/agent/route/route_linux.go 48.76% <77.77%> (+5.76%) ⬆️
pkg/agent/config/node_config.go 94.11% <83.33%> (-5.89%) ⬇️
pkg/agent/config/traffic_encryption_mode.go 91.66% <91.66%> (ø)
pkg/agent/openflow/pipeline.go 83.62% <100.00%> (+10.41%) ⬆️
pkg/controller/egress/ipallocator/allocator.go 65.00% <0.00%> (-15.42%) ⬇️
... and 278 more

@xliuxu xliuxu marked this pull request as draft June 24, 2021 03:02
@xliuxu xliuxu force-pushed the wireguard_client branch 7 times, most recently from 2a6e9cf to 2564d69 Compare June 28, 2021 14:59
@xliuxu
Copy link
Contributor Author

xliuxu commented Jun 28, 2021

/test-e2e

@xliuxu xliuxu marked this pull request as ready for review June 28, 2021 15:25
@xliuxu
Copy link
Contributor Author

xliuxu commented Jun 28, 2021

/test-e2e

@xliuxu
Copy link
Contributor Author

xliuxu commented Jun 30, 2021

/test-e2e

@xliuxu xliuxu changed the title Add WireGuard client for tunnel traffic encryption Add WireGuard support for tunnel traffic encryption Jun 30, 2021
@xliuxu
Copy link
Contributor Author

xliuxu commented Jun 30, 2021

/test-all

@xliuxu xliuxu force-pushed the wireguard_client branch 2 times, most recently from 5166d25 to 8ef795f Compare June 30, 2021 08:04
@xliuxu
Copy link
Contributor Author

xliuxu commented Jun 30, 2021

/test-conformance
/test-e2e

@xliuxu
Copy link
Contributor Author

xliuxu commented Jul 1, 2021

/test-e2e

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

sorry for the late review

#trafficEncryptionMode: none

# The port for WireGuard to receive traffic.
#wireGuardPort: 51820
Copy link
Contributor

Choose a reason for hiding this comment

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

have we decided that it is unlikely we will be adding more wireguard config parameters in the future? I'm referring to an earlier suggestion of grouping wireguard parameters in their own dictionary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just saw your reply: #2297 (comment)
refactoring in the future is not great as it breaks backward compatibility. I'll leave it up to you to decide based on how confident you are that we will not need additional configuration parameters in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to group WireGuard related settings together. PTAL.

cmd/antrea-agent/agent.go Outdated Show resolved Hide resolved
hack/generate-manifest.sh Outdated Show resolved Hide resolved
hack/generate-manifest.sh Outdated Show resolved Hide resolved
pkg/agent/openflow/client_test.go Outdated Show resolved Hide resolved
pkg/agent/wireguard/client_linux.go Outdated Show resolved Hide resolved
pkg/agent/wireguard/client_linux.go Outdated Show resolved Hide resolved
pkg/agent/wireguard/client_linux.go Outdated Show resolved Hide resolved
pkg/agent/types/annotations.go Outdated Show resolved Hide resolved
@@ -77,8 +77,18 @@ featureGates:
# also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
#defaultMTU: 0

# Whether or not to enable IPsec encryption of tunnel traffic.
#enableIPSecTunnel: false
# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to understand "Currently encryption only works with antrea encap mode."
I see a check in the code for IPsec but not for Wireguard. I was also under the assumption that with @tnqn's design, the encap mode didn't really matter since we would always end up using a Wireguard tunnel. It seems to me that we are running the WireGuard e2e tests on Kind in all traffic modes, including noEncap.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. In this implementation, WG is like a new encap mode. But it wont work with networkPolicyOnly right? I need to think through the relations between encap mode and WG..

(Originally I suggested to support only encap mode the same way as IPsec for a shared implementation, but as the current code does not looks complex either, I am not against to it).

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should still require trafficEncryptionMode = encap, and add a comment on tunnelType saying tunnelType will not take effect when trafficEncryptionMode = wireGuard.

Maybe also move trafficEncryptionMode to be after tunnelType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. WG is like a new encap mode. I have updated the comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Originally, I thought WG should be viewed as encap, as it has the same behavior with other tunnel types from user perspective, and it is definitely not noEncap or hybrid (which require underlay network to forward/route Pod traffic). So, in my mind, we should enforce trafficEncapMode = encap, and document tunnelType will not take effect. I talked to @xliuxu about this offline.

However, after more thinking, I got the question on networkPolicyOnly mode. Originally I thought WG cannot work with networkPolicyOnly mode, but now I start to think it depends on how we define networkPolicyOnly mode: if we expect another CNI to handle connectivity/routing, then when WG is enabled, what that CNI does might not take effect; but if we assume another CNI could just do IPAM and users do not care how forwarding/routing is done, then WG might still work with the CNI.

But at least for this version, I still suggest to enforce trafficEncapMode = encap, and we can spend more time to think through the use case of another IPAM + WG, and validate it indeed works (let me know if we already tested WG + networkPolicyOnly on AKS and EKS).

@tnqn @antoninbas : thoughts.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe I'm over simplifying but it feels like Wireguard is really just an encrypted overlay. In a way, wireguard could really be merged with tunnelType and tunnelType could have the following values: vxlan, geneve, gre, vxlanIPsec, geneveIPsec, greIPsec, wireguard. It feels to me that for for networkPolicyOnly mode (IPAM + WG), it's not really specific to Wireguard at all. You could do the same thing with a Geneve overlay. At the end of the day, packets are encapsulated and I don't think that what you expect of networkPolicyOnly mode. Am I missing something?

For this PR / release, I agree with you: we should enforce trafficEncapMode == encap.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

Thought over. Probably we should keep networkPolicyOnly as is (it means another CNI should take care of traffic routing across Nodes and it could support traffic encryption too). Later, if there are requirements, we can support other IPAM plugin separately (with Antrea handles routing).

Copy link
Contributor

Choose a reason for hiding this comment

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

You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

It's pretty similar no? With Wireguard, you still have to install Pod routes through the wireguard device on the host and configure the wireguard peers, while with Geneve it would be OVS flows. So the implementation is a bit different, but the amount of information you need is the same (list of Pods IPs for each Node)?

// tunnelPeerIP is the Node Internal Address. In a dual-stack setup, one Node has 2 Node Internal
// Addresses (IPv4 and IPv6) .
if c.networkConfig.TrafficEncryptionMode != config.TrafficEncryptionModeWireGuard &&
((!isIPv6 && c.networkConfig.TrafficEncapMode.NeedsEncapToPeer(tunnelPeerIPs.IPv4, c.nodeConfig.NodeTransportIPv4Addr)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we can consider to change NeedsEncapToPeer to a func of NetworkConfig, and cover TrafficEncryptionModeWireGuard. No strong opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that. I forgot to leave a comment in my review but I was going to suggest moving the checks to a dedicated function or updating NeedsEncapToPeer.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we decide WG requires trafficEncapMode = encap, probably we still keep the current definition (or we rename NeedsEncapToPeer to sth. like NeedsOVSTunnelToPeer and move it to NetworkConfig and cover TrafficEncryptionModeWireGuard).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think WG can work with all trafficEncapModes. I moved the function NeedsEncapToPeer to NetworkConfig to cover TrafficEncryptionMode settings.

@@ -77,8 +77,18 @@ featureGates:
# also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
#defaultMTU: 0

# Whether or not to enable IPsec encryption of tunnel traffic.
#enableIPSecTunnel: false
# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch. In this implementation, WG is like a new encap mode. But it wont work with networkPolicyOnly right? I need to think through the relations between encap mode and WG..

(Originally I suggested to support only encap mode the same way as IPsec for a shared implementation, but as the current code does not looks complex either, I am not against to it).

@xliuxu
Copy link
Contributor Author

xliuxu commented Aug 27, 2021

Thanks @antoninbas and @jianjuns for the review. I have addressed all comments. PTAL.

@xliuxu
Copy link
Contributor Author

xliuxu commented Aug 27, 2021

/test-all

build/yamls/base/conf/antrea-agent.conf Outdated Show resolved Hide resolved
build/yamls/base/conf/antrea-agent.conf Outdated Show resolved Hide resolved
cmd/antrea-agent/config.go Outdated Show resolved Hide resolved
@@ -134,8 +138,8 @@ func (o *Options) validate(args []string) error {
if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) {
return fmt.Errorf("TrafficEncapMode %s requires AntreaProxy to be enabled", o.config.TrafficEncapMode)
}
if o.config.EnableIPSecTunnel {
return fmt.Errorf("IPsec tunnel may only be enabled in %s mode", config.TrafficEncapModeEncap)
if encryptionMode == config.TrafficEncryptionModeIPSec {
Copy link
Member

Choose a reason for hiding this comment

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

From the discussion, I think this should check encryptionMode != TrafficEncryptionModeNone now?

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.

// NeedsDirectRoutingToPeer returns true if Pod traffic to peer Node needs a direct route installed to the routing table.
func (nc *NetworkConfig) NeedsDirectRoutingToPeer(peerIP net.IP, localIP *net.IPNet) bool {
if nc.TrafficEncryptionMode == TrafficEncryptionModeWireGuard {
return true
Copy link
Member

Choose a reason for hiding this comment

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

This change seems not required and might be incorrect? We install direct route when NeedsDirectRoutingToPeer is true. I see you check TrafficEncryptionMode == TrafficEncryptionModeWireGuard first before checking this, then this change is not needed anway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I deleted the check for WireGuard in this function.

@xliuxu xliuxu force-pushed the wireguard_client branch 2 times, most recently from fbb0bf5 to 859f43b Compare August 27, 2021 03:24
jianjuns
jianjuns previously approved these changes Aug 27, 2021
# - geneve (default)
# - vxlan
# - gre
# - stt
#tunnelType: geneve

# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: antrea encap mode -> encap mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated.

@@ -77,8 +77,18 @@ featureGates:
# also adjust MTU to accommodate for tunnel encapsulation overhead (if applicable).
#defaultMTU: 0

# Whether or not to enable IPsec encryption of tunnel traffic.
#enableIPSecTunnel: false
# Determines how tunnel traffic is encrypted. Currently encryption only works with antrea encap mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also say that. But major code changes are needed to support another IPAM with Geneve (while it is much easier with WG, as WG handles traffic routing among Nodes).

Thought over. Probably we should keep networkPolicyOnly as is (it means another CNI should take care of traffic routing across Nodes and it could support traffic encryption too). Later, if there are requirements, we can support other IPAM plugin separately (with Antrea handles routing).

@@ -126,3 +142,17 @@ func IsIPv6Enabled(nodeConfig *NodeConfig, trafficEncapMode TrafficEncapModeType
return nodeConfig.PodIPv6CIDR != nil ||
(trafficEncapMode.IsNetworkPolicyOnly() && nodeConfig.NodeIPv6Addr != nil)
}

// NeedsEncapToPeer returns true if Pod traffic to peer Node needs to be encapsulated.
Copy link
Contributor

Choose a reason for hiding this comment

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

"needs to be encapsulated by OVS tunneling"

Actually, I prefer to rename the func to NeedsTunnelToPeer, and here we can say "needs to be tunneled".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. updated.

antoninbas
antoninbas previously approved these changes Aug 27, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

Looks good on my side

This PR implements antrea-io#2243. Change tunnel traffic encryption option
to enum type. The options contains none (default), ipsec and wireguard.

Signed-off-by: Xu Liu <[email protected]>
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

@tnqn
Copy link
Member

tnqn commented Aug 27, 2021

/test-all

@tnqn
Copy link
Member

tnqn commented Aug 27, 2021

/test-conformance
/test-ipv6-only-e2e
/test-ipv6-ds-e2e

@tnqn
Copy link
Member

tnqn commented Aug 27, 2021

/test-ipv6-e2e

@tnqn tnqn merged commit a2aac03 into antrea-io:main Aug 27, 2021
@tnqn tnqn mentioned this pull request Aug 27, 2021
4 tasks
@xliuxu xliuxu deleted the wireguard_client branch August 27, 2021 13:15
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.

7 participants