Skip to content

Conversation

@woj-i
Copy link
Contributor

@woj-i woj-i commented Nov 20, 2015

@andrewor14 the same PR as in branch 1.5
@harishreedharan

@harishreedharan
Copy link
Contributor

LGTM. Are there any other configs required? I remember Hadoop security had a bunch of configs.

/cc @tgravescs

@woj-i
Copy link
Contributor Author

woj-i commented Nov 20, 2015

This is the one parameter needed to update kerberos credentials based on a keytab. The other parameters can be loaded from system environments or config files.

@tgravescs
Copy link
Contributor

so Spark has never officially supported this outside of YARN - local mode, standalone, etc. So this isn't a bug, but would be an improvement.

--keytab and --principal options are under the YARN only section, documentation is for yarn only, etc..

I'm fine if you want to take this on but I think if you make it more then YARN you need to make sure it works in all the environments or at least the one we intend to support and its all documented, tested, etc.

There have been attempts at adding this to standalone in the past but have always had various issues. cc @pwendell as he might remember more about that mode.

I also don't like forcing the HADOOP_SECURITY_AUTHENTICATION setting for all environments to kerberos. For YARN this should be picked up from the hadoop configuration if other modes require it thats fine but at the same time if other modes are using hadoop and kerberos perhaps they should pick up the proper hadoop configuration instead of hardcoding it.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

Hi!
Can you test it on all environments? If not, then I can shrink the scope to the YARN and the local mode.

In case of HADOOP_SECURITY_AUTHENTICATION there isn't this parameter in SparkConf, neither in HiveContext. What can you suggest? I think if the keytab is defined then authentication method should be Kerberos, shouldn't it?

@vanzin
Copy link
Contributor

vanzin commented Nov 23, 2015

So, is there any code in any of the other backends to actually use those config options?

Looking at the bug, isn't the fix actually achieved by just doing the login (UserGroupInformation.loginUserFromKeytab(args.principal, args.keytab))?

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

No It's not enough. Please look at the code of the method UserGroupInformation.loginUserFromKeytab(args.principal, args.keytab). There is a check of authentication type. If it's simple (what is default in the classloader context) then parameters are not used. So that I think the recent patch need this fix. I faced the situation I described. :)

@vanzin
Copy link
Contributor

vanzin commented Nov 23, 2015

I know that, but that's not what I'm asking about. I'm asking about all the other code; why do you need to set spark.yarn.keytab for other backends?

I also agree with Tom that the authentication config value should come from the hadoop configuration, and not be set explicitly like this. If you need to set that, you're using the wrong configuration.

Also, I'm a little skeptical that this would work; anybody who has kerberos auth for the Hive metastore probably has the same for HDFS, and while this patch would make it work for local mode, it would not work for Standalone nor Mesos.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

Let's see the situation when I wrap the context of SparkContext with my UGI and even wrap the HiveContext with my UGI then most of Spark functions work with my provided credentials. However, the ClientWrapper object is created in another (the classloader) context, so the UGI provided in my application doesn't work anymore. That's why the property has to be set. Is it clear or should I describe it in details?

@vanzin
Copy link
Contributor

vanzin commented Nov 23, 2015

Ah, I see. ClientWrapper is also reading those properties (which was my original question). Still, I don't think changing the Hadoop configuration is the right thing (as has been pointed out, you're just using the wrong configuration from the get go if you need to do that).

Also, in non-local, non-yarn mode, this will just lead to different issues down the line, so those other modes should probably be excluded from this.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

Ok, I can prepare a commit with:

  1. Shrinking the scope of this feature to YARN or LOCAL modes
  2. I'll try to load the "authentication method" property from Hadoop config file specified under a HADOOP_CONF_DIR environment variable or the classpath (file core-site.xml).

@vanzin , @tgravescs what do you think?

@vanzin
Copy link
Contributor

vanzin commented Nov 23, 2015

I'll try to load the "authentication method" property from Hadoop config file

