Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix!: do not create provisioner job for orphan delete#18460

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
johnstcn wants to merge1 commit intomainfromcj/delete-orphan-no-build
Closed
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletionscli/cliui/provisionerjob.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
Expand Down
7 changes: 6 additions & 1 deletioncli/cliutil/provisionerwarn.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,8 @@ import (
"io"
"strings"

"github.com/google/uuid"

"github.com/coder/coder/v2/cli/cliui"
"github.com/coder/coder/v2/codersdk"
)
Expand All@@ -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
Expand Down
11 changes: 11 additions & 0 deletionscli/cliutil/provisionerwarn_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,6 +4,7 @@ import (
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/require"

"github.com/coder/coder/v2/cli/cliutil"
Expand All@@ -19,13 +20,20 @@ func TestWarnMatchedProvisioners(t *testing.T) {
job codersdk.ProvisionerJob
expect string
}{
{
name: "empty",
mp: nil,
job: codersdk.ProvisionerJob{},
expect: "",
},
{
name: "no_match",
mp: &codersdk.MatchedProvisioners{
Count: 0,
Available: 0,
},
job: codersdk.ProvisionerJob{
ID: uuid.New(),
Status: codersdk.ProvisionerJobPending,
},
expect: `there are no provisioners that accept the required tags`,
Expand All@@ -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`,
Expand All@@ -48,13 +57,15 @@ func TestWarnMatchedProvisioners(t *testing.T) {
Available: 1,
},
job: codersdk.ProvisionerJob{
ID: uuid.New(),
Status: codersdk.ProvisionerJobPending,
},
},
{
name: "not_pending",
mp: &codersdk.MatchedProvisioners{},
job: codersdk.ProvisionerJob{
ID: uuid.New(),
Status: codersdk.ProvisionerJobRunning,
},
},
Expand Down
33 changes: 23 additions & 10 deletionscli/delete_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -4,6 +4,7 @@ import (
"context"
"fmt"
"io"
"net/http"
"testing"

"github.com/stretchr/testify/assert"
Expand DownExpand Up@@ -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.
Expand Down
58 changes: 33 additions & 25 deletionscoderd/workspacebuilds.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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{
Expand DownExpand Up@@ -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
Expand All@@ -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(
Expand Down
48 changes: 37 additions & 11 deletionscoderd/workspacebuilds_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)
})
}

Expand Down
45 changes: 43 additions & 2 deletionscoderd/wsbuilder/wsbuilder.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 tosendanystate. This can be used to deleted orphaned workspaces
// setting orphan: true means not tonot createanybuild job and immediately mark the workspace as deleted.
//
// setting explicit to a non-nil value means to use the provided state
//
Expand DownExpand Up@@ -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,
Expand DownExpand Up@@ -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{
Expand Down
Loading
Loading
[8]ページ先頭
©2009-2025 Movatter.jp