K8s: remove ephemeral secrets implizite via GC and explizite

.golangci.yaml

@@ -239,6 +239,8 @@ linters:
           alias: kube_errors
         - pkg: k8s.io/client-go/tools/clientcmd
           alias: kube_client_cmd
+        - pkg: k8s.io/apimachinery/pkg/types
+          alias: kube_types
 
         # docker
         - pkg: github.com/docker/cli/cli/config/types

pipeline/backend/kubernetes/kubernetes.go

@@ -274,8 +274,13 @@ func (e *kube) StartStep(ctx context.Context, step *types.Step, taskUUID string)
 	}
 
 	log.Trace().Str("taskUUID", taskUUID).Msgf("starting step: %s", step.Name)
-	_, err = startPod(ctx, e, step, options, taskUUID)
-	return err
+	pod, err := startPod(ctx, e, step, options, taskUUID)
+	if err != nil {
+		return err
+	}
+
+	// Make the step pod the owner of the ephemeral secrets so GC can remove them when the pod is removed
+	return setSecretsOwner(ctx, e, step, pod)
 }
 
 // WaitStep waits for the pipeline step to complete and returns
@@ -517,29 +522,38 @@ func (e *kube) DestroyStep(ctx context.Context, step *types.Step, taskUUID strin
 // DestroyWorkflow destroys the pipeline environment.
 func (e *kube) DestroyWorkflow(ctx context.Context, conf *types.Config, taskUUID string) error {
 	log.Trace().Str("taskUUID", taskUUID).Msg("deleting Kubernetes primitives")
+	var errs []error
 
 	for _, stage := range conf.Stages {
 		for _, step := range stage.Steps {
-			err := stopPod(ctx, e, step, e.config.newDefaultDeleteOptions())
-			if err != nil {
-				return err
+			// Clean up the step secrets in any case so we get no opaque ones.
+			if needsRegistrySecret(step) {
+				if err := stopRegistrySecret(ctx, e, step, e.config.newDefaultDeleteOptions()); err != nil {
+					errs = append(errs, err)
+				}
+			}
+			if needsStepSecret(step) {
+				if err := stopStepSecret(ctx, e, step, e.config.newDefaultDeleteOptions()); err != nil {
+					errs = append(errs, err)
+				}
+			}
+			if err := stopPod(ctx, e, step, e.config.newDefaultDeleteOptions()); err != nil {
+				errs = append(errs, err)
 			}
 		}
 	}
 
 	namespace := e.config.GetNamespace(conf.Stages[0].Steps[0].OrgID)
 
 	log.Trace().Str("taskUUID", taskUUID).Msgf("deleting workflow headless service")
-	err := e.stopHeadlessService(ctx, e, namespace, taskUUID)
-	if err != nil {
-		return err
+	if err := e.stopHeadlessService(ctx, e, namespace, taskUUID); err != nil {
+		errs = append(errs, err)
 	}
 
 	log.Trace().Str("taskUUID", taskUUID).Msgf("deleting workflow volume")
-	err = stopVolume(ctx, e, conf.Volume, e.config.GetNamespace(conf.Stages[0].Steps[0].OrgID), e.config.newDefaultDeleteOptions())
-	if err != nil {
-		return err
+	if err := stopVolume(ctx, e, conf.Volume, namespace, e.config.newDefaultDeleteOptions()); err != nil {
+		errs = append(errs, err)
 	}
 
-	return nil
+	return errors.Join(errs...)
 }

pipeline/backend/kubernetes/kubernetes_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"github.com/urfave/cli/v3"
 	kube_core_v1 "k8s.io/api/core/v1"
+	kube_errors "k8s.io/apimachinery/pkg/api/errors"
 	kube_meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
 
@@ -335,3 +336,40 @@ func TestWaitStepNoGoroutineLeak(t *testing.T) {
 		"goroutines leaked after canceling %d WaitStep calls: got %d leaked",
 		numSteps, leaked)
 }
+
+func TestDestroyWorkflowCleansOrphanedSecrets(t *testing.T) {
+	const namespace = "test-ns"
+	ctx := context.Background()
+	client := fake.NewClientset()
+	engine := makeEngine(client)
+
+	step := &types.Step{
+		UUID:          "01he8bebctabr3kgk0qj36d2me-0",
+		Name:          "go-test",
+		Image:         "alpine",
+		SecretMapping: map[string]string{"FOO": "bar"},
+		AuthConfig:    types.Auth{Username: "foo", Password: "bar"},
+	}
+
+	// Secrets created but the pod was never started (e.g. canceled workflow),
+	// so no owner reference points at a pod and Kubernetes will not GC them.
+	require.NoError(t, startRegistrySecret(ctx, engine, step))
+	require.NoError(t, startStepSecret(ctx, engine, step))
+
+	conf := &types.Config{
+		Volume: "test-volume",
+		Stages: []*types.Stage{{Steps: []*types.Step{step}}},
+	}
+
+	require.NoError(t, engine.DestroyWorkflow(ctx, conf, "task-uuid"))
+
+	regName, err := registrySecretName(step)
+	require.NoError(t, err)
+	stepName, err := stepSecretName(step)
+	require.NoError(t, err)
+
+	for _, name := range []string{regName, stepName} {
+		_, err := client.CoreV1().Secrets(namespace).Get(ctx, name, kube_meta_v1.GetOptions{})
+		assert.True(t, kube_errors.IsNotFound(err), "secret %s should be deleted by DestroyWorkflow", name)
+	}
+}

pipeline/backend/kubernetes/secrets.go

@@ -27,6 +27,7 @@ import (
 	kube_core_v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	kube_meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kube_types "k8s.io/apimachinery/pkg/types"
 
 	"go.woodpecker-ci.org/woodpecker/v3/pipeline"
 	"go.woodpecker-ci.org/woodpecker/v3/pipeline/backend/types"
@@ -201,6 +202,54 @@ func secretReference(name string) kube_core_v1.LocalObjectReference {
 	}
 }
 
+// setSecretsOwner sets the step pod as owner of the ephemeral secrets created for the step.
+// This lets GC remove the secrets when the pod is removed.
+func setSecretsOwner(ctx context.Context, engine *kube, step *types.Step, pod *kube_core_v1.Pod) error {
+	var names []string
+	if needsRegistrySecret(step) {
+		name, err := registrySecretName(step)
+		if err != nil {
+			return err
+		}
+		names = append(names, name)
+	}
+	if needsStepSecret(step) {
+		name, err := stepSecretName(step)
+		if err != nil {
+			return err
+		}
+		names = append(names, name)
+	}
+
+	for _, name := range names {
+		if err := setSecretOwner(ctx, engine, step, name, pod); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func setSecretOwner(ctx context.Context, engine *kube, step *types.Step, secretName string, pod *kube_core_v1.Pod) error {
+	patch, err := json.Marshal(map[string]any{
+		"metadata": map[string]any{
+			"ownerReferences": []kube_meta_v1.OwnerReference{{
+				APIVersion: "v1",
+				Kind:       "Pod",
+				Name:       pod.Name,
+				UID:        pod.UID,
+			}},
+		},
+	})
+	if err != nil {
+		return err
+	}
+
+	log.Trace().Str("name", secretName).Msg("setting secret owner")
+	_, err = engine.client.CoreV1().Secrets(engine.config.GetNamespace(step.OrgID)).
+		Patch(ctx, secretName, kube_types.MergePatchType, patch, kube_meta_v1.PatchOptions{})
+	return err
+}
+
 func needsRegistrySecret(step *types.Step) bool {
 	return step.AuthConfig.Username != "" && step.AuthConfig.Password != ""
 }

pipeline/backend/kubernetes/secrets_test.go

@@ -15,12 +15,16 @@
 package kubernetes
 
 import (
+	"context"
 	"encoding/json"
 	"testing"
 
 	"github.com/kinbiko/jsonassert"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	kube_core_v1 "k8s.io/api/core/v1"
+	kube_meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
 
 	"go.woodpecker-ci.org/woodpecker/v3/pipeline/backend/types"
 )
@@ -240,3 +244,46 @@ func TestRegistrySecret(t *testing.T) {
 	ja := jsonassert.New(t)
 	ja.Assertf(string(secretJSON), expected)
 }
+
+func TestSetSecretsOwner(t *testing.T) {
+	const namespace = "test-ns"
+	ctx := context.Background()
+	client := fake.NewClientset()
+	engine := makeEngine(client)
+
+	step := &types.Step{
+		UUID:          "01he8bebctabr3kgk0qj36d2me-0",
+		Name:          "go-test",
+		Image:         "alpine",
+		SecretMapping: map[string]string{"FOO": "bar"},
+		AuthConfig:    types.Auth{Username: "foo", Password: "bar"},
+	}
+
+	require.NoError(t, startRegistrySecret(ctx, engine, step))
+	require.NoError(t, startStepSecret(ctx, engine, step))
+
+	pod := &kube_core_v1.Pod{
+		ObjectMeta: kube_meta_v1.ObjectMeta{
+			Name:      "wp-01he8bebctabr3kgk0qj36d2me-0",
+			Namespace: namespace,
+			UID:       "pod-uid-123",
+		},
+	}
+
+	require.NoError(t, setSecretsOwner(ctx, engine, step, pod))
+
+	regName, err := registrySecretName(step)
+	require.NoError(t, err)
+	stepName, err := stepSecretName(step)
+	require.NoError(t, err)
+
+	for _, name := range []string{regName, stepName} {
+		secret, err := client.CoreV1().Secrets(namespace).Get(ctx, name, kube_meta_v1.GetOptions{})
+		require.NoError(t, err)
+		require.Len(t, secret.OwnerReferences, 1, "secret %s should have one owner", name)
+		owner := secret.OwnerReferences[0]
+		assert.Equal(t, "Pod", owner.Kind)
+		assert.Equal(t, pod.Name, owner.Name)
+		assert.Equal(t, pod.UID, owner.UID)
+	}
+}