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

new feature: allow inject a customized signer for aws s3 #4960

Open
1 task done
flaneur2020 opened this issue Aug 5, 2024 · 4 comments
Open
1 task done

new feature: allow inject a customized signer for aws s3 #4960

flaneur2020 opened this issue Aug 5, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@flaneur2020
Copy link
Contributor

Feature Description

background: apache/iceberg-rust#506

It seems we need to implement this signer in iceberg-rust and pass it as a customized signer to opendal.

Problem and Solution

but currently the signer is a struct AwsV4Signer.

we need make the signer in S3Core a trait before allowing it customizable.

maybe we can defined a trait called S3Signer in opendal, and make AwsV4Signer as it's implementation by default.

Additional Context

No response

Are you willing to contribute to the development of this feature?

  • Yes, I am willing to contribute to the development of this feature.
@flaneur2020 flaneur2020 added the enhancement New feature or request label Aug 5, 2024
@flaneur2020
Copy link
Contributor Author

flaneur2020 commented Aug 5, 2024

to make the Signer customizable, i've got some difficulties to solely add it in the opendal part.

adding a trait called S3Signer in opendal requires a signature to be object safe, but a signature like fn sign<T>(&self, req: &mut Request<T>, cred: &AwsCredential) -> Result<()> is
not object safe because it contains a generic parameter inside it.

so i think there might have two ways to make it possible:

1: add a GenericSigner trait with fn sign(&self, req: &mut dyn SignableRequest, cred: &AwsCredential) -> Result<()> in reqsign.

but as Signer accepts &mut impl SignableRequest, we may have to change the api of Signer to allow it accept trait objects.

2: change Signer in reqsign into an enum like:

pub enum Signer {
  V4(V4Signer),
  Custom(Box<dyn CustomSigner>)
}

this approach can make the api of Signer unchanged, but would introduce a small dispatch cost on every sign.

(i'd prefer the second approach, it seems less intrusive)

@Xuanwo any suggestion about this?

@Xuanwo
Copy link
Member

Xuanwo commented Aug 5, 2024

Hi, S3::customized_credential_load should allow you to implement your own logic.

@flaneur2020
Copy link
Contributor Author

Hi, S3::customized_credential_load should allow you to implement your own logic.

imho customized_credential_load is to customize the credential logic, but here we need customize is the signing logic.

the Signer currently is not a trait like dyn AwsCredentialLoad yet.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 5, 2024

Thank you for the clarification. I now understand what the problem is.

My current ideas:

  • Add an Sign trait for entire reqsign

     use http;
     
     #[async_trait]
     trait Sign: 'static {
     	type Credential;
     
         async fn sign(&self, req: &mut http::request::Parts, cred: &Self::Credential) -> Result<()>
     }
  • Expose a Signer from the reqsign

    reqsign will expose a type AwsSigner = Arc<dyn Sign<Credential=AwsCredential>>.

  • OpenDAL can accept any impl Sign<Credential=AwsCredential> with AwsV4Signer as default impl.

Xuanwo pushed a commit to Xuanwo/reqsign that referenced this issue Aug 6, 2024
background:
apache/opendal#4960 (comment)

this PR introduced a Sign trait, which is suitable on the cases like
when we want customize signing logics like
apache/iceberg-rust#506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants