Skip to content
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.

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

Add flask config: MAX_CONTENT_LENGTH #36401

Merged
merged 4 commits into from Dec 30, 2023

Conversation

yo1956
Copy link
Contributor

@yo1956 yo1956 commented Dec 24, 2023

Add flask config to limit the allowed payload.
closes: #35196

I set it to limit to 1MB.
Before and after adding this config, I tried sending a string larger than 1MB from the Edit User page.
Before adding the config, it was possible to send; after adding, I confirmed that the attached error page was received.

request_entity_too_large_2023-12-24 14 35 41

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Dec 24, 2023
@yo1956 yo1956 marked this pull request as ready for review December 24, 2023 06:16
@eladkal eladkal changed the title Add flask config: MAX_CONTENT_LENGTH (#35196) Add flask config: MAX_CONTENT_LENGTH Dec 24, 2023
Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

How about triggering a DAG with input configuration? Does it require more than 1MB? If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements. For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

@yo1956
Copy link
Contributor Author

yo1956 commented Dec 24, 2023

Thank you for your comment.

How about triggering a DAG with input configuration?

Sorry, what does this mean?

Does it require more than 1MB?

I'm not really sure what would be an appropriate value. Do you have any ideas?

If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements.

Does this mean instead of hardcoding the value, we can set an item in airflow.cfg and get that value as a flask config?

For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

Certainly, I feel it's feasible. Sorry, could you please clarify what you mean by "rather than relying on the defaults"?

@dirrao
Copy link
Collaborator

dirrao commented Dec 24, 2023

Thank you for your comment.

How about triggering a DAG with input configuration?

Sorry, what does this mean?

Does it require more than 1MB?

I'm not really sure what would be an appropriate value. Do you have any ideas?

We can pass the JSON payload while triggering the DAG from the Airflow UI. JSON payload can be arbitrary length. It is much bigger. I haven't come across a use case where we pass more than 1MB. However, I am just checking.

If we move this configuration to the web server section in airflow cfg, then we can set based on our requirements.

Does this mean instead of hardcoding the value, we can set an item in airflow.cfg and get that value as a flask config?

Trying to see if can avoid hardcoding in the code.

For user profile information, is it feasible to restrict the size of form fields rather than relying on the defaults?

Certainly, I feel it's feasible. Sorry, could you please clarify what you mean by "rather than relying on the defaults"?

I mean relying on the same default value for multiple forms in the Airflow UI.

airflow/www/app.py Outdated Show resolved Hide resolved
@potiuk
Copy link
Member

potiuk commented Dec 30, 2023

@hussein-awala ?

@eladkal eladkal added this to the Airflow 2.9.0 milestone Dec 30, 2023
@eladkal eladkal added the type:improvement Changelog: Improvements label Dec 30, 2023
@potiuk potiuk modified the milestones: Airflow 2.9.0, Airflow 2.8.1 Dec 30, 2023
Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

I was checking to see if it accepts float or if we need to round the calculated value, but it looks like Flask implicitly does that.

LGTM, just a small nit, IMHO allowed_payload_size is a better name.

airflow/config_templates/config.yml Outdated Show resolved Hide resolved
airflow/www/app.py Outdated Show resolved Hide resolved
@eladkal eladkal added type:bug-fix Changelog: Bug Fixes and removed type:improvement Changelog: Improvements labels Dec 30, 2023
Copy link
Collaborator

@dirrao dirrao left a comment

Choose a reason for hiding this comment

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

LGTM

@hussein-awala hussein-awala merged commit 84063e7 into apache:main Dec 30, 2023
52 checks passed
ephraimbuddy pushed a commit that referenced this pull request Jan 11, 2024
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A long user name in "edit user" can break the webserver
5 participants