Skip to content

Commit

Permalink
Merge pull request kubernetes#128763 from srivastav-abhishek/fix-err-…
Browse files Browse the repository at this point in the history 
…string

Fixed failing UT TestWriteKubeletConfigFiles by removing privilege check and adding proper error handling
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Nov 13, 2024
2 parents 5ee686b + 56e3c78 commit deecaf7
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 98 deletions.
6 changes: 5 additions & 1 deletion cmd/kubeadm/app/cmd/phases/upgrade/kubeletconfig.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
25 changes: 11 additions & 14 deletions cmd/kubeadm/app/phases/upgrade/postupgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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:
Expand Down
24 changes: 0 additions & 24 deletions cmd/kubeadm/app/phases/upgrade/postupgrade_others_test.go
View file

This file was deleted.

50 changes: 15 additions & 35 deletions cmd/kubeadm/app/phases/upgrade/postupgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"os"
"path/filepath"
"reflect"
"regexp"
"strings"
"testing"

Expand All @@ -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"
)

Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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)
}
}
}
Expand Down
24 changes: 0 additions & 24 deletions cmd/kubeadm/app/phases/upgrade/postupgrade_windows_test.go
View file

This file was deleted.

0 comments on commit deecaf7

Please sign in to comment.