Skip to content

Commit ca1e0be

Browse files
gmarcianigmarciani
committed
[Validators] Add validator ExtraChefAttributesValidator to validate DevSettings/Cookbook/ExtraChefAttributes.
For now, it only validates that: 1. ExtraChefAttributes is a valid JSON. 2. the attribute `cluster.cfnhup_on_fleet_enabled` is a boolean value. 3. if the attribute `cluster.cfnhup_on_fleet_enabled` is false, returns a warning to notify the user that disabling cfn-hup has consequences on the cluster update behavior.
1 parent 8e49afd commit ca1e0be

File tree

8 files changed

+276
-0
lines changed

8 files changed

+276
-0
lines changed

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ CHANGELOG
44
3.15.0
55
------
66

7+
**CHANGES**
8+
- Add validator that warns against the downsides of disabling cfn-hup on compute and login nodes.
9+
710
**BUG FIXES**
811
- Reduce EFA installation time for Ubuntu by ~20 minutes by only holding kernel packages for the installed kernel.
912

‎cli/src/pcluster/config/common.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
from typing import List, Set
2222

2323
from pcluster.validators.common import AsyncValidator, FailureLevel, ValidationResult, Validator, ValidatorContext
24+
from pcluster.validators.dev_settings_validators import ExtraChefAttributesValidator
2425
from pcluster.validators.iam_validators import AdditionalIamPolicyValidator
2526
from pcluster.validators.networking_validators import LambdaFunctionsVpcConfigValidator
2627
from pcluster.validators.s3_validators import UrlValidator
@@ -333,6 +334,8 @@ def __init__(self, chef_cookbook: str = None, extra_chef_attributes: str = None)
333334
def _register_validators(self, context: ValidatorContext = None):
334335
if self.chef_cookbook is not None:
335336
self._register_validator(UrlValidator, url=self.chef_cookbook)
337+
if self.extra_chef_attributes:
338+
self._register_validator(ExtraChefAttributesValidator, extra_chef_attributes=self.extra_chef_attributes)
336339

337340

338341
class LambdaFunctionsVpcConfig(Resource):

‎cli/src/pcluster/validators/dev_settings_validators.py‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import json
12+
13+
from pcluster.validators.common import FailureLevel, Validator
14+
from pcluster.validators.utils import dig, is_boolean_string, is_valid_json, str_to_bool
15+
16+
EXTRA_CHEF_ATTRIBUTES_PATH = "DevSettings/Cookbook/ExtraChefAttributes"
17+
ATTR_CFN_HUP_ON_FLEET_ENABLED = "cfnhup_on_fleet_enabled"
18+
19+
20+
class ExtraChefAttributesValidator(Validator):
21+
"""Validator for DevSettings/Cookbook/ExtraChefAttributes."""
22+
23+
def _validate(self, extra_chef_attributes: str = None):
24+
"""Validate DevSettings/Cookbook/ExtraChefAttributes."""
25+
if not extra_chef_attributes:
26+
return
27+
elif not is_valid_json(extra_chef_attributes):
28+
self._add_failure(
29+
f"Invalid value in {EXTRA_CHEF_ATTRIBUTES_PATH}: must be a JSON.",
30+
FailureLevel.ERROR,
31+
)
32+
else:
33+
self._validate_cfnup_on_fleet_enabled(json.loads(extra_chef_attributes))
34+
35+
def _validate_cfnup_on_fleet_enabled(self, extra_chef_attributes: dict = None):
36+
cfnhup_on_fleet_enabled = dig(extra_chef_attributes, "cluster", ATTR_CFN_HUP_ON_FLEET_ENABLED)
37+
38+
if cfnhup_on_fleet_enabled is None:
39+
return
40+
41+
if not is_boolean_string(str(cfnhup_on_fleet_enabled)):
42+
self._add_failure(
43+
f"Invalid value in {EXTRA_CHEF_ATTRIBUTES_PATH}: "
44+
f"attribute '{ATTR_CFN_HUP_ON_FLEET_ENABLED}' must be a boolean value.",
45+
FailureLevel.ERROR,
46+
)
47+
return
48+
49+
if str_to_bool(str(cfnhup_on_fleet_enabled)) is False:
50+
self._add_failure(
51+
"Disabling cfn-hup on compute and login nodes removes the possibility "
52+
"to execute in-place cluster updates on compute and login nodes.",
53+
FailureLevel.WARNING,
54+
)

‎cli/src/pcluster/validators/utils.py‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,38 @@
1212
# This module contains all the classes representing the Resources objects.
1313
# These objects are obtained from the configuration file through a conversion based on the Schema classes.
1414
#
15+
import json
16+
from typing import Any
17+
18+
BOOLEAN_VALUES = ["true", "false"]
1519

1620

1721
def get_bucket_name_from_s3_url(import_path):
1822
return import_path.split("/")[2]
23+
24+
25+
def str_to_bool(string: str = None) -> bool:
26+
return str(string).lower() == "true"
27+
28+
29+
def is_boolean_string(value: str) -> bool:
30+
return str(value).lower() in BOOLEAN_VALUES
31+
32+
33+
def is_valid_json(string: str = None) -> bool:
34+
try:
35+
json.loads(string)
36+
return True
37+
except (ValueError, TypeError):
38+
return False
39+
40+
41+
def dig(dictionary: dict, *keys: str) -> dict | None | Any:
42+
if dictionary is None:
43+
return None
44+
value = dictionary
45+
for key in keys:
46+
if value is None or not isinstance(value, dict):
47+
return None
48+
value = value.get(key)
49+
return value

‎cli/tests/pcluster/validators/test_all_validators.py‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
from pcluster.validators import (
2020
cluster_validators,
2121
database_validators,
22+
dev_settings_validators,
2223
ebs_validators,
2324
ec2_validators,
2425
feature_validators,
@@ -67,6 +68,7 @@ async def _validate_async(*args, **kwargs):
6768
modules = [
6869
cluster_validators,
6970
database_validators,
71+
dev_settings_validators,
7072
ebs_validators,
7173
ec2_validators,
7274
fsx_validators,
@@ -282,6 +284,10 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker)
282284
compute_resource_tags_validator = mocker.patch(
283285
tags_validators + ".ComputeResourceTagsValidator._validate", return_value=[]
284286
)
287+
dev_settings_validators = validators_path + ".dev_settings_validators"
288+
extra_chef_attributes_validator = mocker.patch(
289+
dev_settings_validators + ".ExtraChefAttributesValidator._validate", return_value=[]
290+
)
285291
mocker.patch(
286292
"pcluster.config.cluster_config.HeadNode.architecture", new_callable=PropertyMock(return_value="x86_64")
287293
)
@@ -451,6 +457,8 @@ def test_slurm_validators_are_called_with_correct_argument(test_datadir, mocker)
451457
any_order=True,
452458
)
453459

460+
extra_chef_attributes_validator.assert_has_calls([call(extra_chef_attributes='{"attr1": {"attr2": "value"}}')])
461+
454462
# No assertion on the argument for minor validators
455463
name_validator.assert_called()
456464
feature_region_validator.assert_called()

‎cli/tests/pcluster/validators/test_all_validators/test_slurm_validators_are_called_with_correct_argument/slurm.yaml‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,7 @@ Tags:
101101
Value: String
102102
- Key: cluster_tag2
103103
Value: String
104+
DevSettings:
105+
Cookbook:
106+
ExtraChefAttributes: |
107+
{"attr1": {"attr2": "value"}}

‎cli/tests/pcluster/validators/test_dev_settings_validators.py‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import pytest
12+
13+
from pcluster.validators.common import FailureLevel
14+
from pcluster.validators.dev_settings_validators import ExtraChefAttributesValidator
15+
from tests.pcluster.validators.utils import assert_failure_level, assert_failure_messages
16+
17+
18+
@pytest.mark.parametrize(
19+
"extra_chef_attributes, expected_message, expected_failure_level",
20+
[
21+
# cfn-hup enabled, implicitly
22+
(None, None, None),
23+
('{"other_attribute": "value"}', None, None),
24+
# cfn-hup enabled, explicitly
25+
('{"cluster": {"cfnhup_on_fleet_enabled": "true"}}', None, None),
26+
('{"cluster": {"cfnhup_on_fleet_enabled": true}}', None, None),
27+
# cfn-hup disabled
28+
(
29+
'{"cluster": {"cfnhup_on_fleet_enabled": "false"}}',
30+
"Disabling cfn-hup on compute and login nodes removes the possibility "
31+
"to execute in-place cluster updates on compute and login nodes.",
32+
FailureLevel.WARNING,
33+
),
34+
(
35+
'{"cluster": {"cfnhup_on_fleet_enabled": false}}',
36+
"Disabling cfn-hup on compute and login nodes removes the possibility "
37+
"to execute in-place cluster updates on compute and login nodes.",
38+
FailureLevel.WARNING,
39+
),
40+
# cfnhup_on_fleet_enabled: invalid values
41+
(
42+
'{"cluster": {"cfnhup_on_fleet_enabled": "invalid"}}',
43+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: "
44+
"attribute 'cfnhup_on_fleet_enabled' must be a boolean value.",
45+
FailureLevel.ERROR,
46+
),
47+
(
48+
'{"cluster": {"cfnhup_on_fleet_enabled": 123}}',
49+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: "
50+
"attribute 'cfnhup_on_fleet_enabled' must be a boolean value.",
51+
FailureLevel.ERROR,
52+
),
53+
# ExtraChefAttributes: invalid values
54+
(
55+
"invalid json",
56+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: must be a JSON.",
57+
FailureLevel.ERROR,
58+
),
59+
(
60+
"{invalid: json}",
61+
"Invalid value in DevSettings/Cookbook/ExtraChefAttributes: must be a JSON.",
62+
FailureLevel.ERROR,
63+
),
64+
],
65+
)
66+
def test_extra_chef_attributes_validator(extra_chef_attributes, expected_message, expected_failure_level):
67+
actual_failures = ExtraChefAttributesValidator().execute(extra_chef_attributes=extra_chef_attributes)
68+
assert_failure_messages(actual_failures, expected_message)
69+
if expected_failure_level:
70+
assert_failure_level(actual_failures, expected_failure_level)

