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

Let user specify a HTTP proxy and region #109

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LVerneyPEReN
Copy link

Hi,

In many corporate situations, an HTTP proxy is required to contact the object storage endpoint. This PR adds support for this situation, following the indications in https://docs.min.io/docs/python-client-api-reference.html (search for "corporate proxy" in the page).

Additionnally, when using another storage than AWS, the region parameter might differ and be required to be edited. This is typically the case for OVH Public Cloud services where region is to be set to an OVH region (sbg for instance).

Best,

@thomasf
Copy link
Collaborator

thomasf commented Oct 2, 2020

I'm not sure about the proxy stuff. You can create your own minio storage class in your project for that.

The reason being that these settings are kind of arbitrary and different projects might want different settings and I don't want to litter settings with endless amount of options so maybe it's better to construct as a project local storage class?

retries=urllib3.Retry(
                total=5,
                backoff_factor=0.2,
                status_forcelist=[500, 502, 503, 504],
            )

I will think about it some more..

@thomasf
Copy link
Collaborator

thomasf commented Oct 3, 2020

I am btw. working on a new settings format that will make sublcassing the storage class and using whatever settings use less boilerplate..

(work in progress)

the goal is probably to support dict settings that looks something like this (along with a compat settings generator for old style settings)

MINIO = {
    "ENDPOINT": "localhost:9000",
    "ACCESS_KEY": "KBP6WXGPS387090EZMG8",
    "SECRET_KEY": "DRjFXylyfMqn2zilAr33xORhaYz5r9e8r37XPz3A",
    "HTTPS": False,
    "BUCKET": "local-media",
    "CREATE_BUCKET": True,
}

MINIO_MEDIA = {
    "BUCKET_POLICY": "GET",
    "BACKUP_FORMAT": "%c/".
    "BACKUP_BUCKET": "Recycle Bin",
}

I'm note really in a hurry to complete this but maybe the next month or so, better take it slow and reflect over how the settings formats feels .

The point is to make subclassing of the storage class much more convenient and powerful to allow django projects to have many of them easily and customize them with proxies etc.

@LVerneyPEReN
Copy link
Author

I'm not sure about the proxy stuff. You can create your own minio storage class in your project for that.

If this is "my" project, sure this is doable. I came across this particular issue while trying to make a third-party software relying on django-minio-storage work. In such a case, it's way easier to simply edit the settings than to hack into the actual source code to make it work.

The reason being that these settings are kind of arbitrary and different projects might want different settings and I don't want to litter settings with endless amount of options so maybe it's better to construct as a project local storage class?

HTTP/HTTPS proxies are usually quite well formatted (see HTTP_PROXY / HTTPS_PROXY environment variables under Linux for instance). The detailed options here are just a copy/paste from the recommended snippet from Minio doc.

the goal is probably to support dict settings that looks something like this (along with a compat settings generator for old style settings)

If we could pass directly the proxies kwarg, this would be great.

@thomasf
Copy link
Collaborator

thomasf commented Oct 5, 2020

If this is "my" project, sure this is doable. I came across this particular issue while trying to make a third-party software relying on django-minio-storage work. In such a case, it's way easier to simply edit the settings than to hack into the actual source code to make it work.

How.. this should not be a problem. The storages should usually be defined by the project..

If the third party project uses the default media storage and you have your local project class you should be able to use it like this.

DEFAULT_FILE_STORAGE = "my_project.storages.MyMinioMediaStorage"

A third party app should generally not define their own storages because the project should be able to choose which storage classes it uses. If it does it should probably have a django setting to allow control of which storage class to use.

@LVerneyPEReN
Copy link
Author

Indeed, the project I was trying to run is defining it this way https://github.com/Exodus-Privacy/exodus/blob/v1/exodus/exodus/settings/base.py#L140.

Yet, changing it means:

  • Finding out about Django's file storage mechanism and this particular setting.
  • Writing a custom Python class to overload the default Minio storage class with the proper passing of proxies option (hence digging into this repository source code to find the right way to do it)

Compare it with a proper dedicated setting where it would basically be reduced to googling "MINIO_STORAGE_ENDPOINT", coming across this repository documentation and seeing it is possible to pass a standardized HTTP_PROXY format string as "MINIO_****".

@thomasf
Copy link
Collaborator

thomasf commented Oct 5, 2020

doesn't python obey the http_proxy / https_proxy environment variables for global proxy configuation? (I have not checked this)

@LVerneyPEReN
Copy link
Author

Long story short, requests does, urllib3 does not. And from my experience, as soon as you start putting celery in the middle, it's yet another story.

@thomasf
Copy link
Collaborator

thomasf commented Oct 5, 2020

I'll leave this issue open for now..

There is a work around with a custom class for each project which will have to work for now. Some users are already confused by all the settings as is and because of that I want to finish the simplification and renaming of settings first.

After that is done I will revisit this and make a decision. Until then projects will have to subclass minio storage if they need to add their own proxy setting.

Django settings is code as configuration so adding a class certainly can be viewed as being within the limits of deployment configuration. If it's hard for users understand how to do it it's maybe a question of adding a project local sublass example to the usage docs of django-minio-storage

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