Skip to content

Commit eba753b

Browse files
johnstcnjohnstcn
authored
fix: template: enforce bounds of template max_ttl (coder#3662)
This PR makes the following changes: - enforces lower and upper limits on template `max_ttl_ms` - adds a migration to enforce 7-day cap on `max_ttl` - allows setting template `max_ttl` to 0 - updates template edit CLI help to be clearer
1 parent 343d118 commit eba753b

9 files changed

Lines changed: 207 additions & 25 deletions

‎cli/templateedit.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,8 @@ func templateEdit() *cobra.Command {
5959
cmd.Flags().StringVarP(&name, "name", "", "", "Edit the template name")
6060
cmd.Flags().StringVarP(&description, "description", "", "", "Edit the template description")
6161
cmd.Flags().StringVarP(&icon, "icon", "", "", "Edit the template icon path")
62-
cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 0, "Edit the template maximum time before shutdown")
63-
cmd.Flags().DurationVarP(&minAutostartInterval, "min-autostart-interval", "", 0, "Edit the template minimum autostart interval")
62+
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.")
63+
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.")
6464
cliui.AllowSkipPrompt(cmd)
6565

6666
return cmd

‎coderd/authorize.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ func (api *API) Authorize(r *http.Request, action rbac.Action, object rbac.Objec
4949
// This function will log appropriately, but the caller must return an
5050
// error to the api client.
5151
// Eg:
52+
//
5253
// if !h.Authorize(...) {
5354
// httpapi.Forbidden(rw)
5455
// return

‎coderd/database/databasefake/databasefake.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1564,10 +1564,6 @@ func (q *fakeQuerier) InsertTemplate(_ context.Context, arg database.InsertTempl
15641564
q.mutex.Lock()
15651565
defer q.mutex.Unlock()
15661566

1567-
// default values
1568-
if arg.MaxTtl == 0 {
1569-
arg.MaxTtl = int64(168 * time.Hour)
1570-
}
15711567
if arg.MinAutostartInterval == 0 {
15721568
arg.MinAutostartInterval = int64(time.Hour)
15731569
}

‎coderd/database/migrations/000038_template_max_ttl_cap_7_days.down.sql‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
-- this is a no-op

‎coderd/database/migrations/000038_template_max_ttl_cap_7_days.up.sql‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
-- Set a cap of 7 days on template max_ttl
2+
UPDATE templates SET max_ttl = 604800000000000 WHERE max_ttl > 604800000000000;

‎coderd/templates.go‎

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -173,9 +173,28 @@ func (api *API) postTemplateByOrganization(rw http.ResponseWriter, r *http.Reque
173173
}
174174

175175
maxTTL := maxTTLDefault
176-
if !ptr.NilOrZero(createTemplate.MaxTTLMillis) {
176+
if createTemplate.MaxTTLMillis != nil {
177177
maxTTL = time.Duration(*createTemplate.MaxTTLMillis) * time.Millisecond
178178
}
179+
if maxTTL < 0 {
180+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
181+
Message: "Invalid create template request.",
182+
Validations: []codersdk.ValidationError{
183+
{Field: "max_ttl_ms", Detail: "Must be a positive integer."},
184+
},
185+
})
186+
return
187+
}
188+
189+
if maxTTL > maxTTLDefault {
190+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
191+
Message: "Invalid create template request.",
192+
Validations: []codersdk.ValidationError{
193+
{Field: "max_ttl_ms", Detail: "Cannot be greater than " + maxTTLDefault.String()},
194+
},
195+
})
196+
return
197+
}
179198

180199
minAutostartInterval := minAutostartIntervalDefault
181200
if !ptr.NilOrZero(createTemplate.MinAutostartIntervalMillis) {
@@ -384,6 +403,15 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
384403
if req.MinAutostartIntervalMillis < 0 {
385404
validErrs = append(validErrs, codersdk.ValidationError{Field: "min_autostart_interval_ms", Detail: "Must be a positive integer."})
386405
}
406+
if req.MaxTTLMillis > maxTTLDefault.Milliseconds() {
407+
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
408+
Message: "Invalid create template request.",
409+
Validations: []codersdk.ValidationError{
410+
{Field: "max_ttl_ms", Detail: "Cannot be greater than " + maxTTLDefault.String()},
411+
},
412+
})
413+
return
414+
}
387415

