Merge pull request kubernetes#128763 from srivastav-abhishek/fix-err-string

Fixed failing UT TestWriteKubeletConfigFiles by removing privilege check and adding proper error handling

Author: k8s-ci-robot

Diff for: cmd/kubeadm/app/cmd/phases/upgrade/kubeletconfig.go

@@ -28,6 +28,10 @@ import (
 	"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
 )
 
+const (
+	kubeletConfigDir = ""
+)
+
 var (
 	kubeletConfigLongDesc = cmdutil.LongDesc(`
 		Upgrade the kubelet configuration for this node by downloading it from the kubelet-config ConfigMap stored in the cluster
@@ -60,7 +64,7 @@ func runKubeletConfigPhase(c workflow.RunData) error {
 	// Write the configuration for the kubelet down to disk and print the generated manifests instead of dry-running.
 	// If not dry-running, the kubelet config file will be backed up to the /etc/kubernetes/tmp/ dir, so that it could be
 	// recovered if anything goes wrong.
-	err := upgrade.WriteKubeletConfigFiles(data.InitCfg(), data.PatchesDir(), data.DryRun(), data.OutputWriter())
+	err := upgrade.WriteKubeletConfigFiles(data.InitCfg(), kubeletConfigDir, data.PatchesDir(), data.DryRun(), data.OutputWriter())
 	if err != nil {
 		return err
 	}

Diff for: cmd/kubeadm/app/phases/upgrade/postupgrade.go

@@ -96,16 +96,21 @@ func UnupgradedControlPlaneInstances(client clientset.Interface, nodeName string
 }
 
 // WriteKubeletConfigFiles writes the kubelet config file to disk, but first creates a backup of any existing one.
-func WriteKubeletConfigFiles(cfg *kubeadmapi.InitConfiguration, patchesDir string, dryRun bool, out io.Writer) error {
-	// Set up the kubelet directory to use. If dry-running, this will return a fake directory
-	kubeletDir, err := GetKubeletDir(dryRun)
+func WriteKubeletConfigFiles(cfg *kubeadmapi.InitConfiguration, kubeletConfigDir string, patchesDir string, dryRun bool, out io.Writer) error {
+	var (
+		err        error
+		kubeletDir = kubeadmconstants.KubeletRunDirectory
+	)
+	// If dry-running, this will return a directory under /etc/kubernetes/tmp or kubeletConfigDir.
+	if dryRun {
+		kubeletDir, err = kubeadmconstants.CreateTempDir(kubeletConfigDir, "kubeadm-upgrade-dryrun")
+	}
 	if err != nil {
 		// The error here should never occur in reality, would only be thrown if /tmp doesn't exist on the machine.
 		return err
 	}
-
-	// Create a copy of the kubelet config file in the /etc/kubernetes/tmp/ folder.
-	backupDir, err := kubeadmconstants.CreateTempDir("", "kubeadm-kubelet-config")
+	// Create a copy of the kubelet config file in the /etc/kubernetes/tmp or kubeletConfigDir.
+	backupDir, err := kubeadmconstants.CreateTempDir(kubeletConfigDir, "kubeadm-kubelet-config")
 	if err != nil {
 		return err
 	}
@@ -178,14 +183,6 @@ func WriteKubeletConfigFiles(cfg *kubeadmapi.InitConfiguration, patchesDir strin
 	return nil
 }
 
-// GetKubeletDir gets the kubelet directory based on whether the user is dry-running this command or not.
-func GetKubeletDir(dryRun bool) (string, error) {
-	if dryRun {
-		return kubeadmconstants.CreateTempDir("", "kubeadm-upgrade-dryrun")
-	}
-	return kubeadmconstants.KubeletRunDirectory, nil
-}
-
 // UpdateKubeletLocalMode changes the Server URL in the kubelets kubeconfig to the local API endpoint if it is currently
 // set to the ControlPlaneEndpoint.
 // TODO: remove this function once kubeletKubeConfigFilePath goes GA and is hardcoded to enabled by default:

Diff for: cmd/kubeadm/app/phases/upgrade/postupgrade_others_test.go

@@ -20,7 +20,6 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
-	"regexp"
 	"strings"
 	"testing"
 
@@ -35,7 +34,6 @@ import (
 	kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
 	"k8s.io/kubernetes/cmd/kubeadm/app/componentconfigs"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
-	"k8s.io/kubernetes/cmd/kubeadm/app/preflight"
 	testutil "k8s.io/kubernetes/cmd/kubeadm/test"
 )
 
@@ -114,26 +112,15 @@ func TestRollbackFiles(t *testing.T) {
 }
 
 func TestWriteKubeletConfigFiles(t *testing.T) {
-	// exit early if the user doesn't have root permission as the test needs to create /etc/kubernetes directory
-	// while the permission should be granted to the user.
-	isPrivileged := preflight.IsPrivilegedUserCheck{}
-	if _, err := isPrivileged.Check(); err != nil {
-		return
-	}
+	tempDir := t.TempDir()
 	testCases := []struct {
-		name       string
-		dryrun     bool
-		patchesDir string
-		errPattern string
-		cfg        *kubeadmapi.InitConfiguration
+		name          string
+		patchesDir    string
+		expectedError bool
+		cfg           *kubeadmapi.InitConfiguration
 	}{
-		// Be careful that if the dryrun is set to false and the test is run on a live cluster, the kubelet config file might be overwritten.
-		// However, you should be able to find the original config file in /etc/kubernetes/tmp/kubeadm-kubelet-configxxx folder.
-		// The test haven't clean up the temporary file created under /etc/kubernetes/tmp/ as that could be accidentally delete other files in
-		// that folder as well which might be unexpected.
 		{
-			name:   "write kubelet config file successfully",
-			dryrun: true,
+			name: "write kubelet config file successfully",
 			cfg: &kubeadmapi.InitConfiguration{
 				ClusterConfiguration: kubeadmapi.ClusterConfiguration{
 					ComponentConfigs: kubeadmapi.ComponentConfigMap{
@@ -143,16 +130,14 @@ func TestWriteKubeletConfigFiles(t *testing.T) {
 			},
 		},
 		{
-			name:       "aggregate errs: no kubelet config file and cannot read config file",
-			dryrun:     true,
-			errPattern: missingKubeletConfig,
-			cfg:        &kubeadmapi.InitConfiguration{},
+			name:          "aggregate errs: no kubelet config file and cannot read config file",
+			expectedError: true,
+			cfg:           &kubeadmapi.InitConfiguration{},
 		},
 		{
-			name:       "only one err: patch dir does not exist",
-			dryrun:     true,
-			patchesDir: "Bogus",
-			errPattern: "could not list patch files for path \"Bogus\"",
+			name:          "only one err: patch dir does not exist",
+			patchesDir:    "Bogus",
+			expectedError: true,
 			cfg: &kubeadmapi.InitConfiguration{
 				ClusterConfiguration: kubeadmapi.ClusterConfiguration{
 					ComponentConfigs: kubeadmapi.ComponentConfigMap{
@@ -163,14 +148,9 @@ func TestWriteKubeletConfigFiles(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		err := WriteKubeletConfigFiles(tc.cfg, tc.patchesDir, tc.dryrun, os.Stdout)
-		if err != nil && tc.errPattern != "" {
-			if match, _ := regexp.MatchString(tc.errPattern, err.Error()); !match {
-				t.Fatalf("Expected error contains %q, got %v", tc.errPattern, err.Error())
-			}
-		}
-		if err == nil && len(tc.errPattern) != 0 {
-			t.Fatalf("WriteKubeletConfigFiles didn't return error expected %s", tc.errPattern)
+		err := WriteKubeletConfigFiles(tc.cfg, tempDir, tc.patchesDir, true, os.Stdout)
+		if (err != nil) != tc.expectedError {
+			t.Fatalf("expected error: %v, got: %v, error: %v", tc.expectedError, err != nil, err)
 		}
 	}
 }