From 8a091f47763185fff012202df51762ffc9ab1cf2 Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Thu, 24 Nov 2022 09:47:46 +0100 Subject: [PATCH] [cni] MAISTRA-2291 Support deployment of multiple plugin versions in Istio CNI (#688) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 Co-authored-by: rcernich --- cni/pkg/cmd/root.go | 2 ++ cni/pkg/config/config.go | 2 ++ cni/pkg/constants/constants.go | 1 + cni/pkg/install/binaries.go | 10 +++++---- cni/pkg/install/binaries_test.go | 9 +++++++- cni/pkg/install/cniconfig.go | 2 +- cni/pkg/install/install.go | 15 +++++++------ cni/pkg/install/install_test.go | 34 +++++++++++++++++++++++------- cni/pkg/install/kubeconfig.go | 5 +++++ cni/pkg/install/kubeconfig_test.go | 18 ++++++++++++++-- cni/pkg/plugin/iptables_linux.go | 1 + cni/pkg/plugin/kubernetes.go | 8 +++++++ cni/pkg/plugin/redirect.go | 9 ++++++++ cni/pkg/util/util.go | 21 ++++++++++++------ 14 files changed, 109 insertions(+), 28 deletions(-) diff --git a/cni/pkg/cmd/root.go b/cni/pkg/cmd/root.go index 0882841332d..df36b4af9df 100644 --- a/cni/pkg/cmd/root.go +++ b/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), diff --git a/cni/pkg/config/config.go b/cni/pkg/config/config.go index 8b8494d869e..a8f6a2f31d3 100644 --- a/cni/pkg/config/config.go +++ b/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 diff --git a/cni/pkg/constants/constants.go b/cni/pkg/constants/constants.go index a77bfc92070..4307f0675e5 100644 --- a/cni/pkg/constants/constants.go +++ b/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" diff --git a/cni/pkg/install/binaries.go b/cni/pkg/install/binaries.go index f99a9153876..c3e09a55802 100644 --- a/cni/pkg/install/binaries.go +++ b/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) } } diff --git a/cni/pkg/install/binaries_test.go b/cni/pkg/install/binaries_test.go index 638a36813d0..3423cf59a4a 100644 --- a/cni/pkg/install/binaries_test.go +++ b/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) } diff --git a/cni/pkg/install/cniconfig.go b/cni/pkg/install/cniconfig.go index 302bbf4c344..3f46c9a30e7 100644 --- a/cni/pkg/install/cniconfig.go +++ b/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 } diff --git a/cni/pkg/install/install.go b/cni/pkg/install/install.go index f55f7f63e6f..5dd041dfc70 100644 --- a/cni/pkg/install/install.go +++ b/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 diff --git a/cni/pkg/install/install_test.go b/cni/pkg/install/install_test.go index 4a28d214cd8..3a55de20220 100644 --- a/cni/pkg/install/install_test.go +++ b/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{} diff --git a/cni/pkg/install/kubeconfig.go b/cni/pkg/install/kubeconfig.go index 6f45e8dead5..c469e388526 100644 --- a/cni/pkg/install/kubeconfig.go +++ b/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 { diff --git a/cni/pkg/install/kubeconfig_test.go b/cni/pkg/install/kubeconfig_test.go index a4d83280fd6..efb57da21a6 100644 --- a/cni/pkg/install/kubeconfig_test.go +++ b/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, diff --git a/cni/pkg/plugin/iptables_linux.go b/cni/pkg/plugin/iptables_linux.go index c8a0b4c10c2..ae9fe0e991f 100644 --- a/cni/pkg/plugin/iptables_linux.go +++ b/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) diff --git a/cni/pkg/plugin/kubernetes.go b/cni/pkg/plugin/kubernetes.go index fdaa1f7e6b5..c57f60aaa95 100644 --- a/cni/pkg/plugin/kubernetes.go +++ b/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 } } diff --git a/cni/pkg/plugin/redirect.go b/cni/pkg/plugin/redirect.go index e99d86f6bb3..963175011c5 100644 --- a/cni/pkg/plugin/redirect.go +++ b/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", diff --git a/cni/pkg/util/util.go b/cni/pkg/util/util.go index ce76d84f13d..05e9a835fe1 100644 --- a/cni/pkg/util/util.go +++ b/cni/pkg/util/util.go @@ -19,21 +19,30 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" + "strings" "github.com/fsnotify/fsnotify" "istio.io/istio/pkg/file" ) +type watchPrefix struct { + watcher *fsnotify.Watcher + prefix string +} + // Creates a file watcher that watches for any changes to the directory -func CreateFileWatcher(dirs ...string) (watcher *fsnotify.Watcher, fileModified chan bool, errChan chan error, err error) { +// If `prefix` is non-empty, it will only notify of changes for files whose filename starts with `prefix` +func CreateFileWatcher(prefix string, dirs []string) (watcher *fsnotify.Watcher, fileModified chan bool, errChan chan error, err error) { watcher, err = fsnotify.NewWatcher() if err != nil { return } fileModified, errChan = make(chan bool), make(chan error) - go watchFiles(watcher, fileModified, errChan) + wp := &watchPrefix{watcher: watcher, prefix: prefix} + go watchFiles(wp, fileModified, errChan) for _, dir := range dirs { if file.IsDirWriteable(dir) != nil { @@ -50,17 +59,17 @@ func CreateFileWatcher(dirs ...string) (watcher *fsnotify.Watcher, fileModified return } -func watchFiles(watcher *fsnotify.Watcher, fileModified chan bool, errChan chan error) { +func watchFiles(wp *watchPrefix, fileModified chan bool, 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) { fileModified <- true } - case err, ok := <-watcher.Errors: + case err, ok := <-wp.watcher.Errors: if !ok { return }