ProfileCredentialsProvider can now reload credentials when profile files change #3487

dave-fn · 2022-10-13T16:01:58Z

Motivation and Context

The ProfileCredentialsProvider loads once and doesn't refresh/reload if the ProfileFile it was generated from got updated, which is unlike V1, in which the ProfileCredentialsProvider has support for this try to reload every Duration X (customizable) if the profile file it was generated from have a newer last modified date.

Should fix #1754, similar to PR #1760.
Also addresses #1327.

Modifications

Modified ProfileFile so that is has both the ability to detect if there are disk updates and to reload itself as a new instance. The included modifications are:

Created Builder.BuildDetails as a container of build information. ProfileFile did not keep track of the Path or InputStream used to generate it.
Add methods for finding out if the instance is stale, as determined by the associated file modification timestamp.
Added reload method to return a new instance when the file has been modified, or the same instance otherwise.

Modified ProfileCredentialsProvider for automatically reloading credentials.

Two instance variables are no longer immutable, so the type has been changed to AtomicReference objects.
A CachedSupplier is used to avoid hitting the filesystem whenever resolveCredentials is called.
Updated the ProfileCredentialsProvider.Builder interface for setting credentials refreshing intervals.

Testing

Unit tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

sonarqubecloud · 2022-10-13T17:02:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

82.6% Coverage
0.0% Duplication

dagnir · 2022-10-13T20:55:28Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+    private final AtomicReference<AwsCredentials> credentials;
    private final RuntimeException loadException;

-    private final ProfileFile profileFile;
+    private final AtomicReference<ProfileFile> profileFile;


We don't seem to perform any CAS operations on these so jusing volatile is sufficient

The credentials reference was changed to volatile AwsCredentials.
The reference to profileFile was switched for a refresher object as suggested with another comment.

dagnir · 2022-10-13T21:48:25Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFile.java

+     * @return Either the same instance in case disk has no recent updates, or a newly constructed instance.
+     * @see #isStale()
+     */
+    public ProfileFile reload() {


I think we should extract this functionality out this class. ProfileFile is meant to be immutable so it seems strange that it's able to do this reloading. Moving this functionality out to somewhere else will help simplify things as well since the scope of responsibility will be small

Agreed. I've created a ProfileFileRefresher class and refactored some of the code into it.

dagnir · 2022-10-13T21:56:42Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+        if (needToReloadCredentials()) {
+            synchronized (this) {
+                ProfileFile previousProfileFile = profileFile.get();
+                ProfileFile newProfileFile = previousProfileFile.reload();
+                profileFile.set(newProfileFile);
+
+                AwsCredentialsProvider credentialsProvider = createCredentialsProvider(newProfileFile, this.profileName);
+                nextCredentials = credentialsProvider.resolveCredentials();
+                credentials.set(nextCredentials);
+
+                // The delegate credentials provider may be closeable (eg. if it's an STS credentials provider). In this case,
+                // we should clean it up when this credentials provider is closed.
+                IoUtils.closeIfCloseable(credentialsProvider, null);
+            }
+            staleTime = staleTime(now);
+        } else {
+            nextCredentials = credentials.get();
+            staleTime = pollTime(now);
+        }
+
+        return RefreshResult.builder(nextCredentials)
+                            .staleTime(staleTime)
+                            .build();
+    }


There's a lot going on here that should be simplified once the reloading functionality is moved to a separate class. That way, you don't need to continuously update the profileFile. Instead, you can maintain a final reference to the file loader, and load the file whenever you need.

This is similar to something we did recently with Bearer token support:

aws-sdk-java-v2/services/ssooidc/src/main/java/software/amazon/awssdk/services/ssooidc/SsoOidcTokenProvider.java

Line 70 in 5845142

this.tokenRefresher = CachedTokenRefresher.builder()

which required refreshing credentials from a file

Thanks for the example. This helped a lot.

dagnir · 2022-10-18T18:37:11Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFile.java

+    public boolean isStale() {
+        return isStaleIfAny(diskFileWasModifiedSinceLoad());
+    }
+
+    /**
+     * Checks if the profile files have been changed on disk as of a particular instant.
+     *
+     * @param instant The time to check against for any file modifications.
+     * @return True if any files used to build this object have been modified.
+     * @see #isStale()
+     */
+    public boolean isStaleAsOf(Instant instant) {
+        return isStaleIfAny(details -> details.wasBuiltBefore(instant));
+    }


