Merge pull request #1767 from dcermak/cni-build-tag

Gate CNI support behind the `cni` build tag

.golangci.yml

@@ -7,6 +7,7 @@ run:
     - systemd
     - exclude_graphdriver_btrfs
     - containers_image_openpgp
+    - cni
   concurrency: 6
   deadline: 5m
 linters:

Makefile

@@ -36,15 +36,19 @@ build-cross:
 	$(call go-build,windows,386,${BUILDTAGS})
 
 .PHONY: all
-all: build-amd64 build-386
+all: build-amd64 build-386 build-amd64-cni
 
 .PHONY: build
-build: build-amd64 build-386
+build: build-amd64 build-386 build-amd64-cni
 
 .PHONY: build-amd64
 build-amd64:
 	GOARCH=amd64 $(GO_BUILD) -tags $(BUILDTAGS) ./...
 
+.PHONY: build-amd64-cni
+build-amd64-cni:
+	GOARCH=amd64 $(GO_BUILD) -tags $(BUILDTAGS),cni ./...
+
 .PHONY: build-386
 build-386:
 ifneq ($(shell uname -s), Darwin)
@@ -100,6 +104,7 @@ test: test-unit
 test-unit: netavark-testplugin
 	go test --tags seccomp,$(BUILDTAGS) -v ./...
 	go test --tags remote,$(BUILDTAGS) -v ./pkg/config
+	go test --tags cni,$(BUILDTAGS) -v ./libnetwork/cni
 
 .PHONY: codespell
 codespell:

libnetwork/cni/cni_conversion.go

@@ -1,4 +1,4 @@
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/cni_exec.go

@@ -16,7 +16,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/cni_suite_test.go

@@ -1,4 +1,4 @@
-//go:build linux
+//go:build (linux || freebsd) && cni
 
 package cni_test
 

libnetwork/cni/cni_types.go

@@ -1,4 +1,4 @@
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/config.go

@@ -1,4 +1,4 @@
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/config_freebsd.go

@@ -1,4 +1,4 @@
-//go:build freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/config_linux.go

@@ -1,4 +1,4 @@
-//go:build linux
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/config_test.go

@@ -1,4 +1,4 @@
-//go:build linux
+//go:build (linux || freebsd) && cni
 
 package cni_test
 

libnetwork/cni/network.go

@@ -1,4 +1,4 @@
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/run.go

@@ -1,4 +1,4 @@
-//go:build linux || freebsd
+//go:build (linux || freebsd) && cni
 
 package cni
 

libnetwork/cni/run_test.go

@@ -1,4 +1,4 @@
-//go:build linux
+//go:build (linux || freebsd) && cni
 
 package cni_test
 

libnetwork/network/interface.go

@@ -8,13 +8,10 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/containers/common/libnetwork/cni"
 	"github.com/containers/common/libnetwork/netavark"
 	"github.com/containers/common/libnetwork/types"
 	"github.com/containers/common/pkg/config"
-	"github.com/containers/common/pkg/machine"
 	"github.com/containers/storage"
-	"github.com/containers/storage/pkg/homedir"
 	"github.com/containers/storage/pkg/ioutils"
 	"github.com/containers/storage/pkg/unshare"
 	"github.com/sirupsen/logrus"
