Merge pull request #55648 from mtaufen/kc-rel-paths

Automatic merge from submit-queue (batch tested with PRs 55648, 55274, 54982, 51955, 55639). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Kubelet: Relative paths in local config file

Resolve relative paths against the config file's location.

Issue: #55644
Related comment: https://github.com/kubernetes/kubernetes/pull/53833#issuecomment-344009912

Will add the same behavior for dynamic Kubelet config in a future PR, see issue #55645.

```release-note
Relative paths in the Kubelet's local config files (--init-config-dir) will be resolved relative to the location of the containing files.
```

pkg/kubelet/apis/kubeletconfig/BUILD

@@ -3,12 +3,14 @@ package(default_visibility = ["//visibility:public"])
 load(
     "@io_bazel_rules_go//go:def.bzl",
     "go_library",
+    "go_test",
 )
 
 go_library(
     name = "go_default_library",
     srcs = [
         "doc.go",
+        "helpers.go",
         "register.go",
         "types.go",
         "zz_generated.deepcopy.go",
@@ -39,3 +41,14 @@ filegroup(
     ],
     tags = ["automanaged"],
 )
+
+go_test(
+    name = "go_default_test",
+    srcs = ["helpers_test.go"],
+    importpath = "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig",
+    library = ":go_default_library",
+    deps = [
+        "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",
+    ],
+)

pkg/kubelet/apis/kubeletconfig/helpers.go

@@ -0,0 +1,34 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeletconfig
+
+// KubeletConfigurationPathRefs returns pointers to all of the KubeletConfiguration fields that contain filepaths.
+// You might use this, for example, to resolve all relative paths against some common root before
+// passing the configuration to the application. This method must be kept up to date as new fields are added.
+func KubeletConfigurationPathRefs(kc *KubeletConfiguration) []*string {
+	paths := []*string{}
+	paths = append(paths, &kc.PodManifestPath)
+	paths = append(paths, &kc.Authentication.X509.ClientCAFile)
+	paths = append(paths, &kc.TLSCertFile)
+	paths = append(paths, &kc.TLSPrivateKeyFile)
+	paths = append(paths, &kc.SeccompProfileRoot)
+	paths = append(paths, &kc.ResolverConfig)
+	// TODO(#55562): planning on moving two out of KubeletConfiguration
+	paths = append(paths, &kc.VolumePluginDir)
+	paths = append(paths, &kc.LockFilePath)
+	return paths
+}

pkg/kubelet/apis/kubeletconfig/helpers_test.go

@@ -0,0 +1,241 @@
+/*
+Copyright 2017 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package kubeletconfig
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+)
+
+func TestKubeletConfigurationPathFields(t *testing.T) {
+	// ensure the intersection of kubeletConfigurationPathFieldPaths and KubeletConfigurationNonPathFields is empty
+	if i := kubeletConfigurationPathFieldPaths.Intersection(kubeletConfigurationNonPathFieldPaths); len(i) > 0 {
+		t.Fatalf("expect the intersection of kubeletConfigurationPathFieldPaths and "+
+			"KubeletConfigurationNonPathFields to be emtpy, got:\n%s",
+			strings.Join(i.List(), "\n"))
+	}
+
+	// ensure that kubeletConfigurationPathFields U kubeletConfigurationNonPathFields == allPrimitiveFieldPaths(KubeletConfiguration)
+	expect := sets.NewString().Union(kubeletConfigurationPathFieldPaths).Union(kubeletConfigurationNonPathFieldPaths)
+	result := allPrimitiveFieldPaths(t, reflect.TypeOf(&KubeletConfiguration{}), nil)
+	if !expect.Equal(result) {
+		// expected fields missing from result
+		missing := expect.Difference(result)
+		// unexpected fields in result but not specified in expect
+		unexpected := result.Difference(expect)
+		if len(missing) > 0 {
+			t.Errorf("the following fields were expected, but missing from the result. "+
+				"If the field has been removed, please remove it from the kubeletConfigurationPathFieldPaths set "+
+				"and the KubeletConfigurationPathRefs function, "+
+				"or remove it from the kubeletConfigurationNonPathFieldPaths set, as appropriate:\n%s",
+				strings.Join(missing.List(), "\n"))
+		}
+		if len(unexpected) > 0 {
+			t.Errorf("the following fields were in the result, but unexpected. "+
+				"If the field is new, please add it to the kubeletConfigurationPathFieldPaths set "+
+				"and the KubeletConfigurationPathRefs function, "+
+				"or add it to the kubeletConfigurationNonPathFieldPaths set, as appropriate:\n%s",
+				strings.Join(unexpected.List(), "\n"))
+		}
+	}
+}
+
+func allPrimitiveFieldPaths(t *testing.T, tp reflect.Type, path *field.Path) sets.String {
+	paths := sets.NewString()
+	switch tp.Kind() {
+	case reflect.Ptr:
+		paths.Insert(allPrimitiveFieldPaths(t, tp.Elem(), path).List()...)
+	case reflect.Struct:
+		for i := 0; i < tp.NumField(); i++ {
+			field := tp.Field(i)
+			paths.Insert(allPrimitiveFieldPaths(t, field.Type, path.Child(field.Name)).List()...)
+		}
+	case reflect.Map, reflect.Slice:
+		paths.Insert(allPrimitiveFieldPaths(t, tp.Elem(), path.Key("*")).List()...)
+	case reflect.Interface:
+		t.Fatalf("unexpected interface{} field %s", path.String())
+	default:
+		// if we hit a primitive type, we're at a leaf
+		paths.Insert(path.String())
+	}
+	return paths
+}
+
+// dummy helper types
+type foo struct {
+	foo int
+}
+type bar struct {
+	str    string
+	strptr *string
+
+	ints      []int
+	stringMap map[string]string
+
+	foo    foo
+	fooptr *foo
+
+	bars   []foo
+	barMap map[string]foo
+}
+
+func TestAllPrimitiveFieldPaths(t *testing.T) {
+	expect := sets.NewString(
+		"str",
+		"strptr",
+		"ints[*]",
+		"stringMap[*]",
+		"foo.foo",
+		"fooptr.foo",
+		"bars[*].foo",
+		"barMap[*].foo",
+	)
+	result := allPrimitiveFieldPaths(t, reflect.TypeOf(&bar{}), nil)
+	if !expect.Equal(result) {
+		// expected fields missing from result
+		missing := expect.Difference(result)
+
+		// unexpected fields in result but not specified in expect
+		unexpected := result.Difference(expect)
+
+		if len(missing) > 0 {
+			t.Errorf("the following fields were exepcted, but missing from the result:\n%s", strings.Join(missing.List(), "\n"))
+		}
+		if len(unexpected) > 0 {
+			t.Errorf("the following fields were in the result, but unexpected:\n%s", strings.Join(unexpected.List(), "\n"))
+		}
+	}
+}
+
+var (
+	// KubeletConfiguration fields that contain file paths. If you update this, also update KubeletConfigurationPathRefs!
+	kubeletConfigurationPathFieldPaths = sets.NewString(
+		"PodManifestPath",
+		"Authentication.X509.ClientCAFile",
+		"TLSCertFile",
+		"TLSPrivateKeyFile",
+		"SeccompProfileRoot",
+		"ResolverConfig",
+		"VolumePluginDir",
+		"LockFilePath",
+	)
+
+	// KubeletConfiguration fields that do not contain file paths.
+	kubeletConfigurationNonPathFieldPaths = sets.NewString(
+		"Address",
+		"AllowPrivileged",
+		"Authentication.Anonymous.Enabled",
+		"Authentication.Webhook.CacheTTL.Duration",
+		"Authentication.Webhook.Enabled",
+		"Authorization.Mode",
+		"Authorization.Webhook.CacheAuthorizedTTL.Duration",
+		"Authorization.Webhook.CacheUnauthorizedTTL.Duration",
+		"CAdvisorPort",
+		"CPUCFSQuota",
+		"CPUManagerPolicy",
+		"CPUManagerReconcilePeriod.Duration",
+		"CgroupDriver",
+		"CgroupRoot",
+		"CgroupsPerQOS",
+		"ClusterDNS[*]",
+		"ClusterDomain",
+		"ConfigTrialDuration.Duration",
+		"ContentType",
+		"EnableContentionProfiling",
+		"EnableControllerAttachDetach",
+		"EnableDebuggingHandlers",
+		"EnableServer",
+		"EnforceNodeAllocatable[*]",
+		"EventBurst",
+		"EventRecordQPS",
+		"EvictionHard",
+		"EvictionMaxPodGracePeriod",
+		"EvictionMinimumReclaim",
+		"EvictionPressureTransitionPeriod.Duration",
+		"EvictionSoft",
+		"EvictionSoftGracePeriod",
+		"ExitOnLockContention",
+		"FailSwapOn",
+		"FeatureGates[*]",
+		"FileCheckFrequency.Duration",
+		"HTTPCheckFrequency.Duration",
+		"HairpinMode",
+		"HealthzBindAddress",
+		"HealthzPort",
+		"HostIPCSources[*]",
+		"HostNetworkSources[*]",
+		"HostPIDSources[*]",
+		"IPTablesDropBit",
+		"IPTablesMasqueradeBit",
+		"ImageGCHighThresholdPercent",
+		"ImageGCLowThresholdPercent",
+		"ImageMinimumGCAge.Duration",
+		"KubeAPIBurst",
+		"KubeAPIQPS",
+		"KubeReservedCgroup",
+		"KubeReserved[*]",
+		"KubeletCgroups",
+		"MakeIPTablesUtilChains",
+		"ManifestURL",
+		"ManifestURLHeader[*][*]",
+		"MaxOpenFiles",
+		"MaxPods",
+		"NodeLabels[*]",
+		"NodeStatusUpdateFrequency.Duration",
+		"OOMScoreAdj",
+		"PodCIDR",
+		"PodsPerCore",
+		"Port",
+		"ProtectKernelDefaults",
+		"ReadOnlyPort",
+		"RegisterNode",
+		"RegisterWithTaints[*].Effect",
+		"RegisterWithTaints[*].Key",
+		"RegisterWithTaints[*].TimeAdded.Time.ext",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.cacheEnd",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.cacheStart",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.cacheZone.isDST",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.cacheZone.name",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.cacheZone.offset",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.name",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.tx[*].index",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.tx[*].isstd",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.tx[*].isutc",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.tx[*].when",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.zone[*].isDST",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.zone[*].name",
+		"RegisterWithTaints[*].TimeAdded.Time.loc.zone[*].offset",
+		"RegisterWithTaints[*].TimeAdded.Time.wall",
+		"RegisterWithTaints[*].Value",
+		"RegistryBurst",
+		"RegistryPullQPS",
+		"RuntimeRequestTimeout.Duration",
+		"SerializeImagePulls",
+		"StreamingConnectionIdleTimeout.Duration",
+		"SyncFrequency.Duration",
+		"SystemCgroups",
+		"SystemReservedCgroup",
+		"SystemReserved[*]",
+		"TypeMeta.APIVersion",
+		"TypeMeta.Kind",
+		"VolumeStatsAggPeriod.Duration",
+	)
+)

pkg/kubelet/kubeletconfig/configfiles/BUILD

@@ -44,7 +44,6 @@ go_test(
         "//pkg/kubelet/kubeletconfig/util/files:go_default_library",
         "//pkg/kubelet/kubeletconfig/util/test:go_default_library",
         "//pkg/util/filesystem:go_default_library",
-        "//vendor/github.com/davecgh/go-spew/spew:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
     ],
 )

pkg/kubelet/kubeletconfig/configfiles/configfiles.go

@@ -72,5 +72,23 @@ func (loader *fsLoader) Load() (*kubeletconfig.KubeletConfiguration, error) {
 		return nil, fmt.Errorf("init config file %q was empty, but some parameters are required", path)
 	}
 
-	return utilcodec.DecodeKubeletConfiguration(loader.kubeletCodecs, data)
+	kc, err := utilcodec.DecodeKubeletConfiguration(loader.kubeletCodecs, data)
+	if err != nil {
+		return nil, err
+	}
+
+	// make all paths absolute
+	resolveRelativePaths(kubeletconfig.KubeletConfigurationPathRefs(kc), loader.configDir)
+	return kc, nil
+}
+
+// resolveRelativePaths makes relative paths absolute by resolving them against `root`
+func resolveRelativePaths(paths []*string, root string) {
+	for _, path := range paths {
+		// leave empty paths alone, "no path" is a valid input
+		// do not attempt to resolve paths that are already absolute
+		if len(*path) > 0 && !filepath.IsAbs(*path) {
+			*path = filepath.Join(root, *path)
+		}
+	}
 }

pkg/kubelet/kubeletconfig/configfiles/configfiles_test.go

@@ -19,11 +19,8 @@ package configfiles
 import (
 	"fmt"
 	"path/filepath"
-	"strings"
 	"testing"
 
-	"github.com/davecgh/go-spew/spew"
-
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig"
 	kubeletscheme "k8s.io/kubernetes/pkg/kubelet/apis/kubeletconfig/scheme"
@@ -33,6 +30,165 @@ import (
 	utilfs "k8s.io/kubernetes/pkg/util/filesystem"
 )
 
+const configDir = "/test-config-dir"
+const relativePath = "relative/path/test"
+
+func TestLoad(t *testing.T) {
+	cases := []struct {
+		desc   string
+		file   *string
+		expect *kubeletconfig.KubeletConfiguration
+		err    string
+	}{
+		// missing file
+		{
+			"missing file",
+			nil,
+			nil,
+			"failed to read",
+		},
+		// empty file
+		{
+			"empty file",
+			newString(``),
+			nil,
+			"was empty",
+		},
+		// invalid format
+		{
+			"invalid yaml",
+			newString(`*`),
+			nil,
+			"failed to decode",
+		},
+		{
+			"invalid json",
+			newString(`{*`),
+			nil,
+			"failed to decode",
+		},
+		// invalid object
+		{
+			"missing kind",
+			newString(`{"apiVersion":"kubeletconfig/v1alpha1"}`),
+			nil,
+			"failed to decode",
+		},
+		{
+			"missing version",
+			newString(`{"kind":"KubeletConfiguration"}`),
+			nil,
+			"failed to decode",
+		},
+		{
+			"unregistered kind",
+			newString(`{"kind":"BogusKind","apiVersion":"kubeletconfig/v1alpha1"}`),
+			nil,
+			"failed to decode",
+		},
+		{
+			"unregistered version",
+			newString(`{"kind":"KubeletConfiguration","apiVersion":"bogusversion"}`),
+			nil,
+			"failed to decode",
+		},
+
+		// empty object with correct kind and version should result in the defaults for that kind and version
+		{
+			"default from yaml",
+			newString(`kind: KubeletConfiguration
+apiVersion: kubeletconfig/v1alpha1`),
+			newConfig(t),
+			"",
+		},
+		{
+			"default from json",
+			newString(`{"kind":"KubeletConfiguration","apiVersion":"kubeletconfig/v1alpha1"}`),
+			newConfig(t),
+			"",
+		},
+
+		// relative path
+		{
+			"yaml, relative path is resolved",
+			newString(fmt.Sprintf(`kind: KubeletConfiguration
+apiVersion: kubeletconfig/v1alpha1
+podManifestPath: %s`, relativePath)),
+			func() *kubeletconfig.KubeletConfiguration {
+				kc := newConfig(t)
+				kc.PodManifestPath = filepath.Join(configDir, relativePath)
+				return kc
+			}(),
+			"",
+		},
+		{
+			"json, relative path is resolved",
+			newString(fmt.Sprintf(`{"kind":"KubeletConfiguration","apiVersion":"kubeletconfig/v1alpha1","podManifestPath":"%s"}`, relativePath)),
+			func() *kubeletconfig.KubeletConfiguration {
+				kc := newConfig(t)
+				kc.PodManifestPath = filepath.Join(configDir, relativePath)
+				return kc
+			}(),
+			"",
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			fs := utilfs.NewFakeFs()
+			if c.file != nil {
+				if err := addFile(fs, filepath.Join(configDir, kubeletFile), *c.file); err != nil {
+					t.Fatalf("unexpected error: %v", err)
+				}
+			}
+			loader, err := NewFsLoader(fs, configDir)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			kc, err := loader.Load()
+			if utiltest.SkipRest(t, c.desc, err, c.err) {
+				return
+			}
+			if !apiequality.Semantic.DeepEqual(c.expect, kc) {
+				t.Fatalf("expect %#v but got %#v", *c.expect, *kc)
+			}
+		})
+
+	}
+}
+
+func TestResolveRelativePaths(t *testing.T) {
+	absolutePath := filepath.Join(configDir, "absolute")
+	cases := []struct {
+		desc   string
+		path   string
+		expect string
+	}{
+		{"empty path", "", ""},
+		{"absolute path", absolutePath, absolutePath},
+		{"relative path", relativePath, filepath.Join(configDir, relativePath)},
+	}
+
+	paths := kubeletconfig.KubeletConfigurationPathRefs(newConfig(t))
+	if len(paths) == 0 {
+		t.Fatalf("requires at least one path field to exist in the KubeletConfiguration type")
+	}
+	for _, c := range cases {
+		t.Run(c.desc, func(t *testing.T) {
+			// set the path, resolve it, and check if it resolved as we would expect
+			*(paths[0]) = c.path
+			resolveRelativePaths(paths, configDir)
+			if *(paths[0]) != c.expect {
+				t.Fatalf("expect %s but got %s", c.expect, *(paths[0]))
+			}
+		})
+	}
+}
+
+func newString(s string) *string {
+	return &s
+}
+
 func addFile(fs utilfs.Filesystem, path string, file string) error {
 	if err := utilfiles.EnsureDir(fs, filepath.Dir(path)); err != nil {
 		return err
@@ -40,73 +196,18 @@ func addFile(fs utilfs.Filesystem, path string, file string) error {
 	return utilfiles.ReplaceFile(fs, path, []byte(file))
 }
 
-func TestLoad(t *testing.T) {
+func newConfig(t *testing.T) *kubeletconfig.KubeletConfiguration {
 	kubeletScheme, _, err := kubeletscheme.NewSchemeAndCodecs()
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-
 	// get the built-in default configuration
 	external := &kubeletconfigv1alpha1.KubeletConfiguration{}
 	kubeletScheme.Default(external)
-	defaultConfig := &kubeletconfig.KubeletConfiguration{}
+	kc := &kubeletconfig.KubeletConfiguration{}
-	err = kubeletScheme.Convert(external, defaultConfig, nil)
+	err = kubeletScheme.Convert(external, kc, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-
+	return kc
-	cases := []struct {
-		desc   string
-		file   string
-		expect *kubeletconfig.KubeletConfiguration
-		err    string
-	}{
-		{"empty data", ``, nil, "was empty"},
-		// invalid format
-		{"invalid yaml", `*`, nil, "failed to decode"},
-		{"invalid json", `{*`, nil, "failed to decode"},
-		// invalid object
-		{"missing kind", `{"apiVersion":"kubeletconfig/v1alpha1"}`, nil, "failed to decode"},
-		{"missing version", `{"kind":"KubeletConfiguration"}`, nil, "failed to decode"},
-		{"unregistered kind", `{"kind":"BogusKind","apiVersion":"kubeletconfig/v1alpha1"}`, nil, "failed to decode"},
-		{"unregistered version", `{"kind":"KubeletConfiguration","apiVersion":"bogusversion"}`, nil, "failed to decode"},
-		// empty object with correct kind and version should result in the defaults for that kind and version
-		{"default from yaml", `kind: KubeletConfiguration
-apiVersion: kubeletconfig/v1alpha1`, defaultConfig, ""},
-		{"default from json", `{"kind":"KubeletConfiguration","apiVersion":"kubeletconfig/v1alpha1"}`, defaultConfig, ""},
-	}
-
-	fs := utilfs.NewFakeFs()
-	for i := range cases {
-		dir := fmt.Sprintf("/%d", i)
-		if err := addFile(fs, filepath.Join(dir, kubeletFile), cases[i].file); err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		loader, err := NewFsLoader(fs, dir)
-		if err != nil {
-			t.Fatalf("unexpected error: %v", err)
-		}
-		kc, err := loader.Load()
-		if utiltest.SkipRest(t, cases[i].desc, err, cases[i].err) {
-			continue
-		}
-		// we expect the parsed configuration to match what we described in the ConfigMap
-		if !apiequality.Semantic.DeepEqual(cases[i].expect, kc) {
-			t.Errorf("case %q, expect config %s but got %s", cases[i].desc, spew.Sdump(cases[i].expect), spew.Sdump(kc))
-		}
-	}
-
-	// finally test for a missing file
-	desc := "missing kubelet file"
-	contains := "failed to read"
-	loader, err := NewFsLoader(fs, "/fake")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	_, err = loader.Load()
-	if err == nil {
-		t.Errorf("case %q, expect error to contain %q but got nil error", desc, contains)
-	} else if !strings.Contains(err.Error(), contains) {
-		t.Errorf("case %q, expect error to contain %q but got %q", desc, contains, err.Error())
-	}
 }