diff --git a/cli/templateedit.go b/cli/templateedit.go index df3c582203d13..e48b509a42484 100644 --- a/cli/templateedit.go +++ b/cli/templateedit.go @@ -59,8 +59,8 @@ func templateEdit() *cobra.Command { cmd.Flags().StringVarP(&name, "name", "", "", "Edit the template name") cmd.Flags().StringVarP(&description, "description", "", "", "Edit the template description") cmd.Flags().StringVarP(&icon, "icon", "", "", "Edit the template icon path") - cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 0, "Edit the template maximum time before shutdown") - cmd.Flags().DurationVarP(&minAutostartInterval, "min-autostart-interval", "", 0, "Edit the template minimum autostart interval") + cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 0, "Edit the template maximum time before shutdown - workspaces created from this template cannot stay running longer than this.") + cmd.Flags().DurationVarP(&minAutostartInterval, "min-autostart-interval", "", 0, "Edit the template minimum autostart interval - workspaces created from this template must wait at least this long between autostarts.") cliui.AllowSkipPrompt(cmd) return cmd diff --git a/coderd/authorize.go b/coderd/authorize.go index b68e1b68544a3..ab81c4ee81f6a 100644 --- a/coderd/authorize.go +++ b/coderd/authorize.go @@ -49,6 +49,7 @@ func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objec // This function will log appropriately, but the caller must return an // error to the api client. // Eg: +// // if !h.Authorize(...) { // httpapi.Forbidden(rw) // return diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 7ba138c820312..b92d066749569 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1564,10 +1564,6 @@ func (q *fakeQuerier) InsertTemplate(_ context.Context, arg database.InsertTempl q.mutex.Lock() defer q.mutex.Unlock() - // default values - if arg.MaxTtl == 0 { - arg.MaxTtl = int64(168 * time.Hour) - } if arg.MinAutostartInterval == 0 { arg.MinAutostartInterval = int64(time.Hour) } diff --git a/coderd/database/migrations/000038_template_max_ttl_cap_7_days.down.sql b/coderd/database/migrations/000038_template_max_ttl_cap_7_days.down.sql new file mode 100644 index 0000000000000..15e22fad5f6ae --- /dev/null +++ b/coderd/database/migrations/000038_template_max_ttl_cap_7_days.down.sql @@ -0,0 +1 @@ +-- this is a no-op diff --git a/coderd/database/migrations/000038_template_max_ttl_cap_7_days.up.sql b/coderd/database/migrations/000038_template_max_ttl_cap_7_days.up.sql new file mode 100644 index 0000000000000..083f9501aafaf --- /dev/null +++ b/coderd/database/migrations/000038_template_max_ttl_cap_7_days.up.sql @@ -0,0 +1,2 @@ +-- Set a cap of 7 days on template max_ttl +UPDATE templates SET max_ttl = 604800000000000 WHERE max_ttl > 604800000000000; diff --git a/coderd/templates.go b/coderd/templates.go index 217bab3f36330..b33d5b406220c 100644 --- a/coderd/templates.go +++ b/coderd/templates.go @@ -173,9 +173,28 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque } maxTTL := maxTTLDefault - if !ptr.NilOrZero(createTemplate.MaxTTLMillis) { + if createTemplate.MaxTTLMillis != nil { maxTTL = time.Duration(*createTemplate.MaxTTLMillis) * time.Millisecond } + if maxTTL < 0 { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid create template request.", + Validations: []codersdk.ValidationError{ + {Field: "max_ttl_ms", Detail: "Must be a positive integer."}, + }, + }) + return + } + + if maxTTL > maxTTLDefault { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid create template request.", + Validations: []codersdk.ValidationError{ + {Field: "max_ttl_ms", Detail: "Cannot be greater than " + maxTTLDefault.String()}, + }, + }) + return + } minAutostartInterval := minAutostartIntervalDefault if !ptr.NilOrZero(createTemplate.MinAutostartIntervalMillis) { @@ -384,6 +403,15 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { if req.MinAutostartIntervalMillis < 0 { validErrs = append(validErrs, codersdk.ValidationError{Field: "min_autostart_interval_ms", Detail: "Must be a positive integer."}) } + if req.MaxTTLMillis > maxTTLDefault.Milliseconds() { + httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid create template request.", + Validations: []codersdk.ValidationError{ + {Field: "max_ttl_ms", Detail: "Cannot be greater than " + maxTTLDefault.String()}, + }, + }) + return + } if len(validErrs) > 0 { httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{ @@ -433,9 +461,6 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) { if icon == "" { icon = template.Icon } - if maxTTL == 0 { - maxTTL = time.Duration(template.MaxTtl) - } if minAutostartInterval == 0 { minAutostartInterval = time.Duration(template.MinAutostartInterval) } diff --git a/coderd/templates_test.go b/coderd/templates_test.go index 20fec8e64e144..ba01d3f3d5739 100644 --- a/coderd/templates_test.go +++ b/coderd/templates_test.go @@ -108,6 +108,64 @@ func TestPostTemplateByOrganization(t *testing.T) { require.Equal(t, http.StatusConflict, apiErr.StatusCode()) }) + t.Run("MaxTTLTooLow", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{ + Name: "testing", + VersionID: version.ID, + MaxTTLMillis: ptr.Ref(int64(-1)), + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, err.Error(), "max_ttl_ms: Must be a positive integer") + }) + + t.Run("MaxTTLTooHigh", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{ + Name: "testing", + VersionID: version.ID, + MaxTTLMillis: ptr.Ref(365 * 24 * time.Hour.Milliseconds()), + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, err.Error(), "max_ttl_ms: Cannot be greater than") + }) + + t.Run("NoMaxTTL", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + got, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{ + Name: "testing", + VersionID: version.ID, + MaxTTLMillis: ptr.Ref(int64(0)), + }) + require.NoError(t, err) + require.Zero(t, got.MaxTTLMillis) + }) + t.Run("Unauthorized", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) @@ -271,6 +329,87 @@ func TestPatchTemplateMeta(t *testing.T) { assert.Equal(t, req.MinAutostartIntervalMillis, updated.MinAutostartIntervalMillis) }) + t.Run("NoMaxTTL", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds()) + }) + req := codersdk.UpdateTemplateMeta{ + MaxTTLMillis: 0, + } + + // We're too fast! Sleep so we can be sure that updatedAt is greater + time.Sleep(time.Millisecond * 5) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.UpdateTemplateMeta(ctx, template.ID, req) + require.NoError(t, err) + + // Extra paranoid: did it _really_ happen? + updated, err := client.Template(ctx, template.ID) + require.NoError(t, err) + assert.Greater(t, updated.UpdatedAt, template.UpdatedAt) + assert.Equal(t, req.MaxTTLMillis, updated.MaxTTLMillis) + }) + + t.Run("MaxTTLTooLow", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds()) + }) + req := codersdk.UpdateTemplateMeta{ + MaxTTLMillis: -1, + } + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.UpdateTemplateMeta(ctx, template.ID, req) + require.ErrorContains(t, err, "max_ttl_ms: Must be a positive integer") + + // Ensure no update occurred + updated, err := client.Template(ctx, template.ID) + require.NoError(t, err) + assert.Equal(t, updated.UpdatedAt, template.UpdatedAt) + assert.Equal(t, updated.MaxTTLMillis, template.MaxTTLMillis) + }) + + t.Run("MaxTTLTooHigh", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds()) + }) + req := codersdk.UpdateTemplateMeta{ + MaxTTLMillis: 365 * 24 * time.Hour.Milliseconds(), + } + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + _, err := client.UpdateTemplateMeta(ctx, template.ID, req) + require.ErrorContains(t, err, "max_ttl_ms: Cannot be greater than") + + // Ensure no update occurred + updated, err := client.Template(ctx, template.ID) + require.NoError(t, err) + assert.Equal(t, updated.UpdatedAt, template.UpdatedAt) + assert.Equal(t, updated.MaxTTLMillis, template.MaxTTLMillis) + }) + t.Run("NotModified", func(t *testing.T) { t.Parallel() diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 3b0e7a486a482..1ed8c9b53ae74 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -32,8 +32,6 @@ import ( "github.com/coder/coder/codersdk" ) -const workspaceDefaultTTL = 12 * time.Hour - var ( ttlMin = time.Minute //nolint:revive // min here means 'minimum' not 'minutes' ttlMax = 7 * 24 * time.Hour @@ -312,11 +310,6 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req return } - if !dbTTL.Valid { - // Default to min(12 hours, template maximum). Just defaulting to template maximum can be surprising. - dbTTL = sql.NullInt64{Valid: true, Int64: min(template.MaxTtl, int64(workspaceDefaultTTL))} - } - workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{ OwnerID: apiKey.UserID, Name: createWorkspace.Name, @@ -923,7 +916,8 @@ func validWorkspaceTTLMillis(millis *int64, max time.Duration) (sql.NullInt64, e return sql.NullInt64{}, errTTLMax } - if truncated > max { + // template level + if max > 0 && truncated > max { return sql.NullInt64{}, xerrors.Errorf("time until shutdown must be below template maximum %s", max.String()) } @@ -1050,10 +1044,3 @@ func splitQueryParameterByDelimiter(query string, delimiter rune, maintainQuotes return parts } - -func min(x, y int64) int64 { - if x < y { - return x - } - return y -} diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index db47eb796bcd5..215325a697a54 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -191,6 +191,26 @@ func TestPostWorkspacesByOrganization(t *testing.T) { _ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID) }) + t.Run("TemplateNoTTL", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) + user := coderdtest.CreateFirstUser(t, client) + version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil) + template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { + ctr.MaxTTLMillis = ptr.Ref(int64(0)) + }) + // Given: the template has no max TTL set + require.Zero(t, template.MaxTTLMillis) + coderdtest.AwaitTemplateVersionJob(t, client, version.ID) + + // When: we create a workspace with autostop not enabled + workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) { + cwr.TTLMillis = ptr.Ref(int64(0)) + }) + // Then: No TTL should be set by the template + require.Nil(t, workspace.TTLMillis) + }) + t.Run("TemplateCustomTTL", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) @@ -1051,6 +1071,17 @@ func TestWorkspaceUpdateTTL(t *testing.T) { expectedError: "ttl_ms: time until shutdown must be below template maximum 8h0m0s", modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref((8 * time.Hour).Milliseconds()) }, }, + { + name: "no template maximum ttl", + ttlMillis: ptr.Ref((7 * 24 * time.Hour).Milliseconds()), + modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref(int64(0)) }, + }, + { + name: "above maximum ttl even with no template max", + ttlMillis: ptr.Ref((365 * 24 * time.Hour).Milliseconds()), + expectedError: "ttl_ms: time until shutdown must be less than 7 days", + modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref(int64(0)) }, + }, } for _, testCase := range testCases {