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

…Istio CNI (maistra#688)

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

Includes:

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

  * MAISTRA-2137 Make network namespace setup executable name configurable (maistra#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 (maistra#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 (maistra#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

@@ -165,6 +165,7 @@ func init() {
 	registerBooleanParameter(constants.UpdateCNIBinaries, true, "Whether to refresh existing binaries when installing CNI")
 	registerStringArrayParameter(constants.SkipCNIBinaries, []string{},
 		"Binaries that should not be installed. Currently Istio only installs one binary `istio-cni`")
+	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 ouptut to")
 	registerBooleanParameter(constants.AmbientEnabled, false, "Whether ambient controller is enabled")
@@ -253,6 +254,7 @@ func constructConfig() (*config.Config, error) {
 
 		CNIBinSourceDir:   constants.CNIBinDir,
 		CNIBinTargetDirs:  []string{constants.HostCNIBinDir, constants.SecondaryBinDir},
+		CNIBinariesPrefix: viper.GetString(constants.CNIBinariesPrefix),
 		UpdateCNIBinaries: viper.GetBool(constants.UpdateCNIBinaries),
 		SkipCNIBinaries:   viper.GetStringSlice(constants.SkipCNIBinaries),
 		MonitoringPort:    viper.GetInt(constants.MonitoringPort),

cni/pkg/config/config.go

@@ -68,6 +68,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
 	// Whether to override existing CNI binaries
 	UpdateCNIBinaries bool
 

cni/pkg/constants/constants.go

@@ -32,6 +32,7 @@ const (
 	SkipTLSVerify        = "skip-tls-verify"
 	SkipCNIBinaries      = "skip-cni-binaries"
 	UpdateCNIBinaries    = "update-cni-binaries"
+	CNIBinariesPrefix    = "cni-binaries-prefix"
 	MonitoringPort       = "monitoring-port"
 	LogUDSAddress        = "log-uds-address"
 	AmbientEnabled       = "ambient-enabled"

cni/pkg/install/binaries.go

@@ -22,8 +22,9 @@ import (
 	"istio.io/istio/pkg/util/sets"
 )
 
-func copyBinaries(srcDir string, targetDirs []string, updateBinaries bool, skipBinaries []string) error {
+func copyBinaries(srcDir string, targetDirs []string, updateBinaries bool, skipBinaries []string, binariesPrefix string) error {
 	skipBinariesSet := sets.New(skipBinaries...)
+
 	for _, targetDir := range targetDirs {
 		if err := file.IsDirWriteable(targetDir); err != nil {
 			installLog.Infof("Directory %s is not writable, skipping.", targetDir)
@@ -46,18 +47,19 @@ func copyBinaries(srcDir string, targetDirs []string, updateBinaries bool, skipB
 				continue
 			}
 
-			targetFilepath := filepath.Join(targetDir, filename)
+			targetFilename := binariesPrefix + filename
+			targetFilepath := filepath.Join(targetDir, targetFilename)
 			if _, err := os.Stat(targetFilepath); err == nil && !updateBinaries {
 				installLog.Infof("%s is already here and UPDATE_CNI_BINARIES isn't true, skipping", targetFilepath)
 				continue
 			}
 
 			srcFilepath := filepath.Join(srcDir, filename)
-			err := file.AtomicCopy(srcFilepath, targetDir, filename)
+			err := file.AtomicCopy(srcFilepath, targetDir, targetFilename)
 			if err != nil {
 				return err
 			}
-			installLog.Infof("Copied %s to %s.", filename, targetDir)
+			installLog.Infof("Copied %s to %s.", filename, targetFilepath)
 		}
 	}
 

cni/pkg/install/binaries_test.go

@@ -28,6 +28,7 @@ func TestCopyBinaries(t *testing.T) {
 		expectedFiles  map[string]string // {filename: contents. ...}
 		updateBinaries bool
 		skipBinaries   []string
+		prefix         string
 	}{
 		{
 			name:          "basic",
@@ -54,6 +55,12 @@ func TestCopyBinaries(t *testing.T) {
 			srcFiles:      map[string]string{"istio-cni": "cni111", "istio-iptables": "iptables111"},
 			expectedFiles: map[string]string{"istio-cni": "cni111"},
 		},
+		{
+			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 {
@@ -74,7 +81,7 @@ func TestCopyBinaries(t *testing.T) {
 				}
 			}
 
-			err := copyBinaries(srcDir, []string{targetDir}, c.updateBinaries, c.skipBinaries)
+			err := copyBinaries(srcDir, []string{targetDir}, c.updateBinaries, c.skipBinaries, c.prefix)
 			if err != nil {
 				t.Fatal(err)
 			}

cni/pkg/install/cniconfig.go

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

cni/pkg/install/install.go

@@ -53,7 +53,7 @@ func NewInstaller(cfg *config.InstallConfig, isReady *atomic.Value) *Installer {
 func (in *Installer) install(ctx context.Context) (err error) {
 	if err = copyBinaries(
 		in.cfg.CNIBinSourceDir, in.cfg.CNIBinTargetDirs,
-		in.cfg.UpdateCNIBinaries, in.cfg.SkipCNIBinaries); err != nil {
+		in.cfg.UpdateCNIBinaries, in.cfg.SkipCNIBinaries, in.cfg.CNIBinariesPrefix); err != nil {
 		cniInstalls.With(resultLabel.Value(resultCopyBinariesFailure)).Increment()
 		return
 	}
@@ -107,6 +107,8 @@ func (in *Installer) Run(ctx context.Context) (err 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 {
@@ -127,7 +129,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
 				}
@@ -156,7 +158,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
@@ -185,7 +187,7 @@ func readServiceAccountToken(saToken string) (string, error) {
 func (in *Installer) sleepCheckInstall(ctx context.Context) error {
 	// Create file watcher before checking for installation
 	// so that no file modifications are missed while and after checking
-	watcher, fileModified, errChan, err := util.CreateFileWatcher(append(in.cfg.CNIBinTargetDirs, in.cfg.MountedCNINetDir)...)
+	watcher, fileModified, errChan, err := util.CreateFileWatcher(in.cfg.CNIBinariesPrefix, append(in.cfg.CNIBinTargetDirs, in.cfg.MountedCNINetDir))
 	if err != nil {
 		return err
 	}
@@ -227,6 +229,7 @@ func checkInstall(cfg *config.InstallConfig, cniConfigFilepath string) error {
 	if !cfg.CNIEnableInstall {
 		return nil
 	}
+	istioCniExecutableName := cfg.CNIBinariesPrefix + "istio-cni"
 	defaultCNIConfigFilename, err := getDefaultCNINetwork(cfg.MountedCNINetDir)
 	if err != nil {
 		return err
@@ -261,7 +264,7 @@ func checkInstall(cfg *config.InstallConfig, cniConfigFilepath string) error {
 			if err != nil {
 				return fmt.Errorf("%s: %w", cniConfigFilepath, err)
 			}
-			if plugin["type"] == "istio-cni" {
+			if plugin["type"] == istioCniExecutableName {
 				return nil
 			}
 		}
@@ -274,7 +277,7 @@ func checkInstall(cfg *config.InstallConfig, cniConfigFilepath string) error {
 		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) {
 		chainedCNIPlugin  bool
 		skipInstall       bool
 		existingConfFiles map[string]string // {srcFilename: targetFilename, ...}
+		cniBinariesPrefix string
 	}{
 		{
 			name:              "preempted config",
@@ -95,6 +96,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 {
@@ -110,10 +117,11 @@ func TestCheckInstall(t *testing.T) {
 			}
 
 			cfg := &config.InstallConfig{
-				MountedCNINetDir: tempDir,
-				CNIConfName:      c.cniConfName,
-				ChainedCNIPlugin: c.chainedCNIPlugin,
-				CNIEnableInstall: !c.skipInstall,
+				MountedCNINetDir:  tempDir,
+				CNIConfName:       c.cniConfName,
+				ChainedCNIPlugin:  c.chainedCNIPlugin,
+				CNIEnableInstall:  !c.skipInstall,
+				CNIBinariesPrefix: c.cniBinariesPrefix,
 			}
 			err := checkInstall(cfg, filepath.Join(tempDir, c.cniConfigFilename))
 			if (c.expectedFailure && err == nil) || (!c.expectedFailure && err != nil) {
@@ -299,6 +307,7 @@ func TestCleanup(t *testing.T) {
 		configFilename         string
 		existingConfigFilename string
 		expectedConfigFilename string
+		cniBinariesPrefix      string
 	}{
 		{
 			name:                   "chained CNI plugin",
@@ -312,6 +321,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 {
@@ -327,7 +343,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)
 			}
 
@@ -338,9 +355,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

@@ -120,6 +120,11 @@ func createKubeconfigFile(cfg *config.InstallConfig, saToken string) (kubeconfig
 		return "", 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 "", err
+	}
+
 	kubeconfigFilepath = filepath.Join(cfg.MountedCNINetDir, cfg.KubeconfigFilename)
 	installLog.Infof("write kubeconfig file %s with: \n%+v", kubeconfigFilepath, kcbbToPrint.String())
 	if err = file.AtomicWrite(kubeconfigFilepath, kcbb.Bytes(), os.FileMode(cfg.KubeconfigMode)); err != nil {

cni/pkg/install/kubeconfig_test.go

@@ -45,6 +45,7 @@ func TestCreateKubeconfigFile(t *testing.T) {
 		saToken            string
 		kubeCAFilepath     string
 		skipTLSVerify      bool
+		cniNetDir          string
 	}{
 		{
 			name:            "k8s service host not set",
@@ -73,12 +74,25 @@ func TestCreateKubeconfigFile(t *testing.T) {
 			saToken:            saToken,
 			kubeCAFilepath:     kubeCAFilepath,
 		},
+		{
+			name:               "nonexistent net.d dir",
+			kubeconfigFilename: "istio-cni-kubeconfig",
+			kubeconfigMode:     constants.DefaultKubeconfigMode,
+			k8sServiceHost:     k8sServiceHost,
+			k8sServicePort:     k8sServicePort,
+			saToken:            saToken,
+			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/plugin/iptables_linux.go

@@ -39,6 +39,7 @@ func (ipt *iptables) Program(podName, netns string, rdrct *Redirect) error {
 	viper.Set(constants.NetworkNamespace, netns)
 	viper.Set(constants.EnvoyPort, rdrct.targetPort)
 	viper.Set(constants.ProxyUID, rdrct.noRedirectUID)
+	viper.Set(constants.ProxyGID, rdrct.noRedirectGID)
 	viper.Set(constants.InboundInterceptionMode, rdrct.redirectMode)
 	viper.Set(constants.ServiceCidr, rdrct.includeIPCidrs)
 	viper.Set(constants.LocalExcludePorts, rdrct.excludeInboundPorts)

cni/pkg/plugin/kubernetes.go

@@ -22,6 +22,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 
+	meshconfig "istio.io/api/mesh/v1alpha1"
 	"istio.io/istio/pkg/kube"
 	"istio.io/pkg/log"
 )
@@ -38,6 +39,9 @@ type PodInfo struct {
 	Labels            map[string]string
 	Annotations       map[string]string
 	ProxyEnvironments map[string]string
+	ProxyConfig       *meshconfig.ProxyConfig
+	ProxyUID          *int64
+	ProxyGID          *int64
 }
 
 // newK8sClient returns a Kubernetes client
@@ -84,6 +88,10 @@ func getK8sPodInfo(client *kubernetes.Clientset, podName, podNamespace string) (
 			for _, e := range container.Env {
 				pi.ProxyEnvironments[e.Name] = e.Value
 			}
+			if container.SecurityContext != nil {
+				pi.ProxyUID = container.SecurityContext.RunAsUser
+				pi.ProxyGID = container.SecurityContext.RunAsGroup
+			}
 			continue
 		}
 	}

cni/pkg/plugin/redirect.go

@@ -32,6 +32,7 @@ const (
 	defaultProxyStatusPort       = "15020"
 	defaultRedirectToPort        = "15001"
 	defaultNoRedirectUID         = "1337"
+	defaultNoRedirectGID         = "1337"
 	defaultRedirectMode          = redirectModeREDIRECT
 	defaultRedirectIPCidr        = "*"
 	defaultRedirectExcludeIPCidr = ""
@@ -77,6 +78,7 @@ type Redirect struct {
 	targetPort           string
 	redirectMode         string
 	noRedirectUID        string
+	noRedirectGID        string
 	includeIPCidrs       string
 	excludeIPCidrs       string
 	excludeInboundPorts  string
@@ -216,6 +218,13 @@ func NewRedirect(pi *PodInfo) (*Redirect, error) {
 			"redirectMode", isFound, valErr)
 	}
 	redir.noRedirectUID = defaultNoRedirectUID
+	if pi.ProxyUID != nil {
+		redir.noRedirectUID = fmt.Sprintf("%d", *pi.ProxyUID)
+	}
+	redir.noRedirectGID = defaultNoRedirectGID
+	if pi.ProxyGID != nil {
+		redir.noRedirectGID = fmt.Sprintf("%d", *pi.ProxyGID)
+	}
 	isFound, redir.includeIPCidrs, valErr = getAnnotationOrDefault("includeIPCidrs", pi.Annotations)
 	if valErr != nil {
 		return nil, fmt.Errorf("annotation value error for value %s; annotationFound = %t: %v",