Allow disable SSL for TableauHook #16365

ciancolo · 2021-06-10T10:15:37Z

Hello all,
~~with the current implementation of the Tableau Hook is not possible to connect to a Tableau Server through SSL.~~ with the current implementation of the Tableau Hook is not possible to connect to a Tableau Server without 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 for more information.
In case of fundamental code change, 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 UPDATING.md.

Fixes: #16306

eladkal

Thanks for this PR!
Left one comment in the code and also two more things:

Update the connection doc https://github.com/apache/airflow/blob/main/docs/apache-airflow-providers-tableau/connections/tableau.rst
The changes you made to the test covers only the default value. Please add a unit test to cover the other case.

eladkal · 2021-06-10T18:15:12Z

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)})


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 defaultFalse? 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?

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

great please ping me when you push the changes

@eladkal I uploaded the requested changes. I am available for any doubts or requests.
Thank you.

eladkal

Overall LGTM.
left some comments.

Please also fix static test which are failing.
Did you test these changes locally with Tableau?

eladkal · 2021-06-20T06:38:37Z

tests/providers/tableau/hooks/test_tableau.py

+                host='tableau',
+                login='user',
+                password='password',
+                extra='{"verify": "my_cert", "cert": "my_client_cert"}',


my_cert isn't a valid option for verify it's a bit confusing.

eladkal · 2021-06-20T06:39:42Z

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',


Suggested change

conn_id='tableau_test_ssl_connection',

conn_id='tableau_test_ssl_false_connection',

eladkal · 2021-06-20T06:40:16Z

tests/providers/tableau/hooks/test_tableau.py

+    @patch('airflow.providers.tableau.hooks.tableau.Server')
+    def test_get_conn_ssl(self, mock_server, mock_tableau_auth):
+        """
+        Test get conn with SSL parameters


Suggested change

Test get conn with SSL parameters

Test get conn with SSL disabled parameters

eladkal · 2021-06-20T06:42:32Z

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),


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

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

…ability.

…a option in Tableau Hook.

potiuk

LGTM. @eladkal ?

github-actions · 2021-07-10T11:33:29Z

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.

* Fixed Tableau connection with ssl mode. * Restored site_id field in tableau hook. * Updated test for provider Tableau Hook to adapt them to the new changes. * Added the name of the parameter in function call for better code readability. * Implemented the possibility to add verify and cert parameters in extra option in Tableau Hook. * Covered connection test with SSL parameter. * Updated documentation for Tableau connection. * Added conversion from string to bool for verify parameter. * Updated Tableau hook test. * Precommit modifications. * Fixed bug in test. * Fixed Tableau docs. Co-authored-by: Michele <mzanchi@tenaris.com>

boring-cyborg bot added the area:providers label Jun 10, 2021

eladkal requested changes Jun 10, 2021

View reviewed changes

eladkal changed the title ~~Implemented Tableau Hook connection through SSL~~ Allow disable SSL for TableauHook Jun 11, 2021

ciancolo requested review from ashb, dimberman, ephraimbuddy, jhtimmins, kaxil, mik-laj, potiuk, ryanahamilton, turbaszek, vikramkoka and XD-DENG as code owners June 18, 2021 15:01

ciancolo force-pushed the fix-tableau-ssl branch from 2f9df89 to 955d8cb Compare June 18, 2021 16:14

eladkal requested changes Jun 20, 2021

View reviewed changes

eladkal removed request for ryanahamilton, ashb, potiuk, dimberman, ephraimbuddy, jhtimmins, kaxil, turbaszek, XD-DENG, mik-laj and vikramkoka June 20, 2021 06:44

Michele added 3 commits July 9, 2021 15:33

Fixed Tableau connection with ssl mode.

4cb65ba

Restored site_id field in tableau hook.

aaf39a2

Updated test for provider Tableau Hook to adapt them to the new changes.

7eef1fb

Michele Zanchi added 7 commits July 9, 2021 15:33

Added the name of the parameter in function call for better code read…

c1bfd20

…ability.

Implemented the possibility to add verify and cert parameters in extr…

049af00

…a option in Tableau Hook.

Covered connection test with SSL parameter.

3241eab

Updated documentation for Tableau connection.

1b56a52

Added conversion from string to bool for verify parameter.

1718ffd

Updated Tableau hook test.

91ef63d

Precommit modifications.

2e3d216

ciancolo force-pushed the fix-tableau-ssl branch from 955d8cb to 2e3d216 Compare July 9, 2021 13:35

Michele Zanchi added 2 commits July 9, 2021 17:56

Fixed bug in test.

3716245

Fixed Tableau docs.

27c4e13

potiuk approved these changes Jul 10, 2021

View reviewed changes

potiuk requested a review from eladkal July 10, 2021 11:20

eladkal approved these changes Jul 10, 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 Jul 10, 2021

eladkal merged commit df0746e into apache:main Jul 10, 2021

potiuk mentioned this pull request Jul 15, 2021

Status of testing Providers that were prepared on July 15, 2021 #17037

Closed

26 tasks

ciancolo mentioned this pull request Jul 21, 2021

Fix bool conversion Verify parameter in Tableau Hook #17125

Merged

potiuk mentioned this pull request Jul 27, 2021

Status of testing Providers that were prepared on July 27, 2021 (rc2) #17268

Closed

66 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow disable SSL for TableauHook #16365

Allow disable SSL for TableauHook #16365

ciancolo commented Jun 10, 2021 •

edited by eladkal

Loading

eladkal left a comment •

edited

Loading

eladkal Jun 10, 2021 •

edited

Loading

ciancolo Jun 11, 2021

eladkal Jun 13, 2021

ciancolo Jun 18, 2021

eladkal left a comment

eladkal Jun 20, 2021

eladkal Jun 20, 2021

eladkal Jun 20, 2021

eladkal Jun 20, 2021

ciancolo Jul 9, 2021

potiuk left a comment

github-actions bot commented Jul 10, 2021

	conn_id='tableau_test_ssl_connection',
	conn_id='tableau_test_ssl_false_connection',

	Test get conn with SSL parameters
	Test get conn with SSL disabled parameters

Allow disable SSL for TableauHook #16365

Allow disable SSL for TableauHook #16365

Conversation

ciancolo commented Jun 10, 2021 • edited by eladkal Loading

eladkal left a comment • edited Loading

Choose a reason for hiding this comment

eladkal Jun 10, 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

eladkal left a 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

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 10, 2021

ciancolo commented Jun 10, 2021 •

edited by eladkal

Loading

eladkal left a comment •

edited

Loading

eladkal Jun 10, 2021 •

edited

Loading