Add EFS access point support (aws-parallelcluster#2337)#6359
Add EFS access point support (aws-parallelcluster#2337)#6359thheinen wants to merge 2 commits intoaws:developfrom
Conversation
Signed-off-by: Thomas Heinen <t.heinen@reply.de>
| 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) |
There was a problem hiding this comment.
Here and in other files, we are assuming that more than one access point can be specified for a given file system (AccesspointsIds rather than AccesspointId).
It's true that a file system can support multiple access points (https://docs.aws.amazon.com/efs/latest/ug/efs-access-points.html#efs-access-poiont-create), but the mount action is always done using a single access point (https://docs.aws.amazon.com/efs/latest/ug/mounting-access-points.html).
So please, let's assume to specify only one access point on the cluster config.
There was a problem hiding this comment.
I think this is actually a per filesystem array, similar to the encryption in transit array
There was a problem hiding this comment.
Yes, it's actually an array of all the separate FS in config. I just aligned naming and structure to the other properties which are also named in plural.
| """ | ||
|
|
||
| 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): |
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/validators/test_efs_validators.py
| self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], | ||
| use_lower_case=True, | ||
| ), | ||
| "efs_accesspoint_ids": to_comma_separated_string( |
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_queues_stack.py#L114
| self._shared_storage_attributes[SharedStorageType.EFS]["IamAuthorizations"], | ||
| use_lower_case=True, | ||
| ), | ||
| "efs_accesspoint_ids": to_comma_separated_string( |
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_login_nodes_stack.py#L23
| "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( |
There was a problem hiding this comment.
This change should be captured in unit test here: https://github.com/aws/aws-parallelcluster/blob/develop/cli/tests/pcluster/templates/test_cluster_stack.py#L901
| """ | ||
|
|
||
| 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): |
There was a problem hiding this comment.
The validator should verify that the provided access point exists
|
Hi @thheinen, Made a first round of review and unblocked PR checks. |
Signed-off-by: Thomas Heinen <t.heinen@reply.de>
3 participants
Description of changes
Add support to specify EFS access points per issue #2337
Tests
Tested mounting EFS with and without accesspoints from an aws-parallelcluster cluster.
More tests to be done but opening a draft PR for discussion and early feedback
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.