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

Make django-mini-backend Region aware #39

Merged
merged 2 commits into from
Feb 4, 2023

Conversation

subho007
Copy link
Contributor

Similar PR was raised at #38 but unfortunately it was not a complete implementation.

The following were changed and added:

  • Get MINIO_REGION value from settings or set it to us-east-1 which is the default value (Minio itself uses that as a default value)
  • If the bucket is public, generate the base URL from the instantiated Minio Client and not hardcode the base URL
  • Django Example App and README.md is updated

This Fixes few problems:

  • Hosted minio solutions are now Region aware, this PR makes it compatible
  • Works with Amazon S3 services with all their latest security changes which they have implemented.

I have already tested it out with local/hosted instance of minio and with AWS S3 service. Forked version of this is published in my PyPI: https://pypi.org/project/ak-django-minio-backend/ (I'll delete as soon as this gets merged, tested, approved released)

@theriverman
Copy link
Owner

thank you very much for the PR, @subho007 - looks like a very high quality set of changes. i'll review it ASAP

Copy link
Owner

@theriverman theriverman left a comment

Choose a reason for hiding this comment

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

i propose to update the return statement to avoid duplicating slashes.

besides that I'm trying to figure out what's wrong with the old method. the Minio client is instantiated from the very same settings.py parameters, therefore, i don't see it as hardcoding. could you point that out for me, please?

django_minio_backend/models.py Outdated Show resolved Hide resolved
The default Region is set to us-east-1
the public URL generated for AWS specific resources is diferent than
minio specific servers. Still, minio SDK has a special reference to
figure out how to generate URLs if AWS is used in the backend.
@subho007
Copy link
Contributor Author

subho007 commented Feb 3, 2023

@theriverman I apologise for not being clear about the issue.

The old implementation takes the hardcoded value written in settings.py to generate the URL, which unfortunately breaks if you use AWS S3 as an endpoint, since AWS S3 endpoint is a location aware endpoint.

Which means, if my S3 bucket is hosted in ap-south-1 region, the GET url should be s3.ap-south-1.amazonaws.com and not s3.amazonaws.com. This is handled in minio-py implementation to get the generated location aware URL for GET request. This is also true if you have S3 acceleration on, and other S3 compliant implementation of aliyuncs

The reference to this is here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L426-L458

The region specific parameters are added here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L535-L572

And for S3 accelerated URL parameters are added here: https://github.com/minio/minio-py/blob/master/minio/helpers.py#L555-L578

Common hosted minio solution may not have any affect, but projects which uses minio instead of boto requires the URLs to be build in specific way with region aware URLs.

Hence, I am using the minio-client itself to get the generated URLs instead of relying on the value in settings.py

Note: You may say why don't I put in MINIO_HOST=s3.ap-south-1.amazonaws.com ? Unfortunately, Minio only understands s3.amazonaws.com and then reconstructs the URLs based on Region/Accelerate properties

@theriverman
Copy link
Owner

@subho007 thank you very much for the changes and for the brief explanation of your request. i've missed that initial detail you aim to support communication towards AWS S3 too. When I was implementing this package I was focusing solely on MinIO, but I don't have anything against this rather minor change.

@theriverman theriverman closed this Feb 4, 2023
@theriverman theriverman reopened this Feb 4, 2023
@theriverman theriverman merged commit abeed73 into theriverman:master Feb 4, 2023
@theriverman
Copy link
Owner

@subho007 v3.4.0 pushed to PyPI. thanks a lot for your contribution!

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.

2 participants