Skip to content

Commit 656d4f4

Browse files
johnstcnjohnstcn
committed
fix(coderd): do not create provisioner job for orphan delete
1 parent 511fd09 commit 656d4f4

File tree

10 files changed

+268
-52
lines changed

10 files changed

+268
-52
lines changed

‎cli/cliui/provisionerjob.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ import (
1919
)
2020

2121
func WorkspaceBuild(ctx context.Context, writer io.Writer, client *codersdk.Client, build uuid.UUID) error {
22+
if build == uuid.Nil {
23+
return nil
24+
}
2225
return ProvisionerJob(ctx, writer, ProvisionerJobOptions{
2326
Fetch: func() (codersdk.ProvisionerJob, error) {
2427
build, err := client.WorkspaceBuild(ctx, build)

‎cli/cliutil/provisionerwarn.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ import (
66
"io"
77
"strings"
88

9+
"github.com/google/uuid"
10+
911
"github.com/coder/coder/v2/cli/cliui"
1012
"github.com/coder/coder/v2/codersdk"
1113
)
@@ -26,12 +28,15 @@ Details:
2628

2729
// WarnMatchedProvisioners warns the user if there are no provisioners that
2830
// match the requested tags for a given provisioner job.
29-
// If the job is not pending, it is ignored.
31+
// If the job is not pending or its ID is nil, it is ignored.
3032
func WarnMatchedProvisioners(w io.Writer, mp *codersdk.MatchedProvisioners, job codersdk.ProvisionerJob) {
3133
if mp == nil {
3234
// Nothing in the response, nothing to do here!
3335
return
3436
}
37+
if job.ID == uuid.Nil {
38+
return
39+
}
3540
if job.Status != codersdk.ProvisionerJobPending {
3641
// Only warn if the job is pending.
3742
return

‎cli/cliutil/provisionerwarn_test.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"strings"
55
"testing"
66

7+
"github.com/google/uuid"
78
"github.com/stretchr/testify/require"
89

910
"github.com/coder/coder/v2/cli/cliutil"
@@ -19,13 +20,20 @@ func TestWarnMatchedProvisioners(t *testing.T) {
1920
job codersdk.ProvisionerJob
2021
expect string
2122
}{
23+
{
24+
name: "empty",
25+
mp: nil,
26+
job: codersdk.ProvisionerJob{},
27+
expect: "",
28+
},
2229
{
2330
name: "no_match",
2431
mp: &codersdk.MatchedProvisioners{
2532
Count: 0,
2633
Available: 0,
2734
},
2835
job: codersdk.ProvisionerJob{
36+
ID: uuid.New(),
2937
Status: codersdk.ProvisionerJobPending,
3038
},
3139
expect: `there are no provisioners that accept the required tags`,
@@ -37,6 +45,7 @@ func TestWarnMatchedProvisioners(t *testing.T) {
3745
Available: 0,
3846
},
3947
job: codersdk.ProvisionerJob{
48+
ID: uuid.New(),
4049
Status: codersdk.ProvisionerJobPending,
4150
},
4251
expect: `Provisioners that accept the required tags have not responded for longer than expected`,
@@ -48,13 +57,15 @@ func TestWarnMatchedProvisioners(t *testing.T) {
4857
Available: 1,
4958
},
5059
job: codersdk.ProvisionerJob{
60+
ID: uuid.New(),
5161
Status: codersdk.ProvisionerJobPending,
5262
},
5363
},
5464
{
5565
name: "not_pending",
5666
mp: &codersdk.MatchedProvisioners{},
5767
job: codersdk.ProvisionerJob{
68+
ID: uuid.New(),
5869
Status: codersdk.ProvisionerJobRunning,
5970
},
6071
},

‎cli/delete_test.go

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"net/http"
78
"testing"
89

910
"github.com/stretchr/testify/assert"
@@ -49,30 +50,42 @@ func TestDelete(t *testing.T) {
4950

5051
t.Run("Orphan", func(t *testing.T) {
5152
t.Parallel()
52-
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
53+
client, closer := coderdtest.NewWithProvisionerCloser(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
5354
owner := coderdtest.CreateFirstUser(t, client)
54-
version := coderdtest.CreateTemplateVersion(t, client, owner.OrganizationID, nil)
55-
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
56-
template := coderdtest.CreateTemplate(t, client, owner.OrganizationID, version.ID)
57-
workspace := coderdtest.CreateWorkspace(t, client, template.ID)
58-
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
55+
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleTemplateAdmin())
56+
version := coderdtest.CreateTemplateVersion(t, templateAdmin, owner.OrganizationID, nil)
57+
coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID)
58+
template := coderdtest.CreateTemplate(t, templateAdmin, owner.OrganizationID, version.ID)
59+
workspace := coderdtest.CreateWorkspace(t, templateAdmin, template.ID)
60+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, workspace.LatestBuild.ID)
61+
62+
// Ensure the provisioner daemon is closed before running the test.
63+
// This is to validate that the orphan deletion does not require
64+
// a provisioner job.
65+
require.NoError(t, closer.Close())
66+
67+
ctx := testutil.Context(t, testutil.WaitShort)
5968
inv, root := clitest.New(t, "delete", workspace.Name, "-y", "--orphan")
69+
clitest.SetupConfig(t, templateAdmin, root)
6070

61-
//nolint:gocritic // Deleting orphaned workspaces requires an admin.
62-
clitest.SetupConfig(t, client, root)
6371
doneChan := make(chan struct{})
6472
pty := ptytest.New(t).Attach(inv)
6573
inv.Stderr = pty.Output()
6674
go func() {
6775
defer close(doneChan)
68-
err := inv.Run()
76+
err := inv.WithContext(ctx).Run()
6977
// When running with the race detector on, we sometimes get an EOF.
7078
if err != nil {
7179
assert.ErrorIs(t, err, io.EOF)
7280
}
7381
}()
7482
pty.ExpectMatch("has been deleted")
75-
<-doneChan
83+
testutil.TryReceive(ctx, t, doneChan)
84+
85+
_, err := client.Workspace(ctx, workspace.ID)
86+
require.Error(t, err)
87+
cerr := coderdtest.SDKError(t, err)
88+
require.Equal(t, http.StatusGone, cerr.StatusCode())
7689
})
7790

7891
// Super orphaned, as the workspace doesn't even have a user.

‎coderd/workspacebuilds.go

Lines changed: 33 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,8 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
365365
builder = builder.VersionID(createBuild.TemplateVersionID)
366366
}
367367

368+
// Deleting with the --orphan flag means we will not attempt to create a
369+
// provisioner job, and we will just mark the workspace as deleted.
368370
if createBuild.Orphan {
369371
if createBuild.Transition != codersdk.WorkspaceTransitionDelete {
370372
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
@@ -430,29 +432,37 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
430432
}
431433
}
432434

433-
apiBuild, err := api.convertWorkspaceBuild(
434-
*workspaceBuild,
435-
workspace,
436-
database.GetProvisionerJobsByIDsWithQueuePositionRow{
437-
ProvisionerJob: *provisionerJob,
438-
QueuePosition: 0,
439-
},
440-
[]database.WorkspaceResource{},
441-
[]database.WorkspaceResourceMetadatum{},
442-
[]database.WorkspaceAgent{},
443-
[]database.WorkspaceApp{},
444-
[]database.WorkspaceAppStatus{},
445-
[]database.WorkspaceAgentScript{},
446-
[]database.WorkspaceAgentLogSource{},
447-
database.TemplateVersion{},
448-
provisionerDaemons,
449-
)
450-
if err != nil {
451-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
452-
Message: "Internal error converting workspace build.",
453-
Detail: err.Error(),
454-
})
455-
return
435+
if workspaceBuild == nil {
436+
// IETF suggests 204 No Content for successful DELETE requests
437+
// This isn't an actual DELETE request, but it is a request to delete.
438+
httpapi.Write(ctx, rw, http.StatusNoContent, nil)
439+
} else {
440+
apiBuild, err := api.convertWorkspaceBuild(
441+
*workspaceBuild,
442+
workspace,
443+
database.GetProvisionerJobsByIDsWithQueuePositionRow{
444+
ProvisionerJob: *provisionerJob,
445+
QueuePosition: 0,
446+
},
447+
[]database.WorkspaceResource{},
448+
[]database.WorkspaceResourceMetadatum{},
449+
[]database.WorkspaceAgent{},
450+
[]database.WorkspaceApp{},
451+
[]database.WorkspaceAppStatus{},
452+
[]database.WorkspaceAgentScript{},
453+
[]database.WorkspaceAgentLogSource{},
454+
database.TemplateVersion{},
455+
provisionerDaemons,
456+
)
457+
if err != nil {
458+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
459+
Message: "Internal error converting workspace build.",
460+
Detail: err.Error(),
461+
})
462+
// No early return here as we still want to publish the workspace update.
463+
} else {
464+
httpapi.Write(ctx, rw, http.StatusCreated, apiBuild)
465+
}
456466
}
457467

458468
// 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) {
478488
Kind: wspubsub.WorkspaceEventKindStateChange,
479489
WorkspaceID: workspace.ID,
480490
})
481-
482-
httpapi.Write(ctx, rw, http.StatusCreated, apiBuild)
483491
}
484492

