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 minio support via http #147

Closed
wants to merge 1 commit into from
Closed

add minio support via http #147

wants to merge 1 commit into from

Conversation

ruledio
Copy link

@ruledio ruledio commented Mar 20, 2018

Adds minio support via HTTP,

@googlebot
Copy link
Collaborator

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
  • Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
  • The email used to register you as an authorized contributor must also be attached to your GitHub account.

@ruledio
Copy link
Author

ruledio commented Mar 20, 2018

I signed it!

@googlebot
Copy link
Collaborator

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@ruledio
Copy link
Author

ruledio commented Mar 20, 2018

I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@willnorris
Copy link
Owner

rather than add an entirely new cache, how hard would it be to modify the existing s3 implementation to support minio as well? I've never used minio, so am not sure what additional pieces it really needs. Can you give me an example of a minio URL? I get that endpoint would point to your minio instance, but what value do you use for region in that case?

@willnorris
Copy link
Owner

willnorris commented Mar 20, 2018

As an example of how I was thinking we could modify the existing s3 cache, what if we pushed any of these additional config options we need into the query string? So you could use minio with a URL such as:

s3://{region}/{bucket}/{path}?endpoint={endpoint}&disableSSL=1&forcePathStyle=1

Or something like that? Or are there other changes for minio that I overlooked? (And actually, could we safely just enable forcePathStyle all the time, even for AWS? Is there a downside to that?)

@ruledio
Copy link
Author

ruledio commented Mar 20, 2018

Not a bad shout, the reason I kept it separate was to keep things simple and not to confuse current users with changes, more than happy to take the advice and take a stab at modifying the current s3cache, I am not a heavy user of s3 or minio so I wouldn't like to comment on the forcePathStyle.

Let me know your thoughts.

@ruledio
Copy link
Author

ruledio commented Mar 20, 2018

ok so looking at this a bit more I can see a few issues that I am not sure how to resolve, in the current URL the region is S3's endpoint, changing this behavior is more than likely going to have an impact on current users, I like the idea of using query strings but not sure how this will work.

S3: s3://{region}/{bucket}/{path} s3://s3-us-east-2.amazonaws.com/bucketname/images
Minio: http://{endpoint}/{bucket}/{path} http://minio.dev:9000/bucketname/images

Any advice here would be great.

@dvaldivia
Copy link

I'd also love to have support for minio, this PR seems to be passing everything, can it be merged?

@willnorris
Copy link
Owner

I would still much rather find a way to make these kinds of s3-compatible services work with the existing s3cache implementation, even if that means we need to make some adjustments to accommodate that. Especially since all this seems to be doing is duplicating the s3cache with just a couple of extra options on the underlying aws.Config struct.

I can take a quick stab at what that might look like, but don't have an easy way to test it. I can try setting up a minio instance when I have a chance, but that may not be anytime soon.

@dvaldivia
Copy link

Thanks for the reply, is there any way I can help you with this? Running minio using docker is quite easy.

Let me know if I can help somehow

willnorris added a commit that referenced this pull request Jan 12, 2019
this should allow using at least some s3-compatible services like minio.

Ref #120, #147
@willnorris
Copy link
Owner

Could you try testing the "s3compat" branch I just pushed and see if that works?

https://github.com/willnorris/imageproxy/tree/s3compat

@ruledio
Copy link
Author

ruledio commented Mar 15, 2019

@willnorris this is working perfectly for me, testing with Minio.

@willnorris
Copy link
Owner

Awesome, thanks for confirming @ruledio! I'll try to get that merged into master and released later today.

@ruledio
Copy link
Author

ruledio commented Mar 15, 2019

A quick note Minio doesn't really have regions so you have to set a fake one on the s3 URL

s3://fake-region/bucket-name/folder-name?endpoint=minio:9000&disableSSL=1&s3ForcePathStyle=1

willnorris added a commit that referenced this pull request Mar 16, 2019
this should allow using at least some s3-compatible services like minio.

Ref #120, #147
willnorris added a commit that referenced this pull request Mar 16, 2019
this should allow using at least some s3-compatible services like minio.

Fixes #120, #147
@willnorris
Copy link
Owner

closing, since e860748 is now merged.

@willnorris willnorris closed this Mar 16, 2019
@ruledio ruledio deleted the minio branch August 9, 2019 15:07
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.

4 participants