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

Create FAB provider and move FAB auth manager in it #35926

Merged
merged 49 commits into from
Dec 11, 2023

Conversation

vincbeck
Copy link
Contributor

@vincbeck vincbeck commented Nov 28, 2023

As a result of the lazy consensus, this PR creates a new provider: fab. This new provider contains (and should always only contain?) the FAB manager that is in core Airflow for now.

It basically moves files from airflow.auth.managers.fab to airflow.providers.fab.auth_manager.

What this PR does NOT include and will be done in following PRs:

  • Pre-install the FAB provider in Airflow
  • Remove flask-login as dependency in core Airflow (only some tests in core Airflow are using it now, should be pretty easy to remove)
  • Move FAB auth manager related configuration from core Airflow config to provider config
  • Move FAB auth manager related documentation from core Airflow documentation to provider documentation
  • Create documentation related to FAB auth manager

^ 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.

@potiuk
Copy link
Member

potiuk commented Dec 8, 2023

🤞

@potiuk
Copy link
Member

potiuk commented Dec 9, 2023

Wooohoooo. Compat tests passed :)

@vincbeck
Copy link
Contributor Author

The build prod image fails again with the latest changes

@vincbeck
Copy link
Contributor Author

When doing the merge, I removed this line, it must be because of that

@potiuk
Copy link
Member

potiuk commented Dec 11, 2023

Nope. It was #36168 - rebase and it should be gone (and my PR also added different behaviour when constraint generation will fail - it will fail at constraint generation time instead of when they should be used.

@vincbeck
Copy link
Contributor Author

CLI is green! (thanks @potiuk for the help!)

@vincbeck vincbeck merged commit 1eca667 into apache:main Dec 11, 2023
76 checks passed
@vincbeck vincbeck deleted the vincbeck/fab_provider branch December 11, 2023 18:38
@potiuk
Copy link
Member

potiuk commented Dec 11, 2023

Woohoooo!

@ephraimbuddy
Copy link
Contributor

I think this move should have been part of 2.8.0. Another option would be to release it as part of 2.8.1 because if there's any bug on FAB that needs fixing in 2.8, we won't be able to do it because of this move.

@potiuk
Copy link
Member

potiuk commented Dec 15, 2023

I think this move should have been part of 2.8.0. Another option would be to release it as part of 2.8.1 because if there's any bug on FAB that needs fixing in 2.8, we won't be able to do it because of this move.

Well, yes and no. I think it will be very easy to apply cherry-picks from main in those cases as this was generally just moving the code - because we do not touch this code unless there is migration to newer FAB.

@potiuk
Copy link
Member

potiuk commented Dec 15, 2023

I am happy to do those cherry-picks, if it makes things easier :)

@eladkal
Copy link
Contributor

eladkal commented Dec 15, 2023

I am happy to do those cherry-picks, if it makes things easier :)

Can this PR be cherry picked to 2.8 branch?
It changed import path in existed migration script airflow/migrations/versions/0073_2_0_0_prefix_dag_permissions.py
to import from fab provider but fab provider can be used only with airflow>=2.9.0
If we cherry pick this PR to 2.8 branch thus it will be part of 2.8.x release, won't it fail any airflow db upgrade ?

@ephraimbuddy
Copy link
Contributor

I am happy to do those cherry-picks, if it makes things easier :)

Can this PR be cherry picked to 2.8 branch? It changed import path in existed migration script airflow/migrations/versions/0073_2_0_0_prefix_dag_permissions.py to import from fab provider but fab provider can be used only with airflow>=2.9.0 If we cherry pick this PR to 2.8 branch thus it will be part of 2.8.x release, won't it fail any airflow db upgrade ?

We shouldn't add more PRs to 2.8 branch. It's late now.
And concerning future cherrypicks, it's not to cherry-pick this particular one but I'm just thinking that if there's an issue in Auth manager in 2.8.0, how do we fix it for 2.8.1 since the authmanager is now moved to the provider. That's my worry but hopefully, we won't have issues.

@potiuk
Copy link
Member

potiuk commented Dec 15, 2023

We can also revert this one and keep a PR open and merge it when we get closer to 2.9.0 - that's also an option @vincbeck ?

@vincbeck
Copy link
Contributor Author

We can just fix the bug in 2.8 branch and then cherry-pick/apply this change to main (or all the way around). I think this is the easiest. Adding this PR to 2.8 is a bit too late IMO. It will be pretty easy to fix since there was no (of very few) code changes, it is mostly code being moved around. I'll be happy to help if that happen

@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Jan 10, 2024
@ashb
Copy link
Member

ashb commented Jun 19, 2024

FYi @vincbeck @ferruzzi This broke the AIRFLOW__WEBSERVER__UPDATE_FAB_PERMS config setting (the old setting simply got ignored)

It should have been added to airflow/configuration.py#L325-L394 to automatically upgrade the config in place and issue a warning to users.

(Or if it wasn't this pr, one of the follow on ones, this was just the first one I done related to the split out)

@vincbeck
Copy link
Contributor Author

I guess the PR you mention is #36232. I was not aware of deprecated_options, pretty handy, thanks for the info. Though, when we lookup for AIRFLOW__FAB__UPDATE_FAB_PERMS, we also lookup for AIRFLOW__WEBSERVER__UPDATE_FAB_PERMS as a fallback. Thus, I am happy to add this option to deprecated_options but if something is broken today, I dont think it will resolve it

@vincbeck
Copy link
Contributor Author

#40325

@vincbeck
Copy link
Contributor Author

vincbeck commented Jun 19, 2024

Oops, #40317 is already addressing the problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AIP-56 Extensible user management area:API Airflow's REST/HTTP API area:CLI area:dev-tools area:providers type:misc/internal Changelog: Misc changes that should appear in change log use public runners Makes sure that Public runners are used even if commiters creates the PR (useful for testing)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants