improve parity for APIGW Resource #7738

bentsku · 2023-02-23T13:32:33Z

Work on APIGW Resource parity, improve UpdateResource parity (patch operations validation), remove moto patches to move them into provider directly (we still manipulates moto entities directly as its tightly integrated, but extend moto with our own store), and fix deleting child resources when deleting a resource that was used as a parent.

Example:
/ -> /pets -> /pets/dogs
We delete /pets, /pets/dogs should be deleted as well because it declared /pets to be its parent.
We do this by mapping children IDs when they are created with a parent ID. When deleting a resource, we check if it is in the parent list, and then recursively delete resources.

remove patches on Resource entity
create AWS validated tests with snapshots for resource manipulation
fix parity for UpdateResource (validation)
fix recursively deleting all child Resource when deleting a parent Resource with DeleteResource

note: I might delete the fixture for creating resource in a following PR, as we clean up all related resources when deleting the RestAPI.
note2: more tests are coming for deleting the resource with more children in a following PR. (#7746)
note3: I'd like to do a store refactor to allow easier validation of resources, see #7750

\cc @whummer

github-actions · 2023-02-23T15:18:59Z

LocalStack integration with Pro

      3 files ±0       3 suites ±0 1h 30m 58s ⏱️ - 2m 46s
1 762 tests +3 1 392 ✔️ +4 370 💤 - 1 0 ❌ ±0
2 480 runs +3 1 768 ✔️ +4 712 💤 - 1 0 ❌ ±0

Results for commit 36b13dc. ± Comparison against base commit 145291f.

♻️ This comment has been updated with latest results.

coveralls · 2023-02-23T21:27:33Z

Coverage: 85.091% (+0.06%) from 85.033% when pulling 36b13dc on apigw-parity-work-2 into 145291f on master.

calvernaz

Thanks for doing this, I know working with the split state doesn't make it easier. LGTM 🚀

I have some comments, that can be handle in next iterations

calvernaz · 2023-02-26T20:37:36Z

localstack/services/apigateway/provider.py

@@ -89,6 +93,16 @@ def on_after_init(self):
    def _get_moto_backend(context: RequestContext) -> apigw_models.APIGatewayBackend:
        return apigw_models.apigateway_backends[context.account_id][context.region]

+    @staticmethod
+    def _remove_rest_api(context: RequestContext, rest_api_id: str) -> None:
+        store = get_apigateway_store(account_id=context.account_id, region=context.region)


it removes from Localstack store, should we check on Moto's as well ?

The call_moto should take care of that as the rest API is contained there, I'm not sure how this could happen in moto

calvernaz · 2023-02-26T20:45:20Z

tests/integration/apigateway/test_apigateway_api.py

@@ -73,6 +75,25 @@ def _factory(*args, **kwargs):
        delete_rest_api_retry(apigateway_client, rest_api_id)


+@pytest.fixture
+def apigw_create_rest_resource(apigateway_client):


I'm curious about how to create this fixture. Deleting the API container should remove most of the resources created under path, templates, etc - backend integration lingers tho. Any particular use case where this didn't happen? meaning deleting the API wasn't removing resources?

I'm deleting it, it was me not exactly knowing how it would behave. Thanks!

calvernaz · 2023-02-26T20:49:44Z

localstack/services/apigateway/provider.py

+    def _remove_rest_api(context: RequestContext, rest_api_id: str) -> None:
+        store = get_apigateway_store(account_id=context.account_id, region=context.region)
+        # clean up this way until we properly encapsulate RestApi in the store
+        store.authorizers.pop(rest_api_id, None)


Since we want to move the state from moto to localstack, in a next iteration we can functionality to the store to delete an API, basically wrapping a method to all the operation below.

Right, I'm not sure about adding methods to the store, I've seen it done in some cases, not sure what the proper way of doing it is, any idea @thrau?

there's something in the old S3 provider which wraps the moto backend with accessor/helper functions. other than that i know of no such patterns.
if you have split brain between moto and localstack stores, and it's a pain, we can look into establishing a pattern.

calvernaz · 2023-02-26T20:53:30Z

localstack/services/apigateway/provider.py

@@ -844,6 +972,17 @@ def filter_function(item):
 # ---------------


+def get_moto_rest_api(context: RequestContext, rest_api_id: str) -> MotoRestAPI:


we do have this method self._get_moto_backend(context) on the provider similar to this one. This one seems to handle mis-lookups, should we highlight that and remove usages of the former?

You're right, the self._get_moto_backend(context) was from my first PR, but not sure it still makes sense now, this new method handles a bit more. If we ever need to access the moto backend to access other kind of resources and not RestAPI, we can create a new method for that resource with the proper exception message. Thanks for picking this up!

bentsku temporarily deployed to localstack-ext-tests February 23, 2023 13:32 — with GitHub Actions Inactive

bentsku temporarily deployed to localstack-ext-tests February 23, 2023 20:37 — with GitHub Actions Inactive

bentsku temporarily deployed to localstack-ext-tests February 24, 2023 09:50 — with GitHub Actions Inactive

bentsku temporarily deployed to localstack-ext-tests February 24, 2023 12:13 — with GitHub Actions Inactive

bentsku temporarily deployed to localstack-ext-tests February 24, 2023 15:01 — with GitHub Actions Inactive

bentsku mentioned this pull request Feb 24, 2023

improve apigw parity work for Resource proxy path #7746

Merged

bentsku temporarily deployed to localstack-ext-tests February 24, 2023 16:01 — with GitHub Actions Inactive

bentsku changed the title ~~wip: work on APIGW Resource, improve parity~~ work on APIGW Resource, improve parity Feb 24, 2023

bentsku marked this pull request as ready for review February 24, 2023 18:31

bentsku requested review from calvernaz, dominikschubert and steffyP as code owners February 24, 2023 18:31

bentsku added the aws:apigateway Amazon API Gateway label Feb 24, 2023

bentsku self-assigned this Feb 24, 2023

bentsku changed the title ~~work on APIGW Resource, improve parity~~ improve parity for APIGW Resource Feb 24, 2023

bentsku temporarily deployed to localstack-ext-tests February 25, 2023 09:59 — with GitHub Actions Inactive

calvernaz approved these changes Feb 26, 2023

View reviewed changes

bentsku added 7 commits February 27, 2023 15:53

remove patch, create AWS validated tests, xfail behaviour update

27d34fb

fix recursive deleting of resource, add validation

30e017f

update children if modifying parentId

178b4e6

delete the resource from its parent children

7c27c5f

refactor for better validation

097d013

guard against state restore

d8944c1

fix comments

36b13dc

bentsku force-pushed the apigw-parity-work-2 branch from 835ca2a to 36b13dc Compare February 27, 2023 13:53

bentsku temporarily deployed to localstack-ext-tests February 27, 2023 13:53 — with GitHub Actions Inactive

bentsku merged commit 9c85f6d into master Feb 28, 2023

bentsku deleted the apigw-parity-work-2 branch February 28, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

improve parity for APIGW Resource #7738

improve parity for APIGW Resource #7738

Uh oh!

bentsku commented Feb 23, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Feb 23, 2023 •

edited

Loading

Uh oh!

coveralls commented Feb 23, 2023 •

edited

Loading

Uh oh!

calvernaz left a comment

Uh oh!

calvernaz Feb 26, 2023

Uh oh!

bentsku Feb 27, 2023

Uh oh!

calvernaz Feb 26, 2023

Uh oh!

bentsku Feb 27, 2023

Uh oh!

calvernaz Feb 26, 2023

Uh oh!

bentsku Feb 27, 2023

Uh oh!

thrau Mar 1, 2023

Uh oh!

calvernaz Feb 26, 2023

Uh oh!

bentsku Feb 27, 2023

Uh oh!

Uh oh!

		@@ -844,6 +972,17 @@ def filter_function(item):
		# ---------------


		def get_moto_rest_api(context: RequestContext, rest_api_id: str) -> MotoRestAPI:

Uh oh!

improve parity for APIGW Resource #7738

improve parity for APIGW Resource #7738

Uh oh!

Conversation

bentsku commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack integration with Pro

Uh oh!

coveralls commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvernaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bentsku commented Feb 23, 2023 •

edited

Loading

github-actions bot commented Feb 23, 2023 •

edited

Loading

coveralls commented Feb 23, 2023 •

edited

Loading