fix: cron string validation #19071
fix: cron string validation #19071
Conversation
Codecov Report
@@ Coverage Diff @@
## main #19071 +/- ##
=======================================
Coverage 67.40% 67.41%
=======================================
Files 993 993
Lines 108812 108864 +52
Branches 2751 2751
=======================================
+ Hits 73347 73392 +45
- Misses 31526 31527 +1
- Partials 3939 3945 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| if err != nil { | ||
| return false, err | ||
| } | ||
| cronParts := strings.Split(cron, " ") |
There was a problem hiding this comment.
s, err := CronParser().Parse(cron)
if err != nil {
return false, err
}
t, ok := s.(*cronlib.SpecSchedule)
if ok {
if t.Second == 0 {
return false, err
}
}
There was a problem hiding this comment.
tried setting cron as 0 0 0 1 1 *, the t.Second is not 0.
| // ValidatePreheatPolicy validate preheat policy | ||
| func (s *Schema) ValidatePreheatPolicy() error { | ||
| // currently only validate cron string of preheat policy | ||
| if s.Trigger != nil && s.Trigger.Type == TriggerTypeScheduled && len(s.Trigger.Settings.Cron) > 0 { |
There was a problem hiding this comment.
duplicate validation in the length of cron string
There was a problem hiding this comment.
This is because we need to skip utils.ValidateCronString() validation for cron with empty string "". In preheat and tag retention, it uses empty string cron to unschedule an existing job.
| // currently only validate cron string of preheat policy | ||
| if s.Trigger != nil && s.Trigger.Type == TriggerTypeScheduled && len(s.Trigger.Settings.Cron) > 0 { | ||
| isValid, err := utils.ValidateCronString(s.Trigger.Settings.Cron) | ||
| if !isValid || err != nil { |
There was a problem hiding this comment.
if err != nil {
return err --- this is 500 error
}
if !isValid {
return BadRequest -- this is 400 error
}
There was a problem hiding this comment.
actually，when the first field of cron is not 0, like "1 0 0 1 1 *", utils.ValidateCronString will return false and with error message. This is a BadRequest.
There was a problem hiding this comment.
maybe for utils.ValidateCronString() we should only return error, instead of return (bool, error)
| @@ -58,6 +60,24 @@ type Metadata struct { | |||
| Scope *Scope `json:"scope" valid:"Required"` | |||
| } | |||
|
|
|||
| // ValidateRetentionPolicy validate the retention policy | |||
| func (m *Metadata) ValidateRetentionPolicy() error { | |||
There was a problem hiding this comment.
Same as above. This is because we need to skip utils.ValidateCronString() validation for cron with empty string "". In preheat and tag retention, it uses empty string cron to unschedule an existing job.
… when there are 6 fields) Signed-off-by: Shengwen Yu <yshengwen@vmware.com>
Thank you for contributing to Harbor!
Comprehensive Summary of your change
Issue being fixed
Fixes #18091
task 2
Please indicate you've done the following: