🤖 feat: delete CoderWorkspacePoolMember workspaces with the identity guard and the last use check

internal/controller/coderworkspacepoolmember_cleanup.go

@@ -0,0 +1,194 @@
+package controller
+
+import (
+	"cmp"
+	"context"
+	"errors"
+	"fmt"
+	"time"
+
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/google/uuid"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/log"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+)
+
+// poolMemberUnknownOutcomePoll is how often a deleted member reads Coder
+// again when its create outcome is still unknown after the settle window.
+const poolMemberUnknownOutcomePoll = 10 * time.Minute
+
+// cleanup runs the delete steps of a member being deleted (plan 6.5). It
+// releases the finalizer once no workspace of the member can exist, the
+// workspace is deleted, the workspace was used, or the control plane is
+// gone. A workspace that nothing proves to be the member's keeps the
+// finalizer, and the controller never touches it.
+func (r *CoderWorkspacePoolMemberReconciler) cleanup(ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember) (ctrl.Result, error) {
+	deleted := meta.FindStatusCondition(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted)
+	switch {
+	case !poolMemberMayHaveWorkspace(member), deleted != nil && deleted.Status == metav1.ConditionTrue:
+		return ctrl.Result{}, r.releaseFinalizer(ctx, member)
+	case deleted != nil && deleted.Reason == "OwnershipUnknown":
+		// Only a person who removes the finalizer releases the member.
+		return ctrl.Result{}, nil
+	}
+	gone, err := controlPlaneGone(ctx, r.Client, member.Namespace, member.Spec.ControlPlaneRef.Name)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+	if gone {
+		// Nothing can reach Coder any more. The docs tell admins to delete
+		// leftover kwp- workspaces.
+		log.FromContext(ctx).Info("The control plane is gone, so the controller releases the pool member without deleting its workspace",
+			"workspaceID", member.Status.WorkspaceID, "workspaceName", member.Status.WorkspaceName)
+		return ctrl.Result{}, r.releaseFinalizer(ctx, member)
+	}
+
+	before := member.Status.DeepCopy()
+	step, requeue, release, err := r.cleanupStep(ctx, member)
+	if step, err = memberWaitOnWrongAnswer(ctx, step, err); err != nil {
+		return ctrl.Result{}, err
+	}
+	if release {
+		return ctrl.Result{}, r.releaseFinalizer(ctx, member)
+	}
+	now := r.Clock.Now()
+	condition := cmp.Or(step.deleted, &metav1.Condition{Status: metav1.ConditionFalse, Reason: step.reason, Message: step.message})
+	member.Status.Phase, member.Status.Reason, member.Status.Message = coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, step.reason, step.message
+	setPoolMemberCondition(member, now, coderv1alpha1.CoderWorkspacePoolMemberConditionReady, metav1.ConditionFalse, step.reason, step.message)
+	setPoolMemberCondition(member, now, coderv1alpha1.CoderWorkspacePoolMemberConditionWorkspaceDeleted, condition.Status, condition.Reason, condition.Message)
+	if err := r.writeStatus(ctx, member, before); err != nil {
+		return ctrl.Result{}, err
+	}
+	if condition.Status == metav1.ConditionTrue {
+		return ctrl.Result{}, r.releaseFinalizer(ctx, member)
+	}
+	if condition.Reason == "OwnershipUnknown" {
+		return ctrl.Result{}, nil
+	}
+	// A wrong Coder answer is a wait without its own poll.
+	return ctrl.Result{RequeueAfter: cmp.Or(requeue, templateTestUnavailablePoll)}, nil
+}
+
+// cleanupStep runs one delete step. release means that the finalizer can go
+// without a status write: no workspace of the member exists, or someone
+// used it.
+func (r *CoderWorkspacePoolMemberReconciler) cleanupStep(
+	ctx context.Context, member *coderv1alpha1.CoderWorkspacePoolMember,
+) (step *coderStep, requeue time.Duration, release bool, err error) {
+	sdk, _, wait, err := controlPlaneCoderClient(ctx, r.Client, member.Namespace, member.Spec.ControlPlaneRef.Name)
+	switch {
+	case wait != nil:
+		return coderWait(wait.reason, "%s", wait.message), templateTestUnavailablePoll, false, nil
+	case err != nil:
+		return nil, 0, false, err
+	}
+	now := r.Clock.Now()
+	if member.Status.WorkspaceID == "" {
+		// A create request may exist: the same confirming reads as before.
+		step, err := confirmPoolMemberCreate(ctx, sdk, member, now)
+		switch {
+		case err != nil:
+			return nil, 0, false, err
+		case !poolMemberMayHaveWorkspace(member):
+			return nil, 0, true, nil // The reads prove that no workspace exists.
+		case member.Status.WorkspaceID != "":
+			return coderWait("Deleting", "The controller adopted workspace %s to delete it.", member.Status.WorkspaceName), coderBuildPoll, false, nil
+		case step.deleted != nil:
+			return step, 0, false, nil
+		case now.Before(member.Status.CreateAttemptTime.Add(templateTestSettleWindow)):
+			return step, coderBuildPoll, false, nil
+		}
+		step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "CreateOutcomeUnknown", Message: step.message}
+		return step, poolMemberUnknownOutcomePoll, false, nil
+	}
+
+	workspace, step, err := guardPoolMemberDelete(ctx, sdk, member)
+	if step != nil || err != nil {
+		return step, templateTestUnavailablePoll, false, err
+	}
+	if member.Spec.ClaimRef == nil && member.Status.DeleteBuildID == "" && member.Status.DeleteAttempts == 0 {
+		// The last use check, before the first delete build: a leaked
+		// workspace is better than lost work.
+		used := meta.IsStatusConditionTrue(member.Status.Conditions, coderv1alpha1.CoderWorkspacePoolMemberConditionUsed)
+		if !used && member.Status.ReadyLastUsedAt != nil {
+			step, err := r.checkPoolMemberUse(ctx, sdk, member, workspace, now)
+			if err != nil || (step != nil && step.reason != "Used") {
+				return step, templateTestUnavailablePoll, false, err
+			}
+			used = step != nil
+		}
+		if used {
+			log.FromContext(ctx).Info("Someone used the workspace without a claim, so the controller releases the pool member without deleting it",
+				"workspaceID", member.Status.WorkspaceID, "workspaceName", member.Status.WorkspaceName)
+			return nil, 0, true, nil
+		}
+	}
+
+	state := workspaceDeleteState{
+		workspaceID: workspace.ID, workspaceName: member.Status.WorkspaceName,
+		deleteBuildID: member.Status.DeleteBuildID, deleteAttempts: member.Status.DeleteAttempts,
+	}
+	result, err := deleteCoderWorkspace(ctx, sdk, &state, now, false)
+	member.Status.DeleteBuildID, member.Status.DeleteAttempts = state.deleteBuildID, state.deleteAttempts
+	switch {
+	case err != nil:
+		return nil, 0, false, err
+	case result.deleted:
+		step := coderWait("Deleted", "The delete build of workspace %s succeeded.", member.Status.WorkspaceName)
+		step.deleted = &metav1.Condition{Status: metav1.ConditionTrue, Reason: "Deleted", Message: step.message}
+		return step, 0, false, nil
+	case result.wait.reason == "CoderUnavailable":
+		return result.wait, templateTestUnavailablePoll, false, nil // Cleanup has no deadline: poll slowly.
+	}
+	return result.wait, cmp.Or(result.requeue, coderBuildPoll), false, nil
+}
+
+// guardPoolMemberDelete is the identity guard before every delete write
+// (plan 6.2 and 6.5 step 1). It reads deleted workspaces too and accepts any
+// latest build. The owner and the name must be the member's, and the build
+// history must prove that the operator user created the workspace with the
+// recorded start build. A status writer can change the IDs, but not that
+// history. A nil step means the member can delete the workspace.
+func guardPoolMemberDelete(
+	ctx context.Context, sdk *codersdk.Client, member *coderv1alpha1.CoderWorkspacePoolMember,
+) (codersdk.Workspace, *coderStep, error) {
+	spec, name := member.Spec, member.Status.WorkspaceName
+	workspaceID, err := uuid.Parse(member.Status.WorkspaceID)
+	if err != nil || workspaceID == uuid.Nil {
+		return codersdk.Workspace{}, nil, fmt.Errorf("assertion failed: pool member %s/%s has workspace ID %q", member.Namespace, member.Name, member.Status.WorkspaceID)
+	}
+	operator, err := sdk.User(ctx, codersdk.Me)
+	if err != nil {
+		return codersdk.Workspace{}, coderStepUnavailable("get operator user", err), nil
+	}
+	if operator.ID == uuid.Nil {
+		return codersdk.Workspace{}, nil, &coderAnswerError{msg: "assertion failed: Coder answered the operator user without an ID"}
+	}
+	workspace, err := sdk.DeletedWorkspace(ctx, workspaceID)
+	if err != nil {
+		return workspace, coderStepUnavailable("get workspace", err), nil
+	}
+	if err := coderAnswerFor("workspace", workspaceID.String(), workspace.ID.String()); err != nil {
+		return workspace, nil, err
+	}
+	var startBuild *codersdk.WorkspaceBuild
+	if workspace.OwnerID.String() == spec.OwnerID && workspace.Name == name {
+		startBuild, err = workspaceProvenance(ctx, sdk, workspace, operator.ID, spec.OrganizationID, spec.TemplateID, spec.TemplateVersionID)
+		var answerErr *coderAnswerError
+		if errors.As(err, &answerErr) {
+			return workspace, nil, err
+		} else if err != nil {
+			return workspace, coderStepUnavailable("get workspace builds", err), nil
+		}
+	}
+	if startBuild == nil || startBuild.ID.String() != member.Status.StartBuildID {
+		step := coderWait("OwnershipUnknown", "Nothing proves that this member created workspace %s, so the controller never deletes it. Only removing the finalizer releases the member.", workspaceID)
+		step.deleted = &metav1.Condition{Status: metav1.ConditionUnknown, Reason: "OwnershipUnknown", Message: step.message}
+		return workspace, step, nil
+	}
+	return workspace, nil, nil
+}

