From 14f484e5c6fd44c83e36b3a4847ea79a50d6bdd9 Mon Sep 17 00:00:00 2001 From: Ayoub Mrini Date: Thu, 20 Aug 2026 19:04:54 +0200 Subject: [PATCH 1/2] Kubelet TLS changes should not reboot nodes Isolate kubelet TLS settings in a dedicated drop-in file with a NodeDisruptionPolicy that triggers a kubelet restart instead of a full node reboot. --- pkg/apihelpers/apihelpers.go | 22 +++ pkg/controller/kubelet-config/helpers.go | 127 ++++++++++++------ pkg/controller/kubelet-config/helpers_test.go | 69 +++++++++- .../kubelet_config_bootstrap.go | 5 +- .../kubelet_config_controller.go | 25 ++-- .../kubelet_config_controller_test.go | 39 +++--- pkg/daemon/constants/constants.go | 6 + pkg/daemon/update_test.go | 59 ++++++++ .../_base/files/kubelet-tls-dropin.yaml | 12 ++ .../_base/files/kubelet.yaml | 5 - .../_base/files/kubelet-tls-dropin.yaml | 12 ++ .../_base/files/kubelet.yaml | 5 - .../_base/files/kubelet-tls-dropin.yaml | 12 ++ .../_base/files/kubelet.yaml | 5 - test/e2e-2of2/nodedisrupt_test.go | 53 ++++++++ 15 files changed, 364 insertions(+), 92 deletions(-) create mode 100644 templates/arbiter/01-arbiter-kubelet/_base/files/kubelet-tls-dropin.yaml create mode 100644 templates/master/01-master-kubelet/_base/files/kubelet-tls-dropin.yaml create mode 100644 templates/worker/01-worker-kubelet/_base/files/kubelet-tls-dropin.yaml diff --git a/pkg/apihelpers/apihelpers.go b/pkg/apihelpers/apihelpers.go index d3550b655a..4dc7ca1fac 100644 --- a/pkg/apihelpers/apihelpers.go +++ b/pkg/apihelpers/apihelpers.go @@ -168,6 +168,28 @@ var defaultClusterPolicies = opv1.NodeDisruptionPolicyClusterStatus{ }, }, }, + // Kubelet reads its config only at startup, a restart is needed. + { + Path: constants.KubeletTLSDropInPath, + Actions: []opv1.NodeDisruptionPolicyStatusAction{ + { + Type: opv1.RestartStatusAction, + Restart: &opv1.RestartService{ + ServiceName: "kubelet.service", + }, + }, + }, + }, + // Static pod manifests: kubelet watches this directory and automatically + // recreates pods when their manifests change on disk + { + Path: "/etc/kubernetes/manifests", + Actions: []opv1.NodeDisruptionPolicyStatusAction{ + { + Type: opv1.NoneStatusAction, + }, + }, + }, }, Units: []opv1.NodeDisruptionPolicyStatusUnit{ { diff --git a/pkg/controller/kubelet-config/helpers.go b/pkg/controller/kubelet-config/helpers.go index 837de0cb32..13d7e553a9 100644 --- a/pkg/controller/kubelet-config/helpers.go +++ b/pkg/controller/kubelet-config/helpers.go @@ -8,6 +8,7 @@ import ( "strings" ign3types "github.com/coreos/ignition/v2/config/v3_5/types" + ghodssyaml "github.com/ghodss/yaml" "github.com/imdario/mergo" osev1 "github.com/openshift/api/config/v1" corev1 "k8s.io/api/core/v1" @@ -26,6 +27,7 @@ import ( mcfgclientset "github.com/openshift/client-go/machineconfiguration/clientset/versioned" "github.com/openshift/machine-config-operator/pkg/apihelpers" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" + "github.com/openshift/machine-config-operator/pkg/daemon/constants" ) const ( @@ -85,6 +87,31 @@ func createNewKubeletIgnition(yamlConfig []byte) *ign3types.File { return &r } +// kubeletTLSDropIn contains only TLS fields. The full KubeletConfiguration cannot be used +// because it emits non-omitempty fields that would overwrite the base config. +type kubeletTLSDropIn struct { + APIVersion string `json:"apiVersion" yaml:"apiVersion"` + Kind string `json:"kind" yaml:"kind"` + TLSMinVersion string `json:"tlsMinVersion,omitempty" yaml:"tlsMinVersion,omitempty"` + TLSCipherSuites []string `json:"tlsCipherSuites,omitempty" yaml:"tlsCipherSuites,omitempty"` +} + +// createKubeletTLSDropInIgnition builds the TLS drop-in YAML. +func createKubeletTLSDropInIgnition(tlsMinVersion string, tlsCipherSuites []string) (*ign3types.File, error) { + cfg := kubeletTLSDropIn{ + APIVersion: "kubelet.config.k8s.io/v1beta1", + Kind: "KubeletConfiguration", + TLSMinVersion: tlsMinVersion, + TLSCipherSuites: tlsCipherSuites, + } + data, err := ghodssyaml.Marshal(cfg) + if err != nil { + return nil, fmt.Errorf("could not marshal kubelet TLS drop-in config: %w", err) + } + r := ctrlcommon.NewIgnFileBytesOverwriting(constants.KubeletTLSDropInPath, data) + return &r, nil +} + func createNewDefaultFeatureGate() *osev1.FeatureGate { return &osev1.FeatureGate{ ObjectMeta: metav1.ObjectMeta{ @@ -182,17 +209,21 @@ func updateMachineConfigwithCgroup(node *osev1.Node, mc *mcfgv1.MachineConfig) e return nil } -func findKubeletConfig(mc *mcfgv1.MachineConfig) (*ign3types.File, error) { +func findFileInMC(mc *mcfgv1.MachineConfig, path string) (*ign3types.File, error) { ignCfg, err := ctrlcommon.ParseAndConvertConfig(mc.Spec.Config.Raw) if err != nil { - return nil, fmt.Errorf("parsing Kubelet Ignition config failed with error: %w", err) + return nil, fmt.Errorf("parsing Ignition config for %s failed: %w", path, err) } for _, c := range ignCfg.Storage.Files { - if c.Path == "/etc/kubernetes/kubelet.conf" { + if c.Path == path { return &c, nil } } - return nil, fmt.Errorf("could not find Kubelet Config") + return nil, fmt.Errorf("could not find file %s", path) +} + +func findKubeletConfig(mc *mcfgv1.MachineConfig) (*ign3types.File, error) { + return findFileInMC(mc, "/etc/kubernetes/kubelet.conf") } // nolint: dupl @@ -520,8 +551,30 @@ func kubeletConfigToIgnFile(cfg *kubeletconfigv1beta1.KubeletConfiguration) (*ig return cfgIgn, nil } +// filterSystemReservedEnforcement removes system-reserved enforcement keys +// and ensures pods enforcement is always present. +func filterSystemReservedEnforcement(enforceNodeAllocatable []string) []string { + filtered := []string{} + hasPods := false + for _, val := range enforceNodeAllocatable { + if val == kubeletypes.SystemReservedEnforcementKey || + val == kubeletypes.SystemReservedCompressibleEnforcementKey { + continue + } + if val == kubeletypes.NodeAllocatableEnforcementKey { + hasPods = true + } + filtered = append(filtered, val) + } + if !hasPods { + filtered = append([]string{kubeletypes.NodeAllocatableEnforcementKey}, filtered...) + } + return filtered +} + // generateKubeletIgnFiles generates the Ignition files from the kubelet config -func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeConfig *kubeletconfigv1beta1.KubeletConfiguration) (*ign3types.File, *ign3types.File, *ign3types.File, error) { +// TODO: refactor return values to improve readability. +func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeConfig *kubeletconfigv1beta1.KubeletConfiguration) (*ign3types.File, *ign3types.File, *ign3types.File, *ign3types.File, error) { var ( kubeletIgnition *ign3types.File logLevelIgnition *ign3types.File @@ -532,22 +585,14 @@ func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeCo if kubeletConfig.Spec.KubeletConfig != nil && kubeletConfig.Spec.KubeletConfig.Raw != nil { specKubeletConfig, err := DecodeKubeletConfig(kubeletConfig.Spec.KubeletConfig.Raw) if err != nil { - return nil, nil, nil, fmt.Errorf("could not deserialize the new Kubelet config: %w", err) - } - - if val, ok := specKubeletConfig.SystemReserved["memory"]; ok { - userDefinedSystemReserved["memory"] = val - delete(specKubeletConfig.SystemReserved, "memory") - } - - if val, ok := specKubeletConfig.SystemReserved["cpu"]; ok { - userDefinedSystemReserved["cpu"] = val - delete(specKubeletConfig.SystemReserved, "cpu") + return nil, nil, nil, nil, fmt.Errorf("could not deserialize the new Kubelet config: %w", err) } - if val, ok := specKubeletConfig.SystemReserved["ephemeral-storage"]; ok { - userDefinedSystemReserved["ephemeral-storage"] = val - delete(specKubeletConfig.SystemReserved, "ephemeral-storage") + for _, key := range []string{"memory", "cpu", "ephemeral-storage"} { + if val, ok := specKubeletConfig.SystemReserved[key]; ok { + userDefinedSystemReserved[key] = val + delete(specKubeletConfig.SystemReserved, key) + } } // FeatureGates must be set from the FeatureGate. @@ -565,7 +610,7 @@ func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeCo // Merge the Old and New err = mergo.Merge(originalKubeConfig, specKubeletConfig, mergo.WithOverride) if err != nil { - return nil, nil, nil, fmt.Errorf("could not merge original config and new config: %w", err) + return nil, nil, nil, nil, fmt.Errorf("could not merge original config and new config: %w", err) } // Empty strings are ignored by mergo, so we need to set them to empty string for SystemReservedCgroup explicitly @@ -592,37 +637,33 @@ func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeCo if originalKubeConfig.ReservedSystemCPUs != "" { klog.Infof("reservedSystemCPUs is set to %s, disabling systemReservedCgroup enforcement", originalKubeConfig.ReservedSystemCPUs) originalKubeConfig.SystemReservedCgroup = "" - // Filter out system-reserved enforcement keys and ensure pods is present - filtered := []string{} - hasPods := false - for _, val := range originalKubeConfig.EnforceNodeAllocatable { - // Skip system-reserved enforcement keys - if val == kubeletypes.SystemReservedEnforcementKey || - val == kubeletypes.SystemReservedCompressibleEnforcementKey { - continue - } - if val == kubeletypes.NodeAllocatableEnforcementKey { - hasPods = true - } - filtered = append(filtered, val) - } - // Ensure pods enforcement is always present - if !hasPods { - filtered = append([]string{kubeletypes.NodeAllocatableEnforcementKey}, filtered...) - } - originalKubeConfig.EnforceNodeAllocatable = filtered + originalKubeConfig.EnforceNodeAllocatable = filterSystemReservedEnforcement(originalKubeConfig.EnforceNodeAllocatable) } if originalKubeConfig.SystemReservedCgroup != "" && originalKubeConfig.SystemCgroups != "" { if originalKubeConfig.SystemReservedCgroup != originalKubeConfig.SystemCgroups { - return nil, nil, nil, fmt.Errorf("invalid merged configuration: systemReservedCgroup (%s) must match systemCgroups (%s)", originalKubeConfig.SystemReservedCgroup, originalKubeConfig.SystemCgroups) + return nil, nil, nil, nil, fmt.Errorf("invalid merged configuration: systemReservedCgroup (%s) must match systemCgroups (%s)", originalKubeConfig.SystemReservedCgroup, originalKubeConfig.SystemCgroups) + } + } + + // Only emit a TLS drop-in when TLS values are explicitly set. + var ( + tlsDropInIgnition *ign3types.File + err error + ) + if originalKubeConfig.TLSMinVersion != "" || len(originalKubeConfig.TLSCipherSuites) > 0 { + tlsDropInIgnition, err = createKubeletTLSDropInIgnition(originalKubeConfig.TLSMinVersion, originalKubeConfig.TLSCipherSuites) + if err != nil { + return nil, nil, nil, nil, err } } + originalKubeConfig.TLSMinVersion = "" + originalKubeConfig.TLSCipherSuites = nil // Encode the new config into an Ignition File - kubeletIgnition, err := kubeletConfigToIgnFile(originalKubeConfig) + kubeletIgnition, err = kubeletConfigToIgnFile(originalKubeConfig) if err != nil { - return nil, nil, nil, fmt.Errorf("could not encode JSON: %w", err) + return nil, nil, nil, nil, fmt.Errorf("could not encode JSON: %w", err) } if kubeletConfig.Spec.LogLevel != nil { @@ -635,5 +676,5 @@ func generateKubeletIgnFiles(kubeletConfig *mcfgv1.KubeletConfig, originalKubeCo autoSizingReservedIgnition = createNewKubeletDynamicSystemReservedIgnition(nil, userDefinedSystemReserved) } - return kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, nil + return kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, tlsDropInIgnition, nil } diff --git a/pkg/controller/kubelet-config/helpers_test.go b/pkg/controller/kubelet-config/helpers_test.go index e4360599af..4cab6c463e 100644 --- a/pkg/controller/kubelet-config/helpers_test.go +++ b/pkg/controller/kubelet-config/helpers_test.go @@ -176,7 +176,7 @@ func TestReserveSystemCPUs(t *testing.T) { Spec: mcfgv1.KubeletConfigSpec{}, } - kubeletIgnition, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + kubeletIgnition, _, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) require.NoError(t, err, "generateKubeletIgnFiles should not return an error") require.NotNil(t, kubeletIgnition, "kubelet ignition file should not be nil") @@ -232,7 +232,7 @@ func TestCGroupKubeletConfigSpec(t *testing.T) { } // Execute: Generate the kubelet ignition files - kubeletIgnition, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + kubeletIgnition, _, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) require.NoError(t, err, "generateKubeletIgnFiles should not return an error") require.NotNil(t, kubeletIgnition, "kubelet ignition file should not be nil") @@ -290,7 +290,7 @@ func TestEmptyStringOverride(t *testing.T) { }, } - kubeletIgnition, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + kubeletIgnition, _, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) require.NoError(t, err, "generateKubeletIgnFiles should not return an error") require.NotNil(t, kubeletIgnition, "kubelet ignition file should not be nil") @@ -343,7 +343,7 @@ func TestPartialUserConfig(t *testing.T) { }, } - kubeletIgnition, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + kubeletIgnition, _, _, _, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) require.NoError(t, err, "generateKubeletIgnFiles should not return an error") require.NotNil(t, kubeletIgnition, "kubelet ignition file should not be nil") @@ -391,8 +391,67 @@ func TestSystemCgroupsMismatch(t *testing.T) { }, } - _, _, _, err = generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + _, _, _, _, err = generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) require.Error(t, err, "generateKubeletIgnFiles should return an error for mismatched cgroups") require.Contains(t, err.Error(), "systemReservedCgroup (/system.slice) must match systemCgroups (/foo.slice)", "error message should indicate cgroup mismatch") } + +func TestTLSDropInOnlyEmittedWhenSet(t *testing.T) { + tests := []struct { + name string + tlsMinVersion string + tlsCipherSuites []string + expectDropIn bool + }{ + { + name: "no TLS set, no drop-in emitted", + expectDropIn: false, + }, + { + name: "only tlsMinVersion set", + tlsMinVersion: "VersionTLS12", + expectDropIn: true, + }, + { + name: "only tlsCipherSuites set", + tlsCipherSuites: []string{"TLS_AES_128_GCM_SHA256"}, + expectDropIn: true, + }, + { + name: "both set", + tlsMinVersion: "VersionTLS13", + tlsCipherSuites: []string{"TLS_AES_128_GCM_SHA256", "TLS_AES_256_GCM_SHA384"}, + expectDropIn: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + originalKubeConfig := &kubeletconfigv1beta1.KubeletConfiguration{ + TLSMinVersion: tc.tlsMinVersion, + TLSCipherSuites: tc.tlsCipherSuites, + } + kubeletConfig := &mcfgv1.KubeletConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "test"}, + Spec: mcfgv1.KubeletConfigSpec{}, + } + + _, _, _, tlsDropIn, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + require.NoError(t, err) + + if !tc.expectDropIn { + require.Nil(t, tlsDropIn, "TLS drop-in should not be emitted when TLS is unset") + return + } + + require.NotNil(t, tlsDropIn, "TLS drop-in should be emitted") + contents, err := ctrlcommon.DecodeIgnitionFileContents(tlsDropIn.Contents.Source, tlsDropIn.Contents.Compression) + require.NoError(t, err) + kc, err := DecodeKubeletConfig(contents) + require.NoError(t, err) + require.Equal(t, tc.tlsMinVersion, kc.TLSMinVersion) + require.Equal(t, tc.tlsCipherSuites, kc.TLSCipherSuites) + }) + } +} diff --git a/pkg/controller/kubelet-config/kubelet_config_bootstrap.go b/pkg/controller/kubelet-config/kubelet_config_bootstrap.go index caef8cde1c..e3007c8c9f 100644 --- a/pkg/controller/kubelet-config/kubelet_config_bootstrap.go +++ b/pkg/controller/kubelet-config/kubelet_config_bootstrap.go @@ -59,7 +59,7 @@ func RunKubeletBootstrap(templateDir string, kubeletConfigs []*mcfgv1.KubeletCon originalKubeConfig.TLSCipherSuites = observedCipherSuites } - kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) + kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, tlsDropInIgnition, err := generateKubeletIgnFiles(kubeletConfig, originalKubeConfig) if err != nil { return nil, err } @@ -74,6 +74,9 @@ func RunKubeletBootstrap(templateDir string, kubeletConfigs []*mcfgv1.KubeletCon if kubeletIgnition != nil { tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *kubeletIgnition) } + if tlsDropInIgnition != nil { + tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *tlsDropInIgnition) + } rawIgn, err := json.Marshal(tempIgnConfig) if err != nil { diff --git a/pkg/controller/kubelet-config/kubelet_config_controller.go b/pkg/controller/kubelet-config/kubelet_config_controller.go index 009b1381d4..d5193403a6 100644 --- a/pkg/controller/kubelet-config/kubelet_config_controller.go +++ b/pkg/controller/kubelet-config/kubelet_config_controller.go @@ -439,23 +439,25 @@ func generateOriginalKubeletConfigWithFeatureGates(cc *mcfgv1.ControllerConfig, return originalKubeConfig, nil } -func generateOriginalKubeletConfigIgn(cc *mcfgv1.ControllerConfig, templatesDir, role string, apiServer *osev1.APIServer) (*ign3types.File, error) { - // Render the default templates +// renderAndExtractFile renders templates for the given role and extracts +// the file at path from the generated MachineConfigs. +func renderAndExtractFile(cc *mcfgv1.ControllerConfig, templatesDir, role string, apiServer *osev1.APIServer, path string) (*ign3types.File, error) { tlsMinVersion, tlsCipherSuites := ctrlcommon.GetSecurityProfileCiphersFromAPIServer(apiServer) rc := &mtmpl.RenderConfig{ControllerConfigSpec: &cc.Spec, TLSMinVersion: tlsMinVersion, TLSCipherSuites: tlsCipherSuites} generatedConfigs, err := mtmpl.GenerateMachineConfigsForRole(rc, role, templatesDir) if err != nil { - return nil, fmt.Errorf("GenerateMachineConfigsforRole failed with error: %w", err) + return nil, fmt.Errorf("GenerateMachineConfigsForRole failed with error: %w", err) } - // Find generated kubelet.config for _, gmc := range generatedConfigs { - gmcKubeletConfig, err := findKubeletConfig(gmc) - if err != nil { - continue + if f, err := findFileInMC(gmc, path); err == nil { + return f, nil } - return gmcKubeletConfig, nil } - return nil, fmt.Errorf("could not generate old kubelet config") + return nil, fmt.Errorf("could not find %s in rendered templates", path) +} + +func generateOriginalKubeletConfigIgn(cc *mcfgv1.ControllerConfig, templatesDir, role string, apiServer *osev1.APIServer) (*ign3types.File, error) { + return renderAndExtractFile(cc, templatesDir, role, apiServer, "/etc/kubernetes/kubelet.conf") } // syncStatusOnly updates the status conditions of a KubeletConfig CR without modifying its spec. @@ -669,7 +671,7 @@ func (ctrl *Controller) syncKubeletConfig(key string) error { originalKubeConfig.TLSCipherSuites = observedCipherSuites } - kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, err := generateKubeletIgnFiles(cfg, originalKubeConfig) + kubeletIgnition, logLevelIgnition, autoSizingReservedIgnition, tlsDropInIgnition, err := generateKubeletIgnFiles(cfg, originalKubeConfig) if err != nil { return ctrl.syncStatusOnly(cfg, err) } @@ -712,6 +714,9 @@ func (ctrl *Controller) syncKubeletConfig(key string) error { if kubeletIgnition != nil { tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *kubeletIgnition) } + if tlsDropInIgnition != nil { + tempIgnConfig.Storage.Files = append(tempIgnConfig.Storage.Files, *tlsDropInIgnition) + } rawIgn, err := json.Marshal(tempIgnConfig) if err != nil { diff --git a/pkg/controller/kubelet-config/kubelet_config_controller_test.go b/pkg/controller/kubelet-config/kubelet_config_controller_test.go index 9eb9f2e777..962aea82f2 100644 --- a/pkg/controller/kubelet-config/kubelet_config_controller_test.go +++ b/pkg/controller/kubelet-config/kubelet_config_controller_test.go @@ -36,6 +36,7 @@ import ( informers "github.com/openshift/client-go/machineconfiguration/informers/externalversions" mcfglistersv1 "github.com/openshift/client-go/machineconfiguration/listers/machineconfiguration/v1" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" + "github.com/openshift/machine-config-operator/pkg/daemon/constants" "github.com/openshift/machine-config-operator/pkg/version" "github.com/openshift/machine-config-operator/test/helpers" ) @@ -1636,7 +1637,8 @@ func TestCleanUpDuplicatedMC(t *testing.T) { } } -// This test ensures that that the rendering works properly for all types of TLS security profiles +// TestKubeletConfigTLSRender verifies that template rendering produces the correct TLS drop-in +// for each security profile type (Old, Intermediate, Custom, nil/empty). func TestKubeletConfigTLSRender(t *testing.T) { TestCases := []struct { name string @@ -1704,14 +1706,15 @@ func TestKubeletConfigTLSRender(t *testing.T) { fgHandler := ctrlcommon.NewFeatureGatesHardcodedHandler([]osev1.FeatureGateName{"Example"}, nil) ctrl := f.newController(fgHandler) - kubeletConfig, err := generateOriginalKubeletConfigIgn(cc, ctrl.templatesDir, "master", testCase.apiserver) - if err != nil { - t.Errorf("could not generate kubelet config from templates %v", err) - } - contents, err := ctrlcommon.DecodeIgnitionFileContents(kubeletConfig.Contents.Source, kubeletConfig.Contents.Compression) + tlsDropIn, err := renderAndExtractFile(cc, ctrl.templatesDir, "master", testCase.apiserver, constants.KubeletTLSDropInPath) + require.NoError(t, err, "could not generate TLS drop-in from templates") + require.NotNil(t, tlsDropIn, "TLS drop-in file should not be nil") + + contents, err := ctrlcommon.DecodeIgnitionFileContents(tlsDropIn.Contents.Source, tlsDropIn.Contents.Compression) require.NoError(t, err) - originalKubeConfig, err := DecodeKubeletConfig(contents) + tlsKubeConfig, err := DecodeKubeletConfig(contents) require.NoError(t, err) + // Generate expected profiles from helper function var expectedTLSCipherSuites []string var expectedTLSMinVersion string @@ -1722,14 +1725,14 @@ func TestKubeletConfigTLSRender(t *testing.T) { expectedTLSMinVersion, expectedTLSCipherSuites = ctrlcommon.GetSecurityProfileCiphers(testCase.apiserver.Spec.TLSSecurityProfile) } - require.Equal(t, originalKubeConfig.TLSCipherSuites, expectedTLSCipherSuites) - require.Equal(t, originalKubeConfig.TLSMinVersion, expectedTLSMinVersion) + require.Equal(t, tlsKubeConfig.TLSCipherSuites, expectedTLSCipherSuites) + require.Equal(t, tlsKubeConfig.TLSMinVersion, expectedTLSMinVersion) }) } } -// This test ensures that a user defined kubeletConfiguration with a TLS profile will override -// the TLS profile specified by the APIServer object +// TestKubeletConfigTLSOverride verifies that KubeletConfig.spec.tlsSecurityProfile +// takes precedence over APIServer.spec.tlsSecurityProfile in the TLS drop-in. func TestKubeletConfigTLSOverride(t *testing.T) { f := newFixture(t) fgHandler := ctrlcommon.NewFeatureGatesHardcodedHandler([]osev1.FeatureGateName{"Example"}, nil) @@ -1762,22 +1765,22 @@ func TestKubeletConfigTLSOverride(t *testing.T) { t.Errorf("syncHandler returned: %v", err) } - // Grab the MachineConfig generated by the controller, and decode the content of the kubelet config file + // Grab the MachineConfig generated by the controller, and decode the TLS drop-in file generatedConfig, err := ctrl.client.MachineconfigurationV1().MachineConfigs().Get(context.TODO(), "99-master-generated-kubelet", metav1.GetOptions{}) require.NoError(t, err) generatedIgnConfig, err := ctrlcommon.ParseAndConvertConfig(generatedConfig.Spec.Config.Raw) require.NoError(t, err) - var kubeletConfigFile ign3types.File + var tlsDropInFile ign3types.File for _, file := range generatedIgnConfig.Storage.Files { - if file.Path == "/etc/kubernetes/kubelet.conf" { - kubeletConfigFile = file + if file.Path == constants.KubeletTLSDropInPath { + tlsDropInFile = file } } - require.NotEmpty(t, kubeletConfigFile) + require.NotEmpty(t, tlsDropInFile, "TLS drop-in file should be present in generated MachineConfig") - // Decode this to a native KubeletConfiguration object - contents, err := ctrlcommon.DecodeIgnitionFileContents(kubeletConfigFile.Contents.Source, kubeletConfigFile.Contents.Compression) + // Decode to a native KubeletConfiguration object + contents, err := ctrlcommon.DecodeIgnitionFileContents(tlsDropInFile.Contents.Source, tlsDropInFile.Contents.Compression) require.NoError(t, err) kc, err := DecodeKubeletConfig(contents) require.NoError(t, err) diff --git a/pkg/daemon/constants/constants.go b/pkg/daemon/constants/constants.go index 03411fb0ec..746481815c 100644 --- a/pkg/daemon/constants/constants.go +++ b/pkg/daemon/constants/constants.go @@ -153,6 +153,12 @@ const ( IRIRegistryServiceName = "iri-registry.service" IRIRegistryDataPath = "/var/lib/iri-registry" + // KubeletTLSDropInPath is the kubelet TLS drop-in (tlsMinVersion, tlsCipherSuites). + // TODO: decide on the "99" prefix — it gives TLS highest precedence in kubelet's + // --config-dir merge order (alphabetical, last wins). Revisit whether a lower prefix + // (like CRI-O's "01-ctrcfg-*" pattern) is more appropriate. + KubeletTLSDropInPath = "/etc/openshift/kubelet.conf.d/99-tls.conf" + // rpm-ostree command arguments RPMOSTreeUpdateArg = "update" RPMOSTreeInstallArg = "--install" diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index c15b9ba10f..001198ce7d 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -17,6 +17,7 @@ import ( ign3types "github.com/coreos/ignition/v2/config/v3_5/types" mcfgv1 "github.com/openshift/api/machineconfiguration/v1" opv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/machine-config-operator/pkg/apihelpers" ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common" "github.com/openshift/machine-config-operator/pkg/daemon/constants" "github.com/openshift/machine-config-operator/pkg/daemon/osrelease" @@ -681,6 +682,64 @@ func TestCalculatePostConfigChangeAction(t *testing.T) { } } +// TestNodeDisruptionPolicyStaticPodManifests verifies that changes to files under +// /etc/kubernetes/manifests resolve to "None" via the default NodeDisruptionPolicy. +func TestNodeDisruptionPolicyStaticPodManifests(t *testing.T) { + // Get the default cluster policies (no user overrides). + clusterPolicies := apihelpers.MergeClusterPolicies(opv1.NodeDisruptionPolicyConfig{}) + + tests := []struct { + name string + diffFileSet []string + expectedActions []opv1.NodeDisruptionPolicyStatusAction + }{ + { + name: "static pod manifest change alone results in None", + diffFileSet: []string{"/etc/kubernetes/manifests/criometricsproxy.yaml"}, + expectedActions: []opv1.NodeDisruptionPolicyStatusAction{ + {Type: opv1.NoneStatusAction}, + }, + }, + { + name: "any file under /etc/kubernetes/manifests results in None", + diffFileSet: []string{"/etc/kubernetes/manifests/some-new-static-pod.yaml"}, + expectedActions: []opv1.NodeDisruptionPolicyStatusAction{ + {Type: opv1.NoneStatusAction}, + }, + }, + { + name: "TLS drop-in + static pod manifest = restart kubelet (None is stripped)", + diffFileSet: []string{ + constants.KubeletTLSDropInPath, + "/etc/kubernetes/manifests/criometricsproxy.yaml", + }, + expectedActions: []opv1.NodeDisruptionPolicyStatusAction{ + { + Type: opv1.RestartStatusAction, + Restart: &opv1.RestartService{ + ServiceName: "kubelet.service", + }, + }, + }, + }, + { + name: "unknown file without policy defaults to Reboot", + diffFileSet: []string{"/etc/some-random-file"}, + expectedActions: []opv1.NodeDisruptionPolicyStatusAction{ + {Type: opv1.RebootStatusAction}, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actions := calculatePostConfigChangeNodeDisruptionActionFromMCDiffs( + false, tc.diffFileSet, nil, clusterPolicies) + require.Equal(t, tc.expectedActions, actions) + }) + } +} + // checkReconcilableResults is a shortcut for verifying results that should be reconcilable func checkReconcilableResults(t *testing.T, key string, reconcilableError error) { if reconcilableError != nil { diff --git a/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet-tls-dropin.yaml b/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet-tls-dropin.yaml new file mode 100644 index 0000000000..bdd9aeb699 --- /dev/null +++ b/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet-tls-dropin.yaml @@ -0,0 +1,12 @@ +# Kubelet TLS drop-in, merged via --config-dir. +mode: 0644 +path: "/etc/openshift/kubelet.conf.d/99-tls.conf" +contents: + inline: | + apiVersion: kubelet.config.k8s.io/v1beta1 + kind: KubeletConfiguration + tlsMinVersion: {{.TLSMinVersion}} + tlsCipherSuites: + {{- range .TLSCipherSuites }} + - {{ . }} + {{- end }} diff --git a/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet.yaml b/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet.yaml index e536c6f63f..395bbe98c8 100644 --- a/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet.yaml +++ b/templates/arbiter/01-arbiter-kubelet/_base/files/kubelet.yaml @@ -31,8 +31,3 @@ contents: nodeStatusUpdateFrequency: 10s nodeStatusReportFrequency: 5m serverTLSBootstrap: true - tlsMinVersion: {{.TLSMinVersion}} - tlsCipherSuites: - {{- range .TLSCipherSuites }} - - {{ . }} - {{- end }} diff --git a/templates/master/01-master-kubelet/_base/files/kubelet-tls-dropin.yaml b/templates/master/01-master-kubelet/_base/files/kubelet-tls-dropin.yaml new file mode 100644 index 0000000000..bdd9aeb699 --- /dev/null +++ b/templates/master/01-master-kubelet/_base/files/kubelet-tls-dropin.yaml @@ -0,0 +1,12 @@ +# Kubelet TLS drop-in, merged via --config-dir. +mode: 0644 +path: "/etc/openshift/kubelet.conf.d/99-tls.conf" +contents: + inline: | + apiVersion: kubelet.config.k8s.io/v1beta1 + kind: KubeletConfiguration + tlsMinVersion: {{.TLSMinVersion}} + tlsCipherSuites: + {{- range .TLSCipherSuites }} + - {{ . }} + {{- end }} diff --git a/templates/master/01-master-kubelet/_base/files/kubelet.yaml b/templates/master/01-master-kubelet/_base/files/kubelet.yaml index e536c6f63f..395bbe98c8 100644 --- a/templates/master/01-master-kubelet/_base/files/kubelet.yaml +++ b/templates/master/01-master-kubelet/_base/files/kubelet.yaml @@ -31,8 +31,3 @@ contents: nodeStatusUpdateFrequency: 10s nodeStatusReportFrequency: 5m serverTLSBootstrap: true - tlsMinVersion: {{.TLSMinVersion}} - tlsCipherSuites: - {{- range .TLSCipherSuites }} - - {{ . }} - {{- end }} diff --git a/templates/worker/01-worker-kubelet/_base/files/kubelet-tls-dropin.yaml b/templates/worker/01-worker-kubelet/_base/files/kubelet-tls-dropin.yaml new file mode 100644 index 0000000000..bdd9aeb699 --- /dev/null +++ b/templates/worker/01-worker-kubelet/_base/files/kubelet-tls-dropin.yaml @@ -0,0 +1,12 @@ +# Kubelet TLS drop-in, merged via --config-dir. +mode: 0644 +path: "/etc/openshift/kubelet.conf.d/99-tls.conf" +contents: + inline: | + apiVersion: kubelet.config.k8s.io/v1beta1 + kind: KubeletConfiguration + tlsMinVersion: {{.TLSMinVersion}} + tlsCipherSuites: + {{- range .TLSCipherSuites }} + - {{ . }} + {{- end }} diff --git a/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml b/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml index a67b686070..2909b7aad5 100644 --- a/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml +++ b/templates/worker/01-worker-kubelet/_base/files/kubelet.yaml @@ -35,8 +35,3 @@ contents: nodeStatusUpdateFrequency: 10s nodeStatusReportFrequency: 5m serverTLSBootstrap: true - tlsMinVersion: {{.TLSMinVersion}} - tlsCipherSuites: - {{- range .TLSCipherSuites }} - - {{ . }} - {{- end }} diff --git a/test/e2e-2of2/nodedisrupt_test.go b/test/e2e-2of2/nodedisrupt_test.go index 5c67c85a6d..b51613930e 100644 --- a/test/e2e-2of2/nodedisrupt_test.go +++ b/test/e2e-2of2/nodedisrupt_test.go @@ -5,10 +5,12 @@ import ( "context" "fmt" "testing" + "time" "github.com/BurntSushi/toml" "github.com/containers/image/v5/pkg/sysregistriesv2" + configv1 "github.com/openshift/api/config/v1" v1 "github.com/openshift/api/machineconfiguration/v1" opv1 "github.com/openshift/api/operator/v1" mcoac "github.com/openshift/client-go/operator/applyconfigurations/operator/v1" @@ -23,6 +25,7 @@ import ( ign3types "github.com/coreos/ignition/v2/config/v3_5/types" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" ) const ( @@ -227,3 +230,53 @@ func checkNodeDisruptionAction(t *testing.T, cs *framework.ClientSet, testMC *v1 t.Logf("Successfully verified %s", expectedAction) } + +// TestKubeletTLSProfileChangeNoReboot verifies that changing the kubelet TLS +// security profile triggers a kubelet restart, not a reboot. +func TestKubeletTLSProfileChangeNoReboot(t *testing.T) { + cs := framework.NewClientSet("") + ctx := context.Background() + + node := helpers.GetRandomNode(t, cs, "worker") + initialUptime := helpers.GetNodeUptime(t, cs, node) + mcdPod, err := helpers.MCDForNode(cs, &node) + require.NoError(t, err) + + oldRenderedMC := helpers.GetMcName(t, cs, "worker") + + kcName := "test-tls-no-reboot" + kc := &v1.KubeletConfig{ + ObjectMeta: metav1.ObjectMeta{Name: kcName}, + Spec: v1.KubeletConfigSpec{ + MachineConfigPoolSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{"pools.operator.machineconfiguration.openshift.io/worker": ""}, + }, + TLSSecurityProfile: &configv1.TLSSecurityProfile{Type: configv1.TLSProfileOldType}, + }, + } + _, err = cs.KubeletConfigs().Create(ctx, kc, metav1.CreateOptions{}) + require.NoError(t, err) + t.Cleanup(func() { + _ = cs.KubeletConfigs().Delete(ctx, kcName, metav1.DeleteOptions{}) + helpers.WaitForPoolComplete(t, cs, "worker", oldRenderedMC) + }) + + // Wait for the pool to fully converge. + var newRenderedMC string + err = wait.PollUntilContextTimeout(ctx, 5*time.Second, 10*time.Minute, true, func(ctx context.Context) (bool, error) { + mcp, err := cs.MachineConfigPools().Get(ctx, "worker", metav1.GetOptions{}) + if err != nil { + return false, err + } + if mcp.Status.Configuration.Name != oldRenderedMC && mcp.Status.Configuration.Name != "" { + newRenderedMC = mcp.Status.Configuration.Name + return true, nil + } + return false, nil + }) + require.NoError(t, err, "timed out waiting for worker pool to converge") + + helpers.AssertNodeNotReboot(t, cs, node, initialUptime) + helpers.AssertMCDLogsContain(t, cs, mcdPod, &node, + fmt.Sprintf("Performing post config change action: Restart for config %s", newRenderedMC)) +} From 95d48a4edc9bdd199ca0c78dabfe0038b004b4ae Mon Sep 17 00:00:00 2001 From: Ayoub Mrini Date: Fri, 21 Aug 2026 16:22:17 +0200 Subject: [PATCH 2/2] test/extended-priv: add no-reboot assertions to TLS security tests Add checkRebootAction(false, ...) after each MCP rollout in tests 75222 and 75543 to verify that TLS profile changes do not trigger node reboots. These tests are currently excluded (OCPBUGS-76990), but adding the assertions now ensures the non-disruptive behavior is validated once they are re-enabled. --- test/extended-priv/mco_security.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/extended-priv/mco_security.go b/test/extended-priv/mco_security.go index 1cc357e40b..dcf4075db4 100644 --- a/test/extended-priv/mco_security.go +++ b/test/extended-priv/mco_security.go @@ -552,6 +552,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati var ( apiServer = NewResource(oc.AsAdmin(), "apiserver", "cluster") + node = wMcp.GetSortedNodesOrFail()[0] ) exutil.By("Verify for Intermediate TLS Profile") @@ -578,6 +579,8 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati logger.Infof("OK!\n") }(apiServer.GetSpecOrFail()) + startTime := node.GetDateOrFail() + exutil.By("Patch the Custom tlsSecurityProfile") o.Expect(apiServer.Patch("json", `[{ "op": "add", "path": "/spec/tlsSecurityProfile", "value": {"type": "Custom","custom": {"ciphers": ["ECDHE-ECDSA-CHACHA20-POLY1305","ECDHE-RSA-CHACHA20-POLY1305", "ECDHE-RSA-AES128-GCM-SHA256", "ECDHE-ECDSA-AES128-GCM-SHA256" ],"minTLSVersion": "VersionTLS11"}}}]`)).To(o.Succeed(), "Error patching tlsSecurityProfile") @@ -591,6 +594,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati mMcp.waitForComplete() wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("Verify for Custom TLS Profile") customCipherSuite := []string{"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256", "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256", "TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256"} @@ -609,6 +613,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati "The controller pod didn't acquire the lease properly.") mMcp.waitForComplete() wMcp.waitForComplete() + checkRebootAction(false, node, startTime) exutil.By("Verify for Old TLS Profile") csNameList = getCipherSuitesNameforSpecificVersion(VersionTLS10) @@ -627,6 +632,8 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati mMcp.waitForComplete() wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) + exutil.By("Verify for Modern TLS Profile") csNameList = getCipherSuitesNameforSpecificVersion(VersionTLS13) validateCorrectTLSProfileSecurity(oc, "Modern", "VersionTLS13", csNameList) @@ -669,6 +676,8 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati logger.Infof("OK!\n") }(apiServer.GetSpecOrFail()) + startTime := node.GetDateOrFail() + exutil.By("Patch the Old tlsSecurityProfile") o.Expect(apiServer.Patch("json", `[{ "op": "add", "path": "/spec/tlsSecurityProfile", "value": {"type": "Old","old": {}}}]`)).To(o.Succeed(), "Error patching http proxy") @@ -681,6 +690,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati mMcp.waitForComplete() wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("Verify for Old TLS Profile in kubeletConfig") csVersion10 := getCipherSuitesNameforSpecificVersion(VersionTLS10) @@ -699,6 +709,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati exutil.By("Wait for Worker MachineConfigPool to be updated") wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("Verify for Custom TLS Profile in kubeletConfig") validateCorrectTLSProfileSecurityInKubeletConfig(node, "VersionTLS11", customCipherSuite) @@ -716,6 +727,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati mMcp.waitForComplete() wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("Verify for Modern TLS Profile") csVersion13 := getCipherSuitesNameforSpecificVersion(VersionTLS13) @@ -736,6 +748,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati mMcp.waitForComplete() wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("Verify for Intermediate TLS Profile pod logs") validateCorrectTLSProfileSecurity(oc, "Intermediate", "VersionTLS12", csVersion12) @@ -747,6 +760,7 @@ var _ = g.Describe("[sig-mco][Suite:openshift/machine-config-operator/longdurati o.Expect(kc).NotTo(Exist()) wMcp.waitForComplete() logger.Infof("OK!\n") + checkRebootAction(false, node, startTime) exutil.By("To check the kubeletConfig to have same tls setting as of API server") validateCorrectTLSProfileSecurityInKubeletConfig(node, "VersionTLS12", csVersion12)