‎cli/tests/pcluster/validators/test_utils.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,103 @@
1+
# Copyright 2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License"). You may not use this file except in compliance
4+
# with the License. A copy of the License is located at
5+
#
6+
# http://aws.amazon.com/apache2.0/
7+
#
8+
# or in the "LICENSE.txt" file accompanying this file. This file is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES
9+
# OR CONDITIONS OF ANY KIND, express or implied. See the License for the specific language governing permissions and
10+
# limitations under the License.
11+
import pytest
12+
13+
from pcluster.validators.utils import dig, is_boolean_string, is_valid_json, str_to_bool
14+
15+
16+
@pytest.mark.parametrize(
17+
"string_value, expected_result",
18+
[
19+
("true", True),
20+
("True", True),
21+
("TRUE", True),
22+
("false", False),
23+
("False", False),
24+
("FALSE", False),
25+
("yes", False),
26+
("no", False),
27+
("1", False),
28+
("0", False),
29+
(None, False),
30+
("", False),
31+
],
32+
)
33+
def test_str_to_bool(string_value, expected_result):
34+
assert str_to_bool(string_value) == expected_result
35+
36+
37+
@pytest.mark.parametrize(
38+
"value, expected_result",
39+
[
40+
("true", True),
41+
("True", True),
42+
("TRUE", True),
43+
("false", True),
44+
("False", True),
45+
("FALSE", True),
46+
(True, True),
47+
(False, True),
48+
(None, False),
49+
("", False),
50+
("yes", False),
51+
("no", False),
52+
("1", False),
53+
("0", False),
54+
(1, False),
55+
(0, False),
56+
],
57+
)
58+
def test_is_boolean_string(value, expected_result):
59+
assert is_boolean_string(value) == expected_result
60+
61+
62+
@pytest.mark.parametrize(
63+
"json_string, expected_result",
64+
[
65+
('{"key": "value"}', True),
66+
('{"nested": {"key": "value"}}', True),
67+
("[]", True),
68+
('[{"key": "value"}]', True),
69+
("null", True),
70+
("true", True),
71+
("false", True),
72+
('"string"', True),
73+
("123", True),
74+
("invalid json", False),
75+
("{invalid: json}", False),
76+
('{"unclosed": "json"', False),
77+
(None, False),
78+
("", False),
79+
],
80+
)
81+
def test_is_valid_json(json_string, expected_result):
82+
assert is_valid_json(json_string) == expected_result
83+
84+
85+
@pytest.mark.parametrize(
86+
"dictionary, keys, expected_result",
87+
[
88+
({"a": {"b": {"c": "value"}}}, ("a", "b", "c"), "value"),
89+
({"a": {"b": "value"}}, ("a", "b"), "value"),
90+
({"a": "value"}, ("a",), "value"),
91+
({"a": {"b": {"c": "value"}}}, ("a", "b"), {"c": "value"}),
92+
({"a": {"b": {"c": "value"}}}, ("a", "nonexistent"), None),
93+
({"a": {"b": {"c": "value"}}}, ("nonexistent",), None),
94+
({"a": {"b": {"c": "value"}}}, ("a", "b", "c", "d"), None),
95+
({}, ("a",), None),
96+
(None, ("a",), None),
97+
({"a": None}, ("a", "b"), None),
98+
({"a": "not_dict"}, ("a", "b"), None),
99+
({"a": {"b": None}}, ("a", "b", "c"), None),
100+
],
101+
)
102+
def test_dig(dictionary, keys, expected_result):
103+
assert dig(dictionary, *keys) == expected_result

0 commit comments

Comments
 (0)