feat: add controls to template for determining startup days #10226

Emyrk · 2023-10-11T20:10:02Z

What this does

Allows the template admins to manage which days are valid for autostart. The autostart days are relative to the autostart cron string time zone.

So if admins want to stop autostart on Sat/Sun, they can disable the days in the template for auto start, and all autostart on those days will fail. Note that the autostart day is the day of the next Cron transition in the timezone selected by a developer. So the day is relative to the user, not the admin.

So if a developer really wants to be get around this, they can. Say they live in Sydney Australia and want auto start on Saturday at 8am, but saturday is blocked. They could set their auto start to 2pm in GMT-7 (San Francisco). So the Friday is the "local day" used, and it gets around the saturday. Just wanted to mention that strangeness, but this feature is a cost savings measure, and not intended as a kind of security feature.

Future Work

Implement UI

Emyrk · 2023-10-12T20:37:05Z

coderd/autobuild/lifecycle_executor.go

+	// The nextTransition is when the auto start should kick off. If it lands on a
+	// forbidden day, do not allow the auto start. We use the time location of the
+	// schedule to determine the weekday. So if "Saturday" is disallowed, the
+	// definition of "Saturday" depends on the location of the schedule.
+	zonedTransition := nextTransition.In(sched.Location())
+	allowed := templateSchedule.AutostartRequirement.DaysMap()[zonedTransition.Weekday()]
+	if !allowed {
+		return false
+	}


This is the affect on autostart

johnstcn

I don't see any major issues, but would like to see a couple more tests before approving.

I think dbgen is missing the additional field here as well?

Regarding the "workaround", this does nothing to stop a developer from manually starting a workspace on a specific day of the week, or having some external automation via the CLI starting a workspace for them.

johnstcn · 2023-10-13T08:18:22Z

coderd/database/queries/templates.sql

-	failure_ttl = $9,
-	time_til_dormant = $10,
-	time_til_dormant_autodelete = $11
+	autostart_block_days_of_week = $9,


Storing a blocklist instead of an allowlist seems strange to me.

Yea, however I really wanted the zero value to be the correct default value.

If I flip this, then we have to store 0b01111111 or 127, which feels very "magical" from a default POV.

That makes sense 👍

coderd/schedule/template.go

johnstcn · 2023-10-13T08:20:06Z

coderd/database/modelmethods.go

