Merge #74881

74881: jobs,backup,vendor: switch from gorhill/cronexpr to robfig/cron r=dt a=dt gorhill/cronexpr is abandoned, and we recently found a known panic that was crashing nodes that upstream considered known behavior when an invalid expression was provided. While robfig/cron is a full scheduled job execution framework, we can use its parser separately as well, which is what this change does. When presented with the same malformed expression as before, CREATE SCHEDULE now returns an error. Release note (bug fix): certain malformed backup schedule expressions no longer cause the node to crash Co-authored-by: David Taylor <[email protected]>
cockroachdb · Jan 16, 2022 · 4b41789 · 4b41789
2 parents 5ad21e3 + 3ac757a
commit 4b41789
Show file tree

Hide file tree

Showing 14 changed files with 53 additions and 43 deletions.
diff --git a/DEPS.bzl b/DEPS.bzl
@@ -3403,16 +3403,6 @@ def go_deps():
             "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/gordonklaus/ineffassign/com_github_gordonklaus_ineffassign-v0.0.0-20200309095847-7953dde2c7bf.zip",
         ],
     )
-    go_repository(
-        name = "com_github_gorhill_cronexpr",
-        build_file_proto_mode = "disable_global",
-        importpath = "github.com/gorhill/cronexpr",
-        sha256 = "742d8957d3f9fe773150fb3164868a755b2af5b705b38c72c45ca5386715c617",
-        strip_prefix = "github.com/gorhill/[email protected]",
-        urls = [
-            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/gorhill/cronexpr/com_github_gorhill_cronexpr-v0.0.0-20180427100037-88b0669f7d75.zip",
-        ],
-    )
     go_repository(
         name = "com_github_gorilla_context",
         build_file_proto_mode = "disable_global",
@@ -6444,6 +6434,16 @@ def go_deps():
             "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/retailnext/hllpp/com_github_retailnext_hllpp-v1.0.1-0.20180308014038-101a6d2f8b52.zip",
         ],
     )
