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

[s3] Add client_config configuration setting #1386

Merged
merged 1 commit into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions docs/backends/amazon-S3.rst
Original file line number Diff line number Diff line change
Expand Up @@ -242,12 +242,25 @@ Settings
The signature versions are not backwards compatible so be careful about url endpoints if making this change
for legacy projects.

``client_config`` or ``AWS_S3_CLIENT_CONFIG``

Default: ``None``

An instance of ``botocore.config.Config`` to do advanced configuration of the client such as
``max_pool_connections``. See all options in the `Botocore docs`_.

.. note::

Setting this overrides the settings for ``addressing_style``, ``signature_version`` and
``proxies``. Include them as arguments to your ``botocore.config.Config`` class if you need them.

.. _AWS Signature Version 4: https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html
.. _S3 region list: https://docs.aws.amazon.com/general/latest/gr/s3.html#s3_region
.. _list of canned ACLs: https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl
.. _Boto3 docs for uploading files: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.put_object
.. _Boto3 docs for TransferConfig: https://boto3.amazonaws.com/v1/documentation/api/latest/reference/customizations/s3.html#boto3.s3.transfer.TransferConfig
.. _ManifestStaticFilesStorage: https://docs.djangoproject.com/en/3.1/ref/contrib/staticfiles/#manifeststaticfilesstorage
.. _Botocore docs: https://botocore.amazonaws.com/v1/documentation/api/latest/reference/config.html#botocore.config.Config

.. _cloudfront-signed-url-header:

Expand Down
18 changes: 14 additions & 4 deletions storages/backends/s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
import boto3.session
import s3transfer.constants
from boto3.s3.transfer import TransferConfig
from botocore.client import Config
from botocore.config import Config
from botocore.exceptions import ClientError
from botocore.signers import CloudFrontSigner
except ImportError as e:
Expand Down Expand Up @@ -318,8 +318,17 @@ def __init__(self, **settings):
self._bucket = None
self._connections = threading.local()

if not self.config:
self.config = Config(
if self.config is not None:
warnings.warn(
"The 'config' class property is deprecated and will be "
"removed in a future version. Use AWS_S3_CLIENT_CONFIG "
"to customize any of the botocore.config.Config parameters.",
DeprecationWarning,
)
self.client_config = self.config

if self.client_config is None:
Copy link

@mgzwarrior mgzwarrior Jun 14, 2024

Choose a reason for hiding this comment

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

I'm thinking that there is an unintended logical change here that makes self.client_config a required field, defaulted to None. See here for more - #1410

Copy link
Owner Author

Choose a reason for hiding this comment

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

See the logic here.

Choose a reason for hiding this comment

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

I don't think I am following. The get_default_settings() method here seems to return {} by default.

In our case, we have overridden this method as such:

    def get_default_settings(self):
        return {
            "access_key": setting(f'{self.setting_prefix}_ACCESS_KEY_ID'),
            "secret_key": setting(f'{self.setting_prefix}_SECRET_ACCESS_KEY'),
            ...,
        }

However, we don't have a setting for client_config because this is new. This seems to be the root cause of the error: object has no attribute 'client_config'. One solution I could try is to add a dummy attribute to the above method and default to None, but I shouldn't have to if the setting is not required, right?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't provide backwards compat guarantees on get_default_settings as that is not a public API. If you want this to reliably work I'd recommend something like

settings = super().get_default_settings()
...your changes here

self.client_config = Config(
s3={"addressing_style": self.addressing_style},
signature_version=self.signature_version,
proxies=self.proxies,
Expand Down Expand Up @@ -407,6 +416,7 @@ def get_default_settings(self):
"default_acl": setting("AWS_DEFAULT_ACL", None),
"use_threads": setting("AWS_S3_USE_THREADS", True),
"transfer_config": setting("AWS_S3_TRANSFER_CONFIG", None),
"client_config": setting("AWS_S3_CLIENT_CONFIG", None),
}

def __getstate__(self):
Expand All @@ -430,7 +440,7 @@ def connection(self):
region_name=self.region_name,
use_ssl=self.use_ssl,
endpoint_url=self.endpoint_url,
config=self.config,
config=self.client_config,
verify=self.verify,
)
return self._connections.connection
Expand Down
13 changes: 13 additions & 0 deletions tests/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import boto3
import boto3.s3.transfer
from botocore.config import Config as ClientConfig
from botocore.exceptions import ClientError
from django.conf import settings
from django.core.exceptions import ImproperlyConfigured
Expand Down Expand Up @@ -42,6 +43,18 @@ def test_s3_session(self):
_ = storage.connection
mock_session.assert_called_once_with(profile_name="test_profile")

def test_client_config(self):
with override_settings(
AWS_S3_CLIENT_CONFIG=ClientConfig(max_pool_connections=30)
):
storage = s3.S3Storage()
with mock.patch("boto3.Session.resource") as mock_resource:
_ = storage.connection
mock_resource.assert_called_once()
self.assertEqual(
30, mock_resource.call_args[1]["config"].max_pool_connections
)

def test_pickle_with_bucket(self):
"""
Test that the storage can be pickled with a bucket attached
Expand Down
Loading