Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: support IRSA for aws s3 provider #749

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

Anhui-tqhuang
Copy link
Contributor

No description provided.

Copy link
Owner

@gaul gaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return new Credentials(creds.getCredentials().getAWSAccessKeyId(),
creds.getCredentials().getAWSSecretKey());
}
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the Apache jclouds STS supplier for consistency:

https://stackoverflow.com/questions/23520216/using-aws-s3-via-jclouds-how-to-assume-role

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, i am still testing this change, btw, the sts api only supports assume role not assume role with web identity 🤔

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on your comment? Do you mean that the AWS STS library only supports assume role? Or do you mean the jclouds code only allows it? If the former, it would be better to remove the aws-java-sdk-sts dependency. But if the latter, I am willing to take this if you can explain more clearly what this means.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assume role is:

  • you have your own credentials (access key id and access secret key), then you have the role arn that you want to assume
  • assume role with web identity is, you have only one role arn and one web identity token

jclouds doesn't have a way to let you load you default aws credentials including web identity token

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this is something like what we have done for azure

new DefaultAzureCredentialBuilder().build());

Copy link
Contributor Author

@Anhui-tqhuang Anhui-tqhuang Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, default aws credential provider chan here could help the program to load default credentials from many kind of aws sdk compatible env like

  • aws credentials: AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY
  • iam role service account: AWS_ROLE_ARN, AWS_WEB_IDENTITY_TOKEN_FILE
  • etc ...
    public DefaultAWSCredentialsProviderChain() {
        super(new EnvironmentVariableCredentialsProvider(),
              new SystemPropertiesCredentialsProvider(),
              WebIdentityTokenCredentialsProvider.create(),
              new ProfileCredentialsProvider(),
              new EC2ContainerCredentialsProviderWrapper());
    }

pom.xml Outdated Show resolved Hide resolved
Copy link
Owner

@gaul gaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please alphabetize imports as Checkstyle suggests.

@Anhui-tqhuang
Copy link
Contributor Author

@gaul hey, page updated https://github.com/gaul/s3proxy/wiki/Storage-backend-examples

and my change tested

$ aws s3 ls xxx --endpoint http://0.0.0.0:4449
                           PRE customer-pg-backups/
...

@Anhui-tqhuang Anhui-tqhuang marked this pull request as ready for review January 2, 2025 03:49
@Anhui-tqhuang Anhui-tqhuang requested a review from gaul January 2, 2025 03:58
pom.xml Outdated
<dependency>
<groupId>commons-logging</groupId>
<artifactId>commons-logging</artifactId>
<version>1.2</version>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without this, aws sdk will not be able to work with this error due to this exclude

    <dependency>
      <groupId>com.amazonaws</groupId>
      <artifactId>aws-java-sdk-s3</artifactId>
      <version>1.12.261</version>
      <scope>test</scope>
      <exclusions>
        <exclusion>
          <groupId>commons-logging</groupId>
          <artifactId>commons-logging</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember the full context for this from 8 years ago but e569b19 suggests that I configured the AWS SDK to use the sl4fj logging like the rest of S3Proxy/jclouds. Can you investigate this further?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i tried to remove that dependency and i got this error

Caused by: java.lang.NoClassDefFoundError: Could not initialize class com.amazonaws.auth.DefaultAWSCredentialsProviderChain
	at org.gaul.s3proxy.Main$2.get(Main.java:399)
	at org.gaul.s3proxy.Main$2.get(Main.java:396)
	at org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:241)
	at org.jclouds.rest.internal.RestAnnotationProcessor.apply(RestAnnotationProcessor.java:137)
	at org.jclouds.rest.internal.InvokeHttpMethod.toCommand(InvokeHttpMethod.java:189)
	at org.jclouds.rest.internal.InvokeHttpMethod.invoke(InvokeHttpMethod.java:85)
	at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:74)
	at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:45)
	at org.jclouds.rest.internal.DelegatesToInvocationFunction.handle(DelegatesToInvocationFunction.java:156)
	at org.jclouds.rest.internal.DelegatesToInvocationFunction.invoke(DelegatesToInvocationFunction.java:123)
	at jdk.proxy2/jdk.proxy2.$Proxy65.getBucketLocation(Unknown Source)
	at org.jclouds.s3.config.S3HttpApiModule$3.load(S3HttpApiModule.java:129)
	at org.jclouds.s3.config.S3HttpApiModule$3.load(S3HttpApiModule.java:125)
	at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3574)
	at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2316)
	at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2189)
	at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2079)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i figured out that the program could only work when this dependency is added 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any hints? 🤔 as i am not familiar with java really 😢

Copy link
Contributor Author

@Anhui-tqhuang Anhui-tqhuang Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i saw these imports

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;

in the class definition

public class DefaultAWSCredentialsProviderChain extends AWSCredentialsProviderChain {
...
}

public class AWSCredentialsProviderChain implements AWSCredentialsProvider {
....
}

I don't know if it is possible to replace the logging that the package has imported 🤔

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove the <scope>test</scope> from <artifactId>jcl-over-slf4j</artifactId>? This should address the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaul yep, that works!, thanks! i removed the common logging dependency

return new Credentials(creds.getCredentials().getAWSAccessKeyId(),
creds.getCredentials().getAWSSecretKey());
}
};
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on your comment? Do you mean that the AWS STS library only supports assume role? Or do you mean the jclouds code only allows it? If the former, it would be better to remove the aws-java-sdk-sts dependency. But if the latter, I am willing to take this if you can explain more clearly what this means.

@gaul gaul merged commit 25110e5 into gaul:master Jan 3, 2025
@gaul
Copy link
Owner

gaul commented Jan 3, 2025

Thank you for your contribution @Anhui-tqhuang!

@Anhui-tqhuang
Copy link
Contributor Author

@gaul thx a lot!

@Anhui-tqhuang Anhui-tqhuang deleted the support-irsa branch January 3, 2025 06:14
@Anhui-tqhuang
Copy link
Contributor Author

@gaul and thanks for fixing 3ff077f

i just noticed that and spike how to fix it 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants