Skip to content

Conversation

@AhmadMasry
Copy link
Contributor

Context

#34049
Add support to use S3 repos using AWS SSO

Contributor Checklist

  • Review Contribution Guidelines.
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Make sure all contributed code can be distributed under the terms of the Apache License 2.0, e.g. the code was written by yourself or the original code is licensed under a license compatible to Apache License 2.0.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team.
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective.
  • Provide unit tests (under <subproject>/src/test) to verify logic.
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes.
  • Ensure that tests pass sanity check: ./gradlew sanityCheck.
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:quickTest.

Reviewing cheatsheet

Before merging the PR, comments starting with

  • ❌ ❓must be fixed
  • 🤔 💅 should be fixed
  • 💭 may be fixed
  • 🎉 celebrate happy things

@AhmadMasry AhmadMasry requested review from a team as code owners July 1, 2025 11:18
@AhmadMasry AhmadMasry requested a review from jvandort July 1, 2025 11:18
@bot-gradle bot-gradle added from:contributor PR by an external contributor to-triage labels Jul 1, 2025
@cobexer cobexer removed the request for review from a team July 1, 2025 13:57
@ljacomet
Copy link
Member

ljacomet commented Jul 2, 2025

Thank you for your proposed contribution!

This PR has a valid DCO. The relevant team for this area will confirm the design of the implementation choices.
In particular, we need to figure out if we can test this integration or not.

@ljacomet ljacomet added in:dependency-remoting interaction with dependency repositories, mostly networked 👋 team-triage Issues that need to be triaged by a specific team and removed to-triage labels Jul 2, 2025
@AhmadMasry
Copy link
Contributor Author

Locally it can be tested easily by trying to build the project using gradle without cache on a device with AWS CLI configured with SSO credentials.
It can be tasted on runners, by not providing any credentials and configuring aws sso profile.

@big-guy
Copy link
Member

big-guy commented Jul 22, 2025

We need to find a way to test this in an automated way.

Adding more features to the S3-based repositories is not in scope right now for us. We haven't looked, but we also need to consider how this impacts moving to AWS SDK v2. Sorry, we probably won't look into this soon.

@big-guy big-guy removed the 👋 team-triage Issues that need to be triaged by a specific team label Jul 22, 2025
@AhmadMasry
Copy link
Contributor Author

Thank you for the note, but I would really appreciate reconsider this because of the following reasons:

  1. AWS discribes the credential provider chain and how AWS SDKs resolve IAM Identity Center credentials and provide details how to use SSO with Java SDK v2 in the provided links.
  2. I do not know if the testing infrastructure has access to an AWS account with Organization setup, if so, I can work on a test, otherwise, even Minio does not provide a way to test it.
  3. The change does not include any logic change, it only added the dependency needed to enable the SDK to resolve SSO credentials.
  4. The migration to AWS SDK v2 is very straight forward, just replace with the equivalent dependencies like Use aws-sdk-java-v2. #29686 without any logic change.
  5. We previously contributed to Gradle by updating the AWS SDK version to support EKS Pod Identities, and that was approved and merged based on AWS documentation and because the tests did not break, not because it was actually tested on self hosted EKS Github ARC pods that uses EKS Pod Identities which is what we use internally.

Thank you again for your time and effort.

@ov7a ov7a added this to the 9.x milestone Aug 6, 2025
@ov7a ov7a removed the to-triage label Aug 6, 2025
@AhmadMasry
Copy link
Contributor Author

Hi;
Sorry for keep asking, but it there any thing I can help with to facilitate the merge of this PR if approved?
Also, I can help in porting the change to AWS SDK v2 branch if the branched used in #29686 is the one to be merged, as I noticed it is tagged to be in 9.1.
Thank you very much

@jvandort
Copy link
Member

We will likely not want to accept this change without tests

@AhmadMasry
Copy link
Contributor Author

Thank you @jvandort
Did you have the chance to check my last response about tests?
TLDR, if there is any kind of infrastructure that I can test against, I will be more than happy to write the tests, just guide me what to do.
Thanks again.

@jvandort
Copy link
Member

We have test infrastructure to mock an s3 endpoint

https://github.com/gradle/gradle/blob/master/platforms/software/resources-s3/src/integTest/groovy/org/gradle/integtests/resource/s3/fixtures/S3Server.groovy

And tests that utilize this test fixture

https://github.com/gradle/gradle/blob/master/platforms/software/resources-s3/src/integTest/groovy

I would expect to see a test that sets the proper env vars and verifies that those credentials are sent to the s3 server

@AhmadMasry
Copy link
Contributor Author

AhmadMasry commented Aug 29, 2025

Thank you for your guidance.
Here is the catch, including com.amazonaws:aws-java-sdk-sso will prevent the need of setting the proper environment variables for access key, secret key and session token, as they will be automatically detected by credential provider chain in this test and in here
The current tests should be enough as it works exactly like STS #15333 and EC2 IAM profiles 7283def .
Is it possible if you can confirm my claim?
Thank you again for your time.

@AhmadMasry
Copy link
Contributor Author

Hi
Sorry for keep nagging on this, but did anyone by chance validated my claims about the tests?
Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

from:contributor PR by an external contributor in:dependency-remoting interaction with dependency repositories, mostly networked

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants