search

datreeio / datree

Prevent Kubernetes misconfigurations from reaching production (again 😤 )! From code to cloud, Datree provides an E2E policy enforcement solution to run automatic checks for rule violations. See our docs: https://hub.datree.io
https://datree.io
Apache License 2.0
6.39k stars 363 forks source link

Rule CRONJOB_INVALID_SCHEDULE_VALUE is incomplete #711

Closed kedith closed 2 years ago

kedith commented 2 years ago

Describe the bug The default rule #8 is incomplete. I found a valid CronJob schedule pattern which isn't accepted by the rule.

To Reproduce Steps to reproduce the behavior:

Expected behavior The rule should also accept 00 for minute/hour, not just 0.

Datree version (run datree version):

Additional context
I am trying to integrate datree in a project and it fails on a valid CronJob definition we have. This rule is useful and it would be great if the regex would be fixed.

eyarz commented 2 years ago

good catch. I updated the regex and put it on regex101. @kedith before I'm opening a PR for this fix, can you help me verify the new (regex) pattern by testing some valid & invalid cron examples? I want to verify it's not producing false positive/negative results.

kedith commented 2 years ago

thanks @eyarz. I will try to test it with some values too.

kedith commented 2 years ago

* 00 * * * should be accepted by the regex as well.

eyarz commented 2 years ago

oops... @kedith I fixed the regex and also added validation for * * * * 00, so please give it another try now.

If you are wondering why I missed all those 00 expressions is because it was not provided as an allowed value in the CRON expression definition 😅

image

kedith commented 2 years ago

@eyarz I tested it with more values. It seems to pass on all my examples but I found some corner cases where it shouldn't pass but passes. I thought of testing ranges where the starting value is still valid but the upper range not, ex: * * * 12-14 *, * * * * 5,8

I found this website useful while thinking of valid/invalid patterns https://crontab.guru/

Thanks a lot for your fast responses and help 🙂

eyarz commented 2 years ago

@kedith you are right, the regex didn't validate the range for the second set of numbers after , or - 🙈 I updated the regex, and I think we are ready now! 🤞

I'm familiar with crontab.guru, but the problem is that you can only generate random examples of valid cron expressions...

kedith commented 2 years ago

Great! I don't have any other ideas for test cases. Thanks a lot @eyarz 🎉!

eyarz commented 2 years ago

The rule logic was updated on will be fixed on the next CLI release. @kedith it looks like we just created the first regex to validate cron statements so I also published it on StackOverflow 🤓