bug: AWS S3 SDK v3 (PR #221) does not seem to work with Digital Ocean Spaces

[mman]

I am experimenting a bit with the PR #221 and although it works nicely with AWS S3, it does not seem to work with DO spaces. There are two issues.

1. When you use Read/Write/Delete bucket scoped access key, the s3 adapter fails with AccessDenied error when trying to create already existing bucket. We always try to createBucket before performing any other operation.
2. When using FullAccess access key, the s3 adapter fails with BucketAlreadyExists error. If the bucket does not exist, first save will create it, and any other use of the adapter will fail with BucketAlreadyExists error.

Looks like there is a difference in how AWS S3 JS SDK v2 and v3 handled the response of createBucket API call, although I could not find (yet) the offending line of code to confirm it.

In any way, PR #221 does not seem to work with DO Spaces as is.

[parse-github-assistant]

Thanks for opening this issue!

[mman]

So this commit here: mman@0f8e404 swallows the BucketAlreadyExists error the same way it swallows the BucketAlreadyOwnedByYou and allows the FullAccess access key to work with the DO.

To support using the limited Read/Write/Delete access key we need to modify the logic to first use headBucket.

[mman]

And this commit here: mman@8fb6f0e uses HeadBucketCommand to verify existence and access rights to a bucket before attempting to create it. This allows use of Read/Write/Delete access key provided by DO.

[vahidalizad]

Thank you for reporting this and for taking the time to dig into it and experiment with the PR

I think the first fix (swallowing BucketAlreadyExists) is a better choice since it keeps the logic simple. The second fix using HeadBucketCommand achieves the same result but only adds complexity to the code without additional value, as errors will naturally arise during operations like delete or write if permissions are insufficient.

I’m open to discussion if you feel otherwise let me know your thoughts

[mman]

@vahidalizad The second fix and usage of HeadBucketCommand is necessary if you want to configure the adapter with access key that lacks permission to create buckets. In my deployments I always create a bucket, and then create access key with least permissions required. Digital Ocean these days offers only two types of keys, FullAccess, and ReadWriteDelete. The second one can do every operation on a bucket but can not create it and thus fails in the existing code with AccessDenied whereas using the HeadBucketCommand works as expected for already existing buckets.

[vahidalizad]

@vahidalizad The second fix and usage of HeadBucketCommand is necessary if you want to configure the adapter with access key that lacks permission to create buckets. In my deployments I always create a bucket, and then create access key with least permissions required. Digital Ocean these days offers only two types of keys, FullAccess, and ReadWriteDelete. The second one can do every operation on a bucket but can not create it and thus fails in the existing code with AccessDenied whereas using the HeadBucketCommand works as expected for already existing buckets.

Oh, now I get it! Thanks for clarifying. We'll go with the second fix then.

If you agree, we can update it to:

  try {
    // Check if the bucket exists
    await this._s3Client.send(new HeadBucketCommand({ Bucket: this._bucket }));
    this._hasBucket = true;
  } catch (error) {
    if (error.name !== 'NotFound') {
      // If the error is something other than "NotFound", rethrow it
      throw error;
    }

    // If the bucket does not exist, attempt to create it
    try {
      await this._s3Client.send(new CreateBucketCommand({ Bucket: this._bucket }));
      this._hasBucket = true;
    } catch (creationError) {
      // Handle specific errors during bucket creation
      if (creationError.name === 'BucketAlreadyExists' || creationError.name === 'BucketAlreadyOwnedByYou') {
        this._hasBucket = true;
      } else {
        throw creationError;
      }
    }
  }

[mman]

Looks clean to me :-) Thanks!

[mtrezza]

Does anyone want to open a PR? Or has there already been one opened and not linked?

[mman]

@mtrezza I got lost a bit but I think it has already been fixed as part of the original PR. I am using it in production since January with restricted DO keys and everything works as expected, so this one can be closed.

[mtrezza]

Even better, thanks for confirming!

Closed via #221