Merge credentials reloading feature branch to mainline #3712

dave-fn · 2023-01-24T21:03:22Z

Motivation and Context

Add support for reloading credentials provided by ProfileCredentialsProvider if file changes on disk.
In addition, propagate changes to other classes with similar behavior: such as DefaultCredentialsProvider, InstanceProfileCredentialsProvider and ProfileTokenProvider.
Finally, some service classes also implemented this functionality so that configuration options might be changed on the fly.

Modifications

Multiple PRs

Testing

Added new test cases
Will run integ tests

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

…les 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

* Added methods for aggregating ProfileFile objects * Removed redundant logic, changelog entry * Removed redundant methods * Use compare and set to make thread safe

* Misc fixes * Reload credentials by DefaultCredentialsProvider; pass supplier to InstanceProfileCredentialsProvider * Fix code alignment

…plier (#3607)

* Updated ProfileTokenProvider * Updated tests, do not explicitly swallow exceptions * ProfileTokenProviderLoader can use Supplier<Profilefile> internally * Simplified ProfileTokenProviderLoader API; implemented synchronized block

* S3 support classes take Supplier<ProfileFile> * Review comments

* Presigners, other Support classes take Supplier<ProfileFile> * Split new ProfileFile tests from existing parameterized tests * Improved tests readability

#3692) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simple ProfileFile * Addressed review comments

millems · 2023-01-25T22:13:48Z

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

+        if (profileFile instanceof SdkAutoCloseable) {
+            ((SdkAutoCloseable) profileFile).close();
+        }


This is a red flag to me: closing something we didn't necessarily create. What if this is being used by someone else and we just closed it/broke it?

My bigger question is: why are profile file suppliers closeable?

A problem indeed. I even brought this up during a discussion, so I'm not sure why I didn't remove it.

Re: closeable suppliers--it's only because the specific implementation with reloadWhenModified uses ProfileFileRefresher internally, which in turn uses CachedSupplier being AutoCloseable.
This is also something you did touch upon in another comment.

millems · 2023-01-25T22:19:15Z

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

+            = ProfileFileLocation.configurationFileLocation()
+                                 .map(path -> reloadWhenModified(path, ProfileFile.Type.CONFIGURATION));
+
+        ProfileFileSupplier supplier = () -> null;


Should we return an empty file instead of null? Or do we handle null everywhere that we handle profile files?

Empty file indeed. Changed.

millems · 2023-01-25T22:22:21Z

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

+                    aggregator.addFile(supplier.get());
+                }
+
+                return refreshAndGetCurrentAggregate(aggregator);


Do we really need to re-aggregate with every profile file get()?

My understanding is that if an aggregate file is made up of N subfiles, then whenever we call get() we need to look at the new contents of the individual files. The need to reaggregate is to determine if the new contents are different from the current aggregate.

Got it, yeah if we can avoid re-aggregating when the files haven't changed that's a good fix.

Reimplemented previous code that was looking at whether the individual files had changes. With this new version, we should not re-aggregate if not needed.

millems · 2023-01-25T22:23:35Z

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