It's still unclear to me why you need to do that at all. If you set HADOOP_CONF_DIR to the right config directory, or place both core-site.xml and hive-site.xml in $SPARK_HOME/conf, wouldn't either of those be enough for the code to pick up the value of the authentication config?

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

No. I need to load the property, because it's not loaded automatically by UserGroupInformation.loginUserFromKeytab(args.principal, args.keytab), so it's required to load a keytab properly for objects created in another context than mine (e.g. classloader context for ClientWrapper).

@vanzin
Copy link
Contributor

vanzin commented Nov 23, 2015

UserGroupInformation.loginUserFromKeytab calls isSecurityEnabled which calls ensureInitialized which calls initialize(new Configuration(), false);.

That new Configuration() call should read core-site.xml from your classpath, which happens when you do either of the things I mentioned above. So perhaps there's some issue with how ClientWrapper is reading the configuration, but your solution and your analysis of the underlying problem are not entirely correct.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 23, 2015

I'll check it tomorrow.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 24, 2015

Hi!
I checked the code, you are right, by default xmls are loaded. However, in local mode it's cumbersome to attach hadoop config from the cluster. What do you think to set this parameter in local mode only?

@vanzin
Copy link
Contributor

vanzin commented Nov 24, 2015

That would be more acceptable; although you already have to provide the Hive configuration if you're accessing the metastore, so for correctness you should be providing everything (otherwise how do you plan to read the data stored in the tables?).

@woj-i
Copy link
Contributor Author

woj-i commented Nov 24, 2015

I see my problem. Defining the environment variable HADOOP_CONF_DIR is not enough and doesn't work (not read config files there). Copy of configs to the classpath works. Thanks @vanzin . You are right about the hive-site. All in all- I don't need to set additional config, just have keytab properties in YARN mode as well as LOCAL mode. I've made a commit.

I plan to read data from tables by sql method on a HiveContext

Copy link
Contributor

Choose a reason for hiding this comment

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

Better than this would be to update docs/sql-programming-guide.md. It currently only mentions hive-site.xml, when really you need core-site.xml and hdfs-site.xml too (otherwise kerberos, in your case, or things like HDFS HA won't work).

The other changes in this file can be reverted, too.

@vanzin
Copy link
Contributor

vanzin commented Nov 24, 2015

ok to test

@SparkQA
Copy link

SparkQA commented Nov 25, 2015

Test build #46625 has finished for PR 9859 at commit 7802c0c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 25, 2015

@vanzin thanks for your support. I've made a commit with documentation update and clean the code as you asked.

@SparkQA
Copy link

SparkQA commented Nov 25, 2015

Test build #46672 has finished for PR 9859 at commit 9365a7f.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Works also with the "local" master.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "for HDFS configuration"

@vanzin
Copy link
Contributor

vanzin commented Nov 25, 2015

Just minor nits, otherwise LGTM.

@vanzin
Copy link
Contributor

vanzin commented Nov 25, 2015

Also, just for posterity, if you're running local mode, you should be able to kinit before running your Spark app, and then you don't need to provide principal / keytab at all.

@woj-i
Copy link
Contributor Author

woj-i commented Nov 26, 2015

Thank you @vanzin for your help. I commited the nits in the documentation.

@SparkQA
Copy link

SparkQA commented Nov 26, 2015

Test build #46766 has finished for PR 9859 at commit 449cbbb.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Dec 1, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Dec 1, 2015

Test build #46923 has finished for PR 9859 at commit 449cbbb.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@vanzin
Copy link
Contributor

vanzin commented Dec 1, 2015

The change doesn't affect pyspark; so merging (master and 1.6).

asfgit pushed a commit that referenced this pull request Dec 1, 2015
andrewor14 the same PR as in branch 1.5
harishreedharan

Author: woj-i <[email protected]>

Closes #9859 from woj-i/master.

(cherry picked from commit 6a8cf80)
Signed-off-by: Marcelo Vanzin <[email protected]>
@asfgit asfgit closed this in 6a8cf80 Dec 1, 2015
Sign up for free to 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.

5 participants