You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "harshitaajoshi (via GitHub)" <gi...@apache.org> on 2026/10/25 21:08:45 UTC

[PR] Reuse AWS service identity credentials across requests for SigV4 federation [polaris]

harshitaajoshi opened a new pull request, #5869:
URL: https://github.com/apache/polaris/pull/5869

   Fixes #4711
   
   ## Why
   
   `DefaultServiceIdentityProvider` is request scoped, and every call to `getServiceIdentityCredential` built a new `AwsIamServiceIdentityCredential` through `AwsIamServiceIdentityConfiguration.awsCredentialsProvider()`, which creates a new `DefaultCredentialsProvider` each time. Nothing caches the result in between (`SigV4ConnectionCredentialVendor` asks for it on every federated request).
   
   With the AWS default credential chain, each `DefaultCredentialsProvider` instance owns its own credential cache, and under web identity (e.g. IRSA on EKS) its own STS client. A new instance per request therefore means a fresh `AssumeRoleWithWebIdentity` call per request, which matches the CloudTrail pattern in the issue. Those per request providers were also never closed.
   
   ## Change
   
   - New `@ApplicationScoped` `ServiceIdentityCredentialCache` that holds the resolved credential per (resolved realm, identity type), i.e. per source of configuration. It is filled lazily with `computeIfAbsent`, does not cache empty results, and closes `SdkAutoCloseable` AWS providers on shutdown.
   - `DefaultServiceIdentityProvider` resolves credentials through the cache. Its public no-arg constructor is unchanged.
   
   Service identity configuration is fixed at startup, so a resolved credential stays valid for the application lifetime, and the SDK provider refreshes the underlying session credentials itself. Static key configurations behave as before (a `StaticCredentialsProvider` is now simply reused). No SPI or interface changes.
   
   Notes for review:
   - The key is (realm, type) rather than the identity reference URN, because the URN encodes the default realm as `system:default`; keying by URN would let a realm with that literal name share the default realm's credential.
   - Realms without their own configuration fall back to the default realm's configuration and therefore share its credential, which is the same identity they used before.
   - The cached `ServiceIdentityCredential` instance is shared across requests. `ServiceIdentityCredential#setIdentityInfoReference` is public but has no callers today; happy to switch to caching only the AWS provider if reviewers prefer.
   
   ## Out of scope
   
   - `StorageConfiguration.stsCredentials()` follows the same pattern for the storage path, though `StorageCredentialCache` limits it to cache misses (raised in the issue).
   - Caching of the downstream SigV4 `AssumeRole` session credentials.
   - #5664 touches `DefaultServiceIdentityProvider#allocateServiceIdentity` and its test; this PR changes the constructor and `getServiceIdentityCredential`, so the two should compose cleanly. I'll rebase onto whichever lands first.
   
   ## Tests
   
   - `testGetServiceIdentityCredentialIsReusedAcrossRequests`: two request scoped providers in the same realm get the same AWS credentials provider. Fails on `main`, passes with this change.
   - `testGetServiceIdentityCredentialIsSharedOnlyByRealmsWithTheSameIdentity`: a realm falling back to the default configuration shares the default credential; a realm with its own configuration gets its own (static) credential.
   - `testCredentialCacheShutdownClosesCredentialsProviders`: shutdown closes closeable providers.
   - Existing tests use the injected CDI bean. `./gradlew :polaris-runtime-service:check` passes locally.
   
   This change was prepared with AI assistance; I reviewed and tested it end to end.
   
   ## Checklist
   - [x] ๐Ÿ›ก๏ธ Don't disclose security issues! (contact security@apache.org)
   - [x] ๐Ÿ”— Clearly explained why the changes are needed, or linked related issues: Fixes #4711
   - [x] ๐Ÿงช Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] ๐Ÿ’ก Added comments for complex logic
   - [x] ๐Ÿงพ Updated `CHANGELOG.md` (if needed): not needed, no configuration or upgrade impact
   - [x] ๐Ÿ“š Updated documentation in `site/content/in-dev/unreleased` (if needed): not needed
   


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reuse AWS service identity credentials across requests for SigV4 federation [polaris]

