ansible-0001: bench/patch update to reflect real fix with Role.__hash__
Bench bench-ansible-0001.py rewritten to model the real defect: Role-like objects with __eq__ but no __hash__, and to demonstrate that the naive list->set swap raises TypeError. bench_fixed uses a MockRole with both __eq__ and __hash__. Four scales D=100..2000, min of 3 trials per scale. Patch doc ansible-0001-role-get-vars-seen-list.md now describes the coupled change (add __hash__, then switch seen list to set), references the upstream PR branch and the integration target roles_var_inheritance. Added ansible-0001-role-get-vars-seen-list.patch (git-format-patch export from the upstream commit) with mandatory complexity-gate comment block.
This commit is contained in:
parent
99be1fbed9
commit
dbd8849a08
4 changed files with 268 additions and 48 deletions
|
|
@ -1,5 +1,5 @@
|
|||
# UNDF: UNDF-2026-000000005
|
||||
# ansible-0001: Role.get_vars() seen-list O(D²) deduplication
|
||||
# ansible-0001: Role.get_vars() seen-list O(D^2) deduplication
|
||||
|
||||
## Classification
|
||||
- **Severity**: MEDIUM
|
||||
|
|
@ -19,17 +19,42 @@ for dep in self.get_all_dependencies():
|
|||
```
|
||||
|
||||
## Pattern
|
||||
`seen` is initialized as a Python `list`. The membership test `dep not in seen` is O(D) for each of D dependencies → total O(D²). In a large Ansible playbook with deeply nested roles (e.g. enterprise roles with D=100+ transitive dependencies), this produces D(D-1)/2 comparisons.
|
||||
`seen` initializes as a Python `list`. Membership test `dep not in seen` runs
|
||||
O(D) for each of D dependencies → total O(D^2). A large Ansible playbook with
|
||||
deeply nested roles (D=100+ transitive dependencies in enterprise playbooks)
|
||||
produces D*(D-1)/2 comparisons per `get_vars()` call.
|
||||
|
||||
## Speedup
|
||||
At D=200 dependencies: 19,900 comparisons → 200 comparisons (99.5x reduction)
|
||||
At D=200 dependencies: 19,900 comparisons collapse to 200 (99.5x reduction).
|
||||
Measured bench (`bench/bench-ansible-0001.py`, `MockRole` with `__eq__` +
|
||||
`__hash__`): 9x–270x across D=100..2000.
|
||||
|
||||
## Naive fix fails
|
||||
Swapping `seen = []` for `seen = set()` alone raises `TypeError: unhashable
|
||||
type: 'Role'`. `Role` defines `__eq__` at `lib/ansible/playbook/role/__init__.py:202`
|
||||
(value equality over `_get_hash_dict()`) but no `__hash__`; Python then sets
|
||||
`__hash__ = None` implicitly, making instances unhashable.
|
||||
|
||||
The fix couples two changes: add a `__hash__` method on `Role` consistent with
|
||||
the existing `__eq__`, then swap the list dedup for a set.
|
||||
|
||||
## Patch
|
||||
|
||||
```diff
|
||||
--- a/lib/ansible/playbook/role/__init__.py
|
||||
+++ b/lib/ansible/playbook/role/__init__.py
|
||||
@@ -536,10 +536,10 @@ class Role(Base, Become, Conditional, Taggable, CollectionSearch):
|
||||
@@ -202,6 +202,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
|
||||
def __eq__(self, other):
|
||||
if not isinstance(other, Role):
|
||||
return False
|
||||
|
||||
return self._get_hash_dict() == other._get_hash_dict()
|
||||
|
||||
+ def __hash__(self):
|
||||
+ # Subset of _get_hash_dict fields; any two roles that compare equal share the same (name, path).
|
||||
+ return hash((self.get_name(), self.get_role_path()))
|
||||
+
|
||||
@@ -536,10 +540,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
|
||||
# get exported variables from meta/dependencies
|
||||
- seen = []
|
||||
+ seen = set()
|
||||
|
|
@ -43,8 +68,27 @@ At D=200 dependencies: 19,900 comparisons → 200 comparisons (99.5x reduction)
|
|||
+ seen.add(dep)
|
||||
```
|
||||
|
||||
Note: `Role` objects are used as set members; Python uses identity (`id()`) by default for unhashed objects, which is correct here — same object in memory = same dep. If Role doesn't define `__hash__`, Python uses the default identity hash.
|
||||
## Hash/eq contract
|
||||
Python requires `a == b` implies `hash(a) == hash(b)`. `Role.__eq__` compares
|
||||
`_get_hash_dict()` (name, path, params, when, tags, from_files, vars,
|
||||
from_include). `__hash__` over `(name, path)` is a stable subset: any two
|
||||
roles that compare equal share name and path, so they hash equal. Hash
|
||||
collisions on differing params/when/etc. fall through to `__eq__` and resolve
|
||||
correctly — allowed under the contract.
|
||||
|
||||
## Complexity
|
||||
- Before: O(D²) — D = number of transitive role dependencies
|
||||
- After: O(D) — set membership is O(1) amortized
|
||||
- Before: O(D^2) — D = number of transitive role dependencies
|
||||
- After: O(D) — set membership O(1) amortized
|
||||
|
||||
## Complexity gate (bench)
|
||||
`defects/ansible/bench/bench-ansible-0001.py` runs 4 scales (D=100..2000),
|
||||
min of 3 trials per scale. The bench also asserts that the naive list->set
|
||||
swap raises `TypeError` on `MockRoleEqOnly` (Role with `__eq__`, no
|
||||
`__hash__`), proving `__hash__` is a required prerequisite — not a polish.
|
||||
|
||||
Results committed at `defects/ansible/bench/results.txt`.
|
||||
|
||||
## Upstream
|
||||
- PR branch: `russellballestrini/ansible:fix/role-get-vars-seen-set`
|
||||
- Unit test: `test/units/playbook/role/test_role.py::TestRole::test_role_is_hashable_and_set_dedupes`
|
||||
- Integration target: `test/integration/targets/roles_var_inheritance` (exercises shared transitive dep dedup via `common_dep` -> `nested_dep`)
|
||||
|
|
|
|||
113
defects/ansible/patch/ansible-0001-role-get-vars-seen-list.patch
Normal file
113
defects/ansible/patch/ansible-0001-role-get-vars-seen-list.patch
Normal file
|
|
@ -0,0 +1,113 @@
|
|||
# UNDF: UNDF-2026-000000005
|
||||
# CWE-407: Algorithmic Complexity, O(D^2) -> O(D) in ansible.playbook.role.Role.get_vars()
|
||||
#
|
||||
# Defect: Role.get_vars() dedupes transitive deps with `seen = []` plus
|
||||
# `dep not in seen`, O(D) per iteration. Total cost: O(D^2) over D transitive
|
||||
# role dependencies. At D=2000: ~200ms per get_vars() call.
|
||||
#
|
||||
# Root cause: a naive `seen = set()` swap raises TypeError — Role defines
|
||||
# __eq__ without __hash__, so instances are unhashable by default.
|
||||
#
|
||||
# Fix: add __hash__ on Role hashing (name, path) (a stable subset of
|
||||
# _get_hash_dict equality fields), then swap `seen = []` for `seen = set()`
|
||||
# and `seen.append` for `seen.add`. Total cost after: O(D).
|
||||
# At D=2000: ~0.75ms per get_vars() call (~270x faster).
|
||||
#
|
||||
# Complexity gate (defects/ansible/bench/bench-ansible-0001.py):
|
||||
# Four scales D=100,500,1000,2000, min of 3 trials per scale.
|
||||
# Asserts naive list->set swap raises TypeError on MockRoleEqOnly.
|
||||
# Speedup at D=2000 observed >= ~130x on CPython 3.12.
|
||||
#
|
||||
# Upstream tests (ansible.git):
|
||||
# Unit: test/units/playbook/role/test_role.py::TestRole::test_role_is_hashable_and_set_dedupes
|
||||
# Integration: test/integration/targets/roles_var_inheritance (shared common_dep -> nested_dep)
|
||||
#
|
||||
From 6a5f7d2596d24acaffb480808076fc6990353e02 Mon Sep 17 00:00:00 2001
|
||||
From: "russell@unturf.com" <russell@unturf.com>
|
||||
Date: Thu, 23 Apr 2026 12:56:21 -0400
|
||||
Subject: [PATCH] playbook/role: dedupe dependency vars via set
|
||||
|
||||
Role.get_vars() previously used a list for `seen` dependency dedup,
|
||||
making the membership check O(D) per iteration and total O(D^2) over
|
||||
D transitive dependencies. Switch to a set, reducing to O(D).
|
||||
|
||||
Role defines __eq__ without __hash__ (implicitly unhashable), so add
|
||||
__hash__ hashing (name, path) -- a stable subset of the equality
|
||||
fields, preserving the eq/hash contract.
|
||||
---
|
||||
.../fragments/role-get-vars-seen-set.yml | 2 ++
|
||||
lib/ansible/playbook/role/__init__.py | 8 ++++++--
|
||||
test/units/playbook/role/test_role.py | 19 +++++++++++++++++++
|
||||
3 files changed, 27 insertions(+), 2 deletions(-)
|
||||
create mode 100644 changelogs/fragments/role-get-vars-seen-set.yml
|
||||
|
||||
diff --git a/changelogs/fragments/role-get-vars-seen-set.yml b/changelogs/fragments/role-get-vars-seen-set.yml
|
||||
new file mode 100644
|
||||
index 0000000..a3b6946
|
||||
--- /dev/null
|
||||
+++ b/changelogs/fragments/role-get-vars-seen-set.yml
|
||||
@@ -0,0 +1,2 @@
|
||||
+minor_changes:
|
||||
+ - role - ``Role.get_vars()`` now deduplicates transitive dependencies via a set rather than a list, reducing complexity from O(D\ :sup:`2`\ ) to O(D); ``Role`` gains an ``__hash__`` method consistent with its existing ``__eq__``.
|
||||
diff --git a/lib/ansible/playbook/role/__init__.py b/lib/ansible/playbook/role/__init__.py
|
||||
index ab79c55..05a2bb3 100644
|
||||
--- a/lib/ansible/playbook/role/__init__.py
|
||||
+++ b/lib/ansible/playbook/role/__init__.py
|
||||
@@ -205,6 +205,10 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
|
||||
|
||||
return self._get_hash_dict() == other._get_hash_dict()
|
||||
|
||||
+ def __hash__(self):
|
||||
+ # Subset of _get_hash_dict fields; any two roles that compare equal share the same (name, path).
|
||||
+ return hash((self.get_name(), self.get_role_path()))
|
||||
+
|
||||
@staticmethod
|
||||
def load(role_include, play, parent_role=None, from_files=None, from_include=False, validate=True, public=None, static=True, rescuable=True):
|
||||
if from_files is None:
|
||||
@@ -536,14 +540,14 @@ class Role(Base, Conditional, Taggable, CollectionSearch, Delegatable):
|
||||
all_vars = self.get_inherited_vars(dep_chain, only_exports=only_exports)
|
||||
|
||||
# get exported variables from meta/dependencies
|
||||
- seen = []
|
||||
+ seen = set()
|
||||
for dep in self.get_all_dependencies():
|
||||
# Avoid rerunning dupe deps since they can have vars from previous invocations and they accumulate in deps
|
||||
# TODO: re-examine dep loading to see if we are somehow improperly adding the same dep too many times
|
||||
if dep not in seen:
|
||||
# only take 'exportable' vars from deps
|
||||
all_vars = combine_vars(all_vars, dep.get_vars(include_params=False, only_exports=True))
|
||||
- seen.append(dep)
|
||||
+ seen.add(dep)
|
||||
|
||||
# role_vars come from vars/ in a role
|
||||
all_vars = combine_vars(all_vars, self._role_vars)
|
||||
diff --git a/test/units/playbook/role/test_role.py b/test/units/playbook/role/test_role.py
|
||||
index cbfe776..2163849 100644
|
||||
--- a/test/units/playbook/role/test_role.py
|
||||
+++ b/test/units/playbook/role/test_role.py
|
||||
@@ -410,3 +410,22 @@ class TestRole(unittest.TestCase):
|
||||
r = Role.load(i, play=mock_play)
|
||||
|
||||
self.assertEqual(r.get_name(), "foo_complex")
|
||||
+
|
||||
+ @patch('ansible.playbook.role.definition.unfrackpath', mock_unfrackpath_noop)
|
||||
+ def test_role_is_hashable_and_set_dedupes(self):
|
||||
+ fake_loader = DictDataLoader({
|
||||
+ "/etc/ansible/roles/foo_hashable/tasks/main.yml": "- shell: echo hi",
|
||||
+ })
|
||||
+
|
||||
+ mock_play = MagicMock()
|
||||
+ mock_play.role_cache = {}
|
||||
+
|
||||
+ i1 = RoleInclude.load(dict(role='foo_hashable'), play=mock_play, loader=fake_loader)
|
||||
+ r1 = Role.load(i1, play=mock_play)
|
||||
+ i2 = RoleInclude.load(dict(role='foo_hashable'), play=mock_play, loader=fake_loader)
|
||||
+ r2 = Role.load(i2, play=mock_play)
|
||||
+
|
||||
+ hash(r1)
|
||||
+ self.assertEqual(r1, r2)
|
||||
+ self.assertEqual(hash(r1), hash(r2))
|
||||
+ self.assertEqual(len({r1, r2}), 1)
|
||||
--
|
||||
2.43.0
|
||||
|
||||
Loading…
Add table
Add a link
Reference in a new issue