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

Some improvements/fixes for dag_run and task_instance endpoints #34942

Merged
merged 3 commits into from Oct 16, 2023

Conversation

hussein-awala
Copy link
Member

This PR updates the required permissions for some endpoints:

  • update_dag_run_state, clear_dag_run and set_dag_run_note -> it replaces can read dag by can edit dag similar to what we have in the other endpoints (for example the one used to clear TI state)
  • post_clear_task_instances -> it replaces can read dagrun by can edit dagrun, because the endpoint accepts a parameter reset_dag_runs to reset the dagruns states, which should check for edit on dagrun resource.
  • set_task_instance_note -> it removes can read dag because it is useless, the edit permission should be sufficient

I'm not sure if we consider some of these changes as breaking changes, I can create different PRs if needed.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Oct 14, 2023
@potiuk
Copy link
Member

potiuk commented Oct 14, 2023

Oh yeah. Two days ago I noticed the same problems while reviewing #34317

See my comments here: #34317 (comment) and #34317 (comment) (and few others).

This code will go away soon (cc: @vincbeck ). But maybe it's worth to reconcile the two changes and see if all the read->edit changes are same in both (@hussein-awala and @vincbeck ?). It looks they are all the same.

And maybe it makes sense to merge this change first and #34317 afterwards to clearly separate fix from the Auth manager refactor?

And no - I think it should not be treated as a breaking change, the previous settings made really no sense (having access to task_instance while not having access to dag_run was wrong because by changing task_instance state you can impact state of the dag_run), however we should have a significant note about it.

@potiuk
Copy link
Member

potiuk commented Oct 14, 2023

(also this change can be cherry-pickable ot 2.7.3 so that's why I think it should be merged first and cherry-picked) - provisionally marked it for 2.7.3.

@hussein-awala
Copy link
Member Author

And maybe it makes sense to merge this change first and #34317 afterwards to clearly separate fix from the Auth manager refactor?

I just checked the other PR. I agree about merging this first to make the transition to the new permissions checker more clear for the users.

@vincbeck
Copy link
Contributor

Oh yeah. Two days ago I noticed the same problems while reviewing #34317

See my comments here: #34317 (comment) and #34317 (comment) (and few others).

This code will go away soon (cc: @vincbeck ). But maybe it's worth to reconcile the two changes and see if all the read->edit changes are same in both (@hussein-awala and @vincbeck ?). It looks they are all the same.

And maybe it makes sense to merge this change first and #34317 afterwards to clearly separate fix from the Auth manager refactor?

And no - I think it should not be treated as a breaking change, the previous settings made really no sense (having access to task_instance while not having access to dag_run was wrong because by changing task_instance state you can impact state of the dag_run), however we should have a significant note about it.

Yep I agree, let's merge this one first and I'll make the change in my PR as well

@potiuk potiuk merged commit 84d9940 into apache:main Oct 16, 2023
43 checks passed
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Oct 27, 2023
ephraimbuddy pushed a commit that referenced this pull request Oct 29, 2023
* Some improvements/fixes for dag_run and task_instance endpoints

* Fix tests

(cherry picked from commit 84d9940)
ephraimbuddy pushed a commit that referenced this pull request Oct 30, 2023
* Some improvements/fixes for dag_run and task_instance endpoints

* Fix tests

(cherry picked from commit 84d9940)
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 type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants