kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows

[hwdef]

fix absolute paths do not work properly in config files in windows

What type of PR is this?

/kind bug

What this PR does / why we need it:

Because linux and windows express the absolute path differently, some errors may be caused.

This pr fixes this bug, the fix method comes from https://gist.github.com/neolit123/1b7375c8a956155a2ca1cdd901336bce

Which issue(s) this PR fixes:

Fixes # kubernetes/kubeadm#2419

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kubeadm: fix a bug on Windows worker nodes, where the downloaded KubeletConfiguration from the cluster can contain Linux paths that do not work on Windows and can trip the kubelet binary.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[hwdef]

/assign @neolit123

We can communicate here.

[hwdef]

@neolit123
Compared with your code, I changed this because this field becomes a pointer.

[neolit123]

i guess this was causing the panic you saw when testing?

OK, i just saw your comment below.

[hwdef]

And the mutatePathsOnWindows is panicing because cfg.ResolverConfig is nil.

[neolit123]

thanks, i will have a look tomorrow.

[neolit123]

/ok-to-test
/triage accepted
/priority backlog
/sig windows

[neolit123]

/release-note-edit

kubeadm: fix a bug on Windows worker nodes, where the downloaded KubeletConfiguration from the cluster can contain Linux paths that do not work on Windows and can trip the kubelet binary.

[neolit123]

given i wrote this a while back in gist the change looks fine to me, but i would like to get a second opinion on the idea to have Mutate() on the component config.

/cc @SataQiu @pacoxu
any objections to that?

[SataQiu]

It seems ok to add the Mutate() error function as a post hook before writing config to the disk.
Not sure we should check the os by runtime.GOOS != "windows" or // +build !windows.

[neolit123]

It seems ok to add the Mutate() error function as a post hook before writing config to the disk.

Mutate operates on the KubeletConfiguration object and Marshal finalizes the object converting it to yaml string, thus it has to be before Marshal.

Not sure we should check the os by runtime.GOOS != "windows" or // +build !windows

+build !windows is better, but one problem is that if we move the *windows logic to _windows.go files, the unit test will not be run...currently the k8s CI only runs the Linux unit tests.

@hwdef i think we can have a TODO above the `if runtime.GOOS != "windows" line

TODO: use build tags and move the Windows related logic to _windows.go files once the kubeadm code base is unit tested for Windows as part of CI - "GOOS=windows go test ...".

(please keep the commits squashed)

[RA489]

/retest

[neolit123]

added a couple of new comments above:
#105992 (comment)
#105992 (comment)

note that code freeze for 1.23 is early next week...otherwise the PR will have to wait for 1.24 (starts in mid december)

[hwdef]

/test pull-kubernetes-unit

[hwdef]

@neolit123
I updated the code and the test code. And I did some test in windows node. It looks like normol.

And I add the blank line in import, I got the ci error again.I think these errors are weird. And I didn't find any solution, please take a look.

[neolit123]

the errors in CI are because there are multiple pkg/errors imports.
also include a fix for a mistake i've made.

cfg.ResolverConfig should always be made "".

diff --git a/cmd/kubeadm/app/componentconfigs/kubelet.go b/cmd/kubeadm/app/componentconfigs/kubelet.go
index 397965971cd..2da3e8139f7 100644
--- a/cmd/kubeadm/app/componentconfigs/kubelet.go
+++ b/cmd/kubeadm/app/componentconfigs/kubelet.go
@@ -24,7 +24,6 @@ import (
 
        "github.com/pkg/errors"
 
-       "github.com/pkg/errors"
        "k8s.io/apimachinery/pkg/util/version"
        clientset "k8s.io/client-go/kubernetes"
        "k8s.io/klog/v2"
@@ -284,9 +283,7 @@ func mutatePathsOnWindows(cfg *kubeletconfig.KubeletConfiguration, drive string)
 
        // Mutate the fields we care about.
        klog.V(2).Infof("[componentconfig] kubelet/Windows: changing field \"resolverConfig\" to empty")
-       if cfg.ResolverConfig == nil {
-               cfg.ResolverConfig = utilpointer.String("") // Make sure this is empty on Windows
-       }
+       cfg.ResolverConfig = utilpointer.String("") // Make sure this is empty on Windows
        mutateStringField("staticPodPath", &cfg.StaticPodPath)
        mutateStringField("authentication.x509.clientCAFile", &cfg.Authentication.X509.ClientCAFile)
 }
diff --git a/cmd/kubeadm/app/componentconfigs/kubelet_test.go b/cmd/kubeadm/app/componentconfigs/kubelet_test.go
index e1385bb67e4..2002fca8c89 100644
--- a/cmd/kubeadm/app/componentconfigs/kubelet_test.go
+++ b/cmd/kubeadm/app/componentconfigs/kubelet_test.go
@@ -320,7 +320,7 @@ func TestMutatePathsOnWindows(t *testing.T) {
                                },
                        },
                        expected: &kubeletconfig.KubeletConfiguration{
-                               ResolverConfig: &fooResolverConfig,
+                               ResolverConfig: utilpointer.String(""),
                                StaticPodPath:  filepath.Join(drive, "/foo/staticpods"),
                                Authentication: kubeletconfig.KubeletAuthentication{
                                        X509: kubeletconfig.KubeletX509Authentication{
@@ -341,7 +341,7 @@ func TestMutatePathsOnWindows(t *testing.T) {
                                },
                        },
                        expected: &kubeletconfig.KubeletConfiguration{
-                               ResolverConfig: &fooResolverConfig,
+                               ResolverConfig: utilpointer.String(""),
                                StaticPodPath:  "./foo/staticpods",
                                Authentication: kubeletconfig.KubeletAuthentication{
                                        X509: kubeletconfig.KubeletX509Authentication{

[neolit123]

/retitle kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows

[hwdef]

@neolit123 I updated the code and test code.
But regarding the ci error, it still has not been resolved.I checked the code and confirmed that only one "github.com/pkg/errors" package was imported. When I add a blank line after "github.com/pkg/errors", ci will think that I have imported two error packages, After deleting this blank line, everything looks normal.

[neolit123]

i tested your patch locally and i saw 2 pkg error imports:

       "github.com/pkg/errors" <---- 1
 
       "github.com/pkg/errors" <---- 2
        "k8s.io/apimachinery/pkg/util/version"
        clientset "k8s.io/client-go/kubernetes"
        "k8s.io/klog/v2"

this is not visible in the github diff for some reason.
this is odd....

[neolit123]

the pkg/error import is already out of place in the current master branch:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/componentconfigs/kubelet.go#L22

so let's leave it like that...

[neolit123]

thanks for the PR!
/lgtm

[neolit123]

/hold cancel