8000 Lamba: fix unhandled error when SubnetIds is invalid by bentsku · Pull Request #12293 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

Lamba: fix unhandled error when SubnetIds is invalid #12293

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 21, 2025
Merged

Lamba: fix unhandled error when SubnetIds is invalid #12293

merged 1 commit into from
Feb 21, 2025

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Feb 20, 2025 •
edited
Loading

Motivation

While working with Lambda and during a support case, I realized we didn't handle the case when the SubnetIds would be invalid.

While this PR does not implement complet behavior (we only validate the first subnet id in the list), it at least makes sure that we don't return InternalException.

This was the error:

  File "<>/localstack/localstack-core/localstack/services/lambda_/provider.py", line 1007, in create_function
    vpc_config=self._build_vpc_config(
               ^^^^^^^^^^^^^^^^^^^^^^^
  File "<>/localstack/localstack-core/localstack/services/lambda_/provider.py", line 491, in _build_vpc_config
    vpc_id=self._resolve_vpc_id(account_id, region_name, subnet_ids[0]),
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<>/localstack/localstack-core/localstack/services/lambda_/provider.py", line 473, in _resolve_vpc_id
    ).ec2.describe_subnets(SubnetIds=[subnet_id])["Subnets"][0]["VpcId"]
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<>/.venv/lib/python3.11/site-packages/botocore/client.py", line 569, in _api_call
    return self._make_api_call(operation_name, kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<>/.venv/lib/python3.11/site-packages/botocore/client.py", line 1023, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.exceptions.ClientError: An error occurred (InvalidSubnetID.NotFound) when calling the DescribeSubnets operation (reached max retries: 0): The subnet ID 'bad-format-1f2656ac' does not exist
, headers={'Content-Type': 'application/json', 'X-Amzn-Errortype': 'InternalError', 'Content-Length': '3057', 'x-amzn-requestid': 'aa043e2f-52d3-4289-8646-ad4ee10ba812', 'x-amz-request-id': 'aa043e2f-52d3-4289-8646-ad4ee10ba812'})

And we would return an InternalError to the client.

This PR also implements a skipped test to validate the SecurityGroups. I did not implement the logic to actually validate as it is not currently checked, and am not sure if it could lead to issues with users.

Changes

  • add 2 tests for validating SubnetIds and SecurityGroups
  • implement regex validation for the SubnetIds[0] of the list
  • add a try...except block around the connect_to call in order to surface the proper exception to the client

@bentsku bentsku added aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases labels Feb 20, 2025
@bentsku bentsku added this to the 4.2 milestone Feb 20, 2025
@bentsku bentsku self-assigned this Feb 20, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Feb 20, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 29m 23s ⏱️ - 20m 47s
3 108 tests  - 991  2 887 ✅  - 880  221 💤  - 111  0 ❌ ±0 
3 110 runs   - 991  2 887 ✅  - 880  223 💤  - 111  0 ❌ ±0 

Results for commit 873eebc. ± Comparison against base commit f269fe2.

This pull request removes 993 and adds 2 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.TestLambdaFunction ‑ test_invalid_vpc_config_security_group
tests.aws.services.lambda_.test_lambda_api.TestLambdaFunction ‑ test_invalid_vpc_config_subnet
This pull request removes 112 skipped tests and adds 1 skipped test. Note that renamed tests count towards both. 
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…

tests.aws.services.lambda_.test_lambda_api.TestLambdaFunction ‑ test_invalid_vpc_config_security_group

♻️ This comment has been updated with latest results.

joe4dev
joe4dev approved these changes Feb 21, 2025
View reviewed changes
Copy link
Member
@joe4dev joe4dev left a comment

Choose a reason for hiding this comment

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

Thank you for the fix @bentsku 👏👏

tests/aws/services/lambda_/test_lambda_api.py
@@ -1264,6 +1264,112 @@ def test_vpc_config(
"delete_vpcconfig_get_function_response", delete_vpcconfig_get_function_response
)

@markers.aws.validated
def test_invalid_vpc_config_subnet(
self, create_lambda_function, lambda_su_role, snapshot, aws_client, cleanups
Copy link
Member

Choose a reason for hiding this comment

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

nit: create_lambda_function fixture unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, will update 👍

Copy link
Contributor Author
@bentsku bentsku Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh no I'm so sorry, I thought I had pushed the changes but I forgot! 😭and I merged thinking the pipeline was already green... there was create_lambda_function and cleanups being unused...

Copy link
Member

Choose a reason for hiding this comment

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

No worries at all. It was just a tiny nit ☺️

tests/aws/services/lambda_/test_lambda_api.py
wrong_format_subnet_id = f"bad-format-{short_uid()}"

# AWS validates the Security Group first, so we need a valid one to test SubnetsIds
security_groups = aws_client.ec2.describe_security_groups(MaxResults=5)["SecurityGroups"]
Copy link
Member

Choose a reason for hiding this comment

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

I guess this just assumes that there is at least one valid security group (probably a sensible assumption)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think I've seen other tests doing this assumption with the default VPC, so figured I could as it'd be easier than to create the full VPC. Hopefully this is "safe" to do 😅

@bentsku bentsku force-pushed the fix-lambda-subnet-validation branch from 83da8ee to 873eebc Compare February 21, 2025 12:04
@bentsku bentsku merged commit 230914f into master Feb 21, 2025
31 checks passed
@bentsku bentsku deleted the fix-lambda-subnet-validation branch February 21, 2025 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@dfangl dfangl Awaiting requested review from dfangl dfangl is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

Assignees

@bentsku bentsku

Labels
aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @joe4dev
0