Skip to content

Conversation

@alexrashed
Copy link
Member

@alexrashed alexrashed commented Feb 6, 2024

Motivation

This PR addresses multiple issues around the ASF API updates.
In the past we had issues several times because a botocore update slipped in (botocore was not pinned), while the ASF APIs have not been updated (they only run every Monday morning).
This can then cause issues in the fallback forwarding due to our strict APIs:

  • The operation calls are dispatched using keyword arguments.
  • Our parser is based on the specs contained in the botocore package.
    • This means that our parser can parse new parameters from an incoming request if the botocore package is updated, but the operation's method signature has not been updated yet (adding this new parameter to the list of keyword arguments).
  • When dispatching the parsed request (including a new parameter), we can a TypeError.

The last instance of this issue occurred during our 3.1 release: CircleCI Job # 175308

botocore.exceptions.ClientError: An error occurred (InternalError) when calling the CreateRouteTable operation (reached max retries: 0): exception while calling ec2.CreateRouteTable: Traceback (most recent call last):
  File "/opt/code/localstack/.venv/lib/python3.11/site-packages/rolo/gateway/chain.py", line 166, in handle
    handler(self, self.context, response)
  File "/opt/code/localstack/localstack/aws/handlers/service.py", line 112, in __call__
    handler(chain, context, response)
  File "/opt/code/localstack/localstack/aws/handlers/service.py", line 82, in __call__
    skeleton_response = self.skeleton.invoke(context)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/localstack/localstack/aws/skeleton.py", line 154, in invoke
    return self.dispatch_request(serializer, context, instance)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/localstack/localstack/aws/skeleton.py", line 168, in dispatch_request
    result = handler(context, instance) or {}
             ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/localstack/localstack/aws/forwarder.py", line 135, in _call
    return handler(context, req)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/localstack/localstack/aws/skeleton.py", line 118, in __call__
    return self.fn(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/code/localstack/localstack/aws/api/core.py", line 161, in operation_marker
    return fn(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^
TypeError: Ec2Api.create_route_table() got an unexpected keyword argument 'client_token'

This PR aims at solving this issue with two different approaches at once:

  • We pin botocore and only update it together with the generated APIs.
  • We implement forward-compatibility in the ASF APIs by adding **kwargs to all ASF API definitions and implementations.
    • This is necessary, because we have other projects which are also generating APIs based on botocore. In these projects we cannot pin botocore, because it's transitively pinned though localstack-core.

This PR is basically an extension of #10127 (now that we have proper dependency pinning, explicitly ping botocore and tie it to the ASF updates).

Changes

The changes are clearly separated in different commits:

  1. updates the scaffold to add **kwargs to all generated API operations.
  2. pins the version of botocore in the setup.cfg to the version which was last used to update the ASF APIs (Update ASF APIs #10173).
  3. changes the ASF Update workflow to also update the pin on botocore.
  4. updates the generated APIs to reflect the latest changes in the scaffold
  5. updates the service provider implementations after the API (superclasses) updates

Testing

  • We do have a nice unit test which checks that the provider signatures do not differ from the signatures of the generated APIs: test_asf_providers.py
  • I verified manually that the catching all keyword args in the generated APIs fixes the issue outlined above.
  • We have lots of integration tests testing the providers ;)
  • I manually ran the ASF update action which resulted in the following PR: Update ASF APIs #10190

@alexrashed alexrashed added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Feb 6, 2024
@alexrashed alexrashed added this to the 3.2 milestone Feb 6, 2024
@alexrashed alexrashed self-assigned this Feb 6, 2024
@github-actions
Copy link

github-actions bot commented Feb 6, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 11s ⏱️
386 tests 336 ✅  50 💤 0 ❌
772 runs  672 ✅ 100 💤 0 ❌

Results for commit fc24ad8.

♻️ This comment has been updated with latest results.

@alexrashed
Copy link
Member Author

The last step - updating the service provider implementations after the API superclasses were updated - would have meant to iterate over hundreds of operations manually. In order to avoid this, I wrote a small script which uses inspect and libcst to perform the modification automatically:

import inspect
from collections import defaultdict
from pathlib import Path
from types import FunctionType

import libcst

from localstack.testing.aws.asf_utils import collect_implemented_provider_operations

# collect all provider operations
provider_ops = collect_implemented_provider_operations()

# iterate over all operations and group all operations which need to be modified per provider file
operations_per_provider_file: dict[str, list[str]] = defaultdict(list)
for provider_class, base_class, provider_function in provider_ops:
    try:
        sub_function = getattr(provider_class, provider_function)
    except AttributeError:
        raise AttributeError(
            f"Given method name ('{provider_function}') is not a method of the sub class ('{provider_class.__name__}')."
        )

    if not isinstance(sub_function, FunctionType):
        raise AttributeError(
            f"Given method name ('{provider_function}') is not a method of the sub class ('{provider_class.__name__}')."
        )

    if not getattr(sub_function, "expand_parameters", True):
        # if the operation on the subclass has the "expand_parameters" attribute (it has a handler decorator) set to False, we don't care
        continue

    if wrapped := getattr(sub_function, "__wrapped__", False):
        # if the operation on the subclass has a decorator, unwrap it
        sub_function = wrapped

    operations_per_provider_file[inspect.getfile(provider_class)].append(sub_function.__name__)


class ProviderMethodTransformer(libcst.CSTTransformer):
    def __init__(self, operations: list[str]):
        self.operations = operations
        super().__init__()

    def leave_FunctionDef(
        self, original_node: libcst.FunctionDef, updated_node: libcst.FunctionDef
    ) -> libcst.CSTNode:
        if original_node.name.value in self.operations and not original_node.params.star_kwarg:
            print(f"  -> {original_node.name.value}")
            new_params = updated_node.params.with_changes(
                star_kwarg=libcst.Param(name=libcst.Name("kwargs"))
            )
            return updated_node.with_changes(params=new_params)

        return updated_node


# iterate over all providers and tranform the found operations
for provider_file, ops in operations_per_provider_file.items():
    print(f"- {provider_file}")
    module = libcst.parse_module(Path(provider_file).read_bytes())
    updated_module = module.visit(ProviderMethodTransformer(ops))
    Path(provider_file).write_text(updated_module.code)

@alexrashed alexrashed force-pushed the pin-asf-api-botocore branch from 9c156f1 to ecf10b1 Compare February 6, 2024 14:09
@coveralls
Copy link

coveralls commented Feb 6, 2024

Coverage Status

coverage: 83.839% (-0.02%) from 83.861%
when pulling fc24ad8 on pin-asf-api-botocore
into 871bfb8 on master.

@github-actions
Copy link

github-actions bot commented Feb 6, 2024

LocalStack Community integration with Pro

    2 files      2 suites   1h 21m 24s ⏱️
2 622 tests 2 373 ✅ 249 💤 0 ❌
2 624 runs  2 373 ✅ 251 💤 0 ❌

Results for commit fc24ad8.

♻️ This comment has been updated with latest results.

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

Changes look great! Just before we go this route: have you thought about solving this on the dispatcher level, i.e., filtering the kwargs in the Skeleton based on the handler's signature, so superfluous arguments don't even make it into the method call?

That would limit the necessary code changes. And I'm worried that we will become lazy with updating providers, now that we also no longer get warnings in the IDE that signatures between API stubs and providers no longer match, and drift will become worse over time.

@alexrashed
Copy link
Member Author

alexrashed commented Feb 6, 2024

Good question, and I thought about this a lot actually. I would argue that:

  • The check would be an unnecessary runtime overhead.
  • This will still be enforced by the unit test mentioned above.
    • I was the only one who proactively added new parameters every Monday with the ASF PRs. There wasn't a single contribution from any service owners (also because distributing that would take longer than fixing it). I will continue doing that for now.
  • Service owners would still get the reviewer notification through the CODEOWNERS config.
  • We could actually use this in the future in some providers (if we decide to weaken the unit test enforcing the equality of the functions).
  • In the future we could even implement this adjustment to be done automatically, with a modification of the script I posted above.

@alexrashed alexrashed force-pushed the pin-asf-api-botocore branch 2 times, most recently from 134c2f9 to 7665953 Compare February 6, 2024 17:31
@alexrashed alexrashed mentioned this pull request Feb 6, 2024
@alexrashed alexrashed force-pushed the pin-asf-api-botocore branch from 7665953 to 5cfc33f Compare February 6, 2024 17:43
Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

well clearly you thought about it so let's go with this solution :-) other than that i don't like the look of the trailing **kwargsattribute in every method, i don't have a strong argument against it right now. the runtime overhead of filtering a dict would be negligible though.

anyway, great work on the overall changes!

@alexrashed alexrashed force-pushed the pin-asf-api-botocore branch from 5cfc33f to fc24ad8 Compare February 7, 2024 07:46
@alexrashed alexrashed merged commit d808535 into master Feb 7, 2024
@alexrashed alexrashed deleted the pin-asf-api-botocore branch February 7, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants