[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914

Allenz5 · 2025-07-14T18:43:58Z

Description of changes

Add a SharedStorageEfsSettings section under the HeadNode section to include all settings related to Efs shared storage
Add an Encrypted parameter under the SharedStorageEfsSettings section to allow users to specify encryption for EFS shared storage
Add a SharedStorageEfsSettingsValidator to verify that SharedStorageEfsSettings should only be used when SharedStorageType is Efs

Tests

Add test_shared_storage_efs_settings_validator to test the behavior of SharedStorageEfsSettingsValidator
[TODO] Update the integration tests test_internal_efs and test_shared_home to verify EFS encryption

References

Checklist

Make sure you are pointing to the right branch.
If you're creating a patch for a branch other than develop add the branch name as prefix in the PR title (e.g. [release-3.6]).
Check all commits' messages are clear, describing what and why vs how.
Make sure to have added unit tests or integration tests to cover the new/modified code.
Check if documentation is impacted by this change.

Please review the guidelines for contributing and Pull Request Instructions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…into efs-encryption

… check that SharedStorageEfsSettings can only be used when SharedStorageType is specified as Efs

…Validator's behavior and registration

…into efs-encryption

…er into efs-encryption

cli/src/pcluster/config/cluster_config.py

cli/src/pcluster/schemas/cluster_schema.py

cli/src/pcluster/validators/cluster_validators.py

cli/tests/pcluster/validators/test_cluster_validators.py

himani2411 · 2025-07-14T19:46:50Z

Please mention that Integ tests that you will modify is ONGOING

…ings_validator`

…d `SharedStorageEfsSettingsSchema` class to make them clearer.

…into efs-encryption

…o common.py.

…validator and unit test

…er into efs-encryption

…y from _add_resources to decrease complexity

…ster_config_limits unit test

himani2411 · 2025-07-15T21:49:30Z

LGTM! As discussed will wait for the Integ tests changes before we merge the PR

gmarciani · 2025-07-15T21:57:42Z

cli/src/pcluster/templates/awsbatch_builder.py

@@ -22,7 +22,8 @@
 from aws_cdk.aws_ec2 import CfnSecurityGroup
 from aws_cdk.core import CfnOutput, CfnResource, Construct, Fn, Stack

-from pcluster.config.cluster_config import AwsBatchClusterConfig, CapacityType, SharedStorageType
+from pcluster.config.cluster_config import AwsBatchClusterConfig, CapacityType
+from pcluster.config.common import SharedStorageType


Here and in other part of the PR.
This change is motivated by a refactoring, unrelated to the main goal of this PR.
A best practice is to keep the PR minimal and avoid refactoring on components that are out of scope for the main goal. The refactoring you're suggesting here is correct, but I suggest to do it in a follow up PR.

gmarciani · 2025-07-15T22:03:39Z

cli/src/pcluster/templates/cluster_stack.py

+        if self.config.head_node.shared_storage_efs_settings:
+            encrypted = self.config.head_node.shared_storage_efs_settings.encrypted
+        else:
+            encrypted = False


[Single Responsibility / Maintainability] The resource SharedStorageEfsSettings should be the only responsible to determine the default value of the encryption. To this aim, you should define the default value into a constant SharedStorageEfsSettings .DEFAULT_ENCRYPTION and reference the constant here. In this way we do not risk to scatter default values in different places. This prevent scattering responsibilities in different places, makes the code easier to maintain as it avoids code duplications and risk of misalignments.

gmarciani · 2025-07-15T22:04:43Z

cli/src/pcluster/templates/cluster_stack.py

+        else:
+            encrypted = False
+        internal_efs_storage_shared = SharedEfs(
+            mount_dir="/opt/parallelcluster/init_shared",


Why init_shared and not, for example, shared? I know this was already in the code, but do we have an understanding about it?

gmarciani · 2025-07-15T22:11:58Z

cli/src/pcluster/validators/cluster_validators.py

+    def _validate(self, shared_storage_type: str, shared_storage_efs_settings):
+        if shared_storage_efs_settings and shared_storage_type.lower() != SharedStorageType.EFS.value:
+            self._add_failure(
+                "SharedStorageEfsSettings is specified "


The error message can be simplified to SharedStorageEfsSettings can only be used when SharedStorageType is {SharedStorageType.EFS.value.capitalize()}.

I suggest to use the capitalize because, even if we are able to handle both Efs and efs as values, in our public doc we always use capitalized strings.

gmarciani · 2025-07-15T22:22:22Z

cli/tests/pcluster/validators/test_all_validators.py

@@ -392,6 +395,10 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker)
        ],
        any_order=True,
    )
+    shared_storage_efs_settings_validator.assert_has_calls(
+        [call(shared_storage_type="Ebs", shared_storage_efs_settings=None)],


[Test] This unit test would make mnore sense if we use a configuration that inject both shared_storage_type and shared_storage_efs_settings into the validators, otherwise you could have the doubt that there exists cases where shared_storage_efs_settings is not injected.

My suggestion is to cover two scenarios:

default case (the one you did): shared_storage_type=Ebs and shared_storage_efs_settings is not set

case where shared_storage_efs_settings is injected: shared_storage_type=Efs and shared_storage_efs_settings is set.

gmarciani · 2025-07-15T22:23:39Z

cli/tests/pcluster/validators/test_cluster_validators.py

@@ -2075,6 +2076,50 @@ def test_mixed_security_group_overwrite_validator(head_node_security_groups, que
    assert_failure_messages(actual_failures, expected_message)


+@pytest.mark.parametrize(


[Test] It's a best practice to define test cases with a self-explanatory id. This improves readability and troubleshooting.
See example:

aws-parallelcluster/cli/tests/pcluster/api/controllers/test_cluster_operations_controller.py

Lines 138 to 146 in 4f4c670

pytest.param(

{"clusterName": "cluster", "clusterConfiguration": CONFIG},

[ValidationResult("message", FailureLevel.WARNING, "type")],

None,

None,

"us-east-1",

None,

id="test with all errors",

),

gmarciani · 2025-07-15T22:25:52Z

cli/src/pcluster/templates/cluster_stack.py

+            mount_dir="/opt/parallelcluster/init_shared",
+            name="internal_pcluster_shared",
+            throughput_mode="elastic",
+            encrypted=encrypted,


[Test] We should capture this property in a unit test. the unit test should verify that when encrypted is specified in the cluster config, then the cluster template has the EFS resource with that property set to the expected value.

See example:

aws-parallelcluster/cli/tests/pcluster/templates/test_cluster_stack.py

Lines 217 to 249 in bf22eba

def test_add_alarms(mocker, config_file_name):

mock_aws_api(mocker)

# mock bucket initialization parameters

mock_bucket(mocker)

mock_bucket_object_utils(mocker)

input_yaml, cluster = load_cluster_model_from_yaml(config_file_name)

generated_template, _ = CDKTemplateBuilder().build_cluster_template(

cluster_config=cluster, bucket=dummy_cluster_bucket(), stack_name="clustername"

)

simple_type = "AWS::CloudWatch::Alarm"

composite_type = "AWS::CloudWatch::CompositeAlarm"

head_node_alarms = [

{"name": "clustername-HeadNode", "type": composite_type},

{"name": "clustername-HeadNode-Health", "type": simple_type},

{"name": "clustername-HeadNode-Cpu", "type": simple_type},

{"name": "clustername-HeadNode-Mem", "type": simple_type},

{"name": "clustername-HeadNode-Disk", "type": simple_type},

]

if cluster.are_alarms_enabled:

for alarm in head_node_alarms:

matched_resources = get_resources(

generated_template, type=alarm["type"], properties={"AlarmName": alarm["name"]}

)

assert_that(matched_resources).is_length(1)

else:

matched_simple_alarms = get_resources(generated_template, type=simple_type)

matched_composite_alarms = get_resources(generated_template, type=composite_type)

assert_that(matched_simple_alarms).is_empty()

assert_that(matched_composite_alarms).is_empty()

Allenz5 and others added 9 commits July 7, 2025 14:17

[Efs Encryption] Add HeadNode/SharedStorageSettings/Encrypted

f36a98e

Merge branch 'develop' of https://github.com/aws/aws-parallelcluster …

230d6a8

…into efs-encryption

[Efs Encryption] Update SharedStorageEfsSettingsEncryptedValidator to…

ac5f9fb

… check that SharedStorageEfsSettings can only be used when SharedStorageType is specified as Efs

[Efs Encryption] Add unit tests for SharedStorageEfsSettingsEncrypted…

df0da09

…Validator's behavior and registration

[Efs Encryption] Fix parameter name in cluster_schema.py

a942241

Merge branch 'aws:develop' into efs-encryption

a4ae51e

[Efs Encryption] Fix parameter naming

1931dc8

Merge branch 'develop' of https://github.com/aws/aws-parallelcluster …

63bfa91

…into efs-encryption

Merge branch 'efs-encryption' of github.com:Allenz5/aws-parallelclust…

cbb5415

…er into efs-encryption

Allenz5 requested review from a team as code owners July 14, 2025 18:43

himani2411 reviewed Jul 14, 2025

View reviewed changes

cli/src/pcluster/config/cluster_config.py Outdated Show resolved Hide resolved

himani2411 reviewed Jul 14, 2025

View reviewed changes

cli/src/pcluster/schemas/cluster_schema.py Outdated Show resolved Hide resolved

himani2411 reviewed Jul 14, 2025

View reviewed changes

cli/src/pcluster/validators/cluster_validators.py Outdated Show resolved Hide resolved

himani2411 reviewed Jul 14, 2025

View reviewed changes

cli/tests/pcluster/validators/test_cluster_validators.py Outdated Show resolved Hide resolved

Allenz5 and others added 10 commits July 14, 2025 17:40

[Efs Encryption] Add more test cases in `test_shared_storage_efs_sett…

be5ee63

…ings_validator`

[Efs Encryption] Update the comments of SharedStorageEfsSettings an…

91047d8

…d `SharedStorageEfsSettingsSchema` class to make them clearer.

Merge branch 'aws:develop' into efs-encryption

ea3b32a

Merge branch 'develop' of https://github.com/aws/aws-parallelcluster …

bb4e1e4

…into efs-encryption

[Efs Encryption] Move SharedStorageType Enum from cluster_config.py t…

88b3781

…o common.py.

[Efs Encryption] Change hardcoded ShareStorageType to an enum in the …

7508cfc

…validator and unit test

[Efs Encryption] Fix format issues

944762a

Merge branch 'efs-encryption' of github.com:Allenz5/aws-parallelclust…

b7936e4

…er into efs-encryption

[Efs Encryption] Move the addition of internal EFS shared storage awa…

90b2691

…y from _add_resources to decrease complexity

[Efs Encryption] Add SharedStorageEfsSettings section to the test_clu…

3fc1d57

…ster_config_limits unit test

Allenz5 force-pushed the efs-encryption branch from 6b4cf6e to 3fc1d57 Compare July 15, 2025 19:47

Allenz5 and others added 2 commits July 15, 2025 15:56

[Efs Encryption] Fix SharedStorageEfsSettingsValidator type issue

5ba7bbd

Merge branch 'develop' into efs-encryption

c27abc0

gmarciani reviewed Jul 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914

[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914

Allenz5 commented Jul 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

himani2411 commented Jul 14, 2025

Uh oh!

himani2411 commented Jul 15, 2025 •

edited

Loading

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

gmarciani Jul 15, 2025

Uh oh!

Uh oh!

		@@ -2075,6 +2076,50 @@ def test_mixed_security_group_overwrite_validator(head_node_security_groups, que
		assert_failure_messages(actual_failures, expected_message)


		@pytest.mark.parametrize(

	pytest.param(
	{"clusterName": "cluster", "clusterConfiguration": CONFIG},
	[ValidationResult("message", FailureLevel.WARNING, "type")],
	None,
	None,
	"us-east-1",
	None,
	id="test with all errors",
	),

	def test_add_alarms(mocker, config_file_name):
	mock_aws_api(mocker)
	# mock bucket initialization parameters
	mock_bucket(mocker)
	mock_bucket_object_utils(mocker)

	input_yaml, cluster = load_cluster_model_from_yaml(config_file_name)
	generated_template, _ = CDKTemplateBuilder().build_cluster_template(
	cluster_config=cluster, bucket=dummy_cluster_bucket(), stack_name="clustername"
	)

	simple_type = "AWS::CloudWatch::Alarm"
	composite_type = "AWS::CloudWatch::CompositeAlarm"

	head_node_alarms = [
	{"name": "clustername-HeadNode", "type": composite_type},
	{"name": "clustername-HeadNode-Health", "type": simple_type},
	{"name": "clustername-HeadNode-Cpu", "type": simple_type},
	{"name": "clustername-HeadNode-Mem", "type": simple_type},
	{"name": "clustername-HeadNode-Disk", "type": simple_type},
	]

	if cluster.are_alarms_enabled:
	for alarm in head_node_alarms:
	matched_resources = get_resources(
	generated_template, type=alarm["type"], properties={"AlarmName": alarm["name"]}
	)
	assert_that(matched_resources).is_length(1)
	else:
	matched_simple_alarms = get_resources(generated_template, type=simple_type)
	matched_composite_alarms = get_resources(generated_template, type=composite_type)
	assert_that(matched_simple_alarms).is_empty()
	assert_that(matched_composite_alarms).is_empty()

[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914

Are you sure you want to change the base?

[Efs encryption][Draft] Support HeadNode Efs SharedStorage Encryption #6914

Conversation

Allenz5 commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Tests

References

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

himani2411 commented Jul 14, 2025

Uh oh!

himani2411 commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

gmarciani Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Allenz5 commented Jul 14, 2025 •

edited

Loading

himani2411 commented Jul 15, 2025 •

edited

Loading