[lxc-devel] [lxd/master] Network: Don't use stable volatile MAC when using bridge.external_interfaces with no bridge IPs
tomponline on Github
lxc-bot at linuxcontainers.org
Fri Jul 24 16:41:18 UTC 2020
A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 641 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20200724/80ee9f5d/attachment.bin>
-------------- next part --------------
From 1781ca3fce05f8743356151dcf572af9bcb52c10 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 24 Jul 2020 17:31:22 +0100
Subject: [PATCH 1/2] lxd/networks: Allow update/removal of node-specific key
in non-clustered mode
Updates doNetworkUpdate to detect when in non-clustered mode and allow the update/removal of node-specific keys when doing a PUT.
Otherwise it is not possible to update/remove node-specific keys when doing a PUT, as they are re-added from the current config.
Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
lxd/networks.go | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/lxd/networks.go b/lxd/networks.go
index 0aca89662b..c5c25ce106 100644
--- a/lxd/networks.go
+++ b/lxd/networks.go
@@ -692,7 +692,7 @@ func networkPut(d *Daemon, r *http.Request) response.Response {
}
}
- return doNetworkUpdate(d, name, req, targetNode, isClusterNotification(r), r.Method)
+ return doNetworkUpdate(d, name, req, targetNode, isClusterNotification(r), r.Method, clustered)
}
func networkPatch(d *Daemon, r *http.Request) response.Response {
@@ -701,7 +701,7 @@ func networkPatch(d *Daemon, r *http.Request) response.Response {
// doNetworkUpdate loads the current local network config, merges with the requested network config, validates
// and applies the changes. Will also notify other cluster nodes of non-node specific config if needed.
-func doNetworkUpdate(d *Daemon, name string, req api.NetworkPut, targetNode string, clusterNotification bool, httpMethod string) response.Response {
+func doNetworkUpdate(d *Daemon, name string, req api.NetworkPut, targetNode string, clusterNotification bool, httpMethod string, clustered bool) response.Response {
// Load the local node-specific network.
n, err := network.LoadByName(d.State(), name)
if err != nil {
@@ -712,8 +712,10 @@ func doNetworkUpdate(d *Daemon, name string, req api.NetworkPut, targetNode stri
req.Config = map[string]string{}
}
- if targetNode == "" && httpMethod != http.MethodPatch {
- // If non-node specific config being updated via "put" method, then merge the current
+ // Normally a "put" request will replace all existing config, however when clustered, we need to account
+ // for the node specific config keys and not replace them when the request doesn't specify a specific node.
+ if targetNode == "" && httpMethod != http.MethodPatch && clustered {
+ // If non-node specific config being updated via "put" method in cluster, then merge the current
// node-specific network config with the submitted config to allow validation.
// This allows removal of non-node specific keys when they are absent from request config.
for k, v := range n.Config() {
From b17902d0693f2e067167a6d5c75d24f0d419dfad Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Fri, 24 Jul 2020 17:33:49 +0100
Subject: [PATCH 2/2] lxd/network/driver/bridge: Adds safety check for volatile
MAC address usage
- Adds a stableMACSafe function that inspects the network config and validates whether it is safe to use a stable volatile MAC.
- The scenario when it is considered unsafe to use a stable volatile MAC is when bridge.external_interfaces is in use and there are no IP addresses on the bridge interface.
- This is because in a cluster environment we cannot be sure that the bridge interface on each node are not connected to the same network segment, and using a stable MAC could cause conflicts.
- In the case where a random MAC address is to be used, we now explicitly set it to avoid the bridge MAC changing when instances are connected.
- To avoid changing the random MAC address on LXD restart and causing network glitches, we only set this if the bridge interface has just been created.
- If a static or stable volatile MAC is available then this is always set on LXD start to ensure consistency.
Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
lxd/network/driver_bridge.go | 83 ++++++++++++++++++++++++------------
1 file changed, 55 insertions(+), 28 deletions(-)
diff --git a/lxd/network/driver_bridge.go b/lxd/network/driver_bridge.go
index 2cf58f2f84..f87585e333 100644
--- a/lxd/network/driver_bridge.go
+++ b/lxd/network/driver_bridge.go
@@ -45,31 +45,38 @@ type bridge struct {
common
}
-// getHwaddr retrieves existing static or volatile MAC address from config.
-func (n *bridge) getHwaddr(config map[string]string) string {
- hwAddr := config["bridge.hwaddr"]
- if hwAddr == "" {
- hwAddr = config["volatile.bridge.hwaddr"]
+// fillHwaddr populates the volatile.bridge.hwaddr in config if it, nor bridge.hwaddr, are already set.
+func (n *bridge) fillHwaddr(config map[string]string) error {
+ if config["bridge.hwaddr"] != "" || config["volatile.bridge.hwaddr"] != "" {
+ return nil
}
- return hwAddr
-}
-
-// fillHwaddr populates the volatile.bridge.hwaddr in config if it, nor bridge.hwaddr, are already set.
-// Returns MAC address generated if needed to, otherwise empty string.
-func (n *bridge) fillHwaddr(config map[string]string) (string, error) {
// If no existing MAC address, generate a new one and store in volatile.
- if n.getHwaddr(config) == "" {
- hwAddr, err := instance.DeviceNextInterfaceHWAddr()
- if err != nil {
- return "", errors.Wrapf(err, "Failed generating MAC address")
- }
-
- config["volatile.bridge.hwaddr"] = hwAddr
- return config["volatile.bridge.hwaddr"], nil
+ hwAddr, err := instance.DeviceNextInterfaceHWAddr()
+ if err != nil {
+ return errors.Wrapf(err, "Failed generating MAC address")
}
- return "", nil
+ config["volatile.bridge.hwaddr"] = hwAddr
+ return nil
+}
+
+// stableMACSafe returns whether it is safe to use the stable volatile MAC for the bridge interface.
+// It is not suitable to use the stable volatile MAC when "bridge.external_interfaces" is non-empty and the bridge
+// interface has no IPv4 or IPv6 address set. This is because in a clustered environment the same bridge config is
+// applied to all nodes, and if the bridge is being used to connect multiple nodes to the same network segment it
+// would cause MAC conflicts to use the the same stable MAC on all nodes. Normally if an IP address is specified
+// then connecting multiple nodes to the same network segment would also cause IP conflicts, so if an IP is defined
+// then we assume this is not being done. However if IP addresses are explicitly set to "none" and
+// "bridge.external_interfaces" then it may not be safe to use a stable volatile MAC in a clustered environment.
+func (n *bridge) stableMACSafe() bool {
+ // We can't be sure that multiple clustered nodes aren't connected to the same network segment so don't
+ // use a stable volatile MAC for the bridge interface to avoid introducing a MAC conflict.
+ if n.config["bridge.external_interfaces"] != "" && n.config["ipv4.address"] == "none" && n.config["ipv6.address"] == "none" {
+ return false
+ }
+
+ return true
}
// fillConfig fills requested config with any default values.
@@ -102,7 +109,7 @@ func (n *bridge) fillConfig(config map[string]string) error {
// If no static hwaddr specified generate a volatile one to store in DB record so that
// there are no races when starting the network at the same time on multiple cluster nodes.
- _, err := n.fillHwaddr(config)
+ err := n.fillHwaddr(config)
if err != nil {
return err
}
@@ -414,7 +421,8 @@ func (n *bridge) setup(oldConfig map[string]string) error {
}
}
- // Create the bridge interface.
+ // Create the bridge interface if doesn't exist.
+ createdBridge := false
if !n.isRunning() {
if n.config["bridge.driver"] == "openvswitch" {
ovs := openvswitch.NewOVS()
@@ -432,6 +440,8 @@ func (n *bridge) setup(oldConfig map[string]string) error {
return err
}
}
+
+ createdBridge = true
}
// Get a list of tunnels.
@@ -505,13 +515,30 @@ func (n *bridge) setup(oldConfig map[string]string) error {
return err
}
- // If static or persistent volatile MAC address present, use that.
- // We do not generate missing persistent volatile MAC address at start time so as not to cause DB races
- // when starting an existing network without volatile key in a cluster. This also allows the previous
- // behavior for networks (i.e random MAC at start if not specified) until the network is next updated.
- hwAddr := n.getHwaddr(n.config)
+ // Always prefer static MAC address if set.
+ hwAddr := n.config["bridge.hwaddr"]
+
+ // If no static MAC address set, and it is safe to use the stable volatile address, then use that.
+ if hwAddr == "" && n.stableMACSafe() {
+ // We do not generate missing stable volatile MAC address at start time so as not to cause DB races
+ // when starting an existing network without volatile key in a cluster. This also allows the old
+ // behavior for networks (i.e random MAC at start) until the network is next updated.
+ hwAddr = n.config["volatile.bridge.hwaddr"]
+ }
+
+ // If MAC address is not set statically and no stable volatile MAC address available, then generate a
+ // temporary one to use on initial bridge setup. Do this explicitly rather than letting the bridge device
+ // generate one so that the MAC address stays stable when ports are connected to it.
+ if hwAddr == "" && createdBridge {
+ hwAddr, err = instance.DeviceNextInterfaceHWAddr()
+ if err != nil {
+ return errors.Wrapf(err, "Failed generating temporary MAC address")
+ }
+ n.logger.Info("Generated temporary MAC for bridge interface", log.Ctx{"hwaddr": hwAddr})
+ }
+
+ // Set the MAC address on the bridge interface if specified.
if hwAddr != "" {
- // Set the MAC address on the bridge interface.
_, err = shared.RunCommand("ip", "link", "set", "dev", n.name, "address", hwAddr)
if err != nil {
return err
More information about the lxc-devel
mailing list