485493
func (api *API) notifyWorkspaceUpdated(

‎coderd/workspacebuilds_test.go

Lines changed: 37 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -373,40 +373,66 @@ func TestWorkspaceBuildsProvisionerState(t *testing.T) {
373373
t.Parallel()
374374
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
375375
first := coderdtest.CreateFirstUser(t, client)
376+
templateAdmin, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID, rbac.RoleTemplateAdmin())
377+
member, _ := coderdtest.CreateAnotherUser(t, client, first.OrganizationID)
376378

377379
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
378380
defer cancel()
379381

380-
version := coderdtest.CreateTemplateVersion(t, client, first.OrganizationID, nil)
381-
template := coderdtest.CreateTemplate(t, client, first.OrganizationID, version.ID)
382-
coderdtest.AwaitTemplateVersionJobCompleted(t, client, version.ID)
382+
version := coderdtest.CreateTemplateVersion(t, templateAdmin, first.OrganizationID, nil)
383+
template := coderdtest.CreateTemplate(t, templateAdmin, first.OrganizationID, version.ID)
384+
coderdtest.AwaitTemplateVersionJobCompleted(t, templateAdmin, version.ID)
383385

384-
workspace := coderdtest.CreateWorkspace(t, client, template.ID)
385-
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, workspace.LatestBuild.ID)
386+
workspace := coderdtest.CreateWorkspace(t, templateAdmin, template.ID)
387+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, templateAdmin, workspace.LatestBuild.ID)
386388

