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.

Sign up for GitHub

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

Fix reload credentials not working when modified - external PR : #5185 #5259

Closed
wants to merge 15 commits into from
Closed

Fix reload credentials not working when modified - external PR : #5185 #5259

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
91c6a1f
Creating a pr from a branch for external PR 5185; as it cannot load c…
anirudh9391 May 31, 2024
94d8401
Fixed indendation
anirudh9391 May 31, 2024
9295eb7
Fix indendation
anirudh9391 Jun 1, 2024
9327518
Initializing with aggregator
anirudh9391 Jun 4, 2024
776aa6b
Fixed indendation
anirudh9391 Jun 4, 2024
ba3c6b5
Merge branch 'master' into anirudkr/reload-credential-fix-muendrasan
anirudh9391 Jun 4, 2024
9ab0b47
Refactor ProfileSupplier instantiation
anirudh9391 Jun 4, 2024
7ca7075
initialized profile supplier to null
anirudh9391 Jun 11, 2024
6681fab
Merge branch 'master' into anirudkr/reload-credential-fix-muendrasan
anirudh9391 Jun 11, 2024
87bd23b
Merge branch 'master' into anirudkr/reload-credential-fix-muendrasan
anirudh9391 Jun 18, 2024
032a2e5
Increasing timeout value
anirudh9391 Jun 18, 2024
536548c
increased thread sleep duration
anirudh9391 Jun 18, 2024
a469d8a
Added retry
anirudh9391 Jun 19, 2024
badc28c
Merge branch 'master' into anirudkr/reload-credential-fix-muendrasan
anirudh9391 Jul 1, 2024
2b330db
Merge branch 'master' into anirudkr/reload-credential-fix-muendrasan
L-Applin Jul 22, 2024
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
4 changes: 2 additions & 2 deletions ...main/java/software/amazon/awssdk/auth/credentials/InstanceProfileCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ private InstanceProfileCredentialsProvider(BuilderImpl builder) {
this.asyncCredentialUpdateEnabled = builder.asyncCredentialUpdateEnabled;
this.asyncThreadName = builder.asyncThreadName;
this.profileFile = Optional.ofNullable(builder.profileFile)
.orElseGet(() -> ProfileFileSupplier.fixedProfileFile(ProfileFile.defaultProfileFile()));
.orElseGet(ProfileFileSupplier::defaultSupplier);
this.profileName = Optional.ofNullable(builder.profileName)
.orElseGet(ProfileFileSystemSetting.AWS_PROFILE::getStringValueOrThrow);

Expand Down Expand Up @@ -311,7 +311,7 @@ public interface Builder extends HttpCredentialsProvider.Builder<InstanceProfile
* Configure the profile file used for loading IMDS-related configuration, like the endpoint mode (IPv4 vs IPv6).
*
* <p>By default, {@link ProfileFile#defaultProfileFile()} is used.
*
*
* @see #profileFile(Supplier)
*/
Builder profileFile(ProfileFile profileFile);
Expand Down
6 changes: 3 additions & 3 deletions ...uth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ private ProfileCredentialsProvider(BuilderImpl builder) {
.orElseGet(ProfileFileSystemSetting.AWS_PROFILE::getStringValueOrThrow);
selectedProfileSupplier =
Optional.ofNullable(builder.profileFile)
.orElseGet(() -> ProfileFileSupplier.fixedProfileFile(builder.defaultProfileFileLoader.get()));
.orElseGet(() -> builder.defaultProfileFileLoader);

} catch (RuntimeException e) {
// If we couldn't load the credentials provider for some reason, save an exception describing why. This exception
Expand Down Expand Up @@ -196,7 +196,7 @@ public interface Builder extends CopyableBuilder<Builder, ProfileCredentialsProv
* Define the mechanism for loading profile files.
*
* @param profileFileSupplier Supplier interface for generating a ProfileFile instance.
* @see #profileFile(ProfileFile)
* @see #profileFile(ProfileFile)
*/
Builder profileFile(Supplier<ProfileFile> profileFileSupplier);

Expand All @@ -216,7 +216,7 @@ public interface Builder extends CopyableBuilder<Builder, ProfileCredentialsProv
static final class BuilderImpl implements Builder {
private Supplier<ProfileFile> profileFile;
private String profileName;
private Supplier<ProfileFile> defaultProfileFileLoader = ProfileFile::defaultProfileFile;
private Supplier<ProfileFile> defaultProfileFileLoader = ProfileFileSupplier.defaultSupplier();

BuilderImpl() {
}
Expand Down
48 changes: 48 additions & 0 deletions ...src/test/java/software/amazon/awssdk/auth/credentials/DefaultCredentialsProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,26 @@

import static org.assertj.core.api.AssertionsForClassTypes.assertThat;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.function.Supplier;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
import software.amazon.awssdk.profiles.ProfileFile;
import software.amazon.awssdk.profiles.ProfileFileSupplier;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
import software.amazon.awssdk.utils.StringInputStream;
import software.amazon.awssdk.utils.internal.SystemSettingUtilsTestBackdoor;

class DefaultCredentialsProviderTest {

@TempDir
private Path testDirectory;

@Test
void resolveCredentials_ProfileCredentialsProviderWithProfileFile_returnsCredentials() {
DefaultCredentialsProvider provider = DefaultCredentialsProvider
Expand Down Expand Up @@ -97,6 +107,44 @@ void resolveCredentials_ProfileCredentialsProviderWithSupplierProfileFile_return
});
}

@Test
void resolveCredentials_DefaultCredentialProviderWithReloadWhenModified() throws Exception {
Path credentialsFilePath = generateTestCredentialsFile("customAccess", "customSecret");
SystemSettingUtilsTestBackdoor.addEnvironmentVariableOverride(ProfileFileSystemSetting.AWS_SHARED_CREDENTIALS_FILE.environmentVariable(),
credentialsFilePath.toString());
DefaultCredentialsProvider provider = DefaultCredentialsProvider.create();

assertThat(provider.resolveCredentials()).satisfies(awsCredentials -> {
assertThat(awsCredentials.accessKeyId()).isEqualTo("customAccess");
assertThat(awsCredentials.secretAccessKey()).isEqualTo("customSecret");
});

generateTestCredentialsFile("modifiedAccess", "modifiedSecret");

// without sleep the assertion fails, as there is delay in config reload
Thread.sleep(2000);
Copy link

@munendrasn munendrasn Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@anirudh9391 For Java8, the test is failing pos this, had included delay to ensure latest config is picked up, but it is failing occasionally,

Error:    DefaultCredentialsProviderTest.resolveCredentials_DefaultCredentialProviderWithReloadWhenModified:126->lambda$resolveCredentials_DefaultCredentialProviderWithReloadWhenModified$7:127
expected: "modifiedAccess"
 but was: "customAccess"

Should we increase the sleep value? Is there any alternative way to validate this?

For Java17, the test failures seems to be unrelated & from s3 module

2024-06-11T16:42:29.7804890Z [ERROR] Errors:
2024-06-11T16:42:29.7805324Z [ERROR]   CrtDownloadErrorTest.verifyCrtResource:73 ? Runtime Timeout waiting for resource count to drop to zero
2024-06-11T16:42:29.7805829Z [ERROR]   CrtCredentialProviderAdapterTest.verifyCrtResource:43 ? Runtime Timeout waiting for resource count to drop to zero
2024-06-11T16:42:29.7806270Z [ERROR]   S3CrtClientWiremockTest.verifyCrtResource:92 ? Runtime Timeout waiting for resource count to drop to zero

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Java17 error seems to be transient in nature. It seems to succeed locally. I will play around with timeout values a bit.

Copy link

Choose a reason for hiding this comment

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

Due to the caching of ProfileFile for short duration,

aws-sdk-java-v2/utils/src/main/java/software/amazon/awssdk/utils/cache/CachedSupplier.java

Line 242 in fb7b6ea

Instant newStale = now.plusSeconds(1);

the credentials are not reloaded immediately, hence, Thread.sleep is required, added 2s sleep which in most cases sufficient, is not working in CI sometimes.
Here, sleep duration can be increased or assertion can retried with smaller timeout values

For reference, in ProfileRefresherTest

aws-sdk-java-v2/core/profiles/src/test/java/software/amazon/awssdk/profiles/internal/ProfileFileRefresherTest.java

Line 134 in fb7b6ea

Duration refreshInterval = Duration.ofSeconds(15);

Custom clock is used to advance the timer to validate refresh without sleep which might not be possible here, ie to set custom clock in ProfileSupplier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it with a 5 second delay. Seems to trip up the java21 and java8 builds, both of which fail assertion. Please feel free to cut another revision with any modifications to add retries or anything else. I think we can keep timeout at 5 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed another revision with a retry. Lets see if this works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like the retry failed all the java builds

Copy link

Choose a reason for hiding this comment

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

Thanks @anirudh9391 , will take a look. Going through the commit, I think retry should be added to second assertion, once credentials are modified

Copy link

Choose a reason for hiding this comment

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

Updated the retry logic, along with additional logging to debug the test, if it fails again in the commits e360e9d and f330457 respectively

Copy link

Choose a reason for hiding this comment

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

@anirudh9391 could you please cherry-pick latest commit for the fix, and further debugging?

assertThat(provider.resolveCredentials()).satisfies(awsCredentials -> {
assertThat(awsCredentials.accessKeyId()).isEqualTo("modifiedAccess");
assertThat(awsCredentials.secretAccessKey()).isEqualTo("modifiedSecret");
});
SystemSettingUtilsTestBackdoor.clearEnvironmentVariableOverrides();
}

private Path generateTestFile(String contents, String filename) {
try {
Files.createDirectories(testDirectory);
return Files.write(testDirectory.resolve(filename), contents.getBytes(StandardCharsets.UTF_8));
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private Path generateTestCredentialsFile(String accessKeyId, String secretAccessKey) {
String contents = String.format("[default]\naws_access_key_id = %s\naws_secret_access_key = %s\n",
accessKeyId, secretAccessKey);
return generateTestFile(contents, "credentials.txt");
}

private ProfileFile credentialFile(String credentialFile) {
return ProfileFile.builder()
.content(new StringInputStream(credentialFile))
Expand Down
4 changes: 3 additions & 1 deletion core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,15 @@ static ProfileFileSupplier defaultSupplier() {
= ProfileFileLocation.configurationFileLocation()
.map(path -> reloadWhenModified(path, ProfileFile.Type.CONFIGURATION));

ProfileFileSupplier supplier = () -> ProfileFile.builder().build();
ProfileFileSupplier supplier = null;
if (credentialsSupplierOptional.isPresent() && configurationSupplierOptional.isPresent()) {
supplier = aggregate(credentialsSupplierOptional.get(), configurationSupplierOptional.get());
} else if (credentialsSupplierOptional.isPresent()) {
supplier = credentialsSupplierOptional.get();
} else if (configurationSupplierOptional.isPresent()) {
supplier = configurationSupplierOptional.get();
} else {
supplier = fixedProfileFile(ProfileFile.aggregator().build());
}

return supplier;
Expand Down
4 changes: 3 additions & 1 deletion ...ore/src/main/java/software/amazon/awssdk/core/client/builder/SdkDefaultClientBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
import software.amazon.awssdk.identity.spi.IdentityProviders;
import software.amazon.awssdk.metrics.MetricPublisher;
import software.amazon.awssdk.profiles.ProfileFile;
import software.amazon.awssdk.profiles.ProfileFileSupplier;
import software.amazon.awssdk.profiles.ProfileFileSystemSetting;
import software.amazon.awssdk.profiles.ProfileProperty;
import software.amazon.awssdk.utils.AttributeMap;
Expand Down Expand Up @@ -252,7 +253,8 @@ protected SdkClientConfiguration mergeChildDefaults(SdkClientConfiguration confi
* Apply global default configuration
*/
private SdkClientConfiguration mergeGlobalDefaults(SdkClientConfiguration configuration) {
Supplier<ProfileFile> defaultProfileFileSupplier = new Lazy<>(ProfileFile::defaultProfileFile)::getValue;
Supplier<ProfileFileSupplier> profileFileSupplierProvider = new Lazy<>(ProfileFileSupplier::defaultSupplier)::getValue;
Supplier<ProfileFile> defaultProfileFileSupplier = profileFileSupplierProvider.get();

configuration = configuration.merge(c -> c.option(EXECUTION_INTERCEPTORS, new ArrayList<>())
.option(METRIC_PUBLISHERS, new ArrayList<>())
Expand Down
2 changes: 1 addition & 1 deletion services/s3/src/main/java/software/amazon/awssdk/services/s3/S3Utilities.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ private S3Utilities(Builder builder) {
this.region = Validate.paramNotNull(builder.region, "Region");
this.endpoint = builder.endpoint;
this.profileFile = Optional.ofNullable(builder.profileFile)
.orElseGet(() -> ProfileFileSupplier.fixedProfileFile(ProfileFile.defaultProfileFile()));
.orElseGet(ProfileFileSupplier::defaultSupplier);
this.profileName = builder.profileName;

if (builder.s3Configuration == null) {
Expand Down
Loading