[cni] MAISTRA-2291 Support deployment of multiple plugin versions in …

…Istio CNI (#688)

* [cni] MAISTRA-2132 Support deployment of multiple plugin versions in Istio CNI (#271)

Includes:

  * MAISTRA-2135 Add unit tests for our CNI binary-prefix work (#325)

  * MAISTRA-2137 Make network namespace setup executable name configurable (#273)

    To support the deployment of multiple CNI plugin versions, the name of the
    executable that is invoked to set up the network namespace must be configurable.

  * OSSM-1430: CNI: Watch for modified files with a prefix (#510)

    Because our CNI pod contains more than one container, and they write to
    the same directory, and they watch for changes on those directories,
    changes made by one container trigger the watch on the other, which will
    responde by copying the files to the directory, which will in turn
    trigger the watcher of the other container in an endless loop.

    This leads to high CPU usage on the node.

    This PR changes the logic to only monitor for files that have the
    desired prefix. Thus, for example, the 2.2 container will only react to
    changes to files whose names  start with "v2-2". This avoid this race
    condition and achieve the same end result.

* [cni] MAISTRA-2051 use correct UID/GID in istio-iptables

* OSSM-2082 CNI installer now creates the net.d directory if necessary (#638)

* fix(lint): replaces deprecated pkg io/ioutil

* fix: reverts back to t.TempDir() calls

Co-authored-by: Marko Lukša <marko.luksa@gmail.com>
Co-authored-by: rcernich <rcernich@redhat.com>

cni/pkg/cmd/root.go

@@ -164,6 +164,7 @@ func init() {
 	registerIntegerParameter(constants.KubeconfigMode, constants.DefaultKubeconfigMode, "File mode of the kubeconfig file")
 	registerStringParameter(constants.KubeCAFile, "", "CA file for kubeconfig. Defaults to the same as install-cni pod")
 	registerBooleanParameter(constants.SkipTLSVerify, false, "Whether to use insecure TLS in kubeconfig file")
+	registerStringParameter(constants.CNIBinariesPrefix, "", "The filename prefix to add to each binary when copying")
 	registerIntegerParameter(constants.MonitoringPort, 15014, "HTTP port to serve prometheus metrics")
 	registerStringParameter(constants.LogUDSAddress, "/var/run/istio-cni/log.sock", "The UDS server address which CNI plugin will copy log output to")
 	registerBooleanParameter(constants.AmbientEnabled, false, "Whether ambient controller is enabled")
@@ -242,10 +243,11 @@ func constructConfig() (*config.Config, error) {
 		K8sServicePort:     os.Getenv("KUBERNETES_SERVICE_PORT"),
 		K8sNodeName:        os.Getenv("KUBERNETES_NODE_NAME"),
 
-		CNIBinSourceDir:  constants.CNIBinDir,
-		CNIBinTargetDirs: []string{constants.HostCNIBinDir, constants.SecondaryBinDir},
-		MonitoringPort:   viper.GetInt(constants.MonitoringPort),
-		LogUDSAddress:    viper.GetString(constants.LogUDSAddress),
+		CNIBinSourceDir:   constants.CNIBinDir,
+		CNIBinTargetDirs:  []string{constants.HostCNIBinDir, constants.SecondaryBinDir},
+		CNIBinariesPrefix: viper.GetString(constants.CNIBinariesPrefix),
+		MonitoringPort:    viper.GetInt(constants.MonitoringPort),
+		LogUDSAddress:     viper.GetString(constants.LogUDSAddress),
 
 		AmbientEnabled: viper.GetBool(constants.AmbientEnabled),
 		EbpfEnabled:    viper.GetBool(constants.EbpfEnabled),

cni/pkg/config/config.go

@@ -64,6 +64,8 @@ type InstallConfig struct {
 	CNIBinSourceDir string
 	// Directories into which to copy the CNI binaries
 	CNIBinTargetDirs []string
+	// The prefix to add to the name of each CNI binary
+	CNIBinariesPrefix string
 
 	// The HTTP port for monitoring
 	MonitoringPort int

cni/pkg/constants/constants.go

@@ -28,6 +28,7 @@ const (
 	KubeconfigMode       = "kubeconfig-mode"
 	KubeCAFile           = "kube-ca-file"
 	SkipTLSVerify        = "skip-tls-verify"
+	CNIBinariesPrefix    = "cni-binaries-prefix"
 	MonitoringPort       = "monitoring-port"
 	LogUDSAddress        = "log-uds-address"
 	AmbientEnabled       = "ambient-enabled"

cni/pkg/install/binaries.go

@@ -24,7 +24,7 @@ import (
 
 // Copies/mirrors any files present in a single source dir to N number of target dirs
 // and returns a set of the filenames copied.
-func copyBinaries(srcDir string, targetDirs []string) (sets.Set[string], error) {
+func copyBinaries(srcDir string, targetDirs []string, binariesPrefix string) (sets.Set[string], error) {
 	copiedFilenames := sets.Set[string]{}
 	srcFiles, err := os.ReadDir(srcDir)
 	if err != nil {
@@ -37,22 +37,24 @@ func copyBinaries(srcDir string, targetDirs []string) (sets.Set[string], error)
 		}
 
 		filename := f.Name()
+		targetFilename := binariesPrefix + filename
 		srcFilepath := filepath.Join(srcDir, filename)
 
 		for _, targetDir := range targetDirs {
 			if err := file.IsDirWriteable(targetDir); err != nil {
 				installLog.Infof("Directory %s is not writable, skipping.", targetDir)
 				continue
 			}
+			targetFilepath := filepath.Join(targetDir, targetFilename)
 
-			err := file.AtomicCopy(srcFilepath, targetDir, filename)
+			err := file.AtomicCopy(srcFilepath, targetDir, targetFilename)
 			if err != nil {
 				return copiedFilenames, err
 			}
-			installLog.Infof("Copied %s to %s.", filename, targetDir)
+			installLog.Infof("Copied %s to %s.", filename, targetFilepath)
 		}
 
-		copiedFilenames.Insert(filename)
+		copiedFilenames.Insert(targetFilename)
 	}
 
 	return copiedFilenames, nil

cni/pkg/install/binaries_test.go

@@ -28,6 +28,7 @@ func TestCopyBinaries(t *testing.T) {
 		srcFiles      map[string]string
 		existingFiles map[string]string
 		expectedFiles map[string]string
+		prefix        string
 	}{
 		{
 			name:          "basic",
@@ -40,6 +41,12 @@ func TestCopyBinaries(t *testing.T) {
 			existingFiles: map[string]string{"istio-cni": "cni000", "istio-iptables": "iptables111"},
 			expectedFiles: map[string]string{"istio-cni": "cni111", "istio-iptables": "iptables111"},
 		},
+		{
+			name:          "binaries prefix",
+			prefix:        "prefix-",
+			srcFiles:      map[string]string{"istio-cni": "cni111", "istio-iptables": "iptables111"},
+			expectedFiles: map[string]string{"prefix-istio-cni": "cni111", "prefix-istio-iptables": "iptables111"},
+		},
 	}
 
 	for _, c := range cases {
@@ -54,7 +61,7 @@ func TestCopyBinaries(t *testing.T) {
 				file.WriteOrFail(t, filepath.Join(targetDir, filename), []byte(contents))
 			}
 
-			binariesCopied, err := copyBinaries(srcDir, []string{targetDir})
+			binariesCopied, err := copyBinaries(srcDir, []string{targetDir}, c.prefix)
 			if err != nil {
 				t.Fatal(err)
 			}

cni/pkg/install/cniconfig.go

@@ -176,7 +176,7 @@ func getCNIConfigFilepath(ctx context.Context, cfg pluginConfig) (string, error)
 		return filepath.Join(cfg.mountedCNINetDir, filename), nil
 	}
 
-	watcher, err := util.CreateFileWatcher(cfg.mountedCNINetDir)
+	watcher, err := util.CreateFileWatcher("", cfg.mountedCNINetDir)
 	if err != nil {
 		return "", err
 	}

cni/pkg/install/install.go

@@ -51,7 +51,7 @@ func (in *Installer) installAll(ctx context.Context) (sets.Set[string], error) {
 	// Install binaries
 	// Currently we _always_ do this, since the binaries do not live in a shared location
 	// and we harm no one by doing so.
-	copiedFiles, err := copyBinaries(in.cfg.CNIBinSourceDir, in.cfg.CNIBinTargetDirs)
+	copiedFiles, err := copyBinaries(in.cfg.CNIBinSourceDir, in.cfg.CNIBinTargetDirs, in.cfg.CNIBinariesPrefix)
 	if err != nil {
 		cniInstalls.With(resultLabel.Value(resultCopyBinariesFailure)).Increment()
 		return copiedFiles, fmt.Errorf("copy binaries: %v", err)
@@ -117,6 +117,8 @@ func (in *Installer) Run(ctx context.Context) error {
 
 // Cleanup remove Istio CNI's config, kubeconfig file, and binaries.
 func (in *Installer) Cleanup() error {
+	istioCniExecutableName := in.cfg.CNIBinariesPrefix + "istio-cni"
+
 	installLog.Info("Cleaning up.")
 	if len(in.cniConfigFilepath) > 0 && file.Exists(in.cniConfigFilepath) {
 		if in.cfg.ChainedCNIPlugin {
@@ -137,7 +139,7 @@ func (in *Installer) Cleanup() error {
 				if err != nil {
 					return fmt.Errorf("%s: %w", in.cniConfigFilepath, err)
 				}
-				if plugin["type"] == "istio-cni" {
+				if plugin["type"] == istioCniExecutableName {
 					cniConfigMap["plugins"] = append(plugins[:i], plugins[i+1:]...)
 					break
 				}
@@ -166,7 +168,7 @@ func (in *Installer) Cleanup() error {
 	}
 
 	for _, targetDir := range in.cfg.CNIBinTargetDirs {
-		if istioCNIBin := filepath.Join(targetDir, "istio-cni"); file.Exists(istioCNIBin) {
+		if istioCNIBin := filepath.Join(targetDir, istioCniExecutableName); file.Exists(istioCNIBin) {
 			installLog.Infof("Removing binary: %s", istioCNIBin)
 			if err := os.Remove(istioCNIBin); err != nil {
 				return err
@@ -201,7 +203,7 @@ func (in *Installer) sleepWatchInstall(ctx context.Context, installedBinFiles se
 	//
 	// Additionally, fsnotify will lose existing watches on atomic copies (due to overwrite/rename),
 	// so we have to re-watch after re-copy to make sure we always have fresh watches.
-	watcher, err := util.CreateFileWatcher(targets...)
+	watcher, err := util.CreateFileWatcher(in.cfg.CNIBinariesPrefix, targets...)
 	if err != nil {
 		return err
 	}
@@ -240,6 +242,7 @@ func (in *Installer) sleepWatchInstall(ctx context.Context, installedBinFiles se
 
 // checkValidCNIConfig returns an error if an invalid CNI configuration is detected
 func checkValidCNIConfig(cfg *config.InstallConfig, cniConfigFilepath string) error {
+	istioCniExecutableName := cfg.CNIBinariesPrefix + "istio-cni"
 	defaultCNIConfigFilename, err := getDefaultCNINetwork(cfg.MountedCNINetDir)
 	if err != nil {
 		return err
@@ -274,7 +277,7 @@ func checkValidCNIConfig(cfg *config.InstallConfig, cniConfigFilepath string) er
 			if err != nil {
 				return fmt.Errorf("%s: %w", cniConfigFilepath, err)
 			}
-			if plugin["type"] == "istio-cni" {
+			if plugin["type"] == istioCniExecutableName {
 				return nil
 			}
 		}
@@ -287,7 +290,7 @@ func checkValidCNIConfig(cfg *config.InstallConfig, cniConfigFilepath string) er
 		return err
 	}
 
-	if cniConfigMap["type"] != "istio-cni" {
+	if cniConfigMap["type"] != istioCniExecutableName {
 		return fmt.Errorf("istio-cni CNI config file modified: %s", cniConfigFilepath)
 	}
 	return nil

cni/pkg/install/install_test.go

@@ -37,6 +37,7 @@ func TestCheckInstall(t *testing.T) {
 		cniConfName       string
 		chainedCNIPlugin  bool
 		existingConfFiles map[string]string // {srcFilename: targetFilename, ...}
+		cniBinariesPrefix string
 	}{
 		{
 			name:              "preempted config",
@@ -89,6 +90,12 @@ func TestCheckInstall(t *testing.T) {
 			cniConfigFilename: "istio-cni.conf",
 			existingConfFiles: map[string]string{"istio-cni.conf": "istio-cni.conf"},
 		},
+		{
+			name:              "custom binaries prefix",
+			cniConfigFilename: "istio-cni.conf",
+			cniBinariesPrefix: "prefix-",
+			existingConfFiles: map[string]string{"istio-cni-prefixed.conf": "istio-cni.conf"},
+		},
 	}
 
 	for _, c := range cases {
@@ -104,9 +111,10 @@ func TestCheckInstall(t *testing.T) {
 			}
 
 			cfg := &config.InstallConfig{
-				MountedCNINetDir: tempDir,
-				CNIConfName:      c.cniConfName,
-				ChainedCNIPlugin: c.chainedCNIPlugin,
+				MountedCNINetDir:  tempDir,
+				CNIConfName:       c.cniConfName,
+				ChainedCNIPlugin:  c.chainedCNIPlugin,
+				CNIBinariesPrefix: c.cniBinariesPrefix,
 			}
 			err := checkValidCNIConfig(cfg, filepath.Join(tempDir, c.cniConfigFilename))
 			if (c.expectedFailure && err == nil) || (!c.expectedFailure && err != nil) {
@@ -290,6 +298,7 @@ func TestCleanup(t *testing.T) {
 		configFilename         string
 		existingConfigFilename string
 		expectedConfigFilename string
+		cniBinariesPrefix      string
 	}{
 		{
 			name:                   "chained CNI plugin",
@@ -303,6 +312,13 @@ func TestCleanup(t *testing.T) {
 			configFilename:         "istio-cni.conf",
 			existingConfigFilename: "istio-cni.conf",
 		},
+		{
+			name:                   "prefix",
+			cniBinariesPrefix:      "prefix-",
+			configFilename:         "list-cni-prefixed.conf",
+			existingConfigFilename: "list-with-istio.conflist",
+			expectedConfigFilename: "list-no-istio.conflist",
+		},
 	}
 
 	for _, c := range cases {
@@ -318,7 +334,8 @@ func TestCleanup(t *testing.T) {
 			}
 
 			// Create existing binary files
-			if err := os.WriteFile(filepath.Join(cniBinDir, "istio-cni"), []byte{1, 2, 3}, 0o755); err != nil {
+			filename := c.cniBinariesPrefix + "istio-cni"
+			if err := os.WriteFile(filepath.Join(cniBinDir, filename), []byte{1, 2, 3}, 0o755); err != nil {
 				t.Fatal(err)
 			}
 
@@ -329,9 +346,10 @@ func TestCleanup(t *testing.T) {
 			}
 
 			cfg := &config.InstallConfig{
-				MountedCNINetDir: cniNetDir,
-				ChainedCNIPlugin: c.chainedCNIPlugin,
-				CNIBinTargetDirs: []string{cniBinDir},
+				MountedCNINetDir:  cniNetDir,
+				ChainedCNIPlugin:  c.chainedCNIPlugin,
+				CNIBinariesPrefix: c.cniBinariesPrefix,
+				CNIBinTargetDirs:  []string{cniBinDir},
 			}
 
 			isReady := &atomic.Value{}

cni/pkg/install/kubeconfig.go

@@ -119,6 +119,11 @@ func createKubeConfig(cfg *config.InstallConfig) (kubeconfig, error) {
 		return kubeconfig{}, err
 	}
 
+	// When using Multus, the net.d dir might not exist yet, so we must create it
+	if err := os.MkdirAll(cfg.MountedCNINetDir, os.FileMode(0o755)); err != nil {
+		return kubeconfig{}, err
+	}
+
 	return kubeconfig{
 		Full:     string(fullYaml),
 		Redacted: string(redacted),

cni/pkg/install/kubeconfig_test.go

@@ -43,6 +43,7 @@ func TestCreateValidKubeconfigFile(t *testing.T) {
 		k8sServicePort     string
 		kubeCAFilepath     string
 		skipTLSVerify      bool
+		cniNetDir          string
 	}{
 		{
 			name:            "k8s service host not set",
@@ -65,12 +66,22 @@ func TestCreateValidKubeconfigFile(t *testing.T) {
 			k8sServicePort: k8sServicePort,
 			kubeCAFilepath: kubeCAFilepath,
 		},
+		{
+			name:           "nonexistent net.d dir",
+			k8sServiceHost: k8sServiceHost,
+			k8sServicePort: k8sServicePort,
+			skipTLSVerify:  true,
+			cniNetDir:      filepath.Join(t.TempDir(), "nonexistent-dir"),
+		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			// Create temp directory for files
-			tempDir := t.TempDir()
+			tempDir := c.cniNetDir
+			// Create temp directory for files if not provided in test case
+			if tempDir == "" {
+				tempDir = t.TempDir()
+			}
 
 			cfg := &config.InstallConfig{
 				MountedCNINetDir:   tempDir,

cni/pkg/util/util.go

@@ -19,6 +19,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
+	"strings"
 
 	"github.com/fsnotify/fsnotify"
 
@@ -48,15 +50,22 @@ func (w *Watcher) Close() {
 	_ = w.watcher.Close()
 }
 
+type watchPrefix struct {
+	watcher *fsnotify.Watcher
+	prefix  string
+}
+
 // Creates a file watcher that watches for any changes to the directory
-func CreateFileWatcher(paths ...string) (*Watcher, error) {
+// If `prefix` is non-empty, it will only notify of changes for files whose filename starts with `prefix`
+func CreateFileWatcher(prefix string, paths ...string) (*Watcher, error) {
 	watcher, err := fsnotify.NewWatcher()
 	if err != nil {
 		return nil, fmt.Errorf("watcher create: %v", err)
 	}
 
 	fileModified, errChan := make(chan struct{}), make(chan error)
-	go watchFiles(watcher, fileModified, errChan)
+	wp := &watchPrefix{watcher: watcher, prefix: prefix}
+	go watchFiles(wp, fileModified, errChan)
 
 	for _, path := range paths {
 		if !file.Exists(path) {
@@ -78,18 +87,18 @@ func CreateFileWatcher(paths ...string) (*Watcher, error) {
 	}, nil
 }
 
-func watchFiles(watcher *fsnotify.Watcher, fileModified chan struct{}, errChan chan error) {
+func watchFiles(wp *watchPrefix, fileModified chan struct{}, errChan chan error) {
 	for {
 		select {
-		case event, ok := <-watcher.Events:
+		case event, ok := <-wp.watcher.Events:
 			if !ok {
 				return
 			}
-			if event.Op&(fsnotify.Create|fsnotify.Write|fsnotify.Remove) != 0 {
+			if event.Op&(fsnotify.Create|fsnotify.Write|fsnotify.Remove) != 0 && strings.HasPrefix(filepath.Base(event.Name), wp.prefix) {
 				log.Infof("file modified: %v", event.Name)
 				fileModified <- struct{}{}
 			}
-		case err, ok := <-watcher.Errors:
+		case err, ok := <-wp.watcher.Errors:
 			if !ok {
 				return
 			}