From 8b6a1bb5388e2e36d6332b54353359f0486767c9 Mon Sep 17 00:00:00 2001 From: Tim Lane Date: Thu, 21 Oct 2021 12:06:58 -0700 Subject: [PATCH] Include tags in RunInstances dry run This change is motivated by users whose organizational security policies require certain tags to be present when they're attempting to launch instances. Without this change, the dry run performed as part of validation would fail, and the CLI would incorrectly tell the customer that there's an issue with their configuration file. Signed-off-by: Tim Lane --- CHANGELOG.md | 3 +++ cli/src/pcluster/config/cluster_config.py | 6 +++-- .../pcluster/validators/cluster_validators.py | 19 +++++++++++++--- .../validators/test_cluster_validators.py | 22 +++++++++++++++++++ 4 files changed, 45 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44387bccc9..ecc1996db9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ CHANGELOG 3.0.1 ------ +**CHANGES** +- Include tags from cluster configuration file in the RunInstances dry runs performed during configuration validation. + **ENHANCEMENTS** - Add `pcluster3-config-converter` CLI command to convert cluster configuration from ParallelCluster 2 to ParallelCluster 3 version. - The region parameter is now retrieved from the provider chain, thus supporting the use of profiles and defaults diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index aeb2b40c3f..f681cab5c3 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -972,7 +972,9 @@ def _register_validators(self): SubnetsValidator, subnet_ids=self.compute_subnet_ids + [self.head_node.networking.subnet_id] ) self._register_storage_validators() - self._register_validator(HeadNodeLaunchTemplateValidator, head_node=self.head_node, ami_id=self.head_node_ami) + self._register_validator( + HeadNodeLaunchTemplateValidator, head_node=self.head_node, ami_id=self.head_node_ami, tags=self.tags + ) if self.head_node.dcv: self._register_validator( @@ -1520,7 +1522,7 @@ def _register_validators(self): for queue in self.scheduling.queues: self._register_validator( - ComputeResourceLaunchTemplateValidator, queue=queue, ami_id=self.image_dict[queue.name] + ComputeResourceLaunchTemplateValidator, queue=queue, ami_id=self.image_dict[queue.name], tags=self.tags ) queue_image = self.image_dict[queue.name] if queue_image not in checked_images and queue.queue_ami: diff --git a/cli/src/pcluster/validators/cluster_validators.py b/cli/src/pcluster/validators/cluster_validators.py index c0c8610b12..ddcc4d7da5 100644 --- a/cli/src/pcluster/validators/cluster_validators.py +++ b/cli/src/pcluster/validators/cluster_validators.py @@ -799,11 +799,21 @@ def _ec2_run_instance(self, availability_zone: str, **kwargs): # noqa: C901 FIX FailureLevel.ERROR, ) + @staticmethod + def _generate_tag_specifications(tags): + """Turn list of Tag objects into tag specifications required by RunInstances.""" + tag_specifications = [] + if tags: + tag_specifications.append( + {"ResourceType": "instance", "Tags": [{"Key": tag.key, "Value": tag.value} for tag in tags]} + ) + return tag_specifications + class HeadNodeLaunchTemplateValidator(_LaunchTemplateValidator): """Try to launch the requested instance (in dry-run mode) to verify configuration parameters.""" - def _validate(self, head_node, ami_id): + def _validate(self, head_node, ami_id, tags): try: head_node_security_groups = [] if head_node.networking.security_groups: @@ -835,6 +845,7 @@ def _validate(self, head_node, ami_id): CpuOptions=head_node_cpu_options, NetworkInterfaces=head_node_network_interfaces, DryRun=True, + TagSpecifications=self._generate_tag_specifications(tags), ) except Exception as e: self._add_failure( @@ -865,7 +876,7 @@ def _validate(self, imds_secured: bool, scheduler: str): class ComputeResourceLaunchTemplateValidator(_LaunchTemplateValidator): """Try to launch the requested instances (in dry-run mode) to verify configuration parameters.""" - def _validate(self, queue, ami_id): + def _validate(self, queue, ami_id, tags): try: # Retrieve network parameters queue_subnet_id = queue.networking.subnet_ids[0] @@ -893,6 +904,7 @@ def _validate(self, queue, ami_id): subnet_id=queue_subnet_id, security_groups_ids=queue_security_groups, placement_group=queue_placement_group, + tags=tags, ) except Exception as e: self._add_failure( @@ -900,7 +912,7 @@ def _validate(self, queue, ami_id): ) def _test_compute_resource( - self, compute_resource, use_public_ips, ami_id, subnet_id, security_groups_ids, placement_group + self, compute_resource, use_public_ips, ami_id, subnet_id, security_groups_ids, placement_group, tags ): """Test Compute Resource Instance Configuration.""" compute_cpu_options = ( @@ -925,6 +937,7 @@ def _test_compute_resource( Placement=placement_group, NetworkInterfaces=network_interfaces, DryRun=True, + TagSpecifications=self._generate_tag_specifications(tags), ) diff --git a/cli/tests/pcluster/validators/test_cluster_validators.py b/cli/tests/pcluster/validators/test_cluster_validators.py index 9e68d6c693..953fd876e0 100644 --- a/cli/tests/pcluster/validators/test_cluster_validators.py +++ b/cli/tests/pcluster/validators/test_cluster_validators.py @@ -9,8 +9,10 @@ # OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and # limitations under the License. import pytest +from assertpy import assert_that from pcluster.aws.aws_resources import InstanceTypeInfo +from pcluster.config.cluster_config import Tag from pcluster.constants import PCLUSTER_NAME_MAX_LENGTH from pcluster.validators.cluster_validators import ( FSX_MESSAGES, @@ -39,6 +41,7 @@ RegionValidator, SchedulerOsValidator, SharedStorageNameValidator, + _LaunchTemplateValidator, ) from tests.pcluster.aws.dummy_aws_api import mock_aws_api from tests.pcluster.validators.utils import assert_failure_messages @@ -885,3 +888,22 @@ def test_hosted_zone_validator(mocker, vpcs, is_private_zone, domain_name, expec cluster_name="ThisClusterNameShouldBeRightSize-ContainAHyphen-AndANumber12", ) assert_failure_messages(actual_failures, expected_message) + + +@pytest.mark.parametrize( + "input_tags", + [ + [], + [{"key": "SomeKey", "value": "SomeValue"}], + ], +) +def test_generate_tag_specifications(input_tags): + """Verify function to generate tag specifications for dry runs of RunInstances works as expected.""" + input_tags = [Tag(tag.get("key"), tag.get("value")) for tag in input_tags] + if input_tags: + expected_output_tags = [ + {"ResourceType": "instance", "Tags": [{"Key": tag.key, "Value": tag.value} for tag in input_tags]} + ] + else: + expected_output_tags = [] + assert_that(_LaunchTemplateValidator._generate_tag_specifications(input_tags)).is_equal_to(expected_output_tags)