388416
if len(validErrs) > 0 {
389417
httpapi.Write(rw, http.StatusBadRequest, codersdk.Response{
@@ -433,9 +461,6 @@ func (api *API) patchTemplateMeta(rw http.ResponseWriter, r *http.Request) {
433461
if icon == "" {
434462
icon = template.Icon
435463
}
436-
if maxTTL == 0 {
437-
maxTTL = time.Duration(template.MaxTtl)
438-
}
439464
if minAutostartInterval == 0 {
440465
minAutostartInterval = time.Duration(template.MinAutostartInterval)
441466
}

‎coderd/templates_test.go‎

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,64 @@ func TestPostTemplateByOrganization(t *testing.T) {
108108
require.Equal(t, http.StatusConflict, apiErr.StatusCode())
109109
})
110110

111+
t.Run("MaxTTLTooLow", func(t *testing.T) {
112+
t.Parallel()
113+
client := coderdtest.New(t, nil)
114+
user := coderdtest.CreateFirstUser(t, client)
115+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
116+
117+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
118+
defer cancel()
119+
120+
_, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{
121+
Name: "testing",
122+
VersionID: version.ID,
123+
MaxTTLMillis: ptr.Ref(int64(-1)),
124+
})
125+
var apiErr *codersdk.Error
126+
require.ErrorAs(t, err, &apiErr)
127+
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
128+
require.Contains(t, err.Error(), "max_ttl_ms: Must be a positive integer")
129+
})
130+
131+
t.Run("MaxTTLTooHigh", func(t *testing.T) {
132+
t.Parallel()
133+
client := coderdtest.New(t, nil)
134+
user := coderdtest.CreateFirstUser(t, client)
135+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
136+
137+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
138+
defer cancel()
139+
140+
_, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{
141+
Name: "testing",
142+
VersionID: version.ID,
143+
MaxTTLMillis: ptr.Ref(365 * 24 * time.Hour.Milliseconds()),
144+
})
145+
var apiErr *codersdk.Error
146+
require.ErrorAs(t, err, &apiErr)
147+
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
148+
require.Contains(t, err.Error(), "max_ttl_ms: Cannot be greater than")
149+
})
150+
151+
t.Run("NoMaxTTL", func(t *testing.T) {
152+
t.Parallel()
153+
client := coderdtest.New(t, nil)
154+
user := coderdtest.CreateFirstUser(t, client)
155+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
156+
157+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
158+
defer cancel()
159+
160+
got, err := client.CreateTemplate(ctx, user.OrganizationID, codersdk.CreateTemplateRequest{
161+
Name: "testing",
162+
VersionID: version.ID,
163+
MaxTTLMillis: ptr.Ref(int64(0)),
164+
})
165+
require.NoError(t, err)
166+
require.Zero(t, got.MaxTTLMillis)
167+
})
168+
111169
t.Run("Unauthorized", func(t *testing.T) {
112170
t.Parallel()
113171
client := coderdtest.New(t, nil)
@@ -271,6 +329,87 @@ func TestPatchTemplateMeta(t *testing.T) {
271329
assert.Equal(t, req.MinAutostartIntervalMillis, updated.MinAutostartIntervalMillis)
272330
})
273331

332+
t.Run("NoMaxTTL", func(t *testing.T) {
333+
t.Parallel()
334+
335+
client := coderdtest.New(t, nil)
336+
user := coderdtest.CreateFirstUser(t, client)
337+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
338+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
339+
ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds())
340+
})
341+
req := codersdk.UpdateTemplateMeta{
342+
MaxTTLMillis: 0,
343+
}
344+
345+
// We're too fast! Sleep so we can be sure that updatedAt is greater
346+
time.Sleep(time.Millisecond * 5)
347+
348+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
349+
defer cancel()
350+
351+
_, err := client.UpdateTemplateMeta(ctx, template.ID, req)
352+
require.NoError(t, err)
353+
354+
// Extra paranoid: did it _really_ happen?
355+
updated, err := client.Template(ctx, template.ID)
356+
require.NoError(t, err)
357+
assert.Greater(t, updated.UpdatedAt, template.UpdatedAt)
358+
assert.Equal(t, req.MaxTTLMillis, updated.MaxTTLMillis)
359+
})
360+
361+
t.Run("MaxTTLTooLow", func(t *testing.T) {
362+
t.Parallel()
363+
364+
client := coderdtest.New(t, nil)
365+
user := coderdtest.CreateFirstUser(t, client)
366+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
367+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
368+
ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds())
369+
})
370+
req := codersdk.UpdateTemplateMeta{
371+
MaxTTLMillis: -1,
372+
}
373+
374+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
375+
defer cancel()
376+
377+
_, err := client.UpdateTemplateMeta(ctx, template.ID, req)
378+
require.ErrorContains(t, err, "max_ttl_ms: Must be a positive integer")
379+
380+
// Ensure no update occurred
381+
updated, err := client.Template(ctx, template.ID)
382+
require.NoError(t, err)
383+
assert.Equal(t, updated.UpdatedAt, template.UpdatedAt)
384+
assert.Equal(t, updated.MaxTTLMillis, template.MaxTTLMillis)
385+
})
386+
387+
t.Run("MaxTTLTooHigh", func(t *testing.T) {
388+
t.Parallel()
389+
390+
client := coderdtest.New(t, nil)
391+
user := coderdtest.CreateFirstUser(t, client)
392+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
393+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
394+
ctr.MaxTTLMillis = ptr.Ref(24 * time.Hour.Milliseconds())
395+
})
396+
req := codersdk.UpdateTemplateMeta{
397+
MaxTTLMillis: 365 * 24 * time.Hour.Milliseconds(),
398+
}
399+
400+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
401+
defer cancel()
402+
403+
_, err := client.UpdateTemplateMeta(ctx, template.ID, req)
404+
require.ErrorContains(t, err, "max_ttl_ms: Cannot be greater than")
405+
406+
// Ensure no update occurred
407+
updated, err := client.Template(ctx, template.ID)
408+
require.NoError(t, err)
409+
assert.Equal(t, updated.UpdatedAt, template.UpdatedAt)
410+
assert.Equal(t, updated.MaxTTLMillis, template.MaxTTLMillis)
411+
})
412+
274413
t.Run("NotModified", func(t *testing.T) {
275414
t.Parallel()
276415

‎coderd/workspaces.go‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,6 @@ import (
3232
"github.com/coder/coder/codersdk"
3333
)
3434

35-
const workspaceDefaultTTL = 12 * time.Hour
36-
3735
var (
3836
ttlMin = time.Minute //nolint:revive // min here means 'minimum' not 'minutes'
3937
ttlMax = 7 * 24 * time.Hour
@@ -312,11 +310,6 @@ func (api *API) postWorkspacesByOrganization(rw http.ResponseWriter, r *http.Req
312310
return
313311
}
314312

315-
if !dbTTL.Valid {
316-
// Default to min(12 hours, template maximum). Just defaulting to template maximum can be surprising.
317-
dbTTL = sql.NullInt64{Valid: true, Int64: min(template.MaxTtl, int64(workspaceDefaultTTL))}
318-
}
319-
320313
workspace, err := api.Database.GetWorkspaceByOwnerIDAndName(r.Context(), database.GetWorkspaceByOwnerIDAndNameParams{
321314
OwnerID: apiKey.UserID,
322315
Name: createWorkspace.Name,
@@ -923,7 +916,8 @@ func validWorkspaceTTLMillis(millis *int64, max time.Duration) (sql.NullInt64, e
923916
return sql.NullInt64{}, errTTLMax
924917
}
925918

926-
if truncated > max {
919+
// template level
920+
if max > 0 && truncated > max {
927921
return sql.NullInt64{}, xerrors.Errorf("time until shutdown must be below template maximum %s", max.String())
928922
}
929923

@@ -1050,10 +1044,3 @@ func splitQueryParameterByDelimiter(query string, delimiter rune, maintainQuotes
10501044

10511045
return parts
10521046
}
1053-
1054-
func min(x, y int64) int64 {
1055-
if x < y {
1056-
return x
1057-
}
1058-
return y
1059-
}

‎coderd/workspaces_test.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,26 @@ func TestPostWorkspacesByOrganization(t *testing.T) {
191191
_ = coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID)
192192
})
193193

194+
t.Run("TemplateNoTTL", func(t *testing.T) {
195+
t.Parallel()
196+
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
197+
user := coderdtest.CreateFirstUser(t, client)
198+
version := coderdtest.CreateTemplateVersion(t, client, user.OrganizationID, nil)
199+
template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) {
200+
ctr.MaxTTLMillis = ptr.Ref(int64(0))
201+
})
202+
// Given: the template has no max TTL set
203+
require.Zero(t, template.MaxTTLMillis)
204+
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
205+
206+
// When: we create a workspace with autostop not enabled
207+
workspace := coderdtest.CreateWorkspace(t, client, user.OrganizationID, template.ID, func(cwr *codersdk.CreateWorkspaceRequest) {
208+
cwr.TTLMillis = ptr.Ref(int64(0))
209+
})
210+
// Then: No TTL should be set by the template
211+
require.Nil(t, workspace.TTLMillis)
212+
})
213+
194214
t.Run("TemplateCustomTTL", func(t *testing.T) {
195215
t.Parallel()
196216
client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true})
@@ -1051,6 +1071,17 @@ func TestWorkspaceUpdateTTL(t *testing.T) {
10511071
expectedError: "ttl_ms: time until shutdown must be below template maximum 8h0m0s",
10521072
modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref((8 * time.Hour).Milliseconds()) },
10531073
},
1074+
{
1075+
name: "no template maximum ttl",
1076+
ttlMillis: ptr.Ref((7 * 24 * time.Hour).Milliseconds()),
1077+
modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref(int64(0)) },
1078+
},
1079+
{
1080+
name: "above maximum ttl even with no template max",
1081+
ttlMillis: ptr.Ref((365 * 24 * time.Hour).Milliseconds()),
1082+
expectedError: "ttl_ms: time until shutdown must be less than 7 days",
1083+
modifyTemplate: func(ctr *codersdk.CreateTemplateRequest) { ctr.MaxTTLMillis = ptr.Ref(int64(0)) },
1084+
},
10541085
}
10551086

10561087
for _, testCase := range testCases {

0 commit comments

Comments
 (0)