Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add stale time config to InstanceProfileCredentialsProvider #5758

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ public final class InstanceProfileCredentialsProvider

private final String profileName;

private final Duration staleTime;

/**
* @see #builder()
*/
Expand All @@ -108,6 +110,8 @@ private InstanceProfileCredentialsProvider(BuilderImpl builder) {
.profileName(profileName)
.build();

this.staleTime = Validate.getOrDefault(builder.staleTime, () -> Duration.ofSeconds(1));

if (Boolean.TRUE.equals(builder.asyncCredentialUpdateEnabled)) {
Validate.paramNotBlank(builder.asyncThreadName, "asyncThreadName");
this.credentialsCache = CachedSupplier.builder(this::refreshCredentials)
Expand Down Expand Up @@ -174,7 +178,7 @@ private Instant staleTime(Instant expiration) {
return null;
}

return expiration.minusSeconds(1);
return expiration.minus(staleTime);
Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to understand comment in #3314 (comment)

Looks like the 1 sec is intentional , if a user configures an excessively large stale time (e.g., expiration.minusMinutes(10)), credentials might be marked as stale too early, causing unnecessary refreshes and increased load on the credential provider. WDYT ?

}

private Instant prefetchTime(Instant expiration) {
Expand Down Expand Up @@ -340,6 +344,13 @@ public interface Builder extends HttpCredentialsProvider.Builder<InstanceProfile
*/
Builder profileName(String profileName);

/**
* Configure the amount of time before the moment of expiration of credentials for which to consider the credentials to
* be stale. A higher value can lead to a higher rate of request being made. The default is 1 sec.
* @param duration the amount of time before expiration for when to consider the credentials to be stale and need refresh
*/
Builder staleTime(Duration duration);

/**
* Build a {@link InstanceProfileCredentialsProvider} from the provided configuration.
*/
Expand All @@ -355,6 +366,7 @@ static final class BuilderImpl implements Builder {
private String asyncThreadName;
private Supplier<ProfileFile> profileFile;
private String profileName;
private Duration staleTime;

private BuilderImpl() {
asyncThreadName("instance-profile-credentials-provider");
Expand All @@ -367,6 +379,7 @@ private BuilderImpl(InstanceProfileCredentialsProvider provider) {
this.asyncThreadName = provider.asyncThreadName;
this.profileFile = provider.profileFile;
this.profileName = provider.profileName;
this.staleTime = provider.staleTime;
}

Builder clock(Clock clock) {
Expand Down Expand Up @@ -435,6 +448,16 @@ public void setProfileName(String profileName) {
profileName(profileName);
}

@Override
public Builder staleTime(Duration duration) {
this.staleTime = duration;
return this;
}

public void setStaleTime(Duration duration) {
staleTime(duration);
}

@Override
public InstanceProfileCredentialsProvider build() {
return new InstanceProfileCredentialsProvider(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@

import java.io.IOException;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.auth.credentials.ContainerCredentialsProvider;
import software.amazon.awssdk.auth.credentials.InstanceProfileCredentialsProvider;
import software.amazon.awssdk.http.HttpStatusFamily;
import software.amazon.awssdk.regions.util.ResourcesEndpointRetryParameters;
import software.amazon.awssdk.regions.util.ResourcesEndpointRetryPolicy;

/**
* Retry policy shared by {@link InstanceProfileCredentialsProvider} and {@link ContainerCredentialsProvider#}.
*/
@SdkInternalApi
public final class ContainerCredentialsRetryPolicy implements ResourcesEndpointRetryPolicy {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.Optional;
import software.amazon.awssdk.annotations.SdkInternalApi;
import software.amazon.awssdk.regions.util.ResourcesEndpointProvider;
import software.amazon.awssdk.regions.util.ResourcesEndpointRetryPolicy;
import software.amazon.awssdk.utils.Validate;

@SdkInternalApi
Expand Down Expand Up @@ -58,6 +59,11 @@ public Map<String, String> headers() {
return Collections.unmodifiableMap(headers);
}

@Override
public ResourcesEndpointRetryPolicy retryPolicy() {
return new ContainerCredentialsRetryPolicy();
}
Comment on lines +63 to +65
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can add retry at this point for IMDS credentials provider because it may increase latency and break customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, that was a ask from Hadoop. Instead we can add it as an optional config as well


public static class Builder {
private URI endpoint;
private Map<String, String> additionalHeaders = new HashMap<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -596,13 +596,62 @@ void imdsCallFrequencyIsLimited() {
}
}

@Test
void testErrorWhileCacheIsStale_shouldRecover() {
AdjustableClock clock = new AdjustableClock();

Instant now = Instant.now();
Instant expiration = now.plus(Duration.ofHours(6));

String successfulCredentialsResponse =
"{"
+ "\"AccessKeyId\":\"ACCESS_KEY_ID\","
+ "\"SecretAccessKey\":\"SECRET_ACCESS_KEY\","
+ "\"Expiration\":\"" + DateUtils.formatIso8601Date(expiration) + '"'
+ "}";

String staleResponse =
"{"
+ "\"AccessKeyId\":\"ACCESS_KEY_ID_2\","
+ "\"SecretAccessKey\":\"SECRET_ACCESS_KEY_2\","
+ "\"Expiration\":\"" + DateUtils.formatIso8601Date(Instant.now()) + '"'
+ "}";


Duration staleTime = Duration.ofMinutes(5);
AwsCredentialsProvider provider = credentialsProviderWithClock(clock, staleTime);

// cache expiration with expiration = 6 hours
clock.time = now;
stubSecureCredentialsResponse(aResponse().withBody(successfulCredentialsResponse));
AwsCredentials validCreds = provider.resolveCredentials();

// failure while cache is stale
clock.time = expiration.minus(staleTime.minus(Duration.ofMinutes(2)));
stubTokenFetchErrorResponse(aResponse().withFixedDelay(2000).withBody(STUB_CREDENTIALS), 500);
stubSecureCredentialsResponse(aResponse().withBody(staleResponse));
AwsCredentials refreshedWhileStale = provider.resolveCredentials();

assertThat(refreshedWhileStale).isNotEqualTo(validCreds);
assertThat(refreshedWhileStale.secretAccessKey()).isEqualTo("SECRET_ACCESS_KEY_2");
}

private AwsCredentialsProvider credentialsProviderWithClock(Clock clock) {
InstanceProfileCredentialsProvider.BuilderImpl builder =
(InstanceProfileCredentialsProvider.BuilderImpl) InstanceProfileCredentialsProvider.builder();
builder.clock(clock);
return builder.build();
}

private AwsCredentialsProvider credentialsProviderWithClock(Clock clock, Duration staleTime) {
InstanceProfileCredentialsProvider.BuilderImpl builder =
(InstanceProfileCredentialsProvider.BuilderImpl) InstanceProfileCredentialsProvider.builder();
builder.clock(clock);
builder.staleTime(staleTime);
return builder.build();
}


private static class AdjustableClock extends Clock {
private Instant time;

Expand Down
6 changes: 6 additions & 0 deletions core/auth/src/test/resources/log4j2.properties
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,9 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender
#
#logger.netty.name = io.netty.handler.logging
#logger.netty.level = debug

#logger.cache.name = software.amazon.awssdk.utils.cache.CachedSupplier
#logger.cache.level = DEBUG

#logger.instance.name = software.amazon.awssdk.auth.credentials.InstanceProfileCredentialsProvider
#logger.instance.level = DEBUG
Loading