parthchandra commented on code in PR #57828:
URL: https://github.com/apache/spark/pull/57828#discussion_r3730972672
##########
core/src/test/scala/org/apache/spark/deploy/security/NonKerberosCredentialsSuite.scala:
##########
@@ -79,6 +79,20 @@ private class TestFailingProvider extends
HadoopDelegationTokenProvider {
}
}
+private class TestRequirementFailingProvider extends
HadoopDelegationTokenProvider {
+ override def serviceName: String = "test-requirement-failing"
+
+ override def delegationTokensRequired(
+ sparkConf: SparkConf, hadoopConf: Configuration): Boolean = {
+ throw new RuntimeException("Simulated provider requirement failure")
Review Comment:
This throws unconditionally and because this provider is loaded globally,
this could affect any core test that builds a manager with direct credentials
providers. Should we put the `throw` behind a config that our tests set
explicitly or do we want all tests to be implicitly aware of this? Your call.
##########
core/src/main/scala/org/apache/spark/deploy/security/HadoopDelegationTokenManager.scala:
##########
@@ -198,24 +207,30 @@ private[spark] class HadoopDelegationTokenManager(
val creds = new Credentials()
var failureCount = 0
val nextRenewal = delegationTokenProviders.values.flatMap { provider =>
- if (provider.delegationTokensRequired(sparkConf, hadoopConf)) {
- if (isolateFailures) {
- try {
+ if (isolateFailures) {
+ try {
+ if (provider.delegationTokensRequired(sparkConf, hadoopConf)) {
provider.obtainDelegationTokens(hadoopConf, sparkConf, creds)
- } catch {
- case e: Exception =>
- logWarning(log"Failed to obtain credentials from " +
- log"${MDC(LogKeys.SERVICE_NAME, provider.serviceName)}.", e)
- failureCount += 1
- None
+ } else {
+ logDebug(s"Service ${provider.serviceName} does not require a
token." +
+ s" Check your configuration to see if security is disabled or
not.")
+ None
}
- } else {
- provider.obtainDelegationTokens(hadoopConf, sparkConf, creds)
+ } catch {
+ case e: Exception =>
+ logWarning(log"Failed to obtain credentials from " +
+ log"${MDC(LogKeys.SERVICE_NAME, provider.serviceName)}.", e)
+ failureCount += 1
+ None
}
} else {
- logDebug(s"Service ${provider.serviceName} does not require a token." +
- s" Check your configuration to see if security is disabled or not.")
- None
+ if (provider.delegationTokensRequired(sparkConf, hadoopConf)) {
Review Comment:
Should we add a try-catch here as well? This path can be reached by
non-kerberos path and a provider like `TestRequirementFailingProvider` can
cause that path to fail?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
