From 5550a39fa4af5c880dc1ce42e9f999c89638d543 Mon Sep 17 00:00:00 2001 From: Thomas Heinen Date: Fri, 19 Jul 2024 18:31:24 +0200 Subject: [PATCH 01/11] Add EFS access point support (aws-parallelcluster#2337) Signed-off-by: Thomas Heinen --- cli/src/pcluster/config/cluster_config.py | 3 +++ cli/src/pcluster/schemas/cluster_schema.py | 5 ++++- cli/src/pcluster/templates/cluster_stack.py | 5 +++++ cli/src/pcluster/templates/login_nodes_stack.py | 4 ++++ cli/src/pcluster/templates/queues_stack.py | 4 ++++ cli/src/pcluster/validators/efs_validators.py | 2 +- .../pcluster3_config_converter/pcluster3_config_converter.py | 1 + 7 files changed, 22 insertions(+), 2 deletions(-) diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index 2420d945bd..ff2bc2eb15 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -371,6 +371,7 @@ def __init__( deletion_policy: str = None, encryption_in_transit: bool = None, iam_authorization: bool = None, + accesspoint_id: str = None, ): super().__init__() self.mount_dir = Resource.init_param(mount_dir) @@ -387,6 +388,7 @@ def __init__( ) self.encryption_in_transit = Resource.init_param(encryption_in_transit, default=False) self.iam_authorization = Resource.init_param(iam_authorization, default=False) + self.accesspoint_id = Resource.init_param(accesspoint_id) def _register_validators(self, context: ValidatorContext = None): # noqa: D102 #pylint: disable=unused-argument self._register_validator(SharedStorageNameValidator, name=self.name) @@ -398,6 +400,7 @@ def _register_validators(self, context: ValidatorContext = None): # noqa: D102 EfsMountOptionsValidator, encryption_in_transit=self.encryption_in_transit, iam_authorization=self.iam_authorization, + accesspoint_id=self.accesspoint_id, name=self.name, ) diff --git a/cli/src/pcluster/schemas/cluster_schema.py b/cli/src/pcluster/schemas/cluster_schema.py index cbc498ba3b..3a1f73e18d 100644 --- a/cli/src/pcluster/schemas/cluster_schema.py +++ b/cli/src/pcluster/schemas/cluster_schema.py @@ -321,6 +321,9 @@ class EfsSettingsSchema(BaseSchema): deletion_policy = fields.Str( validate=validate.OneOf(DELETION_POLICIES), metadata={"update_policy": UpdatePolicy.SUPPORTED} ) + accesspoint_id = fields.Str( + validate=validate.Regexp(r"^fsap-[0-9a-z]{17}$"), + ) encryption_in_transit = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) iam_authorization = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) @@ -331,7 +334,7 @@ def validate_file_system_id_ignored_parameters(self, data, **kwargs): messages = [] if data.get("file_system_id") is not None: for key in data: - if key is not None and key not in ["encryption_in_transit", "iam_authorization", "file_system_id"]: + if key is not None and key not in ["encryption_in_transit", "iam_authorization", "file_system_id", "accesspoint_id"]: messages.append(EFS_MESSAGES["errors"]["ignored_param_with_efs_fs_id"].format(efs_param=key)) if messages: raise ValidationError(message=messages) diff --git a/cli/src/pcluster/templates/cluster_stack.py b/cli/src/pcluster/templates/cluster_stack.py index 04359c0618..b191da3a0a 100644 --- a/cli/src/pcluster/templates/cluster_stack.py +++ b/cli/src/pcluster/templates/cluster_stack.py @@ -1115,6 +1115,7 @@ def _add_efs_storage(self, id: str, shared_efs: SharedEfs): shared_efs.encryption_in_transit ) self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"].append(shared_efs.iam_authorization) + self.shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"].append(shared_efs.accesspoint_id) return efs_id @@ -1294,6 +1295,10 @@ def _add_head_node(self): "efs_iam_authorizations": to_comma_separated_string( self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True ), + "efs_accesspoint_ids": to_comma_separated_string( + self.shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + use_lower_case=True, + ), "fsx_fs_ids": get_shared_storage_ids_by_type(self.shared_storage_infos, SharedStorageType.FSX), "fsx_mount_names": to_comma_separated_string( self.shared_storage_attributes[SharedStorageType.FSX]["MountNames"] diff --git a/cli/src/pcluster/templates/login_nodes_stack.py b/cli/src/pcluster/templates/login_nodes_stack.py index 178bb949d2..5547fdd002 100644 --- a/cli/src/pcluster/templates/login_nodes_stack.py +++ b/cli/src/pcluster/templates/login_nodes_stack.py @@ -237,6 +237,10 @@ def _add_login_nodes_pool_launch_template(self): self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True, ), + "efs_accesspoint_ids": to_comma_separated_string( + self._shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + use_lower_case=True, + ), "enable_intel_hpc_platform": "true" if self._config.is_intel_hpc_platform_enabled else "false", "ephemeral_dir": DEFAULT_EPHEMERAL_DIR, "fsx_fs_ids": get_shared_storage_ids_by_type(self._shared_storage_infos, SharedStorageType.FSX), diff --git a/cli/src/pcluster/templates/queues_stack.py b/cli/src/pcluster/templates/queues_stack.py index 9f2a824a6e..fa67a3d692 100644 --- a/cli/src/pcluster/templates/queues_stack.py +++ b/cli/src/pcluster/templates/queues_stack.py @@ -335,6 +335,10 @@ def _add_compute_resource_launch_template( self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True, ), + "efs_accesspoint_ids": to_comma_separated_string( + self._shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + use_lower_case=True, + ), "fsx_fs_ids": get_shared_storage_ids_by_type(self._shared_storage_infos, SharedStorageType.FSX), "fsx_mount_names": to_comma_separated_string( self._shared_storage_attributes[SharedStorageType.FSX]["MountNames"] diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index 35ca33c4df..8ba24865f5 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -18,7 +18,7 @@ class EfsMountOptionsValidator(Validator): IAM Authorization requires Encryption in Transit. """ - def _validate(self, encryption_in_transit: bool, iam_authorization: bool, name: str): + def _validate(self, encryption_in_transit: bool, iam_authorization: bool, accesspoint_id: str, name: str): if iam_authorization and not encryption_in_transit: self._add_failure( "EFS IAM authorization cannot be enabled when encryption in-transit is disabled. " diff --git a/cli/src/pcluster3_config_converter/pcluster3_config_converter.py b/cli/src/pcluster3_config_converter/pcluster3_config_converter.py index 5a26fcade3..e0f148d7a0 100644 --- a/cli/src/pcluster3_config_converter/pcluster3_config_converter.py +++ b/cli/src/pcluster3_config_converter/pcluster3_config_converter.py @@ -296,6 +296,7 @@ def convert_efs_settings(self, section_name): ("efs_kms_key_id", "KmsKeyId"), ("provisioned_throughput", "ProvisionedThroughput", "getint"), ("throughput_mode", "ThroughputMode"), + ("accesspoint_id", "AccesspointId"), ] efs_section, efs_dict, _section_label = self.convert_storage_base( "efs", efs_label.strip(), additional_items From 80ddc7854defe1a77e3d76a9c399a5e437a0c54c Mon Sep 17 00:00:00 2001 From: Thomas Heinen Date: Fri, 2 Aug 2024 13:19:50 +0000 Subject: [PATCH 02/11] Rename new setting, Add changelog, Fix style, fix some tests Signed-off-by: Thomas Heinen --- CHANGELOG.md | 1 + cli/src/pcluster/aws/efs.py | 11 +++++++++++ cli/src/pcluster/config/cluster_config.py | 6 +++--- cli/src/pcluster/schemas/cluster_schema.py | 11 ++++++++--- cli/src/pcluster/templates/cluster_stack.py | 6 +++--- cli/src/pcluster/templates/login_nodes_stack.py | 4 ++-- cli/src/pcluster/templates/queues_stack.py | 4 ++-- .../pcluster3_config_converter.py | 2 +- .../head_node_default.dna.json | 1 + .../test_login_nodes_dna_json/dna-1.json | 1 + .../test_login_nodes_dna_json/dna-2.json | 1 + .../test_compute_nodes_dna_json/dna-1.json | 1 + .../test_compute_nodes_dna_json/dna-2.json | 1 + 13 files changed, 36 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 826cf8f21c..e715191f64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ CHANGELOG - Add support for ap-southeast-3 region. - Add security groups to login node network load balancer. - Add `AllowedIps` configuration for login nodes. +- Add new configuration `SharedStorage/EfsSettings/AccessPointId` to specify an optional EFS access point for a mount **BUG FIXES** - Fix validator `EfaPlacementGroupValidator` so that it does not suggest to configure a Placement Group when Capacity Blocks are used. diff --git a/cli/src/pcluster/aws/efs.py b/cli/src/pcluster/aws/efs.py index ed55457416..5dd711e365 100644 --- a/cli/src/pcluster/aws/efs.py +++ b/cli/src/pcluster/aws/efs.py @@ -80,3 +80,14 @@ def describe_file_system(self, efs_fs_id): :return: the mount_target_ids """ return self._client.describe_file_systems(FileSystemId=efs_fs_id) + + @AWSExceptionHandler.handle_client_exception + @Cache.cached + def describe_access_point(self, access_point_id): + """ + Describe access point attributes for the given EFS access point id. + + :param efaccess_point_ids_ap_id: EFS access point Id + :return: the access_point details + """ + return self._client.describe_access_points(AccessPointId=access_point_id) \ No newline at end of file diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index ff2bc2eb15..999b0a0c87 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -371,7 +371,7 @@ def __init__( deletion_policy: str = None, encryption_in_transit: bool = None, iam_authorization: bool = None, - accesspoint_id: str = None, + access_point_id: str = None, ): super().__init__() self.mount_dir = Resource.init_param(mount_dir) @@ -388,7 +388,7 @@ def __init__( ) self.encryption_in_transit = Resource.init_param(encryption_in_transit, default=False) self.iam_authorization = Resource.init_param(iam_authorization, default=False) - self.accesspoint_id = Resource.init_param(accesspoint_id) + self.access_point_id = Resource.init_param(access_point_id) def _register_validators(self, context: ValidatorContext = None): # noqa: D102 #pylint: disable=unused-argument self._register_validator(SharedStorageNameValidator, name=self.name) @@ -400,7 +400,7 @@ def _register_validators(self, context: ValidatorContext = None): # noqa: D102 EfsMountOptionsValidator, encryption_in_transit=self.encryption_in_transit, iam_authorization=self.iam_authorization, - accesspoint_id=self.accesspoint_id, + access_point_id=self.access_point_id, name=self.name, ) diff --git a/cli/src/pcluster/schemas/cluster_schema.py b/cli/src/pcluster/schemas/cluster_schema.py index 3a1f73e18d..a30d99e370 100644 --- a/cli/src/pcluster/schemas/cluster_schema.py +++ b/cli/src/pcluster/schemas/cluster_schema.py @@ -321,8 +321,8 @@ class EfsSettingsSchema(BaseSchema): deletion_policy = fields.Str( validate=validate.OneOf(DELETION_POLICIES), metadata={"update_policy": UpdatePolicy.SUPPORTED} ) - accesspoint_id = fields.Str( - validate=validate.Regexp(r"^fsap-[0-9a-z]{17}$"), + access_point_id = fields.Str( + validate=validate.Regexp(r"^fsap-[0-9a-z]{17}$"), metadata={"update_policy": UpdatePolicy.SUPPORTED} ) encryption_in_transit = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) iam_authorization = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) @@ -334,7 +334,12 @@ def validate_file_system_id_ignored_parameters(self, data, **kwargs): messages = [] if data.get("file_system_id") is not None: for key in data: - if key is not None and key not in ["encryption_in_transit", "iam_authorization", "file_system_id", "accesspoint_id"]: + if key is not None and key not in [ + "encryption_in_transit", + "iam_authorization", + "file_system_id", + "access_point_id" + ]: messages.append(EFS_MESSAGES["errors"]["ignored_param_with_efs_fs_id"].format(efs_param=key)) if messages: raise ValidationError(message=messages) diff --git a/cli/src/pcluster/templates/cluster_stack.py b/cli/src/pcluster/templates/cluster_stack.py index b191da3a0a..c81a170242 100644 --- a/cli/src/pcluster/templates/cluster_stack.py +++ b/cli/src/pcluster/templates/cluster_stack.py @@ -1115,7 +1115,7 @@ def _add_efs_storage(self, id: str, shared_efs: SharedEfs): shared_efs.encryption_in_transit ) self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"].append(shared_efs.iam_authorization) - self.shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"].append(shared_efs.accesspoint_id) + self.shared_storage_attributes[SharedStorageType.EFS]["AccessPointIds"].append(shared_efs.access_point_id) return efs_id @@ -1295,8 +1295,8 @@ def _add_head_node(self): "efs_iam_authorizations": to_comma_separated_string( self.shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True ), - "efs_accesspoint_ids": to_comma_separated_string( - self.shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + "efs_access_point_ids": to_comma_separated_string( + self.shared_storage_attributes[SharedStorageType.EFS]["AccessPointIds"], use_lower_case=True, ), "fsx_fs_ids": get_shared_storage_ids_by_type(self.shared_storage_infos, SharedStorageType.FSX), diff --git a/cli/src/pcluster/templates/login_nodes_stack.py b/cli/src/pcluster/templates/login_nodes_stack.py index 5547fdd002..8fd66ff264 100644 --- a/cli/src/pcluster/templates/login_nodes_stack.py +++ b/cli/src/pcluster/templates/login_nodes_stack.py @@ -237,8 +237,8 @@ def _add_login_nodes_pool_launch_template(self): self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True, ), - "efs_accesspoint_ids": to_comma_separated_string( - self._shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + "efs_access_point_ids": to_comma_separated_string( + self._shared_storage_attributes[SharedStorageType.EFS]["AccessPointIds"], use_lower_case=True, ), "enable_intel_hpc_platform": "true" if self._config.is_intel_hpc_platform_enabled else "false", diff --git a/cli/src/pcluster/templates/queues_stack.py b/cli/src/pcluster/templates/queues_stack.py index fa67a3d692..19cb4a357f 100644 --- a/cli/src/pcluster/templates/queues_stack.py +++ b/cli/src/pcluster/templates/queues_stack.py @@ -335,8 +335,8 @@ def _add_compute_resource_launch_template( self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], use_lower_case=True, ), - "efs_accesspoint_ids": to_comma_separated_string( - self._shared_storage_attributes[SharedStorageType.EFS]["AccesspointIds"], + "efs_access_point_ids": to_comma_separated_string( + self._shared_storage_attributes[SharedStorageType.EFS]["AccessPointIds"], use_lower_case=True, ), "fsx_fs_ids": get_shared_storage_ids_by_type(self._shared_storage_infos, SharedStorageType.FSX), diff --git a/cli/src/pcluster3_config_converter/pcluster3_config_converter.py b/cli/src/pcluster3_config_converter/pcluster3_config_converter.py index e0f148d7a0..d1ccb32b0c 100644 --- a/cli/src/pcluster3_config_converter/pcluster3_config_converter.py +++ b/cli/src/pcluster3_config_converter/pcluster3_config_converter.py @@ -296,7 +296,7 @@ def convert_efs_settings(self, section_name): ("efs_kms_key_id", "KmsKeyId"), ("provisioned_throughput", "ProvisionedThroughput", "getint"), ("throughput_mode", "ThroughputMode"), - ("accesspoint_id", "AccesspointId"), + ("access_point_id", "AccessPointId"), ] efs_section, efs_dict, _section_label = self.convert_storage_base( "efs", efs_label.strip(), additional_items diff --git a/cli/tests/pcluster/templates/test_cluster_stack/test_head_node_dna_json/head_node_default.dna.json b/cli/tests/pcluster/templates/test_cluster_stack/test_head_node_dna_json/head_node_default.dna.json index 0862440334..30db9a7a44 100644 --- a/cli/tests/pcluster/templates/test_cluster_stack/test_head_node_dna_json/head_node_default.dna.json +++ b/cli/tests/pcluster/templates/test_cluster_stack/test_head_node_dna_json/head_node_default.dna.json @@ -20,6 +20,7 @@ "efs_fs_ids": "", "efs_iam_authorizations": "", "efs_shared_dirs": "", + "efs_access_point_ids": "", "enable_intel_hpc_platform": "false", "ephemeral_dir": "/scratch", "fsx_dns_names": "", diff --git a/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-1.json b/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-1.json index b286ed6203..170fcdef01 100644 --- a/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-1.json +++ b/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-1.json @@ -22,6 +22,7 @@ "efs_fs_ids": "", "efs_iam_authorizations": "", "efs_shared_dirs": "", + "efs_access_point_ids": "", "enable_intel_hpc_platform": "false", "ephemeral_dir": "/scratch", "fsx_dns_names": "", diff --git a/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-2.json b/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-2.json index c67887e3a4..7164689caa 100644 --- a/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-2.json +++ b/cli/tests/pcluster/templates/test_login_nodes_stack/test_login_nodes_dna_json/dna-2.json @@ -22,6 +22,7 @@ "efs_fs_ids": "", "efs_iam_authorizations": "", "efs_shared_dirs": "", + "efs_access_point_ids": "", "enable_intel_hpc_platform": "false", "ephemeral_dir": "/scratch", "fsx_dns_names": "", diff --git a/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-1.json b/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-1.json index 69a56fa080..18140d9c0c 100644 --- a/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-1.json +++ b/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-1.json @@ -20,6 +20,7 @@ "efs_fs_ids": "", "efs_iam_authorizations": "", "efs_shared_dirs": "", + "efs_access_point_ids": "", "enable_efa": "NONE", "enable_efa_gdr": "NONE", "enable_intel_hpc_platform": "false", diff --git a/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-2.json b/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-2.json index 56d0da1a2c..2f4f04c400 100644 --- a/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-2.json +++ b/cli/tests/pcluster/templates/test_queues_stack/test_compute_nodes_dna_json/dna-2.json @@ -20,6 +20,7 @@ "efs_fs_ids": "", "efs_iam_authorizations": "", "efs_shared_dirs": "", + "efs_access_point_ids": "", "enable_efa": "NONE", "enable_efa_gdr": "NONE", "enable_intel_hpc_platform": "false", From e81495c01fa2d90f250e29effb42f841bf14e2c3 Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Fri, 2 Aug 2024 11:42:04 +0000 Subject: [PATCH 03/11] Added tests for EFS Access Points --- cli/src/pcluster/validators/efs_validators.py | 8 +- .../validators/test_efs_validators.py | 42 +++++++++- tests/integration-tests/configs/develop.yaml | 6 ++ tests/integration-tests/conftest.py | 28 ++++++- .../tests/storage/storage_common.py | 9 +- .../tests/storage/test_efs.py | 83 +++++++++++++++++++ .../pcluster.config.yaml | 51 ++++++++++++ 7 files changed, 217 insertions(+), 10 deletions(-) create mode 100644 tests/integration-tests/tests/storage/test_efs/test_efs_access_point/pcluster.config.yaml diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index 8ba24865f5..486fdfb273 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -18,10 +18,16 @@ class EfsMountOptionsValidator(Validator): IAM Authorization requires Encryption in Transit. """ - def _validate(self, encryption_in_transit: bool, iam_authorization: bool, accesspoint_id: str, name: str): + def _validate(self, encryption_in_transit: bool, iam_authorization: bool, access_point_id: str, name: str): if iam_authorization and not encryption_in_transit: self._add_failure( "EFS IAM authorization cannot be enabled when encryption in-transit is disabled. " f"Please either disable IAM authorization or enable encryption in-transit for file system {name}", FailureLevel.ERROR, ) + if access_point_id and not name: + self._add_failure( + "An access point can only be specified when using an existing EFS file system. " + f"Please either remove the access point id {access_point_id} or provide the file system id for the access point", + FailureLevel.ERROR, + ) diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index 70a8408179..f3c955d47e 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -27,8 +27,8 @@ False, True, "EFS IAM authorization cannot be enabled when encryption in-transit is disabled. " - "Please either disable IAM authorization or enable encryption in-transit for file system " - "", + "Please either disable IAM authorization or enable encryption in-transit " + "for file system ", ), ( True, @@ -42,8 +42,42 @@ ), ], ) -def test_efs_mount_options_validator(encryption_in_transit, iam_authorization, expected_message): +def test_efs_mount_options_validator( + encryption_in_transit, iam_authorization, access_point_id, file_system_id, expected_message +): actual_failures = EfsMountOptionsValidator().execute( - encryption_in_transit, iam_authorization, "" + encryption_in_transit, iam_authorization, None, "" ) assert_failure_messages(actual_failures, expected_message) + + +@pytest.mark.parametrize( + "access_point_id, name_of_the_file_system, expected_message", + [ + ( + None, + None, + None, + ), + ( + "", + None, + "An access point can only be specified when using an existing EFS file system. " + "Please either remove the access point id " + "or provide the file system id for the access point", + ), + ( + "", + "", + None, + ), + ( + None, + "", + None, + ), + ], +) +def test_efs_access_point_validator(access_point_id, name_of_the_file_system, expected_message): + actual_failures = EfsMountOptionsValidator().execute(False, False, access_point_id, name_of_the_file_system) + assert_failure_messages(actual_failures, expected_message) diff --git a/tests/integration-tests/configs/develop.yaml b/tests/integration-tests/configs/develop.yaml index 81fdbad379..c3d1e21d40 100644 --- a/tests/integration-tests/configs/develop.yaml +++ b/tests/integration-tests/configs/develop.yaml @@ -635,6 +635,12 @@ test-suites: instances: {{ common.INSTANCES_DEFAULT_X86 }} oss: ["rhel8"] schedulers: ["slurm"] + test_efs.py::test_efs_access_point: + dimensions: + - regions: ["us-east-2"] + instances: {{ common.INSTANCES_DEFAULT_X86 }} + oss: ["alinux2"] + schedulers: ["slurm"] test_raid.py::test_raid_fault_tolerance_mode: dimensions: - regions: ["cn-northwest-1"] diff --git a/tests/integration-tests/conftest.py b/tests/integration-tests/conftest.py index 1db53d37e7..06f215de74 100644 --- a/tests/integration-tests/conftest.py +++ b/tests/integration-tests/conftest.py @@ -54,7 +54,7 @@ from jinja2.sandbox import SandboxedEnvironment from troposphere import Ref, Sub, Template, ec2, resourcegroups from troposphere.ec2 import PlacementGroup -from troposphere.efs import FileSystem as EFSFileSystem +from troposphere.efs import FileSystem as EFSFileSystem, AccessPoint as EFSAccessPoint from troposphere.efs import MountTarget from troposphere.fsx import ( ClientConfigurations, @@ -1785,6 +1785,32 @@ def create_efs(num=1): for stack in created_stacks: cfn_stacks_factory.delete_stack(stack.name, region) +@pytest.fixture(scope="class") +def efs_access_point_stack_factory(cfn_stacks_factory, request, region, vpc_stack): + """EFS stack contains a single efs and a single access point resource.""" + created_stacks = [] + + def create_access_points(efs_fs_id, num=1): + ap_template = Template() + ap_template.set_version("2010-09-09") + ap_template.set_description("Access Point stack created for testing existing EFS wtith Access points") + access_point_resource_name = "AccessPointResourceResource" + for i in range(num): + access_point = EFSAccessPoint(f"{access_point_resource_name}{i}") + access_point.FileSystemId = efs_fs_id + ap_template.add_resource(access_point) + stack_name = generate_stack_name("integ-tests-efs-ap", request.config.getoption("stackname_suffix")) + stack = CfnStack(name=stack_name, region=region, template=ap_template.to_json()) + cfn_stacks_factory.create_stack(stack) + created_stacks.append(stack) + return [stack.cfn_resources[f"{access_point_resource_name}{i}"] for i in range(num)] + + yield create_access_points + + if not request.config.getoption("no_delete"): + for stack in created_stacks: + cfn_stacks_factory.delete_stack(stack.name, region) + @pytest.fixture(scope="class") def efs_mount_target_stack_factory(cfn_stacks_factory, request, region, vpc_stack): diff --git a/tests/integration-tests/tests/storage/storage_common.py b/tests/integration-tests/tests/storage/storage_common.py index b8b2e2c6ae..525b230d76 100644 --- a/tests/integration-tests/tests/storage/storage_common.py +++ b/tests/integration-tests/tests/storage/storage_common.py @@ -222,7 +222,7 @@ def test_raid_correctly_mounted(remote_command_executor, mount_dir, volume_size) def write_file_into_efs( - region, vpc_stack: CfnVpcStack, efs_ids, request, key_name, cfn_stacks_factory, efs_mount_target_stack_factory + region, vpc_stack: CfnVpcStack, efs_ids, request, key_name, cfn_stacks_factory, efs_mount_target_stack_factory, access_point_id=None ): """Write file stack contains an instance to write an empty file with random name into each of the efs in efs_ids.""" write_file_template = Template() @@ -236,7 +236,7 @@ def write_file_into_efs( write_file_user_data = "" for efs_id in efs_ids: random_file_name = random_alphanumeric() - write_file_user_data += _write_user_data(efs_id, random_file_name) + write_file_user_data += _write_user_data(efs_id, random_file_name, access_point_id=access_point_id) random_file_names.append(random_file_name) user_data = f""" #cloud-config @@ -312,11 +312,12 @@ def write_file_into_efs( return random_file_names -def _write_user_data(efs_id, random_file_name): +def _write_user_data(efs_id, random_file_name, access_point_id=None): mount_dir = "/mnt/efs/fs" + access_point_mount_parameter = f",accesspoint={access_point_id}" if access_point_id is not None else "" return f""" - mkdir -p {mount_dir} - - mount -t efs -o tls,iam {efs_id}:/ {mount_dir} + - mount -t efs -o tls,iam{access_point_mount_parameter} {efs_id}:/ {mount_dir} - touch {mount_dir}/{random_file_name} - umount {mount_dir} """ # noqa: E501 diff --git a/tests/integration-tests/tests/storage/test_efs.py b/tests/integration-tests/tests/storage/test_efs.py index 8d93445276..e007a36d6f 100644 --- a/tests/integration-tests/tests/storage/test_efs.py +++ b/tests/integration-tests/tests/storage/test_efs.py @@ -200,7 +200,90 @@ def test_multiple_efs( encryption_in_transits, ) +@pytest.mark.usefixtures("instance") +def test_efs_access_point( + os, + region, + scheduler, + efs_stack_factory, + efs_mount_target_stack_factory, + efs_access_point_stack_factory, + pcluster_config_reader, + clusters_factory, + vpc_stack, + request, + key_name, + cfn_stacks_factory, + scheduler_commands_factory, +): + """ + Test when efs_fs_id is provided in the config file, the existing efs can be correctly mounted. + + To verify the efs is the existing efs, the test expects a file with random ran inside the efs mounted + """ + # Names of files that will be written from separate instance. The test checks the cluster nodes can access them. + efs_filenames = [] + iam_authorizations = [False, False, True] if scheduler != "awsbatch" else 3 * [False] + encryption_in_transits = [False, True, True] if scheduler != "awsbatch" else 3 * [False] + # create an additional EFS with file system policy to prevent anonymous access + efs_filesystem_id = efs_stack_factory()[0] + efs_mount_target_stack_factory([efs_filesystem_id]) + access_point_id = efs_access_point_stack_factory(efs_fs_id=efs_filesystem_id)[0] + if scheduler != "awsbatch": + account_id = ( + boto3.client("sts", region_name=region, endpoint_url=get_sts_endpoint(region)) + .get_caller_identity() + .get("Account") + ) + policy = { + "Version": "2012-10-17", + "Id": "efs-policy-denying-access-for-direct-efs-access", + "Statement": [ + { + "Sid": "efs-block-not-access-point-in-account", + "Effect": "Deny", + "Principal": {"AWS": "*"}, + "Action": [ + "elasticfilesystem:ClientMount", + "elasticfilesystem:ClientRootAccess", + "elasticfilesystem:ClientWrite", + ], + "Resource": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" + f"file-system/{efs_filesystem_id}", + "Condition": { + "StringNotLike": {"elasticfilesystem:AccessPointArn": + f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:access-point/{access_point_id}" + } + }, + }, + { + "Sid": "efs-allow-accesspoint-in-account", + "Effect": "Allow", + "Principal": {"AWS": "*"}, + "Action": [ + "elasticfilesystem:ClientMount", + "elasticfilesystem:ClientRootAccess", + "elasticfilesystem:ClientWrite", + ], + "Resource": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" + f"file-system/{efs_filesystem_id}" + } + ] + } + boto3.client("efs").put_file_system_policy(FileSystemId=efs_filesystem_id, Policy=json.dumps(policy)) + + mount_dir = "efs_mount_dir" + cluster_config = pcluster_config_reader(mount_dir=mount_dir, efs_filesystem_id=efs_filesystem_id, access_point_id=access_point_id) + cluster = clusters_factory(cluster_config) + remote_command_executor = RemoteCommandExecutor(cluster) + + mount_dir = "/" + mount_dir + scheduler_commands = scheduler_commands_factory(remote_command_executor) + test_efs_correctly_mounted(remote_command_executor, mount_dir) + _test_efs_correctly_shared(remote_command_executor, mount_dir, scheduler_commands) + + def _check_efs_after_nodes_reboot( all_mount_dirs, cluster, diff --git a/tests/integration-tests/tests/storage/test_efs/test_efs_access_point/pcluster.config.yaml b/tests/integration-tests/tests/storage/test_efs/test_efs_access_point/pcluster.config.yaml new file mode 100644 index 0000000000..712a0f0cfd --- /dev/null +++ b/tests/integration-tests/tests/storage/test_efs/test_efs_access_point/pcluster.config.yaml @@ -0,0 +1,51 @@ +Image: + Os: {{ os }} +HeadNode: + InstanceType: {{ instance }} + Networking: + SubnetId: {{ public_subnet_ids[0] }} + Ssh: + KeyName: {{ key_name }} + Imds: + Secured: {{ imds_secured }} +Scheduling: + Scheduler: {{ scheduler }} + {% if scheduler == "awsbatch" %}AwsBatchQueues:{% else %}SlurmQueues:{% endif %} + - Name: queue-0 + ComputeResources: + - Name: compute-resource-0 + {% if scheduler == "awsbatch" %} + InstanceTypes: + - {{ instance }} + MinvCpus: 4 + DesiredvCpus: 4 + {% else %} + Instances: + - InstanceType: {{ instance }} + MinCount: 1 + MaxCount: 1 + {% endif %} + Networking: + SubnetIds: + - {{ private_subnet_ids[1] }} + {% if scheduler == "slurm" %} + - Name: queue-1 + ComputeResources: + - Name: compute-resource-0 + Instances: + - InstanceType: {{ instance }} + MinCount: 1 + MaxCount: 1 + Networking: + SubnetIds: + - {% if private_subnet_ids|length >= 3 %} {{ private_subnet_ids[2] }} {% else %} {{ private_subnet_ids[1] }} {% endif %} + {% endif %} +# This compute subnet would be in a different AZ than head node for regions defined in AVAILABILITY_ZONE_OVERRIDES +# See conftest for details +SharedStorage: + - MountDir: {{ mount_dir }} + Name: efs + StorageType: Efs + EfsSettings: + FileSystemId: {{ efs_filesystem_id }} + AccessPointId: {{ access_point_id }} \ No newline at end of file From 0b184a6cf864d7a211ab931e5eff869fab8dc98a Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Fri, 9 Aug 2024 17:15:27 +0000 Subject: [PATCH 04/11] Improved unit test for access point --- cli/src/pcluster/config/cluster_config.py | 9 ++++++--- cli/src/pcluster/schemas/cluster_schema.py | 2 +- cli/src/pcluster/validators/efs_validators.py | 14 ++++++++++++-- .../pcluster/validators/test_efs_validators.py | 18 ++++++++++-------- 4 files changed, 29 insertions(+), 14 deletions(-) diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index 999b0a0c87..74f7048133 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -149,7 +149,7 @@ PlacementGroupCapacityTypeValidator, PlacementGroupNamingValidator, ) -from pcluster.validators.efs_validators import EfsMountOptionsValidator +from pcluster.validators.efs_validators import EfsMountOptionsValidator, EfsAccessPointOptionsValidator from pcluster.validators.feature_validators import FeatureRegionValidator from pcluster.validators.fsx_validators import ( FsxAutoImportValidator, @@ -400,10 +400,13 @@ def _register_validators(self, context: ValidatorContext = None): # noqa: D102 EfsMountOptionsValidator, encryption_in_transit=self.encryption_in_transit, iam_authorization=self.iam_authorization, - access_point_id=self.access_point_id, name=self.name, ) - + self._register_validator( + EfsAccessPointOptionsValidator, + access_point_id=self.access_point_id, + file_system_id=self.file_system_id, + ) class BaseSharedFsx(Resource): """Represent the shared FSX resource.""" diff --git a/cli/src/pcluster/schemas/cluster_schema.py b/cli/src/pcluster/schemas/cluster_schema.py index a30d99e370..1d0b7647dc 100644 --- a/cli/src/pcluster/schemas/cluster_schema.py +++ b/cli/src/pcluster/schemas/cluster_schema.py @@ -322,7 +322,7 @@ class EfsSettingsSchema(BaseSchema): validate=validate.OneOf(DELETION_POLICIES), metadata={"update_policy": UpdatePolicy.SUPPORTED} ) access_point_id = fields.Str( - validate=validate.Regexp(r"^fsap-[0-9a-z]{17}$"), metadata={"update_policy": UpdatePolicy.SUPPORTED} + validate=validate.Regexp(r"^fsap-[0-9a-z]{17}$"), metadata={"update_policy": UpdatePolicy.UNSUPPORTED} ) encryption_in_transit = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) iam_authorization = fields.Bool(metadata={"update_policy": UpdatePolicy.UNSUPPORTED}) diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index 486fdfb273..3e2a3c5b4a 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -18,14 +18,24 @@ class EfsMountOptionsValidator(Validator): IAM Authorization requires Encryption in Transit. """ - def _validate(self, encryption_in_transit: bool, iam_authorization: bool, access_point_id: str, name: str): + def _validate(self, encryption_in_transit: bool, iam_authorization: bool, name: str): if iam_authorization and not encryption_in_transit: self._add_failure( "EFS IAM authorization cannot be enabled when encryption in-transit is disabled. " f"Please either disable IAM authorization or enable encryption in-transit for file system {name}", FailureLevel.ERROR, ) - if access_point_id and not name: + +class EfsAccessPointOptionsValidator(Validator): + """ + EFS Mount Options validator. + + IAM Authorization requires Encryption in Transit. + """ + + def _validate(self, access_point_id: str, file_system_id: str): + + if access_point_id and not file_system_id: self._add_failure( "An access point can only be specified when using an existing EFS file system. " f"Please either remove the access point id {access_point_id} or provide the file system id for the access point", diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index f3c955d47e..6b5233581d 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -11,7 +11,7 @@ import pytest -from pcluster.validators.efs_validators import EfsMountOptionsValidator +from pcluster.validators.efs_validators import EfsMountOptionsValidator, EfsAccessPointOptionsValidator from tests.pcluster.validators.utils import assert_failure_messages @@ -43,16 +43,16 @@ ], ) def test_efs_mount_options_validator( - encryption_in_transit, iam_authorization, access_point_id, file_system_id, expected_message + encryption_in_transit, iam_authorization, expected_message ): actual_failures = EfsMountOptionsValidator().execute( - encryption_in_transit, iam_authorization, None, "" + encryption_in_transit, iam_authorization, "" ) assert_failure_messages(actual_failures, expected_message) @pytest.mark.parametrize( - "access_point_id, name_of_the_file_system, expected_message", + "access_point_id, file_system_id, expected_message", [ ( None, @@ -68,16 +68,18 @@ def test_efs_mount_options_validator( ), ( "", - "", + "", None, ), ( None, - "", + "", None, ), ], ) -def test_efs_access_point_validator(access_point_id, name_of_the_file_system, expected_message): - actual_failures = EfsMountOptionsValidator().execute(False, False, access_point_id, name_of_the_file_system) +def test_efs_access_point_with_filesystem_validator(access_point_id, file_system_id, expected_message): + actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, file_system_id) assert_failure_messages(actual_failures, expected_message) + + From 80a35558344d14e3f19d654bf44b6768596a6345 Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Sat, 10 Aug 2024 10:05:06 +0000 Subject: [PATCH 05/11] Access point requires TLS https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#mount-with-access-point Added unit test --- cli/src/pcluster/config/cluster_config.py | 1 + cli/src/pcluster/validators/efs_validators.py | 9 +++++- .../validators/test_efs_validators.py | 32 +++++++++++++++++-- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index 74f7048133..25feab7754 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -406,6 +406,7 @@ def _register_validators(self, context: ValidatorContext = None): # noqa: D102 EfsAccessPointOptionsValidator, access_point_id=self.access_point_id, file_system_id=self.file_system_id, + encryption_in_transit=self.encryption_in_transit ) class BaseSharedFsx(Resource): diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index 3e2a3c5b4a..64ccbfee16 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -33,7 +33,7 @@ class EfsAccessPointOptionsValidator(Validator): IAM Authorization requires Encryption in Transit. """ - def _validate(self, access_point_id: str, file_system_id: str): + def _validate(self, access_point_id: str, file_system_id: str, encryption_in_transit: bool): if access_point_id and not file_system_id: self._add_failure( @@ -41,3 +41,10 @@ def _validate(self, access_point_id: str, file_system_id: str): f"Please either remove the access point id {access_point_id} or provide the file system id for the access point", FailureLevel.ERROR, ) + + if access_point_id and not encryption_in_transit: + self._add_failure( + "An access point can only be specified when encryption in transit is enabled. " + f"Please either remove the access point id {access_point_id} or enable encryption in transit.", + FailureLevel.ERROR, + ) diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index 6b5233581d..63107b5561 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -79,7 +79,35 @@ def test_efs_mount_options_validator( ], ) def test_efs_access_point_with_filesystem_validator(access_point_id, file_system_id, expected_message): - actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, file_system_id) + actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, file_system_id, True) assert_failure_messages(actual_failures, expected_message) - +@pytest.mark.parametrize( + "access_point_id, encryption_in_transit, expected_message", + [ + ( + None, + False, + None, + ), + ( + "", + False, + "An access point can only be specified when encryption in transit is enabled. " + "Please either remove the access point id or enable encryption in transit.", + ), + ( + "", + True, + None, + ), + ( + None, + True, + None, + ), + ], +) +def test_efs_access_point_with_filesystem_validator(access_point_id, encryption_in_transit, expected_message): + actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, "", encryption_in_transit) + assert_failure_messages(actual_failures, expected_message) \ No newline at end of file From 715596e8a516318dc3c3e1f13b73ccd3ae61c7a8 Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 14:24:27 +0000 Subject: [PATCH 06/11] used black formatter --- cli/src/pcluster/aws/efs.py | 2 +- cli/src/pcluster/config/cluster_config.py | 3 ++- cli/src/pcluster/schemas/cluster_schema.py | 2 +- cli/src/pcluster/validators/efs_validators.py | 3 ++- .../pcluster/validators/test_efs_validators.py | 11 ++++++----- tests/integration-tests/conftest.py | 1 + .../tests/storage/storage_common.py | 9 ++++++++- .../integration-tests/tests/storage/test_efs.py | 16 +++++++++------- 8 files changed, 30 insertions(+), 17 deletions(-) diff --git a/cli/src/pcluster/aws/efs.py b/cli/src/pcluster/aws/efs.py index 5dd711e365..9aa3ce94df 100644 --- a/cli/src/pcluster/aws/efs.py +++ b/cli/src/pcluster/aws/efs.py @@ -90,4 +90,4 @@ def describe_access_point(self, access_point_id): :param efaccess_point_ids_ap_id: EFS access point Id :return: the access_point details """ - return self._client.describe_access_points(AccessPointId=access_point_id) \ No newline at end of file + return self._client.describe_access_points(AccessPointId=access_point_id) diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index 25feab7754..4e2338dfa8 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -406,9 +406,10 @@ def _register_validators(self, context: ValidatorContext = None): # noqa: D102 EfsAccessPointOptionsValidator, access_point_id=self.access_point_id, file_system_id=self.file_system_id, - encryption_in_transit=self.encryption_in_transit + encryption_in_transit=self.encryption_in_transit, ) + class BaseSharedFsx(Resource): """Represent the shared FSX resource.""" diff --git a/cli/src/pcluster/schemas/cluster_schema.py b/cli/src/pcluster/schemas/cluster_schema.py index 1d0b7647dc..b33850100c 100644 --- a/cli/src/pcluster/schemas/cluster_schema.py +++ b/cli/src/pcluster/schemas/cluster_schema.py @@ -338,7 +338,7 @@ def validate_file_system_id_ignored_parameters(self, data, **kwargs): "encryption_in_transit", "iam_authorization", "file_system_id", - "access_point_id" + "access_point_id", ]: messages.append(EFS_MESSAGES["errors"]["ignored_param_with_efs_fs_id"].format(efs_param=key)) if messages: diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index 64ccbfee16..f9c5d8e05f 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -26,6 +26,7 @@ def _validate(self, encryption_in_transit: bool, iam_authorization: bool, name: FailureLevel.ERROR, ) + class EfsAccessPointOptionsValidator(Validator): """ EFS Mount Options validator. @@ -33,7 +34,7 @@ class EfsAccessPointOptionsValidator(Validator): IAM Authorization requires Encryption in Transit. """ - def _validate(self, access_point_id: str, file_system_id: str, encryption_in_transit: bool): + def _validate(self, access_point_id: str, file_system_id: str, encryption_in_transit: bool): if access_point_id and not file_system_id: self._add_failure( diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index 63107b5561..8f2d4f6022 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -42,9 +42,7 @@ ), ], ) -def test_efs_mount_options_validator( - encryption_in_transit, iam_authorization, expected_message -): +def test_efs_mount_options_validator(encryption_in_transit, iam_authorization, expected_message): actual_failures = EfsMountOptionsValidator().execute( encryption_in_transit, iam_authorization, "" ) @@ -82,6 +80,7 @@ def test_efs_access_point_with_filesystem_validator(access_point_id, file_system actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, file_system_id, True) assert_failure_messages(actual_failures, expected_message) + @pytest.mark.parametrize( "access_point_id, encryption_in_transit, expected_message", [ @@ -109,5 +108,7 @@ def test_efs_access_point_with_filesystem_validator(access_point_id, file_system ], ) def test_efs_access_point_with_filesystem_validator(access_point_id, encryption_in_transit, expected_message): - actual_failures = EfsAccessPointOptionsValidator().execute(access_point_id, "", encryption_in_transit) - assert_failure_messages(actual_failures, expected_message) \ No newline at end of file + actual_failures = EfsAccessPointOptionsValidator().execute( + access_point_id, "", encryption_in_transit + ) + assert_failure_messages(actual_failures, expected_message) diff --git a/tests/integration-tests/conftest.py b/tests/integration-tests/conftest.py index 06f215de74..f2f34bd707 100644 --- a/tests/integration-tests/conftest.py +++ b/tests/integration-tests/conftest.py @@ -1785,6 +1785,7 @@ def create_efs(num=1): for stack in created_stacks: cfn_stacks_factory.delete_stack(stack.name, region) + @pytest.fixture(scope="class") def efs_access_point_stack_factory(cfn_stacks_factory, request, region, vpc_stack): """EFS stack contains a single efs and a single access point resource.""" diff --git a/tests/integration-tests/tests/storage/storage_common.py b/tests/integration-tests/tests/storage/storage_common.py index 525b230d76..1b067ff74c 100644 --- a/tests/integration-tests/tests/storage/storage_common.py +++ b/tests/integration-tests/tests/storage/storage_common.py @@ -222,7 +222,14 @@ def test_raid_correctly_mounted(remote_command_executor, mount_dir, volume_size) def write_file_into_efs( - region, vpc_stack: CfnVpcStack, efs_ids, request, key_name, cfn_stacks_factory, efs_mount_target_stack_factory, access_point_id=None + region, + vpc_stack: CfnVpcStack, + efs_ids, + request, + key_name, + cfn_stacks_factory, + efs_mount_target_stack_factory, + access_point_id=None, ): """Write file stack contains an instance to write an empty file with random name into each of the efs in efs_ids.""" write_file_template = Template() diff --git a/tests/integration-tests/tests/storage/test_efs.py b/tests/integration-tests/tests/storage/test_efs.py index e007a36d6f..1e9ab8cd25 100644 --- a/tests/integration-tests/tests/storage/test_efs.py +++ b/tests/integration-tests/tests/storage/test_efs.py @@ -200,6 +200,7 @@ def test_multiple_efs( encryption_in_transits, ) + @pytest.mark.usefixtures("instance") def test_efs_access_point( os, @@ -251,8 +252,8 @@ def test_efs_access_point( "Resource": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" f"file-system/{efs_filesystem_id}", "Condition": { - "StringNotLike": {"elasticfilesystem:AccessPointArn": - f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:access-point/{access_point_id}" + "StringNotLike": { + "elasticfilesystem:AccessPointArn": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:access-point/{access_point_id}" } }, }, @@ -266,14 +267,16 @@ def test_efs_access_point( "elasticfilesystem:ClientWrite", ], "Resource": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" - f"file-system/{efs_filesystem_id}" - } - ] + f"file-system/{efs_filesystem_id}", + }, + ], } boto3.client("efs").put_file_system_policy(FileSystemId=efs_filesystem_id, Policy=json.dumps(policy)) mount_dir = "efs_mount_dir" - cluster_config = pcluster_config_reader(mount_dir=mount_dir, efs_filesystem_id=efs_filesystem_id, access_point_id=access_point_id) + cluster_config = pcluster_config_reader( + mount_dir=mount_dir, efs_filesystem_id=efs_filesystem_id, access_point_id=access_point_id + ) cluster = clusters_factory(cluster_config) remote_command_executor = RemoteCommandExecutor(cluster) @@ -283,7 +286,6 @@ def test_efs_access_point( _test_efs_correctly_shared(remote_command_executor, mount_dir, scheduler_commands) - def _check_efs_after_nodes_reboot( all_mount_dirs, cluster, From 05724ae048bed69e0f7f37ff14b87deb6bd0f039 Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 20:08:19 +0000 Subject: [PATCH 07/11] Updated snapshot to included AccessPointId --- .../slurm.full_config.snapshot.yaml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/cli/tests/pcluster/templates/test_cluster_stack/test_cluster_config_limits/slurm.full_config.snapshot.yaml b/cli/tests/pcluster/templates/test_cluster_stack/test_cluster_config_limits/slurm.full_config.snapshot.yaml index a3e3ae6742..91a7b59934 100644 --- a/cli/tests/pcluster/templates/test_cluster_stack/test_cluster_config_limits/slurm.full_config.snapshot.yaml +++ b/cli/tests/pcluster/templates/test_cluster_stack/test_cluster_config_limits/slurm.full_config.snapshot.yaml @@ -3888,6 +3888,7 @@ SharedStorage: Name: nameebs4 StorageType: Ebs - EfsSettings: + AccessPointId: null DeletionPolicy: Delete Encrypted: false EncryptionInTransit: false @@ -3901,6 +3902,7 @@ SharedStorage: Name: efs0 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3914,6 +3916,7 @@ SharedStorage: Name: existing-efs10 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3927,6 +3930,7 @@ SharedStorage: Name: existing-efs11 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3940,6 +3944,7 @@ SharedStorage: Name: existing-efs12 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3953,6 +3958,7 @@ SharedStorage: Name: existing-efs13 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3966,6 +3972,7 @@ SharedStorage: Name: existing-efs14 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3979,6 +3986,7 @@ SharedStorage: Name: existing-efs15 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -3992,6 +4000,7 @@ SharedStorage: Name: existing-efs16 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4005,6 +4014,7 @@ SharedStorage: Name: existing-efs17 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4018,6 +4028,7 @@ SharedStorage: Name: existing-efs18 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4031,6 +4042,7 @@ SharedStorage: Name: existing-efs19 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4044,6 +4056,7 @@ SharedStorage: Name: existing-efs20 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4057,6 +4070,7 @@ SharedStorage: Name: existing-efs21 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4070,6 +4084,7 @@ SharedStorage: Name: existing-efs22 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4083,6 +4098,7 @@ SharedStorage: Name: existing-efs23 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4096,6 +4112,7 @@ SharedStorage: Name: existing-efs24 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4109,6 +4126,7 @@ SharedStorage: Name: existing-efs25 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4122,6 +4140,7 @@ SharedStorage: Name: existing-efs26 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4135,6 +4154,7 @@ SharedStorage: Name: existing-efs27 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false @@ -4148,6 +4168,7 @@ SharedStorage: Name: existing-efs28 StorageType: Efs - EfsSettings: + AccessPointId: null DeletionPolicy: null Encrypted: false EncryptionInTransit: false From f1633826f41252a84a2737d6c5b52de1b8129bdd Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 20:18:53 +0000 Subject: [PATCH 08/11] code linter fixes --- cli/src/pcluster/config/cluster_config.py | 2 +- cli/tests/pcluster/validators/test_efs_validators.py | 2 +- tests/integration-tests/conftest.py | 3 ++- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cli/src/pcluster/config/cluster_config.py b/cli/src/pcluster/config/cluster_config.py index d2aa5a2c36..42c7cfc16f 100644 --- a/cli/src/pcluster/config/cluster_config.py +++ b/cli/src/pcluster/config/cluster_config.py @@ -149,7 +149,7 @@ PlacementGroupCapacityTypeValidator, PlacementGroupNamingValidator, ) -from pcluster.validators.efs_validators import EfsMountOptionsValidator, EfsAccessPointOptionsValidator +from pcluster.validators.efs_validators import EfsAccessPointOptionsValidator, EfsMountOptionsValidator from pcluster.validators.feature_validators import FeatureRegionValidator from pcluster.validators.fsx_validators import ( FsxAutoImportValidator, diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index 8f2d4f6022..7715422b9e 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -11,7 +11,7 @@ import pytest -from pcluster.validators.efs_validators import EfsMountOptionsValidator, EfsAccessPointOptionsValidator +from pcluster.validators.efs_validators import EfsAccessPointOptionsValidator, EfsMountOptionsValidator from tests.pcluster.validators.utils import assert_failure_messages diff --git a/tests/integration-tests/conftest.py b/tests/integration-tests/conftest.py index f2f34bd707..dbcea062b4 100644 --- a/tests/integration-tests/conftest.py +++ b/tests/integration-tests/conftest.py @@ -54,7 +54,8 @@ from jinja2.sandbox import SandboxedEnvironment from troposphere import Ref, Sub, Template, ec2, resourcegroups from troposphere.ec2 import PlacementGroup -from troposphere.efs import FileSystem as EFSFileSystem, AccessPoint as EFSAccessPoint +from troposphere.efs import AccessPoint as EFSAccessPoint +from troposphere.efs import FileSystem as EFSFileSystem from troposphere.efs import MountTarget from troposphere.fsx import ( ClientConfigurations, From f040d5df23c2a80d26e90536e53714dd18efa651 Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 21:00:13 +0000 Subject: [PATCH 09/11] Fixed Code Linters.. --- cli/src/pcluster/validators/efs_validators.py | 3 ++- cli/tests/pcluster/validators/test_efs_validators.py | 4 +++- tests/integration-tests/tests/storage/storage_common.py | 2 +- tests/integration-tests/tests/storage/test_efs.py | 6 ++---- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/cli/src/pcluster/validators/efs_validators.py b/cli/src/pcluster/validators/efs_validators.py index f9c5d8e05f..4bd93f2adf 100644 --- a/cli/src/pcluster/validators/efs_validators.py +++ b/cli/src/pcluster/validators/efs_validators.py @@ -39,7 +39,8 @@ def _validate(self, access_point_id: str, file_system_id: str, encryption_in_tra if access_point_id and not file_system_id: self._add_failure( "An access point can only be specified when using an existing EFS file system. " - f"Please either remove the access point id {access_point_id} or provide the file system id for the access point", + f"Please either remove the access point id {access_point_id} " + "or provide the file system id for the access point", FailureLevel.ERROR, ) diff --git a/cli/tests/pcluster/validators/test_efs_validators.py b/cli/tests/pcluster/validators/test_efs_validators.py index 7715422b9e..327a03b966 100644 --- a/cli/tests/pcluster/validators/test_efs_validators.py +++ b/cli/tests/pcluster/validators/test_efs_validators.py @@ -107,7 +107,9 @@ def test_efs_access_point_with_filesystem_validator(access_point_id, file_system ), ], ) -def test_efs_access_point_with_filesystem_validator(access_point_id, encryption_in_transit, expected_message): +def test_efs_access_point_with_filesystem_encryption_validator( + access_point_id, encryption_in_transit, expected_message +): actual_failures = EfsAccessPointOptionsValidator().execute( access_point_id, "", encryption_in_transit ) diff --git a/tests/integration-tests/tests/storage/storage_common.py b/tests/integration-tests/tests/storage/storage_common.py index 1b067ff74c..8386ae77da 100644 --- a/tests/integration-tests/tests/storage/storage_common.py +++ b/tests/integration-tests/tests/storage/storage_common.py @@ -324,7 +324,7 @@ def _write_user_data(efs_id, random_file_name, access_point_id=None): access_point_mount_parameter = f",accesspoint={access_point_id}" if access_point_id is not None else "" return f""" - mkdir -p {mount_dir} - - mount -t efs -o tls,iam{access_point_mount_parameter} {efs_id}:/ {mount_dir} + - mount -t efs -o tls,iam{access_point_mount_parameter} {efs_id}:/ {mount_dir} - touch {mount_dir}/{random_file_name} - umount {mount_dir} """ # noqa: E501 diff --git a/tests/integration-tests/tests/storage/test_efs.py b/tests/integration-tests/tests/storage/test_efs.py index 1e9ab8cd25..0ced3ba676 100644 --- a/tests/integration-tests/tests/storage/test_efs.py +++ b/tests/integration-tests/tests/storage/test_efs.py @@ -223,9 +223,6 @@ def test_efs_access_point( To verify the efs is the existing efs, the test expects a file with random ran inside the efs mounted """ # Names of files that will be written from separate instance. The test checks the cluster nodes can access them. - efs_filenames = [] - iam_authorizations = [False, False, True] if scheduler != "awsbatch" else 3 * [False] - encryption_in_transits = [False, True, True] if scheduler != "awsbatch" else 3 * [False] # create an additional EFS with file system policy to prevent anonymous access efs_filesystem_id = efs_stack_factory()[0] efs_mount_target_stack_factory([efs_filesystem_id]) @@ -253,7 +250,8 @@ def test_efs_access_point( f"file-system/{efs_filesystem_id}", "Condition": { "StringNotLike": { - "elasticfilesystem:AccessPointArn": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:access-point/{access_point_id}" + "elasticfilesystem:AccessPointArn": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" + f"access-point/{access_point_id}" } }, }, From 20c8b1fc33a2a6f9118c879787204f2042d672ee Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 21:12:13 +0000 Subject: [PATCH 10/11] Ignoring one formatter as it is in conflict with another... --- tests/integration-tests/tests/storage/test_efs.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/integration-tests/tests/storage/test_efs.py b/tests/integration-tests/tests/storage/test_efs.py index 0ced3ba676..89e1678da6 100644 --- a/tests/integration-tests/tests/storage/test_efs.py +++ b/tests/integration-tests/tests/storage/test_efs.py @@ -250,7 +250,8 @@ def test_efs_access_point( f"file-system/{efs_filesystem_id}", "Condition": { "StringNotLike": { - "elasticfilesystem:AccessPointArn": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" + "elasticfilesystem:AccessPointArn": + f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" f"access-point/{access_point_id}" } }, From 32326ed4ec366ff7848e8543e66f7b8d73793b6e Mon Sep 17 00:00:00 2001 From: Matt Pawelczyk Date: Tue, 13 Aug 2024 21:27:21 +0000 Subject: [PATCH 11/11] Ignored Flake8 too-long-line --- tests/integration-tests/tests/storage/test_efs.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/integration-tests/tests/storage/test_efs.py b/tests/integration-tests/tests/storage/test_efs.py index 89e1678da6..5cf594c19d 100644 --- a/tests/integration-tests/tests/storage/test_efs.py +++ b/tests/integration-tests/tests/storage/test_efs.py @@ -250,8 +250,7 @@ def test_efs_access_point( f"file-system/{efs_filesystem_id}", "Condition": { "StringNotLike": { - "elasticfilesystem:AccessPointArn": - f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" + "elasticfilesystem:AccessPointArn": f"arn:{get_arn_partition(region)}:elasticfilesystem:{region}:{account_id}:" # noqa: E501 f"access-point/{access_point_id}" } },