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 2021/06/10 10:15:47 UTC
[GitHub] [airflow] ciancolo opened a new pull request #16365: Implemented Tableau Hook connection through SSL
ciancolo opened a new pull request #16365:
URL: https://github.com/apache/airflow/pull/16365
Hello all,
with the current implementation of the Tableau Hook is not possible to connect to a Tableau Server through SSL. This issue is due that the implementation of the hook does not consider the possibility to specify the certificate to verify.
With the new implementation, it is possible to specify that in extra parameter in HTTP or Tableau connection:
I also updated the tests for Tableau Hook in order to adapt them to the new implementation.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
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/main/UPDATING.md).
--
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] eladkal commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649417418
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
if I read the tableau lib code correctly it should be possible to set both verify and cert yet with this change you allow only verify.
Some like how it is used at:
https://community.tableau.com/s/question/0D54T00000F33bd/tableauserverclient-signin-with-ssl-certificate
Also is it smart to keep the default`False`? WDYT?
Edit: Also I'm a bit confused according to the library example https://github.com/tableau/server-client-python/blob/ce37a2063eddf317aaf2703aa3ebccf8053e1c8a/samples/set_http_options.py#L35:L36 the default is actually `True` so without the changes of this PR you should have `verify=True` by default. isn't 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] eladkal commented on a change in pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r650520563
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
great please ping me when you push the changes
--
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] eladkal commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649417418
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
if I read the tableau lib code correctly it should be possible to set both verify and cert yet with this change you allow only verify.
Some like how it is used at:
https://community.tableau.com/s/question/0D54T00000F33bd/tableauserverclient-signin-with-ssl-certificate
Also is it smart to keep the default`False`? WDYT?
**Edit:** Also I'm a bit confused according to the library example https://github.com/tableau/server-client-python/blob/ce37a2063eddf317aaf2703aa3ebccf8053e1c8a/samples/set_http_options.py#L35:L36 the default is actually `True` so without the changes of this PR you should have `verify=True` by default. isn't 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] github-actions[bot] commented on pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#issuecomment-877622911
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.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r654885676
##########
File path: tests/providers/tableau/hooks/test_tableau.py
##########
@@ -52,6 +52,24 @@ def setUp(self):
extra='{"token_name": "my_token", "personal_access_token": "my_personal_access_token"}',
)
)
+ db.merge_conn(
+ models.Connection(
+ conn_id='tableau_test_ssl_connection',
+ conn_type='tableau',
+ host='tableau',
+ login='user',
+ password='password',
+ extra='{"verify": "my_cert", "cert": "my_client_cert"}',
Review comment:
my_cert isn't a valid option for verify it's a bit confusing.
##########
File path: tests/providers/tableau/hooks/test_tableau.py
##########
@@ -87,6 +105,46 @@ def test_get_conn_auth_via_token_and_site_in_init(self, mock_server, mock_tablea
)
mock_server.return_value.auth.sign_out.assert_called_once_with()
+ @patch('airflow.providers.tableau.hooks.tableau.TableauAuth')
+ @patch('airflow.providers.tableau.hooks.tableau.Server')
+ def test_get_conn_ssl(self, mock_server, mock_tableau_auth):
+ """
+ Test get conn with SSL parameters
Review comment:
```suggestion
Test get conn with SSL disabled parameters
```
##########
File path: tests/providers/tableau/hooks/test_tableau.py
##########
@@ -52,6 +52,24 @@ def setUp(self):
extra='{"token_name": "my_token", "personal_access_token": "my_personal_access_token"}',
)
)
+ db.merge_conn(
+ models.Connection(
+ conn_id='tableau_test_ssl_connection',
Review comment:
```suggestion
conn_id='tableau_test_ssl_false_connection',
```
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +57,10 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', True),
Review comment:
When user set to False I'm not sure if this will give us `{'verify': False}`. I think this will be `{'verify': 'False'}` because there is no conversion of string to bool
--
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] ciancolo commented on a change in pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
ciancolo commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r666967855
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +57,10 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', True),
Review comment:
Hi @eladkal,
sorry for the late response, but I was very busy at work.
Anyway, I applied some of the changes as you suggested, in particular:
- Introduced the conversion to boolean
- Renamed some variables and comments in tests
Just a comment, the parameter `verify` in Requests module can be a boolean (True or False) or a string that contains the path to the certificate. This is necessary because Certifi, the module used to manage SSL certificates from Requests, reads (as default) only the certificates contained in file `cacert.pem` of contained in the module itself ([here ](https://github.com/certifi/python-certifi)the official docs). There are cases, for example mine, that the user wants to use the certificates installed in the OS, then he/she has to specify the path to the certificate. To better clarify the utilization of the parameter I added an extra test.
I'm currently using the first version of this PR in my Airflow installation. I tested both the default and the path case and they work, but until now I never tested the False case. I tested just before these new updates and with the Boolean conversion, it works as well.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] eladkal commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649417418
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
if I read the tableau lib code correctly it should be possible to set both verify and cert yet with this change you allow only verify.
Some like how it is used at:
https://community.tableau.com/s/question/0D54T00000F33bd/tableauserverclient-signin-with-ssl-certificate
Also why the default is `False`?
--
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] ciancolo commented on a change in pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
ciancolo commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r654549147
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
@eladkal I uploaded the requested changes. I am available for any doubts or requests.
Thank 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] eladkal commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL
Posted by GitBox <gi...@apache.org>.
eladkal commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649417418
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
if I read the tableau lib code correctly it should be possible to set both verify and cert yet with this change you allow only verify.
Some like how it is used at:
https://community.tableau.com/s/question/0D54T00000F33bd/tableauserverclient-signin-with-ssl-certificate
Also is it smart to keep the default`False`? 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] ciancolo commented on a change in pull request #16365: Implemented Tableau Hook connection through SSL
Posted by GitBox <gi...@apache.org>.
ciancolo commented on a change in pull request #16365:
URL: https://github.com/apache/airflow/pull/16365#discussion_r649751086
##########
File path: airflow/providers/tableau/hooks/tableau.py
##########
@@ -61,7 +61,9 @@ def __init__(self, site_id: Optional[str] = None, tableau_conn_id: str = default
self.tableau_conn_id = tableau_conn_id
self.conn = self.get_connection(self.tableau_conn_id)
self.site_id = site_id or self.conn.extra_dejson.get('site_id', '')
- self.server = Server(self.conn.host, use_server_version=True)
+ self.server = Server(self.conn.host)
+ self.server.add_http_options(options_dict={'verify': self.conn.extra_dejson.get('verify', False)})
Review comment:
Thank you for the review.
I completely agree with the changes you are requiring; in the next few days, I will implement them.
For the default value of `verify` I set the value False following the same Tableau example you linked above. The Tableau doc is not clear and I thought it was the right value in case of an HTTP connection.
However, looking deeper into the implementation code of `tableauserverclient` module ([here](https://github.com/tableau/server-client-python/blob/ce37a2063eddf317aaf2703aa3ebccf8053e1c8a/tableauserverclient/server/server.py)) I think it is ok (or better) also set `verify=True` as default. I mean with the current Tableau Hook implementation it works with HTTP connection and with that default we can avoid not verify SSL connection.
So I agree with you to set `verify=True` as default.
--
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] eladkal merged pull request #16365: Allow disable SSL for TableauHook
Posted by GitBox <gi...@apache.org>.
eladkal merged pull request #16365:
URL: https://github.com/apache/airflow/pull/16365
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org