[Bug] Use subnet hash for ELB target group so that a new target group is created whenever we update the LoginNode's subnetID

Author: Himani Anil Deshpande

cli/src/pcluster/templates/login_nodes_stack.py

@@ -404,9 +404,10 @@ def _get_launching_lifecycle_hook_specification(self):
         )
 
     def _add_login_nodes_pool_target_group(self):
+        subnet_hash = create_hash_suffix(self._pool.networking.subnet_ids[0])[:8]
         return elbv2.NetworkTargetGroup(
             self,
-            f"{self._pool.name}TargetGroup",
+            f"{self._pool.name}TargetGroup{subnet_hash}",
             health_check=elbv2.HealthCheck(
                 port="22",
                 protocol=elbv2.Protocol.TCP,
@@ -415,11 +416,14 @@ def _add_login_nodes_pool_target_group(self):
             protocol=elbv2.Protocol.TCP,
             target_type=elbv2.TargetType.INSTANCE,
             vpc=self._vpc,
+            # AWS Target Group name limit is 32 characters.
+            # With 3 args: (7 chars * 3) + (2 hyphens) + (1 hyphen) + (8 char hash) = 32 chars
             target_group_name=_get_resource_combination_name(
                 self._config.cluster_name,
                 self._pool.name,
+                subnet_hash,
                 partial_length=7,
-                hash_length=16,
+                hash_length=8,
             ),
         )
 

cli/tests/pcluster/templates/test_cluster_stack.py

@@ -876,7 +876,7 @@ def test_login_nodes_traffic_management_resources_values_properties(
 
     auto_scaling_group_id = "clusternametestloginnodespool1clusternametestloginnodespool1AutoScalingGroup5EBA3937"
     load_balancer_id = "clusternametestloginnodespool1testloginnodespool1LoadBalancerE1D4FCC7"
-    target_group_id = "clusternametestloginnodespool1testloginnodespool1TargetGroup713F5EC5"
+    target_group_id = "clusternametestloginnodespool1testloginnodespool1TargetGroup4592cff378B1099C"
     listener_id = (
         "clusternametestloginnodespool1testloginnodespool1LoadBalancerLoginNodesListenertestloginnodespool165B4D3DC"
     )
@@ -1357,19 +1357,18 @@ def test_custom_munge_key_iam_policy(mocker, test_datadir, config_file_name):
 
 
 @pytest.mark.parametrize(
-    "resource_name_1, resource_name_2, partial_length, hash_length, expected_combination_name",
+    "resource_names, partial_length, hash_length, expected_combination_name",
     [
-        ("test-cluster", "test-pool", 7, 16, "test-cl-test-po-18c74b16dfbc78ac"),
-        ("abcdefghijk", "lmnopqrst", 8, 14, "abcdefgh-lmnopqrs-dd65eea0329dcb"),
-        ("a", "b", 7, 16, "a-b-fb8e20fc2e4c3f24"),
+        (["test-cluster", "test-pool"], 7, 16, "test-cl-test-po-18c74b16dfbc78ac"),
+        (["abcdefghijk", "lmnopqrst"], 8, 14, "abcdefgh-lmnopqrs-dd65eea0329dcb"),
+        (["a", "b"], 7, 16, "a-b-fb8e20fc2e4c3f24"),
+        # Test with 3 resource names (like target group: cluster_name, pool_name, subnet_hash)
+        (["clustername", "loginpool", "092512032a1df0b2c"], 7, 8, "cluster-loginpo-0925120-fde21041"),
     ],
 )
-def test_resource_combination_name(
-    resource_name_1, resource_name_2, partial_length, hash_length, expected_combination_name
-):
+def test_resource_combination_name(resource_names, partial_length, hash_length, expected_combination_name):
     combination_name = _get_resource_combination_name(
-        resource_name_1,
-        resource_name_2,
+        *resource_names,
         partial_length=partial_length,
         hash_length=hash_length,
     )

cli/tests/pcluster/templates/test_login_nodes_stack.py

@@ -78,3 +78,86 @@ def render_join(elem: dict):
         else:
             raise ValueError("Found unsupported item type while rendering Fn::Join")
     return sep.join(rendered_body)
+
+
+@pytest.mark.parametrize(
+    "config_file_name, keep_original_subnet",
+    [
+        ("config-1.yaml", True),
+        ("config-2.yaml", False),
+    ],
+)
+def test_target_group_with_config_files(mocker, test_datadir, config_file_name, keep_original_subnet):
+    """
+    Test target group logical ID and name change when subnet is modified.
+
+    This test verifies that:
+    1. Target group logical ID and name are generated correctly
+    2. When subnet changes, both logical ID and target group name change
+    3. Target group name stays within AWS 32 char limit
+
+    This ensures that cluster updates with subnet changes create new target groups,
+    avoiding "target group cannot be associated with more than one load balancer"
+    and "target group name already exists" errors.
+    """
+    mock_aws_api(mocker)
+    mock_bucket_object_utils(mocker)
+
+    def get_target_group_info(cdk_assets):
+        """Extract target group logical ID and name from CDK assets."""
+        for asset in cdk_assets:
+            asset_content = asset.get("content")
+            if asset_content and "Resources" in asset_content:
+                for resource_name, resource in asset_content["Resources"].items():
+                    if resource.get("Type") == "AWS::ElasticLoadBalancingV2::TargetGroup":
+                        return resource_name, resource["Properties"]["Name"]
+        return None, None
+
+    def build_template(config_yaml):
+        """Build CDK template from config yaml."""
+        cluster_config = ClusterSchema(cluster_name="clustername").load(config_yaml)
+        _, cdk_assets = CDKTemplateBuilder().build_cluster_template(
+            cluster_config=cluster_config, bucket=dummy_cluster_bucket(), stack_name="clustername"
+        )
+        return cdk_assets
+
+    def assert_target_group_info(logical_id, tg_name):
+        """Verify target group logical ID and name are not None and within AWS 32 char limit."""
+        assert_that(logical_id).is_not_none()
+        assert_that(tg_name).is_not_none()
+        assert_that(len(tg_name)).is_less_than_or_equal_to(32)
+
+    # Read yaml config
+    input_yaml = load_yaml_dict(test_datadir / config_file_name)
+    original_subnet = input_yaml["LoginNodes"]["Pools"][0]["Networking"]["SubnetIds"][0]
+
+    # Build template with original subnet
+    cdk_assets_original = build_template(input_yaml)
+    logical_id_original, tg_name_original = get_target_group_info(cdk_assets_original)
+
+    # Verify original target group was created correctly
+    assert_target_group_info(logical_id_original, tg_name_original)
+
+    # Modify subnet to a new value
+    new_subnet = "subnet-12345678901234567"
+    new_pool_count = 0
+    if keep_original_subnet:
+        new_subnet = original_subnet
+        new_pool_count = 1
+    input_yaml["LoginNodes"]["Pools"][0]["Networking"]["SubnetIds"][0] = new_subnet
+    input_yaml["LoginNodes"]["Pools"][0]["Count"] = new_pool_count
+
+    # Build template with new subnet
+    cdk_assets_new = build_template(input_yaml)
+    logical_id_new, tg_name_new = get_target_group_info(cdk_assets_new)
+
+    if keep_original_subnet:
+        assert_that(logical_id_new).is_equal_to(logical_id_original)
+        assert_that(tg_name_new).is_equal_to(tg_name_original)
+    else:
+        # Verify that both logical ID and target group name changed when subnet changed
+        assert_that(logical_id_new).is_not_equal_to(logical_id_original)
+        assert_that(tg_name_new).is_not_equal_to(tg_name_original)
+
+    # Verify new target group was created correctly
+    assert_target_group_info(logical_id_new, tg_name_new)

cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-1.yaml

@@ -0,0 +1,35 @@
+DeploymentSettings:
+  DisableSudoAccessForDefaultUser: True
+  DefaultUserHome: Local
+Region: us-east-1
+Image:
+  Os: alinux2
+LoginNodes:
+  Pools:
+    - Name: login
+      InstanceType: t3.micro
+      Count: 1
+      Networking:
+        SubnetIds:
+          - subnet-12345678
+      Dcv:
+        Enabled: true
+        Port: 8444
+HeadNode:
+  InstanceType: t3.micro
+  Networking:
+    SubnetId: subnet-12345678
+  Ssh:
+    KeyName: ec2-key-name
+Scheduling:
+  Scheduler: slurm
+  SlurmQueues:
+  - Name: queue1
+    ComputeResources:
+    - Name: cr1
+      InstanceType: c4.xlarge
+      MinCount: 0
+      MaxCount: 10
+    Networking:
+      SubnetIds:
+      - subnet-12345678

cli/tests/pcluster/templates/test_login_nodes_stack/test_target_group_with_config_files/config-2.yaml

@@ -0,0 +1,38 @@
+Region: us-east-1
+Image:
+  Os: alinux2
+LoginNodes:
+  Pools:
+    - Name: login
+      InstanceType: t3.micro
+      Count: 1
+      Networking:
+        SubnetIds:
+          - subnet-12345678
+      Dcv:
+        Enabled: false
+HeadNode:
+  InstanceType: t3.micro
+  Networking:
+    SubnetId: subnet-12345678
+  Ssh:
+    KeyName: ec2-key-name
+Scheduling:
+  Scheduler: slurm
+  SlurmQueues:
+  - Name: queue1
+    ComputeResources:
+    - Name: cr1
+      InstanceType: c4.xlarge
+      MinCount: 0
+      MaxCount: 10
+    Networking:
+      SubnetIds:
+      - subnet-12345678
+DirectoryService:
+  DomainName: corp.pcluster.com
+  DomainAddr: ldaps://corp.pcluster.com
+  PasswordSecretArn: arn:aws:secretsmanager:eu-west-1:XXXXXXXXXXXX:secret:XXXXXXXXXX
+  DomainReadOnlyUser: cn=ReadOnlyUser,ou=Users,ou=CORP,dc=corp,dc=pcluster,dc=com
+  LdapTlsReqCert: never
+  GenerateSshKeysForUsers: true