fix: autostart/schedule: move to 5-valued cron string with weekly val…

…idation
coder · johnstcn · Apr 18, 2022 · Apr 16, 2022 · Apr 16, 2022 · Apr 16, 2022
commit 967ead8bb5fe039fe6142f4357089a306d29e84e
diff --git a/coderd/autostart/schedule/schedule.go b/coderd/autostart/schedule/schedule.go
@@ -3,23 +3,27 @@
 package schedule
 
 import (
+	"strings"
 	"time"
 
 	"github.com/robfig/cron/v3"
 	"golang.org/x/xerrors"
 )
 
 // For the purposes of this library, we only need minute, hour, and
-// day-of-week.
-const parserFormatWeekly = cron.Minute | cron.Hour | cron.Dow
+// day-of-week. However to ensure interoperability we will use the standard
+// five-valued cron format. Descriptors are not supported.
+const parserFormat = cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow
 
-var defaultParser = cron.NewParser(parserFormatWeekly)
+var defaultParser = cron.NewParser(parserFormat)
 
 // Weekly parses a Schedule from spec scoped to a recurring weekly event.
 // Spec consists of the following space-delimited fields, in the following order:
 // - timezone e.g. CRON_TZ=US/Central (optional)
 // - minutes of hour e.g. 30 (required)
 // - hour of day e.g. 9 (required)
+// - day of month (must be *)
+// - month (must be *)
 // - day of week e.g. 1 (required)
 //
 // Example Usage:
@@ -31,6 +35,10 @@ var defaultParser = cron.NewParser(parserFormatWeekly)
 //  fmt.Println(sched.Next(time.Now()).Format(time.RFC3339))
 //  // Output: 2022-04-04T14:30:00Z
 func Weekly(spec string) (*Schedule, error) {
+	if err := validateWeeklySpec(spec); err != nil {
+		return nil, xerrors.Errorf("validate weekly schedule: %w", err)
+	}
+
 	specSched, err := defaultParser.Parse(spec)
 	if err != nil {
 		return nil, xerrors.Errorf("parse schedule: %w", err)
@@ -65,3 +73,19 @@ func (s Schedule) String() string {
 func (s Schedule) Next(t time.Time) time.Time {
 	return s.sched.Next(t)
 }
+
+// validateWeeklySpec ensures that the day-of-month and month options of
+// spec are both set to *
+func validateWeeklySpec(spec string) error {
+	parts := strings.Fields(spec)
+	if len(parts) < 5 {
+		return xerrors.Errorf("expected schedule to consist of 5 fields with an optional CRON_TZ=<timezone> prefix")
+	}
+	if len(parts) == 6 {
+		parts = parts[1:]
+	}
+	if parts[2] != "*" || parts[3] != "*" {
+		return xerrors.Errorf("expected month and dom to be *")
+	}
+	return nil
+}
diff --git a/coderd/autostart/schedule/schedule_test.go b/coderd/autostart/schedule/schedule_test.go
@@ -20,14 +20,14 @@ func Test_Weekly(t *testing.T) {
 	}{
 		{
 			name:          "with timezone",
-			spec:          "CRON_TZ=US/Central 30 9 1-5",
+			spec:          "CRON_TZ=US/Central 30 9 * * 1-5",
 			at:            time.Date(2022, 4, 1, 14, 29, 0, 0, time.UTC),
 			expectedNext:  time.Date(2022, 4, 1, 14, 30, 0, 0, time.UTC),
 			expectedError: "",
 		},
 		{
 			name:          "without timezone",
-			spec:          "30 9 1-5",
+			spec:          "30 9 * * 1-5",
 			at:            time.Date(2022, 4, 1, 9, 29, 0, 0, time.Local),
 			expectedNext:  time.Date(2022, 4, 1, 9, 30, 0, 0, time.Local),
 			expectedError: "",
@@ -37,15 +37,43 @@ func Test_Weekly(t *testing.T) {
 			spec:          "asdfasdfasdfsd",
 			at:            time.Time{},
 			expectedNext:  time.Time{},
-			expectedError: "parse schedule: expected exactly 3 fields, found 1: [asdfasdfasdfsd]",
+			expectedError: "validate weekly schedule: expected schedule to consist of 5 fields with an optional CRON_TZ=<timezone> prefix",
 		},
 		{
 			name:          "invalid location",
-			spec:          "CRON_TZ=Fictional/Country 30 9 1-5",
+			spec:          "CRON_TZ=Fictional/Country 30 9 * * 1-5",
 			at:            time.Time{},
 			expectedNext:  time.Time{},
 			expectedError: "parse schedule: provided bad location Fictional/Country: unknown time zone Fictional/Country",
 		},
+		{
+			name:          "invalid schedule with 3 fields",
+			spec:          "CRON_TZ=Fictional/Country 30 9 1-5",
+			at:            time.Time{},
+			expectedNext:  time.Time{},
+			expectedError: "validate weekly schedule: expected schedule to consist of 5 fields with an optional CRON_TZ=<timezone> prefix",
+		},
+		{
+			name:          "invalid schedule with 3 fields and no timezone",
+			spec:          "30 9 1-5",
+			at:            time.Time{},
+			expectedNext:  time.Time{},
+			expectedError: "validate weekly schedule: expected schedule to consist of 5 fields with an optional CRON_TZ=<timezone> prefix",
+		},
+		{
+			name:          "valid schedule with 5 fields but month and dom not set to *",
+			spec:          "30 9 1 1 1-5",
+			at:            time.Time{},
+			expectedNext:  time.Time{},
+			expectedError: "validate weekly schedule: expected month and dom to be *",
+		},
+		{
+			name:          "valid schedule with 5 fields and timezone but month and dom not set to *",
+			spec:          "CRON_TZ=Europe/Dublin 30 9 1 1 1-5",
+			at:            time.Time{},
+			expectedNext:  time.Time{},
+			expectedError: "validate weekly schedule: expected month and dom to be *",
+		},
 	}
 
 	for _, testCase := range testCases {