cloud-fan commented on code in PR #57828:
URL: https://github.com/apache/spark/pull/57828#discussion_r3735871786
##########
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:
Good point. Addressed in 88dd06fef54: TestRequirementFailingProvider is now
inert by default and throws only when
spark.security.credentials.test-requirement-failing.enabled=true is explicitly
set by the two tests that exercise this failure.
##########
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:
I do not think we should catch on this branch. The
non-Kerberos/direct-provider call sites both invoke
obtainDelegationTokens(isolateFailures = true), so requirement-check failures
are already isolated there. The isolateFailures = false branch is reached only
from the Kerberos paths (doLogin / doAs) and intentionally preserves the
existing behavior of propagating provider failures. Adding a catch here would
change that Kerberos behavior as well.
--
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]
