From 656d4f4051e7e7f83e5010bd21847fd40ad96f23 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Thu, 19 Jun 2025 15:47:54 +0100 Subject: [PATCH] fix(coderd): do not create provisioner job for orphan delete --- cli/cliui/provisionerjob.go | 3 + cli/cliutil/provisionerwarn.go | 7 ++- cli/cliutil/provisionerwarn_test.go | 11 ++++ cli/delete_test.go | 33 +++++++--- coderd/workspacebuilds.go | 58 ++++++++++-------- coderd/workspacebuilds_test.go | 48 +++++++++++---- coderd/wsbuilder/wsbuilder.go | 45 +++++++++++++- coderd/wsbuilder/wsbuilder_test.go | 95 +++++++++++++++++++++++++++++ codersdk/workspaces.go | 14 ++++- site/src/api/api.ts | 6 ++ 10 files changed, 268 insertions(+), 52 deletions(-) diff --git a/cli/cliui/provisionerjob.go b/cli/cliui/provisionerjob.go index 36efa04a8a91a..6b7633f569b9f 100644 --- a/cli/cliui/provisionerjob.go +++ b/cli/cliui/provisionerjob.go @@ -19,6 +19,9 @@ import ( ) func WorkspaceBuild(ctx context.Context, writer io.Writer, client *codersdk.Client, build uuid.UUID) error { + if build == uuid.Nil { + return nil + } return ProvisionerJob(ctx, writer, ProvisionerJobOptions{ Fetch: func() (codersdk.ProvisionerJob, error) { build, err := client.WorkspaceBuild(ctx, build) diff --git a/cli/cliutil/provisionerwarn.go b/cli/cliutil/provisionerwarn.go index 861add25f7d31..9ddf8a565ca72 100644 --- a/cli/cliutil/provisionerwarn.go +++ b/cli/cliutil/provisionerwarn.go @@ -6,6 +6,8 @@ import ( "io" "strings" + "github.com/google/uuid" + "github.com/coder/coder/v2/cli/cliui" "github.com/coder/coder/v2/codersdk" ) @@ -26,12 +28,15 @@ Details: // WarnMatchedProvisioners warns the user if there are no provisioners that // match the requested tags for a given provisioner job. -// If the job is not pending, it is ignored. +// If the job is not pending or its ID is nil, it is ignored. func WarnMatchedProvisioners(w io.Writer, mp *codersdk.MatchedProvisioners, job codersdk.ProvisionerJob) { if mp == nil { // Nothing in the response, nothing to do here! return } + if job.ID == uuid.Nil { + return + } if job.Status != codersdk.ProvisionerJobPending { // Only warn if the job is pending. return diff --git a/cli/cliutil/provisionerwarn_test.go b/cli/cliutil/provisionerwarn_test.go index a737223310d75..b0a540f562832 100644 --- a/cli/cliutil/provisionerwarn_test.go +++ b/cli/cliutil/provisionerwarn_test.go @@ -4,6 +4,7 @@ import ( "strings" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/require" "github.com/coder/coder/v2/cli/cliutil" @@ -19,6 +20,12 @@ func TestWarnMatchedProvisioners(t *testing.T) { job codersdk.ProvisionerJob expect string }{ + { + name: "empty", + mp: nil, + job: codersdk.ProvisionerJob{}, + expect: "", + }, { name: "no_match", mp: &codersdk.MatchedProvisioners{ @@ -26,6 +33,7 @@ func TestWarnMatchedProvisioners(t *testing.T) { Available: 0, }, job: codersdk.ProvisionerJob{ + ID: uuid.New(), Status: codersdk.ProvisionerJobPending, }, expect: `there are no provisioners that accept the required tags`, @@ -37,6 +45,7 @@ func TestWarnMatchedProvisioners(t *testing.T) { Available: 0, }, job: codersdk.ProvisionerJob{ + ID: uuid.New(), Status: codersdk.ProvisionerJobPending, }, expect: `Provisioners that accept the required tags have not responded for longer than expected`, @@ -48,6 +57,7 @@ func TestWarnMatchedProvisioners(t *testing.T) { Available: 1, }, job: codersdk.ProvisionerJob{ + ID: uuid.New(), Status: codersdk.ProvisionerJobPending, }, }, @@ -55,6 +65,7 @@ func TestWarnMatchedProvisioners(t *testing.T) { name: "not_pending", mp: &codersdk.MatchedProvisioners{}, job: codersdk.ProvisionerJob{ + ID: uuid.New(), Status: codersdk.ProvisionerJobRunning, }, }, diff --git a/cli/delete_test.go b/cli/delete_test.go index 1d4dc8dfb40ad..1f9be4f566c73 100644 --- a/cli/delete_test.go +++ b/cli/delete_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "net/http" "testing" "github.com/stretchr/testify/assert" @@ -49,30 +50,42 @@ func TestDelete(t *testing.T) { t.Run("Orphan", func(t *testing.T) { t.Parallel() - client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) + client, closer := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) owner := coderdtest.CreateFirstUser(t, client) - version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) - template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin()) + version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, nil) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) + template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID) + workspace := coderdtest.CreateWorkspace(t, templateAdmin, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, workspace.LatestBuild.ID) + + // Ensure the provisioner daemon is closed before running the test. + // This is to validate that the orphan deletion does not require + // a provisioner job. + require.NoError(t, closer.Close()) + + ctx := testutil.Context(t, testutil.WaitShort) inv, root := clitest.New(t, "delete", workspace.Name, "-y", "--orphan") + clitest.SetupConfig(t, templateAdmin, root) - //nolint:gocritic // Deleting orphaned workspaces requires an admin. - clitest.SetupConfig(t, client, root) doneChan := make(chan struct{}) pty := ptytest.New(t).Attach(inv) inv.Stderr = pty.Output() go func() { defer close(doneChan) - err := inv.Run() + err := inv.WithContext(ctx).Run() // When running with the race detector on, we sometimes get an EOF. if err != nil { assert.ErrorIs(t, err, io.EOF) } }() pty.ExpectMatch("has been deleted") - <-doneChan + testutil.TryReceive(ctx, t, doneChan) + + _, err := client.Workspace(ctx, workspace.ID) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusGone, cerr.StatusCode()) }) // Super orphaned, as the workspace doesn't even have a user. diff --git a/coderd/workspacebuilds.go b/coderd/workspacebuilds.go index 4d90948a8f9a1..3b2d3bb0f47fa 100644 --- a/coderd/workspacebuilds.go +++ b/coderd/workspacebuilds.go @@ -365,6 +365,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { builder = builder.VersionID(createBuild.TemplateVersionID) } + // Deleting with the --orphan flag means we will not attempt to create a + // provisioner job, and we will just mark the workspace as deleted. if createBuild.Orphan { if createBuild.Transition != codersdk.WorkspaceTransitionDelete { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ @@ -430,29 +432,37 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { } } - apiBuild, err := api.convertWorkspaceBuild( - *workspaceBuild, - workspace, - database.GetProvisionerJobsByIDsWithQueuePositionRow{ - ProvisionerJob: *provisionerJob, - QueuePosition: 0, - }, - []database.WorkspaceResource{}, - []database.WorkspaceResourceMetadatum{}, - []database.WorkspaceAgent{}, - []database.WorkspaceApp{}, - []database.WorkspaceAppStatus{}, - []database.WorkspaceAgentScript{}, - []database.WorkspaceAgentLogSource{}, - database.TemplateVersion{}, - provisionerDaemons, - ) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error converting workspace build.", - Detail: err.Error(), - }) - return + if workspaceBuild == nil { + // IETF suggests 204 No Content for successful DELETE requests + // This isn't an actual DELETE request, but it is a request to delete. + httpapi.Write(ctx, rw, http.StatusNoContent, nil) + } else { + apiBuild, err := api.convertWorkspaceBuild( + *workspaceBuild, + workspace, + database.GetProvisionerJobsByIDsWithQueuePositionRow{ + ProvisionerJob: *provisionerJob, + QueuePosition: 0, + }, + []database.WorkspaceResource{}, + []database.WorkspaceResourceMetadatum{}, + []database.WorkspaceAgent{}, + []database.WorkspaceApp{}, + []database.WorkspaceAppStatus{}, + []database.WorkspaceAgentScript{}, + []database.WorkspaceAgentLogSource{}, + database.TemplateVersion{}, + provisionerDaemons, + ) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error converting workspace build.", + Detail: err.Error(), + }) + // No early return here as we still want to publish the workspace update. + } else { + httpapi.Write(ctx, rw, http.StatusCreated, apiBuild) + } } // If this workspace build has a different template version ID to the previous build @@ -478,8 +488,6 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) { Kind: wspubsub.WorkspaceEventKindStateChange, WorkspaceID: workspace.ID, }) - - httpapi.Write(ctx, rw, http.StatusCreated, apiBuild) } func (api *API) notifyWorkspaceUpdated( diff --git a/coderd/workspacebuilds_test.go b/coderd/workspacebuilds_test.go index ac33c9e92c4f7..00d76115b4863 100644 --- a/coderd/workspacebuilds_test.go +++ b/coderd/workspacebuilds_test.go @@ -373,40 +373,66 @@ func TestWorkspaceBuildsProvisionerState(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true}) first := coderdtest.CreateFirstUser(t, client) + templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin()) + member, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID) ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil) - template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID) - coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID) + version := coderdtest.CreateTemplateVersion(t, templateAdmin, first.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, templateAdmin, first.OrganizationID, version.ID) + coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID) - workspace := coderdtest.CreateWorkspace(t, client, template.ID) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID) + workspace := coderdtest.CreateWorkspace(t, templateAdmin, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, workspace.LatestBuild.ID) - // Providing both state and orphan fails. + // Trying to orphan without delete transition fails. _, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: workspace.LatestBuild.TemplateVersionID, + Transition: codersdk.WorkspaceTransitionStart, + Orphan: true, + }) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + + // Providing both state and orphan fails. + _, err = client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: workspace.LatestBuild.TemplateVersionID, Transition: codersdk.WorkspaceTransitionDelete, ProvisionerState: []byte(" "), Orphan: true, }) require.Error(t, err) - cerr := coderdtest.SDKError(t, err) + cerr = coderdtest.SDKError(t, err) require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + // Trying to orphan without being a template admin fails. + memberWorkspace := coderdtest.CreateWorkspace(t, member, template.ID) + coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, memberWorkspace.LatestBuild.ID) + _, err = member.CreateWorkspaceBuild(ctx, memberWorkspace.ID, codersdk.CreateWorkspaceBuildRequest{ + TemplateVersionID: workspace.LatestBuild.TemplateVersionID, + Transition: codersdk.WorkspaceTransitionDelete, + Orphan: true, + }) + require.Error(t, err) + cerr = coderdtest.SDKError(t, err) + require.Equal(t, http.StatusForbidden, cerr.StatusCode()) + require.Contains(t, cerr.Message, "Only template managers may orphan") + // Regular orphan operation succeeds. - build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ + build, err := templateAdmin.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{ TemplateVersionID: workspace.LatestBuild.TemplateVersionID, Transition: codersdk.WorkspaceTransitionDelete, Orphan: true, }) require.NoError(t, err) - coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID) + require.Empty(t, build) - _, err = client.Workspace(ctx, workspace.ID) - require.Error(t, err) + ws, err := client.Workspace(ctx, workspace.ID) + require.Empty(t, ws) require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode()) + require.Error(t, err) }) } diff --git a/coderd/wsbuilder/wsbuilder.go b/coderd/wsbuilder/wsbuilder.go index 9605df58014de..1f5eff22bcfc4 100644 --- a/coderd/wsbuilder/wsbuilder.go +++ b/coderd/wsbuilder/wsbuilder.go @@ -108,7 +108,7 @@ type versionTarget struct { // The zero value of this struct means to use state from the last build. If there is no last build, no state is // provided (i.e. first build on a newly created workspace). // -// setting orphan: true means not to send any state. This can be used to deleted orphaned workspaces +// setting orphan: true means not to not create any build job and immediately mark the workspace as deleted. // // setting explicit to a non-nil value means to use the provided state // @@ -335,6 +335,31 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object b.reason = database.BuildReasonInitiator } + // Deleting a workspace with orphan set does not require creating any + // workspace build or provisioner job. In fact, creating a workspace build + // job in this case would be counter-productive. + if b.state.orphan { + if b.trans != database.WorkspaceTransitionDelete { + // This may also be checked by the caller, but we check it here for the + // sake of completeness. + return nil, nil, nil, BuildError{ + Status: http.StatusBadRequest, + Message: "Orphan can only be set when deleting a workspace", + Wrapped: xerrors.New("orphan can only be set when deleting a workspace"), + } + } + if err := b.store.UpdateWorkspaceDeletedByID(b.ctx, database.UpdateWorkspaceDeletedByIDParams{ + ID: b.workspace.ID, + Deleted: true, + }); err != nil { + return nil, nil, nil, BuildError{ + Status: http.StatusInternalServerError, + Message: "Internal error marking workspace as deleted", + } + } + return nil, nil, nil, nil + } + workspaceBuildID := uuid.New() input, err := json.Marshal(provisionerdserver.WorkspaceProvisionJob{ WorkspaceBuildID: workspaceBuildID, @@ -939,11 +964,27 @@ func (b *Builder) authorize(authFunc func(action policy.Action, object rbac.Obje // If custom state, deny request since user could be corrupting or leaking // cloud state. - if b.state.explicit != nil || b.state.orphan { + if b.state.explicit != nil { if !authFunc(policy.ActionUpdate, template.RBACObject()) { return BuildError{http.StatusForbidden, "Only template managers may provide custom state", xerrors.New("Only template managers may provide custom state")} } } + // Orphaning can only be done by template managers, since it may require + // manual cleanup of cloud resource. + if b.state.orphan { + if !authFunc(policy.ActionUpdate, template.RBACObject()) { + return BuildError{http.StatusForbidden, "Only template managers may orphan", xerrors.New("Only template managers may orphan")} + } + } + + // Requesting both custom state and orphan is unclear, so deny it. + if b.state.explicit != nil && b.state.orphan { + return BuildError{ + http.StatusBadRequest, + "Orphaning a workspace build with custom state is not allowed as the intent is unclear.", + xerrors.New("Orphaning a workspace build with custom state is not allowed as the intent is unclear."), + } + } if b.logLevel != "" && !authFunc(policy.ActionRead, rbac.ResourceDeploymentConfig) { return BuildError{ diff --git a/coderd/wsbuilder/wsbuilder_test.go b/coderd/wsbuilder/wsbuilder_test.go index 58999a33e6e5e..672dc1d3939bf 100644 --- a/coderd/wsbuilder/wsbuilder_test.go +++ b/coderd/wsbuilder/wsbuilder_test.go @@ -865,6 +865,101 @@ func TestProvisionerVersionSupportsDynamicParameters(t *testing.T) { } } +func TestBuilder_Orphan(t *testing.T) { + t.Parallel() + + expects := func(mTx *dbmock.MockStore) { + mTx.EXPECT().GetTemplateByID(gomock.Any(), templateID). + Times(1). + Return(database.Template{ + ID: templateID, + OrganizationID: orgID, + Provisioner: database.ProvisionerTypeTerraform, + ActiveVersionID: activeVersionID, + UseClassicParameterFlow: true, + }, nil) + mTx.EXPECT().GetTemplateVersionByID(gomock.Any(), activeVersionID). + Times(1). + Return(database.TemplateVersion{ + ID: activeVersionID, + TemplateID: uuid.NullUUID{UUID: templateID, Valid: true}, + OrganizationID: orgID, + Name: "active", + JobID: lastBuildJobID, + }, nil) + mTx.EXPECT().GetLatestWorkspaceBuildByWorkspaceID(gomock.Any(), workspaceID). + Times(1). + Return(database.WorkspaceBuild{ + ID: lastBuildID, + WorkspaceID: workspaceID, + TemplateVersionID: activeVersionID, + BuildNumber: 1, + Transition: database.WorkspaceTransitionStart, + InitiatorID: userID, + JobID: lastBuildJobID, + ProvisionerState: []byte("last build state"), + Reason: database.BuildReasonInitiator, + }, nil) + + mTx.EXPECT().GetProvisionerJobByID(gomock.Any(), lastBuildJobID). + Times(2). + Return(database.ProvisionerJob{ + ID: lastBuildJobID, + OrganizationID: orgID, + InitiatorID: userID, + Provisioner: database.ProvisionerTypeTerraform, + StorageMethod: database.ProvisionerStorageMethodFile, + FileID: activeFileID, + Type: database.ProvisionerJobTypeWorkspaceBuild, + StartedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, + UpdatedAt: time.Now(), + CompletedAt: sql.NullTime{Time: dbtime.Now(), Valid: true}, + }, nil) + } + + t.Run("OK", func(t *testing.T) { + t.Parallel() + req := require.New(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + mDB := expectDB(t, + // Inputs + expects, + // Outputs + func(mTx *dbmock.MockStore) { + mTx.EXPECT().UpdateWorkspaceDeletedByID(gomock.Any(), database.UpdateWorkspaceDeletedByIDParams{ + ID: workspaceID, + Deleted: true, + }).Times(1).Return(nil) + }, + ) + + ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} + uut := wsbuilder.New(ws, database.WorkspaceTransitionDelete).Orphan() + // nolint: dogsled + _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + req.NoError(err) + }) + + t.Run("NotDelete", func(t *testing.T) { + t.Parallel() + req := require.New(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + mDB := expectDB(t, + // Inputs + expects, + // No Outputs + ) + + ws := database.Workspace{ID: workspaceID, TemplateID: templateID, OwnerID: userID} + uut := wsbuilder.New(ws, database.WorkspaceTransitionStart).Orphan() + // nolint: dogsled + _, _, _, err := uut.Build(ctx, mDB, nil, audit.WorkspaceBuildBaggage{}) + req.ErrorContains(err, "orphan can only be set when deleting a workspace") + }) +} + type txExpect func(mTx *dbmock.MockStore) func expectDB(t *testing.T, opts ...txExpect) *dbmock.MockStore { diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index 2c73d60a2696c..9f60dbc75d34e 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -188,11 +188,19 @@ func (c *Client) CreateWorkspaceBuild(ctx context.Context, workspace uuid.UUID, return WorkspaceBuild{}, err } defer res.Body.Close() - if res.StatusCode != http.StatusCreated { + switch res.StatusCode { + case http.StatusCreated: + var workspaceBuild WorkspaceBuild + return workspaceBuild, json.NewDecoder(res.Body).Decode(&workspaceBuild) + case http.StatusNoContent: + // 204 No Content is only expected in response to an orphan-delete. + if request.Transition == WorkspaceTransitionDelete && request.Orphan { + return WorkspaceBuild{}, nil + } + fallthrough + default: return WorkspaceBuild{}, ReadBodyAsError(res) } - var workspaceBuild WorkspaceBuild - return workspaceBuild, json.NewDecoder(res.Body).Decode(&workspaceBuild) } func (c *Client) WatchWorkspace(ctx context.Context, id uuid.UUID) (<-chan Workspace, error) { diff --git a/site/src/api/api.ts b/site/src/api/api.ts index 5b7cde65fb2ce..8a9d25eaf69ee 100644 --- a/site/src/api/api.ts +++ b/site/src/api/api.ts @@ -1235,6 +1235,12 @@ class ApiMethods { data, ); + if (response.status === 204) { + if (data.transition === "delete" && data.orphan) { + return {} as TypesGen.WorkspaceBuild; + } + } + return response.data; };