Skip to content

[SPARK-57891][CORE] Add CredentialProvider SPI and ServiceLoader discovery#57191

Closed
yadavay-amzn wants to merge 4 commits into
apache:masterfrom
yadavay-amzn:SPARK-57891
Closed

[SPARK-57891][CORE] Add CredentialProvider SPI and ServiceLoader discovery#57191
yadavay-amzn wants to merge 4 commits into
apache:masterfrom
yadavay-amzn:SPARK-57891

Conversation

@yadavay-amzn

@yadavay-amzn yadavay-amzn commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This is task 2 of the OIDC Credential Propagation SPIP (SPARK-57703), building on the core types added in SPARK-57890. It introduces the pluggable provider SPI in org.apache.spark.security (spark-core):

  • CredentialProvider - a @DeveloperApi interface that providers (AWS STS, Azure, GCP, etc.) implement to exchange a user identity for a short-lived ServiceCredential
  • CredentialResolutionException - a @DeveloperApi checked exception thrown by resolve.
  • CredentialProviderLoader - discovers implementations via ServiceLoader and selects a provider per scheme. Scheme keys are normalized to lowercase. Selection follows an explicit-configuration policy: spark.security.credentials.provider.<scheme> names the fully-qualified provider class to use for that scheme. If exactly one discovered provider supports a scheme, no configuration is needed; if multiple support it and the conf is unset, a clear error is raised listing the candidates. Each selected provider is initialized exactly once.

A FakeCredentialProvider (and a second one sharing a scheme) plus a META-INF/services registration are added under core/src/test to exercise discovery and selection.

Why are the changes needed?

There is currently no pluggable SPI for exchanging an identity token for short-lived service credentials. This is the extension point the credential-propagation framework builds on. See the SPIP design document, Appendix A.

Does this PR introduce any user-facing change?

No behavior change. It adds new @DeveloperApi types only; nothing uses them until the follow-up (SPARK-57892 / the manager task).

How was this patch tested?

New JUnit CredentialProviderLoaderSuite covering the acceptance criteria, ServiceLoader discovery, provider selection/ambiguity, error cases, and null-input guards.

Was this patch authored or co-authored using generative AI tooling?

No.

// Initialize exactly once under the lock (first-conf-wins).
synchronized (CredentialProviderLoader.class) {
if (!initializedProviders.contains(selected)) {
selected.init(conf);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have a security concern here. The full Spark configuration map is passed to init(). Since providers are discovered via ServiceLoader (potentially third-party JARs), a buggy or malicious provider receives all config properties including secrets from other subsystems (e.g., spark.authenticate.secret, database passwords, cloud keys).

Spark has precedent for namespace scoping. DataSourceV2Utils.extractSessionConfigs() filters the full conf to only spark.datasource.<keyPrefix>.* before passing to providers. Similar to this approach, can we filter the map to only pass spark.security.credentials.* keys?

@yadavay-amzn yadavay-amzn Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing that out, it's a very valid concern and something I missed!

Updated, init() conf now filtered to only spark.security.credentials.* (prefix filter matching the DataSourceV2Utils.extractSessionConfigs precedent).
Also added a test to check spark.authenticate.secret/spark.ssl.keyPassword do not reach init().

// Initialize exactly once under the lock (first-conf-wins).
synchronized (CredentialProviderLoader.class) {
if (!initializedProviders.contains(selected)) {
selected.init(conf);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What happens if init() throws? Currently the provider is not added to initializedProviders, so a subsequent call would retry init(). This retry behavior seems reasonable, but:

  • The provider instance may be in a half-initialized state after a failed init(). Is that safe to retry?
  • Should the contract document whether init() is expected to be idempotent-safe (i.e., safe to call again after a previous failure)?

How about:

  1. Documenting the retry-on-failure behavior in the init() Javadoc (e.g., "If init() throws, it may be retried on the next resolution attempt. Implementations should be safe to call again after a prior failure."), or
  2. Adding a test verifying this behavior

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Documented the retry behavior and added a test (testInitRetryAfterFailure) to show the loader retries and succeeds.

@sarutak

sarutak commented Jul 13, 2026

Copy link
Copy Markdown
Member

Let's remove the "Design notes (feedback/suggestions welcome)" section from the description because it's used as commit log when this PR is merged.

…ty.credentials.*, reject empty scheme, document thread-safety and init retry
@yadavay-amzn

Copy link
Copy Markdown
Contributor Author

@sarutak Thanks for reviewing!
Updated the PR description and addressed the other comments (replies in-line).

One note about

The provider instance may be in a half-initialized state after a failed init(). Is that safe to retry?

In the testInitRetryAfterFailure, the fake throws immediately (before setting any state), so it does not exercise a half-initialized instance. It proves retry happens, not that the retry of partial-state is safe.

In this per-workload phase init() runs once at startup, so repeated re-init is rare and relying on the documented contract seems reasonable here.
If per-session/Connect support later makes re-init frequent, a robust option would be for the loader to discard and re-create a fresh provider instance on an init() failure, so a retry never runs on a half-initialized instance regardless of the implementation. Since CredentialProviderLoader is @Private, that can be added later.

I'm assuming this okay to do as a followup but wanted to check if you'd like this incorporated into this PR.

@sarutak

sarutak commented Jul 13, 2026

Copy link
Copy Markdown
Member

I'm assuming this okay to do as a followup but wanted to check if you'd like this incorporated into this PR.

Yes, it's OK to do as a followup.

@sarutak sarutak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Pending GA.

@sarutak sarutak closed this in 475a771 Jul 14, 2026
sarutak pushed a commit that referenced this pull request Jul 14, 2026
…overy

### What changes were proposed in this pull request?

This is task 2 of the OIDC Credential Propagation SPIP ([SPARK-57703](https://issues.apache.org/jira/browse/SPARK-57703)), building on the core types added in SPARK-57890. It introduces the pluggable provider SPI in `org.apache.spark.security` (spark-core):

- `CredentialProvider` - a `DeveloperApi` interface that providers (AWS STS, Azure, GCP, etc.) implement to exchange a user identity for a short-lived `ServiceCredential`
- `CredentialResolutionException` - a `DeveloperApi` checked exception thrown by `resolve`.
- `CredentialProviderLoader` - discovers implementations via `ServiceLoader` and selects a provider per scheme. Scheme keys are normalized to lowercase. Selection follows an explicit-configuration policy: `spark.security.credentials.provider.<scheme>` names the fully-qualified provider class to use for that scheme. If exactly one discovered provider supports a scheme, no configuration is needed; if multiple support it and the conf is unset, a clear error is raised listing the candidates. Each selected provider is initialized exactly once.

A `FakeCredentialProvider` (and a second one sharing a scheme) plus a `META-INF/services` registration are added under `core/src/test` to exercise discovery and selection.

### Why are the changes needed?

There is currently no pluggable SPI for exchanging an identity token for short-lived service credentials. This is the extension point the credential-propagation framework builds on. See the SPIP design document, Appendix A.

### Does this PR introduce _any_ user-facing change?

No behavior change. It adds new `DeveloperApi` types only; nothing uses them until the follow-up (SPARK-57892 / the manager task).

### How was this patch tested?

New JUnit `CredentialProviderLoaderSuite` covering the acceptance criteria, ServiceLoader discovery, provider selection/ambiguity, error cases, and null-input guards.

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #57191 from yadavay-amzn/SPARK-57891.

Authored-by: Anupam Yadav <anupamy030@gmail.com>
Signed-off-by: Kousuke Saruta <sarutak@apache.org>
(cherry picked from commit 475a771)
Signed-off-by: Kousuke Saruta <sarutak@apache.org>
@sarutak

sarutak commented Jul 14, 2026

Copy link
Copy Markdown
Member

Merge Summary:

Posted by merge_spark_pr.py

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.

2 participants