Repository: spegel-org/spegel (go)

Issue:
Clean up containerd mirror configuration
Spegel makes persistent changes to the hosts filesystem. This is required to setup the mirror configuration. The mirror configuration should be present on the node for as long as Spegel is being used. A good practice however would be to revert the mirror configuration back to its pre Spegel state if Spegel ever was removed. This is especially useful for those who are just evaluating Spegel but chose not to use it. Keeping the mirror configuration will not break the cluster as it would always fallback to the original registry, and the configuration would be removed when old nodes are removed from the cluster.

The old solution was to add an option to remove the configuration files during shutdown. While a good option it wasn't optimal as it is beneficial to keep the configuration during versions updates of Spegel, as it would fallback to other Spegel instances in the cluster. 

An alternative solution is to create a Helm uninstall hook which would run and remove the configuration from all nodes when the Helm chart is removed. This has the benefit of cleaning up stateful changes and returning the node configuration to the state it existed in before Spegel.

Pull request:
Add delete hook to cleanup configuration from host when chart is uninstalled
This only took me two years but finally was able to come up with a solution that I don't hate. Its also a solution that has been right in front of me the whole time.

The cleanup solution uses Helm delete hooks to create a DaemonSet when the chart is uninstalled. These pods run the cleanup command which will clear the Containerd mirror configuration directory and restore any files from the backup directory. The solution would have ended there if Helm supported health checking DaemonSets as part as hooks, but it doesn't.

To enable Helm to remove the DaemonSet when the cleanup succeeds we need to create a waiting Pod. Without this Pod Helm would remove the DaemonSet as soon as it was created. With the Pod Helm will wait for the Pod to successfully complete running. Nothing will be deleted until that happens. We use this to our benefit, by having the wait Pod poll the cleanup readiness probe of all the cleanup pods. This probe is only running once the cleanup has succeeded. When the probes from all of the Pods are successful the wait Pod will exit successfully which in turn triggers Helm to remove all of the hook resources.

