- Notifications
You must be signed in to change notification settings - Fork928
feat: enforce template-level constraints for TTL and autostart#2018
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
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
20 commits Select commitHold shift + click to select a range
1713431 RED: add failing unit tests
johnstcnf11f942 RED: add migrations and make gen
johnstcnc5c9a7a ORANGE: fix audit/diff tests
johnstcn74f4dee fixup! RED: add migrations and make gen
johnstcn7da27a0 fixup! RED: add migrations and make gen
johnstcn5ad048b feat: schedule: add schedule.Min function to determine minimum interval
johnstcn65b980e ORANGE: coderd: fix a whole bunch of tests
johnstcnaa298a6 RED: fast-fail some slow-failing tests
johnstcna5ec9bf GREEN: fix even more tests
johnstcna6255d6 GREEN: fix final unit test
johnstcn245795e cli: templatecreate: add CLI flags --max-ttl --min-autostart-interval
johnstcn709db5b fixup! cli: templatecreate: add CLI flags --max-ttl --min-autostart-i…
johnstcnc4a0927 make gen
johnstcn244e479 update test entities
johnstcn60ef479 Merge remote-tracking branch 'origin/main' into 1433-template-constra…
johnstcnc9aab8b fixup! Merge remote-tracking branch 'origin/main' into 1433-template-…
johnstcnf997af2 coderd: default newly-minted workspaces to template max_ttl
johnstcnd7f697b address PR comment
johnstcnebe4c15 fix broken unit test
johnstcn08ef2eb fix some more tests
johnstcnFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
23 changes: 23 additions & 0 deletionscli/autostart_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -6,6 +6,7 @@ import ( | ||
| "fmt" | ||
| "os" | ||
| "testing" | ||
| "time" | ||
| "github.com/stretchr/testify/require" | ||
| @@ -158,4 +159,26 @@ func TestAutostart(t *testing.T) { | ||
| require.NoError(t, err, "fetch updated workspace") | ||
| require.Equal(t, expectedSchedule, *updated.AutostartSchedule, "expected default autostart schedule") | ||
| }) | ||
| t.Run("BelowTemplateConstraint", func(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Question(gut-check): Should there be a similar case forAboveTemplateConstraint ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I don't see any reason to enforce an upper bound on the interval between successive autostarts. | ||
| t.Parallel() | ||
| var ( | ||
| 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, func(ctr *codersdk.CreateTemplateRequest) { | ||
| ctr.MinAutostartIntervalMillis = ptr.Ref(time.Hour.Milliseconds()) | ||
| }) | ||
| workspace = coderdtest.CreateWorkspace(t, client, user.OrganizationID, project.ID) | ||
| cmdArgs = []string{"autostart", "enable", workspace.Name, "--minute", "*", "--hour", "*"} | ||
| ) | ||
| cmd, root := clitest.New(t, cmdArgs...) | ||
| clitest.SetupConfig(t, client, root) | ||
| err := cmd.Execute() | ||
| require.ErrorContains(t, err, "schedule: Minimum autostart interval 1m0s below template minimum 1h0m0s") | ||
| }) | ||
| } | ||
12 changes: 12 additions & 0 deletionscli/bump_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
55 changes: 39 additions & 16 deletionscli/create.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -61,20 +61,6 @@ func create() *cobra.Command { | ||
| } | ||
| } | ||
johnstcn marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||
| _, err = client.WorkspaceByOwnerAndName(cmd.Context(), codersdk.Me, workspaceName) | ||
| if err == nil { | ||
| return xerrors.Errorf("A workspace already exists named %q!", workspaceName) | ||
| @@ -129,6 +115,23 @@ func create() *cobra.Command { | ||
| } | ||
| } | ||
| schedSpec, err := validSchedule( | ||
| autostartMinute, | ||
| autostartHour, | ||
| autostartDow, | ||
| tzName, | ||
| time.Duration(template.MinAutostartIntervalMillis)*time.Millisecond, | ||
| ) | ||
| if err != nil { | ||
| return xerrors.Errorf("Invalid autostart schedule: %w", err) | ||
| } | ||
| if ttl < time.Minute { | ||
| return xerrors.Errorf("TTL must be at least 1 minute") | ||
| } | ||
| if ttlMax := time.Duration(template.MaxTTLMillis) * time.Millisecond; ttl > ttlMax { | ||
| return xerrors.Errorf("TTL must be below template maximum %s", ttlMax) | ||
| } | ||
Comment on lines +118 to +134 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Review: We're duplicating some validations here because I want the errors to show upbefore the whole workspace plan happens. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Interesting to do it in the cli. Can we somehow put the validation on a datastructure and reuse it for the backend + frontend? Like reuse the same code? Not a huge deal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. It gets validated on the BE as well but if we don't at least validate in the And yes, this is definitely a good candidate to be DRYed up a bit. | ||
| templateVersion, err := client.TemplateVersion(cmd.Context(), template.ActiveVersionID) | ||
| if err != nil { | ||
| return err | ||
| @@ -226,7 +229,7 @@ func create() *cobra.Command { | ||
| workspace, err := client.CreateWorkspace(cmd.Context(), organization.ID, codersdk.CreateWorkspaceRequest{ | ||
| TemplateID: template.ID, | ||
| Name: workspaceName, | ||
| AutostartSchedule: schedSpec, | ||
| TTLMillis: ptr.Ref(ttl.Milliseconds()), | ||
| ParameterValues: parameters, | ||
| }) | ||
| @@ -262,7 +265,27 @@ func create() *cobra.Command { | ||
| cliflag.StringVarP(cmd.Flags(), &autostartMinute, "autostart-minute", "", "CODER_WORKSPACE_AUTOSTART_MINUTE", "0", "Specify the minute(s) at which the workspace should autostart (e.g. 0).") | ||
| cliflag.StringVarP(cmd.Flags(), &autostartHour, "autostart-hour", "", "CODER_WORKSPACE_AUTOSTART_HOUR", "9", "Specify the hour(s) at which the workspace should autostart (e.g. 9).") | ||
| cliflag.StringVarP(cmd.Flags(), &autostartDow, "autostart-day-of-week", "", "CODER_WORKSPACE_AUTOSTART_DOW", "MON-FRI", "Specify the days(s) on which the workspace should autostart (e.g. MON,TUE,WED,THU,FRI)") | ||
| cliflag.StringVarP(cmd.Flags(), &tzName, "tz", "", "TZ", "UTC", "Specify your timezone location for workspace autostart (e.g. US/Central).") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Review: defaulting TZ to UTC here explicitly. Will hopefully have some fancier auto-detection in a separate issue. | ||
| cliflag.DurationVarP(cmd.Flags(), &ttl, "ttl", "", "CODER_WORKSPACE_TTL", 8*time.Hour, "Specify a time-to-live (TTL) for the workspace (e.g. 8h).") | ||
| return cmd | ||
| } | ||
| func validSchedule(minute, hour, dow, tzName string, min time.Duration) (*string, error) { | ||
| _, err := time.LoadLocation(tzName) | ||
| if err != nil { | ||
| return nil, xerrors.Errorf("Invalid workspace autostart timezone: %w", err) | ||
| } | ||
| schedSpec := fmt.Sprintf("CRON_TZ=%s %s %s * * %s", tzName, minute, hour, dow) | ||
| sched, err := schedule.Weekly(schedSpec) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if schedMin := sched.Min(); schedMin < min { | ||
| return nil, xerrors.Errorf("minimum autostart interval %s is above template constraint %s", schedMin, min) | ||
| } | ||
| return &schedSpec, nil | ||
| } | ||
72 changes: 58 additions & 14 deletionscli/create_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,6 +14,7 @@ import ( | ||
| "github.com/coder/coder/cli/clitest" | ||
| "github.com/coder/coder/coderd/coderdtest" | ||
| "github.com/coder/coder/coderd/database" | ||
| "github.com/coder/coder/coderd/util/ptr" | ||
| "github.com/coder/coder/codersdk" | ||
| "github.com/coder/coder/provisioner/echo" | ||
| "github.com/coder/coder/provisionersdk/proto" | ||
| @@ -62,6 +63,57 @@ func TestCreate(t *testing.T) { | ||
| <-doneChan | ||
| }) | ||
| t.Run("AboveTemplateMaxTTL", 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) | ||
| coderdtest.AwaitTemplateVersionJob(t, client, version.ID) | ||
| template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { | ||
| ctr.MaxTTLMillis = ptr.Ref((12 * time.Hour).Milliseconds()) | ||
| }) | ||
| args := []string{ | ||
| "create", | ||
| "my-workspace", | ||
| "--template", template.Name, | ||
| "--ttl", "12h1m", | ||
| "-y", // don't bother with waiting | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Review: I'm slightly side-stepping here because I don't want to deal with confirms | ||
| } | ||
| cmd, root := clitest.New(t, args...) | ||
| clitest.SetupConfig(t, client, root) | ||
| pty := ptytest.New(t) | ||
| cmd.SetIn(pty.Input()) | ||
| cmd.SetOut(pty.Output()) | ||
| err := cmd.Execute() | ||
| assert.ErrorContains(t, err, "TTL must be below template maximum 12h0m0s") | ||
| }) | ||
| t.Run("BelowTemplateMinAutostartInterval", 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) | ||
| coderdtest.AwaitTemplateVersionJob(t, client, version.ID) | ||
| template := coderdtest.CreateTemplate(t, client, user.OrganizationID, version.ID, func(ctr *codersdk.CreateTemplateRequest) { | ||
| ctr.MinAutostartIntervalMillis = ptr.Ref(time.Hour.Milliseconds()) | ||
| }) | ||
| args := []string{ | ||
| "create", | ||
| "my-workspace", | ||
| "--template", template.Name, | ||
| "--autostart-minute", "*", // Every minute | ||
| "--autostart-hour", "*", // Every hour | ||
| "-y", // don't bother with waiting | ||
| } | ||
| cmd, root := clitest.New(t, args...) | ||
| clitest.SetupConfig(t, client, root) | ||
| pty := ptytest.New(t) | ||
| cmd.SetIn(pty.Input()) | ||
| cmd.SetOut(pty.Output()) | ||
| err := cmd.Execute() | ||
| assert.ErrorContains(t, err, "minimum autostart interval 1m0s is above template constraint 1h0m0s") | ||
| }) | ||
| t.Run("CreateErrInvalidTz", func(t *testing.T) { | ||
| t.Parallel() | ||
| client := coderdtest.New(t, &coderdtest.Options{IncludeProvisionerD: true}) | ||
| @@ -74,19 +126,15 @@ func TestCreate(t *testing.T) { | ||
| "my-workspace", | ||
| "--template", template.Name, | ||
| "--tz", "invalid", | ||
| "-y", | ||
| } | ||
| cmd, root := clitest.New(t, args...) | ||
| clitest.SetupConfig(t, client, root) | ||
| pty := ptytest.New(t) | ||
| cmd.SetIn(pty.Input()) | ||
| cmd.SetOut(pty.Output()) | ||
| err := cmd.Execute() | ||
| assert.ErrorContains(t, err, "Invalid autostart schedule: Invalid workspace autostart timezone: unknown time zone invalid") | ||
| }) | ||
| t.Run("CreateErrInvalidTTL", func(t *testing.T) { | ||
| @@ -101,19 +149,15 @@ func TestCreate(t *testing.T) { | ||
| "my-workspace", | ||
| "--template", template.Name, | ||
| "--ttl", "0s", | ||
| "-y", | ||
| } | ||
| cmd, root := clitest.New(t, args...) | ||
| clitest.SetupConfig(t, client, root) | ||
| pty := ptytest.New(t) | ||
| cmd.SetIn(pty.Input()) | ||
| cmd.SetOut(pty.Output()) | ||
| err := cmd.Execute() | ||
| assert.EqualError(t, err, "TTL must be at least 1 minute") | ||
Comment on lines -111 to +160 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Review: as above, removing parallelism from this test as it's not strictly necessary. | ||
| }) | ||
| t.Run("CreateFromListWithSkip", func(t *testing.T) { | ||
25 changes: 17 additions & 8 deletionscli/templatecreate.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -14,16 +14,19 @@ import ( | ||
| "github.com/coder/coder/cli/cliui" | ||
| "github.com/coder/coder/coderd/database" | ||
| "github.com/coder/coder/coderd/util/ptr" | ||
| "github.com/coder/coder/codersdk" | ||
| "github.com/coder/coder/provisionerd" | ||
| "github.com/coder/coder/provisionersdk" | ||
| ) | ||
| func templateCreate() *cobra.Command { | ||
| var ( | ||
| directory string | ||
| provisioner string | ||
| parameterFile string | ||
| maxTTL time.Duration | ||
| minAutostartInterval time.Duration | ||
| ) | ||
| cmd := &cobra.Command{ | ||
| Use: "create [name]", | ||
| @@ -92,11 +95,15 @@ func templateCreate() *cobra.Command { | ||
| return err | ||
| } | ||
| createReq := codersdk.CreateTemplateRequest{ | ||
| Name: templateName, | ||
| VersionID: job.ID, | ||
| ParameterValues: parameters, | ||
| MaxTTLMillis: ptr.Ref(maxTTL.Milliseconds()), | ||
| MinAutostartIntervalMillis: ptr.Ref(minAutostartInterval.Milliseconds()), | ||
| } | ||
| _, err = client.CreateTemplate(cmd.Context(), organization.ID, createReq) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| @@ -115,6 +122,8 @@ func templateCreate() *cobra.Command { | ||
| cmd.Flags().StringVarP(&directory, "directory", "d", currentDirectory, "Specify the directory to create from") | ||
| cmd.Flags().StringVarP(&provisioner, "test.provisioner", "", "terraform", "Customize the provisioner backend") | ||
| cmd.Flags().StringVarP(¶meterFile, "parameter-file", "", "", "Specify a file path with parameter values.") | ||
| cmd.Flags().DurationVarP(&maxTTL, "max-ttl", "", 168*time.Hour, "Specify a maximum TTL for worksapces created from this template.") | ||
| cmd.Flags().DurationVarP(&minAutostartInterval, "min-autostart-interval", "", time.Hour, "Specify a minimum autostart interval for workspaces created from this template.") | ||
Contributor
| ||
| // This is for testing! | ||
| err := cmd.Flags().MarkHidden("test.provisioner") | ||
| if err != nil { | ||
11 changes: 10 additions & 1 deletioncli/templatecreate_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -24,7 +24,16 @@ func TestTemplateCreate(t *testing.T) { | ||
| Parse: echo.ParseComplete, | ||
| Provision: echo.ProvisionComplete, | ||
| }) | ||
| args := []string{ | ||
| "templates", | ||
| "create", | ||
| "my-template", | ||
| "--directory", source, | ||
| "--test.provisioner", string(database.ProvisionerTypeEcho), | ||
| "--max-ttl", "24h", | ||
| "--min-autostart-interval", "2h", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. 👍🏻 | ||
| } | ||
| cmd, root := clitest.New(t, args...) | ||
| clitest.SetupConfig(t, client, root) | ||
| pty := ptytest.New(t) | ||
| cmd.SetIn(pty.Input()) | ||
33 changes: 33 additions & 0 deletionscli/ttl_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.