internal/controller/coderworkspacepoolmember_cleanup_test.go

@@ -0,0 +1,164 @@
+package controller_test
+
+import (
+	"testing"
+	"time"
+
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+)
+
+func (e *poolMemberEnv) deleteMember(t *testing.T, key types.NamespacedName) {
+	t.Helper()
+	require.NoError(t, k8sClient.Delete(e.ctx, e.reconcileMember(t, key, 0)))
+}
+
+func (e *poolMemberEnv) coderWrites() int {
+	return e.fake.requestCount(routeCreateBuild) + e.fake.requestCount(routeCancelBuild) + e.fake.requestCount(routeExtendWorkspace)
+}
+
+// TestPoolMemberCleanupDeletesWorkspace: deleting a member deletes its
+// workspace with the delete steps, then releases the finalizer.
+func TestPoolMemberCleanupDeletesWorkspace(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	key, _ := e.readyMember(t)
+	e.deleteMember(t, key)
+	member := e.reconcileMember(t, key, 1)
+	requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build")
+	requireDeletedCondition(t, member, metav1.ConditionFalse, "Deleting")
+	require.Equal(t, 1, e.fake.requestCount(routeCreateBuild))
+	e.fake.setBuildJob(uuid.MustParse(member.Status.DeleteBuildID), codersdk.ProvisionerJobSucceeded)
+	require.Nil(t, e.reconcileMember(t, key, 1), "the finalizer goes once the delete build succeeded")
+
+	// A bound member is deleted too, even after the claimant used it.
+	key, workspaceID := e.readyMember(t)
+	member = e.reconcileMember(t, key, 0)
+	member.Spec.ClaimRef = &coderv1alpha1.CoderWorkspaceClaimReference{Name: "build-1", UID: uuid.NewString()}
+	require.NoError(t, k8sClient.Update(e.ctx, member))
+	e.fake.markUsed(workspaceID, e.clock.Now().Add(time.Minute))
+	e.deleteMember(t, key)
+	member = e.reconcileMember(t, key, 1)
+	requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build")
+}
+
+// TestPoolMemberCleanupGuardsDelete is mutation check 3: before any delete
+// write, the member proves that the workspace and its start build are its
+// own. Otherwise it keeps the finalizer and never touches the workspace.
+func TestPoolMemberCleanupGuardsDelete(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	sdk := e.fake.client(t, 5*time.Second)
+	other, err := sdk.CreateUserWorkspace(e.ctx, e.tester.String(), codersdk.CreateWorkspaceRequest{TemplateVersionID: e.v1, Name: "someone-elses"})
+	require.NoError(t, err)
+	e.fake.setBuildJob(other.LatestBuild.ID, codersdk.ProvisionerJobSucceeded)
+	for _, tc := range []struct {
+		name string
+		edit func(t *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, workspaceID uuid.UUID)
+	}{
+		{name: "another workspace of the owner", edit: func(_ *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, _ uuid.UUID) {
+			member.Status.WorkspaceID, member.Status.StartBuildID = other.ID.String(), other.LatestBuild.ID.String()
+		}},
+		{name: "a start build by someone else", edit: func(_ *testing.T, member *coderv1alpha1.CoderWorkspacePoolMember, workspaceID uuid.UUID) {
+			build := e.fake.addExternalBuild(workspaceID, e.tester, codersdk.WorkspaceTransitionStart)
+			e.fake.setBuildJob(build.ID, codersdk.ProvisionerJobSucceeded)
+			member.Status.StartBuildID = build.ID.String()
+		}},
+	} {
+		key, workspaceID := e.readyMember(t)
+		member := e.reconcileMember(t, key, 0)
+		tc.edit(t, member, workspaceID)
+		require.NoError(t, k8sClient.Status().Update(e.ctx, member))
+		e.deleteMember(t, key)
+		writes := e.coderWrites()
+		member = e.reconcileMember(t, key, 1)
+		requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "OwnershipUnknown", "never deletes it")
+		requireDeletedCondition(t, member, metav1.ConditionUnknown, "OwnershipUnknown")
+		require.Zero(t, e.lastStep.RequeueAfter, tc.name)
+		reads := e.fake.totalRequests()
+		require.NotNil(t, e.reconcileMember(t, key, 2), "%s: the finalizer stays", tc.name)
+		require.Equal(t, reads, e.fake.totalRequests(), "%s: no Coder call after OwnershipUnknown", tc.name)
+		require.Equal(t, writes, e.coderWrites(), "%s: no Coder write", tc.name)
+	}
+
+	// A person releases a member by removing its finalizer. While another
+	// finalizer keeps the object, the controller still never touches the
+	// workspace.
+	key, _ := e.readyMember(t)
+	member := e.reconcileMember(t, key, 0)
+	member.Finalizers = append(member.Finalizers, "test.coder.com/hold")
+	require.NoError(t, k8sClient.Update(e.ctx, member))
+	e.deleteMember(t, key)
+	member = e.reconcileMember(t, key, 0)
+	member.Finalizers = []string{"test.coder.com/hold"}
+	require.NoError(t, k8sClient.Update(e.ctx, member))
+	requests := e.fake.totalRequests()
+	member = e.reconcileMember(t, key, 2)
+	require.Equal(t, requests, e.fake.totalRequests(), "no Coder call after the finalizer is gone")
+	member.Finalizers = nil
+	require.NoError(t, k8sClient.Update(e.ctx, member))
+}
+
+// TestPoolMemberCleanupLastUseCheck is mutation check 11: before the first
+// delete build of an unclaimed member, for example after its pool was
+// deleted, the member reads last_used_at again. A used workspace stays in
+// Coder, and the member releases its finalizer without a Coder write.
+func TestPoolMemberCleanupLastUseCheck(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	key, workspaceID := e.readyMember(t)
+	e.fake.markUsed(workspaceID, e.clock.Now().Add(time.Minute))
+	e.deleteMember(t, key)
+	require.Nil(t, e.reconcileMember(t, key, 1))
+	require.Zero(t, e.coderWrites(), "a used workspace is never deleted")
+
+	// A template-wide write is not use: the workspace is deleted.
+	key, _ = e.readyMember(t)
+	e.clock.SetTime(e.clock.Now().Add(2 * time.Minute))
+	e.fake.touchTemplateWorkspaces(e.tplID, e.clock.Now())
+	e.deleteMember(t, key)
+	requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "started a delete build")
+}
+
+// TestPoolMemberCleanupWithoutWorkspaceID: a member deleted while its create
+// outcome is unknown confirms it by reads first, and a member whose control
+// plane is gone is released without a Coder call.
+func TestPoolMemberCleanupWithoutWorkspaceID(t *testing.T) {
+	t.Parallel()
+	e := newPoolMemberEnv(t)
+	// The create took effect, but the answer was lost: the reads adopt the
+	// workspace, then the member deletes it.
+	key := e.uncertainMember(t, fakeFault{Status: 502, AfterCommit: true}, nil)
+	e.deleteMember(t, key)
+	member := e.reconcileMember(t, key, 1)
+	requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "Deleting", "adopted workspace")
+	require.NotEmpty(t, member.Status.WorkspaceID)
+	e.reconcileMember(t, key, 1)
+	require.Equal(t, 1, e.fake.requestCount(routeCancelBuild), "the start build is canceled before the delete build")
+
+	// No workspace appears: within the settle window the member reads again,
+	// after it the site owner's 404 proves that none exists.
+	key = e.uncertainMember(t, fakeFault{Status: 502}, nil)
+	marker := e.reconcileMember(t, key, 0).Status.CreateAttemptTime.Time
+	e.deleteMember(t, key)
+	requireMember(t, e.reconcileMember(t, key, 1), coderv1alpha1.CoderWorkspacePoolMemberPhaseDeleting, "ConfirmingCreate", "")
+	require.Equal(t, 5*time.Second, e.lastStep.RequeueAfter)
+	e.clock.SetTime(marker.Add(15 * time.Minute))
+	require.Nil(t, e.reconcileMember(t, key, 1))
+
+	// The control plane is gone: nothing can reach Coder.
+	e.clock.SetTime(time.Now().Truncate(time.Second))
+	key, _ = e.readyMember(t)
+	e.deleteMember(t, key)
+	cp := &coderv1alpha1.CoderControlPlane{}
+	require.NoError(t, k8sClient.Get(e.ctx, types.NamespacedName{Namespace: e.ns, Name: "coder"}, cp))
+	require.NoError(t, k8sClient.Delete(e.ctx, cp))
+	requests := e.fake.totalRequests()
+	require.Nil(t, e.reconcileMember(t, key, 1))
+	require.Equal(t, requests, e.fake.totalRequests())
+}