+import software.amazon.awssdk.profiles.internal.ProfileFileRefresher;
+
+@SdkInternalApi
+final class ProfileFileSupplierBuilder {


Does anything use this class?

Only a test class. Mainly for the clock() method making it easier to setup tests involving timing.

Gotcha, I searched this CR to see if things were using it and I must have missed something using it.

millems · 2023-01-25T22:26:35Z

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

+
+    @Override
+    public void close() {
+        profileFileCache.close();


We only need to close the cached supplier when it is async. I think it's fine to exclude this close for now so that we don't have to deal with managing the lifecycle of a profile file supplier.

millems · 2023-01-25T22:44:04Z

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

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


It doesn't look like we're actually using the cached supplier here for any of its caching functionality, just as a proxy for calling refreshResult. Do we actually need it, or can we rewrite this to not require it?

Yeah. We can probably discuss this offline, but yes, the original intent of CachedSupplier was to abstract away managing individually the profile files. I may have chipped too much off and now it doesn't really make sense to use it.
One benefit of still using the cached supplier, if I understand correctly is to avoid refreshing a value when the request occurs within a timeframe (jitter time), which in this case means not hitting the disk excessively in a short amount of time.

Ah, I didn't think of that benefit. I think excluding jittering for local resources isn't a major risk. The failure mode is pretty different than a remote resource where jittering becomes important.

millems · 2023-01-25T22:47:51Z

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

+        Instant now = clock.instant();
+        Instant staleTime = now;


Why have two variables for the same value?

Was being too verbose. Remove this and other instance.

millems · 2023-01-25T22:49:30Z

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

+        } catch (RuntimeException exception) {
+            Instant now = Instant.now();
+            Instant staleTime = now;
+            ProfileFile exceptionProfileFile = exceptionHandler.apply(exception);


Why do we not just throw the exception? What's the purpose of an exception profile file?

I was copying CachedTokenRefresher. I guess we could recover from an exception and provide a fallback profile file, maybe the default.

Interesting. I was thinking the intuitive behavior would just be to throw the exception (failing whatever profile file refresh is happening) and then let the next get() call try again. Essentially, push the failure handling upstream.

Trying to do something more intelligent might be confusing for the user to understand.

Agreed this may be too complex. Removed.

sonarqubecloud · 2023-02-06T19:38:24Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
295 Code Smells

77.7% Coverage
4.4% Duplication

…59e84a727 Pull request: release <- staging/0065af7f-0a1b-4ce9-b143-81d59e84a727

dave-fn added 11 commits November 21, 2022 08:00

ProfileFileSupplier API changes, aggregating (#3558)

6b186a2

* 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)

c3e743b

* Misc fixes * Reload credentials by DefaultCredentialsProvider; pass supplier to InstanceProfileCredentialsProvider * Fix code alignment

Update public APIs to Supplier<ProfileFile> instead of ProfileFileSup…

2f8f819

…plier (#3607)

ProfileTokenProvider Reload ProfileFile (#3608)

68ff756

* 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)

6f2bafd

S3 support classes take Supplier<ProfileFile> (#3653)

b88ea40

* S3 support classes take Supplier<ProfileFile> * Review comments

Presigners, other Support classes take Supplier<ProfileFile> (#3677)

6ae05dc

* 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)

77e45cf

Leverage SdkClientOption and SdkExecutionAttributes; fallback to simp… (

70fa31a

#3692) * Leverage SdkClientOption and SdkExecutionAttributes; fallback to simple ProfileFile * Addressed review comments

Updated changelog entry (#3699)

b6e29f7

dave-fn requested a review from a team as a code owner January 24, 2023 21:03

millems reviewed Jan 25, 2023

View reviewed changes

dave-fn added 2 commits January 26, 2023 11:07

Fixed review comments

0890ecf

Removed unnecessary logic

adac974

dave-fn force-pushed the feature/master/credentials-reload branch from 60e3eae to adac974 Compare January 27, 2023 20:38

dave-fn added 2 commits February 1, 2023 14:30

Deprecated SdkClientOption PROFILE_FILE

bcb7a4b

Deprecated SdkExecutionAttribute PROFILE_FILE

e8ba9fd

millems approved these changes Feb 6, 2023

View reviewed changes

L-Applin approved these changes Feb 6, 2023

View reviewed changes

Updated changelog entry

627e560

dave-fn merged commit f3a111d into master Feb 6, 2023

dave-fn deleted the feature/master/credentials-reload branch February 6, 2023 19:39

munendrasn mentioned this pull request Aug 7, 2023

Credentials reloading is not working in the recent versions (2.20.x) #4268

Open

aws-sdk-java-automation added a commit that referenced this pull request Mar 5, 2025

Merge pull request #3712 from aws/staging/0065af7f-0a1b-4ce9-b143-81d…

5967ea8

…59e84a727 Pull request: release <- staging/0065af7f-0a1b-4ce9-b143-81d59e84a727

Merge credentials reloading feature branch to mainline #3712

Merge credentials reloading feature branch to mainline #3712

Uh oh!

Conversation

dave-fn commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-fn Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-fn Jan 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dave-fn Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 6, 2023

Uh oh!

Uh oh!

dave-fn commented Jan 24, 2023 •

edited

Loading

dave-fn Jan 26, 2023 •

edited

Loading

dave-fn Jan 26, 2023 •

edited

Loading

dave-fn Jan 27, 2023 •

edited

Loading