+func (t Template) AutostartAllowedDays() uint8 {
+	// Just flip the binary 0s to 1s and vice versa.
+	// There is an extra day with the 8th bit that needs to be zeroed.
+	return ^uint8(t.AutostartBlockDaysOfWeek) & 0b01111111


It's even more strange that we store a blocklist and then expose this function to flip it 🤔

From the UX POV, I think the deny list is more difficult to understand, so I flip it when returning it from the BE.

Consumers of the API will still have to submit a denylist, right?

No, because we gatekeep all schedule changes by the AGPL and Enterprise interfaces, the inversion happens right before we do the db query.

coder/enterprise/coderd/schedule/template.go

Line 150 in 25c3b1b

AutostartBlockDaysOfWeek: int16(^opts.AutostartRequirement.DaysOfWeek & 0b01111111),

Consumers of the database.Template struct do see the denylist, and should use this model method to get the allowlist:

coder/coderd/database/modelmethods.go

Line 130 in 25c3b1b

func (t Template) AutostartAllowedDays() uint8 {

There's no easy way to encapsulate this to hide the underlying implementation other than doing something overkill like making a custom uint8 type with a custom postgres scanner. Feels overkill though.

I read that line and then completely forgot about it 😆

Yeah a custom scanner would definitely be overkill. But I like having the inversion happening at the API level so nobody needs to worry about it.

And another comment is I cannot write that function in the schedule package because that imports the database package. So I just wrote the binary flip 🤷‍♂️. Should be the only spot it's needed though. Most of the binary logic comes from that schedule package

cli/templateedit_test.go

coderd/autobuild/lifecycle_executor_test.go

enterprise/coderd/templates_test.go

Emyrk · 2023-10-13T16:06:40Z

The DBGen for template is incomplete atm since some fields like this one only come from updating.

So I should do the logic like I do for provisioner job probably:

coder/coderd/database/dbgen/dbgen.go

Lines 342 to 393 in 25c3b1b

    
           job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{ 
        
           	ID:             jobID, 
        
           	CreatedAt:      takeFirst(orig.CreatedAt, dbtime.Now()), 
        
           	UpdatedAt:      takeFirst(orig.UpdatedAt, dbtime.Now()), 
        
           	OrganizationID: takeFirst(orig.OrganizationID, uuid.New()), 
        
           	InitiatorID:    takeFirst(orig.InitiatorID, uuid.New()), 
        
           	Provisioner:    takeFirst(orig.Provisioner, database.ProvisionerTypeEcho), 
        
           	StorageMethod:  takeFirst(orig.StorageMethod, database.ProvisionerStorageMethodFile), 
        
           	FileID:         takeFirst(orig.FileID, uuid.New()), 
        
           	Type:           takeFirst(orig.Type, database.ProvisionerJobTypeWorkspaceBuild), 
        
           	Input:          takeFirstSlice(orig.Input, []byte("{}")), 
        
           	Tags:           orig.Tags, 
        
           	TraceMetadata:  pqtype.NullRawMessage{}, 
        
           }) 
        
           require.NoError(t, err, "insert job") 
        
           if ps != nil { 
        
           	err = provisionerjobs.PostJob(ps, job) 
        
           	require.NoError(t, err, "post job to pubsub") 
        
           } 
        
           if !orig.StartedAt.Time.IsZero() { 
        
           	job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{ 
        
           		StartedAt: orig.StartedAt, 
        
           		Types:     []database.ProvisionerType{database.ProvisionerTypeEcho}, 
        
           		Tags:      must(json.Marshal(orig.Tags)), 
        
           		WorkerID:  uuid.NullUUID{}, 
        
           	}) 
        
           	require.NoError(t, err) 
        
           	// There is no easy way to make sure we acquire the correct job. 
        
           	require.Equal(t, jobID, job.ID, "acquired incorrect job") 
        
           } 
        
           if !orig.CompletedAt.Time.IsZero() || orig.Error.String != "" { 
        
           	err := db.UpdateProvisionerJobWithCompleteByID(genCtx, database.UpdateProvisionerJobWithCompleteByIDParams{ 
        
           		ID:          jobID, 
        
           		UpdatedAt:   job.UpdatedAt, 
        
           		CompletedAt: orig.CompletedAt, 
        
           		Error:       orig.Error, 
        
           		ErrorCode:   orig.ErrorCode, 
        
           	}) 
        
           	require.NoError(t, err) 
        
           } 
        
           if !orig.CanceledAt.Time.IsZero() { 
        
           	err := db.UpdateProvisionerJobWithCancelByID(genCtx, database.UpdateProvisionerJobWithCancelByIDParams{ 
        
           		ID:          jobID, 
        
           		CanceledAt:  orig.CanceledAt, 
        
           		CompletedAt: orig.CompletedAt, 
        
           	}) 
        
           	require.NoError(t, err) 
        
           } 
        
           job, err = db.GetProvisionerJobByID(genCtx, jobID) 
        
           require.NoError(t, err)

But all the extra fields are currently broken in that regard. We should make an issue to fix a few of these

johnstcn · 2023-10-13T16:46:45Z

The DBGen for template is incomplete atm since some fields like this one only come from updating.

So I should do the logic like I do for provisioner job probably:

coder/coderd/database/dbgen/dbgen.go

Lines 342 to 393 in 25c3b1b

job, err := db.InsertProvisionerJob(genCtx, database.InsertProvisionerJobParams{

ID: jobID,

CreatedAt: takeFirst(orig.CreatedAt, dbtime.Now()),

UpdatedAt: takeFirst(orig.UpdatedAt, dbtime.Now()),

OrganizationID: takeFirst(orig.OrganizationID, uuid.New()),

InitiatorID: takeFirst(orig.InitiatorID, uuid.New()),

Provisioner: takeFirst(orig.Provisioner, database.ProvisionerTypeEcho),

StorageMethod: takeFirst(orig.StorageMethod, database.ProvisionerStorageMethodFile),

FileID: takeFirst(orig.FileID, uuid.New()),

Type: takeFirst(orig.Type, database.ProvisionerJobTypeWorkspaceBuild),

Input: takeFirstSlice(orig.Input, []byte("{}")),

Tags: orig.Tags,

TraceMetadata: pqtype.NullRawMessage{},

})

