Skip to content

Cleanup middlewares #52116

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

Merged
merged 1 commit into from
Jun 25, 2025
Merged

Cleanup middlewares #52116

merged 1 commit into from
Jun 25, 2025

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Jun 23, 2025

There are currently 2 middlewares in core Airflow:

  • SimpleAllAdminMiddleware. Specific to simple auth manager. It should only be added if SAM is configured in the environment
  • FlaskExceptionsMiddleware. Specific to FAB auth manager. It should only be added if FAB auth manager is configured in the environment. However, when working on this change and testing it, I found out this middleware is not necessary. When removed, the translation of exceptions/status code between Flask and Fastapi was still working without it. I guess WSGIMiddleware handles it.

^ 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 airflow-core/newsfragments.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Jun 23, 2025
@vincbeck vincbeck force-pushed the vincbeck/sam_middleware branch from a1de74a to dabc30e Compare June 23, 2025 21:32
@vincbeck vincbeck changed the title Add SimpleAllAdminMiddleware in SAM Cleanup middlewares Jun 23, 2025
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

LGTM, tests are not happy though

@vincbeck vincbeck force-pushed the vincbeck/sam_middleware branch from dabc30e to a41e26c Compare June 25, 2025 13:11
@vincbeck
Copy link
Contributor Author

LGTM, tests are not happy though

Tests are actually right :) I was wrong in my testing (or actually understanding of the middleware), we cannot add the middleware as part of the auth manager. Though, I added a check to verify that the auth manager configured in the environment is SAM. So the intent of the PR did not change.

@vincbeck vincbeck merged commit f0c3296 into apache:main Jun 25, 2025
99 checks passed
@vincbeck vincbeck deleted the vincbeck/sam_middleware branch June 25, 2025 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:API Airflow's REST/HTTP API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants