Authentication with AAD tokens in Databricks provider #19335

alexott · 2021-10-30T18:16:22Z

Many organizations don't allow to use personal access tokens, and instead force to use native platform authentication. This PR adds the possibility to authenticate to Azure Databricks workspaces using the Azure Active Directory tokens generated from Azure Service Princpal's ID and secret. It supports authentication when SP is either user inside workspace or it's outside of the workspace

boring-cyborg · 2021-10-30T18:16:25Z

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 Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

Pay attention to the quality of your code (flake8, 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.
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

Many organizations don't allow to use personal access tokens, and instead force to use native platform authentication. This PR adds the possibility to authenticate to Azure Databricks workspaces using the Azure Active Directory tokens generated from Azure Service Principal's ID and secret.

alexott · 2021-11-06T16:18:34Z

@mik-laj @vikramkoka would it possible to review this? I don't understand why Static check tests are failing

setup.py

freget · 2021-11-06T21:56:43Z

docs/apache-airflow-providers-databricks/connections/databricks.rst


    * ``token``: Specify PAT to use.

+    Following parameters are necessary if using authentication with AAD token:
+
+    * ``azure_client_id``: ID of the Azure Service Principal


Why don't we put the client_id and client_secret into the user and password fields of the connection? Ultimately, it's just a user and a password.

freget · 2021-11-06T22:02:10Z

airflow/providers/databricks/hooks/databricks.py

+        elif (
+            'azure_client_id' in self.databricks_conn.extra_dejson
+            and 'azure_client_secret' in self.databricks_conn.extra_dejson
+            and 'azure_tenant_id' in self.databricks_conn.extra_dejson


Enforcing all three values to be present may result in unexpected behavior. Maybe we should raise an Exception if at least one, but not all three azure_* configs are set?

good idea, although isn't required when we change to login/password

freget · 2021-11-06T22:05:46Z

airflow/providers/databricks/hooks/databricks.py

+            self.log.info('Using AAD Token for SPN. ')
+
+            if 'host' in self.databricks_conn.extra_dejson:
+                host = self._parse_host(self.databricks_conn.extra_dejson['host'])


To be honest I did not understand why host is allowed as an extra variable for token authentication - I suspected something about backward compatibility. For user / password authentication, host may only be submitted as a normal connection argument. I would prefer to do it the same way with SP based authentication.

yeah, looks like more for compatibility...

potiuk · 2021-11-07T12:08:47Z

airflow/providers/databricks/hooks/databricks.py

+                        "resource": resource,
+                    }
+                    resp = requests.get(
+                        "http://169.254.169.254/metadata/identity/oauth2/token",


Why fixed IP address here? This sounds very wrong

I understanda this is azrure's metadata server? But is there no better way to reach the metadata server (and should we only limit it if we check in the environment that we are running on Azure managed vm ?

For example in Google's VM you can use metadata.google.internal name https://cloud.google.com/compute/docs/metadata/overview

it's a fixed address for internal metadata service - see the linked documentation

Yeah - but see my comment above. At the very least (if we cannot get symbolic name) this address should be extracte as "AZURE_METADATA_IP" for example and explained in comments, without the need of reading the docs actually.

And we should likely check if we are in Azure VM (and fail if we aren't) without even reaching out to the metadata server. Not really necessary but It would be nice to check it (via env vars I guess) before - otherwise you might get strange errors when you enable it by mistake on non-azure managed-identity server. Google's meta-data servers I think have the same IP address, so the responses from it might be confusing.

See for example the comment here (accepted answer is to use metadata server but the comment is about long timeouts and confusing behaviour on non-azure instances): https://stackoverflow.com/questions/54913218/detect-whether-running-on-azure-iaas-vm-net-application

potiuk · 2021-11-07T15:28:58Z

@mik-laj @vikramkoka would it possible to review this? I don't understand why Static check tests are failing

tests/providers/databricks/hooks/test_databricks.py:24:1: F401 'unittest.mock.call' imported but unused

potiuk · 2021-11-07T15:29:45Z

I recommend to install pre-commit - this way you can run all the static checks locally before sending them to CI

also removed dependency on the azure-identity

potiuk

I do not know too much abot AAD in Databricks, but it looks reasonable !

github-actions · 2021-11-08T09:23:02Z

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

potiuk · 2021-11-08T09:23:14Z

@freget - wdyt ?

freget

LGTM

alexott requested review from mik-laj and vikramkoka as code owners October 30, 2021 18:16

boring-cyborg bot added area:providers kind:documentation labels Oct 30, 2021

alexott force-pushed the databricks-support-aad-token branch from 7511075 to 611ef98 Compare October 31, 2021 10:22

alexott changed the title ~~New functionality: Authentication with AAD tokens in Databricks provider~~ [DRAFT] New functionality: Authentication with AAD tokens in Databricks provider Oct 31, 2021

alexott changed the title ~~[DRAFT] New functionality: Authentication with AAD tokens in Databricks provider~~ New functionality: Authentication with AAD tokens in Databricks provider Nov 1, 2021

alexott added 2 commits November 6, 2021 12:50

Add unit test for AAD authentication

f1ec0c2

alexott mentioned this pull request Nov 6, 2021

Databricks Provider: Allow authentication via Azure Service Principals #19433

Closed

alexott changed the title ~~New functionality: Authentication with AAD tokens in Databricks provider~~ Authentication with AAD tokens in Databricks provider Nov 6, 2021

freget reviewed Nov 6, 2021

View reviewed changes

Added support for SP being outside of the workspace

5fe4bf0

alexott force-pushed the databricks-support-aad-token branch 2 times, most recently from 106f07e to 55279fe Compare November 7, 2021 10:15

potiuk reviewed Nov 7, 2021

View reviewed changes

Added support for Azure managed identity

b03306c

also removed dependency on the azure-identity

alexott force-pushed the databricks-support-aad-token branch from 55279fe to b03306c Compare November 7, 2021 18:04

potiuk approved these changes Nov 8, 2021

View reviewed changes

github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 8, 2021

freget approved these changes Nov 8, 2021

View reviewed changes

potiuk merged commit 28b51fb into apache:main Nov 8, 2021

This was referenced Nov 30, 2021

Status of testing Providers that were prepared on November 30, 2021 #19895

Closed

Status of testing Providers that were prepared on December 07, 2021 #20097

Closed

potiuk mentioned this pull request Dec 11, 2021

Status of testing Providers that were prepared on December 11, 2021 #20220

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authentication with AAD tokens in Databricks provider #19335

Authentication with AAD tokens in Databricks provider #19335

alexott commented Oct 30, 2021 •

edited

Loading

boring-cyborg bot commented Oct 30, 2021

alexott commented Nov 6, 2021

freget Nov 6, 2021

alexott Nov 7, 2021

freget Nov 6, 2021

alexott Nov 7, 2021 •

edited

Loading

freget Nov 6, 2021

alexott Nov 7, 2021

potiuk Nov 7, 2021

potiuk Nov 7, 2021

alexott Nov 7, 2021

potiuk Nov 7, 2021 •

edited

Loading

potiuk Nov 7, 2021

potiuk Nov 7, 2021

potiuk commented Nov 7, 2021

potiuk commented Nov 7, 2021

potiuk left a comment

github-actions bot commented Nov 8, 2021

potiuk commented Nov 8, 2021

freget left a comment

Authentication with AAD tokens in Databricks provider #19335

Authentication with AAD tokens in Databricks provider #19335

Conversation

alexott commented Oct 30, 2021 • edited Loading

boring-cyborg bot commented Oct 30, 2021

alexott commented Nov 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexott Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk Nov 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potiuk commented Nov 7, 2021

potiuk commented Nov 7, 2021

potiuk left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 8, 2021

potiuk commented Nov 8, 2021

freget left a comment

Choose a reason for hiding this comment

alexott commented Oct 30, 2021 •

edited

Loading

alexott Nov 7, 2021 •

edited

Loading

potiuk Nov 7, 2021 •

edited

Loading