From c6df467c7cce8900fc3c405ca74322828bd9bb08 Mon Sep 17 00:00:00 2001 From: johnstcn Date: Tue, 14 Jun 2022 16:15:56 +0000 Subject: [PATCH 1/3] fix: allow setting workspace deadline as early as now plus 30 minutes --- cli/bump.go | 20 ++++++++++++-------- cli/bump_test.go | 4 ++-- coderd/workspaces.go | 15 +++------------ coderd/workspaces_test.go | 37 +++++++++++++++++++++++-------------- 4 files changed, 40 insertions(+), 36 deletions(-) diff --git a/cli/bump.go b/cli/bump.go index e49f922495837..d949559adaeb2 100644 --- a/cli/bump.go +++ b/cli/bump.go @@ -5,6 +5,8 @@ import ( "strings" "time" + "github.com/coder/coder/coderd/util/tz" + "github.com/spf13/cobra" "golang.org/x/xerrors" @@ -12,15 +14,15 @@ import ( ) const ( - bumpDescriptionLong = `To extend the autostop deadline for a workspace.` + bumpDescriptionLong = `Make your workspace stop at a certain point in the future.` ) func bump() *cobra.Command { bumpCmd := &cobra.Command{ Args: cobra.RangeArgs(1, 2), Annotations: workspaceCommand, - Use: "bump ", - Short: "Extend the autostop deadline for a workspace.", + Use: "bump ", + Short: "Make your workspace stop at a certain point in the future.", Long: bumpDescriptionLong, Example: "coder bump my-workspace 90m", RunE: func(cmd *cobra.Command, args []string) error { @@ -39,17 +41,20 @@ func bump() *cobra.Command { return xerrors.Errorf("get workspace: %w", err) } - newDeadline := time.Now().Add(bumpDuration) + loc, err := tz.TimezoneIANA() + if err != nil { + loc = time.UTC // best guess + } - if newDeadline.Before(workspace.LatestBuild.Deadline) { + if bumpDuration < 29*time.Minute { _, _ = fmt.Fprintf( cmd.OutOrStdout(), - "The proposed deadline is %s before the current deadline.\n", - workspace.LatestBuild.Deadline.Sub(newDeadline).Round(time.Minute), + "Please specify a duration of at least 30 minutes.\n", ) return nil } + newDeadline := time.Now().In(loc).Add(bumpDuration) if err := client.PutExtendWorkspace(cmd.Context(), workspace.ID, codersdk.PutExtendWorkspaceRequest{ Deadline: newDeadline, }); err != nil { @@ -62,7 +67,6 @@ func bump() *cobra.Command { newDeadline.Format(timeFormat), newDeadline.Format(dateFormat), ) - return nil }, } diff --git a/cli/bump_test.go b/cli/bump_test.go index 635bb0b90956e..da8fc70151394 100644 --- a/cli/bump_test.go +++ b/cli/bump_test.go @@ -124,8 +124,8 @@ func TestBump(t *testing.T) { workspace, err = client.Workspace(ctx, workspace.ID) require.NoError(t, err) - // TODO(cian): need to stop and start the workspace as we do not update the deadline yet - // see: https://github.com/coder/coder/issues/1783 + // NOTE(cian): need to stop and start the workspace as we do not update the deadline + // see: https://github.com/coder/coder/issues/2224 coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStart, database.WorkspaceTransitionStop) coderdtest.MustTransitionWorkspace(t, client, workspace.ID, database.WorkspaceTransitionStop, database.WorkspaceTransitionStart) diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 0ee26342ab2fe..e8105bc548dd3 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -883,18 +883,9 @@ func validWorkspaceDeadline(old, new time.Time) error { return xerrors.New("nothing to do: no existing deadline set") } - now := time.Now() - if new.Before(now) { - return xerrors.New("new deadline must be in the future") - } - - delta := new.Sub(old) - if delta < time.Minute { - return xerrors.New("minimum extension is one minute") - } - - if delta > 24*time.Hour { - return xerrors.New("maximum extension is 24 hours") + soon := time.Now().Add(29 * time.Minute) + if new.Before(soon) { + return xerrors.New("new deadline must be at least 30 minutes in the future") } return nil diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index cdab39422ea74..70c100a42285b 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -974,22 +974,23 @@ func TestWorkspaceUpdateTTL(t *testing.T) { func TestWorkspaceExtend(t *testing.T) { t.Parallel() var ( + ttl = 8 * time.Hour + newDeadline = time.Now().Add(ttl + time.Hour).UTC() ctx = context.Background() client = coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) user = coderdtest.CreateFirstUser(t, client) version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) project = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID) - extend = 90 * time.Minute - _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) - oldDeadline = time.Now().Add(time.Duration(*workspace.TTLMillis) * time.Millisecond).UTC() - newDeadline = time.Now().Add(time.Duration(*workspace.TTLMillis)*time.Millisecond + extend).UTC() + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.TTLMillis = ptr.Ref(ttl.Milliseconds()) + }) + _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) ) workspace, err := client.Workspace(ctx, workspace.ID) require.NoError(t, err, "fetch provisioned workspace") - require.InDelta(t, oldDeadline.Unix(), workspace.LatestBuild.Deadline.Unix(), 60) + oldDeadline := workspace.LatestBuild.Deadline // Updating the deadline should succeed req := codersdk.PutExtendWorkspaceRequest{ @@ -1001,7 +1002,7 @@ func TestWorkspaceExtend(t *testing.T) { // Ensure deadline set correctly updated, err := client.Workspace(ctx, workspace.ID) require.NoError(t, err, "failed to fetch updated workspace") - require.InDelta(t, newDeadline.Unix(), updated.LatestBuild.Deadline.Unix(), 60) + require.WithinDuration(t, newDeadline, updated.LatestBuild.Deadline, time.Minute) // Zero time should fail err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ @@ -1009,22 +1010,30 @@ func TestWorkspaceExtend(t *testing.T) { }) require.ErrorContains(t, err, "deadline: Validation failed for tag \"required\" with value: \"0001-01-01 00:00:00 +0000 UTC\"", "setting an empty deadline on a workspace should fail") - // Updating with an earlier time should also fail + // Updating with a deadline 29 minutes in the future should fail + deadlineTooSoon := time.Now().Add(29 * time.Minute) + err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ + Deadline: deadlineTooSoon, + }) + require.ErrorContains(t, err, "new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") + + // Updating with a deadline 30 minutes in the future should succeed + deadlineJustSoonEnough := time.Now().Add(30 * time.Minute) err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ - Deadline: oldDeadline, + Deadline: deadlineJustSoonEnough, }) - require.ErrorContains(t, err, "deadline: minimum extension is one minute", "setting an earlier deadline should fail") + require.NoError(t, err, "setting a deadline at least 30 minutes in the future should succeed") - // Updating with a time far in the future should also fail + // Updating with a deadline an hour before the previous deadline should succeed err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ - Deadline: oldDeadline.AddDate(1, 0, 0), + Deadline: oldDeadline.Add(-time.Hour), }) - require.ErrorContains(t, err, "deadline: maximum extension is 24 hours", "setting an earlier deadline should fail") + require.NoError(t, err, "setting an earlier deadline should not fail") // Ensure deadline still set correctly updated, err = client.Workspace(ctx, workspace.ID) require.NoError(t, err, "failed to fetch updated workspace") - require.InDelta(t, newDeadline.Unix(), updated.LatestBuild.Deadline.Unix(), 60) + require.WithinDuration(t, oldDeadline.Add(-time.Hour), updated.LatestBuild.Deadline, time.Minute) } func TestWorkspaceWatcher(t *testing.T) { From 308b348507ac45523662dacc2deb8b55210fb629 Mon Sep 17 00:00:00 2001 From: johnstcn Date: Tue, 14 Jun 2022 19:58:28 +0000 Subject: [PATCH 2/3] copy changes --- cli/bump.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/bump.go b/cli/bump.go index d949559adaeb2..a6156e1361e5d 100644 --- a/cli/bump.go +++ b/cli/bump.go @@ -14,7 +14,7 @@ import ( ) const ( - bumpDescriptionLong = `Make your workspace stop at a certain point in the future.` + bumpDescriptionLong = `Specify a duration from now when you would like your workspace to shut down.` ) func bump() *cobra.Command { @@ -22,7 +22,7 @@ func bump() *cobra.Command { Args: cobra.RangeArgs(1, 2), Annotations: workspaceCommand, Use: "bump ", - Short: "Make your workspace stop at a certain point in the future.", + Short: "Specify a duration from now when you would like your workspace to shut down.", Long: bumpDescriptionLong, Example: "coder bump my-workspace 90m", RunE: func(cmd *cobra.Command, args []string) error { From 1708c212179e32d224555b66ccf1506d0138ab3e Mon Sep 17 00:00:00 2001 From: johnstcn Date: Tue, 14 Jun 2022 21:12:55 +0000 Subject: [PATCH 3/3] address PR comments, actually enforce template level limits --- cli/bump.go | 15 ++++++++---- coderd/workspaces.go | 48 ++++++++++++++++++++++++++++++++------- coderd/workspaces_test.go | 11 +++++++-- 3 files changed, 59 insertions(+), 15 deletions(-) diff --git a/cli/bump.go b/cli/bump.go index a6156e1361e5d..8a98f361e31ce 100644 --- a/cli/bump.go +++ b/cli/bump.go @@ -5,16 +5,21 @@ import ( "strings" "time" - "github.com/coder/coder/coderd/util/tz" - "github.com/spf13/cobra" "golang.org/x/xerrors" + "github.com/coder/coder/coderd/util/tz" "github.com/coder/coder/codersdk" ) const ( - bumpDescriptionLong = `Specify a duration from now when you would like your workspace to shut down.` + bumpDescriptionShort = `Shut your workspace down after a given duration has passed.` + bumpDescriptionLong = `Modify the time at which your workspace will shut down automatically. +* Provide a duration from now (for example, 1h30m). +* The minimum duration is 30 minutes. +* If the workspace template restricts the maximum runtime of a workspace, this will be enforced here. +* If the workspace does not already have a shutdown scheduled, this does nothing. +` ) func bump() *cobra.Command { @@ -22,7 +27,7 @@ func bump() *cobra.Command { Args: cobra.RangeArgs(1, 2), Annotations: workspaceCommand, Use: "bump ", - Short: "Specify a duration from now when you would like your workspace to shut down.", + Short: bumpDescriptionShort, Long: bumpDescriptionLong, Example: "coder bump my-workspace 90m", RunE: func(cmd *cobra.Command, args []string) error { @@ -43,7 +48,7 @@ func bump() *cobra.Command { loc, err := tz.TimezoneIANA() if err != nil { - loc = time.UTC // best guess + loc = time.UTC // best effort } if bumpDuration < 29*time.Minute { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index e8105bc548dd3..4ec20d66120da 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -575,21 +575,47 @@ func (api *API) putExtendWorkspace(rw http.ResponseWriter, r *http.Request) { resp := httpapi.Response{} err := api.Database.InTx(func(s database.Store) error { + template, err := s.GetTemplateByID(r.Context(), workspace.TemplateID) + if err != nil { + code = http.StatusInternalServerError + resp.Message = "Error fetching workspace template!" + return xerrors.Errorf("get workspace template: %w", err) + } + build, err := s.GetLatestWorkspaceBuildByWorkspaceID(r.Context(), workspace.ID) if err != nil { code = http.StatusInternalServerError - resp.Message = "Workspace not found." + resp.Message = "Error fetching workspace build." return xerrors.Errorf("get latest workspace build: %w", err) } + job, err := s.GetProvisionerJobByID(r.Context(), build.JobID) + if err != nil { + code = http.StatusInternalServerError + resp.Message = "Error fetching workspace provisioner job." + return xerrors.Errorf("get provisioner job: %w", err) + } + if build.Transition != database.WorkspaceTransitionStart { code = http.StatusConflict resp.Message = "Workspace must be started, current status: " + string(build.Transition) return xerrors.Errorf("workspace must be started, current status: %s", build.Transition) } + if !job.CompletedAt.Valid { + code = http.StatusConflict + resp.Message = "Workspace is still building!" + return xerrors.Errorf("workspace is still building") + } + + if build.Deadline.IsZero() { + code = http.StatusConflict + resp.Message = "Workspace shutdown is manual." + return xerrors.Errorf("workspace shutdown is manual") + } + newDeadline := req.Deadline.UTC() - if err := validWorkspaceDeadline(build.Deadline, newDeadline); err != nil { + if err := validWorkspaceDeadline(job.CompletedAt.Time, newDeadline, time.Duration(template.MaxTtl)); err != nil { code = http.StatusBadRequest resp.Message = "Bad extend workspace request." resp.Validations = append(resp.Validations, httpapi.Error{Field: "deadline", Detail: err.Error()}) @@ -878,16 +904,22 @@ func validWorkspaceTTLMillis(millis *int64, max time.Duration) (sql.NullInt64, e }, nil } -func validWorkspaceDeadline(old, new time.Time) error { - if old.IsZero() { - return xerrors.New("nothing to do: no existing deadline set") - } - +func validWorkspaceDeadline(startedAt, newDeadline time.Time, max time.Duration) error { soon := time.Now().Add(29 * time.Minute) - if new.Before(soon) { + if newDeadline.Before(soon) { return xerrors.New("new deadline must be at least 30 minutes in the future") } + // No idea how this could happen. + if newDeadline.Before(startedAt) { + return xerrors.Errorf("new deadline must be before workspace start time") + } + + delta := newDeadline.Sub(startedAt) + if delta > max { + return xerrors.New("new deadline is greater than template allows") + } + return nil } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 70c100a42285b..7eac1d50324b3 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -981,8 +981,8 @@ func TestWorkspaceExtend(t *testing.T) { user = coderdtest.CreateFirstUser(t, client) version = coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) _ = coderdtest.AwaitTemplateVersionJob(t, client, version.ID) - project = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) - workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + template = coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID) + workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { cwr.TTLMillis = ptr.Ref(ttl.Milliseconds()) }) _ = coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID) @@ -1017,6 +1017,13 @@ func TestWorkspaceExtend(t *testing.T) { }) require.ErrorContains(t, err, "new deadline must be at least 30 minutes in the future", "setting a deadline less than 30 minutes in the future should fail") + // And with a deadline greater than the template max_ttl should also fail + deadlineExceedsMaxTTL := time.Now().Add(time.Duration(template.MaxTTLMillis) * time.Millisecond).Add(time.Minute) + err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{ + Deadline: deadlineExceedsMaxTTL, + }) + require.ErrorContains(t, err, "new deadline is greater than template allows", "setting a deadline greater than that allowed by the template should fail") + // Updating with a deadline 30 minutes in the future should succeed deadlineJustSoonEnough := time.Now().Add(30 * time.Minute) err = client.PutExtendWorkspace(ctx, workspace.ID, codersdk.PutExtendWorkspaceRequest{