Posted by "dimas-b (via GitHub) via issues" <is...@polaris.apache.org>.
dimas-b commented on code in PR #5869:
URL: https://github.com/apache/polaris/pull/5869#discussion_r4429693959


##########
runtime/service/src/main/java/org/apache/polaris/service/identity/provider/DefaultServiceIdentityProvider.java:
##########
@@ -126,7 +134,22 @@ public Optional<ServiceIdentityCredential> getServiceIdentityCredential(
             identityConfig ->
                 buildIdentityInfoReference(realm, identityConfig.getType()).equals(ref))
         .findFirst()
-        .flatMap(identityConfig -> identityConfig.asServiceIdentityCredential(ref));
+        .flatMap(identityConfig -> resolveCredential(identityConfig, ref));
+  }
+
+  private Optional<? extends ServiceIdentityCredential> resolveCredential(
+      ResolvableServiceIdentityConfiguration identityConfig, SecretReference ref) {
+    // The AWS credentials provider is the expensive part (under web identity it owns an STS client
+    // and a credential cache), so it is shared across requests; the credential itself is built
+    // per call and never shared.
+    if (identityConfig instanceof AwsIamServiceIdentityConfiguration awsConfig) {
+      return Optional.of(
+          new AwsIamServiceIdentityCredential(
+              ref,
+              awsConfig.iamArn(),
+              credentialCache.awsCredentialsProvider(realm, awsConfig::awsCredentialsProvider)));

Review Comment:
   Is it not easier to cache a runtime-global `DefaultCredentialsProvider` (for `awsConfig::awsCredentialsProvider()`)? It is shared across all realms ๐Ÿค” 



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reuse AWS service identity credentials across requests for SigV4 federation [polaris]

Posted by "harshitaajoshi (via GitHub)" <gi...@apache.org>.
harshitaajoshi commented on PR #5869:
URL: https://github.com/apache/polaris/pull/5869#issuecomment-6434143636

   Thanks for the review! On the empty result test: since the cache now only holds providers and the factory always returns one, there is no empty case left, so I dropped that javadoc line instead of adding a test.
   
   @vigneshio the changes since your approval are limited to these two points, would you mind taking another look?


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reuse AWS service identity credentials across requests for SigV4 federation [polaris]

Posted by "harshitaajoshi (via GitHub)" <gi...@apache.org>.
harshitaajoshi commented on code in PR #5869:
URL: https://github.com/apache/polaris/pull/5869#discussion_r4385229385


##########
runtime/service/src/main/java/org/apache/polaris/service/identity/provider/ServiceIdentityCredentialCache.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.identity.provider;
+
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+import org.apache.polaris.core.identity.ServiceIdentityType;
+import org.apache.polaris.core.identity.credential.AwsIamServiceIdentityCredential;
+import org.apache.polaris.core.identity.credential.ServiceIdentityCredential;
+import software.amazon.awssdk.utils.SdkAutoCloseable;
+
+/**
+ * Application-scoped cache of resolved {@link ServiceIdentityCredential}s, keyed by the realm whose
+ * configuration they were resolved from and their identity type.
+ *
+ * <p>{@link DefaultServiceIdentityProvider} is request scoped, so without this cache every request
+ * would build a new credential and, with it, a new AWS credentials provider. With the AWS default
+ * credential chain (e.g. web identity on EKS), each provider instance owns its own STS client and
+ * credential cache, so a new instance per request means a new STS call per request. Service
+ * identity configuration is fixed at startup, so a credential resolved for a reference stays valid
+ * for the lifetime of the application, and the SDK provider refreshes the underlying session
+ * credentials itself.
+ */
+@ApplicationScoped
+public class ServiceIdentityCredentialCache {
+
+  private record Key(String realm, ServiceIdentityType type) {}
+
+  private final ConcurrentMap<Key, ServiceIdentityCredential> credentials =

Review Comment:
   Good call, done in be80e9bd15. The cache now holds only the `AwsCredentialsProvider` per realm, and `DefaultServiceIdentityProvider` builds a fresh `AwsIamServiceIdentityCredential` on every call, so nothing mutable is shared across requests. The reuse test also asserts the credential objects are distinct.



##########
runtime/service/src/main/java/org/apache/polaris/service/identity/provider/ServiceIdentityCredentialCache.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.identity.provider;
+
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+import org.apache.polaris.core.identity.ServiceIdentityType;
+import org.apache.polaris.core.identity.credential.AwsIamServiceIdentityCredential;
+import org.apache.polaris.core.identity.credential.ServiceIdentityCredential;
+import software.amazon.awssdk.utils.SdkAutoCloseable;
+
+/**
+ * Application-scoped cache of resolved {@link ServiceIdentityCredential}s, keyed by the realm whose
+ * configuration they were resolved from and their identity type.
+ *
+ * <p>{@link DefaultServiceIdentityProvider} is request scoped, so without this cache every request
+ * would build a new credential and, with it, a new AWS credentials provider. With the AWS default
+ * credential chain (e.g. web identity on EKS), each provider instance owns its own STS client and
+ * credential cache, so a new instance per request means a new STS call per request. Service
+ * identity configuration is fixed at startup, so a credential resolved for a reference stays valid
+ * for the lifetime of the application, and the SDK provider refreshes the underlying session
+ * credentials itself.
+ */
+@ApplicationScoped
+public class ServiceIdentityCredentialCache {
+
+  private record Key(String realm, ServiceIdentityType type) {}
+
+  private final ConcurrentMap<Key, ServiceIdentityCredential> credentials =
+      new ConcurrentHashMap<>();
+
+  /**
+   * Returns the credential cached for the given realm and identity type, resolving it with {@code
+   * resolver} on first use. Empty results are not cached.
+   */
+  Optional<ServiceIdentityCredential> getOrResolve(
+      String realm,
+      ServiceIdentityType type,
+      Supplier<Optional<? extends ServiceIdentityCredential>> resolver) {
+    return Optional.ofNullable(
+        credentials.computeIfAbsent(new Key(realm, type), key -> resolver.get().orElse(null)));
+  }
+
+  @PreDestroy
+  void shutdown() {
+    credentials
+        .values()
+        .forEach(
+            credential -> {
+              if (credential instanceof AwsIamServiceIdentityCredential awsCredential
+                  && awsCredential.getAwsCredentialsProvider()
+                      instanceof SdkAutoCloseable closeable) {
+                closeable.close();

Review Comment:
   Done. Each `close()` is wrapped in a try/catch that logs a warning and continues, and `clear()` always runs. Added a test where one provider throws on close and the other is still closed.



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reuse AWS service identity credentials across requests for SigV4 federation [polaris]

Posted by "orca-review-bot (via GitHub)" <gi...@apache.org>.
orca-review-bot commented on code in PR #5869:
URL: https://github.com/apache/polaris/pull/5869#discussion_r4382257594


##########
runtime/service/src/main/java/org/apache/polaris/service/identity/provider/ServiceIdentityCredentialCache.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.identity.provider;
+
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+import org.apache.polaris.core.identity.ServiceIdentityType;
+import org.apache.polaris.core.identity.credential.AwsIamServiceIdentityCredential;
+import org.apache.polaris.core.identity.credential.ServiceIdentityCredential;
+import software.amazon.awssdk.utils.SdkAutoCloseable;
+
+/**
+ * Application-scoped cache of resolved {@link ServiceIdentityCredential}s, keyed by the realm whose
+ * configuration they were resolved from and their identity type.
+ *
+ * <p>{@link DefaultServiceIdentityProvider} is request scoped, so without this cache every request
+ * would build a new credential and, with it, a new AWS credentials provider. With the AWS default
+ * credential chain (e.g. web identity on EKS), each provider instance owns its own STS client and
+ * credential cache, so a new instance per request means a new STS call per request. Service
+ * identity configuration is fixed at startup, so a credential resolved for a reference stays valid
+ * for the lifetime of the application, and the SDK provider refreshes the underlying session
+ * credentials itself.
+ */
+@ApplicationScoped
+public class ServiceIdentityCredentialCache {
+
+  private record Key(String realm, ServiceIdentityType type) {}
+
+  private final ConcurrentMap<Key, ServiceIdentityCredential> credentials =

Review Comment:
   The cached `ServiceIdentityCredential` is now one object shared by every request, and every realm, on this key. It has a public `setIdentityInfoReference`. Nothing calls it today, so this is fine for now. But the first caller that does would change the credential everyone else sees. Since the provider is the only expensive part, can we cache just the `AwsCredentialsProvider` and build a fresh credential wrapper per call? You offered this in the PR description, and I'd take it.
   
   <sub>๐Ÿค– AI-generated review comment โ€” agents make mistakes, so please verify before acting on it.</sub>



##########
runtime/service/src/main/java/org/apache/polaris/service/identity/provider/ServiceIdentityCredentialCache.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.polaris.service.identity.provider;
+
+import jakarta.annotation.PreDestroy;
+import jakarta.enterprise.context.ApplicationScoped;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Supplier;
+import org.apache.polaris.core.identity.ServiceIdentityType;
+import org.apache.polaris.core.identity.credential.AwsIamServiceIdentityCredential;
+import org.apache.polaris.core.identity.credential.ServiceIdentityCredential;
+import software.amazon.awssdk.utils.SdkAutoCloseable;
+
+/**
+ * Application-scoped cache of resolved {@link ServiceIdentityCredential}s, keyed by the realm whose
+ * configuration they were resolved from and their identity type.
+ *
+ * <p>{@link DefaultServiceIdentityProvider} is request scoped, so without this cache every request
+ * would build a new credential and, with it, a new AWS credentials provider. With the AWS default
+ * credential chain (e.g. web identity on EKS), each provider instance owns its own STS client and
+ * credential cache, so a new instance per request means a new STS call per request. Service
+ * identity configuration is fixed at startup, so a credential resolved for a reference stays valid
+ * for the lifetime of the application, and the SDK provider refreshes the underlying session
+ * credentials itself.
+ */
+@ApplicationScoped
+public class ServiceIdentityCredentialCache {
+
+  private record Key(String realm, ServiceIdentityType type) {}
+
+  private final ConcurrentMap<Key, ServiceIdentityCredential> credentials =
+      new ConcurrentHashMap<>();
+
+  /**
+   * Returns the credential cached for the given realm and identity type, resolving it with {@code
+   * resolver} on first use. Empty results are not cached.
+   */
+  Optional<ServiceIdentityCredential> getOrResolve(
+      String realm,
+      ServiceIdentityType type,
+      Supplier<Optional<? extends ServiceIdentityCredential>> resolver) {
+    return Optional.ofNullable(
+        credentials.computeIfAbsent(new Key(realm, type), key -> resolver.get().orElse(null)));
+  }
+
+  @PreDestroy
+  void shutdown() {
+    credentials
+        .values()
+        .forEach(
+            credential -> {
+              if (credential instanceof AwsIamServiceIdentityCredential awsCredential
+                  && awsCredential.getAwsCredentialsProvider()
+                      instanceof SdkAutoCloseable closeable) {
+                closeable.close();

Review Comment:
   If one provider's `close()` throws, the remaining providers don't get closed and `clear()` never runs. Can we wrap this in a try/catch that logs and keeps going?
   
   <sub>๐Ÿค– AI-generated review comment โ€” agents make mistakes, so please verify before acting on it.</sub>



-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org