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.

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

Bugfix: Webserver returns 500 for POST requests to api/dag/*/dagrun from anonymous user #36275

Merged
merged 10 commits into from Dec 23, 2023
Merged

Bugfix: Webserver returns 500 for POST requests to api/dag/*/dagrun from anonymous user #36275

merged 10 commits into from Dec 23, 2023

Conversation

pateash
Copy link
Contributor

@pateash pateash commented Dec 18, 2023 •
edited

closes: #36110
related: #36206

Description

While creating a dag_note in anonymous mode ( API auth set to backend default )

export AIRFLOW__API__AUTH_BACKENDS= airflow.api.auth.backend.default
user_id is coming as 'None' as userContext is not available.
which is invalid for an integer field in the database and causing SQL alchemy Exception.

Proposed Solution

Check if the user_id is None, we will not cast it into string type which will resolve the issue.

ephraimbuddy
ephraimbuddy previously requested changes Dec 18, 2023
View reviewed changes
Copy link
Contributor

@ephraimbuddy ephraimbuddy left a comment

Choose a reason for hiding this comment

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

You can only disable authentication for the experimental REST API not the stable REST API so this is not an issue. See: https://airflow.apache.org/docs/apache-airflow/stable/security/api.html#disable-authentication

@ephraimbuddy ephraimbuddy dismissed their stale review December 18, 2023 07:14

Oh, my bad, this is for the experimental REST API

@pateash pateash marked this pull request as ready for review December 18, 2023 07:19
hussein-awala
hussein-awala reviewed Dec 18, 2023
View reviewed changes
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.

Could you add a test for this change?

@pateash
Copy link
Contributor Author

pateash commented Dec 18, 2023 •
edited

Could you add a test for this change?

@hussein-awala
I have tried, but not able to create a client without any auth.
I have added a test, which is utilizing Anonymous user, but still getting 401 (Unauthorized),
might need some help in getting this correct, Any experts on Testing Airflow APIs @eladkal @ephraimbuddy

@pateash pateash force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch from 97fb3d8 to 63b2253 Compare December 18, 2023 10:42
hussein-awala
hussein-awala approved these changes Dec 18, 2023
View reviewed changes
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.

LGTM

tests/api_connexion/endpoints/test_dag_run_endpoint.py Show resolved Hide resolved
@pateash pateash force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch from 293045f to 713f503 Compare December 19, 2023 07:13
@eladkal eladkal added this to the Airflow 2.8.1 milestone Dec 19, 2023
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Dec 19, 2023
@pateash pateash force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch 2 times, most recently from b53e2bb to 5ca2ca6 Compare December 19, 2023 17:00
@pateash
Copy link
Contributor Author

pateash commented Dec 19, 2023 •
edited

one of the test started failing after the rebase ( not related to this changes )
rebasing to see if this has been fixed already.
image

@potiuk potiuk force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch from 5ca2ca6 to 837f36a Compare December 19, 2023 23:57
@potiuk
Copy link
Member

potiuk commented Dec 19, 2023

Rebased. Likely some intermittent issue or smth connected with FAB changes being merged.

@pateash pateash force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch from 837f36a to 74dd475 Compare December 20, 2023 06:09
@potiuk
Copy link
Member

potiuk commented Dec 21, 2023

Nope. Looks like real test failure that you need to fix - likely coming from recent changes in FAB structure.

@hussein-awala
Copy link
Member

I think the change I pushed is not isolated, and it impacts the application context for other tests, I will take a second look this evening.

@pateash pateash force-pushed the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch from 74dd475 to b6f9154 Compare December 23, 2023 15:35
@hussein-awala
Copy link
Member

I fixed it by updating the configuration after initializing the flask application: 826f4e9

@hussein-awala hussein-awala merged commit 71bc871 into apache:main Dec 23, 2023
52 checks passed
@pateash pateash deleted the 36110-webserver-returns-500-creating-dagnote-using-anonymous-user branch December 23, 2023 21:54
ephraimbuddy pushed a commit that referenced this pull request Jan 11, 2024
@pateash @ephraimbuddy
Bugfix: Webserver returns 500 for POST requests to api/dag/*/dagrun f…
5dad650 
…rom anonymous user (#36275)

* airflow#36110 -  bugfix

* return type fixed

* airflow#36110 -  bugfix

* airflow#36110 -  fixes

* airflow#36110 -  fixes

* airflow#36110 -  adding test

* airflow#36110 -  adding test

* Fix unit test

* Don't call get_id twice

* Update app configuration after initialization

---------

Co-authored-by: hussein-awala <hussein@awala.fr>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit 71bc871)
@ephraimbuddy ephraimbuddy mentioned this pull request Jan 16, 2024
Closed
68 tasks
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
@pateash @hussein-awala
@uranusjr @abhishekbhakat
Bugfix: Webserver returns 500 for POST requests to api/dag/*/dagrun f…
7af17a3 
…rom anonymous user (apache#36275)

* airflow#36110 -  bugfix

* return type fixed

* airflow#36110 -  bugfix

* airflow#36110 -  fixes

* airflow#36110 -  fixes

* airflow#36110 -  adding test

* airflow#36110 -  adding test

* Fix unit test

* Don't call get_id twice

* Update app configuration after initialization

---------

Co-authored-by: hussein-awala <hussein@awala.fr>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uranusjr uranusjr uranusjr approved these changes

@Lee-W Lee-W Lee-W approved these changes

@hussein-awala hussein-awala hussein-awala approved these changes

@eladkal eladkal eladkal approved these changes

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy

Assignees
No one assigned
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Milestone
Airflow 2.8.1
Development

Successfully merging this pull request may close these issues.

Webserver returns 500 for POST requests to api/dag/*/dagrun from anonymous user
7 participants
@pateash @potiuk @hussein-awala @uranusjr @ephraimbuddy @Lee-W @eladkal