Improving caching of S3 endpoints by omitting unused parameters (#10165)

Author: ashovlin

.changes/next-release/enhancement-s3-29532.json

@@ -0,0 +1,5 @@
+{
+  "type": "enhancement",
+  "category": "``s3``",
+  "description": "Improve caching of S3 endpoints, which should improve performance of sync and recursive copy commands that interact with multiple keys in the same bucket"
+}

awscli/botocore/endpoint_provider.py

@@ -47,6 +47,10 @@
     r"^(?!-)[a-zA-Z\d-]{1,63}(?<!-)$",
 )
 CACHE_SIZE = 100
+# S3 endpoint ruleset parameters that are defined but not currently referenced.
+# They are excluded from the cache key to avoid cache thrashing when
+# accessing multiple objects in the same bucket.
+S3_UNREFERENCED_PARAMS = {'Key', 'Prefix', 'CopySource'}
 ARN_PARSER = ArnParser()
 STRING_FORMATTER = Formatter()
 
@@ -700,16 +704,22 @@ def evaluate(self, input_parameters):
 class EndpointProvider:
     """Derives endpoints from a RuleSet for given input parameters."""
 
-    def __init__(self, ruleset_data, partition_data):
+    def __init__(self, ruleset_data, partition_data, excluded_params=None):
         self.ruleset = RuleSet(**ruleset_data, partitions=partition_data)
+        self._excluded_params = excluded_params or frozenset()
 
-    @lru_cache_weakref(maxsize=CACHE_SIZE)
     def resolve_endpoint(self, **input_parameters):
         """Match input parameters to a rule.
 
         :type input_parameters: dict
         :rtype: RuleSetEndpoint
         """
+        for param in self._excluded_params:
+            input_parameters.pop(param, None)
+        return self._resolve_endpoint(**input_parameters)
+
+    @lru_cache_weakref(maxsize=CACHE_SIZE)
+    def _resolve_endpoint(self, **input_parameters):
         params_for_error = input_parameters.copy()
         endpoint = self.ruleset.evaluate(input_parameters)
         if endpoint is None:

awscli/botocore/regions.py

@@ -25,7 +25,7 @@
 import jmespath
 from botocore import UNSIGNED, xform_name
 from botocore.auth import AUTH_TYPE_MAPS, resolve_auth_scheme_preference
