[node-agent] Delete systemd unit files and drop-ins only if the unit has been created by node-agent (gardener#11015)

* Fix: Reduce file permissions for last applied OSC

* Ingest containerd drop-in into OSC instead of writing it directly to the disk

This prevents side effects when the containerd.service unit is changed by extensions too.

* Delete systemd unit files and drop-ins only if the unit has been created by node-agent

* Address PR review feedback

* Address PR review feedback v2

Author: oliver-goetz

pkg/nodeagent/controller/operatingsystemconfig/changes.go

@@ -31,18 +31,18 @@ func init() {
 	decoder = serializer.NewCodecFactory(scheme).UniversalDeserializer()
 }
 
-func extractOSCFromSecret(secret *corev1.Secret) (*extensionsv1alpha1.OperatingSystemConfig, []byte, string, error) {
+func extractOSCFromSecret(secret *corev1.Secret) (*extensionsv1alpha1.OperatingSystemConfig, string, error) {
 	oscRaw, ok := secret.Data[nodeagentv1alpha1.DataKeyOperatingSystemConfig]
 	if !ok {
-		return nil, nil, "", fmt.Errorf("no %s key found in OSC secret", nodeagentv1alpha1.DataKeyOperatingSystemConfig)
+		return nil, "", fmt.Errorf("no %s key found in OSC secret", nodeagentv1alpha1.DataKeyOperatingSystemConfig)
 	}
 
 	osc := &extensionsv1alpha1.OperatingSystemConfig{}
 	if err := runtime.DecodeInto(decoder, oscRaw, osc); err != nil {
-		return nil, nil, "", fmt.Errorf("unable to decode OSC from secret data key %s: %w", nodeagentv1alpha1.DataKeyOperatingSystemConfig, err)
+		return nil, "", fmt.Errorf("unable to decode OSC from secret data key %s: %w", nodeagentv1alpha1.DataKeyOperatingSystemConfig, err)
 	}
 
-	return osc, oscRaw, secret.Annotations[nodeagentv1alpha1.AnnotationKeyChecksumDownloadedOperatingSystemConfig], nil
+	return osc, secret.Annotations[nodeagentv1alpha1.AnnotationKeyChecksumDownloadedOperatingSystemConfig], nil
 }
 
 type operatingSystemConfigChanges struct {

pkg/nodeagent/controller/operatingsystemconfig/containerd.go

@@ -25,6 +25,7 @@ import (
 	"github.com/spf13/afero"
 	"k8s.io/utils/ptr"
 
+	v1beta1constants "github.com/gardener/gardener/pkg/apis/core/v1beta1/constants"
 	extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1"
 	extensionsv1alpha1helper "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1/helper"
 	"github.com/gardener/gardener/pkg/utils/flow"
@@ -33,8 +34,8 @@ import (
 )
 
 // ReconcileContainerdConfig sets required values of the given containerd configuration.
-func (r *Reconciler) ReconcileContainerdConfig(ctx context.Context, log logr.Logger, criConfig *extensionsv1alpha1.CRIConfig) error {
-	if !extensionsv1alpha1helper.HasContainerdConfiguration(criConfig) {
+func (r *Reconciler) ReconcileContainerdConfig(ctx context.Context, log logr.Logger, osc *extensionsv1alpha1.OperatingSystemConfig) error {
+	if !extensionsv1alpha1helper.HasContainerdConfiguration(osc.Spec.CRIConfig) {
 		return nil
 	}
 
@@ -46,14 +47,13 @@ func (r *Reconciler) ReconcileContainerdConfig(ctx context.Context, log logr.Log
 		return fmt.Errorf("failed to ensure containerd default config: %w", err)
 	}
 
-	if err := r.ensureContainerdEnvironment(); err != nil {
-		return fmt.Errorf("failed to ensure containerd environment: %w", err)
-	}
-
-	if err := r.ensureContainerdConfiguration(log, criConfig); err != nil {
+	if err := r.ensureContainerdConfiguration(log, osc.Spec.CRIConfig); err != nil {
 		return fmt.Errorf("failed to ensure containerd config: %w", err)
 	}
 
+	// Add the containerd drop-in to the OSC to prevent side effects when containerd.service is changed by extensions too.
+	addContainerdEnvironmentDropIn(osc)
+
 	return nil
 }
 
@@ -81,11 +81,36 @@ func (r *Reconciler) ReconcileContainerdRegistries(ctx context.Context, log logr
 	}
 }
 
+// addContainerdEnvironmentDropIn ingests a drop-in to set the environment for the 'containerd' service.
+func addContainerdEnvironmentDropIn(osc *extensionsv1alpha1.OperatingSystemConfig) {
+	if osc.Spec.CRIConfig == nil {
+		return
+	}
+
+	unitDropIn := extensionsv1alpha1.DropIn{
+		Name: "30-env_config.conf",
+		Content: `[Service]
+Environment="PATH=` + extensionsv1alpha1.ContainerDRuntimeContainersBinFolder + `:` + os.Getenv("PATH") + `"
+`,
+	}
+
+	for i, unit := range osc.Spec.Units {
+		if unit.Name == v1beta1constants.OperatingSystemConfigUnitNameContainerDService {
+			osc.Spec.Units[i].DropIns = append(osc.Spec.Units[i].DropIns, unitDropIn)
+			return
+		}
+	}
+
+	osc.Spec.Units = append(osc.Spec.Units, extensionsv1alpha1.Unit{
+		Name:    v1beta1constants.OperatingSystemConfigUnitNameContainerDService,
+		DropIns: []extensionsv1alpha1.DropIn{unitDropIn},
+	})
+}
+
 const (
 	baseDir   = "/etc/containerd"
 	certsDir  = baseDir + "/certs.d"
 	configDir = baseDir + "/conf.d"
-	dropinDir = "/etc/systemd/system/containerd.service.d"
 )
 
 func (r *Reconciler) ensureContainerdConfigDirectories() error {
@@ -94,7 +119,6 @@ func (r *Reconciler) ensureContainerdConfigDirectories() error {
 		baseDir,
 		configDir,
 		certsDir,
-		dropinDir,
 	} {
 		if err := r.FS.MkdirAll(dir, defaultDirPermissions); err != nil {
 			return fmt.Errorf("failure for directory %q: %w", dir, err)
@@ -130,32 +154,6 @@ func (r *Reconciler) ensureContainerdDefaultConfig(ctx context.Context) error {
 	return r.FS.WriteFile(configFile, output, 0644)
 }
 
-// ensureContainerdEnvironment sets the environment for the 'containerd' service.
-func (r *Reconciler) ensureContainerdEnvironment() error {
-	var (
-		unitDropin = `[Service]
-Environment="PATH=` + extensionsv1alpha1.ContainerDRuntimeContainersBinFolder + `:` + os.Getenv("PATH") + `"
-`
-	)
-
-	containerdEnvFilePath := path.Join(dropinDir, "30-env_config.conf")
-	exists, err := r.FS.Exists(containerdEnvFilePath)
-	if err != nil {
-		return err
-	}
-
-	if exists {
-		return nil
-	}
-
-	err = r.FS.WriteFile(containerdEnvFilePath, []byte(unitDropin), 0644)
-	if err != nil {
-		return fmt.Errorf("unable to write unit dropin: %w", err)
-	}
-
-	return nil
-}
-
 // ensureContainerdConfiguration sets the configuration for containerd.
 func (r *Reconciler) ensureContainerdConfiguration(log logr.Logger, criConfig *extensionsv1alpha1.CRIConfig) error {
 	config, err := r.FS.ReadFile(configFile)

pkg/nodeagent/controller/operatingsystemconfig/reconciler.go

@@ -20,6 +20,10 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
+	jsonserializer "k8s.io/apimachinery/pkg/runtime/serializer/json"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/ptr"
@@ -43,6 +47,16 @@ import (
 
 const lastAppliedOperatingSystemConfigFilePath = nodeagentv1alpha1.BaseDir + "/last-applied-osc.yaml"
 
+var codec runtime.Codec
+
+func init() {
+	scheme := runtime.NewScheme()
+	utilruntime.Must(extensionsv1alpha1.AddToScheme(scheme))
+	ser := jsonserializer.NewSerializerWithOptions(jsonserializer.DefaultMetaFactory, scheme, scheme, jsonserializer.SerializerOptions{Yaml: true, Pretty: false, Strict: false})
+	versions := schema.GroupVersions([]schema.GroupVersion{nodeagentv1alpha1.SchemeGroupVersion, extensionsv1alpha1.SchemeGroupVersion})
+	codec = serializer.NewCodecFactory(scheme).CodecForVersions(ser, ser, versions, versions)
+}
+
 // Reconciler decodes the OperatingSystemConfig resources from secrets and applies the systemd units and files to the
 // node.
 type Reconciler struct {
@@ -85,11 +99,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, nil
 	}
 
-	osc, oscRaw, oscChecksum, err := extractOSCFromSecret(secret)
+	osc, oscChecksum, err := extractOSCFromSecret(secret)
 	if err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed extracting OSC from secret: %w", err)
 	}
 
+	log.Info("Applying containerd configuration")
+	if err := r.ReconcileContainerdConfig(ctx, log, osc); err != nil {
+		return reconcile.Result{}, fmt.Errorf("failed reconciling containerd configuration: %w", err)
+	}
+
 	oscChanges, err := computeOperatingSystemConfigChanges(r.FS, osc)
 	if err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed calculating the OSC changes: %w", err)
@@ -100,11 +119,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 		return reconcile.Result{}, nil
 	}
 
-	log.Info("Applying containerd configuration")
-	if err := r.ReconcileContainerdConfig(ctx, log, osc.Spec.CRIConfig); err != nil {
-		return reconcile.Result{}, fmt.Errorf("failed reconciling containerd configuration: %w", err)
-	}
-
 	log.Info("Applying new or changed inline files")
 	if err := r.applyChangedInlineFiles(log, oscChanges.files.changed); err != nil {
 		return reconcile.Result{}, fmt.Errorf("failed applying changed inline files: %w", err)
@@ -170,7 +184,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	)
 
 	log.Info("Persisting current operating system config as 'last-applied' file to the disk", "path", lastAppliedOperatingSystemConfigFilePath)
-	if err := r.FS.WriteFile(lastAppliedOperatingSystemConfigFilePath, oscRaw, 0644); err != nil {
+	oscRaw, err := runtime.Encode(codec, osc)
+	if err != nil {
+		return reconcile.Result{}, fmt.Errorf("unable to encode OSC: %w", err)
+	}
+
+	if err := r.FS.WriteFile(lastAppliedOperatingSystemConfigFilePath, oscRaw, 0600); err != nil {
 		return reconcile.Result{}, fmt.Errorf("unable to write current OSC to file path %q: %w", lastAppliedOperatingSystemConfigFilePath, err)
 	}
 
@@ -387,14 +406,21 @@ func (r *Reconciler) applyChangedUnits(ctx context.Context, log logr.Logger, uni
 
 func (r *Reconciler) removeDeletedUnits(ctx context.Context, log logr.Logger, node client.Object, units []extensionsv1alpha1.Unit) error {
 	for _, unit := range units {
+		// The unit has been created by gardener-node-agent if it has content.
+		// Otherwise, it might be a default OS unit which was enabled/disabled or where drop-ins were added.
+		unitCreatedByNodeAgent := unit.Content != nil
+
 		unitFilePath := path.Join(etcSystemdSystem, unit.Name)
 
 		unitFileExists, err := r.FS.Exists(unitFilePath)
 		if err != nil {
 			return fmt.Errorf("unable to check whether unit file %q exists: %w", unitFilePath, err)
 		}
 
-		if unitFileExists {
+		// Only stop and remove the unit file if it was created by gardener-node-agent. Otherwise, this could affect
+		// default OS units where we add and remove drop-ins only. If operators want to stop and disable units,
+		// they can do it by adding a unit to OSC which applies the `stop` command.
+		if unitFileExists && unitCreatedByNodeAgent {
 			if err := r.DBus.Disable(ctx, unit.Name); err != nil {
 				return fmt.Errorf("unable to disable deleted unit %q: %w", unit.Name, err)
 			}
@@ -405,11 +431,37 @@ func (r *Reconciler) removeDeletedUnits(ctx context.Context, log logr.Logger, no
 
 			if err := r.FS.Remove(unitFilePath); err != nil && !errors.Is(err, afero.ErrFileNotFound) {
 				return fmt.Errorf("unable to delete systemd unit file of deleted unit %q: %w", unit.Name, err)
+			} else {
+				log.Info("Unit was not created by gardener-node-agent, skipping deletion of unit file", "unitName", unit.Name)
+			}
+		}
+
+		dropInFolder := unitFilePath + ".d"
+
+		if exists, err := r.FS.Exists(dropInFolder); err != nil {
+			return fmt.Errorf("unable to check whether drop-in folder %q exists: %w", dropInFolder, err)
+		} else if exists {
+			for _, dropIn := range unit.DropIns {
+				dropInFilePath := path.Join(dropInFolder, dropIn.Name)
+				if err := r.FS.Remove(dropInFilePath); err != nil && !errors.Is(err, afero.ErrFileNotFound) {
+					return fmt.Errorf("unable to delete drop-in file %q of deleted unit %q: %w", dropInFilePath, unit.Name, err)
+				}
+			}
+
+			if empty, err := r.FS.IsEmpty(dropInFolder); err != nil {
+				return fmt.Errorf("unable to check whether drop-in folder %q is empty: %w", dropInFolder, err)
+			} else if empty {
+				if err := r.FS.RemoveAll(dropInFolder); err != nil && !errors.Is(err, afero.ErrFileNotFound) {
+					return fmt.Errorf("unable to delete systemd drop-in folder of deleted unit %q: %w", unit.Name, err)
+				}
 			}
 		}
 
-		if err := r.FS.RemoveAll(unitFilePath + ".d"); err != nil && !errors.Is(err, afero.ErrFileNotFound) {
-			return fmt.Errorf("unable to delete systemd drop-in folder of deleted unit %q: %w", unit.Name, err)
+		// If the unit was not created by gardener-node-agent, but it exists on the node and was removed from OSC. Restart it to apply changes.
+		if unitFileExists && !unitCreatedByNodeAgent {
+			if err := r.DBus.Restart(ctx, r.Recorder, node, unit.Name); err != nil {
+				return fmt.Errorf("unable to restart unit %q removed from OSC but not created by gardener-node-agent: %w", unit.Name, err)
+			}
 		}
 
 		log.Info("Successfully removed no longer needed unit", "unitName", unit.Name)