-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Conversation
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 29m 23s ⏱️ - 20m 47s 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.
This pull request removes 112 skipped tests and adds 1 skipped test. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this 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 👏👏
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, will update 👍
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
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"] |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 😅
83da8ee
to
873eebc
Compare
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:
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
SubnetIds
andSecurityGroups
SubnetIds[0]
of the listconnect_to
call in order to surface the proper exception to the client