require.NoError(t, err, "insert job")

if ps != nil {

err = provisionerjobs.PostJob(ps, job)

require.NoError(t, err, "post job to pubsub")

}

if !orig.StartedAt.Time.IsZero() {

job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{

StartedAt: orig.StartedAt,

Types: []database.ProvisionerType{database.ProvisionerTypeEcho},

Tags: must(json.Marshal(orig.Tags)),

WorkerID: uuid.NullUUID{},

})

require.NoError(t, err)

// There is no easy way to make sure we acquire the correct job.

require.Equal(t, jobID, job.ID, "acquired incorrect job")

}

if !orig.CompletedAt.Time.IsZero() || orig.Error.String != "" {

err := db.UpdateProvisionerJobWithCompleteByID(genCtx, database.UpdateProvisionerJobWithCompleteByIDParams{

ID: jobID,

UpdatedAt: job.UpdatedAt,

CompletedAt: orig.CompletedAt,

Error: orig.Error,

ErrorCode: orig.ErrorCode,

})

require.NoError(t, err)

}

if !orig.CanceledAt.Time.IsZero() {

err := db.UpdateProvisionerJobWithCancelByID(genCtx, database.UpdateProvisionerJobWithCancelByIDParams{

ID: jobID,

CanceledAt: orig.CanceledAt,

CompletedAt: orig.CompletedAt,

})

require.NoError(t, err)

}

job, err = db.GetProvisionerJobByID(genCtx, jobID)

require.NoError(t, err)

But all the extra fields are currently broken in that regard. We should make an issue to fix a few of these

Fair enough. That can be done in a follow-up.

johnstcn

LGTM!

Emyrk added 2 commits October 11, 2023 14:58

feat: template controls which days can autostart

0d09d4c

Equal times should be valid for autostart

1975a45

github-actions bot assigned Emyrk Oct 11, 2023

Emyrk added 2 commits October 11, 2023 15:40

typo on 'Requirement'

2511a18

add audit action track to new field

9af5475

Emyrk changed the title ~~feat: template controls which days can autostart~~ feat: add controls to template for determining startup days Oct 11, 2023

Emyrk added 3 commits October 11, 2023 16:03

fix js test entities

3bac180

make gen

6cafda3

Add unit test for autostart enabled days

7812a9d

Emyrk marked this pull request as ready for review October 12, 2023 19:35

Emyrk added 4 commits October 12, 2023 14:36

Linting + test

18c7093

Add auto start to template edit

352058b

Add unit tests

4fdc57c

Make gen

78257b8

Emyrk commented Oct 12, 2023

View reviewed changes

Emyrk requested a review from johnstcn October 12, 2023 20:37

johnstcn reviewed Oct 13, 2023

View reviewed changes

Emyrk added 2 commits October 13, 2023 10:52

Add unit test to test blocking autostart with DaysOfWeek

db25e84

Add assertions to existing tests

25c3b1b

Add a test for invalid input

51b8031

johnstcn approved these changes Oct 13, 2023

View reviewed changes

Emyrk merged commit 39c0539 into main Oct 13, 2023

Emyrk deleted the stevenmasley/autostart_schedule branch October 13, 2023 16:57

github-actions bot locked and limited conversation to collaborators Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add controls to template for determining startup days #10226

feat: add controls to template for determining startup days #10226

Emyrk commented Oct 11, 2023 •

edited

Loading

Emyrk Oct 12, 2023

johnstcn left a comment

johnstcn Oct 13, 2023

Emyrk Oct 13, 2023

johnstcn Oct 13, 2023

johnstcn Oct 13, 2023

Emyrk Oct 13, 2023

johnstcn Oct 13, 2023

Emyrk Oct 13, 2023

johnstcn Oct 13, 2023

Emyrk Oct 13, 2023

Emyrk commented Oct 13, 2023

johnstcn commented Oct 13, 2023

johnstcn left a comment

feat: add controls to template for determining startup days #10226

feat: add controls to template for determining startup days #10226

Conversation

Emyrk commented Oct 11, 2023 • edited Loading

What this does

Future Work

Choose a reason for hiding this comment

johnstcn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented Oct 13, 2023

johnstcn commented Oct 13, 2023

johnstcn left a comment

Choose a reason for hiding this comment

Emyrk commented Oct 11, 2023 •

edited

Loading