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

Add Ceph RGW S3 compatibility. #21

Closed
wants to merge 3 commits into from
Closed

Conversation

ismaelpuerto
Copy link

Support to use ceph o minio as S3 provider

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2022

CLA assistant check
All committers have signed the CLA.

@oleiade
Copy link
Member

oleiade commented Oct 31, 2022

Hi @ismaelpuerto

Thanks a lot for your contribution 🙇🏻 🎉

First and foremost, could you give me a quick run-through of what Ceph is as I'm not familiar with it?

I see most of the changes in your PR are related to having the ability to set the target host manually. We have been working on improving the signature code, and have introduced a settable host for all AWSClient inherited classes, such as S3Client. I believe this would address what you're trying to achieve.

This is a matter of days before we merge it to master, thus, I'd like for us to wait until it's the case to reevaluate this PR. I think once the changes are merged, you might be able to perform the actions you want to perform, without any custom code in the library.

Let me know what you think 🙇🏻

@ismaelpuerto
Copy link
Author

Hello @oleiade

Thanks for your fast reply, Ceph it's a software-defined storage with the capacity to expose S3. The structure of url in ceph is:

http(s)?://hostname/bucketname/file1.txt

It can serve this at amazon style but require additional configuration in DNS. More info in this link https://docs.ceph.com/en/quincy/radosgw/s3/commons/

Happen the same with minio

I didn't see the branch when I did this PR, but I thing that cover my case. I suppose that the new branch it's a WIP but I can test and give you feedback. Be free if you want close this PR.

Thanks

@imiric imiric mentioned this pull request Nov 17, 2022
Copy link

@imiric imiric left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @ismaelpuerto! 🙇

I would prefer making these changes agnostic to any S3-compatible API, so that we could support MinIO as well, like you mention. And it should also have documentation in the README for how to use it, in a separate section (e.g. "Support for S3-compatible APIs"), with examples for Ceph and MinIO.

EDIT: Apologies, I didn't read @oleiade's response above in detail. In that case, if host can be manually set, then it should work for all S3-compatible APIs. So, indeed, let's wait until those changes are merged.

*
* @type {boolean}
*/
rgw?: booleaan
Copy link

Choose a reason for hiding this comment

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

Suggested change
rgw?: booleaan
rgw?: boolean

Although, is this property required? We should aim to make this library work with any S3-compatible API, so adding some Ceph-specific options seems unnecessary.

Could we make this work by just overriding endpoint?

@jayush12
Copy link

Thanks for the PR @ismaelpuerto! 🙇

I would prefer making these changes agnostic to any S3-compatible API, so that we could support MinIO as well, like you mention. And it should also have documentation in the README for how to use it, in a separate section (e.g. "Support for S3-compatible APIs"), with examples for Ceph and MinIO.

EDIT: Apologies, I didn't read @oleiade's response above in detail. In that case, if host can be manually set, then it should work for all S3-compatible APIs. So, indeed, let's wait until those changes are merged.

Hello @imiric ,

Any update when the new changes with capability to set host will be available for public use. And are you considering about giving capability to set endpoint and port also.

Also for setting host externally, will it work for min.io if we pass host url along with port

@oleiade
Copy link
Member

oleiade commented Nov 18, 2022

Hey folks @jayush12 @ismaelpuerto

Sorry for the lack of recent updates on this. Although we initially intended to merge the signature rewrite sooner we weren't able to prioritize it yet. I expect we should be able to move forward with this in the next couple of weeks 👍🏻

@oleiade
Copy link
Member

oleiade commented Dec 8, 2022

Hi folks, we have published version v0.7.0 of the sdk, which supports settings a client's host directly. We do it in the context of our tests here.

@ismaelpuerto could you please rebase your PR on master, and let me know if your need is addressed by our recent changes? Otherwise I'm happy to discuss this PR further 🙇🏻

@ismaelpuerto
Copy link
Author

Hello @oleiade

Thanks, I will work with the last version and come back with a new PR, additionally I will change the name of variable: rgw for forcePathStyle to be more generic as propose @imiric.

@oleiade
Copy link
Member

oleiade commented May 5, 2023

As detailled in #26 I believe this has been addressed in #44.

Closing. Feel free to reopen if you're still experiencing the issue.

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.

5 participants