Fixes #67 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 686e1d2..387da67 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- [#832](https://github.com/spegel-org/spegel/pull/832) Add delete hook to cleanup configuration from host when chart is uninstalled.
+
 ### Changed
 
 - [#812](https://github.com/spegel-org/spegel/pull/812) Upgrade to Go 1.24.1 and switch to use go tool for helm docs.
diff --git a/charts/spegel/README.md b/charts/spegel/README.md
index ba54ba7..a7d230f 100644
--- a/charts/spegel/README.md
+++ b/charts/spegel/README.md
@@ -31,6 +31,7 @@ Read the [getting started](https://spegel.dev/docs/getting-started/) guide to de
 | resources | object | `{"limits":{"memory":"128Mi"},"requests":{"memory":"128Mi"}}` | Resource requests and limits for the Spegel container. |
 | revisionHistoryLimit | int | `10` | The number of old history to retain to allow rollback. |
 | securityContext | object | `{}` | Security context for the Spegel container. |
+| service.cleanup.port | int | `8080` | Port to expose cleanup probe on. |
 | service.metrics.port | int | `9090` | Port to expose the metrics via the service. |
 | service.registry.hostPort | int | `30020` | Local host port to expose the registry. |
 | service.registry.nodeIp | string | `""` | Override the NODE_ID environment variable. It defaults to the field status.hostIP |
diff --git a/charts/spegel/templates/post-delete-hook.yaml b/charts/spegel/templates/post-delete-hook.yaml
new file mode 100644
index 0000000..6009e75
--- /dev/null
+++ b/charts/spegel/templates/post-delete-hook.yaml
@@ -0,0 +1,106 @@
+{{- if .Values.spegel.containerdMirrorAdd }}
+apiVersion: apps/v1
+kind: DaemonSet
+metadata:
+  name: {{ include "spegel.fullname" . }}-cleanup
+  namespace: {{ include "spegel.namespace" . }}
+  labels:
+    app.kubernetes.io/component: cleanup
+    {{- include "spegel.labels" . | nindent 4 }}
+  annotations:
+    helm.sh/hook: "post-delete"
+    helm.sh/hook-delete-policy: "before-hook-creation, hook-succeeded"
+    helm.sh/hook-weight: "0"
+spec:
+  selector:
+    matchLabels:
+      app.kubernetes.io/component: cleanup
+      {{- include "spegel.selectorLabels" . | nindent 6 }}
+  template:
+    metadata:
+      labels:
+        app.kubernetes.io/component: cleanup
+        {{- include "spegel.selectorLabels" . | nindent 8 }}
+    spec:
+      {{- with .Values.imagePullSecrets }}
+      imagePullSecrets:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+      securityContext:
+        {{- toYaml .Values.podSecurityContext | nindent 8 }}
+      priorityClassName: {{ .Values.priorityClassName }}
+      containers:
+      - name: cleanup
+        image: "{{ include "spegel.image" . }}"
+        imagePullPolicy: {{ .Values.image.pullPolicy }}
+        args:
+          - cleanup
+          - --containerd-registry-config-path={{ .Values.spegel.containerdRegistryConfigPath }}
+          - --addr=:{{ .Values.service.cleanup.port }}
+        readinessProbe:
+          httpGet:
+            path: /healthz
+            port: readiness
+        ports:
+          - name: readiness
+            containerPort: {{ .Values.service.cleanup.port }}
+            protocol: TCP
+        volumeMounts:
+          - name: containerd-config
+            mountPath: {{ .Values.spegel.containerdRegistryConfigPath }}
+      volumes:
+        - name: containerd-config
+          hostPath:
+            path: {{ .Values.spegel.containerdRegistryConfigPath }}
+            type: DirectoryOrCreate
+      {{- with .Values.tolerations }}
+      tolerations:
+        {{- toYaml . | nindent 8 }}
+      {{- end }}
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: {{ include "spegel.fullname" . }}-cleanup
+  namespace: {{ include "spegel.namespace" . }}
+  labels:
+    app.kubernetes.io/component: cleanup
+    {{- include "spegel.labels" . | nindent 4 }}
+  annotations:
+    helm.sh/hook: "post-delete"
+    helm.sh/hook-delete-policy: "before-hook-creation, hook-succeeded"
+    helm.sh/hook-weight: "0"
+spec:
+  selector:
+    app.kubernetes.io/component: cleanup
+    {{- include "spegel.selectorLabels" . | nindent 4 }}
+  clusterIP: None
+  publishNotReadyAddresses: true
+  ports:
+    - name: readiness
+      port: {{ .Values.service.cleanup.port }}
+      protocol: TCP
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: {{ include "spegel.fullname" . }}-cleanup-wait
+  namespace: {{ include "spegel.namespace" . }}
+  labels:
+    app.kubernetes.io/component: cleanup-wait
+    {{- include "spegel.labels" . | nindent 4 }}
+  annotations:
+    helm.sh/hook: "post-delete"
+    helm.sh/hook-delete-policy: "before-hook-creation, hook-succeeded"
+    helm.sh/hook-weight: "1"
+spec:
+  containers:
+    - name: cleanup-wait
+      image: "{{ include "spegel.image" . }}"
+      imagePullPolicy: {{ .Values.image.pullPolicy }}
+      args:
+        - cleanup-wait
+        - --probe-endpoint={{ include "spegel.fullname" . }}-cleanup.{{ include "spegel.namespace" . }}.svc.{{ .Values.clusterDomain }}:{{ .Values.service.cleanup.port }}
+  restartPolicy: Never
+  terminationGracePeriodSeconds: 0
+{{- end }}
diff --git a/charts/spegel/values.yaml b/charts/spegel/values.yaml
index f36d693..d0cf648 100644
--- a/charts/spegel/values.yaml
+++ b/charts/spegel/values.yaml
@@ -61,6 +61,9 @@ service:
   metrics:
     # -- Port to expose the metrics via the service.
     port: 9090
+  cleanup:
+    # -- Port to expose cleanup probe on.
+    port: 8080
 
 # -- Resource requests and limits for the Spegel container.
 resources:
diff --git a/main.go b/main.go
index cbd6675..eeeda4b 100644
--- a/main.go
+++ b/main.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"log/slog"
 	"net"
 	"net/http"
@@ -61,9 +62,22 @@ type RegistryCmd struct {
 	DebugWebEnabled              bool          `arg:"--debug-web-enabled,env:DEBUG_WEB_ENABLED" default:"false" help:"When true enables debug web page."`
 }
 
+type CleanupCmd struct {
+	Addr                         string `arg:"--addr,required,env:ADDR" help:"address to run readiness probe on."`
+	ContainerdRegistryConfigPath string `arg:"--containerd-registry-config-path,env:CONTAINERD_REGISTRY_CONFIG_PATH" default:"/etc/containerd/certs.d" help:"Directory where mirror configuration is written."`
+}
+
+type CleanupWaitCmd struct {
+	ProbeEndpoint string        `arg:"--probe-endpoint,required,env:PROBE_ENDPOINT" help:"endpoint to probe cleanup jobs from."`
+	Threshold     int           `arg:"--threshold,env:THRESHOLD" default:"3" help:"amount of consecutive successful probes to consider cleanup done."`
+	Period        time.Duration `arg:"--period,env:PERIOD" default:"2s" help:"address to run readiness probe on."`
+}
+
 type Arguments struct {
 	Configuration *ConfigurationCmd `arg:"subcommand:configuration"`
 	Registry      *RegistryCmd      `arg:"subcommand:registry"`
+	Cleanup       *CleanupCmd       `arg:"subcommand:cleanup"`
+	CleanupWait   *CleanupWaitCmd   `arg:"subcommand:cleanup-wait"`
 	LogLevel      slog.Level        `arg:"--log-level,env:LOG_LEVEL" default:"INFO" help:"Minimum log level to output. Value should be DEBUG, INFO, WARN, or ERROR."`
 }
 
@@ -96,6 +110,10 @@ func run(ctx context.Context, args *Arguments) error {
 		return configurationCommand(ctx, args.Configuration)
 	case args.Registry != nil:
 		return registryCommand(ctx, args.Registry)
+	case args.Cleanup != nil:
+		return cleanupCommand(ctx, args.Cleanup)
+	case args.CleanupWait != nil:
+		return cleanupWaitCommand(ctx, args.CleanupWait)
 	default:
 		return errors.New("unknown subcommand")
 	}
@@ -254,3 +272,118 @@ func loadBasicAuth() (string, string, error) {
 	}
 	return string(username), string(password), nil
 }
+
+func cleanupCommand(ctx context.Context, args *CleanupCmd) error {
+	log := logr.FromContextOrDiscard(ctx)
+
+	fs := afero.NewOsFs()
+	err := oci.CleanupMirrorConfiguration(ctx, fs, args.ContainerdRegistryConfigPath)
+	if err != nil {
+		return err
+	}
+
+	g, ctx := errgroup.WithContext(ctx)
+
+	mux := http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		if req.Method != http.MethodGet && req.URL.Path != "/healthz" {
+			log.Error(errors.New("unknown request"), "unsupported probe request", "path", req.URL.Path, "method", req.Method)
+			rw.WriteHeader(http.StatusNotFound)
+			return
+		}
+		rw.WriteHeader(http.StatusOK)
+	})
+	srv := &http.Server{
+		Addr:    args.Addr,
+		Handler: mux,
+	}
+	g.Go(func() error {
+		if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+			return err
+		}
+		return nil
+	})
+	g.Go(func() error {
+		<-ctx.Done()
+		shutdownCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+		defer cancel()
+		return srv.Shutdown(shutdownCtx)
+	})
+
+	log.Info("waiting to be shutdown")
+	err = g.Wait()
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func cleanupWaitCommand(ctx context.Context, args *CleanupWaitCmd) error {
+	log := logr.FromContextOrDiscard(ctx)
+
+	addr, port, err := net.SplitHostPort(args.ProbeEndpoint)
+	if err != nil {
+		return err
+	}
+
+	resolver := &net.Resolver{}
+	client := &http.Client{}
+	thresholdCount := 0
+	for {
+		time.Sleep(args.Period)
+		start := time.Now()
+
+		log.Info("running probe lookup", "host", addr)
+		ips, err := resolver.LookupIPAddr(ctx, addr)
+		if err != nil {
+			log.Error(err, "cleanup probe lookup failed")
+			thresholdCount = 0
+			continue
+		}
+
+		log.Info("running probe request", "endpoints", len(ips))
+		g, gCtx := errgroup.WithContext(ctx)
+		g.SetLimit(10)
+		for _, ip := range ips {
+			g.Go(func() error {
+				u := url.URL{
+					Scheme: "http",
+					Host:   net.JoinHostPort(ip.String(), port),
+					Path:   "/healthz",
+				}
+				reqCtx, cancel := context.WithTimeout(gCtx, 1*time.Second)
+				defer cancel()
+				req, err := http.NewRequestWithContext(reqCtx, http.MethodGet, u.String(), nil)
+				if err != nil {
+					return err
+				}
+				resp, err := client.Do(req)
+				if err != nil {
+					return err
+				}
+				defer resp.Body.Close()
+				_, err = io.Copy(io.Discard, resp.Body)
+				if err != nil {
+					return err
+				}
+				if resp.StatusCode != http.StatusOK {
+					return fmt.Errorf("unexpected status code %s", resp.Status)
+				}
+				return nil
+			})
+		}
+		err = g.Wait()
+		if err != nil {
+			log.Error(err, "cleanup probe request failed")
+			thresholdCount = 0
+			continue
+		}
+
+		thresholdCount += 1
+		log.Info("probe ran successfully", "threshold", thresholdCount, "duration", time.Since(start).String())
+		if thresholdCount == args.Threshold {
+			log.Info("probe threshold reached")
+			return nil
+		}
+	}
+}
diff --git a/pkg/oci/containerd.go b/pkg/oci/containerd.go
index 83dc024..9a354e8 100644
--- a/pkg/oci/containerd.go
+++ b/pkg/oci/containerd.go
@@ -417,6 +417,50 @@ func AddMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string,
 	return nil
 }
 
