The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/7400
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) ===
Warning contains the issue as to why the xtables driver we are using as "last resort" is not fully compatible and may still cause unexpected behaviour.
From 33b8ecfeb4c0c2369b6866be39366aadd72a3622 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Wed, 20 May 2020 16:58:33 +0100
Subject: [PATCH 1/4] lxd/firewall/firewall/interface: Change definition of
Compat() to return compat issue error
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/firewall/firewall_interface.go | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lxd/firewall/firewall_interface.go
b/lxd/firewall/firewall_interface.go
index 932c38ddb9..103b5fa11e 100644
--- a/lxd/firewall/firewall_interface.go
+++ b/lxd/firewall/firewall_interface.go
@@ -9,7 +9,7 @@ import (
// Firewall represents an LXD firewall.
type Firewall interface {
String() string
- Compat() (bool, bool)
+ Compat() (bool, error)
NetworkSetupForwardingPolicy(networkName string, ipVersion uint, allow
bool) error
NetworkSetupOutboundNAT(networkName string, subnet *net.IPNet, srcIP
net.IP, append bool) error
From b3f3013db74bbf2fba135874bf14d66c2fd8bf0b Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Wed, 20 May 2020 16:59:12 +0100
Subject: [PATCH 2/4] lxd/firewall/drivers/driver/nftables: Updates Compat() to
return compat issues as error
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/firewall/drivers/drivers_nftables.go | 26 ++++++++++--------------
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/lxd/firewall/drivers/drivers_nftables.go
b/lxd/firewall/drivers/drivers_nftables.go
index f4d295304e..5b99d3b292 100644
--- a/lxd/firewall/drivers/drivers_nftables.go
+++ b/lxd/firewall/drivers/drivers_nftables.go
@@ -15,7 +15,6 @@ import (
deviceConfig "github.com/lxc/lxd/lxd/device/config"
"github.com/lxc/lxd/lxd/project"
"github.com/lxc/lxd/shared"
- "github.com/lxc/lxd/shared/logger"
"github.com/lxc/lxd/shared/version"
)
@@ -38,60 +37,57 @@ func (d Nftables) String() string {
return "nftables"
}
-// Compat returns whether the host is compatible with this driver and whether
the driver backend is in use.
-func (d Nftables) Compat() (bool, bool) {
+// Compat returns whether the driver backend is in use, and any host
compatibility errors.
+func (d Nftables) Compat() (bool, error) {
// Get the kernel version.
uname, err := shared.Uname()
if err != nil {
- return false, false
+ return false, err
}
// We require a 5.x kernel to avoid weird conflicts with xtables.
if len(uname.Release) > 1 {
verInt, err := strconv.Atoi(uname.Release[0:1])
if err != nil {
- return false, false
+ return false, errors.Wrapf(err, "Failed parsing kernel
version")
}
if verInt < 5 {
- return false, false
+ return false, fmt.Errorf("Kernel version does not meet
minimum requirement of 5")
}
}
// Check if nftables nft command exists, if not use xtables.
_, err = exec.LookPath("nft")
if err != nil {
- return false, false
+ return false, fmt.Errorf("Backend command %q missing", "nft")
}
// Get nftables version.
nftVersion, err := d.hostVersion()
if err != nil {
- logger.Debugf("Firewall nftables failed detecting nft version:
%v", err)
- return false, false
+ return false, errors.Wrapf(err, "Failed detecting nft version")
}
// Check nft version meets minimum required.
minVer, _ := version.NewDottedVersion(nftablesMinVersion)
if nftVersion.Compare(minVer) < 0 {
- logger.Debugf("Firewall nftables detected nft version %q is too
low, need %q or above", nftVersion, nftablesMinVersion)
- return false, false
+ return false, fmt.Errorf("nft version %q is too low, need %q or
above", nftVersion, nftablesMinVersion)
}
// Check whether in use by parsing ruleset and looking for existing
rules.
ruleset, err := d.nftParseRuleset()
if err != nil {
- logger.Errorf("Firewall nftables unable to parse existing
ruleset: %v", err)
- return false, false
+ return false, errors.Wrapf(err, "Failed parsing nftables
existing ruleset")
}
for _, item := range ruleset {
if item.Type == "rule" {
- return true, true // At least one rule found indicates
in use.
+ return true, nil // At least one rule found indicates
in use.
}
}
- return true, false
+ return false, nil
}
// nftGenericItem represents some common fields amongst the different nftables
types.
From 964f6522faf25b8c35efdea4998f5bc2b18e99ec Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Wed, 20 May 2020 16:59:35 +0100
Subject: [PATCH 3/4] lxd/firewall/drivers/drivers/xtables: Updates Compat() to
return compat issues as error
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/firewall/drivers/drivers_xtables.go | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/lxd/firewall/drivers/drivers_xtables.go
b/lxd/firewall/drivers/drivers_xtables.go
index 836dfce32e..728161786e 100644
--- a/lxd/firewall/drivers/drivers_xtables.go
+++ b/lxd/firewall/drivers/drivers_xtables.go
@@ -25,8 +25,8 @@ func (d Xtables) String() string {
return "xtables"
}
-// Compat returns whether the host is compatible with this driver and whether
the driver backend is in use.
-func (d Xtables) Compat() (bool, bool) {
+// Compat returns whether the driver backend is in use, and any host
compatibility errors.
+func (d Xtables) Compat() (bool, error) {
// xtables commands can be powered by nftables, so check we are using
non-nft version first, otherwise
// we should be using the nftables driver instead.
cmds := []string{"iptables", "ip6tables", "ebtables"}
@@ -34,34 +34,32 @@ func (d Xtables) Compat() (bool, bool) {
// Check command exists.
_, err := exec.LookPath(cmd)
if err != nil {
- logger.Debugf("Firewall xtables backend command %q
missing", cmd)
- return false, false
+ return false, fmt.Errorf("Backend command %q missing",
cmd)
}
// Check whether it is an nftables shim.
if d.xtablesIsNftables(cmd) {
- logger.Debugf("Firewall xtables backend command %q is
an nftables shim", cmd)
- return false, false
+ return false, fmt.Errorf("Backend command %q is an
nftables shim", cmd)
}
}
// Check whether any of the backends are in use already.
if d.iptablesInUse("iptables") {
logger.Debug("Firewall xtables detected iptables is in use")
- return true, true
+ return true, nil
}
if d.iptablesInUse("ip6tables") {
logger.Debug("Firewall xtables detected ip6tables is in use")
- return true, true
+ return true, nil
}
if d.ebtablesInUse() {
logger.Debug("Firewall xtables detected ebtables is in use")
- return true, true
+ return true, nil
}
- return true, false
+ return false, nil
}
// xtablesIsNftables checks whether the specified xtables backend command is
actually an nftables shim.
From f6db49212b3bcd6f44a25c66d339d2453336529d Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parr...@canonical.com>
Date: Wed, 20 May 2020 17:01:07 +0100
Subject: [PATCH 4/4] lxd/firewall/firewall/load: Updates driver detection to
warn when falling back to non-compatible xtables
And why it is not compatible.
Signed-off-by: Thomas Parrott <thomas.parr...@canonical.com>
---
lxd/firewall/firewall_load.go | 42 ++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/lxd/firewall/firewall_load.go b/lxd/firewall/firewall_load.go
index 367a4ec0d2..b85a9eb273 100644
--- a/lxd/firewall/firewall_load.go
+++ b/lxd/firewall/firewall_load.go
@@ -2,6 +2,7 @@ package firewall
import (
"github.com/lxc/lxd/lxd/firewall/drivers"
+ "github.com/lxc/lxd/shared/logger"
)
// New returns an appropriate firewall implementation.
@@ -10,26 +11,35 @@ func New() Firewall {
nftables := drivers.Nftables{}
xtables := drivers.Xtables{}
- // If nftables is compatible and already in use, then we prefer to use
the nftables driver irrespective of
- // whether xtables is in use or not.
- nftablesCompat, nftablesInUse := nftables.Compat()
- if nftablesCompat && nftablesInUse {
+ nftablesInUse, nftablesCompatErr := nftables.Compat()
+ if nftablesCompatErr != nil {
+ logger.Debugf(`Firewall detected "nftables" incompatibility:
%v`, nftablesCompatErr)
+ } else if nftablesInUse {
+ // If nftables is compatible and already in use, then we prefer
to use the nftables driver
+ // irrespective of whether xtables is in use or not.
return nftables
- } else if !nftablesCompat {
- // Note: If nftables isn't compatible, we fallback to xtables
without considering whether xtables
- // is itself compatible. This continues the existing behaviour
of allowing LXD to start with
- // potentially an incomplete firewall backend, so that only
networks and instances using those
- // features will fail to start later.
- return xtables
}
- // If xtables is compatible and already in use, then we prefer to stick
with the xtables driver rather than
- // mix the use of firewall drivers on the system.
- xtablesCompat, xtablesInUse := xtables.Compat()
- if xtablesCompat && xtablesInUse {
+ xtablesInUse, xtablesCompatErr := xtables.Compat()
+ if xtablesCompatErr != nil {
+ logger.Debugf(`Firewall detected "xtables" incompatibility:
%v`, xtablesCompatErr)
+ } else if xtablesInUse {
+ // If xtables is compatible and already in use, then we prefer
to stick with the xtables driver
+ // rather than mix the use of firewall drivers on the system.
return xtables
}
- // Otherwise prefer nftables as default.
- return nftables
+ // If nftables is compatible, but not in use, and xtables is not
compatible or not in use, use nftables.
+ if nftablesCompatErr == nil {
+ return nftables
+ }
+
+ // If neither nftables nor xtables are compatible, we fallback to
xtables.
+ // This continues the existing behaviour of allowing LXD to start with
potentially an incomplete firewall
+ // backend, so that only networks and instances using those features
may fail to function properly.
+ // The most common scenario for this is when xtables is using nft shim
commands but the nft command itself
+ // is not installed. In this case LXD will use the xtables shim
commands but with the potential of problems
+ // due to differences between the original xtables commands and the
shim commands provided by nft.
+ logger.Warnf(`Firewall failed to detect any compatible driver, falling
back to "xtables" (but some features may not work as expected due to: %v)`,
xtablesCompatErr)
+ return xtables
}
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