387-
// Providing both state and orphan fails.
389+
// Trying to orphan without delete transition fails.
388390
_, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
391+
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
392+
Transition: codersdk.WorkspaceTransitionStart,
393+
Orphan: true,
394+
})
395+
require.Error(t, err)
396+
cerr := coderdtest.SDKError(t, err)
397+
require.Equal(t, http.StatusBadRequest, cerr.StatusCode())
398+
399+
// Providing both state and orphan fails.
400+
_, err = client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
389401
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
390402
Transition: codersdk.WorkspaceTransitionDelete,
391403
ProvisionerState: []byte(" "),
392404
Orphan: true,
393405
})
394406
require.Error(t, err)
395-
cerr := coderdtest.SDKError(t, err)
407+
cerr = coderdtest.SDKError(t, err)
396408
require.Equal(t, http.StatusBadRequest, cerr.StatusCode())
397409

410+
// Trying to orphan without being a template admin fails.
411+
memberWorkspace := coderdtest.CreateWorkspace(t, member, template.ID)
412+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, member, memberWorkspace.LatestBuild.ID)
413+
_, err = member.CreateWorkspaceBuild(ctx, memberWorkspace.ID, codersdk.CreateWorkspaceBuildRequest{
414+
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
415+
Transition: codersdk.WorkspaceTransitionDelete,
416+
Orphan: true,
417+
})
418+
require.Error(t, err)
419+
cerr = coderdtest.SDKError(t, err)
420+
require.Equal(t, http.StatusForbidden, cerr.StatusCode())
421+
require.Contains(t, cerr.Message, "Only template managers may orphan")
422+
398423
// Regular orphan operation succeeds.
399-
build, err := client.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
424+
build, err := templateAdmin.CreateWorkspaceBuild(ctx, workspace.ID, codersdk.CreateWorkspaceBuildRequest{
400425
TemplateVersionID: workspace.LatestBuild.TemplateVersionID,
401426
Transition: codersdk.WorkspaceTransitionDelete,
402427
Orphan: true,
403428
})
404429
require.NoError(t, err)
405-
coderdtest.AwaitWorkspaceBuildJobCompleted(t, client, build.ID)
430+
require.Empty(t, build)
406431

