From 3055757b09ab27d8b8e4d2017e98e90522b81750 Mon Sep 17 00:00:00 2001 From: Skyler Clark Date: Tue, 12 Sep 2023 14:33:23 -0400 Subject: [PATCH] Copy Azure setup files only on Azure AzureCloudNodeManagerPath was being copied to all platforms, causing errors. This change ensures AzureCloudNodeManagerPath is only copied on Azure. --- pkg/csr/csr.go | 2 +- pkg/nodeconfig/nodeconfig.go | 2 +- pkg/windows/windows.go | 52 +++++++++++++++++++++--------------- pkg/windows/windows_test.go | 36 +++++++++++++++++++++++++ 4 files changed, 68 insertions(+), 24 deletions(-) diff --git a/pkg/csr/csr.go b/pkg/csr/csr.go index a8ce4038b..754cd159b 100644 --- a/pkg/csr/csr.go +++ b/pkg/csr/csr.go @@ -317,7 +317,7 @@ func matchesHostname(nodeName string, windowsInstances []*instance.Info, // findHostName returns the actual host name of the instance by running the 'hostname' command func findHostName(instanceInfo *instance.Info, instanceSigner ssh.Signer) (string, error) { // We don't need to pass most args here as we just need to be able to run commands on the instance. - win, err := windows.New("", instanceInfo, instanceSigner) + win, err := windows.New("", instanceInfo, instanceSigner, nil) if err != nil { return "", fmt.Errorf("error instantiating Windows instance: %w", err) } diff --git a/pkg/nodeconfig/nodeconfig.go b/pkg/nodeconfig/nodeconfig.go index cca028695..d403eacdb 100644 --- a/pkg/nodeconfig/nodeconfig.go +++ b/pkg/nodeconfig/nodeconfig.go @@ -125,7 +125,7 @@ func NewNodeConfig(c client.Client, clientset *kubernetes.Clientset, clusterServ } log := ctrl.Log.WithName(fmt.Sprintf("nc %s", instanceInfo.Address)) - win, err := windows.New(clusterDNS, instanceInfo, signer) + win, err := windows.New(clusterDNS, instanceInfo, signer, &platformType) if err != nil { return nil, fmt.Errorf("error instantiating Windows instance from VM: %w", err) } diff --git a/pkg/windows/windows.go b/pkg/windows/windows.go index 7b3ed32c5..8326b0b09 100644 --- a/pkg/windows/windows.go +++ b/pkg/windows/windows.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/go-logr/logr" + config "github.com/openshift/api/config/v1" "golang.org/x/crypto/ssh" "k8s.io/apimachinery/pkg/util/wait" ctrl "sigs.k8s.io/controller-runtime" @@ -137,8 +138,6 @@ const ( ) var ( - // filesToTransfer is a map of what files should be copied to the Windows VM and where they should be copied to - filesToTransfer map[*payload.FileInfo]string // RequiredServices is a list of Windows services installed by WMCO. WICD owns all services aside from itself. // The order of this slice matters due to service dependencies. If a service depends on another service, the // dependent service should be placed before the service it depends on. @@ -167,11 +166,22 @@ var ( } ) -// getFilesToTransfer returns the properly populated filesToTransfer map. Note this does not include the WICD binary. -func getFilesToTransfer() (map[*payload.FileInfo]string, error) { - if filesToTransfer != nil { - return filesToTransfer, nil +// createPayload returns the map of files to transfer with generated file info +func createPayload(platform *config.PlatformType) (map[*payload.FileInfo]string, error) { + srcDestPairs := getFilesToTransfer(platform) + files := make(map[*payload.FileInfo]string) + for src, dest := range srcDestPairs { + f, err := payload.NewFileInfo(src) + if err != nil { + return nil, fmt.Errorf("could not create FileInfo object for file %s: %w", src, err) + } + files[f] = dest } + return files, nil +} + +// getFilesToTransfer returns the properly populated filesToTransfer map. Note this does not include the WICD binary. +func getFilesToTransfer(platform *config.PlatformType) map[string]string { srcDestPairs := map[string]string{ payload.GcpGetValidHostnameScriptPath: remoteDir, payload.WinDefenderExclusionScriptPath: remoteDir, @@ -183,7 +193,6 @@ func getFilesToTransfer() (map[*payload.FileInfo]string, error) { payload.WinOverlayCNIPlugin: cniDir, payload.KubeProxyPath: K8sDir, payload.KubeletPath: K8sDir, - payload.AzureCloudNodeManagerPath: K8sDir, payload.KubeLogRunnerPath: K8sDir, payload.CSIProxyPath: K8sDir, payload.ContainerdPath: ContainerdDir, @@ -191,16 +200,11 @@ func getFilesToTransfer() (map[*payload.FileInfo]string, error) { payload.ContainerdConfPath: ContainerdDir, payload.NetworkConfigurationScript: remoteDir, } - files := make(map[*payload.FileInfo]string) - for src, dest := range srcDestPairs { - f, err := payload.NewFileInfo(src) - if err != nil { - return nil, fmt.Errorf("could not create FileInfo object for file %s: %w", src, err) - } - files[f] = dest + + if platform != nil && *platform == config.AzurePlatformType { + srcDestPairs[payload.AzureCloudNodeManagerPath] = K8sDir } - filesToTransfer = files - return filesToTransfer, nil + return srcDestPairs } // GetK8sDir returns the location of the kubernetes executable directory @@ -257,10 +261,12 @@ type windows struct { log logr.Logger // defaultShellPowerShell indicates if the default SSH shell is PowerShell defaultShellPowerShell bool + // filesToTransfer is the map of files needed for the windows VM + filesToTransfer map[*payload.FileInfo]string } // New returns a new Windows instance constructed from the given WindowsVM -func New(clusterDNS string, instanceInfo *instance.Info, signer ssh.Signer) (Windows, error) { +func New(clusterDNS string, instanceInfo *instance.Info, signer ssh.Signer, platform *config.PlatformType) (Windows, error) { log := ctrl.Log.WithName(fmt.Sprintf("wc %s", instanceInfo.Address)) log.V(1).Info("initializing SSH connection") conn, err := newSshConnectivity(instanceInfo.Username, instanceInfo.Address, signer, log) @@ -268,12 +274,18 @@ func New(clusterDNS string, instanceInfo *instance.Info, signer ssh.Signer) (Win return nil, fmt.Errorf("unable to setup VM %s sshConnectivity: %w", instanceInfo.Address, err) } + files, err := createPayload(platform) + if err != nil { + return nil, fmt.Errorf("unable to create payload: %w", err) + } + return &windows{ interact: conn, clusterDNS: clusterDNS, instance: instanceInfo, log: log, defaultShellPowerShell: defaultShellPowershell(conn), + filesToTransfer: files, }, nil } @@ -652,11 +664,7 @@ func (vm *windows) removeDirectories() error { // transferFiles copies various files required for configuring the Windows node, to the VM. func (vm *windows) transferFiles() error { vm.log.Info("transferring files") - filesToTransfer, err := getFilesToTransfer() - if err != nil { - return fmt.Errorf("error getting list of files to transfer: %w", err) - } - for src, dest := range filesToTransfer { + for src, dest := range vm.filesToTransfer { if err := vm.EnsureFile(src, dest); err != nil { return fmt.Errorf("error copying %s to %s: %w", src.Path, dest, err) } diff --git a/pkg/windows/windows_test.go b/pkg/windows/windows_test.go index 461383500..f43b0743a 100644 --- a/pkg/windows/windows_test.go +++ b/pkg/windows/windows_test.go @@ -3,9 +3,45 @@ package windows import ( "testing" + config "github.com/openshift/api/config/v1" "github.com/stretchr/testify/assert" + + "github.com/openshift/windows-machine-config-operator/pkg/nodeconfig/payload" ) +func TestGetFilesToTransfer(t *testing.T) { + testCases := []struct { + name string + platform *config.PlatformType + }{ + { + name: "test AWS", + platform: func() *config.PlatformType { t := config.AWSPlatformType; return &t }(), + }, + { + name: "test Azure", + platform: func() *config.PlatformType { t := config.AzurePlatformType; return &t }(), + }, + { + name: "test Nil", + platform: nil, + }, + } + + for _, test := range testCases { + t.Run(test.name, func(t *testing.T) { + files := getFilesToTransfer(test.platform) + if test.platform != nil && *test.platform == config.AzurePlatformType { + file := files[payload.AzureCloudNodeManagerPath] + assert.Equal(t, K8sDir, file) + } else { + _, exists := files[payload.AzureCloudNodeManagerPath] + assert.False(t, exists) + } + }) + } +} + func TestSplitPath(t *testing.T) { testCases := []struct { name string