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

Module incompatible with S3-compatible services #57

Closed
Schachte opened this issue Aug 15, 2023 · 7 comments
Closed

Module incompatible with S3-compatible services #57

Schachte opened this issue Aug 15, 2023 · 7 comments
Assignees
Labels
duplicate This issue or pull request already exists

Comments

@Schachte
Copy link

So far I'm not having much luck getting this module working with S3-compatible APIs such as Cloudflare R2. I notice the constructor for the config does not support the traditional endpoint param that you would see in other APIs. Is this intentional?

@Schachte Schachte changed the title module incompatible with S3-compatible APIs Module incompatible with S3-compatible services Aug 15, 2023
@oleiade oleiade added the duplicate This issue or pull request already exists label Aug 15, 2023
@oleiade oleiade self-assigned this Aug 15, 2023
@oleiade
Copy link
Member

oleiade commented Aug 15, 2023

Hi @Schachte 👋

Thanks for bringing this to our attention! While we primarily focus on AWS S3, it's great when our library works with other storage solutions. However, such compatibility is more of a happy coincidence than a design feature.

It might seem like a simple tweak to accommodate other systems, but each addition can inadvertently set an expectation for us to support and maintain those systems in the long run.

Given the size of our team, we've chosen to focus our resources on ensuring solely AWS S3 support. Any changes to accommodate other systems would mean that, while we're happy to integrate them, we won't be able to support those features officially.

We genuinely value contributions from our community. So, if you're inclined to tailor the library to your needs, please do open a PR. We'd greatly appreciate it! 🤝🙇

duplicate of #50

@oleiade
Copy link
Member

oleiade commented Aug 15, 2023

Sharing some of my research process in the open 🤓

I've been consulting the official AWS node SDK API documentation, and couldn't find any reference to an endpoint client.

@oleiade
Copy link
Member

oleiade commented Aug 15, 2023

It turns out, the aws sdk is quite convoluted and consists of (too many) layers of abstractions above layers of abstraction.

It is indeed possible to build a:

😕

Now I need to figure out how this is used throughout the code.

@oleiade
Copy link
Member

oleiade commented Aug 15, 2023

I've found a couple articles on how users use the official SDK to connect to alternative S3-compatible backends:

@oleiade
Copy link
Member

oleiade commented Aug 15, 2023

Interestingly it looks like the node aws SDK has a dedicated type for endpoints too.

@Schachte
Copy link
Author

Schachte commented Aug 15, 2023

Hey @oleiade, thanks for the response! I took a brief look into the code and also see some things. I actually got this to work, but the semantics of endpoint & host are a bit off I think.

I think the crux of the issue here is that, while you can set the endpoint in the AWS config here:
https://github.com/grafana/k6-jslib-aws/blob/main/src/internal/client.ts#L22

where the config actually allows for an override here:
https://github.com/grafana/k6-jslib-aws/blob/main/src/internal/config.ts#L47

the root of the issue lies in the super call to the extended parent class here:
https://github.com/grafana/k6-jslib-aws/blob/main/src/internal/s3.ts#L12

At this layer, there is basically no opportunity to reuse the existing endpoint that was configured and instead it looks for a host. Given that the host isn't pulled from the config at all, it overrides it with an opinionated string here:
https://github.com/grafana/k6-jslib-aws/blob/main/src/internal/client.ts#L33 that is incompatible with services like r2.

Luckily, there is a setter to override the private class-member variable and that resolves the issue. So for Cloudflare, you would just

let awsConfig = new AWSConfig(
  {
    accessKeyId: ACCESS_KEY_ID,
    secretAccessKey: ACCESS_KEY_SECRET,
    region: "auto",
    endpoint: `${ACCOUNT_ID}.r2.cloudflarestorage.com`,
  });

const s3 = new S3Client(awsConfig);
s3.host = `${ACCOUNT_ID}.r2.cloudflarestorage.com`;

This is a bit goofy though. I'd have to peek at the other AWS implementation to see how this is more gracefully handled without needing to do the extra set step, but if y'all are over capacity, happy to take a stab at it. IMO there should be some cross-referencing of the endpoint in the super call.

Usage in the VU func looks like:

export default async function () {
  const listObjectsResponse = await s3.listBuckets();
  console.log(listObjectsResponse); // this yields the expected result-set

  check(listObjectsResponse, {
    "ListBucketsResult was successful": (status) => status !== undefined,
  });
}

@oleiade
Copy link
Member

oleiade commented Aug 16, 2023

Hey @Schachte 👋🏻

We came to the same conclusion indeed 🤝 I've been hacking something together to add an endpoint property to AWSConfig, and got it to mostly work with Digital Ocean spaces.

However, I'm at a point where the super call in the constructor causes issues, as the current behavior is to enforce modifying the internal service's host with the service name.

I expect to have a work-in-progress PR by the end of the day (CET) and add you as a reviewer there. I would appreciate if you could take the time to take it for a spin then, and support me throughout the process of making this work 🙇🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

2 participants