kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows #105992

hwdef · 2021-10-29T01:38:47Z

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 · 2021-10-29T01:41:48Z

/assign @neolit123

We can communicate here.

hwdef · 2021-10-29T02:43:01Z

cmd/kubeadm/app/componentconfigs/kubelet.go

+	}
+
+	// Mutate the fields we care about.
+	mutateStringField("resolvConf", cfg.ResolverConfig, true) // make sure this is disabled on Windows


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

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

OK, i just saw your comment below.

hwdef · 2021-10-29T07:36:36Z

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

neolit123 · 2021-11-01T17:55:27Z

thanks, i will have a look tomorrow.

neolit123

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

neolit123 · 2021-11-02T17:44:08Z

/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 · 2021-11-02T17:45:26Z

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 · 2021-11-07T09:50:18Z

cmd/kubeadm/app/componentconfigs/kubelet.go

@@ -212,6 +216,63 @@ func (kc *kubeletConfig) Default(cfg *kubeadmapi.ClusterConfiguration, _ *kubead
 	}
 }

+// Mutate modifies absolute path fields in the KubeletConfiguration to be Windows compatible absolute paths.
+func (kc *kubeletConfig) Mutate() error {
+	if runtime.GOOS != "windows" {


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.

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 · 2021-11-10T03:20:45Z

/retest

neolit123 · 2021-11-10T20:53:31Z

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 · 2021-11-11T04:46:47Z

/test pull-kubernetes-unit

hwdef · 2021-11-11T07:09:21Z

@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 · 2021-11-11T18:15:17Z

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 · 2021-11-11T18:19:54Z

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

hwdef · 2021-11-12T02:42:01Z

@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.

…indows

neolit123 · 2021-11-12T03:43:36Z

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 · 2021-11-12T03:46:16Z

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 · 2021-11-12T03:47:56Z

/hold cancel

k8s-ci-robot requested review from RA489 and yagonobre October 29, 2021 01:39

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 29, 2021

k8s-ci-robot assigned neolit123 Oct 29, 2021

hwdef commented Oct 29, 2021

View reviewed changes

neolit123 reviewed Nov 2, 2021

View reviewed changes

k8s-ci-robot requested review from pacoxu and SataQiu November 2, 2021 17:45

SataQiu reviewed Nov 7, 2021

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021

hwdef force-pushed the fix-kubeadm-2419 branch from 41b214b to 23065e2 Compare November 10, 2021 03:36

hwdef force-pushed the fix-kubeadm-2419 branch 2 times, most recently from b7e7d63 to 7aff6e3 Compare November 11, 2021 03:17

hwdef force-pushed the fix-kubeadm-2419 branch 3 times, most recently from 2ab266d to 85e3dfc Compare November 11, 2021 06:56

k8s-ci-robot changed the title ~~kubeadm: Fix the problem that the absolute path in the windows do not work in the config file.~~ kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows Nov 11, 2021

hwdef force-pushed the fix-kubeadm-2419 branch from 85e3dfc to 897e609 Compare November 12, 2021 01:46

kubeadm: fix absolute paths do not work properly in config files in w…

b985e09

…indows

hwdef force-pushed the fix-kubeadm-2419 branch from 897e609 to b985e09 Compare November 12, 2021 02:42

neolit123 reviewed Nov 12, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2021

k8s-ci-robot merged commit 35f9bca into kubernetes:master Nov 12, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 12, 2021

neolit123 mentioned this pull request Nov 12, 2021

windows: absolute paths do not work properly in config files kubernetes/kubeadm#2419

Closed

hwdef deleted the fix-kubeadm-2419 branch November 25, 2021 08:54

This was referenced Apr 8, 2022

client CA file can't be found on v1.22.8 capz Windows image kubernetes-sigs/image-builder#853

Closed

Absolute in Windows kubelet don't work < 1.23 kubernetes-sigs/image-builder#856

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows #105992

kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows #105992

hwdef commented Oct 29, 2021 •

edited by k8s-ci-robot

hwdef commented Oct 29, 2021

hwdef Oct 29, 2021

neolit123 Nov 2, 2021 •

edited

hwdef commented Oct 29, 2021

neolit123 commented Nov 1, 2021

neolit123 left a comment

neolit123 commented Nov 2, 2021

neolit123 commented Nov 2, 2021

SataQiu Nov 7, 2021

neolit123 Nov 8, 2021

RA489 commented Nov 10, 2021

neolit123 commented Nov 10, 2021 •

edited

hwdef commented Nov 11, 2021

hwdef commented Nov 11, 2021

neolit123 commented Nov 11, 2021 •

edited

neolit123 commented Nov 11, 2021

hwdef commented Nov 12, 2021

neolit123 commented Nov 12, 2021 •

edited

neolit123 commented Nov 12, 2021

neolit123 left a comment

neolit123 commented Nov 12, 2021

kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows #105992

kubeadm: add mutation for Linux paths in KubeletConfiguration on Windows #105992

Conversation

hwdef commented Oct 29, 2021 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

hwdef commented Oct 29, 2021

hwdef Oct 29, 2021

Choose a reason for hiding this comment

neolit123 Nov 2, 2021 • edited

Choose a reason for hiding this comment

hwdef commented Oct 29, 2021

neolit123 commented Nov 1, 2021

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 2, 2021

neolit123 commented Nov 2, 2021

SataQiu Nov 7, 2021

Choose a reason for hiding this comment

neolit123 Nov 8, 2021

Choose a reason for hiding this comment

RA489 commented Nov 10, 2021

neolit123 commented Nov 10, 2021 • edited

hwdef commented Nov 11, 2021

hwdef commented Nov 11, 2021

neolit123 commented Nov 11, 2021 • edited

neolit123 commented Nov 11, 2021

hwdef commented Nov 12, 2021

neolit123 commented Nov 12, 2021 • edited

neolit123 commented Nov 12, 2021

neolit123 left a comment

Choose a reason for hiding this comment

neolit123 commented Nov 12, 2021

hwdef commented Oct 29, 2021 •

edited by k8s-ci-robot

neolit123 Nov 2, 2021 •

edited

neolit123 commented Nov 10, 2021 •

edited

neolit123 commented Nov 11, 2021 •

edited

neolit123 commented Nov 12, 2021 •

edited