internal/controller/coderworkspacepoolmember_controller.go

@@ -67,12 +67,13 @@ func (r *CoderWorkspacePoolMemberReconciler) Reconcile(ctx context.Context, req
 		return ctrl.Result{}, err
 	}
 	if !member.DeletionTimestamp.IsZero() {
-		// The delete steps (plan 6.5) come later. Until then, a member whose
-		// workspace can exist keeps its finalizer.
-		if poolMemberMayHaveWorkspace(member) {
+		if !controllerutil.ContainsFinalizer(member, coderv1alpha1.CoderWorkspacePoolMemberCleanupFinalizer) {
+			// Removing the finalizer is the way to release a member: the
+			// controller then never touches its workspace, even while another
+			// finalizer keeps the object.
 			return ctrl.Result{}, nil
 		}
-		return ctrl.Result{}, r.releaseFinalizer(ctx, member)
+		return r.cleanup(ctx, member)
 	}
 	switch member.Status.Phase {
 	case coderv1alpha1.CoderWorkspacePoolMemberPhaseFailed:

internal/controller/coderworkspacepoolmember_controller_test.go

@@ -202,8 +202,8 @@ func TestPoolMemberCreatesWorkspace(t *testing.T) {
 	requireMember(t, member, coderv1alpha1.CoderWorkspacePoolMemberPhaseCreating, "WaitingForBuild", "")
 	require.Equal(t, 1, e.fake.requestCount(routeCreateWorkspace), "never a second create request")
 
-	// A member whose workspace can exist keeps its finalizer on deletion
-	// until the delete steps run.
+	// A member whose workspace exists keeps its finalizer on deletion until
+	// the workspace is deleted.
 	require.NoError(t, k8sClient.Delete(e.ctx, member))
 	require.NotNil(t, e.reconcileMember(t, key, 2))
 }