Skip to content

Conversation

@DZDomi
Copy link

@DZDomi DZDomi commented May 22, 2025

Please add a summary of your change

In a previous commit (30728c2) for the function that provides AWS credentials, a crucial if was removed that was responsible for making sure callers of the function GetS3Credentials would not receive expired credentials.

Specifically the removal of the following code caused the problem:

if os.Getenv(awsRoleEnvVar) != "" {
	return nil, nil
}

By not returning nil, nil in any of the code in the specific function and forcing to return some credentials, even if they can expire (for example for IAM roles that are assumed via STS) all functions that would call this function would end up with invalid tokens after some period. This affects specifically long running backup/data mover jobs to cloud storage like S3. This PR addresses this issue, buy checking if the credentials received are able to expire and if yes, will return nil,nil and let the calling functions get their credentials via the default AWS credential provider chain.

Does your change fix a particular issue?

Fixes #8173

Please indicate you've done the following:

blackpiglet and others added 4 commits May 22, 2025 23:11
* Please notice only Kibishii workload support Windows test,
because the other work loads use busybox image, and not support Windows.
* Refactor CreateFileToPod to support Windows.
* Add skip logic for migration test if the version is under 1.16.
* Add main in semver check.

Signed-off-by: Xun Jiang <[email protected]>
Signed-off-by: dominik <[email protected]>
Signed-off-by: Dominik De Zordo <[email protected]>
Signed-off-by: dominik <[email protected]>
@DZDomi DZDomi force-pushed the fix/aws-credentials-chain branch from 295152f to 96a3938 Compare May 22, 2025 21:11
@kaovilai
Copy link
Collaborator

kaovilai commented Jun 4, 2025

Did you have to modify some commits from @blackpiglet as well? Can you cleanup so or only has your commits?

@DZDomi
Copy link
Author

DZDomi commented Jun 9, 2025

hey @kaovilai thanks for looking into the PR. I just noticed I had some extra commits in the PR. I made a new one, which can be found here: #9011 It only includes my changes. Please if you have time, can you look into it?

Thanks!

@DZDomi DZDomi closed this Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrading to any version beyond 1.12 we are getting error of expired token for backing up data using datamover after 1 hr with IRSA

3 participants