sarutak commented on code in PR #58018:
URL: https://github.com/apache/spark/pull/58018#discussion_r3789950932
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -131,6 +131,22 @@ private[spark] class UserCredentialManager(
logInfo(log"Credential acquisition successful. Next renewal in " +
log"${MDC(LogKeys.TIME_UNITS, UIUtils.formatDuration(renewalDelay))}.")
+
+ // Apply additional Spark properties declared by active providers.
+ // This allows provider modules to wire executor-side configuration
+ // (e.g., fs.s3a.aws.credentials.provider) without core having
+ // vendor-specific knowledge. Properties are only set if the user
+ // has not already configured them explicitly.
+ CredentialProviderLoader.discoverAllProviders().forEach { provider =>
Review Comment:
**1. Properties are applied from all discovered providers, not just active
ones.**
`discoverAllProviders()` returns **all** providers found by ServiceLoader on
the classpath, regardless of whether they are actually used in this job.
However, the Javadoc on `additionalSparkProperties()` says "when this provider
is active", and the PR description also says "when the provider is active."
Consider a user who has both `credential-aws` and a future `credential-gcp`
module on the classpath but is only using GCP for this particular job. The
current implementation would still set
`spark.hadoop.fs.s3a.aws.credentials.provider` to
`SparkOidcAwsCredentialsProvider`. If the `credential-aws` module's classes are
not fully available (e.g., fat-JAR assembly excluded the AWS SDK), this leads
to a `ClassNotFoundException` at S3A FileSystem initialization time — even
though the user never intended to use S3.
Note: this also has a design benefit for the SPI contract. If
`additionalSparkProperties()` is guaranteed to be called after `init()` and
`resolve()`, its Javadoc can state this explicitly, allowing future
implementations to return dynamic values based on provider state.
**2. No defensive handling for SPI calls.**
- If a third-party implementation returns `null` (violating the contract),
the inner `forEach` throws NPE and `start()` fails entirely.
- If any provider's `additionalSparkProperties()` throws an exception (e.g.,
accessing uninitialized state), all subsequent providers are skipped and
`start()` may fail.
Spark's existing SPI patterns (e.g., `HadoopDelegationTokenManager`) wrap
each provider call in try-catch with `NonFatal`.
**Requirements for the fix:**
1. Only providers that **successfully resolved credentials** should
contribute `additionalSparkProperties()`. This is the precise definition of
"active"
2. Defensive null check and try-catch per provider to isolate failures
3. Properties should only be applied once (during initial `start()`), not on
every renewal, and renewal-time SparkConf changes don't propagate to
already-running executors
4. `discoverAllProviders()` can likely be removed if the above is addressed,
keeping the public API surface smaller
##########
core/src/main/java/org/apache/spark/security/CredentialProviderLoader.java:
##########
@@ -251,6 +251,20 @@ public static Set<String> discoverAllSchemes() {
return schemes;
}
+ /**
+ * Returns all registered providers discovered via ServiceLoader.
+ * <p>
+ * Unlike {@link #providerFor(String, Map)}, this does not initialize
providers.
+ * It is intended for querying provider metadata (e.g.,
+ * {@link CredentialProvider#additionalSparkProperties()}).
+ *
+ * @return an unmodifiable list of all discovered providers
+ * @since 4.4.0
+ */
+ public static List<CredentialProvider> discoverAllProviders() {
+ return Collections.unmodifiableList(getProviders());
Review Comment:
If property application is scoped to active providers (as suggested above),
`discoverAllProviders()` has no remaining consumer in this PR. I'd suggest
removing it to avoid expanding the public API surface with a method that
exposes internal state (the cached ServiceLoader list) without a clear use case.
If you do keep it for other reasons, the Javadoc "does not initialize
providers" is ambiguous. It should clarify that `init(Map)` is not called,
while ServiceLoader instantiation (no-arg constructor) still occurs.
##########
core/src/main/java/org/apache/spark/security/CredentialProvider.java:
##########
@@ -102,6 +102,28 @@ default Duration suggestedTtl() {
return Duration.ofMinutes(15);
}
+ /**
+ * Returns additional Spark configuration properties that should be set when
this
+ * provider is active.
+ * <p>
+ * The credential management layer applies these entries to {@code
SparkConf} after
+ * successful startup, only if the user has not already set them explicitly.
This
+ * allows provider modules to declare executor-side wiring (e.g., the Hadoop
+ * credentials provider class for a particular filesystem scheme) without
requiring
+ * core to have vendor-specific knowledge.
+ * <p>
+ * Keys must include the {@code spark.hadoop.} prefix if they target Hadoop
+ * configuration on executors (propagated via {@code SparkAppConfig}).
+ * <p>
+ * The default implementation returns an empty map (no additional
properties).
+ *
+ * @return an unmodifiable map of property key-value pairs (never null)
+ * @since 4.4.0
+ */
+ default Map<String, String> additionalSparkProperties() {
Review Comment:
The current Javadoc says "Keys must include the `spark.hadoop.` prefix if
they target Hadoop configuration." This is correct but could be more explicit
about the broader behavior:
- Only `spark.*` keys are effective (SparkConf convention)
- `spark.hadoop.*` keys are propagated to executor Hadoop Configuration with
the prefix stripped (via `SparkHadoopUtil.appendSparkHadoopConfigs()`)
- Other `spark.*` keys are treated as Spark-internal configuration
Suggested Javadoc addition:
```java
* This method is called after {@link #init(Map)} and a successful
* {@link #resolve(UserContext, URI)} invocation. Implementations may
* assume that provider state is fully initialized when this is called.
* <p>
* Keys must use the {@code spark.} prefix to be effective (SparkConf
convention).
* Keys with the {@code spark.hadoop.} prefix are propagated to executor-side
* Hadoop {@code Configuration} with the prefix stripped. Other {@code
spark.*}
* keys are applied as Spark-internal configuration.
```
##########
connector/credential-aws/src/test/java/org/apache/spark/security/aws/AwsStsCredentialProviderSuite.java:
##########
@@ -1002,4 +1002,33 @@ private StsClient createMockStsClient(String
accessKeyId, String secretAccessKey
.thenReturn(response);
return mockSts;
}
+
+ // ========== additionalSparkProperties() ==========
Review Comment:
The added tests verify
`AwsStsCredentialProvider.additionalSparkProperties()` in isolation, which is
good. However, the core logic, the loop in `UserCredentialManager.start()` that
actually applies properties to `SparkConf`, has no dedicated test coverage.
Key scenarios that should be tested:
1. Provider-declared properties are applied to SparkConf
2. User-set properties are NOT overwritten (`contains` check works)
3. Multiple providers' properties are all applied
4. Provider returning null does not crash the manager
5. Provider throwing an exception does not prevent other providers from
being processed
These would go in `UserCredentialManagerSuite` (or equivalent).
##########
core/src/main/scala/org/apache/spark/deploy/security/UserCredentialManager.scala:
##########
@@ -131,6 +131,22 @@ private[spark] class UserCredentialManager(
logInfo(log"Credential acquisition successful. Next renewal in " +
log"${MDC(LogKeys.TIME_UNITS, UIUtils.formatDuration(renewalDelay))}.")
+
+ // Apply additional Spark properties declared by active providers.
+ // This allows provider modules to wire executor-side configuration
+ // (e.g., fs.s3a.aws.credentials.provider) without core having
+ // vendor-specific knowledge. Properties are only set if the user
+ // has not already configured them explicitly.
+ CredentialProviderLoader.discoverAllProviders().forEach { provider =>
+ provider.additionalSparkProperties().forEach { (key, value) =>
+ if (!sparkConf.contains(key)) {
Review Comment:
If two providers declare the same key with different values, the first one
processed "wins" (due to the `contains` check). Since ServiceLoader discovery
order is not deterministic, this produces non-deterministic behavior.
A warning log when a key is already set by a *previous provider* (as opposed
to user-set) would help debugging:
```scala
if (!sparkConf.contains(key)) {
sparkConf.set(key, value)
logInfo(...)
} else {
logDebug(log"Skipped ${MDC(LogKeys.CONFIG, key)} from " +
log"${MDC(LogKeys.CLASS_NAME, provider.getClass.getName)} " +
log"(already configured)")
}
```
--
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]
