The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7696
This e-mail was sent by the LXC bot, direct replies will not reach the author
unless they happen to be subscribed to this list.
=== Description (from pull-request) ===
- Adds check to avoid using stable volatile MAC addresses when using `bridge.external_interfaces` and no bridge IPs to avoid possible MAC conflicts in a clustered environent where multiple nodes are connected to same network segment.
- Fixes a bug that prevented updating/removing a network's node-specific keys when in non-clustered mode.
From 1781ca3fce05f8743356151dcf572af9bcb52c10 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@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.parr...@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.parr...@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.parr...@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
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