@@ -23,8 +20,6 @@ import (
 const (
 	// defaultNetworkBackendFileName is the file name for sentinel file to store the backend
 	defaultNetworkBackendFileName = "defaultNetworkBackend"
-	// cniConfigDirRootless is the directory in XDG_CONFIG_HOME for cni plugins
-	cniConfigDirRootless = "cni/net.d/"
 
 	// netavarkBinary is the name of the netavark binary
 	netavarkBinary = "netavark"
@@ -52,146 +47,94 @@ func NetworkBackend(store storage.Store, conf *config.Config, syslog bool) (type
 		}
 	}
 
-	switch backend {
-	case types.Netavark:
-		netavarkBin, err := conf.FindHelperBinary(netavarkBinary, false)
-		if err != nil {
-			return "", nil, err
-		}
+	return backendFromType(backend, store, conf, syslog)
+}
 
-		aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)
+func netavarkBackendFromConf(store storage.Store, conf *config.Config, syslog bool) (types.ContainerNetwork, error) {
+	netavarkBin, err := conf.FindHelperBinary(netavarkBinary, false)
+	if err != nil {
+		return nil, err
+	}
 
-		confDir := conf.Network.NetworkConfigDir
-		if confDir == "" {
-			confDir = getDefaultNetavarkConfigDir(store)
-		}
+	aardvarkBin, _ := conf.FindHelperBinary(aardvarkBinary, false)
 
-		// We cannot use the runroot for rootful since the network namespace is shared for all
-		// libpod instances they also have to share the same ipam db.
-		// For rootless we have our own network namespace per libpod instances,
-		// so this is not a problem there.
-		runDir := netavarkRunDir
-		if unshare.IsRootless() {
-			runDir = filepath.Join(store.RunRoot(), "networks")
-		}
+	confDir := conf.Network.NetworkConfigDir
+	if confDir == "" {
+		confDir = getDefaultNetavarkConfigDir(store)
+	}
 
-		netInt, err := netavark.NewNetworkInterface(&netavark.InitConfig{
-			Config:           conf,
-			NetworkConfigDir: confDir,
-			NetworkRunDir:    runDir,
-			NetavarkBinary:   netavarkBin,
-			AardvarkBinary:   aardvarkBin,
-			Syslog:           syslog,
-		})
-		return types.Netavark, netInt, err
-	case types.CNI:
-		netInt, err := getCniInterface(conf)
-		return types.CNI, netInt, err
-
-	default:
-		return "", nil, fmt.Errorf("unsupported network backend %q, check network_backend in containers.conf", backend)
+	// We cannot use the runroot for rootful since the network namespace is shared for all
+	// libpod instances they also have to share the same ipam db.
+	// For rootless we have our own network namespace per libpod instances,
+	// so this is not a problem there.
+	runDir := netavarkRunDir
+	if unshare.IsRootless() {
+		runDir = filepath.Join(store.RunRoot(), "networks")
 	}
+
+	netInt, err := netavark.NewNetworkInterface(&netavark.InitConfig{
+		Config:           conf,
+		NetworkConfigDir: confDir,
+		NetworkRunDir:    runDir,
+		NetavarkBinary:   netavarkBin,
+		AardvarkBinary:   aardvarkBin,
+		Syslog:           syslog,
+	})
+	return netInt, err
 }
 
 func defaultNetworkBackend(store storage.Store, conf *config.Config) (backend types.NetworkBackend, err error) {
-	// read defaultNetworkBackend file
+	err = nil
+
 	file := filepath.Join(store.GraphRoot(), defaultNetworkBackendFileName)
-	b, err := os.ReadFile(file)
-	if err == nil {
-		val := string(b)
-		if val == string(types.Netavark) {
-			return types.Netavark, nil
-		}
-		if val == string(types.CNI) {
-			return types.CNI, nil
-		}
-		return "", fmt.Errorf("unknown network backend value %q in %q", val, file)
-	}
-	// fail for all errors except ENOENT
-	if !errors.Is(err, os.ErrNotExist) {
-		return "", fmt.Errorf("could not read network backend value: %w", err)
-	}
 
-	// cache the network backend to make sure always the same one will be used
-	defer func() {
+	writeBackendToFile := func(backendT types.NetworkBackend) {
 		// only write when there is no error
 		if err == nil {
-			if err := ioutils.AtomicWriteFile(file, []byte(backend), 0o644); err != nil {
+			if err := ioutils.AtomicWriteFile(file, []byte(backendT), 0o644); err != nil {
 				logrus.Errorf("could not write network backend to file: %v", err)
 			}
 		}
-	}()
-
-	_, err = conf.FindHelperBinary("netavark", false)
-	if err != nil {
-		// if we cannot find netavark use CNI
-		return types.CNI, nil
-	}
-
-	// If there are any containers then return CNI
-	cons, err := store.Containers()
-	if err != nil {
-		return "", err
-	}
-	if len(cons) != 0 {
-		return types.CNI, nil
-	}
-
-	// If there are any non ReadOnly images then return CNI
-	imgs, err := store.Images()
-	if err != nil {
-		return "", err
-	}
-	for _, i := range imgs {
-		if !i.ReadOnly {
-			return types.CNI, nil
-		}
 	}
 
-	// If there are CNI Networks then return CNI
-	cniInterface, err := getCniInterface(conf)
+	// read defaultNetworkBackend file
+	b, err := os.ReadFile(file)
 	if err == nil {
-		nets, err := cniInterface.NetworkList()
-		// there is always a default network so check > 1
-		if err != nil && !errors.Is(err, os.ErrNotExist) {
-			return "", err
-		}
-
-		if len(nets) > 1 {
-			// we do not have a fresh system so use CNI
-			return types.CNI, nil
-		}
-	}
-	return types.Netavark, nil
-}
+		val := string(b)
 
-func getCniInterface(conf *config.Config) (types.ContainerNetwork, error) {
-	confDir := conf.Network.NetworkConfigDir
-	if confDir == "" {
-		var err error
-		confDir, err = getDefaultCNIConfigDir()
-		if err != nil {
-			return nil, err
+		// if the network backend has been already set previously,
+		// handle the values depending on whether CNI is supported and
+		// whether the network backend is explicitly configured
+		if val == string(types.Netavark) {
+			// netavark is always good
+			return types.Netavark, nil
+		} else if val == string(types.CNI) {
+			if cniSupported {
+				return types.CNI, nil
+			}
+			// the user has *not* configured a network
+			// backend explicitly but used CNI in the past
+			// => we upgrade them in this case to netavark only
+			writeBackendToFile(types.Netavark)
+			logrus.Info("Migrating network backend to netavark as no backend has been configured previously")
+			return types.Netavark, nil
 		}
+		return "", fmt.Errorf("unknown network backend value %q in %q", val, file)
 	}
-	return cni.NewCNINetworkInterface(&cni.InitConfig{
-		Config:       conf,
-		CNIConfigDir: confDir,
-		RunDir:       conf.Engine.TmpDir,
-		IsMachine:    machine.IsGvProxyBased(),
-	})
-}
 
-func getDefaultCNIConfigDir() (string, error) {
-	if !unshare.IsRootless() {
-		return cniConfigDir, nil
+	// fail for all errors except ENOENT
+	if !errors.Is(err, os.ErrNotExist) {
+		return "", fmt.Errorf("could not read network backend value: %w", err)
 	}
 
-	configHome, err := homedir.GetConfigHome()
+	backend, err = networkBackendFromStore(store, conf)
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(configHome, cniConfigDirRootless), nil
+	// cache the network backend to make sure always the same one will be used
+	writeBackendToFile(backend)
+
+	return backend, nil
 }
 
 // getDefaultNetavarkConfigDir return the netavark config dir. For rootful it will