add: TaggingService functionality to Lambda Provider #11745

gregfurman · 2024-10-24T17:58:32Z

Motivation

This PR adds tagging functionality to the Lambda provider that leverages the TaggingService. In addition, we add tagging functionality for event source mapping.
Fixes bug: Terraform deployment for Event Source Mapping failing with ResourceNotFoundException #11739

Changes

Extended TestLambdaTag suite to include tagging tests for Event Source Mappings
Refactored/reworked the internal tag_resource, list_tags, and untag_resource Lambda provider functions to use a provider-wde TaggingService. This allows for tagging of Lambda Event Source Mapping or Lambda Function resources (with support for CSC but this is not yet implemented).

Testing

Manually verified event source mapping creation with Terraform, using the terraform-aws-lambda/event-source-mapping example.

github-actions · 2024-10-24T18:47:17Z

LocalStack Community integration with Pro

2 files ± 0 2 suites ±0 1h 20m 58s ⏱️ - 20m 42s
2 585 tests - 925 2 269 ✅ - 828 316 💤 - 97 0 ❌ ±0
2 587 runs - 925 2 269 ✅ - 828 318 💤 - 97 0 ❌ ±0

Results for commit 4c00bab. ± Comparison against base commit 0338932.

This pull request removes 930 and adds 5 tests. Note that renamed tests count towards both.

tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

tests.aws.services.lambda_.test_lambda_api.TestLambdaTag ‑ test_create_tag_on_esm_create
tests.aws.services.lambda_.test_lambda_api.TestLambdaTag ‑ test_tag_exceptions[event_source_mapping]
tests.aws.services.lambda_.test_lambda_api.TestLambdaTag ‑ test_tag_exceptions[lambda_function]
tests.aws.services.lambda_.test_lambda_api.TestLambdaTag ‑ test_tag_lifecycle[event_source_mapping]
tests.aws.services.lambda_.test_lambda_api.TestLambdaTag ‑ test_tag_lifecycle[lambda_function]

♻️ This comment has been updated with latest results.

…field

localstack-core/localstack/services/lambda_/invocation/models.py

gregfurman · 2024-10-25T13:43:01Z

localstack-core/localstack/services/lambda_/provider.py

-            stored_tags = function.tags or {}
-            stored_tags |= tags
-            self._store_tags(function=function, tags=stored_tags)
+        tag_svc_adapted_tags = [{"Key": key, "Value": value} for key, value in tags.items()]


We need to adapt the tags TaggingService <-> LambdaProvider when doing storage and retrieval of tags.

It seems Lambda is doing something custom here (tag adaptation). Does it still make sense to use the TaggingService?

I like the unified approach to tagging. Even though adapting in this way is a bit janky, I think the benefits of a single svc outweighs the costs of adapting to suit it.

In future, maybe we can extend the tag svc to better account for cases like this.

gregfurman · 2024-10-25T13:43:32Z

localstack-core/localstack/services/lambda_/provider.py

+            case "event-source-mapping":
+                self._get_esm(resource_identifier, account_id, region)
+            case "code-signing-config":
+                raise NotImplementedError("Resource tagging on CSC not yet implemented.")


We haven't implemented tagging on Code SIgning Configuration as of yet, but we can easily extend the above to include it.

gregfurman · 2024-10-25T13:44:43Z

localstack-core/localstack/services/lambda_/provider.py

-        fn = self._get_function(function_name=fn_name, account_id=account_id, region=region)
-
-        self._update_tags(fn, tags)
+        if (resource_id := extract_resource_from_arn(resource)) and resource_id.startswith(


Unfortunately we need to run these dirty checks to ensure a Revision ID gets updated

yeah, I see that we haven't found a good solution yet for dealing with the revision id updates in general 🤔

A small concern here is that the function and function version update are not synchronized with the tagging service, leading to potential race conditions (tag update and revision id updates not synchronized; quite unlikely, and consequences should be negligible). Would function locking via with function.lock around the _store_tags prevent the race condition?

Think we should rather look at improving this entire approach in a follow-up

+1, good idea

joe4dev

Great tagging test additions 💯

As discussed, it might be worth unifying the tags to avoid confusion and improve maintainability 🚀

localstack-core/localstack/services/lambda_/api_utils.py

joe4dev · 2024-10-28T10:35:41Z

localstack-core/localstack/services/lambda_/provider.py

-        fn = self._get_function(function_name=fn_name, account_id=account_id, region=region)
-
-        self._update_tags(fn, tags)
+        if (resource_id := extract_resource_from_arn(resource)) and resource_id.startswith(


yeah, I see that we haven't found a good solution yet for dealing with the revision id updates in general 🤔

A small concern here is that the function and function version update are not synchronized with the tagging service, leading to potential race conditions (tag update and revision id updates not synchronized; quite unlikely, and consequences should be negligible). Would function locking via with function.lock around the _store_tags prevent the race condition?

tests/aws/services/lambda_/test_lambda_api.py

tests/aws/services/lambda_/test_lambda_api.snapshot.json

localstack-core/localstack/services/lambda_/provider.py

joe4dev · 2024-10-28T10:56:44Z

localstack-core/localstack/services/lambda_/provider.py

-            stored_tags = function.tags or {}
-            stored_tags |= tags
-            self._store_tags(function=function, tags=stored_tags)
+        tag_svc_adapted_tags = [{"Key": key, "Value": value} for key, value in tags.items()]


It seems Lambda is doing something custom here (tag adaptation). Does it still make sense to use the TaggingService?

localstack-core/localstack/services/lambda_/invocation/models.py

joe4dev · 2024-10-28T12:25:21Z

localstack-core/localstack/services/lambda_/provider.py

+            function = self._get_function(name, account, region)
+            with function.lock:
+                function.tags = tags
+                # dirty hack for changed revision id, should reevaluate model to prevent this:


style: yeah, this starts spreading like cancer 😅
maybe, we could consider extracting a helper method for this hack

joe4dev

Great work @gregfurman 👏

Thank you for the comprehensive test coverage and unifying tag storage ✨

sidenote: the PR description mentions "Fixes issue#x", which auto-closes the issue. We typically ask the customer for feedback using the response-required tag and stale bot to foster feedback instead of directly closing upon merge. It also avoids notifying the customer too early before an ext image is shipped

localstack-core/localstack/services/lambda_/invocation/lambda_models.py

tests/aws/services/lambda_/test_lambda_api.py

gregfurman added aws:lambda AWS Lambda semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Oct 24, 2024

gregfurman added this to the Playground milestone Oct 24, 2024

gregfurman self-assigned this Oct 24, 2024

gregfurman added 2 commits October 25, 2024 11:25

add: TaggingService functionality to Lambda

0f0ff76

Tag after resource creation and add TODOs for removing Function.tags …

d598037

…field

gregfurman force-pushed the add/lambda/tag-svc branch from e7e4c11 to d598037 Compare October 25, 2024 12:50

gregfurman marked this pull request as ready for review October 25, 2024 13:39

gregfurman requested review from joe4dev, dominikschubert and dfangl as code owners October 25, 2024 13:39

gregfurman commented Oct 25, 2024

View reviewed changes

localstack-core/localstack/services/lambda_/invocation/models.py Show resolved Hide resolved

gregfurman commented Oct 25, 2024

View reviewed changes

joe4dev reviewed Oct 28, 2024

View reviewed changes

Address comments

4c00bab

gregfurman force-pushed the add/lambda/tag-svc branch from 448d396 to 4c00bab Compare October 28, 2024 14:29

joe4dev approved these changes Oct 29, 2024

View reviewed changes

localstack-core/localstack/services/lambda_/invocation/lambda_models.py Show resolved Hide resolved

tests/aws/services/lambda_/test_lambda_api.py Show resolved Hide resolved

gregfurman merged commit f67ad4d into master Oct 29, 2024
33 checks passed

gregfurman deleted the add/lambda/tag-svc branch October 29, 2024 08:43

gregfurman modified the milestones: Playground, 4.0 Nov 7, 2024

Uh oh!

add: TaggingService functionality to Lambda Provider #11745

add: TaggingService functionality to Lambda Provider #11745

Uh oh!

Conversation

gregfurman commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Testing

Uh oh!

github-actions bot commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LocalStack Community integration with Pro

Uh oh!

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!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joe4dev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregfurman commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading