From 29a731375e366b05068edef89380efbac35d3e98 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Fri, 29 Aug 2025 14:17:33 +0100 Subject: [PATCH] refactor: untangle workspace creation from http logic (#19639) Coder Tasks requires us to create a workspace, but we want to be able to return a `codersdk.Task` instead of a `codersdk.Workspace`. This requires untangling `createWorkspace` from directly writing to `http.ResponseWriter`. --- coderd/aitasks.go | 14 +++- coderd/httpapi/httperror/responserror.go | 49 +++++++++++ coderd/workspaces.go | 100 +++++++++++------------ 3 files changed, 106 insertions(+), 57 deletions(-) diff --git a/coderd/aitasks.go b/coderd/aitasks.go index 67f54ca119..c736998b7a 100644 --- a/coderd/aitasks.go +++ b/coderd/aitasks.go @@ -17,6 +17,7 @@ import ( "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/httpapi" + "github.com/coder/coder/v2/coderd/httpapi/httperror" "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/rbac/policy" @@ -154,8 +155,9 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) { // This can be optimized. It exists as it is now for code simplicity. // The most common case is to create a workspace for 'Me'. Which does // not enter this code branch. - template, ok := requestTemplate(ctx, rw, createReq, api.Database) - if !ok { + template, err := requestTemplate(ctx, createReq, api.Database) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) return } @@ -188,7 +190,13 @@ func (api *API) tasksCreate(rw http.ResponseWriter, r *http.Request) { }) defer commitAudit() - createWorkspace(ctx, aReq, apiKey.UserID, api, owner, createReq, rw, r) + w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, createReq, r) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusCreated, w) } // tasksFromWorkspaces converts a slice of API workspaces into tasks, fetching diff --git a/coderd/httpapi/httperror/responserror.go b/coderd/httpapi/httperror/responserror.go index be219f538b..000089b6d0 100644 --- a/coderd/httpapi/httperror/responserror.go +++ b/coderd/httpapi/httperror/responserror.go @@ -1,8 +1,12 @@ package httperror import ( + "context" "errors" + "fmt" + "net/http" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" ) @@ -17,3 +21,48 @@ func IsResponder(err error) (Responder, bool) { } return nil, false } + +func NewResponseError(status int, resp codersdk.Response) error { + return &responseError{ + status: status, + response: resp, + } +} + +func WriteResponseError(ctx context.Context, rw http.ResponseWriter, err error) { + if responseErr, ok := IsResponder(err); ok { + code, resp := responseErr.Response() + + httpapi.Write(ctx, rw, code, resp) + return + } + + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal server error", + Detail: err.Error(), + }) +} + +type responseError struct { + status int + response codersdk.Response +} + +var ( + _ error = (*responseError)(nil) + _ Responder = (*responseError)(nil) +) + +func (e *responseError) Error() string { + return fmt.Sprintf("%s: %s", e.response.Message, e.response.Detail) +} + +func (e *responseError) Status() int { + return e.status +} + +func (e *responseError) Response() (int, codersdk.Response) { + return e.status, e.response +} + +var ErrResourceNotFound = NewResponseError(http.StatusNotFound, httpapi.ResourceNotFoundResponse) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index bcda1dd022..3b8e35c003 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -388,7 +388,13 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req AvatarURL: member.AvatarURL, } - createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r) + w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, r) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusCreated, w) } // Create a new workspace for the currently authenticated user. @@ -442,8 +448,9 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { // This can be optimized. It exists as it is now for code simplicity. // The most common case is to create a workspace for 'Me'. Which does // not enter this code branch. - template, ok := requestTemplate(ctx, rw, req, api.Database) - if !ok { + template, err := requestTemplate(ctx, req, api.Database) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) return } @@ -476,7 +483,14 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { }) defer commitAudit() - createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r) + + w, err := createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, r) + if err != nil { + httperror.WriteResponseError(ctx, rw, err) + return + } + + httpapi.Write(ctx, rw, http.StatusCreated, w) } type workspaceOwner struct { @@ -492,12 +506,11 @@ func createWorkspace( api *API, owner workspaceOwner, req codersdk.CreateWorkspaceRequest, - rw http.ResponseWriter, r *http.Request, -) { - template, ok := requestTemplate(ctx, rw, req, api.Database) - if !ok { - return +) (codersdk.Workspace, error) { + template, err := requestTemplate(ctx, req, api.Database) + if err != nil { + return codersdk.Workspace{}, err } // This is a premature auth check to avoid doing unnecessary work if the user @@ -506,14 +519,12 @@ func createWorkspace( rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) { // If this check fails, return a proper unauthorized error to the user to indicate // what is going on. - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ Message: "Unauthorized to create workspace.", Detail: "You are unable to create a workspace in this organization. " + "It is possible to have access to the template, but not be able to create a workspace. " + "Please contact an administrator about your permissions if you feel this is an error.", - Validations: nil, }) - return } // Update audit log's organization @@ -523,49 +534,42 @@ func createWorkspace( // would be wasted. if !api.Authorize(r, policy.ActionCreate, rbac.ResourceWorkspace.InOrg(template.OrganizationID).WithOwner(owner.ID.String())) { - httpapi.ResourceNotFound(rw) - return + return codersdk.Workspace{}, httperror.ErrResourceNotFound } // The user also needs permission to use the template. At this point they have // read perms, but not necessarily "use". This is also checked in `db.InsertWorkspace`. // Doing this up front can save some work below if the user doesn't have permission. if !api.Authorize(r, policy.ActionUse, template) { - httpapi.Write(ctx, rw, http.StatusForbidden, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusForbidden, codersdk.Response{ Message: fmt.Sprintf("Unauthorized access to use the template %q.", template.Name), Detail: "Although you are able to view the template, you are unable to create a workspace using it. " + "Please contact an administrator about your permissions if you feel this is an error.", - Validations: nil, }) - return } templateAccessControl := (*(api.AccessControlStore.Load())).GetTemplateAccessControl(template) if templateAccessControl.IsDeprecated() { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Template %q has been deprecated, and cannot be used to create a new workspace.", template.Name), // Pass the deprecated message to the user. - Detail: templateAccessControl.Deprecated, - Validations: nil, + Detail: templateAccessControl.Deprecated, }) - return } dbAutostartSchedule, err := validWorkspaceSchedule(req.AutostartSchedule) if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: "Invalid Autostart Schedule.", Validations: []codersdk.ValidationError{{Field: "schedule", Detail: err.Error()}}, }) - return } templateSchedule, err := (*api.TemplateScheduleStore.Load()).Get(ctx, api.Database, template.ID) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching template schedule.", Detail: err.Error(), }) - return } nextStartAt := sql.NullTime{} @@ -578,11 +582,10 @@ func createWorkspace( dbTTL, err := validWorkspaceTTLMillis(req.TTLMillis, templateSchedule.DefaultTTL) if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: "Invalid Workspace Time to Shutdown.", Validations: []codersdk.ValidationError{{Field: "ttl_ms", Detail: err.Error()}}, }) - return } // back-compatibility: default to "never" if not included. @@ -590,11 +593,10 @@ func createWorkspace( if req.AutomaticUpdates != "" { dbAU, err = validWorkspaceAutomaticUpdates(req.AutomaticUpdates) if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: "Invalid Workspace Automatic Updates setting.", Validations: []codersdk.ValidationError{{Field: "automatic_updates", Detail: err.Error()}}, }) - return } } @@ -607,20 +609,18 @@ func createWorkspace( }) if err == nil { // If the workspace already exists, don't allow creation. - httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusConflict, codersdk.Response{ Message: fmt.Sprintf("Workspace %q already exists.", req.Name), Validations: []codersdk.ValidationError{{ Field: "name", Detail: "This value is already in use and should be unique.", }}, }) - return } else if !errors.Is(err, sql.ErrNoRows) { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: fmt.Sprintf("Internal error fetching workspace by name %q.", req.Name), Detail: err.Error(), }) - return } var ( @@ -759,8 +759,7 @@ func createWorkspace( return err }, nil) if err != nil { - httperror.WriteWorkspaceBuildError(ctx, rw, err) - return + return codersdk.Workspace{}, err } err = provisionerjobs.PostJob(api.Pubsub, *provisionerJob) @@ -809,11 +808,10 @@ func createWorkspace( provisionerDaemons, ) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error converting workspace build.", Detail: err.Error(), }) - return } w, err := convertWorkspace( @@ -825,40 +823,38 @@ func createWorkspace( codersdk.WorkspaceAppStatus{}, ) if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return codersdk.Workspace{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error converting workspace.", Detail: err.Error(), }) - return } - httpapi.Write(ctx, rw, http.StatusCreated, w) + + return w, nil } -func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, bool) { +func requestTemplate(ctx context.Context, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, error) { // If we were given a `TemplateVersionID`, we need to determine the `TemplateID` from it. templateID := req.TemplateID if templateID == uuid.Nil { templateVersion, err := db.GetTemplateVersionByID(ctx, req.TemplateVersionID) if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Template version %q doesn't exist.", req.TemplateVersionID), Validations: []codersdk.ValidationError{{ Field: "template_version_id", Detail: "template not found", }}, }) - return database.Template{}, false } if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching template version.", Detail: err.Error(), }) - return database.Template{}, false } if templateVersion.Archived { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Archived template versions cannot be used to make a workspace.", Validations: []codersdk.ValidationError{ { @@ -867,7 +863,6 @@ func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.C }, }, }) - return database.Template{}, false } templateID = templateVersion.TemplateID.UUID @@ -875,29 +870,26 @@ func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.C template, err := db.GetTemplateByID(ctx, templateID) if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Template %q doesn't exist.", templateID), Validations: []codersdk.ValidationError{{ Field: "template_id", Detail: "template not found", }}, }) - return database.Template{}, false } if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching template.", Detail: err.Error(), }) - return database.Template{}, false } if template.Deleted { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + return database.Template{}, httperror.NewResponseError(http.StatusNotFound, codersdk.Response{ Message: fmt.Sprintf("Template %q has been deleted!", template.Name), }) - return database.Template{}, false } - return template, true + return template, nil } func claimPrebuild(