Add validation to block updating order of tags during cluster update (#7134)

hgreebe · web-flow · commit e14d2b68b0e0 · 2025-12-04T21:22:34.000Z
* Add validation that does not allow for cluster update if tag order has been modified

* Update CHANGELOG

* Add update policy for tag order changes

* Fix tox
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,9 @@ CHANGELOG
 - Upgrade jmespath to ~=1.0 (from ~=0.10).
 - Upgrade tabulate to <=0.9.0 (from <=0.8.10).
 
+**BUG FIXES**
+- Add validation to block updates that change tag order. Blocking such change prevents update failures.
+
 3.14.1
 ------
 
diff --git a/cli/src/pcluster/config/config_patch.py b/cli/src/pcluster/config/config_patch.py
@@ -16,6 +16,7 @@
 from typing import List
 
 from pcluster.config.update_policy import UpdatePolicy
+from pcluster.constants import ORDER_SENSITIVE_PARAMETERS
 from pcluster.schemas.cluster_schema import ClusterSchema
 from pcluster.schemas.common_schema import BaseSchema
 
@@ -181,14 +182,48 @@ def _compare_list(self, base_section, target_section, param_path, data_key, fiel
         """
         update_key = field_obj.metadata.get("update_key")
 
+        # Get the lists
+        base_list = base_section.get(data_key, []) if base_section else []
+        target_list = target_section.get(data_key, []) if target_section else []
+
+        # For Tags, check if this is ONLY an order change (same content, different order)
+        if data_key in ORDER_SENSITIVE_PARAMETERS and base_list != target_list:
+            # Check if it's an order-only change
+            try:
+                # We can assume there are no duplicates due to a validator
+                base_set = {frozenset(item.items()) for item in base_list}
+                target_set = {frozenset(item.items()) for item in target_list}
+                is_order_only_change = base_set == target_set
+            except (TypeError, AttributeError):
+                LOGGER.warning("Unable to compare order sensitive parameters.")
+                is_order_only_change = False
+
+            # If it's ONLY an order change, block it here and return
+            if is_order_only_change:
+                self.changes.append(
+                    Change(
+                        param_path,
+                        data_key,
+                        base_list,
+                        target_list,
+                        UpdatePolicy.UNSUPPORTED_ORDER_CHANGE,
+                        is_list=True,
+                    )
+                )
+                return  # Don't process individual items
+            # Otherwise, fall through to item-by-item comparison below
+            # The reason I check if it is an order only change first is because if I just do I order dependent
+            # comparison and fail if there is a difference, then we don't get the item by item comparison in the
+            # change set if the modification was not the order.
+
         # Compare items in the list by searching the right item to compare through update_key value
         # First, compare all sections from target vs base config and mark visited base sections.
-        for target_nested_section in target_section.get(data_key, []):
+        for target_nested_section in target_list:
             update_key_value = target_nested_section.get(update_key)
             base_nested_section = next(
                 (
                     nested_section
-                    for nested_section in base_section.get(data_key, [])
+                    for nested_section in base_list
                     if nested_section.get(update_key) == update_key_value
                 ),
                 None,
diff --git a/cli/src/pcluster/config/update_policy.py b/cli/src/pcluster/config/update_policy.py
@@ -682,6 +682,18 @@ def get_pool_name_from_change_paths(change):
     + ". If you need this change, please consider creating a new cluster instead of updating the existing one.",
 )
 
+UpdatePolicy.UNSUPPORTED_ORDER_CHANGE = UpdatePolicy(
+    name="UNSUPPORTED_ORDER_CHANGE",
+    level=1000,  # Same level as UNSUPPORTED since it should also block updates
+    fail_reason=lambda change, patch: (
+        f"Update actions are not currently supported for the '{change.key}' parameter. "
+        f"The order of {change.key} has changed, but order changes are not supported"
+    ),
+    action_needed=lambda change, patch: (
+        f"Restore the original order of '{change.key}' to match the current configuration."
+    ),
+)
+
 # Block update if cluster has a managed Fsx for Lustre FileSystem, otherwise fallback to QueueUpdateStrategy
 UpdatePolicy.MANAGED_FSX = UpdatePolicy(
     name="MANAGED_FSX",
diff --git a/cli/src/pcluster/constants.py b/cli/src/pcluster/constants.py
@@ -354,3 +354,5 @@ class Operation(Enum):
 }
 # Capacity Block states that are considered inactive (cannot check health status)
 CAPACITY_BLOCK_INACTIVE_STATES = ["scheduled", "payment-pending", "assessing", "delayed"]
+
+ORDER_SENSITIVE_PARAMETERS = ["Tags"]
diff --git a/cli/tests/pcluster/config/test_config_patch.py b/cli/tests/pcluster/config/test_config_patch.py
@@ -86,7 +86,12 @@ def _compare_change(source, target):
     def _sorting_func(change):
         if change.is_list:
             if isinstance(change.new_value, dict):
-                return change.new_value["Name"]
+                if "Key" in change.new_value:
+                    return change.new_value["Key"]
+                elif "Name" in change.new_value:
+                    return change.new_value["Name"]
+                else:
+                    return "-"
             else:
                 return "-"
         else:
@@ -1133,3 +1138,114 @@ def test_patch_check_cluster_resource_bucket(
         line = ["{0}".format(element) if isinstance(element, str) else element for element in line]
         assert_that(expected_message_rows).contains(line)
     assert_that(patch_allowed).is_equal_to(not expected_error_row)
+
+
+@pytest.mark.parametrize(
+    "base_tags, target_tags, expected_changes, expected_policy",
+    [
+        pytest.param(
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [{"Key": "test2", "Value": "val2"}, {"Key": "test1", "Value": "val1"}],
+            [
+                Change(
+                    [],
+                    "Tags",
+                    [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+                    [{"Key": "test2", "Value": "val2"}, {"Key": "test1", "Value": "val1"}],
+                    UpdatePolicy.UNSUPPORTED_ORDER_CHANGE,
+                    is_list=True,
+                )
+            ],
+            UpdatePolicy.UNSUPPORTED_ORDER_CHANGE,
+            id="order_only_change",
+        ),
+        pytest.param(
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}, {"Key": "test3", "Value": "val3"}],
+            [
+                Change(
+                    [],
+                    "Tags",
+                    None,
+                    {"Key": "test3", "Value": "val3"},
+                    UpdatePolicy.UNSUPPORTED,
+                    is_list=True,
+                )
+            ],
+            UpdatePolicy.UNSUPPORTED,
+            id="tag_addition",
+        ),
+        pytest.param(
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [{"Key": "test1", "Value": "val1"}],
+            [
+                Change(
+                    [],
+                    "Tags",
+                    {"Key": "test2", "Value": "val2"},
+                    None,
+                    UpdatePolicy.UNSUPPORTED,
+                    is_list=True,
+                )
+            ],
+            UpdatePolicy.UNSUPPORTED,
+            id="tag_removal",
+        ),
+        pytest.param(
+            [{"Key": "test1", "Value": "old_value"}],
+            [{"Key": "test1", "Value": "new_value"}],
+            [
+                Change(
+                    ["Tags[test1]"],
+                    "Value",
+                    "old_value",
+                    "new_value",
+                    UpdatePolicy.UNSUPPORTED,
+                    is_list=False,
+                )
+            ],
+            UpdatePolicy.UNSUPPORTED,
+            id="tag_value_modification",
+        ),
+        pytest.param(
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [{"Key": "test3", "Value": "val3"}, {"Key": "test2", "Value": "val2"}, {"Key": "test1", "Value": "val1"}],
+            [
+                Change(
+                    [],
+                    "Tags",
+                    None,
+                    {"Key": "test3", "Value": "val3"},
+                    UpdatePolicy.UNSUPPORTED,
+                    is_list=True,
+                )
+            ],
+            UpdatePolicy.UNSUPPORTED,
+            id="order_change_plus_addition",
+        ),
+        pytest.param(
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [{"Key": "test1", "Value": "val1"}, {"Key": "test2", "Value": "val2"}],
+            [],
+            UpdatePolicy.SUPPORTED,
+            id="no_change_identical_tags",
+        ),
+    ],
+)
+def test_tag_updates(
+    mocker, test_datadir, pcluster_config_reader, base_tags, target_tags, expected_changes, expected_policy
+):
+    """Test various tag update scenarios including order changes, additions, and modifications."""
+    mock_aws_api(mocker)
+    dst_config_file = "pcluster.config.dst.yaml"
+    _duplicate_config_file(dst_config_file, test_datadir)
+
+    src_dict = {"tags": base_tags}
+    src_config_file = pcluster_config_reader(**src_dict)
+    src_conf = _load_config(src_config_file)
+
+    dst_dict = {"tags": target_tags}
+    dst_config_file = pcluster_config_reader(dst_config_file, **dst_dict)
+    dst_conf = _load_config(dst_config_file)
+
+    _check_patch(src_conf.source_config, dst_conf.source_config, expected_changes, expected_policy)
diff --git a/cli/tests/pcluster/config/test_config_patch/test_tag_updates/pcluster.config.yaml b/cli/tests/pcluster/config/test_config_patch/test_tag_updates/pcluster.config.yaml
@@ -0,0 +1,28 @@
+Image:
+  Os: alinux2
+HeadNode:
+  InstanceType: t3.micro
+  Networking:
+    SubnetId: subnet-12345678
+  Ssh:
+    KeyName: test-key
+Scheduling:
+  Scheduler: slurm
+  SlurmQueues:
+    - Name: queue1
+      Networking:
+        SubnetIds:
+          - subnet-12345678
+      ComputeResources:
+        - Name: compute-resource1
+          InstanceType: t3.micro
+          MinCount: 1
+          MaxCount: 10
+  {% if tags %}
+Tags:
+  {% for tag in tags %}
+- Key: {{ tag.Key }}
+  Value: {{ tag.Value }}
+  {% endfor %}
+  {% endif %}
+

Original file line number	Diff line number	Diff line change
`@@ -354,3 +354,5 @@ class Operation(Enum):`
`354`	`354`	`}`
`355`	`355`	`# Capacity Block states that are considered inactive (cannot check health status)`
`356`	`356`	`CAPACITY_BLOCK_INACTIVE_STATES = ["scheduled", "payment-pending", "assessing", "delayed"]`
	`357`	`+`
	`358`	`+ORDER_SENSITIVE_PARAMETERS = ["Tags"]`