+    go_repository(
+        name = "com_github_robfig_cron_v3",
+        build_file_proto_mode = "disable_global",
+        importpath = "github.com/robfig/cron/v3",
+        sha256 = "ebe6454642220832a451b8cc50eae5f9150fd8d36b90b242a5de27676be86c70",
+        strip_prefix = "github.com/robfig/cron/[email protected]",
+        urls = [
+            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/robfig/cron/v3/com_github_robfig_cron_v3-v3.0.1.zip",
+        ],
+    )
     go_repository(
         name = "com_github_rogpeppe_fastuuid",
         build_file_proto_mode = "disable_global",

diff --git a/go.mod b/go.mod
@@ -78,7 +78,6 @@ require (
 	github.com/google/go-github v17.0.0+incompatible
 	github.com/google/pprof v0.0.0-20210827144239-02619b876842
 	github.com/google/skylark v0.0.0-20181101142754-a5f7082aabed
-	github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75
 	github.com/gorilla/mux v1.8.0
 	github.com/goware/modvendor v0.5.0
 	github.com/grpc-ecosystem/grpc-gateway v1.16.0
@@ -121,6 +120,7 @@ require (
 	github.com/prometheus/prometheus v1.8.2-0.20210914090109-37468d88dce8
 	github.com/pseudomuto/protoc-gen-doc v1.3.2
 	github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475
+	github.com/robfig/cron/v3 v3.0.1
 	github.com/sasha-s/go-deadlock v0.3.1
 	github.com/shirou/gopsutil/v3 v3.21.12
 	github.com/slack-go/slack v0.9.5

diff --git a/go.sum b/go.sum
@@ -1065,8 +1065,6 @@ github.com/gopherjs/gopherjs v0.0.0-20181017120253-0766667cb4d1/go.mod h1:wJfORR
 github.com/gopherjs/gopherjs v0.0.0-20181103185306-d547d1d9531e h1:JKmoR8x90Iww1ks85zJ1lfDGgIiMDuIptTOhJq+zKyg=
 github.com/gopherjs/gopherjs v0.0.0-20181103185306-d547d1d9531e/go.mod h1:wJfORRmW1u3UXTncJ5qlYoELFm8eSnnEO6hX4iZ3EWY=
 github.com/gordonklaus/ineffassign v0.0.0-20200309095847-7953dde2c7bf/go.mod h1:cuNKsD1zp2v6XfE/orVX2QE1LC+i254ceGcVeDT3pTU=
-github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75 h1:f0n1xnMSmBLzVfsMMvriDyA75NB/oBgILX2GcHXIQzY=
-github.com/gorhill/cronexpr v0.0.0-20180427100037-88b0669f7d75/go.mod h1:g2644b03hfBX9Ov0ZBDgXXens4rxSxmqFBbhvKv2yVA=
 github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg=
 github.com/gorilla/handlers v0.0.0-20150720190736-60c7bfde3e33/go.mod h1:Qkdc/uu4tH4g6mTK6auzZ766c4CA0Ng8+o/OAirnOIQ=
 github.com/gorilla/handlers v1.4.2/go.mod h1:Qkdc/uu4tH4g6mTK6auzZ766c4CA0Ng8+o/OAirnOIQ=
@@ -1782,6 +1780,8 @@ github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475 h1:N/ElC8H3+5X
 github.com/rcrowley/go-metrics v0.0.0-20201227073835-cf1acfcdf475/go.mod h1:bCqnVzQkZxMG4s8nGwiZ5l3QUCyqpo9Y+/ZMZ9VjZe4=
 github.com/remyoudompheng/bigfft v0.0.0-20170806203942-52369c62f446/go.mod h1:uYEyJGbgTkfkS4+E/PavXkNJcbFIpEtjt2B0KDQ5+9M=
 github.com/retailnext/hllpp v1.0.1-0.20180308014038-101a6d2f8b52/go.mod h1:RDpi1RftBQPUCDRw6SmxeaREsAaRKnOclghuzp/WRzc=
+github.com/robfig/cron/v3 v3.0.1 h1:WdRxkvbJztn8LMz/QEvLN5sBU+xKpSqwwUO1Pjr4qDs=
+github.com/robfig/cron/v3 v3.0.1/go.mod h1:eQICP3HwyT7UooqI/z+Ov+PtYAWygg1TEWWzGIFLtro=
 github.com/rogpeppe/fastuuid v0.0.0-20150106093220-6724a57986af/go.mod h1:XWv6SoW27p1b0cqNHllgS5HIMJraePCO15w5zCzIWYg=
 github.com/rogpeppe/fastuuid v1.2.0/go.mod h1:jVj6XXZzXRy/MSR5jhDC/2q6DgLz+nrA6LYCDYWNEvQ=
 github.com/rogpeppe/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4=

diff --git a/pkg/ccl/backupccl/BUILD.bazel b/pkg/ccl/backupccl/BUILD.bazel
@@ -126,9 +126,9 @@ go_library(
         "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_gogo_protobuf//jsonpb",
         "@com_github_gogo_protobuf//types",
-        "@com_github_gorhill_cronexpr//:cronexpr",
         "@com_github_kr_pretty//:pretty",
         "@com_github_lib_pq//oid",
+        "@com_github_robfig_cron_v3//:cron",
         "@com_github_stretchr_testify//require",
     ],
 )
@@ -251,10 +251,10 @@ go_test(
         "@com_github_cockroachdb_pebble//vfs",
         "@com_github_gogo_protobuf//proto",
         "@com_github_gogo_protobuf//types",
-        "@com_github_gorhill_cronexpr//:cronexpr",
         "@com_github_jackc_pgx_v4//:pgx",
         "@com_github_kr_pretty//:pretty",
         "@com_github_lib_pq//:pq",
+        "@com_github_robfig_cron_v3//:cron",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
         "@org_golang_x_sync//errgroup",

diff --git a/pkg/ccl/backupccl/create_scheduled_backup.go b/pkg/ccl/backupccl/create_scheduled_backup.go
@@ -39,7 +39,7 @@ import (
 	"github.com/cockroachdb/errors"
 	"github.com/gogo/protobuf/jsonpb"
 	pbtypes "github.com/gogo/protobuf/types"
-	"github.com/gorhill/cronexpr"
+	"github.com/robfig/cron/v3"
 )
 
 const (
@@ -195,19 +195,19 @@ func computeScheduleRecurrence(
 	if evalFn == nil {
 		return neverRecurs, nil
 	}
-	cron, err := evalFn()
+	cronStr, err := evalFn()
 	if err != nil {
 		return nil, err
 	}
-	expr, err := cronexpr.Parse(cron)
+	expr, err := cron.ParseStandard(cronStr)
 	if err != nil {
 		return nil, errors.Newf(
 			`error parsing schedule expression: %q; it must be a valid cron expression`,
-			cron)
+			cronStr)
 	}
 	nextRun := expr.Next(now)
 	frequency := expr.Next(nextRun).Sub(nextRun)
-	return &scheduleRecurrence{cron, frequency}, nil
+	return &scheduleRecurrence{cronStr, frequency}, nil
 }
 
 var forceFullBackup *scheduleRecurrence

diff --git a/pkg/ccl/backupccl/create_scheduled_backup_test.go b/pkg/ccl/backupccl/create_scheduled_backup_test.go
@@ -40,7 +40,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	pbtypes "github.com/gogo/protobuf/types"
-	"github.com/gorhill/cronexpr"
+	"github.com/robfig/cron/v3"
 	"github.com/stretchr/testify/require"
 )
 
@@ -1078,8 +1078,10 @@ INSERT INTO t values (1), (10), (100);
 		// to the next scheduled recurrence.
 		for _, id := range []int64{fullID, incID} {
 			s := th.loadSchedule(t, id)
+			e, err := cron.ParseStandard(s.ScheduleExpr())
+			require.NoError(t, err)
 			require.EqualValues(t,
-				cronexpr.MustParse(s.ScheduleExpr()).Next(th.env.Now()).Round(time.Microsecond),
+				e.Next(th.env.Now()).Round(time.Microsecond),
 				s.NextRun())
 		}
 

diff --git a/pkg/jobs/BUILD.bazel b/pkg/jobs/BUILD.bazel
@@ -61,8 +61,8 @@ go_library(
         "@com_github_cockroachdb_logtags//:logtags",
         "@com_github_cockroachdb_redact//:redact",
         "@com_github_gogo_protobuf//types",
-        "@com_github_gorhill_cronexpr//:cronexpr",
         "@com_github_prometheus_client_model//go",
+        "@com_github_robfig_cron_v3//:cron",
         "@io_opentelemetry_go_otel//attribute",
     ],
 )
@@ -135,8 +135,8 @@ go_test(
         "@com_github_cockroachdb_redact//:redact",
         "@com_github_gogo_protobuf//types",
         "@com_github_google_go_cmp//cmp",
-        "@com_github_gorhill_cronexpr//:cronexpr",
         "@com_github_kr_pretty//:pretty",
+        "@com_github_robfig_cron_v3//:cron",
         "@com_github_stretchr_testify//assert",
         "@com_github_stretchr_testify//require",
     ],

diff --git a/pkg/jobs/executor_impl_test.go b/pkg/jobs/executor_impl_test.go
@@ -19,7 +19,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/gorhill/cronexpr"
 	"github.com/stretchr/testify/require"
 )
 
@@ -40,7 +39,7 @@ func TestInlineExecutorFailedJobsHandling(t *testing.T) {
 	}{
 		{
 			onError:         jobspb.ScheduleDetails_RETRY_SCHED,
-			expectedNextRun: cronexpr.MustParse("@daily").Next(h.env.Now()).Round(time.Microsecond),
+			expectedNextRun: cronMustParse(t, "@daily").Next(h.env.Now()).Round(time.Microsecond),
 		},
 		{
 			onError:         jobspb.ScheduleDetails_RETRY_SOON,

diff --git a/pkg/jobs/job_scheduler_test.go b/pkg/jobs/job_scheduler_test.go
@@ -38,10 +38,16 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
 	"github.com/gogo/protobuf/types"
-	"github.com/gorhill/cronexpr"
+	"github.com/robfig/cron/v3"
 	"github.com/stretchr/testify/require"
 )
 
+func cronMustParse(t *testing.T, s string) cron.Schedule {
+	e, err := cron.ParseStandard(s)
+	require.NoError(t, err)
+	return e
+}
+
 func TestJobSchedulerReschedulesRunning(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -75,7 +81,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) {
 				}))
 
 			// Verify the job has expected nextRun time.
-			expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime := cronMustParse(t, "@hourly").Next(h.env.Now())
 			loaded := h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -93,7 +99,7 @@ func TestJobSchedulerReschedulesRunning(t *testing.T) {
 			if wait == jobspb.ScheduleDetails_WAIT {
 				expectedRunTime = h.env.Now().Add(recheckRunningAfter)
 			} else {
-				expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+				expectedRunTime = cronMustParse(t, "@hourly").Next(h.env.Now())
 			}
 			loaded = h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
@@ -133,7 +139,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) {
 				}))
 
 			// Verify the job has expected nextRun time.
