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

Deprecated kerberos auth removed #41693

Merged
merged 4 commits into from
Sep 17, 2024
Merged

Deprecated kerberos auth removed #41693

merged 4 commits into from
Sep 17, 2024

Conversation

dirrao
Copy link
Contributor

@dirrao dirrao commented Aug 23, 2024

The following deprecated kerberos auth removed

  1. airflow.auth.managers.fab.api.auth.backend.kerberos_auth
  2. airflow.api.auth.backend.kerberos_auth

@dirrao
deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and ai…
6036212 
…rflow.auth.managers.fab.api.auth.backend.kerberos_aut removed
@dirrao dirrao added airflow3.0:candidate Potential candidates for Airflow 3.0 airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes labels Aug 23, 2024
@dirrao dirrao requested review from uranusjr and potiuk August 23, 2024 10:30
@dirrao
deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and ai…
8cda684 
…rflow.auth.managers.fab.api.auth.backend.kerberos_aut removed
eladkal
eladkal previously requested changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

Breaking change must have news fragment to notify users what was removed and how to mitigate. In this case just alerting about module remove and how to set it up with fab provider

@dirrao
Copy link
Contributor Author

dirrao commented Aug 24, 2024

Breaking change must have news fragment to notify users what was removed and how to mitigate. In this case just alerting about module remove and how to set it up with fab provider

I have already added the news fragment. Am I missing anything?

@dirrao dirrao requested a review from eladkal August 27, 2024 04:26
@potiuk
Copy link
Member

potiuk commented Aug 27, 2024

@eladkal ?

@jscheffl
Copy link
Contributor

Sorry, need to be another bad guy here. We said for Airflow 3 dev rules we must separate PRs for providers and stuff for the core. Assume we need to split this here as well. (1) for provider update and (2) for removing/breaking change in core.

(whereas I assume we don't plan to back-port to 2.10 so technically could be one PR?)

@dirrao
Copy link
Contributor Author

dirrao commented Aug 28, 2024 •
edited
Loading

@eladkal
Let me know if you want me to separate core and providers.

@dirrao
Copy link
Contributor Author

dirrao commented Sep 2, 2024

@eladkal / @jscheffl
Do you want me to create separate PR for provider changes?

@vincbeck
Copy link
Contributor

vincbeck commented Sep 4, 2024

Yep I think it is needed. But we might actually want only core changes, breaking change in providers will result in creating major versions which we might not want yet. I'll let @eladkal confirm

@vincbeck
Copy link
Contributor

vincbeck commented Sep 6, 2024

@dirrao Could you please update this PR (or create another one) to include only changes from core?

@dirrao
Copy link
Contributor Author

dirrao commented Sep 7, 2024 •
edited
Loading

@vincbeck / @jscheffl
Provider changes are dependent on the core. Not sure how to support the backward compatibility. Please take a look at the provider dependency. Do you want me to add if else conditional based logic?

@vincbeck
Copy link
Contributor

vincbeck commented Sep 9, 2024 •
edited
Loading

Actually I think your changes are fine. It seems inevitable to force Airflow 3 to use the latest FAB version provider. See comment here. If that is the case your changes are not breaking for providers. You back ported airflow/api/auth/backend/kerberos_auth.py to airflow/providers/fab/auth_manager/api/auth/backend/kerberos_auth.py (am I right?) so FAB provider no longer depend on core Airflow for Kerberos auth (which is good). The only issue would be to use Airflow 3 with current version of FAB provider or earlier but, as mentioned, if this is something that will not be possible, I think these changes are fine. In this sense, these changes are not breaking for FAB provider.

@dirrao
Copy link
Contributor Author

dirrao commented Sep 10, 2024

Actually I think your changes are fine. It seems inevitable to force Airflow 3 to use the latest FAB version provider. See comment here. If that is the case your changes are not breaking for providers. You back ported airflow/api/auth/backend/kerberos_auth.py to airflow/providers/fab/auth_manager/api/auth/backend/kerberos_auth.py (am I right?) so FAB provider no longer depend on core Airflow for Kerberos auth (which is good). The only issue would be to use Airflow 3 with current version of FAB provider or earlier but, as mentioned, if this is something that will not be possible, I think these changes are fine. In this sense, these changes are not breaking for FAB provider.

Yes, my changes are backward compatible. They will simply start using the FAB provider module instead of the deprecated core auth module.

@dirrao
Copy link
Contributor Author

dirrao commented Sep 10, 2024

@potiuk / @eladkal
I'm awaiting final approval to proceed with the merge. Could you please take a look when you get a chance?

@dirrao dirrao requested a review from potiuk September 16, 2024 03:54
eladkal
eladkal reviewed Sep 16, 2024
View reviewed changes
Copy link
Contributor

@eladkal eladkal left a comment

Choose a reason for hiding this comment

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

The commit message says removed deprecation but this PR also adds more functionality to the provider. Is the commit message right?

@vincbeck
Copy link
Contributor

The commit message says removed deprecation but this PR also adds more functionality to the provider. Is the commit message right?

The only code added to the provider is code moved from core to provider. There is no new functionality. I think the message is fair

@dirrao
Copy link
Contributor Author

dirrao commented Sep 17, 2024

The commit message says removed deprecation but this PR also adds more functionality to the provider. Is the commit message right?

The only code added to the provider is code moved from core to provider. There is no new functionality. I think the message is fair

Yes. No new functionality added.

@eladkal
Copy link
Contributor

eladkal commented Sep 17, 2024

The only code added to the provider is code moved from core to provider. There is no new functionality. I think the message is fair

ok then I will modify the provider change log

@eladkal eladkal merged commit db7f927 into apache:main Sep 17, 2024
55 of 56 checks passed
gopidesupavan pushed a commit to gopidesupavan/airflow that referenced this pull request Sep 17, 2024
@dirrao @gopidesupavan
Deprecated kerberos auth removed (apache#41693)
8149359 
* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed

* news fragment added

* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed
@eladkal eladkal mentioned this pull request Sep 21, 2024
Closed
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
@dirrao @joaopamaral
Deprecated kerberos auth removed (apache#41693)
16f8170 
* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed

* news fragment added

* deprecatd kerberos auth airflow.api.auth.backend.kerberos_auth and airflow.auth.managers.fab.api.auth.backend.kerberos_aut removed
@Lee-W Lee-W mentioned this pull request Nov 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladkal eladkal eladkal approved these changes

@vincbeck vincbeck vincbeck approved these changes

@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner

@pierrejeambrun pierrejeambrun Awaiting requested review from pierrejeambrun pierrejeambrun is a code owner

@uranusjr uranusjr Awaiting requested review from uranusjr

@potiuk potiuk Awaiting requested review from potiuk

Assignees
No one assigned
Labels
airflow3.0:breaking Candidates for Airflow 3.0 that contain breaking changes airflow3.0:candidate Potential candidates for Airflow 3.0 area:API Airflow's REST/HTTP API area:providers kind:documentation provider:fab
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dirrao @potiuk @jscheffl @vincbeck @eladkal