Lambda ENI cleanup added to security group delete

[gposton]

This adds support for cleaning up ENIs that get automatically created by Lambda functions when dealing with security groups used in Lambda functions

[radeksimko]

Is there any reason why not put this into the initial filtering on the API level? I think that would possibly make the code slightly cleaner + save some bytes between AWS/user and CPU cycles.

When I'm looking at the real ENI created by Lambda I think we can be even more strict in filtering to be really sure we won't delete any ENI by mistake. It would be a sad coincidence, but after all description is something users can specify (i.e. name it AWS Lambda VPC ENI). We want to avoid deleting ENIs that are not managed by the underlying Lambda API if possible.

How about something like this? 😺

aws ec2 describe-network-interfaces \
  --filter \
    'Name=group-id,Values=sg-aaaaaaaa' \
    'Name=description,Values=AWS Lambda VPC ENI: *' \
    'Name=requester-id,Values=*:awslambda_*'

[radeksimko]

^ Theoretically we could still delete ENI which was originally created via IAM role that was called awslambda_..., but that would happen less likely than if we did a simple SG & description match.

[radeksimko]

Hi @gposton
thank you for taking the time to prepare and submit this patch. I left you some comments there.
Let me know what you think.

[gposton]

Great feedback @radeksimko thanks... take another look?

[radeksimko]

Probably just rare edge case, but we may find ENIs that are not attached, so I think we should have a condition here

e.g.

if eni.Attachment != nil {
  // all the detach logic
}

deleteNetworkInterfaceParams := ...

Otherwise ENI that is already detached would cause terraform to crash here (reproducible easily by manually detaching the ENI via AWS Console before running terraform destroy).

[radeksimko]

Thanks for the modifications @gposton
this is looking good except that nitpick with detached ENIs.

It would be awesome to have an acceptance test for this - e.g. cfg to create VPC-enabled Lambda function, then lambda:Invoke, then destroy that cfg

but missing acceptance test is not a blocker for this particular PR.

[gposton]

@radeksimko, apologies for the slow response. Good catch... I made the recommended update to handle the scenario where the ENI is detached. I tested by manually detaching the ENI before destroying the stack and verifying that there were no errors and that the ENI was successfully deleted.

[radeksimko]

Great, thanks for the modification. This LGTM now.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.