-			expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime := cronMustParse(t, "@hourly").Next(h.env.Now())
 			loaded := h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -147,7 +153,7 @@ func TestJobSchedulerExecutesAfterTerminal(t *testing.T) {
 					return s.executeSchedules(ctx, allSchedules, txn)
 				}))
 
-			expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+			expectedRunTime = cronMustParse(t, "@hourly").Next(h.env.Now())
 			loaded = h.loadSchedule(t, j.ScheduleID())
 			require.Equal(t, expectedRunTime, loaded.NextRun())
 		})
@@ -173,7 +179,7 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) {
 		}))
 
 	// Verify the job has expected nextRun time.
-	expectedRunTime := cronexpr.MustParse("@hourly").Next(h.env.Now())
+	expectedRunTime := cronMustParse(t, "@hourly").Next(h.env.Now())
 	loaded := h.loadSchedule(t, j.ScheduleID())
 	require.Equal(t, expectedRunTime, loaded.NextRun())
 
@@ -187,7 +193,7 @@ func TestJobSchedulerExecutesAndSchedulesNextRun(t *testing.T) {
 			return s.executeSchedules(ctx, allSchedules, txn)
 		}))
 
-	expectedRunTime = cronexpr.MustParse("@hourly").Next(h.env.Now())
+	expectedRunTime = cronMustParse(t, "@hourly").Next(h.env.Now())
 	loaded = h.loadSchedule(t, j.ScheduleID())
 	require.Equal(t, expectedRunTime, loaded.NextRun())
 }
@@ -550,7 +556,7 @@ func TestJobSchedulerRetriesFailed(t *testing.T) {
 	startTime := h.env.Now()
 	execTime := startTime.Add(time.Hour).Add(time.Second)
 
-	cron := cronexpr.MustParse("@hourly")
+	cron := cronMustParse(t, "@hourly")
 
 	for _, tc := range []struct {
 		onError jobspb.ScheduleDetails_ErrorHandlingBehavior

diff --git a/pkg/jobs/scheduled_job.go b/pkg/jobs/scheduled_job.go
@@ -27,7 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
-	"github.com/gorhill/cronexpr"
+	"github.com/robfig/cron/v3"
 )
 
 // scheduledJobRecord is a reflective representation of a row in
@@ -194,12 +194,13 @@ func (j *ScheduledJob) Frequency() (time.Duration, error) {
 		return 0, errors.Newf(
 			"schedule %d is not periodic", j.rec.ScheduleID)
 	}
-	expr, err := cronexpr.Parse(j.rec.ScheduleExpr)
+	expr, err := cron.ParseStandard(j.rec.ScheduleExpr)
 	if err != nil {
 		return 0, errors.Wrapf(err,
 			"parsing schedule expression: %q; it must be a valid cron expression",
 			j.rec.ScheduleExpr)
 	}
+
 	next := expr.Next(j.env.Now())
 	nextNext := expr.Next(next)
 	return nextNext.Sub(next), nil
@@ -211,7 +212,7 @@ func (j *ScheduledJob) ScheduleNextRun() error {
 		return errors.Newf(
 			"cannot set next run for schedule %d (empty schedule)", j.rec.ScheduleID)
 	}
-	expr, err := cronexpr.Parse(j.rec.ScheduleExpr)
+	expr, err := cron.ParseStandard(j.rec.ScheduleExpr)
 	if err != nil {
 		return errors.Wrapf(err, "parsing schedule expression: %q", j.rec.ScheduleExpr)
 	}

diff --git a/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel b/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel
@@ -46,7 +46,7 @@ go_library(
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_gogo_protobuf//types",
-        "@com_github_gorhill_cronexpr//:cronexpr",
+        "@com_github_robfig_cron_v3//:cron",
     ],
 )
 

