Skip to content

Adding kerberos support for Presto - #229

Merged
gsilk merged 7 commits into
dropbox:masterfrom
ShashwatArghode:prestoKrbSupport
Apr 11, 2019
Merged

Adding kerberos support for Presto#229
gsilk merged 7 commits into
dropbox:masterfrom
ShashwatArghode:prestoKrbSupport

Conversation

@ShashwatArghode

Copy link
Copy Markdown
Contributor

#228
Connecting to Kerberized Presto cluster using Kerberos authentication.

SQLAlchemy URI:
presto://{Presto-coordinator}:{port}/{catalog}/{schema}?KerberosKeytabPath=/path/to/keytab&KerberosPrincipal=principal&KerberosRemoteServiceName=service&protocol=https

Patch is tested with Apache Superset.

@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #229 into master will decrease coverage by 0.68%.
The diff coverage is 45.45%.

Impacted file tree graph

@@ Coverage Diff @@## master #229 +/- ##
==========================================
- Coverage 93.92% 93.24% -0.69% 
==========================================
Files 14 14 Lines 1482 1495 +13 Branches 157 162 +5 ==========================================
+ Hits 1392 1394 +2 - Misses 64 74 +10 - Partials 26 27 +1
Impacted FilesCoverage Δ
pyhive/presto.py87.01% <45.45%> (-6.61%)⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8e07ca8...dc6c8bf. Read the comment docs.

@gglanzanigglanzani left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good, I've got a couple of minor points!

Comment threadsetup.py Outdated
],
extras_require={
'presto': ['requests>=1.0.0'],
'presto': ['requests>=1.0.0', 'requests_kerberos>=0.12.0'],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is actually only needed when using Kerberos, wouldn't it be better to add an extra extra?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I have added a kerberos field in extra_require. I think that is more suitable. Please let me know your thoughts on this.

Comment threadpyhive/presto.py Outdated
def __init__(self, host, port='8080', username=None, catalog='hive',
schema='default', poll_interval=1, source='pyhive', session_props=None,
protocol='http', password=None, requests_session=None, requests_kwargs=None):
protocol='http', password=None, KerberosRemoteServiceName=None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This would break compatibility if people were using it without named argument. All the kerberos parameters can go at the end.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Incorporated these changes.

Comment threadpyhive/presto.py Outdated
schema='default', poll_interval=1, source='pyhive', session_props=None,
protocol='http', password=None, requests_session=None, requests_kwargs=None):
protocol='http', password=None, KerberosRemoteServiceName=None,
KerberosPrincipal=None, KerberosConfigPath=None, KerberosKeytabPath=None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is there a reason not to use snake case?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Keeping it the same as mentioned in presto docs is the motive.

@chadheyne

Copy link
Copy Markdown

@ShashwatArghode@gglanzani -- what's the status of getting this merged in? Would love to get support for this added in so that we can continue to use Superset.

@ShashwatArghode

Copy link
Copy Markdown
ContributorAuthor

@gglanzani@chadheyne
Sorry for the delay in my response, was caught up in other tasks. I will take a look at the comments/suggestions and will try to merge it ASAP.

@gglanzani

Copy link
Copy Markdown

@ShashwatArghode I’ve left some comments back then in the PR, and I was curious about your opinion on them

@ShashwatArghode

Copy link
Copy Markdown
ContributorAuthor

@gglanzani I have incorporated the comments. But there is some sqlalchemy related test case failure. I am new to sqlalchemy, I will take a better look at it but any help would be appreciated.

@gglanzani

Copy link
Copy Markdown

You probably need to pass resolve_fks=False in sqlalchemy_test_case every time you call reflecttable.

The second exception probably comes from the fact that we use BINARY instead of LargeBinary, but I’m on my iPad and it’s hard to debug from here :)

@chadheyne

Copy link
Copy Markdown

@gglanzani@ShashwatArghode

My read of the failing test cases is that it's related to a new SQLAlchemy version being released and https://github.com/dropbox/PyHive/blob/master/.travis.yml#L9 is pulling the most recent version. The newest version added the resolve_fks argument so I don't think modifying the tests will work as the other configurations would continue to fail.

Since the failing test cases are unrelated to these changes is there any way we can move forward with this pull request and address those failures in a separate issue?

Who do we need to ping on this issue to move forward?

@gglanzani

Copy link
Copy Markdown

@chadheyne You need to ping @gsilk to make it move forward, I am not a committer.

@bolkedebruin

Copy link
Copy Markdown

Ping @gsilk can you have a look?

@gsilk

Copy link
Copy Markdown

Sorry for being tardy on this. I'm going to merge this in and have a look at the failing tests.

@gsilk
gsilk merged commit 6925cd7 into dropbox:masterApr 11, 2019
@VicoWu

VicoWu commented Apr 19, 2019

Copy link
Copy Markdown

@gsilk great thanks for your kerberos feature;

I am running on python 2.7.16 and cannot connect to my presto server ;

 KEYTAB_FILE="/Users/chang.wu/work/workspace/hulu-github-src/hadoop-security/chang.wu.keytab"
cursor = presto.connect(host='presto.server.hulu.com',
port='7778',
protocol='https',
KerberosConfigPath='/etc/krb5.conf',
KerberosPrincipal='chang.wu@TEST.SERVER.HULU.COM',
KerberosRemoteServiceName='presto',
KerberosKeytabPath=KEYTAB_FILE,
).cursor()
cursor.execute('show catalogs')
print cursor.fetchone()
print cursor.fetchall()
Traceback (most recent call last):
File "/Users/chang.wu/work/workspace/hulu-github-src/hadoop-security/code-layer-verification/src/main/java/com/hulu/security/presto/PyHiveKerberosClient.py", line 21, in <module>
cursor.execute('show catalogs')
File "/Users/chang.wu/work/workspace/origin-github-src/PyHive/pyhive/presto.py", line 241, in execute
url, data=sql.encode('utf-8'), headers=headers, **self._requests_kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/requests/api.py", line 116, in post
return request('post', url, data=data, json=json, **kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/requests/api.py", line 60, in request
return session.request(method=method, url=url, **kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/requests/sessions.py", line 533, in request
resp = self.send(prep, **send_kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/requests/sessions.py", line 646, in send
r = adapter.send(request, **kwargs)
File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/requests/adapters.py", line 514, in send
raise SSLError(e, request=request)
requests.exceptions.SSLError: HTTPSConnectionPool(host='lascorehadoop-15d21.server.hulu.com', port=7778): Max retries exceeded with url: /v1/statement (Caused by SSLError(SSLError(1, u'[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:727)'),))
Process finished with exit code 1

I am quite sure that my kerberized presto server is Ok because I could use kerberized java client to connect to it;
Do you know what's happening or do you have any clues? I am so curious that how could I specify the certificate because it is based on SSL/TLS .

@BenoitHanotteBenoitHanotte mentioned this pull request Jun 7, 2019
@cerodav

Copy link
Copy Markdown

Hi, it would be really great if you could give details on how to connect to presto via SQLAlchemy with kerberos based authetication. I don't see any documentation for this anywhere

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@ShashwatArghode@codecov-io@chadheyne@gglanzani@bolkedebruin@gsilk@VicoWu@cerodav