[SPARK-18061][ThriftServer] Add spnego auth support for ThriftServer thrift/http protocol #18628
Conversation
Change-Id: Icec0957ae47cb031883a3c9affcea8580cf9ab35
|
Test build #79592 has finished for PR 18628 at commit
|
|
@cloud-fan would you please help to review, thanks a lot! |
jerryshao
changed the title
Change-Id: I801f048afc6f683f7519b0cbf1c99e2268500c45
|
Test build #79628 has finished for PR 18628 at commit
|
| } catch { | ||
| case e: IOException => | ||
| throw new ServiceException("Unable to login to spnego with given principal/keytab " + | ||
| s"$principal/$keyTabFile", e) |
There was a problem hiding this comment.
shall we hide the principal/keytab?
|
looks good according to the existing similar code, but I'm not very familiar with this part of code, will leave it for a few days if someone else has some comments. Also cc @jiangxb1987 |
| // Try creating spnego UGI if it is configured. | ||
| val principal = hiveConf.getVar(ConfVars.HIVE_SERVER2_SPNEGO_PRINCIPAL) | ||
| val keyTabFile = hiveConf.getVar(ConfVars.HIVE_SERVER2_SPNEGO_KEYTAB) | ||
| if (principal.nonEmpty && keyTabFile.nonEmpty) { |
There was a problem hiding this comment.
Should we log warning if principal or keyTablFile is empty?
There was a problem hiding this comment.
I'm trying to avoid calling log APIs using reflection as mentioned in #15594
|
This looks good to me, cc @cmirash @vanzin @steveloughran |
Change-Id: I486c361a96687e3611754895d895b5326ee22f52
|
Test build #79681 has finished for PR 18628 at commit
|
| } catch { | ||
| case e: IOException => | ||
| throw new ServiceException("Unable to login to spnego with given principal/keytab", e) | ||
| } |
There was a problem hiding this comment.
I'd recommend including as much diagnostics in the exception string, including principal & keytab, and the value of e.toString in the message
throw new ServiceException(s"Unable to login to spnego with principal `$principal and keytab `$keytab`: $e", e)
The scenario to plan for is "nothing works and all you have to go on is that string from the raised exception".
|
|
||
| // Try creating spnego UGI if it is configured. | ||
| val principal = hiveConf.getVar(ConfVars.HIVE_SERVER2_SPNEGO_PRINCIPAL) | ||
| val keyTabFile = hiveConf.getVar(ConfVars.HIVE_SERVER2_SPNEGO_KEYTAB) |
There was a problem hiding this comment.
HiveConf.getVar() doesn't trim thhe results of leading/trailing via {{Configuration.getTrimmed()}}. Check for other uses of the confvar to see if there is any trimming of whitespace before and after their use: if so, you need to copy that.
There was a problem hiding this comment.
I searched the code, looks like there's no other place where using HiveConf.getVar trims the result.
Change-Id: Icce88ff9b266f470fd5407496756c933c0f1f959
|
Test build #80193 has finished for PR 18628 at commit
|
|
Thanks for making sure this is consistent with other uses of Configuration.get(); consistency is critical here |
|
@cloud-fan @jiangxb1987, what do you think about this PR, I think it mostly copies from HS2, and it is quite isolated unless we enabled spnego, so it should be safe to merge. |
|
retest this please |
|
LGTM pending jenkins |
|
Test build #81419 has finished for PR 18628 at commit
|
|
Thanks @jiangxb1987 , let me merge it to master. |
5 participants
What changes were proposed in this pull request?
Spark ThriftServer doesn't support spnego auth for thrift/http protocol, this mainly used for knox+thriftserver scenario. Since in HiveServer2 CLIService there already has existing codes to support it. So here copy it to Spark ThriftServer to make it support.
Related Hive JIRA HIVE-6697.
How was this patch tested?
Manual verification.