You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/07/09 18:32:15 UTC
[GitHub] [airflow] feluelle opened a new pull request #9747: Add AzureBaseHook
feluelle opened a new pull request #9747:
URL: https://github.com/apache/airflow/pull/9747
- refactor/change azure_container_instance to use AzureBaseHook
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452683287
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
+ if key_path:
+ if not key_path.endswith('.json'):
+ raise AirflowException('Unrecognised extension for key file.')
+ self.log.info('Getting connection using a JSON key file.')
+ return get_client_from_auth_file(
+ client_class=self.sdk_client,
+ auth_path=key_path
+ )
+
+ self.log.info('Getting connection using specific credentials and subscription_id.')
+ return self.sdk_client(
+ credentials=ServicePrincipalCredentials(
+ client_id=conn.login,
+ secret=conn.password,
+ tenant=conn.extra_dejson['tenantId']
Review comment:
Yeah (1) sounds better
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-656594728
Ready for another round @mik-laj @turbaszek @kaxil
I also added howto docs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r453293076
##########
File path: docs/howto/connection/azure.rst
##########
@@ -0,0 +1,72 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+
+
+.. _howto/connection:azure:
+
+Microsoft Azure Connection
+==========================
+
+The Microsoft Azure connection type enables the :ref:`Azure Integrations <Azure>`.
+
+Authenticating to Azure
+-----------------------
+
+There are three ways to connect to Azure using Airflow.
+
+1. Use `token credentials
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-token-credentials>`_
+ i.e. add specific credentials (client_id, secret, tenant) and subscription id to the Airflow connection.
+2. Use a `JSON file
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-a-json-file>`_
+ i.e. create a key file on disk and link to it in the Airflow connection.
+3. Use a `JSON dictionary
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-a-json-dictionary>`_
+ i.e. add a key config directly into the Airflow connection.
+
+Only one authorization method can be used at a time. If you need to manage multiple credentials or keys then you should
+configure multiple connections.
+
+Default Connection IDs
+----------------------
+
+Some hooks and operators related to Microsoft Azure use ``azure_default`` by default.
+
+Configuring the Connection
+--------------------------
+
+Login (optional)
+ Specify the ``client_id`` used for the initial connection.
+ This is only needed for *token credentials* authentication mechanism.
+
+Password (optional)
+ Specify the ``secret`` used for the initial connection.
+ This is only needed for *token credentials* authentication mechanism.
+
+Extra (optional)
+ Specify the extra parameters (as json dictionary) that can be used in Azure connection.
+ The following parameters are all optional:
+
+ * ``tenantId``: Specify the tenant to use.
+ This is only needed for *token credentials* authentication mechanism.
+ * ``subscriptionId``: Specify the subscription id to use.
+ This is only needed for *token credentials* authentication mechanism.
+ * ``key_path``: If set, it uses the *JSON file* authentication mechanism.
+ It specifies the path to the json file that contains the authentication information.
+ * ``key_json``: If set, it uses the *JSON dictionary* authentication mechanism.
+ It specifies the json that contains the authentication information. See
Review comment:
Thanks @mik-laj I am gonna add an example URI 😊
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle merged pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle merged pull request #9747:
URL: https://github.com/apache/airflow/pull/9747
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-666226149
No code changes - just a rebase to see if everything 🟢 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658052349
I mean if the documentation is specific enough to tell you how to set the values for login password extra I personally think we don't(/should) add another custom fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658644934
> > > I mean if the documentation is specific enough to tell you how to set the values for login password extra I personally think we shouldn't add custom fields.
> >
> >
> > The extra fields and custom fields are the same. GCP adds its own fields, but this is only a visual matter. All information is. still in extra field, because they don't match any existing fields.
>
> Okay Kamil, I decided to not add this custom ui fields for now, if that's okay?!
Yeah that should be fine as long as people know what to put where
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-678136175
Yes go for it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle edited a comment on pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle edited a comment on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658050653
What do you think of having a custom connection type `azure` with custom fields like `gcp` has **vs** the use of general fields (login, password, extra)? @kaxil @turbaszek @mik-laj
Is it always better* to add a custom type for a new connection?
*i.e. better to understand, ...
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-677574912
@feluelle Do you need any help?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452678758
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
+ if key_path:
+ if not key_path.endswith('.json'):
+ raise AirflowException('Unrecognised extension for key file.')
+ self.log.info('Getting connection using a JSON key file.')
+ return get_client_from_auth_file(
+ client_class=self.sdk_client,
+ auth_path=key_path
+ )
+
+ self.log.info('Getting connection using specific credentials and subscription_id.')
+ return self.sdk_client(
+ credentials=ServicePrincipalCredentials(
+ client_id=conn.login,
+ secret=conn.password,
+ tenant=conn.extra_dejson['tenantId']
Review comment:
1.)
Or just do this:
```suggestion
tenant=conn.extra_dejson.get('tenantId')
```
so azure raises the error which will probably be more expressive?
2.)
Simply validate and raise Exception that the field is missing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452681205
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
By the way..
```suggestion
key_path = conn.extra_dejson.get('key_path')
```
works also, because azure will check the env variable, too.
> auth_path = auth_path or os.environ['AZURE_AUTH_LOCATION']
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452677041
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
+ if key_path:
+ if not key_path.endswith('.json'):
+ raise AirflowException('Unrecognised extension for key file.')
+ self.log.info('Getting connection using a JSON key file.')
+ return get_client_from_auth_file(
+ client_class=self.sdk_client,
+ auth_path=key_path
+ )
+
+ self.log.info('Getting connection using specific credentials and subscription_id.')
+ return self.sdk_client(
+ credentials=ServicePrincipalCredentials(
+ client_id=conn.login,
+ secret=conn.password,
+ tenant=conn.extra_dejson['tenantId']
Review comment:
Should we validate that `tenantId` and `subscriptionId` are set or else this will result in a KeyError
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452449888
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
Should we support [key as json_dict](https://docs.microsoft.com/en-us/python/api/azure-common/azure.common.client_factory?view=azure-python#get-client-from-json-dict-client-class--config-dict----kwargs-)? This will facilitate use with secret backends.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658050653
What do you think of having a custom connection type `azure` with custom fields like `gcp` has **vs** the use of general fields (login, password, extra)? @kaxil @turbaszek @mik-laj
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r453932386
##########
File path: docs/howto/connection/azure.rst
##########
@@ -0,0 +1,72 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
Review comment:
Can you add this file to `.github/boring-cyborg.yml`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658637431
> > I mean if the documentation is specific enough to tell you how to set the values for login password extra I personally think we shouldn't add custom fields.
>
> The extra fields and custom fields are the same. GCP adds its own fields, but this is only a visual matter. All information is. still in extra field, because they don't match any existing fields.
Okay Kamil, I decided to not add this custom ui fields for now, if that's okay?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452665684
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
Makes sense - will add that. 👍
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658304397
> I mean if the documentation is specific enough to tell you how to set the values for login password extra I personally think we shouldn't add custom fields.
The extra fields and custom fields are the same. GCP adds its own fields, but this is only a visual matter. All information is. still in extra field, because they don't match any existing fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452682273
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
So I am removing also the test case because the functionality is being provided (and should be tested) by azure.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452682484
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
+ if key_path:
+ if not key_path.endswith('.json'):
+ raise AirflowException('Unrecognised extension for key file.')
+ self.log.info('Getting connection using a JSON key file.')
+ return get_client_from_auth_file(
+ client_class=self.sdk_client,
+ auth_path=key_path
+ )
+
+ self.log.info('Getting connection using specific credentials and subscription_id.')
+ return self.sdk_client(
+ credentials=ServicePrincipalCredentials(
+ client_id=conn.login,
+ secret=conn.password,
+ tenant=conn.extra_dejson['tenantId']
Review comment:
I'm in favor of 1) this reduce the number of logic on our side
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-666226149
No code changes - just a rebase to see if everything 🟢 :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452754126
##########
File path: docs/howto/connection/azure.rst
##########
@@ -0,0 +1,72 @@
+ .. Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ .. http://www.apache.org/licenses/LICENSE-2.0
+
+ .. Unless required by applicable law or agreed to in writing,
+ software distributed under the License is distributed on an
+ "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ KIND, either express or implied. See the License for the
+ specific language governing permissions and limitations
+ under the License.
+
+
+
+.. _howto/connection:azure:
+
+Microsoft Azure Connection
+==========================
+
+The Microsoft Azure connection type enables the :ref:`Azure Integrations <Azure>`.
+
+Authenticating to Azure
+-----------------------
+
+There are three ways to connect to Azure using Airflow.
+
+1. Use `token credentials
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-token-credentials>`_
+ i.e. add specific credentials (client_id, secret, tenant) and subscription id to the Airflow connection.
+2. Use a `JSON file
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-a-json-file>`_
+ i.e. create a key file on disk and link to it in the Airflow connection.
+3. Use a `JSON dictionary
+ <https://docs.microsoft.com/en-us/azure/developer/python/azure-sdk-authenticate?tabs=cmd#authenticate-with-a-json-dictionary>`_
+ i.e. add a key config directly into the Airflow connection.
+
+Only one authorization method can be used at a time. If you need to manage multiple credentials or keys then you should
+configure multiple connections.
+
+Default Connection IDs
+----------------------
+
+Some hooks and operators related to Microsoft Azure use ``azure_default`` by default.
+
+Configuring the Connection
+--------------------------
+
+Login (optional)
+ Specify the ``client_id`` used for the initial connection.
+ This is only needed for *token credentials* authentication mechanism.
+
+Password (optional)
+ Specify the ``secret`` used for the initial connection.
+ This is only needed for *token credentials* authentication mechanism.
+
+Extra (optional)
+ Specify the extra parameters (as json dictionary) that can be used in Azure connection.
+ The following parameters are all optional:
+
+ * ``tenantId``: Specify the tenant to use.
+ This is only needed for *token credentials* authentication mechanism.
+ * ``subscriptionId``: Specify the subscription id to use.
+ This is only needed for *token credentials* authentication mechanism.
+ * ``key_path``: If set, it uses the *JSON file* authentication mechanism.
+ It specifies the path to the json file that contains the authentication information.
+ * ``key_json``: If set, it uses the *JSON dictionary* authentication mechanism.
+ It specifies the json that contains the authentication information. See
Review comment:
Can you add example URI? https://airflow.readthedocs.io/en/latest/howto/connection/gcp.html
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-678135511
Yes. You have the permission to merge changes, so nobody wanted to take this opportunity away from you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle commented on pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle commented on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-678082619
@mik-laj No, I think this can be merged. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452449888
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
Should we support key as dict? This will facilitate use with secret backends.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] feluelle edited a comment on pull request #9747: [WIP] Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
feluelle edited a comment on pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#issuecomment-658052349
I mean if the documentation is specific enough to tell you how to set the values for login password extra I personally think we shouldn't add custom fields.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #9747: Add AzureBaseHook
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #9747:
URL: https://github.com/apache/airflow/pull/9747#discussion_r452676309
##########
File path: airflow/providers/microsoft/azure/hooks/base_azure.py
##########
@@ -0,0 +1,68 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+from os import getenv
+from typing import Any
+
+from azure.common.client_factory import get_client_from_auth_file
+from azure.common.credentials import ServicePrincipalCredentials
+
+from airflow import AirflowException
+from airflow.hooks.base_hook import BaseHook
+
+
+class AzureBaseHook(BaseHook):
+ """
+ This hook acts as a base hook for azure services. It offers several authentication mechanisms to
+ authenticate the client library used for upstream azure hooks.
+
+ :param sdk_client: The SDKClient to use.
+ :param conn_id: The azure connection id which refers to the information to connect to the service.
+ """
+
+ def __init__(self, sdk_client: Any, conn_id: str = 'azure_default'):
+ self.sdk_client = sdk_client
+ self.conn_id = conn_id
+ super().__init__()
+
+ def get_conn(self) -> Any:
+ """
+ Authenticates the resource using the connection id passed during init.
+
+ :return: the authenticated client.
+ """
+ conn = self.get_connection(self.conn_id)
+
+ key_path = conn.extra_dejson.get('key_path') or getenv('AZURE_AUTH_LOCATION')
Review comment:
good point
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org