|
| 1 | +From ee5578be526815ab3ea63abda6cb95588c8265a3 Mon Sep 17 00:00:00 2001 |
| 2 | +From: Sohan Kunkerkar <sohank2602@gmail.com> |
| 3 | +Date: Thu, 2 Nov 2023 09:24:27 -0400 |
| 4 | +Subject: [PATCH] cmd/kubelet: fix overriding default KubeletConfig fields in |
| 5 | + drop-in configs if not set |
| 6 | + |
| 7 | +This commit resolves an issue where certain KubeletConfig fields, specifically: |
| 8 | +- FileCheckFrequency |
| 9 | +- VolumeStatsAggPeriod |
| 10 | +- EvictionPressureTransitionPeriod |
| 11 | +- Authorization.Mode |
| 12 | +- EvictionHard |
| 13 | +were inadvertently overridden when not explicitly set in drop-in configs. To retain the |
| 14 | +original values if they were absent in the drop-in configs, mergeKubeletConfigurations |
| 15 | +uses a JSON patch merge strategy to selectively merge configurations. It prevents essential |
| 16 | +configuration settings from being overridden, ensuring a more predictable behavior for users. |
| 17 | + |
| 18 | +Signed-off-by: Sohan Kunkerkar <sohank2602@gmail.com> |
| 19 | +Co-authored-by: Peter Hunt <pehunt@redhat.com> |
| 20 | +Signed-off-by: Gavin Inglis <giinglis@amazon.com> |
| 21 | +--- |
| 22 | + cmd/kubelet/app/server.go | 41 +++++++--- |
| 23 | + cmd/kubelet/app/server_test.go | 77 +++++++++++-------- |
| 24 | + go.mod | 2 +- |
| 25 | + .../kubeletconfig/configfiles/configfiles.go | 17 ++++ |
| 26 | + pkg/kubelet/kubeletconfig/util/codec/codec.go | 15 ++++ |
| 27 | + 5 files changed, 108 insertions(+), 44 deletions(-) |
| 28 | + |
| 29 | +diff --git a/cmd/kubelet/app/server.go b/cmd/kubelet/app/server.go |
| 30 | +index bb81ec68738..76e8cdf7b99 100644 |
| 31 | +--- a/cmd/kubelet/app/server.go |
| 32 | ++++ b/cmd/kubelet/app/server.go |
| 33 | +@@ -20,6 +20,7 @@ package app |
| 34 | + import ( |
| 35 | + "context" |
| 36 | + "crypto/tls" |
| 37 | ++ "encoding/json" |
| 38 | + "errors" |
| 39 | + "fmt" |
| 40 | + "io" |
| 41 | +@@ -34,7 +35,7 @@ import ( |
| 42 | + "time" |
| 43 | + |
| 44 | + "github.com/coreos/go-systemd/v22/daemon" |
| 45 | +- "github.com/imdario/mergo" |
| 46 | ++ jsonpatch "github.com/evanphx/json-patch" |
| 47 | + "github.com/spf13/cobra" |
| 48 | + "github.com/spf13/pflag" |
| 49 | + "google.golang.org/grpc/codes" |
| 50 | +@@ -312,30 +313,34 @@ is checked every 20 seconds (also configurable with a flag).`, |
| 51 | + // potentially overriding the previous values. |
| 52 | + func mergeKubeletConfigurations(kubeletConfig *kubeletconfiginternal.KubeletConfiguration, kubeletDropInConfigDir string) error { |
| 53 | + const dropinFileExtension = ".conf" |
| 54 | +- |
| 55 | ++ baseKubeletConfigJSON, err := json.Marshal(kubeletConfig) |
| 56 | ++ if err != nil { |
| 57 | ++ return fmt.Errorf("failed to marshal base config: %w", err) |
| 58 | ++ } |
| 59 | + // Walk through the drop-in directory and update the configuration for each file |
| 60 | +- err := filepath.WalkDir(kubeletDropInConfigDir, func(path string, info fs.DirEntry, err error) error { |
| 61 | ++ if err := filepath.WalkDir(kubeletDropInConfigDir, func(path string, info fs.DirEntry, err error) error { |
| 62 | + if err != nil { |
| 63 | + return err |
| 64 | + } |
| 65 | + if !info.IsDir() && filepath.Ext(info.Name()) == dropinFileExtension { |
| 66 | +- dropinConfig, err := loadConfigFile(path) |
| 67 | ++ dropinConfigJSON, err := loadDropinConfigFileIntoJSON(path) |
| 68 | + if err != nil { |
| 69 | + return fmt.Errorf("failed to load kubelet dropin file, path: %s, error: %w", path, err) |
| 70 | + } |
| 71 | +- |
| 72 | +- // Merge dropinConfig with kubeletConfig |
| 73 | +- if err := mergo.Merge(kubeletConfig, dropinConfig, mergo.WithOverride); err != nil { |
| 74 | +- return fmt.Errorf("failed to merge kubelet drop-in config, path: %s, error: %w", path, err) |
| 75 | ++ mergedConfigJSON, err := jsonpatch.MergePatch(baseKubeletConfigJSON, dropinConfigJSON) |
| 76 | ++ if err != nil { |
| 77 | ++ return fmt.Errorf("failed to merge drop-in and current config: %w", err) |
| 78 | + } |
| 79 | ++ baseKubeletConfigJSON = mergedConfigJSON |
| 80 | + } |
| 81 | + return nil |
| 82 | +- }) |
| 83 | +- |
| 84 | +- if err != nil { |
| 85 | ++ }); err != nil { |
| 86 | + return fmt.Errorf("failed to walk through kubelet dropin directory %q: %w", kubeletDropInConfigDir, err) |
| 87 | + } |
| 88 | + |
| 89 | ++ if err := json.Unmarshal(baseKubeletConfigJSON, kubeletConfig); err != nil { |
| 90 | ++ return fmt.Errorf("failed to unmarshal merged JSON into kubelet configuration: %w", err) |
| 91 | ++ } |
| 92 | + return nil |
| 93 | + } |
| 94 | + |
| 95 | +@@ -415,6 +420,20 @@ func loadConfigFile(name string) (*kubeletconfiginternal.KubeletConfiguration, e |
| 96 | + return kc, err |
| 97 | + } |
| 98 | + |
| 99 | ++func loadDropinConfigFileIntoJSON(name string) ([]byte, error) { |
| 100 | ++ const errFmt = "failed to load drop-in kubelet config file %s, error %v" |
| 101 | ++ // compute absolute path based on current working dir |
| 102 | ++ kubeletConfigFile, err := filepath.Abs(name) |
| 103 | ++ if err != nil { |
| 104 | ++ return nil, fmt.Errorf(errFmt, name, err) |
| 105 | ++ } |
| 106 | ++ loader, err := configfiles.NewFsLoader(&utilfs.DefaultFs{}, kubeletConfigFile) |
| 107 | ++ if err != nil { |
| 108 | ++ return nil, fmt.Errorf(errFmt, name, err) |
| 109 | ++ } |
| 110 | ++ return loader.LoadIntoJSON() |
| 111 | ++} |
| 112 | ++ |
| 113 | + // UnsecuredDependencies returns a Dependencies suitable for being run, or an error if the server setup |
| 114 | + // is not valid. It will not start any background processes, and does not include authentication/authorization |
| 115 | + func UnsecuredDependencies(s *options.KubeletServer, featureGate featuregate.FeatureGate) (*kubelet.Dependencies, error) { |
| 116 | +diff --git a/cmd/kubelet/app/server_test.go b/cmd/kubelet/app/server_test.go |
| 117 | +index 0a4fda291a2..3513c5fc7ed 100644 |
| 118 | +--- a/cmd/kubelet/app/server_test.go |
| 119 | ++++ b/cmd/kubelet/app/server_test.go |
| 120 | +@@ -21,8 +21,11 @@ import ( |
| 121 | + "path/filepath" |
| 122 | + "reflect" |
| 123 | + "testing" |
| 124 | ++ "time" |
| 125 | + |
| 126 | + "github.com/stretchr/testify/require" |
| 127 | ++ "gopkg.in/yaml.v2" |
| 128 | ++ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
| 129 | + "k8s.io/kubernetes/cmd/kubelet/app/options" |
| 130 | + kubeletconfiginternal "k8s.io/kubernetes/pkg/kubelet/apis/config" |
| 131 | + ) |
| 132 | +@@ -71,7 +74,7 @@ func TestValueOfAllocatableResources(t *testing.T) { |
| 133 | + |
| 134 | + func TestMergeKubeletConfigurations(t *testing.T) { |
| 135 | + testCases := []struct { |
| 136 | +- kubeletConfig string |
| 137 | ++ kubeletConfig *kubeletconfiginternal.KubeletConfiguration |
| 138 | + dropin1 string |
| 139 | + dropin2 string |
| 140 | + overwrittenConfigFields map[string]interface{} |
| 141 | +@@ -79,12 +82,14 @@ func TestMergeKubeletConfigurations(t *testing.T) { |
| 142 | + name string |
| 143 | + }{ |
| 144 | + { |
| 145 | +- kubeletConfig: ` |
| 146 | +-apiVersion: kubelet.config.k8s.io/v1beta1 |
| 147 | +-kind: KubeletConfiguration |
| 148 | +-port: 9080 |
| 149 | +-readOnlyPort: 10257 |
| 150 | +-`, |
| 151 | ++ kubeletConfig: &kubeletconfiginternal.KubeletConfiguration{ |
| 152 | ++ TypeMeta: metav1.TypeMeta{ |
| 153 | ++ Kind: "KubeletConfiguration", |
| 154 | ++ APIVersion: "kubelet.config.k8s.io/v1beta1", |
| 155 | ++ }, |
| 156 | ++ Port: int32(9090), |
| 157 | ++ ReadOnlyPort: int32(10257), |
| 158 | ++ }, |
| 159 | + dropin1: ` |
| 160 | + apiVersion: kubelet.config.k8s.io/v1beta1 |
| 161 | + kind: KubeletConfiguration |
| 162 | +@@ -103,13 +108,15 @@ readOnlyPort: 10255 |
| 163 | + name: "kubelet.conf.d overrides kubelet.conf", |
| 164 | + }, |
| 165 | + { |
| 166 | +- kubeletConfig: ` |
| 167 | +-apiVersion: kubelet.config.k8s.io/v1beta1 |
| 168 | +-kind: KubeletConfiguration |
| 169 | +-readOnlyPort: 10256 |
| 170 | +-kubeReserved: |
| 171 | +- memory: 70Mi |
| 172 | +-`, |
| 173 | ++ kubeletConfig: &kubeletconfiginternal.KubeletConfiguration{ |
| 174 | ++ TypeMeta: metav1.TypeMeta{ |
| 175 | ++ Kind: "KubeletConfiguration", |
| 176 | ++ APIVersion: "kubelet.config.k8s.io/v1beta1", |
| 177 | ++ }, |
| 178 | ++ ReadOnlyPort: int32(10256), |
| 179 | ++ KubeReserved: map[string]string{"memory": "100Mi"}, |
| 180 | ++ SyncFrequency: metav1.Duration{Duration: 5 * time.Minute}, |
| 181 | ++ }, |
| 182 | + dropin1: ` |
| 183 | + apiVersion: kubelet.config.k8s.io/v1beta1 |
| 184 | + kind: KubeletConfiguration |
| 185 | +@@ -131,18 +138,19 @@ kubeReserved: |
| 186 | + "cpu": "200m", |
| 187 | + "memory": "100Mi", |
| 188 | + }, |
| 189 | ++ "SyncFrequency": metav1.Duration{Duration: 5 * time.Minute}, |
| 190 | + }, |
| 191 | + name: "kubelet.conf.d overrides kubelet.conf with subfield override", |
| 192 | + }, |
| 193 | + { |
| 194 | +- kubeletConfig: ` |
| 195 | +-apiVersion: kubelet.config.k8s.io/v1beta1 |
| 196 | +-kind: KubeletConfiguration |
| 197 | +-port: 9090 |
| 198 | +-clusterDNS: |
| 199 | +- - 192.168.1.3 |
| 200 | +- - 192.168.1.4 |
| 201 | +-`, |
| 202 | ++ kubeletConfig: &kubeletconfiginternal.KubeletConfiguration{ |
| 203 | ++ TypeMeta: metav1.TypeMeta{ |
| 204 | ++ Kind: "KubeletConfiguration", |
| 205 | ++ APIVersion: "kubelet.config.k8s.io/v1beta1", |
| 206 | ++ }, |
| 207 | ++ Port: int32(9090), |
| 208 | ++ ClusterDNS: []string{"192.168.1.3", "192.168.1.4"}, |
| 209 | ++ }, |
| 210 | + dropin1: ` |
| 211 | + apiVersion: kubelet.config.k8s.io/v1beta1 |
| 212 | + kind: KubeletConfiguration |
| 213 | +@@ -173,6 +181,7 @@ clusterDNS: |
| 214 | + name: "kubelet.conf.d overrides kubelet.conf with slices/lists", |
| 215 | + }, |
| 216 | + { |
| 217 | ++ kubeletConfig: nil, |
| 218 | + dropin1: ` |
| 219 | + apiVersion: kubelet.config.k8s.io/v1beta1 |
| 220 | + kind: KubeletConfiguration |
| 221 | +@@ -195,13 +204,14 @@ readOnlyPort: 10255 |
| 222 | + name: "cli args override kubelet.conf.d", |
| 223 | + }, |
| 224 | + { |
| 225 | +- kubeletConfig: ` |
| 226 | +-apiVersion: kubelet.config.k8s.io/v1beta1 |
| 227 | +-kind: KubeletConfiguration |
| 228 | +-port: 9090 |
| 229 | +-clusterDNS: |
| 230 | +- - 192.168.1.3 |
| 231 | +-`, |
| 232 | ++ kubeletConfig: &kubeletconfiginternal.KubeletConfiguration{ |
| 233 | ++ TypeMeta: metav1.TypeMeta{ |
| 234 | ++ Kind: "KubeletConfiguration", |
| 235 | ++ APIVersion: "kubelet.config.k8s.io/v1beta1", |
| 236 | ++ }, |
| 237 | ++ Port: int32(9090), |
| 238 | ++ ClusterDNS: []string{"192.168.1.3"}, |
| 239 | ++ }, |
| 240 | + overwrittenConfigFields: map[string]interface{}{ |
| 241 | + "Port": int32(9090), |
| 242 | + "ClusterDNS": []string{"192.168.1.2"}, |
| 243 | +@@ -222,12 +232,15 @@ clusterDNS: |
| 244 | + kubeletConfig := &kubeletconfiginternal.KubeletConfiguration{} |
| 245 | + kubeletFlags := &options.KubeletFlags{} |
| 246 | + |
| 247 | +- if len(test.kubeletConfig) > 0 { |
| 248 | ++ if test.kubeletConfig != nil { |
| 249 | + // Create the Kubeletconfig |
| 250 | + kubeletConfFile := filepath.Join(tempDir, "kubelet.conf") |
| 251 | +- err := os.WriteFile(kubeletConfFile, []byte(test.kubeletConfig), 0644) |
| 252 | +- require.NoError(t, err, "failed to create config from a yaml file") |
| 253 | ++ yamlData, err := yaml.Marshal(test.kubeletConfig) // Convert struct to YAML |
| 254 | ++ require.NoError(t, err, "failed to convert kubelet config to YAML") |
| 255 | ++ err = os.WriteFile(kubeletConfFile, yamlData, 0644) |
| 256 | ++ require.NoError(t, err, "failed to create config from YAML data") |
| 257 | + kubeletFlags.KubeletConfigFile = kubeletConfFile |
| 258 | ++ kubeletConfig = test.kubeletConfig |
| 259 | + } |
| 260 | + if len(test.dropin1) > 0 || len(test.dropin2) > 0 { |
| 261 | + // Create kubelet.conf.d directory and drop-in configuration files |
| 262 | +diff --git a/go.mod b/go.mod |
| 263 | +index f5dae6d2475..e3cac99d9be 100644 |
| 264 | +--- a/go.mod |
| 265 | ++++ b/go.mod |
| 266 | +@@ -45,7 +45,6 @@ require ( |
| 267 | + github.com/google/go-cmp v0.6.0 |
| 268 | + github.com/google/gofuzz v1.2.0 |
| 269 | + github.com/google/uuid v1.3.1 |
| 270 | +- github.com/imdario/mergo v0.3.6 |
| 271 | + github.com/ishidawataru/sctp v0.0.0-20190723014705-7c296d48a2b5 |
| 272 | + github.com/libopenstorage/openstorage v1.0.0 |
| 273 | + github.com/lithammer/dedent v1.1.0 |
| 274 | +@@ -185,6 +184,7 @@ require ( |
| 275 | + github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 // indirect |
| 276 | + github.com/grpc-ecosystem/grpc-gateway v1.16.0 // indirect |
| 277 | + github.com/grpc-ecosystem/grpc-gateway/v2 v2.16.0 // indirect |
| 278 | ++ github.com/imdario/mergo v0.3.6 // indirect |
| 279 | + github.com/inconshreveable/mousetrap v1.1.0 // indirect |
| 280 | + github.com/jonboulle/clockwork v0.2.2 // indirect |
| 281 | + github.com/josharian/intern v1.0.0 // indirect |
| 282 | +diff --git a/pkg/kubelet/kubeletconfig/configfiles/configfiles.go b/pkg/kubelet/kubeletconfig/configfiles/configfiles.go |
| 283 | +index 63aef74f126..46423cb241d 100644 |
| 284 | +--- a/pkg/kubelet/kubeletconfig/configfiles/configfiles.go |
| 285 | ++++ b/pkg/kubelet/kubeletconfig/configfiles/configfiles.go |
| 286 | +@@ -31,6 +31,9 @@ import ( |
| 287 | + type Loader interface { |
| 288 | + // Load loads and returns the KubeletConfiguration from the storage layer, or an error if a configuration could not be loaded |
| 289 | + Load() (*kubeletconfig.KubeletConfiguration, error) |
| 290 | ++ // LoadIntoJSON loads and returns the KubeletConfiguration from the storage layer, or an error if a configuration could not be |
| 291 | ++ // loaded. It returns the configuration as a JSON byte slice |
| 292 | ++ LoadIntoJSON() ([]byte, error) |
| 293 | + } |
| 294 | + |
| 295 | + // fsLoader loads configuration from `configDir` |
| 296 | +@@ -78,6 +81,20 @@ func (loader *fsLoader) Load() (*kubeletconfig.KubeletConfiguration, error) { |
| 297 | + return kc, nil |
| 298 | + } |
| 299 | + |
| 300 | ++func (loader *fsLoader) LoadIntoJSON() ([]byte, error) { |
| 301 | ++ data, err := loader.fs.ReadFile(loader.kubeletFile) |
| 302 | ++ if err != nil { |
| 303 | ++ return nil, fmt.Errorf("failed to read drop-in kubelet config file %q, error: %v", loader.kubeletFile, err) |
| 304 | ++ } |
| 305 | ++ |
| 306 | ++ // no configuration is an error, some parameters are required |
| 307 | ++ if len(data) == 0 { |
| 308 | ++ return nil, fmt.Errorf("kubelet config file %q was empty", loader.kubeletFile) |
| 309 | ++ } |
| 310 | ++ |
| 311 | ++ return utilcodec.DecodeKubeletConfigurationIntoJSON(loader.kubeletCodecs, data) |
| 312 | ++} |
| 313 | ++ |
| 314 | + // resolveRelativePaths makes relative paths absolute by resolving them against `root` |
| 315 | + func resolveRelativePaths(paths []*string, root string) { |
| 316 | + for _, path := range paths { |
| 317 | +diff --git a/pkg/kubelet/kubeletconfig/util/codec/codec.go b/pkg/kubelet/kubeletconfig/util/codec/codec.go |
| 318 | +index 8598c0ca2d9..ba4b41cfd85 100644 |
| 319 | +--- a/pkg/kubelet/kubeletconfig/util/codec/codec.go |
| 320 | ++++ b/pkg/kubelet/kubeletconfig/util/codec/codec.go |
| 321 | +@@ -17,6 +17,7 @@ limitations under the License. |
| 322 | + package codec |
| 323 | + |
| 324 | + import ( |
| 325 | ++ "encoding/json" |
| 326 | + "fmt" |
| 327 | + |
| 328 | + "k8s.io/klog/v2" |
| 329 | +@@ -24,6 +25,7 @@ import ( |
| 330 | + // ensure the core apis are installed |
| 331 | + _ "k8s.io/kubernetes/pkg/apis/core/install" |
| 332 | + |
| 333 | ++ "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" |
| 334 | + "k8s.io/apimachinery/pkg/runtime" |
| 335 | + "k8s.io/apimachinery/pkg/runtime/schema" |
| 336 | + "k8s.io/apimachinery/pkg/runtime/serializer" |
| 337 | +@@ -105,3 +107,16 @@ func DecodeKubeletConfiguration(kubeletCodecs *serializer.CodecFactory, data []b |
| 338 | + |
| 339 | + return internalKC, nil |
| 340 | + } |
| 341 | ++ |
| 342 | ++// DecodeKubeletConfigurationIntoJSON decodes a serialized KubeletConfiguration to the internal type. |
| 343 | ++func DecodeKubeletConfigurationIntoJSON(kubeletCodecs *serializer.CodecFactory, data []byte) ([]byte, error) { |
| 344 | ++ // The UniversalDecoder runs defaulting and returns the internal type by default. |
| 345 | ++ obj, _, err := kubeletCodecs.UniversalDecoder().Decode(data, nil, &unstructured.Unstructured{}) |
| 346 | ++ if err != nil { |
| 347 | ++ return nil, err |
| 348 | ++ } |
| 349 | ++ |
| 350 | ++ objT := obj.(*unstructured.Unstructured) |
| 351 | ++ |
| 352 | ++ return json.Marshal(objT.Object) |
| 353 | ++} |
| 354 | +-- |
| 355 | +2.47.1 |
| 356 | + |
0 commit comments