chore: clean up kubelet config file logic (#2613)

The logic to write merged config to a file was only used in versions <=1.28. Those are all EOL now,
delete the old logic and update some old integration tests which still used the legacy logic.

Delete the withPodInfraContainer function for similar reasons.

Inject IMDS client as a dependency so that we can have a unit test for GenerateKubeletConfig.

Author: fletcherw

nodeadm/cmd/nodeadm/init/init.go

@@ -103,7 +103,7 @@ func (c *initCmd) Run(log *zap.Logger, opts *cli.GlobalOptions) error {
 	resources := system.NewResources(system.RealFileSystem{})
 	daemons := []daemon.Daemon{
 		containerd.NewContainerdDaemon(daemonManager, resources),
-		kubelet.NewKubeletDaemon(daemonManager, resources),
+		kubelet.NewKubeletDaemon(daemonManager, resources, imds.DefaultClient()),
 	}
 
 	// to handle edge cases where the cached config is stale (because the user

nodeadm/internal/kubelet/config.go

@@ -35,16 +35,6 @@ const (
 	kubeletConfigPerm = 0644
 )
 
-func (k *kubelet) writeKubeletConfig(cfg *api.NodeConfig) error {
-	// tracking: https://github.com/kubernetes/enhancements/issues/3983
-	// for enabling drop-in configuration
-	if semver.Compare(cfg.Status.KubeletVersion, "v1.29.0") < 0 {
-		return k.writeKubeletConfigToFile(cfg)
-	} else {
-		return k.writeKubeletConfigToDir(cfg)
-	}
-}
-
 // kubeletConfig is an internal-only representation of the kubelet configuration
 // that is generated using sane defaults for EKS. It is a subset of the upstream
 // KubeletConfiguration types:
@@ -233,8 +223,8 @@ func (ksc *kubeletConfig) withNodeLabels(flags map[string]string, nodeLabelFuncs
 	}
 }
 
-func (ksc *kubeletConfig) withNodeIp(cfg *api.NodeConfig, flags map[string]string) error {
-	nodeIp, err := getNodeIp(context.TODO(), cfg, imds.DefaultClient())
+func (ksc *kubeletConfig) withNodeIp(cfg *api.NodeConfig, flags map[string]string, imdsClient imds.IMDSClient) error {
+	nodeIp, err := getNodeIp(context.TODO(), cfg, imdsClient)
 	if err != nil {
 		return err
 	}
@@ -280,8 +270,7 @@ func (ksc *kubeletConfig) withCloudProvider(cfg *api.NodeConfig, flags map[strin
 	flags["hostname-override"] = nodeName
 }
 
-// When the DefaultReservedResources flag is enabled, override the kubelet
-// config with reserved cgroup values on behalf of the user
+// Override the kubelet config with reserved cgroup values on behalf of the user
 func (ksc *kubeletConfig) withDefaultReservedResources(cfg *api.NodeConfig, resources system.Resources) {
 	ksc.SystemReservedCgroup = ptr.String("/system")
 	ksc.KubeReservedCgroup = ptr.String("/runtime")
@@ -298,29 +287,13 @@ func (ksc *kubeletConfig) withDefaultReservedResources(cfg *api.NodeConfig, reso
 	}
 }
 
-// withPodInfraContainerImage determines whether to add the
-// '--pod-infra-container-image' flag, which is used to ensure the sandbox image
-// is not garbage collected.
-//
-// TODO: revisit once the minimum supportted version catches up or the container
-// runtime is moved to containerd 2.0
-func (ksc *kubeletConfig) withPodInfraContainerImage(cfg *api.NodeConfig, flags map[string]string) error {
-	// the flag is a noop on 1.29+, since the behavior was changed to use the
-	// CRI image pinning behavior and no longer considers the flag value.
-	// see: https://github.com/kubernetes/kubernetes/pull/118544
-	if semver.Compare(cfg.Status.KubeletVersion, "v1.29.0") < 0 {
-		flags["pod-infra-container-image"] = cfg.Status.Defaults.SandboxImage
-	}
-	return nil
-}
-
 func (ksc *kubeletConfig) withImageServiceEndpoint(cfg *api.NodeConfig, resources system.Resources) {
 	if containerd.UseSOCISnapshotter(cfg, resources) {
 		ksc.ImageServiceEndpoint = "unix:///run/soci-snapshotter-grpc/soci-snapshotter-grpc.sock"
 	}
 }
 
-func (k *kubelet) GenerateKubeletConfig(cfg *api.NodeConfig) (*kubeletConfig, error) {
+func (k *kubelet) generateKubeletConfig(cfg *api.NodeConfig) (*kubeletConfig, error) {
 	kubeletConfig := defaultKubeletSubConfig()
 
 	if err := kubeletConfig.withFallbackClusterDns(&cfg.Spec.Cluster); err != nil {
@@ -329,10 +302,7 @@ func (k *kubelet) GenerateKubeletConfig(cfg *api.NodeConfig) (*kubeletConfig, er
 	if err := kubeletConfig.withOutpostSetup(cfg); err != nil {
 		return nil, err
 	}
-	if err := kubeletConfig.withNodeIp(cfg, k.flags); err != nil {
-		return nil, err
-	}
-	if err := kubeletConfig.withPodInfraContainerImage(cfg, k.flags); err != nil {
+	if err := kubeletConfig.withNodeIp(cfg, k.flags, k.imdsClient); err != nil {
 		return nil, err
 	}
 
@@ -351,43 +321,11 @@ func (k *kubelet) GenerateKubeletConfig(cfg *api.NodeConfig) (*kubeletConfig, er
 	return &kubeletConfig, nil
 }
 
-// WriteConfig writes the kubelet config to a file.
-// This should only be used for kubelet versions < 1.28.
-func (k *kubelet) writeKubeletConfigToFile(cfg *api.NodeConfig) error {
-	kubeletConfig, err := k.GenerateKubeletConfig(cfg)
-	if err != nil {
-		return err
-	}
-
-	var kubeletConfigBytes []byte
-	if len(cfg.Spec.Kubelet.Config) > 0 {
-		mergedMap, err := util.Merge(kubeletConfig, cfg.Spec.Kubelet.Config, json.Marshal, json.Unmarshal)
-		if err != nil {
-			return err
-		}
-		if kubeletConfigBytes, err = json.MarshalIndent(mergedMap, "", strings.Repeat(" ", 4)); err != nil {
-			return err
-		}
-	} else {
-		var err error
-		if kubeletConfigBytes, err = json.MarshalIndent(kubeletConfig, "", strings.Repeat(" ", 4)); err != nil {
-			return err
-		}
-	}
-
-	configPath := path.Join(kubeletConfigRoot, kubeletConfigFile)
-	k.flags["config"] = configPath
-
-	zap.L().Info("Writing kubelet config to file..", zap.String("path", configPath))
-	return util.WriteFileWithDir(configPath, kubeletConfigBytes, kubeletConfigPerm)
-}
-
-// WriteKubeletConfigToDir writes nodeadm's generated kubelet config to the
+// writeKubeletConfig writes nodeadm's generated kubelet config to the
 // standard config file and writes the user's provided config to a directory for
-// drop-in support. This is only supported on kubelet versions >= 1.28. see:
-// https://kubernetes.io/docs/tasks/administer-cluster/kubelet-config-file/#kubelet-conf-d
-func (k *kubelet) writeKubeletConfigToDir(cfg *api.NodeConfig) error {
-	kubeletConfig, err := k.GenerateKubeletConfig(cfg)
+// drop-in support.
+func (k *kubelet) writeKubeletConfig(cfg *api.NodeConfig) error {
+	kubeletConfig, err := k.generateKubeletConfig(cfg)
 	if err != nil {
 		return err
 	}
@@ -407,10 +345,10 @@ func (k *kubelet) writeKubeletConfigToDir(cfg *api.NodeConfig) error {
 	if len(cfg.Spec.Kubelet.Config) > 0 {
 		dirPath := path.Join(kubeletConfigRoot, kubeletConfigDir)
 		k.flags["config-dir"] = dirPath
-
-		zap.L().Info("Enabling kubelet config drop-in dir..")
-		k.environment["KUBELET_CONFIG_DROPIN_DIR_ALPHA"] = "on"
-		filePath := path.Join(dirPath, "40-nodeadm.conf")
+		if semver.Compare(cfg.Status.KubeletVersion, "v1.30.0") < 0 {
+			zap.L().Info("Enabling kubelet config drop-in dir..")
+			k.environment["KUBELET_CONFIG_DROPIN_DIR_ALPHA"] = "on"
+		}
 
 		// merge in default type metadata like kind and apiVersion in case the
 		// user has not specified this, as it is required to qualify a drop-in
@@ -423,6 +361,7 @@ func (k *kubelet) writeKubeletConfigToDir(cfg *api.NodeConfig) error {
 		if err != nil {
 			return err
 		}
+		filePath := path.Join(dirPath, "40-nodeadm.conf")
 		zap.L().Info("Writing user kubelet config to drop-in file..", zap.String("path", filePath))
 		if err := util.WriteFileWithDir(filePath, userKubeletConfigBytes, kubeletConfigPerm); err != nil {
 			return err

nodeadm/internal/kubelet/config_test.go

@@ -1,10 +1,13 @@
 package kubelet
 
 import (
+	"context"
 	"testing"
 
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/api"
+	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/aws/imds"
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/containerd"
+	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/system"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -13,7 +16,7 @@ func TestKubeletCredentialProvidersFeatureFlag(t *testing.T) {
 		kubeletVersion string
 		expectedValue  *bool
 	}{
-		{kubeletVersion: "v1.28.0"},
+		{kubeletVersion: "v1.35.0"},
 	}
 
 	for _, test := range tests {
@@ -103,3 +106,42 @@ func TestMutableCSINodeAllocatableCountFeatureGate(t *testing.T) {
 		}
 	}
 }
+
+func TestGenerateKubeletConfig(t *testing.T) {
+	mockIMDS := &imds.FakeIMDSClient{
+		GetPropertyFunc: func(ctx context.Context, prop imds.IMDSProperty) (string, error) {
+			if prop == imds.LocalIPv4 {
+				return "10.0.0.1", nil
+			}
+			return "", nil
+		},
+	}
+	k := &kubelet{
+		imdsClient:  mockIMDS,
+		resources:   system.NewResources(system.FakeFileSystem{}),
+		flags:       make(map[string]string),
+		environment: make(map[string]string),
+	}
+	nodeConfig := &api.NodeConfig{
+		Spec: api.NodeConfigSpec{
+			Cluster: api.ClusterDetails{
+				CIDR: "10.100.0.0/16",
+			},
+		},
+		Status: api.NodeConfigStatus{
+			KubeletVersion: "v1.33.0",
+			Instance: api.InstanceDetails{
+				AvailabilityZone: "us-west-2a",
+				ID:               "i-1234567890abcdef0",
+				PrivateDNSName:   "ip-10-0-0-1.us-west-2.compute.internal",
+			},
+		},
+	}
+
+	cfg, err := k.generateKubeletConfig(nodeConfig)
+	assert.NoError(t, err)
+
+	assert.Equal(t, "10.0.0.1", k.flags["node-ip"])
+	assert.Equal(t, "external", k.flags["cloud-provider"])
+	assert.Equal(t, "aws:///us-west-2a/i-1234567890abcdef0", *cfg.ProviderID)
+}

nodeadm/internal/kubelet/daemon.go

@@ -2,6 +2,7 @@ package kubelet
 
 import (
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/api"
+	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/aws/imds"
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/daemon"
 	"github.com/awslabs/amazon-eks-ami/nodeadm/internal/system"
 )
@@ -13,16 +14,18 @@ var _ daemon.Daemon = &kubelet{}
 type kubelet struct {
 	daemonManager daemon.DaemonManager
 	resources     system.Resources
+	imdsClient    imds.IMDSClient
 	// environment variables to write for kubelet
 	environment map[string]string
 	// kubelet config flags without leading dashes
 	flags map[string]string
 }
 
-func NewKubeletDaemon(daemonManager daemon.DaemonManager, resources system.Resources) daemon.Daemon {
+func NewKubeletDaemon(daemonManager daemon.DaemonManager, resources system.Resources, imdsClient imds.IMDSClient) daemon.Daemon {
 	return &kubelet{
 		daemonManager: daemonManager,
 		resources:     resources,
+		imdsClient:    imdsClient,
 		environment:   make(map[string]string),
 		flags:         make(map[string]string),
 	}

nodeadm/test/e2e/cases/custom-max-pods-expression/run.sh

@@ -18,13 +18,3 @@ for SCENARIO in scenarios/*; do
     assert::json-files-equal /etc/kubernetes/kubelet/config.json.d/40-nodeadm.conf "${SCENARIO}/expected-dropin.json"
   fi
 done
-
-# for <= 1.28, user kubelet config was merged into the same final config as the default (rather than written to)
-# a drop-in. confirms that the user maxPods value overrides the result of maxPodsExpression
-mock::kubelet 1.28.0
-
-for SCENARIO in scenarios/*; do
-  nodeadm init --skip run --config-source "file://${SCENARIO}/config.yaml"
-
-  assert::json-files-equal /etc/kubernetes/kubelet/config.json "${SCENARIO}/expected-1-28.json"
-done