diff --git a/pkg/sql/sqlstats/persistedsqlstats/cluster_settings.go b/pkg/sql/sqlstats/persistedsqlstats/cluster_settings.go
@@ -15,7 +15,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/errors"
-	"github.com/gorhill/cronexpr"
+	"github.com/robfig/cron/v3"
 )
 
 // SQLStatsFlushInterval is the cluster setting that controls how often the SQL
@@ -73,7 +73,7 @@ var SQLStatsCleanupRecurrence = settings.RegisterValidatedStringSetting(
 	"cron-tab recurrence for SQL Stats cleanup job",
 	"@hourly", /* defaultValue */
 	func(_ *settings.Values, s string) error {
-		if _, err := cronexpr.Parse(s); err != nil {
+		if _, err := cron.ParseStandard(s); err != nil {
 			return errors.Wrap(err, "invalid cron expression")
 		}
 		return nil

diff --git a/pkg/sql/sqlstats/persistedsqlstats/scheduled_sql_stats_compaction_test.go b/pkg/sql/sqlstats/persistedsqlstats/scheduled_sql_stats_compaction_test.go
@@ -198,7 +198,9 @@ func TestSQLStatsScheduleOperations(t *testing.T) {
 
 	t.Run("warn_schedule_long_run_interval", func(t *testing.T) {
 		t.Run("via cluster setting", func(t *testing.T) {
-			helper.sqlDB.Exec(t, "SET CLUSTER SETTING sql.stats.cleanup.recurrence = '0 59 23 24 12 ? 2099'")
+			// Craft an expression that next repeats next month.
+			expr := fmt.Sprintf("59 23 24 %d ?", timeutil.Now().AddDate(0, 1, 0).Month())
+			helper.sqlDB.Exec(t, "SET CLUSTER SETTING sql.stats.cleanup.recurrence = $1", expr)
 
 			var err error
 			testutils.SucceedsSoon(t, func() error {
@@ -208,7 +210,7 @@ func TestSQLStatsScheduleOperations(t *testing.T) {
 				if err == nil {
 					return errors.Newf("retry: next_run=%s, schedule_expr=%s", sj.NextRun(), sj.ScheduleExpr())
 				}
-				require.Equal(t, "0 59 23 24 12 ? 2099", sj.ScheduleExpr())
+				require.Equal(t, expr, sj.ScheduleExpr())
 				return nil
 			})
 			require.True(t, errors.Is(

diff --git a/vendor b/vendor
+0 −202		github.com/gorhill/cronexpr/APLv2
+0 −674		github.com/gorhill/cronexpr/GPLv3
+0 −134		github.com/gorhill/cronexpr/README.md
+0 −266		github.com/gorhill/cronexpr/cronexpr.go
+0 −292		github.com/gorhill/cronexpr/cronexpr_next.go
+0 −503		github.com/gorhill/cronexpr/cronexpr_parse.go
+22 −0		github.com/robfig/cron/v3/.gitignore
+1 −0		github.com/robfig/cron/v3/.travis.yml
+21 −0		github.com/robfig/cron/v3/LICENSE
+125 −0		github.com/robfig/cron/v3/README.md
+92 −0		github.com/robfig/cron/v3/chain.go
+27 −0		github.com/robfig/cron/v3/constantdelay.go
+355 −0		github.com/robfig/cron/v3/cron.go
+231 −0		github.com/robfig/cron/v3/doc.go
+86 −0		github.com/robfig/cron/v3/logger.go
+45 −0		github.com/robfig/cron/v3/option.go
+434 −0		github.com/robfig/cron/v3/parser.go
+188 −0		github.com/robfig/cron/v3/spec.go
+3 −3		modules.txt