Skip to content

Add kerberos related connection fields(principal, keytab) on SparkSubmitHook #40757

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
Merged
romsharon98 merged 34 commits into from
Sep 17, 2024
Merged

Add kerberos related connection fields(principal, keytab) on SparkSubmitHook #40757

romsharon98 merged 34 commits into from
Sep 17, 2024

Conversation

@seyoon-lim
Copy link
Contributor

@seyoon-lim seyoon-lim commented Jul 12, 2024

closes: #40749

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

@boring-cyborg
Copy link

boring-cyborg bot commented Jul 12, 2024

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@seyoon-lim seyoon-lim marked this pull request as draft July 12, 2024 17:11
@seyoon-lim
Copy link
Contributor Author

seyoon-lim commented Jul 12, 2024

I am currently writing the test code, and I will change the draft status once the test code is completed.

@eladkal eladkal requested a review from romsharon98 July 12, 2024 17:18
@seyoon-lim seyoon-lim force-pushed the feature/add-krb-field-on-spark-conn branch from b71bbac to afb28cb Compare July 12, 2024 17:35
@seyoon-lim seyoon-lim force-pushed the feature/add-krb-field-on-spark-conn branch from 84d8f11 to 8148d5c Compare July 12, 2024 22:26
@seyoon-lim seyoon-lim force-pushed the feature/add-krb-field-on-spark-conn branch from 3b30a60 to 51032ed Compare July 13, 2024 22:23
seyoon-lim
seyoon-lim commented Jul 14, 2024
View reviewed changes
Copy link
Contributor Author

@seyoon-lim seyoon-lim left a comment

Choose a reason for hiding this comment

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

😄

@seyoon-lim seyoon-lim marked this pull request as ready for review July 14, 2024 00:58
@eladkal
Copy link
Contributor

eladkal commented Sep 2, 2024

@seyoon-lim you need to rebase. I can't do that for you as you removed the allow mantainers edits
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork#enabling-repository-maintainer-permissions-on-existing-pull-requests

@seyoon-lim
Copy link
Contributor Author

seyoon-lim commented Sep 2, 2024

@eladkal
I'm sorry, I missed to check the 'Allow edits from maintainers' option.

I've already merged the PR with the main branch, but if necessary, please let me know and I will close and reopen the PR.

@eladkal
Copy link
Contributor

eladkal commented Sep 2, 2024

Cool. Can you address the comments left by @nevcohen ? If issues are resooved then please resolve the threads so we know it was addressed

@seyoon-lim
Copy link
Contributor Author

seyoon-lim commented Sep 2, 2024 •
edited
Loading

Cool. Can you address the comments left by @nevcohen ? If issues are resooved then please resolve the threads so we know it was addressed

@eladkal
I'm not exactly sure what changes are needed for the section in this comment, but I believe I have addressed all the other comments. Thank you for letting me know!

nevcohen
nevcohen reviewed Sep 2, 2024
View reviewed changes
Copy link
Contributor

@nevcohen nevcohen left a comment

Choose a reason for hiding this comment

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

Just a few tests, the rest looks great!

romsharon98
romsharon98 reviewed Sep 6, 2024
View reviewed changes
@eladkal eladkal requested a review from romsharon98 September 16, 2024 11:59
@seyoon-lim seyoon-lim force-pushed the feature/add-krb-field-on-spark-conn branch from 3cf300c to b4364fe Compare September 16, 2024 14:13
@romsharon98 romsharon98 merged commit 1f10532 into apache:main Sep 17, 2024
@boring-cyborg
Copy link

boring-cyborg bot commented Sep 17, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

gopidesupavan pushed a commit to gopidesupavan/airflow that referenced this pull request Sep 17, 2024
@seyoon-lim @softyoungha @gopidesupavan
Add kerberos related connection fields(principal, keytab) on SparkSub…
e7f8862 
…mitHook (apache#40757)

* Add: added kerberos related connection fields(principal, keytab)

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* fixed wrong logic since keytab priority

* fixed wrong temp_file_name logic

* Added annotations on operators/spark_submit.py

* changed raise Exception to AirflowException

* Fixed principal referencing bug

* Added checksum validating logic to do not rewrite keytab

* Fixed arg-type error

* simplified existing keytab compare logic

* Added principal test cases

* Added keytab test cases

* added principal & keytab to expected conn values

* added principal & keytab getter for the borken conneciton case

* added guard logic for the case that failed to b64decode

* removed useless logic on test code

* Mod: improved keytab_value_override test case

* changed func name _get_keytab_from_base64 to _create_keytab_path_from_base64_keytab

* added additional comment on create_keytab parts

* added base64 decode exception test case

* added keytab write & move exception test case

* removed _get_keytab & _get_principal

* rollback _get_keytab & _get_principal

* set default values of keytab & principal on conn_data

---------

Co-authored-by: Youngha, Park <32288527+softyoungha@users.noreply.github.com>
@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
@seyoon-lim @softyoungha @joaopamaral
Add kerberos related connection fields(principal, keytab) on SparkSub…
ccad418 
…mitHook (apache#40757)

* Add: added kerberos related connection fields(principal, keytab)

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* Update airflow/providers/apache/spark/hooks/spark_submit.py

* fixed wrong logic since keytab priority

* fixed wrong temp_file_name logic

* Added annotations on operators/spark_submit.py

* changed raise Exception to AirflowException

* Fixed principal referencing bug

* Added checksum validating logic to do not rewrite keytab

* Fixed arg-type error

* simplified existing keytab compare logic

* Added principal test cases

* Added keytab test cases

* added principal & keytab to expected conn values

* added principal & keytab getter for the borken conneciton case

* added guard logic for the case that failed to b64decode

* removed useless logic on test code

* Mod: improved keytab_value_override test case

* changed func name _get_keytab_from_base64 to _create_keytab_path_from_base64_keytab

* added additional comment on create_keytab parts

* added base64 decode exception test case

* added keytab write & move exception test case

* removed _get_keytab & _get_principal

* rollback _get_keytab & _get_principal

* set default values of keytab & principal on conn_data

---------

Co-authored-by: Youngha, Park <32288527+softyoungha@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@romsharon98 romsharon98 romsharon98 approved these changes

+1 more reviewer

@nevcohen nevcohen nevcohen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers provider:apache-spark

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add a keytab field to the Spark connection (handle the base64 encoded text value as a credential).

5 participants

@seyoon-lim @eladkal @romsharon98 @nevcohen @softyoungha