AWS: Refresh vended credentials #11389

nastra · 2024-10-24T14:15:35Z

No description provided.

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

nastra · 2024-10-24T14:29:56Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+import software.amazon.awssdk.utils.cache.CachedSupplier;
+import software.amazon.awssdk.utils.cache.RefreshResult;
+
+public class VendedCredentialsProvider implements AwsCredentialsProvider, SdkAutoCloseable {


@jackye1995 is this credential provider similar to the one you mentioned a while ago where you guys had a custom credential provider that would always call loadTable()?

Yes this looks similar.

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

danielcweeks

Minor comments, but +1

singhpk234 · 2024-10-24T17:58:07Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+    Optional<Credential> credentialWithPrefix =
+        s3Credentials.stream().max(Comparator.comparingInt(c -> c.prefix().length()));


[doubt] how will this work for credentials which are expanding different prefixes :
for ex :
s3//abc/123
s3//xyz/1

and the request is for s3://xyz ?

Never the less how will this work for cases with same prefixes :

s3://abc/prefix-1/

s3://abc/prefix-123/
we will use prefix-123 here instead of prefix-1 ? for the calls length is not the correct way to find the credentials imho

please let me know your thoughts.

This is actually a good callout and I think an oversight from what we're trying to define in the spec. Here we are taking the most selective prefix, but that doesn't necessarily correspond to the prefix we're using the credential against.

In order to get the behavior we're expecting we would probably need to update S3FileIO::client to take the path as an argument so that the client can be configured/returned with the correct credential. Thoughts @nastra? I assume there is a similar issue on the GCP side.

@singhpk234 to answer your specific question, it should fail because there is no prefix in your example that covers the requested path.

@danielcweeks got you, This is always been a challenge when using credential provider as if you see it's resolveCredenitials() doesn't take any args so to wire in correct creds for correct paths, which we might not
know at the time of creating S3 client

Yeah the credential to use is the credential with the longest matching prefix for a given requested path. In the case that there's no credential with a matching prefix for a given path, I think we'd want to just fail on the client side; s3 or whatever storage system would fail the request anyways but I think we may as well avoid the request if we know that no credential could cover the requested path.

I think what we rather might want to do going forward is only allow the server to return a single credential per provider. That way it's easier for the client

Hm, sure that's easier for the client but I'm not sure about making the protocol unnecessarily restrictive. Though I do understand in most cases a single credential per provider is enough, I just want to make sure we're not putting the spec in an awkward spot in case there are legitimate use cases of different credentials per prefix. In general, we've always followed a pattern of avoiding putting unnecessary restrictions on protocol. Also I don't know if it's really that much more complex for clients to resolve the longest matching prefix for a given path

I agree that it's reasonable to enforce a single prefix for provider since that was the initial ask. There's an opportunity to improve this, but we can follow up with added support. I don't think it's too difficult, but there are a few edge cases that we'd need to work through. I'm ok proceeding with this implementation.

I think I'm good as long as this is just a constraint in the current implementation and we're not requiring/changing any server side expectations in the spec

Need to revisit prefix selection.

ChaladiMohanVamsi · 2024-10-25T08:52:28Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+            LoadCredentialsResponse.class,
+            OAuth2Util.authHeaders(properties.get(OAuth2Properties.TOKEN)),


Assuming the Token here is same short lived token used by RestCatalog instance, are we planning to handle token refresh post its expiration with in the FileIO?

this is indeed an issue that I was planning to address when I wrote the first version of this many months ago but simply forgot to get back to. thanks for raising this @ChaladiMohanVamsi

I think for the initial support for refresh, we can assume that we're bound by the token lifetime for refreshes. It's not perfect, but I'd like to see the resolution of some of the AuthManger refactor before settling on a solution. We're not regressing at this point, so I'm ok with leaving this as is and addressing in the future.

FYI, this is on my todo list for after the AuthManager refactor.

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

amogh-jahagirdar · 2024-10-25T14:59:36Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+    Optional<Credential> credentialWithPrefix =
+        s3Credentials.stream().max(Comparator.comparingInt(c -> c.prefix().length()));


Yeah the credential to use is the credential with the longest matching prefix for a given requested path. In the case that there's no credential with a matching prefix for a given path, I think we'd want to just fail on the client side; s3 or whatever storage system would fail the request anyways but I think we may as well avoid the request if we know that no credential could cover the requested path.

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

jackye1995 · 2024-10-28T19:17:33Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+    return client;
+  }
+
+  private LoadCredentialsResponse fetchCredentials() {


I think there are 2 concepts that would be beneficial to be added, a staleTime and a prefetchTime. You could check the AWS SDK StsCredentialsProvider for how that is implemented. But this prevents edge cases like the credentials is loaded at almost the expiration time and cause errors downstream.

this is already being done further below:

return RefreshResult.builder( (AwsCredentials) AwsSessionCredentials.builder() .accessKeyId(accessKeyId) .secretAccessKey(secretAccessKey) .sessionToken(sessionToken) .expirationTime(expiresAt) .build()) .staleTime(expiresAt) .prefetchTime(prefetchAt) .build();

jackye1995 · 2024-10-28T19:18:25Z

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

+   * <p>When set, the {@link VendedCredentialsProvider} will be used to fetch and refresh vended
+   * credentials.
+   */
+  public static final String REFRESH_CREDENTIALS_ENDPOINT = "client.refresh-credentials-endpoint";


shouldn't this be a s3 FileIO property?

This property needs to be set before FileIO is actually being configured and is similar to the client region. VendedCredentialsProvider is configured when AwsClientProperties#credentialsProvider(..) is called, so I don't think this property should be a FileIO property

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

RussellSpitzer · 2024-10-29T21:11:03Z

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java

@@ -136,6 +156,12 @@ public <T extends AwsClientBuilder> void applyClientCredentialConfigurations(T b
  @SuppressWarnings("checkstyle:HiddenField")
  public AwsCredentialsProvider credentialsProvider(
      String accessKeyId, String secretAccessKey, String sessionToken) {
+    if (refreshCredentialsEnabled && !Strings.isNullOrEmpty(refreshCredentialsEndpoint)) {


nit: Should we log a warning if the endpoint is set but refreshCredentialsEnabled is false? I don't think we should fail but this is probably something a user would want to be aware of.

I'm not sure adding a warning adds a lot of value. It's valid to have the server send you back an endpoint + refresh enabled flag that you then override for cases like Kafka connect. I'd say let's go without a warning for now unless this becomes a place of confusion

RussellSpitzer

This looks fine to me, I left a few nits but mainly I would like someone who is closer to this part of the code to also approve this PR before we merge.

danielcweeks

I think there are two future improvements that we've identified (catalog token refresh and multi-prefix support within cloud provider), but this support goes a long way to improving current use cases and even enables new ones, so I'm +1.

danielcweeks · 2024-10-29T23:07:09Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+            LoadCredentialsResponse.class,
+            OAuth2Util.authHeaders(properties.get(OAuth2Properties.TOKEN)),


I think for the initial support for refresh, we can assume that we're bound by the token lifetime for refreshes. It's not perfect, but I'd like to see the resolution of some of the AuthManger refactor before settling on a solution. We're not regressing at this point, so I'm ok with leaving this as is and addressing in the future.

danielcweeks · 2024-10-29T23:09:34Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+    Optional<Credential> credentialWithPrefix =
+        s3Credentials.stream().max(Comparator.comparingInt(c -> c.prefix().length()));


I agree that it's reasonable to enforce a single prefix for provider since that was the initial ask. There's an opportunity to improve this, but we can follow up with added support. I don't think it's too difficult, but there are a few edge cases that we'd need to work through. I'm ok proceeding with this implementation.

singhpk234 · 2024-10-30T16:23:28Z

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java

+    List<Credential> s3Credentials =
+        response.credentials().stream()
+            .filter(c -> c.prefix().startsWith("s3"))
+            .collect(Collectors.toList());


This imho still doesn't addresses the problem of wiring the credential for the right prefix ?
for ex : S3 rest server returned prefix as "s3://bucket/prefix-1"
but the call was for "s3://bucket/prefix-2", unless there is an enforcement from REST "that it will return only longest common prefix in the response"

I would recommend rather than using starting with "S3" let make it equal to "S3" for now to make sure the client doesn't mess around, what do you think ?

@danielcweeks @RussellSpitzer @amogh-jahagirdar @nastra

@singhpk234 for how the implementation enforces that there's really only a single credential being sent back by the server. I'll be working on supporting and selecting the "right" credential when the server sents back multiple in a follow-up

for how the implementation enforces that there's really only a single credential being sent back by the server

[doubt] how are we enforcing this ? for ex a rest server can send a credential for only one prefix but still for a diff prefix than what being asked for, are you suggesting that for now, rest server should only send back for exactly "s3" prefix ? if yes how are we enforcing this in rest for the meanwhile ?
Is strategy for now to let if fail ?

The catalog has the responsibility of returning a credential with a scoped policy that provides the appropriate access for all prefixes. This isn't about the spec or client trying to enforce that behavior. Either the client will have access or not, that's up to the catalog.

The catalog has the responsibility of returning a credential with a scoped policy that provides the appropriate access for all prefixes

I see, so that fact that only one credential is being returned from the catalog, it itself means that its best prefix that could fit in into all the request the client is allowed make. Make sense ! so we would indeed fail at client trying for an un-acessible prefix from S3 end.

nastra requested a review from danielcweeks October 24, 2024 14:15

github-actions bot added the AWS label Oct 24, 2024

nastra commented Oct 24, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java Show resolved Hide resolved

nastra force-pushed the s3-refresh-vended-credentials branch from 1e200fd to 5e51c5b Compare October 24, 2024 14:28

nastra commented Oct 24, 2024

View reviewed changes

nastra force-pushed the s3-refresh-vended-credentials branch from 5e51c5b to 51424d1 Compare October 24, 2024 14:39

nastra requested a review from jackye1995 October 24, 2024 14:41

danielcweeks reviewed Oct 24, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/S3FileIOProperties.java Outdated Show resolved Hide resolved

danielcweeks reviewed Oct 24, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java Show resolved Hide resolved

danielcweeks previously approved these changes Oct 24, 2024

View reviewed changes

singhpk234 reviewed Oct 24, 2024

View reviewed changes

nastra force-pushed the s3-refresh-vended-credentials branch from 51424d1 to 0551ea6 Compare October 25, 2024 07:20

ChaladiMohanVamsi reviewed Oct 25, 2024

View reviewed changes

nastra closed this Oct 25, 2024

nastra reopened this Oct 25, 2024

amogh-jahagirdar reviewed Oct 25, 2024

View reviewed changes

danielcweeks added this to the Iceberg 1.7.0 milestone Oct 25, 2024

nastra force-pushed the s3-refresh-vended-credentials branch from 0551ea6 to 7ce08bd Compare October 28, 2024 08:27

ChaladiMohanVamsi reviewed Oct 28, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/s3/VendedCredentialsProvider.java Show resolved Hide resolved

jackye1995 reviewed Oct 28, 2024

View reviewed changes

nastra removed this from the Iceberg 1.7.0 milestone Oct 29, 2024

nastra added 3 commits October 29, 2024 12:10

AWS: Refresh vended credentials

4f5de90

only allow one S3 credential

11c5748

fetch token when a credential was given before retrieving vended creds

35f9211

nastra force-pushed the s3-refresh-vended-credentials branch from a770941 to a5b58ed Compare October 29, 2024 16:16

nastra added this to the Iceberg 1.7.0 milestone Oct 29, 2024

nastra requested a review from danielcweeks October 29, 2024 16:19

RussellSpitzer reviewed Oct 29, 2024

View reviewed changes

aws/src/main/java/org/apache/iceberg/aws/AwsClientProperties.java Outdated Show resolved Hide resolved

RussellSpitzer reviewed Oct 29, 2024

View reviewed changes

RussellSpitzer approved these changes Oct 29, 2024

View reviewed changes

danielcweeks approved these changes Oct 29, 2024

View reviewed changes

danielcweeks mentioned this pull request Oct 29, 2024

GCS: Refresh vended credentials #11282

Merged

Use OAuth token from properties for now

41fe921

nastra force-pushed the s3-refresh-vended-credentials branch from a5b58ed to 41fe921 Compare October 30, 2024 06:05

nastra closed this Oct 30, 2024

nastra reopened this Oct 30, 2024

nastra merged commit 9e895cb into apache:main Oct 30, 2024
98 checks passed

nastra deleted the s3-refresh-vended-credentials branch October 30, 2024 09:29

singhpk234 reviewed Oct 30, 2024

View reviewed changes

ChaladiMohanVamsi mentioned this pull request Nov 18, 2024

Azure: Support vended credentials refresh in ADLSFileIO. #11577

Open

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

AWS: Refresh vended credentials (apache#11389)

2010347

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS: Refresh vended credentials #11389

AWS: Refresh vended credentials #11389

nastra commented Oct 24, 2024

nastra Oct 24, 2024 •

edited

Loading

jackye1995 Oct 28, 2024

danielcweeks left a comment

singhpk234 Oct 24, 2024 •

edited

Loading

danielcweeks Oct 24, 2024 •

edited

Loading

danielcweeks Oct 24, 2024

danielcweeks Oct 24, 2024

singhpk234 Oct 24, 2024 •

edited

Loading

amogh-jahagirdar Oct 25, 2024

nastra Oct 28, 2024

amogh-jahagirdar Oct 29, 2024

danielcweeks Oct 29, 2024

amogh-jahagirdar Oct 30, 2024 •

edited

Loading

ChaladiMohanVamsi Oct 25, 2024

nastra Oct 25, 2024

danielcweeks Oct 29, 2024

adutra Dec 4, 2024

amogh-jahagirdar Oct 25, 2024

jackye1995 Oct 28, 2024

nastra Oct 29, 2024

jackye1995 Oct 28, 2024

nastra Oct 29, 2024

RussellSpitzer Oct 29, 2024

nastra Oct 30, 2024

RussellSpitzer left a comment

danielcweeks left a comment

danielcweeks Oct 29, 2024

danielcweeks Oct 29, 2024

singhpk234 Oct 30, 2024

nastra Oct 30, 2024

singhpk234 Oct 30, 2024 •

edited

Loading

danielcweeks Oct 31, 2024

singhpk234 Oct 31, 2024 •

edited

Loading

		Optional<Credential> credentialWithPrefix =
		s3Credentials.stream().max(Comparator.comparingInt(c -> c.prefix().length()));

		LoadCredentialsResponse.class,
		OAuth2Util.authHeaders(properties.get(OAuth2Properties.TOKEN)),

AWS: Refresh vended credentials #11389

AWS: Refresh vended credentials #11389

Conversation

nastra commented Oct 24, 2024

nastra Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielcweeks left a comment

Choose a reason for hiding this comment

singhpk234 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

danielcweeks Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amogh-jahagirdar Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussellSpitzer left a comment

Choose a reason for hiding this comment

danielcweeks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 Oct 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

nastra Oct 24, 2024 •

edited

Loading

singhpk234 Oct 24, 2024 •

edited

Loading

danielcweeks Oct 24, 2024 •

edited

Loading

singhpk234 Oct 24, 2024 •

edited

Loading

amogh-jahagirdar Oct 30, 2024 •

edited

Loading

singhpk234 Oct 30, 2024 •

edited

Loading

singhpk234 Oct 31, 2024 •

edited

Loading