From 7ad87f8c3248effcbd30f34997f2f64724141cee Mon Sep 17 00:00:00 2001 From: Ben Jackson Date: Mon, 16 Sep 2024 09:01:31 +1000 Subject: [PATCH] refactor: use robfigcron for validation after lagoon replacements (#368) --- go.mod | 1 + go.sum | 1 + internal/helpers/helpers_cron.go | 414 +++++++++----------------- internal/helpers/helpers_cron_test.go | 32 +- 4 files changed, 172 insertions(+), 276 deletions(-) diff --git a/go.mod b/go.mod index ed1aed91..d5b3cf66 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/google/go-cmp v0.6.0 github.com/hashicorp/go-retryablehttp v0.7.7 github.com/k8up-io/k8up/v2 v2.11.1 + github.com/robfig/cron/v3 v3.0.1 github.com/spf13/cobra v1.8.1 github.com/uselagoon/machinery v0.0.29 github.com/vshn/k8up v1.99.99 diff --git a/go.sum b/go.sum index d53f54f0..4df037a8 100644 --- a/go.sum +++ b/go.sum @@ -706,6 +706,7 @@ github.com/qri-io/starlib v0.4.2-0.20200213133954-ff2e8cd5ef8d/go.mod h1:7DPO4do github.com/quasilyte/go-consistent v0.0.0-20190521200055-c6f3937de18c/go.mod h1:5STLWrekHfjyYwxBRVRXNOSewLJ3PWfDJd1VyTS21fI= github.com/remyoudompheng/bigfft v0.0.0-20170806203942-52369c62f446/go.mod h1:uYEyJGbgTkfkS4+E/PavXkNJcbFIpEtjt2B0KDQ5+9M= github.com/rhnvrm/simples3 v0.6.1/go.mod h1:Y+3vYm2V7Y4VijFoJHHTrja6OgPrJ2cBti8dPGkC3sA= +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/go-internal v1.1.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFRclV5y23lUDJ4= diff --git a/internal/helpers/helpers_cron.go b/internal/helpers/helpers_cron.go index 5b1563b3..c8b82a74 100644 --- a/internal/helpers/helpers_cron.go +++ b/internal/helpers/helpers_cron.go @@ -4,237 +4,160 @@ import ( "fmt" "math" "regexp" - "slices" "strconv" "strings" "github.com/cxmcc/unixsums/cksum" + + cron "github.com/robfig/cron/v3" ) -func ConvertCrontab(namespace, cron string) (string, error) { - // Seed is used to generate pseudo random numbers. The seed is based on the - // namespace, so will not change after a deployment for a given namespace. - seed := cksum.Cksum([]byte(fmt.Sprintf("%s\n", namespace))) - var minutes, hours, days, months, dayweek string - splitCron := strings.Split(strings.Trim(cron, " "), " ") - // check the provided cron splits into 5 - if len(splitCron) == 5 { - for idx, val := range splitCron { - if idx == 0 { - match1, _ := regexp.MatchString("^(M|H)$", val) - if match1 { - // If just an `M` or `H` (for backwards compatibility) is defined, we - // generate a pseudo random minute. - minutes = strconv.Itoa(int(math.Mod(float64(seed), 60))) - continue - } - match2, _ := regexp.MatchString("^(M|H|\\*)/([0-5]?[0-9])$", val) - if match2 { - // A Minute like M/15 (or H/15 or */15 for backwards compatibility) is defined, create a list of minutes with a random start - // like 4,19,34,49 or 6,21,36,51 - params := getCaptureBlocks("^(?PM|H|\\*)/(?P[0-5]?[0-9])$", val) - step, err := strconv.Atoi(params["P2"]) - if err != nil { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine minutes value", cron) - } - counter := int(math.Mod(float64(seed), float64(step))) - var minutesArr []string - for counter < 60 { - minutesArr = append(minutesArr, fmt.Sprintf("%d", counter)) - counter += step - } - minutes = strings.Join(minutesArr, ",") - continue - } - if isInCSVRange(val, 0, 59) { - // A minute like 0,10,15,30,59 - minutes = val - continue - } - if isInRange(val, 0, 59) { - // A minute like 0-59 - minutes = val - continue - } - if val == "*" { - // otherwise pass the * through - minutes = val - continue - } - // if the value is not valid, return an error with where the issue is - if minutes == "" { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine minutes value", cron) - } - } - if idx == 1 { - match1, _ := regexp.MatchString("^H$", val) - if match1 { - // If just an `H` is defined, we generate a pseudo random hour. - hours = strconv.Itoa(int(math.Mod(float64(seed), 24))) - continue - } - match2, _ := regexp.MatchString("^H\\(([01]?[0-9]|2[0-3])-([01]?[0-9]|2[0-3])\\)$", val) - if match2 { - // If H is defined with a given range, example: H(2-4), we generate a random hour between 2-4 - params := getCaptureBlocks("^H\\((?P[01]?[0-9]|2[0-3])-(?P[01]?[0-9]|2[0-3])\\)$", val) - hFrom, err := strconv.Atoi(params["P1"]) - if err != nil { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine hours value", cron) - } - hTo, err := strconv.Atoi(params["P2"]) - if err != nil { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine hours value", cron) - } - if hFrom < hTo { - // Example: HOUR_FROM: 2, HOUR_TO: 4 - // Calculate the difference between the two hours (in example will be 2) - maxDiff := float64(hTo - hFrom) - // Generate a difference based on the SEED (in example will be 0, 1 or 2) - diff := int(math.Mod(float64(seed), maxDiff)) - // Add the generated difference to the FROM hour (in example will be 2, 3 or 4) - hours = strconv.Itoa(hFrom + diff) - continue - } - if hFrom > hTo { - // If the FROM is larger than the TO, we have a range like 22-2 - // Calculate the difference between the two hours with a 24 hour jump (in example will be 4) - maxDiff := float64(24 - hFrom + hTo) - // Generate a difference based on the SEED (in example will be 0, 1, 2, 3 or 4) - diff := int(math.Mod(float64(seed), maxDiff)) - // Add the generated difference to the FROM hour (in example will be 22, 23, 24, 25 or 26) - if hFrom+diff >= 24 { - // If the hour is higher than 24, we subtract 24 to handle the midnight change - hours = strconv.Itoa(hFrom + diff - 24) - continue - } - hours = strconv.Itoa(hFrom + diff) - continue - } - if hFrom == hTo { - hours = strconv.Itoa(hFrom) - continue - } - } - match3, _ := regexp.MatchString("^(H|\\*)/([01]?[0-9]|2[0-3])$", val) - if match3 { - // An hour like H/15 or */15 is defined, create a list of hours with a random start - // like 1,7,13,19 - params := getCaptureBlocks("^(?PH|\\*)/(?P[01]?[0-9]|2[0-3])$", val) - step, err := strconv.Atoi(params["P2"]) - if err != nil { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine hours value", cron) - } - counter := int(math.Mod(float64(seed), float64(step))) - var hoursArr []string - for counter < 24 { - hoursArr = append(hoursArr, fmt.Sprintf("%d", counter)) - counter += step - } - hours = strings.Join(hoursArr, ",") - continue - } - if isInCSVRange(val, 0, 23) { - hours = val - continue - } - if isInRange(val, 0, 23) { - hours = val - continue - } - if val == "*" { - hours = val - continue - } - sVal := strings.Split(val, ",") - if len(sVal) > 1 { - for _, r := range sVal { - if isInRange(r, 0, 23) { - continue - } - } - hours = val - } - // if the value is not valid, return an error with where the issue is - if hours == "" { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine hours value", cron) - } - } - if idx == 2 { - if isInCSVRange(val, 1, 31) { - days = val - continue - } - if isInRange(val, 1, 31) { - days = val - continue - } - if val == "*" { - days = val - continue - } - // if the value is not valid, return an error with where the issue is - if days == "" { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine days value", cron) - } - } - if idx == 3 { - if isInCSVRange(val, 1, 12) { - months = val - continue - } - if isInRange(val, 1, 12) { - months = val - continue - } - if val == "*" { - months = val - continue - } - if isMonth(val) { - months = val - continue - } - // if the value is not valid, return an error with where the issue is - if months == "" { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine months value", cron) - } - } - if idx == 4 { - if isInCSVRange(val, 0, 6) { - dayweek = val - continue - } - if isInRange(val, 0, 6) { - dayweek = val - continue - } - if val == "*" { - dayweek = val - continue - } - if isDayOfWeek(val) { - dayweek = val - continue - } - // if the value is not valid, return an error with where the issue is - if dayweek == "" { - return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine day(week) value", cron) - } +type Cron struct { + Minute string + Hour string + Day string + Month string + DayOfWeek string +} + +func (c *Cron) String() string { + return fmt.Sprintf("%s %s %s %s %s", c.Minute, c.Hour, c.Day, c.Month, c.DayOfWeek) +} + +// this will check if someone has requested a pseudo random interval for the minute using `M/` or `H/` +func (c *Cron) validateReplaceMinute(seed uint32) error { + match1, _ := regexp.MatchString("^(M|H)$", c.Minute) + if match1 { + // If just an `M` or `H` (for backwards compatibility) is defined, we + // generate a pseudo random minute. + c.Minute = strconv.Itoa(int(math.Mod(float64(seed), 60))) + } + match2, _ := regexp.MatchString("^(M|H|\\*)/([0-5]?[0-9])$", c.Minute) + if match2 { + // A Minute like M/15 (or H/15 or */15 for backwards compatibility) is defined, create a list of minutes with a random start + // like 4,19,34,49 or 6,21,36,51 + params := getCaptureBlocks("^(?PM|H|\\*)/(?P[0-5]?[0-9])$", c.Minute) + step, err := strconv.Atoi(params["P2"]) + if err != nil { + return fmt.Errorf("unable to determine hours value") + } + counter := int(math.Mod(float64(seed), float64(step))) + var minutesArr []string + for counter < 60 { + minutesArr = append(minutesArr, fmt.Sprintf("%d", counter)) + counter += step + } + c.Minute = strings.Join(minutesArr, ",") + } + return nil +} + +// this will check if someone has requested a pseudo random interval for the hour using `H/` +func (c *Cron) validateReplaceHour(seed uint32) error { + match1, _ := regexp.MatchString("^H$", c.Hour) + if match1 { + // If just an `H` is defined, we generate a pseudo random hour. + c.Hour = strconv.Itoa(int(math.Mod(float64(seed), 24))) + } + match2, _ := regexp.MatchString("^H\\(([01]?[0-9]|2[0-3])-([01]?[0-9]|2[0-3])\\)$", c.Hour) + if match2 { + // If H is defined with a given range, example: H(2-4), we generate a random hour between 2-4 + params := getCaptureBlocks("^H\\((?P[01]?[0-9]|2[0-3])-(?P[01]?[0-9]|2[0-3])\\)$", c.Hour) + hFrom, err := strconv.Atoi(params["P1"]) + if err != nil { + return fmt.Errorf("unable to determine hours value") + } + hTo, err := strconv.Atoi(params["P2"]) + if err != nil { + return fmt.Errorf("unable to determine hours value") + } + if hFrom < hTo { + // Example: HOUR_FROM: 2, HOUR_TO: 4 + // Calculate the difference between the two hours (in example will be 2) + maxDiff := float64(hTo - hFrom) + // Generate a difference based on the SEED (in example will be 0, 1 or 2) + diff := int(math.Mod(float64(seed), maxDiff)) + // Add the generated difference to the FROM hour (in example will be 2, 3 or 4) + c.Hour = strconv.Itoa(hFrom + diff) + return nil + } + if hFrom > hTo { + // If the FROM is larger than the TO, we have a range like 22-2 + // Calculate the difference between the two hours with a 24 hour jump (in example will be 4) + maxDiff := float64(24 - hFrom + hTo) + // Generate a difference based on the SEED (in example will be 0, 1, 2, 3 or 4) + diff := int(math.Mod(float64(seed), maxDiff)) + // Add the generated difference to the FROM hour (in example will be 22, 23, 24, 25 or 26) + if hFrom+diff >= 24 { + // If the hour is higher than 24, we subtract 24 to handle the midnight change + c.Hour = strconv.Itoa(hFrom + diff - 24) + return nil } + c.Hour = strconv.Itoa(hFrom + diff) + return nil + } + if hFrom == hTo { + c.Hour = strconv.Itoa(hFrom) + } + } + match3, _ := regexp.MatchString("^(H|\\*)/([01]?[0-9]|2[0-3])$", c.Hour) + if match3 { + // An hour like H/15 or */15 is defined, create a list of hours with a random start + // like 1,7,13,19 + params := getCaptureBlocks("^(?PH|\\*)/(?P[01]?[0-9]|2[0-3])$", c.Hour) + step, err := strconv.Atoi(params["P2"]) + if err != nil { + return fmt.Errorf("unable to determine hours value") + } + counter := int(math.Mod(float64(seed), float64(step))) + var hoursArr []string + for counter < 24 { + hoursArr = append(hoursArr, fmt.Sprintf("%d", counter)) + counter += step } - return fmt.Sprintf("%v %v %v %v %v", minutes, hours, days, months, dayweek), nil + c.Hour = strings.Join(hoursArr, ",") } - if len(splitCron) < 5 && len(splitCron) > 0 || len(splitCron) > 5 { - return "", fmt.Errorf("cron definition '%s' is invalid, %d fields provided, required 5", cron, len(splitCron)) + return nil +} + +func ConvertCrontab(namespace, schedule string) (string, error) { + splitSchedule := strings.Split(strings.Trim(schedule, " "), " ") + seed := cksum.Cksum([]byte(fmt.Sprintf("%s\n", namespace))) + if len(splitSchedule) == 5 { + newSchedule := &Cron{ + Minute: splitSchedule[0], + Hour: splitSchedule[1], + Day: splitSchedule[2], + Month: splitSchedule[3], + DayOfWeek: splitSchedule[4], + } + // validate for any M/H style replacements for pseudo-random intervals + if err := newSchedule.validateReplaceMinute(seed); err != nil { + return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine minutes value", schedule) + } + // validate for any H style replacements for pseudo-random intervals + if err := newSchedule.validateReplaceHour(seed); err != nil { + return "", fmt.Errorf("cron definition '%s' is invalid, unable to determine hours value", schedule) + } + // parse/validate the same as kubernetes once the pseudo-random intervals have been calculated and updated into the schedule + // https://github.com/kubernetes/kubernetes/blob/58c44005cdaec53fe3cb49b2d7a308df3af2d081/pkg/controller/cronjob/cronjob_controllerv2.go#L394 + if _, err := cron.ParseStandard(newSchedule.String()); err != nil { + return "", fmt.Errorf("cron definition '%s' is invalid", schedule) + } + // if valid, return the cron schedule string value + return newSchedule.String(), nil + } + if len(splitSchedule) < 5 && len(splitSchedule) > 0 || len(splitSchedule) > 5 { + return "", fmt.Errorf("cron definition '%s' is invalid, %d fields provided, required 5", schedule, len(splitSchedule)) } - return "", fmt.Errorf("cron definition '%s' is invalid", cron) + return "", fmt.Errorf("cron definition '%s' is invalid", schedule) } -func IsInPodCronjob(cron string) (bool, error) { - splitCron := strings.Split(strings.Trim(cron, " "), " ") +func IsInPodCronjob(schedule string) (bool, error) { + splitSchedule := strings.Split(strings.Trim(schedule, " "), " ") // check the provided cron splits into 5 - if len(splitCron) == 5 { - for idx, val := range splitCron { + if len(splitSchedule) == 5 { + for idx, val := range splitSchedule { if idx == 0 { match1, _ := regexp.MatchString("^(M|H|\\*)/([0-5]?[0-9])$", val) if match1 { @@ -243,7 +166,7 @@ func IsInPodCronjob(cron string) (bool, error) { params := getCaptureBlocks("^(?PM|H|\\*)/(?P[0-5]?[0-9])$", val) step, err := strconv.Atoi(params["P2"]) if err != nil { - return false, fmt.Errorf("cron definition '%s' is invalid, unable to determine minutes value", cron) + return false, fmt.Errorf("cron definition '%s' is invalid, unable to determine minutes value", schedule) } if step < 30 { return true, nil @@ -271,56 +194,3 @@ func getCaptureBlocks(regex, val string) (captureMap map[string]string) { } return captureMap } - -// check if the provided cron time definition is a valid `1,2,4,8` type range -func isInCSVRange(s string, min, max int) bool { - items := strings.Split(s, ",") - for _, val := range items { - num, err := strconv.Atoi(val) - if err != nil { - // not a number, return false - return false - } - if num < min || num > max { - // outside range, return false - return false - } - } - return true -} - -// check if the provided cron day string is a valid -func isDayOfWeek(s string) bool { - days := []string{"SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT"} - return slices.Contains(days, strings.ToUpper(s)) -} - -// check if the provided cron month string is a valid -func isMonth(s string) bool { - days := []string{"JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT", "NOV", "DEC"} - return slices.Contains(days, strings.ToUpper(s)) -} - -// check if the provided cron time definition is a valid `1-2` type range -func isInRange(s string, min, max int) bool { - items := strings.Split(s, "-") - if len(items) > 2 || len(items) < 1 { - // too many or not enough items split by - - return false - } - hFrom, err := strconv.Atoi(items[0]) - if err != nil { - // not a number or error checking if it is, return false - return false - } - hTo, err := strconv.Atoi(items[1]) - if err != nil { - // not a number or error checking if it is, return false - return false - } - if hFrom > hTo || hFrom < min || hFrom > max || hTo < min || hTo > max { - // numbers in range are not in valid format of LOW-HIGH - return false - } - return true -} diff --git a/internal/helpers/helpers_cron_test.go b/internal/helpers/helpers_cron_test.go index df5359e0..f8d72476 100644 --- a/internal/helpers/helpers_cron_test.go +++ b/internal/helpers/helpers_cron_test.go @@ -63,7 +63,7 @@ func TestConvertCrontab(t *testing.T) { namespace: "example-com-main", cron: "M/H5 H(22-2) * * *", }, - wantErrMsg: "cron definition 'M/H5 H(22-2) * * *' is invalid, unable to determine minutes value", + wantErrMsg: "cron definition 'M/H5 H(22-2) * * *' is invalid", wantErr: true, }, { @@ -72,7 +72,7 @@ func TestConvertCrontab(t *testing.T) { namespace: "example-com-main", cron: "M/15 H(H2-2) * * *", }, - wantErrMsg: "cron definition 'M/15 H(H2-2) * * *' is invalid, unable to determine hours value", + wantErrMsg: "cron definition 'M/15 H(H2-2) * * *' is invalid", wantErr: true, }, { @@ -97,7 +97,7 @@ func TestConvertCrontab(t *testing.T) { namespace: "example-com-main", cron: "M/15 H(22-2) * * 1-8", }, - wantErrMsg: "cron definition 'M/15 H(22-2) * * 1-8' is invalid, unable to determine day(week) value", + wantErrMsg: "cron definition 'M/15 H(22-2) * * 1-8' is invalid", wantErr: true, }, { @@ -122,7 +122,7 @@ func TestConvertCrontab(t *testing.T) { namespace: "example-com-main", cron: "15 * 1-32 * *", }, - wantErrMsg: "cron definition '15 * 1-32 * *' is invalid, unable to determine days value", + wantErrMsg: "cron definition '15 * 1-32 * *' is invalid", wantErr: true, }, { @@ -199,6 +199,30 @@ func TestConvertCrontab(t *testing.T) { }, want: "1,31 0-12,22-23 * * *", }, + { + name: "test23 - step hours", + args: args{ + namespace: "example-com-main", + cron: "0 17/12 * * *", + }, + want: "0 17/12 * * *", + }, + { + name: "test24 - step and range hours", + args: args{ + namespace: "example-com-main", + cron: "0 17/12,0-23 * * *", + }, + want: "0 17/12,0-23 * * *", + }, + { + name: "test25 - random minute with step and range hours", + args: args{ + namespace: "example-com-main", + cron: "M/30 17/12,0-23 * * *", + }, + want: "1,31 17/12,0-23 * * *", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {