8000 Fix: validate schedule_expression for EventBridge Scheduler by anisaoshafi · Pull Request #12191 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Fix: validate schedule_expression for EventBridge Scheduler #12191

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jan 27, 2025
Merged

Fix: validate schedule_expression for EventBridge Scheduler #12191

merged 1 commit into from
Jan 27, 2025
+393 −1

Conversation

anisaoshafi
Copy link
Contributor

Motivation

Customer reported an issue for scheduler validation of schedule_expression #9284.
The scheduler relies fully on moto for now, and validation is not implemented for schedule_expression.
Schedule_expression should support at, cron and rate.

Changes

Patched the create_schedule method in order to validate schedule_expression .
Reused some REGEX rules from events service.

@anisaoshafi anisaoshafi requested a review from bentsku January 27, 2025 10:30
@anisaoshafi anisaoshafi self-assigned this Jan 27, 2025
@anisaoshafi anisaoshafi added semver: patch Non-breaking changes which can be included in patch releases aws:scheduler Amazon EventBridge Scheduler labels Jan 27, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Jan 27, 2025 •
edited
Loading

LocalStack Community integration with Pro

 2 files  ±    0   2 suites  ±0   21s ⏱️ - 1h 51m 20s
22 tests  - 4 000  20 ✅  - 3 686  2 💤  - 314  0 ❌ ±0 
24 runs   - 4 000  20 ✅  - 3 686  4 💤  - 314  0 ❌ ±0 

Results for commit 6c02cc2. ± Comparison against base commit b0627a1.

This pull request removes 4017 and adds 17 tests. Note that renamed tests count towards both. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[ rate(10 minutes)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[at(2021-12-31)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[at(2021-12-31T23:59:59Z)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron()]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron(0 1 * * * *)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron(0 dummy ? * MON-FRI *)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron(7 20 * * NOT *)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron(71 8 1 * ? *)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[cron(INVALID)]
tests.aws.services.scheduler.test_scheduler ‑ tests_create_schedule_with_invalid_schedule_expression[rate( 10 minutes )]
…

♻️ This comment has been updated with latest results.

@anisaoshafi anisaoshafi added this to the 4.1 milestone Jan 27, 2025
bentsku
bentsku reviewed Jan 27, 2025
View reviewed changes
Copy link
Contributor
@bentsku bentsku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice port of your previous fix 👌 nice usage of the patch function.

I only have a comment, it seems there code that cannot be executed, and that path is also untested. Once this is addressed, I believe the PR is ready to be merged! 🚀

tests/aws/services/scheduler/test_scheduler.py
"at(2021-12-31)",
],
)
def tests_create_schedule_with_invalid_schedule_expression(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice test! 👌

@anisaoshafi anisaoshafi requested a review from bentsku January 27, 2025 14:58
bentsku
bentsku reviewed Jan 27, 2025
View reviewed changes
tests/aws/services/scheduler/test_scheduler.py Outdated
Comment on lines 135 to 148
with pytest.raises(ParamValidationError) as e:
aws_client.scheduler.create_schedule(
Name=rule_name,
ScheduleExpression="",
FlexibleTimeWindow={
"MaximumWindowInMinutes": 4,
"Mode": "FLEXIBLE",
},
Target={
"Arn": f"arn:aws:lambda:{region_name}:{account_id}:function:dummy",
"RoleArn": f"arn:aws:iam::{account_id}:role/role-name",
},
)
snapshot.match("empty-schedule-expression", e.value)
Copy link
Contributor
@bentsku bentsku Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ParamValidationError indicates that the error is coming from the client itself, and the request is not sent to AWS or LocalStack. We don't need to snapshot those kind of exception as they are dependent on the client only.

If the exception is of type ParamValidationError, we should not snapshot it.

To configure the client to not have validation, and send the invalid request against AWS, you can configure you client this way:

from botocore.client import Config

def tests_create_schedule_with_empty_schedule_expression(
    aws_client_factory, region_name, account_id, snapshot
):
    scheduler_client = aws_client_factory(config=Config(parameter_validation=False)).scheduler
    rule_name = f"rule-{short_uid()}"

   with pytest.raises(ClientError) as e:
        scheduler_client.create_schedule(
        [...]

This way, the client won't validate the request and will let you send the invalid request against AWS, and see the proper exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

understood, thanks for sharing this. I'll keep it in mind for the future and will proceed removing these two dummy tests.

@anisaoshafi anisaoshafi force-pushed the scheduler-cron-validate branch from 3cc3702 to 6c02cc2 Compare January 27, 2025 15:10
@anisaoshafi anisaoshafi requested a review from bentsku January 27, 2025 15:10
@anisaoshafi
Copy link
Contributor Author

LGTM! Nice port of your previous fix 👌 nice usage of the patch function.

I only have a comment, it seems there code that cannot be executed, and that path is also untested. Once this is addressed, I believe the PR is ready to be merged! 🚀

@bentsku Thanks for your comments. Could you please double check this when you have a moment? Thanks 🙌🏼

bentsku
bentsku approved these changes Jan 27, 2025
View reviewed changes
Copy link
Contributor
@bentsku bentsku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for addressing the comments.

I think we could have kept the tests when the schedule_expression was missing, but with the client parameter validation disabled to see what AWS returned, however it's not fully in the scope of the issue, so that's alright, it was just a nice to have as you had already added it 😄 and it's pretty niche as most of the SDK will validate the input.

Anyway, very nice fix and parametrized tests! 🚀💯

@anisaoshafi
Copy link
Contributor Author

LGTM! Thanks for addressing the comments.

I think we could have kept the tests when the schedule_expression was missing, but with the client parameter validation disabled to see what AWS returned, however it's not fully in the scope of the issue, so that's alright, it was just a nice to have as you had already added it 😄 and it's pretty niche as most of the SDK will validate the input.

Anyway, very nice fix and parametrized tests! 🚀💯

Sorry I deleted everything. I'll merge this as is and if I come back to eventbridge might add that test 🙏🏼

@bentsku
Copy link
Contributor
bentsku commented Jan 27, 2025

Sorry I deleted everything. I'll merge this as is and if I come back to eventbridge might add that test 🙏🏼

No worries! You already went through the effort, that was all 😄 but this PR is good as it is, so let's get this merged! 🤝

@anisaoshafi anisaoshafi merged commit 8da6f60 into master Jan 27, 2025
31 checks passed
@anisaoshafi anisaoshafi deleted the scheduler-cron-validate branch January 27, 2025 17:40
@ridgey-dev
Copy link
ridgey-dev commented Jan 30, 2025 •
edited
Loading

I now get the error:

Invalid Schedule Expression at(2025-01-31T15:59:42)

with value:

at(2025-01-31T15:59:42)

Looking at AWS documentation https://docs.aws.amazon.com/scheduler/latest/UserGuide/schedule-types.html#one-time it's valid, but looking at the regex in the PR:

^at[(](0[1-9]|1\d|2[0-8]|29(?=-\d\d-(?!1[01345789]00|2[1235679]00)\d\d(?:[02468][048]|[13579][26]))|30(?!-02)|31(?=-0[13578]|-1[02]))-(0[1-9]|1[0-2])-([12]\d{3}) ([01]\d|2[0-3]):([0-5]\d):([0-5]\d)[)]$

it's expecting d-m-Y instead of Y-m-d?

@anisaoshafi
Copy link
Contributor Author

Hi @ridgey-dev. Thanks for reporting this 🙏🏼
I double checked with aws and the date you passed seems valid.

it's expecting d-m-Y instead of Y-m-d?

No, it's "Y-m-d", I'll fix the validation rule since I closed it too much.

@ridgey-dev
Copy link

Thanks for your quick answer. Looking forward to your fix! Our pipeline that runs the tests on LocalStack is now failing because of this.

@anisaoshafi anisaoshafi mentioned this pull request Feb 3, 2025
Merged
@anisaoshafi
Copy link
Contributor Author

Hi @ridgey-dev, thank you for your patience. We have implemented changes that should resolve your reported issue. Kindly update to the latest LocalStack container image and let us know if your issue has been resolved.

@ridgey-dev
Copy link

It's working again. Thanks for your quick fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bentsku bentsku bentsku approved these changes

@zaingz zaingz Awaiting requested review from zaingz

@joe4dev joe4dev Awaiting requested review from joe4dev

Assignees

@anisaoshafi anisaoshafi

Labels
aws:scheduler Amazon EventBridge Scheduler semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
3 participants
@anisaoshafi @bentsku @ridgey-dev
0