407-
_, err = client.Workspace(ctx, workspace.ID)
408-
require.Error(t, err)
432+
ws, err := client.Workspace(ctx, workspace.ID)
433+
require.Empty(t, ws)
409434
require.Equal(t, http.StatusGone, coderdtest.SDKError(t, err).StatusCode())
435+
require.Error(t, err)
410436
})
411437
}
412438

‎coderd/wsbuilder/wsbuilder.go

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ type versionTarget struct {
108108
// The zero value of this struct means to use state from the last build. If there is no last build, no state is
109109
// provided (i.e. first build on a newly created workspace).
110110
//
111-
// setting orphan: true means not to send any state. This can be used to deleted orphaned workspaces
111+
// setting orphan: true means not to not create any build job and immediately mark the workspace as deleted.
112112
//
113113
// setting explicit to a non-nil value means to use the provided state
114114
//
@@ -335,6 +335,31 @@ func (b *Builder) buildTx(authFunc func(action policy.Action, object rbac.Object
335335
b.reason = database.BuildReasonInitiator
336336
}
337337

338+
// Deleting a workspace with orphan set does not require creating any
339+
// workspace build or provisioner job. In fact, creating a workspace build
340+
// job in this case would be counter-productive.
341+
if b.state.orphan {
342+
if b.trans != database.WorkspaceTransitionDelete {
343+
// This may also be checked by the caller, but we check it here for the
344+
// sake of completeness.
345+
return nil, nil, nil, BuildError{
346+
Status: http.StatusBadRequest,
347+
Message: "Orphan can only be set when deleting a workspace",
348+
Wrapped: xerrors.New("orphan can only be set when deleting a workspace"),
349+
}
350+
}
351+
if err := b.store.UpdateWorkspaceDeletedByID(b.ctx, database.UpdateWorkspaceDeletedByIDParams{
352+
ID: b.workspace.ID,
353+
Deleted: true,
354+
}); err != nil {
355+
return nil, nil, nil, BuildError{
356+
Status: http.StatusInternalServerError,
357+
Message: "Internal error marking workspace as deleted",
358+
}
359+
}
360+
return nil, nil, nil, nil
361+
}
362+
338363
workspaceBuildID := uuid.New()
339364
input, err := json.Marshal(provisionerdserver.WorkspaceProvisionJob{
340365
WorkspaceBuildID: workspaceBuildID,
@@ -939,11 +964,27 @@ func (b *Builder) authorize(authFunc func(action policy.Action, object rbac.Obje
939964

940965
// If custom state, deny request since user could be corrupting or leaking
941966
// cloud state.
942-
if b.state.explicit != nil || b.state.orphan {
967+
if b.state.explicit != nil {
943968
if !authFunc(policy.ActionUpdate, template.RBACObject()) {
944969
return BuildError{http.StatusForbidden, "Only template managers may provide custom state", xerrors.New("Only template managers may provide custom state")}
945970
}
946971
}
972+
// Orphaning can only be done by template managers, since it may require
973+
// manual cleanup of cloud resource.
974+
if b.state.orphan {
975+
if !authFunc(policy.ActionUpdate, template.RBACObject()) {
976+
return BuildError{http.StatusForbidden, "Only template managers may orphan", xerrors.New("Only template managers may orphan")}
977+
}
978+
}
979+
980+
// Requesting both custom state and orphan is unclear, so deny it.
981+
if b.state.explicit != nil && b.state.orphan {
982+
return BuildError{
983+
http.StatusBadRequest,
984+
"Orphaning a workspace build with custom state is not allowed as the intent is unclear.",
985+
xerrors.New("Orphaning a workspace build with custom state is not allowed as the intent is unclear."),
986+
}
987+
}
947988

948989
if b.logLevel != "" && !authFunc(policy.ActionRead, rbac.ResourceDeploymentConfig) {
949990
return BuildError{

0 commit comments

Comments
 (0)