From 80d9316a1d75d24fcef06df5fcc7f0ac6a054cd8 Mon Sep 17 00:00:00 2001 From: Will Thames Date: Mon, 4 Sep 2023 17:17:24 +1000 Subject: [PATCH 1/3] Extend hidden_fields to allow more complicated field definitions This allows us to ignore e.g. the last-applied-configuration annotation by specifying `metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]` --- .github/workflows/ci.yml | 54 ++++--------- plugins/module_utils/k8s/service.py | 51 ++++++++++--- plugins/modules/k8s.py | 3 +- plugins/modules/k8s_info.py | 3 +- .../targets/k8s_hide_fields/tasks/main.yml | 9 +++ tests/unit/module_utils/test_hide_fields.py | 76 +++++++++++++++++++ 6 files changed, 146 insertions(+), 50 deletions(-) create mode 100644 tests/unit/module_utils/test_hide_fields.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 091055c638..4f6807b81f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -34,25 +34,16 @@ jobs: "ansible-version": "stable-2.9" }, { - "ansible-version": "stable-2.12", "python-version": "3.7" }, { "ansible-version": "stable-2.12", "python-version": "3.11" }, - { - "ansible-version": "stable-2.13", - "python-version": "3.7" - }, { "ansible-version": "stable-2.13", "python-version": "3.11" }, - { - "ansible-version": "stable-2.14", - "python-version": "3.7" - }, { "ansible-version": "stable-2.14", "python-version": "3.8" @@ -61,10 +52,6 @@ jobs: "ansible-version": "stable-2.14", "python-version": "3.11" }, - { - "ansible-version": "stable-2.15", - "python-version": "3.7" - }, { "ansible-version": "stable-2.15", "python-version": "3.8" @@ -75,19 +62,19 @@ jobs: }, { "ansible-version": "milestone", - "python-version": "3.7" + "python-version": "3.8" }, { "ansible-version": "milestone", - "python-version": "3.8" + "python-version": "3.9" }, { "ansible-version": "devel", - "python-version": "3.7" + "python-version": "3.8" }, { "ansible-version": "devel", - "python-version": "3.8" + "python-version": "3.9" } ] unit-source: @@ -138,6 +125,10 @@ jobs: "ansible-version": "milestone", "python-version": "3.8" }, + { + "ansible-version": "milestone", + "python-version": "3.9" + }, { "ansible-version": "devel", "python-version": "3.7" @@ -145,6 +136,10 @@ jobs: { "ansible-version": "devel", "python-version": "3.8" + }, + { + "ansible-version": "devel", + "python-version": "3.9" } ] collection_pre_install: '' @@ -187,31 +182,12 @@ jobs: fail-fast: false matrix: ansible-version: - - stable-2.12 + - stable-2.15 - milestone - devel python-version: - - "3.8" - - "3.9" - exclude: - - ansible-version: stable-2.9 - python-version: 3.9 - - ansible-version: stable-2.9 - python-version: 3.10 - - ansible-version: stable-2.9 - python-version: 3.11 - - ansible-version: stable-2.12 - python-version: 3.11 - - ansible-version: stable-2.13 - python-version: 3.11 - - ansible-version: stable-2.14 - python-version: 3.8 - - ansible-version: stable-2.15 - python-version: 3.8 - - ansible-version: milestone - python-version: 3.8 - - ansible-version: devel - python-version: 3.8 + - "3.10" + - "3.11" enable-turbo-mode: - true - false diff --git a/plugins/module_utils/k8s/service.py b/plugins/module_utils/k8s/service.py index 95cf2136c9..0987ab70ac 100644 --- a/plugins/module_utils/k8s/service.py +++ b/plugins/module_utils/k8s/service.py @@ -538,14 +538,47 @@ def hide_fields(definition: dict, hidden_fields: Optional[list]) -> dict: return result -# hide_field is not hugely sophisticated and designed to cope -# with e.g. status or metadata.managedFields rather than e.g. -# spec.template.spec.containers[0].env[3].value +# hide_field should be able to cope with simple or more complicated +# field definitions +# e.g. status or metadata.managedFields or +# spec.template.spec.containers[0].env[3].value or +# metadata.annotations[kubectl.kubernetes.io/last-applied-configuration] def hide_field(definition: dict, hidden_field: str) -> dict: - split = hidden_field.split(".", 1) - if split[0] in definition: - if len(split) == 2: - definition[split[0]] = hide_field(definition[split[0]], split[1]) - else: - del definition[split[0]] + lbracket = hidden_field.find("[") + dot = hidden_field.find(".") + + def dict_contains_key(field: dict, key: str) -> bool: + return key in field + + def list_contains_key(field: list, key: str) -> bool: + return key < len(field) + + field_contains_key = dict_contains_key + + if lbracket != -1 and (dot == -1 or lbracket < dot): + # handle lists and dicts + rbracket = hidden_field.find("]") + key = hidden_field[lbracket + 1:rbracket] + field = hidden_field[:lbracket] + # skip past right bracket and any following dot + rest = hidden_field[rbracket + 2:] + + if key.isdecimal(): + key = int(key) + field_contains_key = list_contains_key + if field in definition and field_contains_key(definition[field], key): + if rest: + definition[field][key] = hide_field(definition[field][key], rest) + else: + del definition[field][key] + if not definition[field]: + del definition[field] + else: + # handle standard fields + split = hidden_field.split(".", 1) + if split[0] in definition: + if len(split) == 2: + definition[split[0]] = hide_field(definition[split[0]], split[1]) + else: + del definition[split[0]] return definition diff --git a/plugins/modules/k8s.py b/plugins/modules/k8s.py index baa28c0429..ffa6fe35f4 100644 --- a/plugins/modules/k8s.py +++ b/plugins/modules/k8s.py @@ -189,7 +189,8 @@ description: - Hide fields matching this option in the result - An example might be C(hidden_fields=[metadata.managedFields]) - - Only field definitions that don't reference list items are supported (so V(spec.containers[0]) would not work) + or C(hidden_fields=[spec.containers[0].env[3].value]) + or C(hidden_fields=[metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]]) type: list elements: str version_added: 2.5.0 diff --git a/plugins/modules/k8s_info.py b/plugins/modules/k8s_info.py index 7f144243c2..dda4723f98 100644 --- a/plugins/modules/k8s_info.py +++ b/plugins/modules/k8s_info.py @@ -48,7 +48,8 @@ description: - Hide fields matching any of the field definitions in the result - An example might be C(hidden_fields=[metadata.managedFields]) - - Only field definitions that don't reference list items are supported (so V(spec.containers[0]) would not work) + or C(hidden_fields=[spec.containers[0].env[3].value]) + or C(hidden_fields=[metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]]) type: list elements: str version_added: 2.5.0 diff --git a/tests/integration/targets/k8s_hide_fields/tasks/main.yml b/tests/integration/targets/k8s_hide_fields/tasks/main.yml index 4b361fb96b..d77342f318 100644 --- a/tests/integration/targets/k8s_hide_fields/tasks/main.yml +++ b/tests/integration/targets/k8s_hide_fields/tasks/main.yml @@ -77,6 +77,7 @@ definition: "{{ hide_fields_base_configmap | combine({'data':{'anew':'value'}}) }}" hidden_fields: - data + - metadata.annotations[kubectl.kubernetes.io/last-applied-configuration] apply: true register: hf6 diff: true @@ -86,6 +87,14 @@ that: - hf6.changed + - name: Ensure hidden fields are not present + assert: + that: + - >- + 'annotations' not in hf6.resources[0].metadata or + q'kubectl.kubernetes.io/last-applied-configuration' + not in hf6.resources[0].metadata.annotations + - name: Hidden field should not show up in deletion k8s: definition: "{{ hide_fields_base_configmap}}" diff --git a/tests/unit/module_utils/test_hide_fields.py b/tests/unit/module_utils/test_hide_fields.py new file mode 100644 index 0000000000..22cc88b794 --- /dev/null +++ b/tests/unit/module_utils/test_hide_fields.py @@ -0,0 +1,76 @@ +from ansible_collections.kubernetes.core.plugins.module_utils.k8s.service import ( + hide_fields, +) + +tests = [ + dict( + output=dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ), + hide_fields=["metadata"], + expected=dict(kind="ConfigMap", data=dict(one="1", two="2")), + ), + dict( + output=dict( + kind="ConfigMap", + metadata=dict( + name="foo", + annotations={ + "kubectl.kubernetes.io/last-applied-configuration": '{"testvalue"}' + }, + ), + data=dict(one="1", two="2"), + ), + hide_fields=[ + "metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]", + "data.one", + ], + expected=dict(kind="ConfigMap", metadata=dict(name="foo"), data=dict(two="2")), + ), + dict( + output=dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + env=[ + dict(name="ENV1", value="env1"), + dict(name="ENV2", value="env2"), + dict(name="ENV3", value="env3"), + ], + ) + ] + ), + ), + hide_fields=["spec.containers[0].env[1].value"], + expected=dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + env=[ + dict(name="ENV1", value="env1"), + dict(name="ENV2"), + dict(name="ENV3", value="env3"), + ], + ) + ] + ), + ), + ), +] + + +def test_hide_fields(): + for test in tests: + if hide_fields(test["output"], test["hide_fields"]) != test["expected"]: + print(test["output"]) + print(hide_fields(test["output"], test["hide_fields"])) + print(test["expected"]) + assert hide_fields(test["output"], test["hide_fields"]) == test["expected"] From 4f60378a1dec79d1ec15272e3350002eb79121d0 Mon Sep 17 00:00:00 2001 From: Will Thames Date: Tue, 5 Sep 2023 12:34:24 +1000 Subject: [PATCH 2/3] Improved refactor of hide_fields to better cope Needed to cope with multiple levels of bracketed keys e.g. spec.template.spec[securityContext][runAsUser] and implementing that lead to a much simpler version that finds the first key and the rest, and deals with just those two. Added more tests to cover some of the new supported cases. --- plugins/module_utils/k8s/service.py | 74 ++++--- tests/unit/module_utils/test_hide_fields.py | 225 ++++++++++++++------ 2 files changed, 204 insertions(+), 95 deletions(-) diff --git a/plugins/module_utils/k8s/service.py b/plugins/module_utils/k8s/service.py index 0987ab70ac..ae19b3860b 100644 --- a/plugins/module_utils/k8s/service.py +++ b/plugins/module_utils/k8s/service.py @@ -544,9 +544,6 @@ def hide_fields(definition: dict, hidden_fields: Optional[list]) -> dict: # spec.template.spec.containers[0].env[3].value or # metadata.annotations[kubectl.kubernetes.io/last-applied-configuration] def hide_field(definition: dict, hidden_field: str) -> dict: - lbracket = hidden_field.find("[") - dot = hidden_field.find(".") - def dict_contains_key(field: dict, key: str) -> bool: return key in field @@ -555,30 +552,49 @@ def list_contains_key(field: list, key: str) -> bool: field_contains_key = dict_contains_key - if lbracket != -1 and (dot == -1 or lbracket < dot): - # handle lists and dicts - rbracket = hidden_field.find("]") - key = hidden_field[lbracket + 1:rbracket] - field = hidden_field[:lbracket] - # skip past right bracket and any following dot - rest = hidden_field[rbracket + 2:] - - if key.isdecimal(): - key = int(key) - field_contains_key = list_contains_key - if field in definition and field_contains_key(definition[field], key): - if rest: - definition[field][key] = hide_field(definition[field][key], rest) - else: - del definition[field][key] - if not definition[field]: - del definition[field] - else: - # handle standard fields - split = hidden_field.split(".", 1) - if split[0] in definition: - if len(split) == 2: - definition[split[0]] = hide_field(definition[split[0]], split[1]) - else: - del definition[split[0]] + (key, rest) = hide_field_split2(hidden_field) + + if key.isdecimal(): + key = int(key) + field_contains_key = list_contains_key + if field_contains_key(definition, key): + if rest: + definition[key] = hide_field(definition[key], rest) + # remove empty dicts and lists from the result + if definition[key] == dict() or definition[key] == list(): + del definition[key] + else: + del definition[key] return definition + + +# hide_field_split2 returns the first key in hidden_field and the rest of the hidden_field +# We expect the first key to either be in brackets, to be terminated by the start of a left +# bracket, or to be terminated by a dot. + +# examples would be: +# field.another.next -> (field, another.next) +# field[key].value -> (field, [key].value) +# [key].value -> (key, value) +# [one][two] -> (one, [two]) + + +def hide_field_split2(hidden_field: str) -> (str, str): + lbracket = hidden_field.find("[") + rbracket = hidden_field.find("]") + dot = hidden_field.find(".") + + if lbracket == 0: + # skip past right bracket and any following dot + rest = hidden_field[rbracket + 1:] + if rest and rest[0] == ".": + rest = rest[1:] + return (hidden_field[lbracket + 1:rbracket], rest) + + if lbracket != -1 and (dot == -1 or lbracket < dot): + return (hidden_field[:lbracket], hidden_field[lbracket:]) + + split = hidden_field.split(".", 1) + if len(split) == 1: + return split[0], "" + return split diff --git a/tests/unit/module_utils/test_hide_fields.py b/tests/unit/module_utils/test_hide_fields.py index 22cc88b794..b772bf2c31 100644 --- a/tests/unit/module_utils/test_hide_fields.py +++ b/tests/unit/module_utils/test_hide_fields.py @@ -2,75 +2,168 @@ hide_fields, ) -tests = [ - dict( - output=dict( - kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + +def test_hiding_missing_field_does_nothing(): + output = dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ) + hidden_fields = ["doesnotexist"] + assert hide_fields(output, hidden_fields) == output + + +def test_hiding_simple_field(): + output = dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ) + hidden_fields = ["metadata"] + expected = dict(kind="ConfigMap", data=dict(one="1", two="2")) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_only_key_in_dict_removes_dict(): + output = dict(kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1")) + hidden_fields = ["data.one"] + expected = dict(kind="ConfigMap", metadata=dict(name="foo")) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_all_keys_in_dict_removes_dict(): + output = dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ) + hidden_fields = ["data.one", "data.two"] + expected = dict(kind="ConfigMap", metadata=dict(name="foo")) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_multiple_fields(): + output = dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ) + hidden_fields = ["metadata", "data.one"] + expected = dict(kind="ConfigMap", data=dict(two="2")) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_dict_key(): + output = dict( + kind="ConfigMap", + metadata=dict( + name="foo", + annotations={ + "kubectl.kubernetes.io/last-applied-configuration": '{"testvalue"}' + }, ), - hide_fields=["metadata"], - expected=dict(kind="ConfigMap", data=dict(one="1", two="2")), - ), - dict( - output=dict( - kind="ConfigMap", - metadata=dict( - name="foo", - annotations={ - "kubectl.kubernetes.io/last-applied-configuration": '{"testvalue"}' - }, - ), - data=dict(one="1", two="2"), + data=dict(one="1", two="2"), + ) + hidden_fields = [ + "metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]", + ] + expected = dict( + kind="ConfigMap", metadata=dict(name="foo"), data=dict(one="1", two="2") + ) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_list_value_key(): + output = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + env=[ + dict(name="ENV1", value="env1"), + dict(name="ENV2", value="env2"), + dict(name="ENV3", value="env3"), + ], + ) + ] ), - hide_fields=[ - "metadata.annotations[kubectl.kubernetes.io/last-applied-configuration]", - "data.one", - ], - expected=dict(kind="ConfigMap", metadata=dict(name="foo"), data=dict(two="2")), - ), - dict( - output=dict( - kind="Pod", - metadata=dict(name="foo"), - spec=dict( - containers=[ - dict( - name="containers", - image="busybox", - env=[ - dict(name="ENV1", value="env1"), - dict(name="ENV2", value="env2"), - dict(name="ENV3", value="env3"), - ], - ) - ] - ), + ) + hidden_fields = ["spec.containers[0].env[1].value"] + expected = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + env=[ + dict(name="ENV1", value="env1"), + dict(name="ENV2"), + dict(name="ENV3", value="env3"), + ], + ) + ] ), - hide_fields=["spec.containers[0].env[1].value"], - expected=dict( - kind="Pod", - metadata=dict(name="foo"), - spec=dict( - containers=[ - dict( - name="containers", - image="busybox", - env=[ - dict(name="ENV1", value="env1"), - dict(name="ENV2"), - dict(name="ENV3", value="env3"), - ], - ) - ] - ), + ) + assert hide_fields(output, hidden_fields) == expected + + +def test_hiding_last_list_item(): + output = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + env=[ + dict(name="ENV1", value="env1"), + ], + ) + ] + ), + ) + hidden_fields = ["spec.containers[0].env[0]"] + expected = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + ) + ] ), - ), -] + ) + assert hide_fields(output, hidden_fields) == expected -def test_hide_fields(): - for test in tests: - if hide_fields(test["output"], test["hide_fields"]) != test["expected"]: - print(test["output"]) - print(hide_fields(test["output"], test["hide_fields"])) - print(test["expected"]) - assert hide_fields(test["output"], test["hide_fields"]) == test["expected"] +def test_hiding_nested_dicts_using_brackets(): + output = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + securityContext=dict(runAsUser=101), + ) + ] + ), + ) + hidden_fields = ["spec.containers[0][securityContext][runAsUser]"] + expected = dict( + kind="Pod", + metadata=dict(name="foo"), + spec=dict( + containers=[ + dict( + name="containers", + image="busybox", + ) + ] + ), + ) + if hide_fields(output, hidden_fields) != expected: + print(output) + print(expected) + assert hide_fields(output, hidden_fields) == expected From 99cfa890907cb68b132969b5edaee3805855813a Mon Sep 17 00:00:00 2001 From: Will Thames Date: Thu, 14 Sep 2023 14:12:56 +1000 Subject: [PATCH 3/3] Improve diff_objects to better hide hidden fields Ensure test correctly captures that hidden fields should be hidden in diffs too --- plugins/module_utils/k8s/service.py | 18 +++++++----------- .../targets/k8s_hide_fields/tasks/main.yml | 14 +++++++++++--- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/plugins/module_utils/k8s/service.py b/plugins/module_utils/k8s/service.py index ae19b3860b..ae5d4b6130 100644 --- a/plugins/module_utils/k8s/service.py +++ b/plugins/module_utils/k8s/service.py @@ -510,23 +510,19 @@ def diff_objects( result["before"] = diff[0] result["after"] = diff[1] - if list(result["after"].keys()) != ["metadata"] or list( + if list(result["after"].keys()) == ["metadata"] and list( result["before"].keys() - ) != ["metadata"]: - return False, result + ) == ["metadata"]: + # If only metadata.generation and metadata.resourceVersion changed, ignore it + ignored_keys = set(["generation", "resourceVersion"]) - # If only metadata.generation and metadata.resourceVersion changed, ignore it - ignored_keys = set(["generation", "resourceVersion"]) - - if not set(result["after"]["metadata"].keys()).issubset(ignored_keys): - return False, result - if not set(result["before"]["metadata"].keys()).issubset(ignored_keys): - return False, result + if set(result["after"]["metadata"].keys()).issubset(ignored_keys) and set(result["before"]["metadata"].keys()).issubset(ignored_keys): + return True, result result["before"] = hide_fields(result["before"], hidden_fields) result["after"] = hide_fields(result["after"], hidden_fields) - return True, result + return False, result def hide_fields(definition: dict, hidden_fields: Optional[list]) -> dict: diff --git a/tests/integration/targets/k8s_hide_fields/tasks/main.yml b/tests/integration/targets/k8s_hide_fields/tasks/main.yml index d77342f318..f54fe9eb6a 100644 --- a/tests/integration/targets/k8s_hide_fields/tasks/main.yml +++ b/tests/integration/targets/k8s_hide_fields/tasks/main.yml @@ -91,9 +91,17 @@ assert: that: - >- - 'annotations' not in hf6.resources[0].metadata or - q'kubectl.kubernetes.io/last-applied-configuration' - not in hf6.resources[0].metadata.annotations + 'annotations' not in hf6.result.metadata or + 'kubectl.kubernetes.io/last-applied-configuration' + not in hf6.result.metadata.annotations + - >- + 'annotations' not in hf6.diff.before.metadata or + 'kubectl.kubernetes.io/last-applied-configuration' + not in hf6.diff.before.metadata.annotations + - >- + 'annotations' not in hf6.diff.after.metadata or + 'kubectl.kubernetes.io/last-applied-configuration' + not in hf6.diff.after.metadata.annotations - name: Hidden field should not show up in deletion k8s: