From b3b5da072940229272997fac93033b2e40ed52a3 Mon Sep 17 00:00:00 2001 From: "almightychang (Sam, Joochul Chang)" Date: Tue, 23 Dec 2025 15:34:43 +0900 Subject: [PATCH 1/3] [Bug] Fix is_subnet_public to use main route table for subnets without explicit association When a subnet has no explicit route table association, it uses the VPC's main route table. The previous implementation incorrectly used route_tables[0] which may not be the main route table, causing public subnets to be incorrectly identified as private. This fix explicitly finds the main route table by checking the "Main" flag in route table associations. If no main route table is found (which should not happen in a valid VPC), an exception is raised with a helpful message. Fixes #7173 --- cli/src/pcluster/aws/ec2.py | 12 +++++ cli/tests/pcluster/aws/test_ec2.py | 82 ++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/cli/src/pcluster/aws/ec2.py b/cli/src/pcluster/aws/ec2.py index 57f576ee8f..30783e645a 100644 --- a/cli/src/pcluster/aws/ec2.py +++ b/cli/src/pcluster/aws/ec2.py @@ -572,6 +572,18 @@ def is_subnet_public(self, subnet_id): if not route_tables: raise Exception("No route tables found. The subnet or VPC configuration may be incorrect.") + # Find the main route table (subnets without explicit association use the main route table) + main_route_table = next( + (rt for rt in route_tables if any(assoc.get("Main") for assoc in rt.get("Associations", []))), + None, + ) + if main_route_table is None: + raise Exception( + f"No main route table found for VPC {vpc_id}. " + "Please explicitly associate the subnet with a route table." + ) + route_tables = [main_route_table] + # Check if any route contains an internet gateway (igw) for route in route_tables[0].get("Routes", []): if "GatewayId" in route and route["GatewayId"].startswith("igw-"): diff --git a/cli/tests/pcluster/aws/test_ec2.py b/cli/tests/pcluster/aws/test_ec2.py index 719e6d4e37..9dcc86385f 100644 --- a/cli/tests/pcluster/aws/test_ec2.py +++ b/cli/tests/pcluster/aws/test_ec2.py @@ -679,6 +679,30 @@ def get_describe_route_tables_mocked_request(subnet_id, gateway_id): ) +def get_describe_route_tables_empty_mocked_request(subnet_id): + return MockedBoto3Request( + method="describe_route_tables", + response={"RouteTables": []}, + expected_params={"Filters": [{"Name": "association.subnet-id", "Values": [subnet_id]}]}, + ) + + +def get_describe_route_tables_by_vpc_mocked_request(vpc_id, route_tables): + return MockedBoto3Request( + method="describe_route_tables", + response={"RouteTables": route_tables}, + expected_params={"Filters": [{"Name": "vpc-id", "Values": [vpc_id]}]}, + ) + + +def get_describe_subnets_for_vpc_mocked_request(subnet_id, vpc_id): + return MockedBoto3Request( + method="describe_subnets", + response={"Subnets": [{"SubnetId": subnet_id, "VpcId": vpc_id}]}, + expected_params={"SubnetIds": [subnet_id]}, + ) + + def test_is_subnet_public(boto3_stubber): # First boto3 call. The subnet should be private subnet_id = "subnet-12345678" @@ -698,3 +722,61 @@ def test_is_subnet_public(boto3_stubber): # Third boto3 call. The result should be from the latest response even if the gateway id of the subnet is different assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is True + + +def test_is_subnet_public_with_main_route_table(boto3_stubber): + # Test when subnet has no explicit route table association (uses main route table) + # This tests the bug fix: should use main route table, not route_tables[0] + subnet_id = "subnet-no-explicit-assoc" + vpc_id = "vpc-12345678" + + route_tables = [ + # First route table (non-main, no IGW) - bug would incorrectly use this + { + "RouteTableId": "rtb-private", + "Associations": [{"Main": False, "SubnetId": "subnet-other"}], + "Routes": [{"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}], + }, + # Main route table with IGW - correct one to use + { + "RouteTableId": "rtb-main", + "Associations": [{"Main": True}], + "Routes": [ + {"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}, + {"DestinationCidrBlock": "0.0.0.0/0", "GatewayId": "igw-12345678"}, + ], + }, + ] + + mocked_requests = [ + get_describe_route_tables_empty_mocked_request(subnet_id), + get_describe_subnets_for_vpc_mocked_request(subnet_id, vpc_id), + get_describe_route_tables_by_vpc_mocked_request(vpc_id, route_tables), + ] + boto3_stubber("ec2", mocked_requests) + + # Should return True because main route table has IGW + assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is True + + +def test_is_subnet_public_main_route_table_no_igw(boto3_stubber): + # Test when main route table has no IGW (private subnet) + subnet_id = "subnet-private" + vpc_id = "vpc-12345678" + + route_tables = [ + { + "RouteTableId": "rtb-main", + "Associations": [{"Main": True}], + "Routes": [{"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}], + }, + ] + + mocked_requests = [ + get_describe_route_tables_empty_mocked_request(subnet_id), + get_describe_subnets_for_vpc_mocked_request(subnet_id, vpc_id), + get_describe_route_tables_by_vpc_mocked_request(vpc_id, route_tables), + ] + boto3_stubber("ec2", mocked_requests) + + assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is False From 3cc4263859ff96fe213d52922833738c4ceab1ea Mon Sep 17 00:00:00 2001 From: almightychang Date: Sat, 27 Dec 2025 15:03:43 +0900 Subject: [PATCH 2/3] Use API filter association.main instead of Python filtering Refactor is_subnet_public to use AWS API filter 'association.main=true' to fetch only the main route table, instead of fetching all route tables and filtering in Python. This is more efficient and cleaner. Update test mocks to match the new API filter parameters. --- cli/src/pcluster/aws/ec2.py | 16 +++------------- cli/tests/pcluster/aws/test_ec2.py | 17 ++++++++--------- 2 files changed, 11 insertions(+), 22 deletions(-) diff --git a/cli/src/pcluster/aws/ec2.py b/cli/src/pcluster/aws/ec2.py index 30783e645a..58e25225a0 100644 --- a/cli/src/pcluster/aws/ec2.py +++ b/cli/src/pcluster/aws/ec2.py @@ -568,22 +568,12 @@ def is_subnet_public(self, subnet_id): raise Exception(f"No subnet found with ID {subnet_id}") vpc_id = subnets[0].get("VpcId") - route_tables = self.describe_route_tables(filters=[{"Name": "vpc-id", "Values": [vpc_id]}]) + route_tables = self.describe_route_tables( + filters=[{"Name": "vpc-id", "Values": [vpc_id]}, {"Name": "association.main", "Values": ["true"]}] + ) if not route_tables: raise Exception("No route tables found. The subnet or VPC configuration may be incorrect.") - # Find the main route table (subnets without explicit association use the main route table) - main_route_table = next( - (rt for rt in route_tables if any(assoc.get("Main") for assoc in rt.get("Associations", []))), - None, - ) - if main_route_table is None: - raise Exception( - f"No main route table found for VPC {vpc_id}. " - "Please explicitly associate the subnet with a route table." - ) - route_tables = [main_route_table] - # Check if any route contains an internet gateway (igw) for route in route_tables[0].get("Routes", []): if "GatewayId" in route and route["GatewayId"].startswith("igw-"): diff --git a/cli/tests/pcluster/aws/test_ec2.py b/cli/tests/pcluster/aws/test_ec2.py index 9dcc86385f..e72ae79966 100644 --- a/cli/tests/pcluster/aws/test_ec2.py +++ b/cli/tests/pcluster/aws/test_ec2.py @@ -691,7 +691,12 @@ def get_describe_route_tables_by_vpc_mocked_request(vpc_id, route_tables): return MockedBoto3Request( method="describe_route_tables", response={"RouteTables": route_tables}, - expected_params={"Filters": [{"Name": "vpc-id", "Values": [vpc_id]}]}, + expected_params={ + "Filters": [ + {"Name": "vpc-id", "Values": [vpc_id]}, + {"Name": "association.main", "Values": ["true"]}, + ] + }, ) @@ -726,18 +731,12 @@ def test_is_subnet_public(boto3_stubber): def test_is_subnet_public_with_main_route_table(boto3_stubber): # Test when subnet has no explicit route table association (uses main route table) - # This tests the bug fix: should use main route table, not route_tables[0] + # The API filter association.main=true returns only the main route table subnet_id = "subnet-no-explicit-assoc" vpc_id = "vpc-12345678" + # API returns only main route table (filtered by association.main=true) route_tables = [ - # First route table (non-main, no IGW) - bug would incorrectly use this - { - "RouteTableId": "rtb-private", - "Associations": [{"Main": False, "SubnetId": "subnet-other"}], - "Routes": [{"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}], - }, - # Main route table with IGW - correct one to use { "RouteTableId": "rtb-main", "Associations": [{"Main": True}], From 9445cc848d51fb4fe92e2a81093098f6ddda32ba Mon Sep 17 00:00:00 2001 From: almightychang Date: Mon, 5 Jan 2026 00:59:52 +0900 Subject: [PATCH 3/3] Refactor main route table tests to use pytest.mark.parametrize Combine test_is_subnet_public_with_main_route_table and test_is_subnet_public_main_route_table_no_igw into a single parameterized test for better maintainability. --- cli/tests/pcluster/aws/test_ec2.py | 52 ++++++++++++------------------ 1 file changed, 21 insertions(+), 31 deletions(-) diff --git a/cli/tests/pcluster/aws/test_ec2.py b/cli/tests/pcluster/aws/test_ec2.py index e72ae79966..a5751db2d5 100644 --- a/cli/tests/pcluster/aws/test_ec2.py +++ b/cli/tests/pcluster/aws/test_ec2.py @@ -729,45 +729,35 @@ def test_is_subnet_public(boto3_stubber): assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is True -def test_is_subnet_public_with_main_route_table(boto3_stubber): - # Test when subnet has no explicit route table association (uses main route table) - # The API filter association.main=true returns only the main route table - subnet_id = "subnet-no-explicit-assoc" - vpc_id = "vpc-12345678" - - # API returns only main route table (filtered by association.main=true) - route_tables = [ - { - "RouteTableId": "rtb-main", - "Associations": [{"Main": True}], - "Routes": [ +@pytest.mark.parametrize( + "subnet_id, routes, expected_result", + [ + pytest.param( + "subnet-no-explicit-assoc", + [ {"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}, {"DestinationCidrBlock": "0.0.0.0/0", "GatewayId": "igw-12345678"}, ], - }, - ] - - mocked_requests = [ - get_describe_route_tables_empty_mocked_request(subnet_id), - get_describe_subnets_for_vpc_mocked_request(subnet_id, vpc_id), - get_describe_route_tables_by_vpc_mocked_request(vpc_id, route_tables), - ] - boto3_stubber("ec2", mocked_requests) - - # Should return True because main route table has IGW - assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is True - - -def test_is_subnet_public_main_route_table_no_igw(boto3_stubber): - # Test when main route table has no IGW (private subnet) - subnet_id = "subnet-private" + True, + id="main route table with igw", + ), + pytest.param( + "subnet-private", + [{"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}], + False, + id="main route table without igw", + ), + ], +) +def test_is_subnet_public_with_main_route_table(boto3_stubber, subnet_id, routes, expected_result): + # Test when subnet has no explicit route table association (uses main route table) vpc_id = "vpc-12345678" route_tables = [ { "RouteTableId": "rtb-main", "Associations": [{"Main": True}], - "Routes": [{"DestinationCidrBlock": "10.0.0.0/16", "GatewayId": "local"}], + "Routes": routes, }, ] @@ -778,4 +768,4 @@ def test_is_subnet_public_main_route_table_no_igw(boto3_stubber): ] boto3_stubber("ec2", mocked_requests) - assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is False + assert AWSApi.instance().ec2.is_subnet_public(subnet_id) is expected_result