Fix reload credentials not working when modified #5185
Conversation
|
@debora-ito @millems Please review, tagging you as changes includes to area that were updated by you recently. Please let me know if approach need to be modified |
debora-ito
added
needs-review
...k-core/src/main/java/software/amazon/awssdk/core/client/builder/SdkDefaultClientBuilder.java
Outdated
Show resolved
Hide resolved
|
CI build failure seems to be unrelated to the PR, it is failing while setting the aws credentials. Please let me know if any setup or changes required |
|
@anirudh9391 the CI build is failing in the setup.. please let me know if anything can be done to resolve it |
|
Based on findings, the secrets apart from GITHUB_TOKEN is not available to PR from forked repository. actions/runner#3039 I don't have access to push the branch to this repository. @anirudh9391 would it be possible to pick these changes & create branch in this repo? |
Yeah I can do that |
ProfileFile's builder expects contentLocation or content to be present. Here, ProfileFile is being used as fallback, ensure it doesn't fail
munendrasn
force-pushed
the
reload-credential-fix
branch
from
a66d381
to
fd04bab
Compare
|
|
||
| int maxRetries = 4; | ||
| int i = 0; | ||
| // check if this can be replaced with RetryPolicy and backoff |
There was a problem hiding this comment.
Can we provide a more meaningful comment here ? Like
"Configuring test with a Retry and backoff to ensure credential loading takes effect"
There was a problem hiding this comment.
NIT: Could we also include a smal note about why we need to retry here? Something like "Waiting until file changes reach the SDK" or something like that?
There was a problem hiding this comment.
@anirudh9391 @L-Applin sure, will update the note
|
Beside the small nit, I posted everything looks fine. I launched a run of the Codebuild jobs for the unit-tests and waiting for them to succeed |
| @@ -36,3 +36,6 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender | |||
| # | |||
| #logger.netty.name = io.netty.handler.logging | |||
| #logger.netty.level = debug | |||
|
|
|||
| logger.cache.name = software.amazon.awssdk.utils.cache | |||
There was a problem hiding this comment.
Actually, could you comment or remove these added logs? We can reactive them on demand in the future
There was a problem hiding this comment.
Enabled the logs to debug the test failures, new test is intermittently failing in CI but succeeds in local setup
More context in this PR #5259
|
The new added test This would need to be resolved before we are able to merge this fix. |
@L-Applin could you please share the link to CI build.. which would have additional logs to debug the issue further.. the ones from the PR seems to have failed because #5185 (comment) |
|
@L-Applin @anirudh9391 would it be possible to share CI build or complete logs of the build with latest changes? The fork PR #5259 doesn't have latest changes |
Sure, I will rerun the tests and try to give you the logs today |
|
Here is the relevant logs for the test failure: This test also fails locally for me, tested on java 17. Isn't it failing for you? |
|
@L-Applin
No, the tests are passing successfully in my local. I'm using Mac-Intel in my local.. From the logs, Log related to last time file got modified Please notice To check whether ProfileFile need to be refreshed, same logic is used here
New commit include additional logs for debugging, and sleep before file modification... if this doesn't work.. other option is explicitly override the Something on these lines, please let me know if I should update this way.. In meantime, I trying to find more info on Path credentialsFilePath2 = generateTestCredentialsFile(parentDirectory,"modifiedAccess", "modifiedSecret");
Instant lastModifiedTime = Files.getLastModifiedTime(credentialsFilePath2).toInstant();
lastModifiedTime = lastModifiedTime.plus(5, ChronoUnit.MINUTES);
Files.setLastModifiedTime(credentialsFilePath2, FileTime.from(lastModifiedTime)); |
Huh, that interesting, I'm on mac-m1. I'm not sure what could cause this discrepancy either, i'll do some investigation on my end as well. In the meantime, I'll re-run the unit tests Codebuild. |
|
| @@ -127,7 +130,10 @@ private boolean canReloadProfileFile() { | |||
|
|
|||
| try { | |||
| Instant lastModifiedInstant = Files.getLastModifiedTime(profileFilePath).toInstant(); | |||
| return currentRefreshRecord.refreshTime.isBefore(lastModifiedInstant); | |||
| boolean canReloadFile = currentRefreshRecord.refreshTime.isBefore(lastModifiedInstant); | |||
| log.info("For path {}, with previous refreshTime {}, last modified time {}, canReloadProfileFile is {}", | |||
There was a problem hiding this comment.
please lower to debug log level, we tend to avoid info log level in the sdk
There was a problem hiding this comment.
If the build is passing, Shall I remove this?
I added it only for debugging, There are few other logs which need to removed, or log level need to be changed
Also, instead of thread sleep.. in the tests should we cleanup and directly update the lastModifiedTime?
Path credentialsFilePath2 = generateTestCredentialsFile(parentDirectory,"modifiedAccess", "modifiedSecret");
Instant lastModifiedTime = Files.getLastModifiedTime(credentialsFilePath2).toInstant();
lastModifiedTime = lastModifiedTime.plus(5, ChronoUnit.MINUTES);
Files.setLastModifiedTime(credentialsFilePath2, FileTime.from(lastModifiedTime));
There was a problem hiding this comment.
I would avoid relying on platform dependant functionalities in unit tests. I'd keep the log, just at a debug level.
There was a problem hiding this comment.
@L-Applin updated the logging, please review
|
@L-Applin Please let me know if any additional changes are required |
|
|
@L-Applin Build failure again due to secrets not being available to PR as I have the PR from the fork.. Please let me know if anything is required from my side |
|
@L-Applin @anirudh9391 @debora-ito |
Ensure defaultSupplier is used instead of fixedSupplier in case of defaultProfileFile.
I have updated the some places where
defaultProfileFilewas used along withfixedProfileSupplierwithdefaultSupplierMotivation and Context
Fixes #4268
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License