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

wireguard: Set wireguard and route MTU to detected MTU #16020

Merged
merged 1 commit into from
May 10, 2021
Merged

wireguard: Set wireguard and route MTU to detected MTU #16020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions daemon/cmd/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,14 @@ func NewDaemon(ctx context.Context, cancel context.CancelFunc, epMgr *endpointma
externalIP = node.GetIPv6()
}
// ExternalIP could be nil but we are covering that case inside NewConfiguration
mtuConfig = mtu.NewConfiguration(authKeySize, option.Config.EnableIPSec, option.Config.Tunnel != option.TunnelDisabled, configuredMTU, externalIP)
mtuConfig = mtu.NewConfiguration(
authKeySize,
option.Config.EnableIPSec,
option.Config.Tunnel != option.TunnelDisabled,
option.Config.EnableWireguard,
configuredMTU,
externalIP,
)

nodeMngr, err := nodemanager.NewManager("all", dp.Node(), ipcache.IPIdentityCache, option.Config)
if err != nil {
Expand Down Expand Up @@ -554,7 +561,7 @@ func NewDaemon(ctx context.Context, cancel context.CancelFunc, epMgr *endpointma
}

if wgAgent := dp.WireguardAgent(); option.Config.EnableWireguard {
if err := wgAgent.Init(); err != nil {
if err := wgAgent.Init(mtuConfig); err != nil {
log.WithError(err).Fatal("Failed to initialize wireguard agent")
}
}
Expand Down
14 changes: 7 additions & 7 deletions daemon/cmd/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "create a client ID and store it locally",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{
Expand Down Expand Up @@ -114,7 +114,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "retrieve nodes diff from a client that was already present",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{
Expand Down Expand Up @@ -168,7 +168,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "retrieve nodes from an expired client, it should be ok because the clean up only happens when on insertion",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{
Expand Down Expand Up @@ -223,7 +223,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "retrieve nodes for a new client, the expired client should be deleted",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{
Expand Down Expand Up @@ -273,7 +273,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "retrieve nodes for a new client, however the randomizer allocated an existing clientID, so we should return a empty clientID",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{
Expand Down Expand Up @@ -323,7 +323,7 @@ func (g *GetNodesSuite) Test_getNodesHandle(c *C) {
{
name: "retrieve nodes for a client that does not want to have diffs, leave all other stored clients alone",
setupArgs: func() args {
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery := nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{})
nodeDiscovery.LocalNode.Name = "foo"
return args{
params: GetClusterNodesParams{},
Expand Down Expand Up @@ -433,7 +433,7 @@ func (g *GetNodesSuite) Test_cleanupClients(c *C) {
h := &getNodes{
clients: args.clients,
d: &Daemon{
nodeDiscovery: nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, 0, nil), &cnitypes.NetConf{}),
nodeDiscovery: nodediscovery.NewNodeDiscovery(nm, mtu.NewConfiguration(0, false, false, false, 0, nil), &cnitypes.NetConf{}),
},
}
h.cleanupClients()
Expand Down
2 changes: 1 addition & 1 deletion pkg/datapath/linux/node_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ const (
func (s *linuxPrivilegedBaseTestSuite) SetUpTest(c *check.C, addressing datapath.NodeAddressing, enableIPv6, enableIPv4 bool) {
bpf.ConfigureResourceLimits()
s.nodeAddressing = addressing
s.mtuConfig = mtu.NewConfiguration(0, false, false, 1500, nil)
s.mtuConfig = mtu.NewConfiguration(0, false, false, false, 1500, nil)
s.enableIPv6 = enableIPv6
s.enableIPv4 = enableIPv4

Expand Down
2 changes: 1 addition & 1 deletion pkg/datapath/linux/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import (
var (
nh = linuxNodeHandler{
nodeConfig: datapath.LocalNodeConfiguration{
MtuConfig: mtu.NewConfiguration(0, false, false, 100, net.IP("1.1.1.1")),
MtuConfig: mtu.NewConfiguration(0, false, false, false, 100, net.IP("1.1.1.1")),
},
nodeAddressing: fake.NewNodeAddressing(),
datapathConfig: DatapathConfiguration{
Expand Down
3 changes: 2 additions & 1 deletion pkg/datapath/wireguard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import (
"net"

"github.com/cilium/cilium/api/v1/models"
"github.com/cilium/cilium/pkg/mtu"
)

// WireguardAgent manages the Wireguard peers
type WireguardAgent interface {
Init() error
Init(mtuConfig mtu.Configuration) error
UpdatePeer(nodeName, pubKeyHex string, nodeIPv4, nodeIPv6 net.IP) error
DeletePeer(nodeName string) error
Status(includePeers bool) (*models.WireguardStatus, error)
Expand Down
2 changes: 1 addition & 1 deletion pkg/ipam/allocator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ func (o *ownerMock) K8sEventReceived(scope string, action string, valid, equal b
func (o *ownerMock) K8sEventProcessed(scope string, action string, status bool) {}
func (o *ownerMock) UpdateCiliumNodeResource() {}

var mtuMock = mtu.NewConfiguration(0, false, false, 1500, nil)
var mtuMock = mtu.NewConfiguration(0, false, false, false, 1500, nil)

func (s *IPAMSuite) TestAllocatedIPDump(c *C) {
fakeAddressing := fake.NewNodeAddressing()
Expand Down
39 changes: 29 additions & 10 deletions pkg/mtu/mtu.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

package mtu

import "net"
import (
"net"
)

const (
// MaxMTU is the highest MTU that can be used for devices and routes
Expand Down Expand Up @@ -60,6 +62,17 @@ const (
// size for GCM(AES*) in RFC4106. Users may input other lengths via
// key secrets.
EncryptionDefaultAuthKeyLength = 16

// WireguardOverhead is an approximation for the overhead of wireguard
// encapsulation.
//
// https://github.com/torvalds/linux/blob/v5.12/drivers/net/wireguard/device.c#L262:
// MESSAGE_MINIMUM_LENGTH: 32B
// Outer IPv4 or IPv6 header: 40B
// Outer UDP header: 8B
// ---
// Total extra bytes: 80B
WireguardOverhead = 80
)

// Configuration is an MTU configuration as returned by NewConfiguration
Expand Down Expand Up @@ -90,15 +103,16 @@ type Configuration struct {
// overhead, if any, but assumes packets are already encrypted.
postEncryptMTU int

encapEnabled bool
encryptEnabled bool
encapEnabled bool
encryptEnabled bool
Copy link
Member

Choose a reason for hiding this comment

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

As a follow up we should probably s/encryptEnabled/ipsecEnabled.

wireguardEnabled bool
}

// NewConfiguration returns a new MTU configuration. The MTU can be manually
// specified, otherwise it will be automatically detected. if encapEnabled is
// true, the MTU is adjusted to account for encapsulation overhead for all
// routes involved in node to node communication.
func NewConfiguration(authKeySize int, encryptEnabled bool, encapEnabled bool, mtu int, mtuDetectIP net.IP) Configuration {
func NewConfiguration(authKeySize int, encryptEnabled bool, encapEnabled bool, wireguardEnabled bool, mtu int, mtuDetectIP net.IP) Configuration {
encryptOverhead := 0

if mtu == 0 {
Expand All @@ -122,12 +136,13 @@ func NewConfiguration(authKeySize int, encryptEnabled bool, encapEnabled bool, m
}

conf := Configuration{
standardMTU: mtu,
tunnelMTU: mtu - (TunnelOverhead + encryptOverhead),
postEncryptMTU: mtu - TunnelOverhead,
preEncryptMTU: mtu - encryptOverhead,
encapEnabled: encapEnabled,
encryptEnabled: encryptEnabled,
standardMTU: mtu,
tunnelMTU: mtu - (TunnelOverhead + encryptOverhead),
postEncryptMTU: mtu - TunnelOverhead,
preEncryptMTU: mtu - encryptOverhead,
encapEnabled: encapEnabled,
encryptEnabled: encryptEnabled,
wireguardEnabled: wireguardEnabled,
}

if conf.tunnelMTU < 0 {
Expand Down Expand Up @@ -155,6 +170,10 @@ func (c *Configuration) GetRoutePostEncryptMTU() int {
// tunneling mode and/or with encryption enabled, this will have tunnel and
// encryption overhead accounted for.
func (c *Configuration) GetRouteMTU() int {
if c.wireguardEnabled {
return c.GetDeviceMTU() - WireguardOverhead
}

if !c.encapEnabled && !c.encryptEnabled {
return c.GetDeviceMTU()
}
Expand Down
35 changes: 22 additions & 13 deletions pkg/mtu/mtu_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,63 +31,72 @@ var _ = Suite(&MTUSuite{})

func (m *MTUSuite) TestNewConfiguration(c *C) {
// Add routes with no encryption or tunnel
conf := NewConfiguration(0, false, false, 0, nil)
conf := NewConfiguration(0, false, false, false, 0, nil)
c.Assert(conf.GetDeviceMTU(), Not(Equals), 0)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU())

// Add routes with no encryption or tunnel and set MTU
conf = NewConfiguration(0, false, false, 1400, nil)
conf = NewConfiguration(0, false, false, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU())

// Add routes with tunnel
conf = NewConfiguration(0, false, true, 1400, nil)
conf = NewConfiguration(0, false, true, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-TunnelOverhead)

// Add routes with tunnel and set MTU
conf = NewConfiguration(0, false, true, 1400, nil)
conf = NewConfiguration(0, false, true, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-TunnelOverhead)

// Add routes with encryption and set MTU using standard 128bit, larger 256bit and smaller 96bit ICVlen keys
conf = NewConfiguration(16, true, false, 1400, nil)
conf = NewConfiguration(16, true, false, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-EncryptionIPsecOverhead)

conf = NewConfiguration(32, true, false, 1400, nil)
conf = NewConfiguration(32, true, false, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-(EncryptionIPsecOverhead+16))

conf = NewConfiguration(12, true, false, 1400, nil)
conf = NewConfiguration(12, true, false, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-(EncryptionIPsecOverhead-4))

// Add routes with encryption and tunnels using standard 128bit, larger 256bit and smaller 96bit ICVlen keys
conf = NewConfiguration(16, true, true, 1400, nil)
conf = NewConfiguration(16, true, true, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-(TunnelOverhead+EncryptionIPsecOverhead))

conf = NewConfiguration(32, true, true, 1400, nil)
conf = NewConfiguration(32, true, true, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-(TunnelOverhead+EncryptionIPsecOverhead+16))

conf = NewConfiguration(32, true, true, 1400, nil)
conf = NewConfiguration(32, true, true, false, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-(TunnelOverhead+EncryptionIPsecOverhead+16))

// Add routes with wireguard enabled
conf = NewConfiguration(32, false, false, true, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-WireguardOverhead)

conf = NewConfiguration(32, false, true, true, 1400, nil)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)
c.Assert(conf.GetRouteMTU(), Equals, conf.GetDeviceMTU()-WireguardOverhead)

testIP1 := net.IPv4(0, 0, 0, 0)
testIP2 := net.IPv4(127, 0, 0, 1)
result, _ := getMTUFromIf(testIP1)
c.Assert(result, Equals, 0)

conf = NewConfiguration(0, true, true, 1400, testIP1)
conf = NewConfiguration(0, true, true, false, 1400, testIP1)
c.Assert(conf.GetDeviceMTU(), Equals, 1400)

conf = NewConfiguration(0, true, true, 0, testIP1)
conf = NewConfiguration(0, true, true, false, 0, testIP1)
c.Assert(conf.GetDeviceMTU(), Equals, 1500)

// Assuming loopback interface always exists and has mtu=65536
conf = NewConfiguration(0, true, true, 0, testIP2)
conf = NewConfiguration(0, true, true, false, 0, testIP2)
c.Assert(conf.GetDeviceMTU(), Equals, 65536)
}
8 changes: 7 additions & 1 deletion pkg/wireguard/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/cilium/cilium/pkg/lock"
"github.com/cilium/cilium/pkg/logging"
"github.com/cilium/cilium/pkg/logging/logfields"
"github.com/cilium/cilium/pkg/mtu"
"github.com/cilium/cilium/pkg/node"
"github.com/cilium/cilium/pkg/option"
"github.com/cilium/cilium/pkg/sysctl"
Expand Down Expand Up @@ -107,7 +108,7 @@ func (a *Agent) Close() error {
}

// Init is called after we have obtained a local Wireguard IP
func (a *Agent) Init() error {
func (a *Agent) Init(mtuConfig mtu.Configuration) error {
link := &netlink.Wireguard{LinkAttrs: netlink.LinkAttrs{Name: types.IfaceName}}
err := netlink.LinkAdd(link)
if err != nil && !errors.Is(err, unix.EEXIST) {
Expand All @@ -134,6 +135,11 @@ func (a *Agent) Init() error {
return err
}

linkMTU := mtuConfig.GetDeviceMTU() - mtu.WireguardOverhead
if err := netlink.LinkSetMTU(link, linkMTU); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not enough. We need to make sure that route MTU in pod netns is set accordingly (MTU minus the WG overhead), grep for GetRouteMTU. Maybe this is exactly the same what you went in:

Should pkg/mtu be refactored so that the overhead computation works for both IPsec and Wireguard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep you're right. The route MTU was set too high. Modified pkg/mtu to handle the wireguard case. That package needs a rewrite soon, but as discussed with @brb this will be addressed in v1.11 anyway, so not worth addressing that now.

return err
}

if err := netlink.LinkSetUp(link); err != nil {
return err
}
Expand Down