Can these just be properties or internal details on the refresher class? IMO ProfileFile shouldn't have any concept of staleness. It should remain a relatively simple class that just contains Profiles and that's it.

Moved this logic to ProfileFileRefresher and new class ReloadingProfileCredentialsProvider

dagnir · 2022-10-18T18:43:51Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFile.java

+    private ProfileFile registerBuildDetails(Builder.BuildDetails details) {
+        this.buildDetails.add(details);
+        return this;
+    }
+
+    private ProfileFile registerBuildDetails(Collection<Builder.BuildDetails> details) {
+        this.buildDetails.addAll(details);
+        return this;
+    }


"register" is not really a verb we use in the codebase. You can take a look at our coding guidelines here https://github.com/aws/aws-sdk-java-v2/blob/master/docs/design/ClientConfiguration.md

probably something like addBuildDetails and buildDetails respectively.

Updated method names.

dagnir · 2022-10-26T17:45:18Z

...c/main/java/software/amazon/awssdk/auth/credentials/ReloadingProfileCredentialsProvider.java

+ * @see ProfileFile
+ */
+@SdkPublicApi
+public final class ReloadingProfileCredentialsProvider


I'm a bit confused. Why do we create a new credentials provider? I think we should keep it in ProfileCredentialsProvider so that customers using the default chain get the benefit right?

Pushed new commit.

dagnir · 2022-11-02T17:52:45Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+        /**
+         * Define the condition for reloading a profile file.
+         *
+         * @param predicate Predicate for determining whether to execute the profileFileSupplier.
+         */
+        Builder profileFileReloadPredicate(Predicate<ProfileFileRefresher.ProfileFileRefreshRecord> predicate);


I don't think this is the right API for this. The reloading logic should be fully encapsulated in the Supplier<ProfileFile>, similar to ContentStreamProvider. This allows us to simplify ProfileFileCredentialsProvider because it doesn't need to contain all the reloading logic; it can be externalized. Then the credentials provider can just call profileFileSupplier.get() whenever it needs a profile file and any reloading will be taken care of internally (or not) by the supplier.

We could think about creating a separate functional API and hang convenience methods off it that helps customers create pre-canned suppliers; for example

public interface ProfileFileSupplier { ProfileFile profileFile(); public static ProfileFileSupplier reloadWhenModified(Path profileFile) { ... } }

This is similar to AsyncRequestBody.

A good example actually is AsyncRequestBody#fromFile

where customer cans just create an AsyncRequestBody by providing a file path, and an instance of AsyncRequestBody will be created for the customer. Internally, it delegates to FileAsyncRequestBody which does the heavy lifting of turning the file into a Publisher.

Created interface ProfileFileSupplier

dagnir · 2022-11-02T17:53:38Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+    public static Predicate<ProfileFileRefresher.ProfileFileRefreshRecord> wasRefreshedBeforeFileModificationTime(Path path) {
+        return refreshRecord -> refreshRecord.wasCreatedBeforeFileModified(path);
+    }


We probably don't need this (see comment below), but in general, we can't make this a public API since ProfileFileRefresher itself is an internal API.

Agreed, removed.

dagnir · 2022-11-07T20:29:26Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+    }
+
+    private boolean isNewProfileFile(ProfileFile profileFile) {
+        return currentProfileFile != profileFile;


should use .equals()

Agreed. Fixed.

dagnir · 2022-11-07T20:40:32Z

core/auth/src/main/java/software/amazon/awssdk/auth/credentials/ProfileCredentialsProvider.java

+            handleProfileFileReload(cachedOrRefreshedProfileFile);
+        }
+
+        return credentials;


We should cache the provider created in updateCredentials, rather than the credentials. This is because depending on the credential provider that's created, the credentials can still change without the profile file changing; e.g. for assume rule.

Sounds good. Change made.

