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

Add lambda model and api updates #7762

Merged
merged 22 commits into from Mar 1, 2023
Merged

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Feb 27, 2023

Contributions

  • Add support for runtime management controls at API level
  • Add support for SnapStart at API level
  • Add snapshot tests for SnapStart API
  • Update all lambda-related snapshot tests due to model updates (only for aws_validated tests)
  • Add platform flag to docker clients for build_image
  • Fix Lambda tests on ARM hosts. This enables running Lambda tests on ARM hosts.
  • Conditionally skip the concurrency tests if the AWS account has too low limits (improve error messages).

Limitations

  • Runtime management controls use a hard-coded sha256 value (8eeff65f6809a3ce81507fe733fe09b835899b99481ba22fd75b5a7338290ec1) instead of inspecting the Docker image.
  • Rust builds on ARM (tested on M1 MacBook Pro) are slow (~5 minutes) due to emulation. See Makefile for potential performance improvements.
  • The SnapStart tests are a little slow (~1-2min) against AWS

Breaking changes

  • This PR includes model changes in lambda and will break CloudPods for the new lambda provider

@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 27, 2023 19:32 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Feb 27, 2023

LocalStack integration with Pro

       3 files  ±0         3 suites  ±0   1h 30m 49s ⏱️ - 1m 6s
1 773 tests +3  1 394 ✔️ ±0  379 💤 +3  0 ±0 
2 499 runs  +9  1 770 ✔️ ±0  729 💤 +9  0 ±0 

Results for commit 25e9a0f. ± Comparison against base commit 8da70d7.

♻️ This comment has been updated with latest results.

@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 27, 2023 22:27 — with GitHub Actions Inactive
@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 28, 2023 09:16 — with GitHub Actions Inactive
@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 28, 2023 10:59 — with GitHub Actions Inactive
@@ -577,6 +587,14 @@ def wait_logs():
"$..StartingPosition",
"$..StateTransitionReason",
"$..Topics",
# resource index mismatch due to SnapStart
Copy link
Member Author

Choose a reason for hiding this comment

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

That is an example where a new feature changes the indexing of snapshots because it matches a resource transformer. It is not ideal that we need to disable these snapshot verifications for the old provider due to an unrelated change.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I'm generally not super happy with the state of the resource transformer. I've added this as a reference in our internal issue tracker for snapshots

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to track that one. I think this might become a nasty source of flaky tests with automated test re-validation.

"$..PolicyArn",
"$..Layers",
# mismatching resource index due to SnapStart
"$..Statement.Condition.ArnLike.'AWS:SourceArn'",
Copy link
Member Author

Choose a reason for hiding this comment

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

: break the JsonPath snapshot comparison and need to be enquoted using single quotes

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add some validation for this to avoid errors like this. Was this fairly obvious when debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

The error was pretty specific (see below) but the stack trace wasn't particularly helpful in identifying the issue because the hook is executed after the test.
It could be nice to give some context when raising (e.g., which snapshot match or which verify skip path).

>       raise JsonPathParserError('Parse error at %s:%s near token %s (%s)'
                                  % (t.lineno, t.col, t.value, t.type))
E       jsonpath_ng.exceptions.JsonPathParserError: Parse error at 1:34 near token : (:)

.venv/lib/python3.10/site-packages/jsonpath_ng/parser.py:81: JsonPathParserError

@joe4dev joe4dev force-pushed the add-lambda-model-and-api-updates branch from f3b3759 to b6b2a31 Compare February 28, 2023 15:46
@joe4dev joe4dev temporarily deployed to localstack-ext-tests February 28, 2023 15:46 — with GitHub Actions Inactive
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM! 🥳

Great that you uncovered a lot of hidden issues that we'll need to tackle at some point. (e.g. tests missing the validated marker, rust compilation, resource transformer)

localstack/services/awslambda/provider.py Show resolved Hide resolved

build:
mkdir -p build
docker run --rm -v $$(pwd)/src:/app -v $$(pwd)/build:/out golang:latest@sha256:b850621230956a6d960d6d7cfaba6a8a2e8e245b230a928ef66aa0cfd065e229 /bin/bash -c "cd /app && GOOS=linux CGO_ENABLED=0 go build -trimpath -ldflags=-buildid= -o /out/main main.go && chown $$(id -u):$$(id -g) /out/main"
docker run --rm --platform $(DOCKER_PLATFORM) -v $$(pwd)/src:/app -v $$(pwd)/build:/out $(DOCKER_GOLANG_IMAGE) /bin/bash -c "cd /app && GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -trimpath -ldflags=-buildid= -o /out/main main.go && chown $$(id -u):$$(id -g) /out/main"
find ./build -exec touch -t 200001010100.00 {} \;
Copy link
Member

Choose a reason for hiding this comment

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

We could remove these since we're not actually checking size & hash of the zip files anymore

Copy link
Member Author

Choose a reason for hiding this comment

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

right, I added a TODO for build refactoring in test_lambda_common.py because the many identical Makefiles could be parametrized per runtime.

I wasn't sure initially whether the touch is for hashing or some caching ...

Comment on lines 177 to 180
else:
pytestmark = pytest.mark.skip_snapshot_verify(
paths=["$..CodeSize", "$..SnapStart"], # FIXME
paths=["$..CodeSize"], # FIXME
)
Copy link
Member

Choose a reason for hiding this comment

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

we can actually just remove codesize as well and add it to the "global" transformers above

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch 👍
I just removed it because CodeSize is already part of the lambda_api() transformer.

Comment on lines +1317 to +1318
"update_function_configuration_response_rev5..RuntimeVersionConfig.RuntimeVersionArn",
"get_function_response_rev6..RuntimeVersionConfig.RuntimeVersionArn",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"update_function_configuration_response_rev5..RuntimeVersionConfig.RuntimeVersionArn",
"get_function_response_rev6..RuntimeVersionConfig.RuntimeVersionArn",
"$..RuntimeVersionConfig.RuntimeVersionArn",

or was there a particular reason for only including those?

Copy link
Member Author

Choose a reason for hiding this comment

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

I still wanted the validation that the RuntimeVersionArn does not change between revisions and not ignore it completely for the entire test

"$..PolicyArn",
"$..Layers",
# mismatching resource index due to SnapStart
"$..Statement.Condition.ArnLike.'AWS:SourceArn'",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add some validation for this to avoid errors like this. Was this fairly obvious when debugging?

@@ -577,6 +587,14 @@ def wait_logs():
"$..StartingPosition",
"$..StateTransitionReason",
"$..Topics",
# resource index mismatch due to SnapStart
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I'm generally not super happy with the state of the resource transformer. I've added this as a reference in our internal issue tracker for snapshots

@joe4dev joe4dev force-pushed the add-lambda-model-and-api-updates branch from b6b2a31 to 25e9a0f Compare March 1, 2023 09:52
@joe4dev joe4dev temporarily deployed to localstack-ext-tests March 1, 2023 09:52 — with GitHub Actions Inactive
@joe4dev joe4dev merged commit ee27d16 into master Mar 1, 2023
@joe4dev joe4dev deleted the add-lambda-model-and-api-updates branch March 1, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants