Add WireGuard tunnels for Multi-cluster traffic #4606

hjiajing · 2023-02-07T01:04:32Z

Add WireGuard tunnels mode for Multi-cluster traffic. WireGuard tunnels will be established between member clusters and responsible for all cross-cluster traffic in this mode.

Implementation:

When a Gateway is enqueued, the controller will initialize the WireGuard interface and route on the host. We use the first IP address of the Service CIDR of the cluster as the WireGuard interface's IP. For example, if the ServiceCIDR is 10.10.1.0/24, then the WireGuard interface's IP address is 10.10.1.0.
When a ClusterInfoImport is enqueued, the controller will try to update the WireGuard peer based on the ClusterInfoImport.

codecov · 2023-02-07T02:15:16Z

Codecov Report

Merging #4606 (725f00e) into main (8eb5c8d) will decrease coverage by 0.20%.
The diff coverage is 9.41%.

@@            Coverage Diff             @@
##             main    #4606      +/-   ##
==========================================
- Coverage   69.79%   69.60%   -0.20%     
==========================================
  Files         402      403       +1     
  Lines       59570    59981     +411     
==========================================
+ Hits        41579    41747     +168     
- Misses      15189    15424     +235     
- Partials     2802     2810       +8

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.38% <ø> (+<0.01%)`	⬆️	Carriedforward from 8eb5c8d
integration-tests	`34.36% <ø> (+0.03%)`	⬆️	Carriedforward from 8eb5c8d
kind-e2e-tests	`47.56% <9.41%> (+0.41%)`	⬆️
unit-tests	`59.75% <ø> (-0.02%)`	⬇️	Carriedforward from 8eb5c8d

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
cmd/antrea-agent/agent.go	`0.00% <ø> (ø)`
...trollers/multicluster/member/gateway_controller.go	`80.55% <ø> (-2.53%)`	⬇️
...gent/controller/noderoute/node_route_controller.go	`67.07% <0.00%> (+0.15%)`	⬆️
pkg/agent/multicluster/mc_route_controller.go	`34.48% <0.00%> (-21.64%)`	⬇️
pkg/agent/openflow/multicluster.go	`94.40% <0.00%> (-0.77%)`	⬇️
pkg/agent/openflow/pipeline.go	`89.12% <ø> (-0.27%)`	⬇️
pkg/agent/route/route_linux.go	`69.48% <0.00%> (-1.59%)`	⬇️
pkg/agent/route/route_windows.go	`34.64% <ø> (ø)`
pkg/agent/wireguard/client_windows.go	`100.00% <ø> (ø)`
pkg/agent/wireguard/client_linux.go	`76.84% <62.06%> (-4.68%)`	⬇️
... and 29 more

luolanzone · 2023-02-08T09:11:22Z

@hjiajing please update your PR summary with well-structure sentences, and also include summary in your top commit to help developer to check what does the commit do without checking the PR in the future.

cmd/antrea-agent/agent.go

pkg/agent/config/node_config.go

multicluster/controllers/multicluster/member/gateway_controller.go

build/charts/antrea/values.yaml

luolanzone · 2023-02-15T10:05:41Z

pkg/agent/openflow/pipeline.go

@@ -377,7 +377,8 @@ var DispositionToString = map[uint32]string{
 var (
 	// snatPktMarkRange takes an 8-bit range of pkt_mark to store the ID of
 	// a SNAT IP. The bit range must match SNATIPMarkMask.
-	snatPktMarkRange = &binding.Range{0, 7}
+	snatPktMarkRange         = &binding.Range{0, 7}
+	multiclusterPktMarkRange = &binding.Range{0, 31}


Better to add a comment for this.

We do not need this range as the mark is constant. LoadPktMarkRange accepts nil as the second parameter.

One bit is enough if the mark is constant, like 0x10/0x10.

cmd/antrea-agent/agent.go

multicluster/controllers/multicluster/member/gateway_controller.go

pkg/agent/route/interfaces.go

pkg/agent/multicluster/mc_route_controller.go

pkg/agent/openflow/multicluster.go

pkg/agent/wireguard/interface.go

pkg/agent/wireguard/client_linux.go

cmd/antrea-agent/agent.go

pkg/agent/config/node_config.go

xliuxu · 2023-02-17T01:50:00Z

pkg/agent/openflow/pipeline.go

@@ -377,7 +377,8 @@ var DispositionToString = map[uint32]string{
 var (
 	// snatPktMarkRange takes an 8-bit range of pkt_mark to store the ID of
 	// a SNAT IP. The bit range must match SNATIPMarkMask.
-	snatPktMarkRange = &binding.Range{0, 7}
+	snatPktMarkRange         = &binding.Range{0, 7}
+	multiclusterPktMarkRange = &binding.Range{0, 31}


We do not need this range as the mark is constant. LoadPktMarkRange accepts nil as the second parameter.

xliuxu · 2023-02-17T01:55:52Z

pkg/agent/openflow/multicluster.go

+
+	// MulticlusterPktMark is the packet marker used for mark the cross-cluster
+	// traffic in Antrea Multi-cluster.
+	MulticlusterPktMark uint32 = 0x4d2


I am not sure if 0x4d2 (1234 in decimal) is a meaningful value here. Egress allocates [1,255] for the pkt_mark. So in theory any value above 255 should be OK. But we might want to reservice more bits in case we want to extend the egress feature in the future.

Done. Just use one bit now.

hongliangl

Maybe add some description about how the traffic of a Pod is forwarded to another cluster via WireGuard.
A question about the design: why policy-based routing (ip rule, pkt mark and another route table) is used? Is there any possible to implement the target by adding static route in main route table? If we introduce policy-based routing in this PR, do you think we can use policy-based routing in other components of Antrea @jianjuns @tnqn ?

pkg/agent/openflow/multicluster.go

hongliangl · 2023-02-20T08:10:06Z

pkg/agent/openflow/pipeline.go

@@ -377,7 +377,8 @@ var DispositionToString = map[uint32]string{
 var (
 	// snatPktMarkRange takes an 8-bit range of pkt_mark to store the ID of
 	// a SNAT IP. The bit range must match SNATIPMarkMask.
-	snatPktMarkRange = &binding.Range{0, 7}
+	snatPktMarkRange         = &binding.Range{0, 7}
+	multiclusterPktMarkRange = &binding.Range{0, 31}


One bit is enough if the mark is constant, like 0x10/0x10.

pkg/agent/route/interfaces.go

pkg/agent/multicluster/mc_route_controller.go

hongliangl · 2023-02-20T08:32:49Z

pkg/agent/multicluster/mc_route_controller.go

+}
+
+func (c *MCRouteController) initializeWireGuardInterface() error {
+	c.wireGuardConfig.MTU = c.nodeConfig.NodeMTU - config.WireGuardOverhead - config.GeneveOverhead


Why geneve overhead is included?

+1， @hjiajing you should check current tunnel type in NetworkConfig.

Thanks for the advice, will do it later.

We should use NetworkConfig.MTUDeduction in Lan's PR #4407.

Thanks for reminder, will rebase and fix it .

pkg/agent/route/route_windows.go

luolanzone · 2023-04-26T02:04:05Z

/test-all

luolanzone · 2023-04-26T02:04:52Z

@hjiajing could you check if possible to improve the patch test coverage?

luolanzone · 2023-05-10T02:20:07Z

@hjiajing please resolve the file conflict.

hjiajing · 2023-05-11T02:18:25Z

/test-all

luolanzone · 2023-05-11T08:29:02Z

/test-e2e
/test-multicluster-e2e
/test-networkpolicy

luolanzone

LGTM overall, a few nits on the test file. @jianjuns could you take a look again? thanks.

pkg/agent/multicluster/mc_route_controller_test.go

cmd/antrea-agent/agent.go

multicluster/apis/multicluster/v1alpha1/gateway_types.go

multicluster/config/crd/bases/multicluster.crd.antrea.io_gateways.yaml

pkg/agent/multicluster/mc_route_controller.go

pkg/agent/openflow/client.go

tnqn · 2023-05-11T17:07:04Z

pkg/agent/route/route_linux.go

+		LinkIndex: linkIndex,
+	}
+
+	return c.netlink.RouteDel(route)


It should check the error is not NotFound, otherwise if the route doesn't exist somehow, the caller will keep retrying.

A check is added, when got "no such process", which means the route does not exist, the function will return nil

pkg/agent/route/route_windows.go

build/charts/antrea/README.md

hjiajing · 2023-05-12T06:51:12Z

@tnqns Hi Quan. I have addressed the comments you left, but after I pushed the latest code, some comments disappeared both in the conversation and Files changed, so I didn't reply every comment you left. Could you please take a look? Thanks.

pkg/agent/multicluster/mc_route_controller.go

tnqn · 2023-05-12T13:41:45Z

pkg/agent/multicluster/mc_route_controller.go

+	remoteWireGuardIP := serviceIP.Mask(serviceCIDR.Mask)
+	remoteWireGuardNet := &net.IPNet{IP: remoteWireGuardIP, Mask: net.CIDRMask(32, 32)}


You mean the value is provided be user, not discovered by program?

tnqn

LGTM overall, some minor comments

pkg/agent/multicluster/mc_route_controller.go

tnqn · 2023-05-12T13:51:37Z

pkg/agent/multicluster/mc_route_controller.go

+		_, serviceCIDR, err := net.ParseCIDR(spec.ServiceCIDR)
+		if err != nil {
+			klog.Warningf("Failed to get the peer Gateway IP for WireGuard, error: %v", err)
+			return nil


I think we can now remove the error handling of net.ParseCIDR assuming the CIDR in the spec is either empty or valid. We should validate input before it's accepted and no need to validate it again when consuming it.

tnqn · 2023-05-12T13:52:53Z

pkg/agent/multicluster/mc_route_controller.go

+func getLocalGatewayIP(gateway *mcv1alpha1.Gateway, enableWireGuard bool) net.IP {
+	if enableWireGuard {
+		if gateway.ServiceCIDR == "" {
+			klog.InfoS("The ServiceCIDR has not been updated, skip.", "Gateway", klog.KObj(gateway))


Suggested change

klog.InfoS("The ServiceCIDR has not been updated, skip.", "Gateway", klog.KObj(gateway))

klog.InfoS("The ServiceCIDR of the Gateway has not been updated, skip it", "Gateway", klog.KObj(gateway))

tnqn · 2023-05-12T13:53:25Z

pkg/agent/multicluster/mc_route_controller.go

+		if err != nil {
+			klog.Warningf("Failed to get the local Gateway IP for WireGuard, error: %v", err)
+			return nil


tnqn · 2023-05-12T14:02:59Z

pkg/agent/multicluster/mc_route_controller.go

+	if cache.Spec.ServiceCIDR == cur.Spec.ServiceCIDR && cache.Spec.WireGuard != nil && cur.Spec.WireGuard != nil &&
+		cache.Spec.WireGuard.PublicKey == cur.Spec.WireGuard.PublicKey {
+		return false
+	}
+	return true


It should return false when both old WireGuard and new WireGuard are nil.

Suggested change

if cache.Spec.ServiceCIDR == cur.Spec.ServiceCIDR && cache.Spec.WireGuard != nil && cur.Spec.WireGuard != nil &&

cache.Spec.WireGuard.PublicKey == cur.Spec.WireGuard.PublicKey {

return false

}

return true

if cache.Spec.ServiceCIDR != cur.Spec.ServiceCIDR {

return true

}

if cache.Spec.WireGuard == nil && cur.Spec.WireGuard == nil {

return false

}

if cache.Spec.WireGuard == nil || cur.Spec.WireGuard == nil {

return true

}

return cache.Spec.WireGuard.PublicKey != cur.Spec.WireGuard.PublicKey

tnqn · 2023-05-12T14:04:30Z

pkg/agent/route/route_windows.go

+}
+
+func (c *Client) DeleteRouteForLink(dstCIDR *net.IPNet, linkIndex int) error {
+	return errors.New("AddRouteForLink is not implemented on Windows")


Suggested change

return errors.New("AddRouteForLink is not implemented on Windows")

return errors.New("DeleteRouteForLink is not implemented on Windows")

Add a traffic encryption mode for cross-cluster traffic. If WireGuard enabled, corresponding WireGuard configuration will be created on the Gateway Node. And all cross-cluster traffic will go through the WireGuard tunnel to remote Gateway. Signed-off-by: hjiajing <hjiajing@vmware.com>

tnqn

LGTM

tnqn · 2023-05-16T15:31:28Z

@jianjuns @luolanzone do you have other comments for this one?

jianjuns

@jianjuns @luolanzone do you have other comments for this one?

I am good.

tnqn · 2023-05-17T02:07:53Z

/test-multicluster-e2e
/test-all

tnqn · 2023-05-17T05:32:53Z

/test-multicluster-e2e

…ntrea-io#4606) Add a traffic encryption mode for cross-cluster traffic. If WireGuard is enabled, corresponding WireGuard configuration will be created on the Gateway Node. And all cross-cluster traffic will go through the WireGuard tunnel to remote Gateway. Signed-off-by: hjiajing <hjiajing@vmware.com>

luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Feb 7, 2023

hjiajing force-pushed the wireguard branch 2 times, most recently from 2d4d744 to 5ca2c28 Compare February 15, 2023 02:36

hjiajing requested a review from jianjuns February 15, 2023 02:37

hjiajing force-pushed the wireguard branch 5 times, most recently from 08769ca to 2154901 Compare February 15, 2023 09:36

luolanzone added this to the Antrea v1.11 release milestone Feb 15, 2023

luolanzone requested a review from xliuxu February 15, 2023 09:57

hjiajing force-pushed the wireguard branch 2 times, most recently from 014d4ae to 4ccdccc Compare February 15, 2023 17:42

jianjuns reviewed Feb 15, 2023

View reviewed changes

hjiajing force-pushed the wireguard branch from 4ccdccc to e14a4f3 Compare February 16, 2023 01:45

luolanzone reviewed Feb 16, 2023

View reviewed changes

hjiajing force-pushed the wireguard branch 6 times, most recently from e25e55c to f5fe28f Compare February 16, 2023 09:41

jianjuns reviewed Feb 16, 2023

View reviewed changes

cmd/antrea-agent/agent.go Show resolved Hide resolved

pkg/agent/config/node_config.go Outdated Show resolved Hide resolved

xliuxu reviewed Feb 17, 2023

View reviewed changes

hongliangl reviewed Feb 20, 2023

View reviewed changes

hjiajing force-pushed the wireguard branch 3 times, most recently from 9017a57 to d979b4e Compare February 22, 2023 08:13

luolanzone previously approved these changes Apr 26, 2023

View reviewed changes

luolanzone added the action/release-note Indicates a PR that should be included in release notes. label May 8, 2023

hjiajing dismissed luolanzone’s stale review via 392338c May 9, 2023 17:08

hjiajing force-pushed the wireguard branch from 2da8c23 to 392338c Compare May 9, 2023 17:08

hjiajing force-pushed the wireguard branch from 392338c to 4509369 Compare May 11, 2023 01:21

luolanzone reviewed May 11, 2023

View reviewed changes

hjiajing force-pushed the wireguard branch from 4509369 to b8e38c3 Compare May 11, 2023 08:48

tnqn reviewed May 11, 2023

View reviewed changes

tnqn reviewed May 12, 2023

View reviewed changes

build/charts/antrea/README.md Outdated Show resolved Hide resolved

hjiajing force-pushed the wireguard branch 2 times, most recently from e02018e to 0055bef Compare May 12, 2023 06:39

tnqn reviewed May 12, 2023

View reviewed changes

luolanzone mentioned this pull request May 15, 2023

Add unit tests for pkg/agent/agent.go #4703

Merged

hjiajing force-pushed the wireguard branch from 0055bef to 11ae823 Compare May 15, 2023 14:26

tnqn approved these changes May 16, 2023

View reviewed changes

jianjuns approved these changes May 16, 2023

View reviewed changes

luolanzone approved these changes May 17, 2023

View reviewed changes

tnqn merged commit 45a06d7 into antrea-io:main May 17, 2023

		remoteWireGuardIP := serviceIP.Mask(serviceCIDR.Mask)
		remoteWireGuardNet := &net.IPNet{IP: remoteWireGuardIP, Mask: net.CIDRMask(32, 32)}

	klog.InfoS("The ServiceCIDR has not been updated, skip.", "Gateway", klog.KObj(gateway))
	klog.InfoS("The ServiceCIDR of the Gateway has not been updated, skip it", "Gateway", klog.KObj(gateway))

	return errors.New("AddRouteForLink is not implemented on Windows")
	return errors.New("DeleteRouteForLink is not implemented on Windows")

Add WireGuard tunnels for Multi-cluster traffic #4606

Add WireGuard tunnels for Multi-cluster traffic #4606

Conversation

hjiajing commented Feb 7, 2023 • edited Loading

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

luolanzone commented Feb 8, 2023

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

hongliangl 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

Choose a reason for hiding this comment

luolanzone commented Apr 26, 2023

luolanzone commented Apr 26, 2023

luolanzone commented May 10, 2023

hjiajing commented May 11, 2023

luolanzone commented May 11, 2023

luolanzone left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjiajing commented May 12, 2023

Choose a reason for hiding this comment

tnqn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented May 16, 2023

jianjuns left a comment

Choose a reason for hiding this comment

tnqn commented May 17, 2023

tnqn commented May 17, 2023

hjiajing commented Feb 7, 2023 •

edited

Loading

codecov bot commented Feb 7, 2023 •

edited

Loading

luolanzone left a comment •

edited

Loading