dagnir · 2022-11-07T20:42:30Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+ */
+@SdkPublicApi
+@FunctionalInterface
+public interface ProfileFileSupplier extends SdkAutoCloseable {


Let's do a mini surface API review for this.

dagnir · 2022-11-07T20:42:51Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+    /**
+     * @return A ProfileFile instance.
+     */
+    ProfileFile getProfileFile();


nit just profileFile

dagnir · 2022-11-07T20:43:45Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+     * Creates a {@link ProfileFileSupplier} capable of producing multiple profile objects from a file. See
+     * {@link ProfileFileSupplier#builder()} to create a customized implementation.


Can we add more details about what it does? Something like "Returns a new ProfileFile when it is modified on disk"

dagnir · 2022-11-07T20:50:00Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/internal/ProfileFileRefresher.java

+        this.profileFileCache = CachedSupplier.builder(this::refreshResult)
+                                              .clock(this.clock)
+                                              .build();
+        this.emptyRefreshRecord = ProfileFileRefreshRecord.builder()


nit: looks like this can be a static constant

Good catch.

dagnir · 2022-11-07T20:50:25Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/internal/ProfileFileRefresher.java

+        }
+
+        try {
+            Instant lastModifiedInstant = Files.getLastModifiedTime(profileFilePath).toInstant();


dagnir · 2022-11-07T20:53:26Z

core/profiles/src/test/java/software/amazon/awssdk/profiles/ProfileFileSupplierTest.java

+    }
+
+    @Test
+    void refreshIfStale_profileModifiedMultipleTimes_reloadsProfileFileOncePerChange() {


dagnir · 2022-11-15T23:20:49Z

core/profiles/src/main/java/software/amazon/awssdk/profiles/ProfileFileSupplier.java

+    /**
+     * A builder for {@link ProfileFileSupplier}.
+     */
+    interface Builder {


Can we move this interface out?

Since this is an internal API, removed interface completely. Only left builder implementation.

…anges

…leFile to ProfileFileRefresher

…lier or Predicate, and dealing with defaults

…rofileCredentialsProvider

…er now extends Supplier; Fixed Javadoc

sonarqubecloud · 2022-11-16T16:10:13Z

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.5% Coverage
0.0% Duplication

dagnir · 2022-11-17T19:02:27Z

Please make sure to run integ tests as well before merging.

dave-fn · 2022-11-21T20:02:20Z

@mina-asham, the AWS Java SDK team would like to thank you for your contribution on PR #1754. Although we didn't end up merging your PR, it did serve as a starting point for functionality that we'll soon be adding.

* ProfileCredentialsProvider can now reload credentials when profile files change (#3487) * ProfileFile can update if disk changed, reload as new instance * ProfileCredentialsProvider reloads credentials if profile file has changes * Created class ProfileFileRefresher * Created ReloadingProfileCredentialsProvider; moved new logic in ProfileFile to ProfileFileRefresher * Fix ReloadingProfileCredentialsBehavior when missing ProfileFile Supplier or Predicate, and dealing with defaults * Consolidated ReloadingProfileCredentialsProvider functionality into ProfileCredentialsProvider * Fix behavior when dealing with defaults * Created ProfileFileSupplier interface; refactored * Misc fixes * Created package-private ProfileFileSupplierBuilder; ProfileFileSupplier now extends Supplier; Fixed Javadoc * Fixed unit tests for default credentials file * Removed ProfileFileSupplier.Builder interface * Code cleanup * ProfileFileSupplier API changes, aggregating (#3558) * Added methods for aggregating ProfileFile objects * Removed redundant logic, changelog entry * Removed redundant methods * Use compare and set to make thread safe * DefaultCredentialsProvider reload profile (#3580) * Misc fixes * Reload credentials by DefaultCredentialsProvider; pass supplier to InstanceProfileCredentialsProvider * Fix code alignment * Update public APIs to Supplier<ProfileFile> instead of ProfileFileSupplier (#3607) * ProfileTokenProvider Reload ProfileFile (#3608) * Updated ProfileTokenProvider * Updated tests, do not explicitly swallow exceptions * ProfileTokenProviderLoader can use Supplier<Profilefile> internally * Simplified ProfileTokenProviderLoader API; implemented synchronized block * Use synchronized block (#3646) * S3 support classes take Supplier<ProfileFile> (#3653) * S3 support classes take Supplier<ProfileFile> * Review comments * Presigners, other Support classes take Supplier<ProfileFile> (#3677) * Presigners, other Support classes take Supplier<ProfileFile> * Split new ProfileFile tests from existing parameterized tests * Improved tests readability * ProfileFile updates to BaseClientBuilderClass, other S3 classes (#3685) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simp… (#3692) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simple ProfileFile * Addressed review comments * Updated changelog entry (#3699) * Fixed review comments * Removed unnecessary logic * Deprecated SdkClientOption PROFILE_FILE * Deprecated SdkExecutionAttribute PROFILE_FILE * Updated changelog entry

…le-plugin Pin OpenRewrite Gradle plugin version

dave-fn requested a review from a team as a code owner October 13, 2022 16:01

dave-fn changed the title ~~ProfileCredentialsProvider can now refresh credentials when profile files change~~ ProfileCredentialsProvider can now reload credentials when profile files change Oct 13, 2022

dagnir reviewed Oct 13, 2022

View reviewed changes

dagnir reviewed Oct 18, 2022

View reviewed changes

dagnir reviewed Oct 26, 2022

View reviewed changes

dagnir reviewed Nov 2, 2022

View reviewed changes

dagnir reviewed Nov 7, 2022

View reviewed changes

dagnir reviewed Nov 15, 2022

View reviewed changes

dave-fn added 11 commits November 16, 2022 06:53

ProfileFile can update if disk changed, reload as new instance

7d580cb

ProfileCredentialsProvider reloads credentials if profile file has ch…

01496b9

…anges

Created class ProfileFileRefresher

ab7706b

Created ReloadingProfileCredentialsProvider; moved new logic in Profi…

a6d2ef1

…leFile to ProfileFileRefresher

Fix ReloadingProfileCredentialsBehavior when missing ProfileFile Supp…

100c579

…lier or Predicate, and dealing with defaults

Consolidated ReloadingProfileCredentialsProvider functionality into P…

bcbcd28

…rofileCredentialsProvider

Fix behavior when dealing with defaults

d5264a8

Created ProfileFileSupplier interface; refactored

fa615ae

Misc fixes

7dec0a0

Created package-private ProfileFileSupplierBuilder; ProfileFileSuppli…

85ecade

…er now extends Supplier; Fixed Javadoc

Fixed unit tests for default credentials file

c088f09

dave-fn force-pushed the features/reload-profile-credentials branch from 074cf21 to c088f09 Compare November 16, 2022 13:53

dave-fn added 2 commits November 16, 2022 08:11

Removed ProfileFileSupplier.Builder interface

115f099

Code cleanup

e6d1615

dagnir approved these changes Nov 16, 2022

View reviewed changes

dave-fn changed the base branch from master to feature/master/credentials-reload November 21, 2022 15:00

dave-fn merged commit 33d4012 into aws:feature/master/credentials-reload Nov 21, 2022

dave-fn deleted the features/reload-profile-credentials branch November 21, 2022 15:01

dave-fn mentioned this pull request Dec 12, 2022

ProfileTokenProvider Reload ProfileFile #3608

Merged

12 tasks

debora-ito mentioned this pull request Jan 10, 2023

Add support for ProfileCredentialsProvider to reload when the profile changes on disk #1760

Closed

13 tasks

DocX mentioned this pull request Apr 8, 2024

Reload ProfileCredentialsProvider if ProfileFile changes on disk #1754

Open

1 task

aws-sdk-java-automation added a commit that referenced this pull request Nov 27, 2024

Merge pull request #3487 from aws/hdavidh/v2-migration-tests-pin-grad…

8683cae

…le-plugin Pin OpenRewrite Gradle plugin version

		* Creates a {@link ProfileFileSupplier} capable of producing multiple profile objects from a file. See
		* {@link ProfileFileSupplier#builder()} to create a customized implementation.

ProfileCredentialsProvider can now reload credentials when profile files change #3487

ProfileCredentialsProvider can now reload credentials when profile files change #3487

Conversation

dave-fn commented Oct 13, 2022 • edited Loading

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

sonarqubecloud bot commented Oct 13, 2022

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

dave-fn Oct 20, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 16, 2022

dagnir commented Nov 17, 2022

dave-fn commented Nov 21, 2022

dave-fn commented Oct 13, 2022 •

edited

Loading

dave-fn Oct 20, 2022 •

edited

Loading