+func CleanupMirrorConfiguration(ctx context.Context, fs afero.Fs, configPath string) error {
+	log := logr.FromContextOrDiscard(ctx)
+
+	// If backup directory does not exist it means mirrors was never configured or cleanup has already run.
+	backupDirPath := path.Join(configPath, backupDir)
+	ok, err := afero.DirExists(fs, backupDirPath)
+	if err != nil {
+		return err
+	}
+	if !ok {
+		log.Info("skipping cleanup because backup directory does not exist")
+		return nil
+	}
+
+	// Remove everything except _backup
+	err = clearConfig(fs, configPath)
+	if err != nil {
+		return err
+	}
+
+	// Move content from backup directory
+	files, err := afero.ReadDir(fs, backupDirPath)
+	if err != nil {
+		return err
+	}
+	for _, fi := range files {
+		oldPath := path.Join(backupDirPath, fi.Name())
+		newPath := path.Join(configPath, fi.Name())
+		err := fs.Rename(oldPath, newPath)
+		if err != nil {
+			return err
+		}
+		log.Info("recovering Containerd host configuration", "path", oldPath)
+	}
+
+	// Remove backup directory to indicate that cleanup has been run.
+	err = fs.RemoveAll(backupDirPath)
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
 func validateRegistries(urls []url.URL) error {
 	errs := []error{}
 	for _, u := range urls {


Test patch:
diff --git a/pkg/oci/containerd_test.go b/pkg/oci/containerd_test.go
index 1ab68d7..434f795 100644
--- a/pkg/oci/containerd_test.go
+++ b/pkg/oci/containerd_test.go
@@ -731,7 +731,27 @@ x-custom-2 = ['foo']
 capabilities = ['pull']
 override_path = true`
 	require.Equal(t, expected, eh)
+}
+
+func TestCleanupMirrorConfiguration(t *testing.T) {
+	t.Parallel()
 
+	fs := afero.NewMemMapFs()
+	err := afero.WriteFile(fs, filepath.Join("certs.d", backupDir, "data.txt"), []byte("hello world"), 0o644)
+	require.NoError(t, err)
+	err = afero.WriteFile(fs, filepath.Join("certs.d", "foo.bin"), []byte("hello world"), 0o644)
+	require.NoError(t, err)
+	err = fs.MkdirAll("certs.d/docker.io", 0o755)
+	require.NoError(t, err)
+
+	for range 2 {
+		err = CleanupMirrorConfiguration(t.Context(), fs, "certs.d")
+		require.NoError(t, err)
+		files, err := afero.ReadDir(fs, "certs.d")
+		require.NoError(t, err)
+		require.Len(t, files, 1)
+		require.Equal(t, "data.txt", files[0].Name())
+	}
 }
 
 func stringListToUrlList(t *testing.T, list []string) []url.URL {
diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go
index 265ef5d..b005076 100644
--- a/test/e2e/e2e_test.go
+++ b/test/e2e/e2e_test.go
@@ -76,6 +76,10 @@ func TestE2E(t *testing.T) {
 	backupHostToml := command(t.Context(), t, fmt.Sprintf("docker exec %s-worker2 cat /etc/containerd/certs.d/_backup/docker.io/hosts.toml", kindName))
 	require.Equal(t, hostsToml, backupHostToml)
 
+	// Cleanup backup for uninstall tests
+	command(t.Context(), t, fmt.Sprintf("docker exec %s-worker2 rm -rf /etc/containerd/certs.d/_backup", kindName))
+	command(t.Context(), t, fmt.Sprintf("docker exec %s-worker2 mkdir /etc/containerd/certs.d/_backup", kindName))
+
 	// Run conformance tests.
 	t.Log("Running conformance tests")
 	command(t.Context(), t, fmt.Sprintf("kubectl --kubeconfig %s create namespace conformance --dry-run=client -o yaml | kubectl --kubeconfig %s apply -f -", kcPath, kcPath))
@@ -171,6 +175,19 @@ func TestE2E(t *testing.T) {
 		restartOutput = command(t.Context(), t, fmt.Sprintf("kubectl --kubeconfig %s --namespace spegel get pods -o=jsonpath='{.items[*].status.containerStatuses[0].restartCount}'", kcPath))
 		return restartOutput == "1"
 	}, 5*time.Second, 1*time.Second)
+
+	// Uninstall Spegel and make sure cleanup is run
+	t.Log("Uninstalling Spegel")
+	command(t.Context(), t, fmt.Sprintf("helm --kubeconfig %s uninstall --timeout 60s --namespace spegel spegel", kcPath))
+	require.Eventually(t, func() bool {
+		allOutput := command(t.Context(), t, fmt.Sprintf("kubectl --kubeconfig %s get all --namespace spegel", kcPath))
+		return allOutput == ""
+	}, 10*time.Second, 1*time.Second)
+	nodes = getNodes(t.Context(), t, kindName)
+	for _, node := range nodes {
+		lsOutput := command(t.Context(), t, fmt.Sprintf("docker exec %s-%s ls /etc/containerd/certs.d", kindName, node))
+		require.Empty(t, lsOutput)
+	}
 }
 
 func TestDevDeploy(t *testing.T) {