From 98be9144c252b30cf1897560af4d58ef074f9f51 Mon Sep 17 00:00:00 2001 From: Simon Gerber Date: Mon, 25 Jul 2022 19:10:35 +0200 Subject: [PATCH] Refresh flattened references when interpolating values Sometimes using nested references to lookup values for parameters based on a parameter which is set differently in the hierarchy for different nodes leads to a ResolveError. The original issue was discovered in Project Syn component [backup-k8up], which supports being compiled multiple times with different configurations in multiple Kapitan targets. If the two configurations use different values for the parameter `backup_k8up.majorVersion`, the nested references which use this parameter to look up values for the appropriate major version sometimes use stale flattened refs when checking whether all nested references are resolved causing a `ResolveError`. From what I was able to determine this happens because nested references are flattened in `RefItem.assembleRefs()` which can cause interpolation to operate with stale flattened references if the same `RefItem` is part of the parameters structure for multiple nodes. We address the problem by calling `value.assembleRefs(self._base)` for each value which is processed in `Parameters._interpolate_inner()`. This fixes the issue both in the included minimal test case and our real-world case when rendering the reclass inventory for Project Syn clusters which include component [backup-k8up] as described in the [component v2 to v3 migration how-to]. I haven't been able to determine exactly how a `RefItem` object can be part of multiple nodes and therefore I'm not sure if it's expected that the same `RefItem` object can be part of multiple nodes or not. Unfortunately this means that I can't tell if my fix is just masking another bug somewhere which wrongly adds the same `RefItem` to the parameters of multiple nodes. [backup-k8up]: https://github.com/projectsyn/component-backup-k8up.git [component migration how-to]: https://hub.syn.tools/backup-k8up/how-tos/upgrade-v2-v3.html#_steps_to_run_two_instances_at_once --- reclass/datatypes/parameters.py | 10 ++++++++++ reclass/tests/data/07/classes/base.yml | 14 ++++++++++++++ reclass/tests/data/07/classes/build.yml | 5 +++++ reclass/tests/data/07/classes/global.yml | 3 +++ reclass/tests/data/07/classes/target.yml | 3 +++ reclass/tests/data/07/nodes/t1.yml | 13 +++++++++++++ reclass/tests/data/07/nodes/t2.yml | 15 +++++++++++++++ reclass/tests/test_core.py | 9 +++++++++ 8 files changed, 72 insertions(+) create mode 100644 reclass/tests/data/07/classes/base.yml create mode 100644 reclass/tests/data/07/classes/build.yml create mode 100644 reclass/tests/data/07/classes/global.yml create mode 100644 reclass/tests/data/07/classes/target.yml create mode 100644 reclass/tests/data/07/nodes/t1.yml create mode 100644 reclass/tests/data/07/nodes/t2.yml diff --git a/reclass/datatypes/parameters.py b/reclass/datatypes/parameters.py index ea803251..02418ebe 100644 --- a/reclass/datatypes/parameters.py +++ b/reclass/datatypes/parameters.py @@ -317,6 +317,16 @@ def _interpolate_inner(self, path, inventory): # list or dict has already been visited by _interpolate_inner del self._unrendered[path] return + # Refresh internal ref cache for the unrendered value with our current context + # to ensure that flattened cached references are updated if necessary if the + # value is part of the parameters tree of multiple nodes. + # + # Side-note (@simu,2022-05-27): I haven't quite determined when a value becomes + # part of the parameters tree of multiple nodes, but I've observed it to be the + # case by looking at `hex(id(value))` for the same parameters key in multiple + # targets in an example inventory which results in a ResolveError without the + # line below. + value.assembleRefs(self._base) self._unrendered[path] = False self._interpolate_references(path, value, inventory) new = self._interpolate_render_value(path, value, inventory) diff --git a/reclass/tests/data/07/classes/base.yml b/reclass/tests/data/07/classes/base.yml new file mode 100644 index 00000000..1cc9fa86 --- /dev/null +++ b/reclass/tests/data/07/classes/base.yml @@ -0,0 +1,14 @@ +parameters: + base: + =_mixed_lookup: + v1: + registry: ghcr.io + repository: path/to/image + tag: v1.2.0 + v2: ${base:image} + + version: v2 + image: + registry: ghcr.io + repository: path/to/image + tag: v2.4.0 diff --git a/reclass/tests/data/07/classes/build.yml b/reclass/tests/data/07/classes/build.yml new file mode 100644 index 00000000..d1b443bc --- /dev/null +++ b/reclass/tests/data/07/classes/build.yml @@ -0,0 +1,5 @@ +parameters: + build: + inputs: + - type: https + source: https://example.com/downloads/${base:_mixed_lookup:${base:version}:tag}/ diff --git a/reclass/tests/data/07/classes/global.yml b/reclass/tests/data/07/classes/global.yml new file mode 100644 index 00000000..7f196232 --- /dev/null +++ b/reclass/tests/data/07/classes/global.yml @@ -0,0 +1,3 @@ +parameters: + base: + version: v1 diff --git a/reclass/tests/data/07/classes/target.yml b/reclass/tests/data/07/classes/target.yml new file mode 100644 index 00000000..199b2854 --- /dev/null +++ b/reclass/tests/data/07/classes/target.yml @@ -0,0 +1,3 @@ +parameters: + instance: + version: v2 diff --git a/reclass/tests/data/07/nodes/t1.yml b/reclass/tests/data/07/nodes/t1.yml new file mode 100644 index 00000000..01b1b041 --- /dev/null +++ b/reclass/tests/data/07/nodes/t1.yml @@ -0,0 +1,13 @@ +classes: + - base + - build + - global + - target +parameters: + _instance: t1 + kapitan: + vars: + target: t1 + +# Expected value for `parameters.build.inputs.0.source: +# https://example.com/downloads/v1.2.0 diff --git a/reclass/tests/data/07/nodes/t2.yml b/reclass/tests/data/07/nodes/t2.yml new file mode 100644 index 00000000..ea73579f --- /dev/null +++ b/reclass/tests/data/07/nodes/t2.yml @@ -0,0 +1,15 @@ +classes: + - base + - build + - global + - target +parameters: + _instance: t2 + base: ${instance} + instance: {} + kapitan: + vars: + target: t2 + +# Expected value for `parameters.build.inputs.0.source: +# https://example.com/downloads/v2.4.0 diff --git a/reclass/tests/test_core.py b/reclass/tests/test_core.py index dd51f3a0..cdebe65c 100644 --- a/reclass/tests/test_core.py +++ b/reclass/tests/test_core.py @@ -123,5 +123,14 @@ def test_application_removal(self): self.assertEqual(A_node['applications'], A_node['parameters']['expected_apps']) self.assertEqual(B_node['applications'], B_node['parameters']['expected_apps']) + def test_mixed_value_constant_parameter_lookup(self): + reclass = self._core("07") + + t1 = reclass.nodeinfo("t1") + t2 = reclass.nodeinfo("t2") + + self.assertEqual(t1["parameters"]["build"]["inputs"][0]["source"], "https://example.com/downloads/v1.2.0/") + self.assertEqual(t2["parameters"]["build"]["inputs"][0]["source"], "https://example.com/downloads/v2.4.0/") + if __name__ == '__main__': unittest.main()