-from botocore.endpoint_provider import EndpointProvider
+from botocore.endpoint_provider import S3_UNREFERENCED_PARAMS, EndpointProvider
 from botocore.exceptions import (
     EndpointProviderError,
     EndpointVariantError,
@@ -476,6 +476,11 @@ def __init__(
         self._provider = EndpointProvider(
             ruleset_data=endpoint_ruleset_data,
             partition_data=partition_data,
+            excluded_params=(
+                S3_UNREFERENCED_PARAMS
+                if service_model.service_name == 's3'
+                else None
+            ),
         )
         self._param_definitions = self._provider.ruleset.parameters
         self._service_model = service_model
@@ -522,9 +527,9 @@ def construct_endpoint(
 
         # The endpoint provider does not support non-secure transport.
         if (
-                not self._use_ssl
-                and provider_result.url.startswith('https://')
-                and 'Endpoint' not in provider_params
+            not self._use_ssl
+            and provider_result.url.startswith('https://')
+            and 'Endpoint' not in provider_params
         ):
             provider_result = provider_result._replace(
                 url=f'http://{provider_result.url[8:]}'

tests/functional/botocore/test_endpoint_rulesets.py

@@ -18,7 +18,10 @@
 import pytest
 from botocore import xform_name
 from botocore.config import Config
-from botocore.endpoint_provider import EndpointProvider
+from botocore.endpoint_provider import (
+    S3_UNREFERENCED_PARAMS,
+    EndpointProvider,
+)
 from botocore.exceptions import (
     BotoCoreError,
     ClientError,
@@ -295,3 +298,34 @@ def builtin_overwriter_handler(builtins, **kwargs):
             except (ClientError, ResponseParserError):
                 pass
         assert len(http_stubber.requests) == 0
+
+
+def _collect_refs_from_rules(rules):
+    """Recursively collect all {\"ref\": \"...\"} values from rules."""
+    refs = set()
+    if isinstance(rules, dict):
+        if 'ref' in rules:
+            refs.add(rules['ref'])
+        for value in rules.values():
+            refs.update(_collect_refs_from_rules(value))
+    elif isinstance(rules, list):
+        for item in rules:
+            refs.update(_collect_refs_from_rules(item))
+    return refs
+
+
+@pytest.mark.validates_model
+def test_s3_unreferenced_endpoint_ruleset_params():
+    """Assert that the set of known S3 endpoint ruleset parameters that are
+    not used in the rules hasn't changed. These are set in the
+    S3_UNREFERENCED_PARAMS constant to prevent them from influencing the cache.
+    """
+    ruleset = LOADER.load_service_model('s3', 'endpoint-rule-set-1')
+    all_params = set(ruleset['parameters'].keys())
+    referenced = _collect_refs_from_rules(ruleset['rules'])
+    unreferenced = all_params - referenced
+    assert unreferenced == S3_UNREFERENCED_PARAMS, (
+        "The set of unused S3 endpoints parameters have changed, examine the "
+        "implication of this on endpoint caching behavior before updating"
+        "S3_UNREFERENCED_PARAMS."
+    )

tests/unit/botocore/test_endpoint_provider.py

@@ -15,17 +15,18 @@
 import logging
 import os
 from unittest import mock
+from unittest.mock import Mock, patch
 
 import pytest
-
 from botocore.endpoint_provider import (
     EndpointProvider,
     EndpointRule,
     ErrorRule,
     RuleCreator,
     RuleSet,
+    RuleSetEndpoint,
     RuleSetStandardLibary,
-    TreeRule, RuleSetEndpoint,
+    TreeRule,
 )
 from botocore.exceptions import (
     EndpointResolutionError,
@@ -34,8 +35,6 @@
 from botocore.loaders import Loader
 from botocore.regions import EndpointRulesetResolver
 
-from unittest.mock import Mock, patch
-
 REGION_TEMPLATE = "{Region}"
 REGION_REF = {"ref": "Region"}
 BUCKET_ARN_REF = {"ref": "bucketArn"}
@@ -89,9 +88,7 @@
                 "disableDoubleEncoding": True,
                 "name": "foo",
                 "signingName": "s3-outposts",
-                "signingRegionSet": [
-                    "*"
-                ]
+                "signingRegionSet": ["*"],
             },
             {
                 "disableDoubleEncoding": True,
@@ -425,7 +422,7 @@ def empty_resolver():
             'rules': [],
         },
         partition_data={},
-        service_model=None,
+        service_model=Mock(service_name='test'),
         builtins={},
         client_context=None,
         event_emitter=None,
@@ -512,6 +509,8 @@ def test_auth_schemes_conversion_first_authtype_unknown(
 def test_get_attr(rule_lib, value, path, expected_value):
     result = rule_lib.get_attr(value, path)
     assert result == expected_value
+
+
 @pytest.mark.parametrize(
     "use_ssl, endpoint_url, provider_params, expected_url",
     [
@@ -569,7 +568,7 @@ def test_construct_endpoint_parametrized(
             'rules': [],
         },
         partition_data={},
-        service_model=None,
+        service_model=Mock(service_name='test'),
         builtins={},
         client_context=None,
         event_emitter=None,
@@ -606,22 +605,22 @@ def test_construct_endpoint_parametrized(
     ],
 )
 def test_auth_scheme_preference(
-    auth_scheme_preference,
-    expected_auth_scheme_name,
-    monkeypatch
+    auth_scheme_preference, expected_auth_scheme_name, monkeypatch
 ):
-    conditions = [
-        PARSE_ARN_FUNC,
-        {
-            "fn": "not",
-            "argv": [STRING_EQUALS_FUNC],
-        },
-        {
-            "fn": "aws.partition",
-            "argv": [REGION_REF],
-            "assign": "PartitionResults",
-        },
-    ],
+    conditions = (
+        [
+            PARSE_ARN_FUNC,
+            {
+                "fn": "not",
+                "argv": [STRING_EQUALS_FUNC],
+            },
+            {
+                "fn": "aws.partition",
+                "argv": [REGION_REF],
+                "assign": "PartitionResults",
+            },
+        ],
+    )
     resolver = EndpointRulesetResolver(
         endpoint_ruleset_data={
             'version': '1.0',
@@ -635,7 +634,7 @@ def test_auth_scheme_preference(
             ],
         },
         partition_data={},
-        service_model=None,
+        service_model=Mock(service_name='test'),
         builtins={},
         client_context=None,
         event_emitter=None,
@@ -651,13 +650,13 @@ def test_auth_scheme_preference(
         mock.patch.dict(
             'botocore.auth.AUTH_TYPE_MAPS',
             {'bar': None, 'foo': None},
-            clear=True
+            clear=True,
         ),
         mock.patch.dict(
             'botocore.auth.AUTH_PREF_TO_SIGNATURE_VERSION',
             {'bar': 'bar', 'foo': 'foo'},
-            clear=True
-        )
+            clear=True,
+        ),
     ):
         name, scheme = resolver.auth_schemes_to_signing_ctx(auth_schemes)
     assert name == expected_auth_scheme_name

tests/unit/botocore/test_utils.py

@@ -18,6 +18,7 @@
 import operator
 from contextlib import contextmanager
 from sys import getrefcount
+from unittest.mock import Mock
 
 import botocore
 import pytest
@@ -1625,7 +1626,7 @@ def setUp(self):
                 'rules': [],
             },
             partition_data={},
-            service_model=None,
+            service_model=Mock(service_name='test'),
             builtins={},
             client_context=None,
             event_emitter=None,