ec2: Enhance security group handling in Spot Fleet launch specifications by nik-localstack · Pull Request #159 · localstack/moto

nik-localstack · 2026-04-09T16:10:03Z

Motivation

When CreateFleet uses a LaunchTemplateSpecification, security groups were never applied to launched instances because group_set was read exclusively from the non-standard GroupSet field, ignoring the actual SecurityGroupIds and NetworkInterfaces[N].Groups fields that AWS clients use.

Solution

The fix falls back through all three sources (GroupSet → SecurityGroupIds → NetworkInterfaces[N].Groups) when building the SpotFleetLaunchSpec, and two AWS-verified regression tests covering both code paths are added in TestFleetSecurityGroups.

viren-nadkarni · 2026-04-16T12:03:56Z

+                                for g in (nic.get("Groups") or [])
+                            ]
+                            or []
+                        ),


Does AWS follow this exact precedence order?

AWS disallows specifying all these parameters on the creation.

viren-nadkarni · 2026-04-16T12:05:18Z

+    @staticmethod
+    def _get_ami_id(ec2_client):
+        """Mock AMI in moto; real Amazon Linux AMI via SSM on AWS."""
+        from tests import allow_aws_request
+
+        if not allow_aws_request():
+            return EXAMPLE_AMI_ID
+        ssm = boto3.client("ssm", region_name=ec2_client.meta.region_name)
+        kernel_61 = "/aws/service/ami-amazon-linux-latest/al2023-ami-kernel-6.1-x86_64"
+        return ssm.get_parameter(Name=kernel_61)["Parameter"]["Value"]
+
+    @staticmethod
+    def _create_vpc_subnet_sg(ec2_client, cleanups, sg_description="fleet sg test"):
+        """Create VPC / subnet / SG, registering cleanups in creation order.
+
+        Reversed teardown (SG → subnet → VPC) respects AWS dependency rules.
+
+        Returns:
+            (vpc_id, subnet_id, sg_id)
+        """
+        vpc_id = ec2_client.create_vpc(CidrBlock="10.0.0.0/16")["Vpc"]["VpcId"]
+        cleanups.append(lambda: ec2_client.delete_vpc(VpcId=vpc_id))
+
+        subnet_id = ec2_client.create_subnet(
+            VpcId=vpc_id,
+            CidrBlock="10.0.0.0/24",
+            AvailabilityZone=ec2_client.meta.region_name + "a",
+        )["Subnet"]["SubnetId"]
+        cleanups.append(lambda: ec2_client.delete_subnet(SubnetId=subnet_id))
+
+        sg_id = ec2_client.create_security_group(
+            GroupName="test-fleet-" + str(uuid4())[:6],
+            Description=sg_description,
+            VpcId=vpc_id,
+        )["GroupId"]
+        cleanups.append(lambda: ec2_client.delete_security_group(GroupId=sg_id))
+
+        return vpc_id, subnet_id, sg_id
+
+    @staticmethod
+    def _cleanup_fleet(ec2_client, fleet_id, instance_ids, subnet_id):
+        """Delete the fleet, wait for instance termination, then wait for ENIs
+        to be fully released from the subnet before delete_subnet / delete_vpc run.
+        """
+        ec2_client.delete_fleets(FleetIds=[fleet_id], TerminateInstances=True)
+        if instance_ids:
+            ec2_client.get_waiter("instance_terminated").wait(InstanceIds=instance_ids)
+        for _ in range(20):
+            enis = ec2_client.describe_network_interfaces(
+                Filters=[{"Name": "subnet-id", "Values": [subnet_id]}]
+            )["NetworkInterfaces"]
+            if not enis:
+                break
+            sleep(3)


These static methods feel like they should be fixtures.

viren-nadkarni · 2026-04-16T12:06:45Z

+        for _ in range(20):
+            enis = ec2_client.describe_network_interfaces(
+                Filters=[{"Name": "subnet-id", "Values": [subnet_id]}]
+            )["NetworkInterfaces"]
+            if not enis:
+                break
+            sleep(3)


Please implement the retry fixture as in LocalStack if possible because this pattern is starting to be quite common.

I agree that we need to implement proper fixtures and improve testing for EC2 in general, but doing it here seems like a wasted effort to me. I think we should discuss about how we test EC2 and maybe move all the tests in localstack repository but this should be a follow-up. Consider that this PR is supposed to be a small bugfix to unblock another initiative.

viren-nadkarni · 2026-04-16T12:10:00Z

+            "Instance has no security groups — SecurityGroupIds from launch template was not applied"
+        )
+        assert sg_id in instance_sg_ids, (
+            f"Expected security group {sg_id!r} to be attached to the instance, "


Why !r? Why not just {sg_id}?

Not specific reason, we can remove the !r

viren-nadkarni · 2026-04-16T12:12:07Z

+            lambda: self._cleanup_fleet(ec2_client, fleet_id, instance_ids, subnet_id)
+        )
+
+        fleet_errors = fleet_res.get("Errors", [])


As per AWS docs, CreateFleet response has no Errors field, but there is an ErrorSet

huh, boto documents these as Errors 🤦
https://docs.aws.amazon.com/boto3/latest/reference/services/ec2/client/create_fleet.html

viren-nadkarni · 2026-04-16T12:13:50Z

+        return ssm.get_parameter(Name=kernel_61)["Parameter"]["Value"]
+
+    @staticmethod
+    def _create_vpc_subnet_sg(ec2_client, cleanups, sg_description="fleet sg test"):


ec2_aws_verified decorator is already capable of creating this, what's the reason it's not used?

Good question, the reason is that it doesn't work well with cleanups. The cleanup runs after the ec2_aws_verified finally blocks, which ends up with VPCs / subnets not being removed.

nik-localstack

Thanks for the feedback @viren-nadkarni
I agree with most of your comments and I think we need to discuss about ec2 testing in general. Would you be willing to have a call tomorrow or early next week ?
In the meantime, can you specify if these comments are blockers for the current PR ?

nik-localstack · 2026-04-16T12:56:46Z

+        return ssm.get_parameter(Name=kernel_61)["Parameter"]["Value"]
+
+    @staticmethod
+    def _create_vpc_subnet_sg(ec2_client, cleanups, sg_description="fleet sg test"):


Good question, the reason is that it doesn't work well with cleanups. The cleanup runs after the ec2_aws_verified finally blocks, which ends up with VPCs / subnets not being removed.

nik-localstack · 2026-04-16T13:02:13Z

+        for _ in range(20):
+            enis = ec2_client.describe_network_interfaces(
+                Filters=[{"Name": "subnet-id", "Values": [subnet_id]}]
+            )["NetworkInterfaces"]
+            if not enis:
+                break
+            sleep(3)


I agree that we need to implement proper fixtures and improve testing for EC2 in general, but doing it here seems like a wasted effort to me. I think we should discuss about how we test EC2 and maybe move all the tests in localstack repository but this should be a follow-up. Consider that this PR is supposed to be a small bugfix to unblock another initiative.

nik-localstack · 2026-04-16T13:03:56Z

+                                for g in (nic.get("Groups") or [])
+                            ]
+                            or []
+                        ),


AWS disallows specifying all these parameters on the creation.

nik-localstack · 2026-04-16T13:04:57Z

+            lambda: self._cleanup_fleet(ec2_client, fleet_id, instance_ids, subnet_id)
+        )
+
+        fleet_errors = fleet_res.get("Errors", [])


huh, boto documents these as Errors 🤦
https://docs.aws.amazon.com/boto3/latest/reference/services/ec2/client/create_fleet.html

nik-localstack · 2026-04-16T13:09:34Z

+            "Instance has no security groups — SecurityGroupIds from launch template was not applied"
+        )
+        assert sg_id in instance_sg_ids, (
+            f"Expected security group {sg_id!r} to be attached to the instance, "


Not specific reason, we can remove the !r

ec2: Enhance security group handling in Spot Fleet launch specifications

2c5f9ab

nik-localstack self-assigned this Apr 9, 2026

nik-localstack marked this pull request as ready for review April 9, 2026 16:34

nik-localstack requested a review from viren-nadkarni as a code owner April 9, 2026 16:34

viren-nadkarni reviewed Apr 16, 2026

View reviewed changes

nik-localstack commented Apr 16, 2026

View reviewed changes

Conversation

nik-localstack commented Apr 9, 2026

Motivation

Solution

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!

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!

nik-localstack left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants