From bbf4a6e448737b04d764296a752d3e3dc9f5ff63 Mon Sep 17 00:00:00 2001 From: abikouo Date: Tue, 3 Jan 2023 17:17:24 +0100 Subject: [PATCH 1/3] fix multiple issues with dry_run logic --- changelogs/fragments/561-fix-dry-run.yml | 3 ++ plugins/module_utils/k8s/client.py | 8 +++-- plugins/module_utils/k8s/core.py | 4 +++ plugins/module_utils/k8s/service.py | 29 ++++++----------- .../targets/k8s_check_mode/aliases | 3 ++ .../targets/k8s_check_mode/defaults/main.yml | 10 ++++++ .../targets/k8s_check_mode/meta/main.yml | 3 ++ .../k8s_check_mode/tasks/check_mode.yml | 32 +++++++++++++++++++ .../targets/k8s_check_mode/tasks/main.yml | 19 +++++++++++ 9 files changed, 89 insertions(+), 22 deletions(-) create mode 100644 changelogs/fragments/561-fix-dry-run.yml create mode 100644 tests/integration/targets/k8s_check_mode/aliases create mode 100644 tests/integration/targets/k8s_check_mode/defaults/main.yml create mode 100644 tests/integration/targets/k8s_check_mode/meta/main.yml create mode 100644 tests/integration/targets/k8s_check_mode/tasks/check_mode.yml create mode 100644 tests/integration/targets/k8s_check_mode/tasks/main.yml diff --git a/changelogs/fragments/561-fix-dry-run.yml b/changelogs/fragments/561-fix-dry-run.yml new file mode 100644 index 0000000000..ace7ee9565 --- /dev/null +++ b/changelogs/fragments/561-fix-dry-run.yml @@ -0,0 +1,3 @@ +--- +bugfixes: + - Fix dry_run logic - Pass the value dry_run=All instead of dry_run=True to the client, add conditional check on kubernetes client version as this feature is supported only for kubernetes >= 18.20.0 (https://github.com/ansible-collections/kubernetes.core/issues/561). diff --git a/plugins/module_utils/k8s/client.py b/plugins/module_utils/k8s/client.py index 70442bec4c..2589e56073 100644 --- a/plugins/module_utils/k8s/client.py +++ b/plugins/module_utils/k8s/client.py @@ -267,6 +267,8 @@ class K8SClient: If there is a need for other methods or attributes to be proxied, they can be added here. """ + K8S_SERVER_DRY_RUN = "All" + def __init__(self, configuration, client, dry_run: bool = False) -> None: self.configuration = configuration self.client = client @@ -305,7 +307,7 @@ def resource(self, kind: str, api_version: str) -> Resource: def _ensure_dry_run(self, params: Dict) -> Dict: if self.dry_run: - params["dry_run"] = True + params["dry_run"] = self.K8S_SERVER_DRY_RUN return params def validate( @@ -354,8 +356,8 @@ def get_api_client(module=None, **kwargs: Optional[Any]) -> K8SClient: raise CoreException(msg) from e dry_run = False - if module: - dry_run = module.params.get("dry_run", False) + if module and module.server_side_dry_run: + dry_run = True k8s_client = K8SClient( configuration=configuration, diff --git a/plugins/module_utils/k8s/core.py b/plugins/module_utils/k8s/core.py index 95815deebf..131e80e29e 100644 --- a/plugins/module_utils/k8s/core.py +++ b/plugins/module_utils/k8s/core.py @@ -47,6 +47,10 @@ def __init__(self, **kwargs) -> None: def check_mode(self): return self._module.check_mode + @property + def server_side_dry_run(self): + return self.check_mode and self.has_at_least("kubernetes", "18.20.0") + @property def _diff(self): return self._module._diff diff --git a/plugins/module_utils/k8s/service.py b/plugins/module_utils/k8s/service.py index af21bd4c82..54307da1ed 100644 --- a/plugins/module_utils/k8s/service.py +++ b/plugins/module_utils/k8s/service.py @@ -80,6 +80,10 @@ def __init__(self, client, module) -> None: self.client = client self.module = module + @property + def _client_side_dry_run(self): + return self.module.check_mode and not self.client.dry_run + def find_resource( self, kind: str, api_version: str, fail: bool = False ) -> Optional[Resource]: @@ -153,8 +157,6 @@ def patch_resource( ) try: params = dict(name=name, namespace=namespace) - if self.module.check_mode: - params["dry_run"] = "All" if merge_type: params["content_type"] = "application/{0}-patch+json".format(merge_type) return self.client.patch(resource, definition, **params).to_dict() @@ -306,15 +308,12 @@ def create(self, resource: Resource, definition: Dict) -> Dict: namespace = definition["metadata"].get("namespace") name = definition["metadata"].get("name") - if self.module.check_mode and not self.client.dry_run: + if self._client_side_dry_run: k8s_obj = _encode_stringdata(definition) else: - params = {} - if self.module.check_mode: - params["dry_run"] = "All" try: k8s_obj = self.client.create( - resource, definition, namespace=namespace, **params + resource, definition, namespace=namespace ).to_dict() except ConflictError: # Some resources, like ProjectRequests, can't be created multiple times, @@ -344,7 +343,7 @@ def apply( server_side_apply = self.module.params.get("server_side_apply") if server_side_apply: requires("kubernetes", "19.15.0", reason="to use server side apply") - if self.module.check_mode and not self.client.dry_run: + if self._client_side_dry_run: ignored, patch = apply_object(resource, _encode_stringdata(definition)) if existing: k8s_obj = dict_merge(existing.to_dict(), patch) @@ -353,8 +352,6 @@ def apply( else: try: params = {} - if self.module.check_mode: - params["dry_run"] = "All" if server_side_apply: params["server_side"] = True params.update(server_side_apply) @@ -377,12 +374,9 @@ def replace( name = definition["metadata"].get("name") namespace = definition["metadata"].get("namespace") - if self.module.check_mode and not self.module.client.dry_run: + if self._client_side_dry_run: k8s_obj = _encode_stringdata(definition) else: - params = {} - if self.module.check_mode: - params["dry_run"] = "All" try: k8s_obj = self.client.replace( resource, @@ -390,7 +384,6 @@ def replace( name=name, namespace=namespace, append_hash=append_hash, - **params ).to_dict() except Exception as e: reason = e.body if hasattr(e, "body") else e @@ -404,7 +397,7 @@ def update( name = definition["metadata"].get("name") namespace = definition["metadata"].get("namespace") - if self.module.check_mode and not self.client.dry_run: + if self._client_side_dry_run: k8s_obj = dict_merge(existing.to_dict(), _encode_stringdata(definition)) else: exception = None @@ -445,7 +438,7 @@ def delete( return {} # Delete the object - if self.module.check_mode and not self.client.dry_run: + if self._client_side_dry_run: return {} if name: @@ -465,8 +458,6 @@ def delete( body.update(delete_options) params["body"] = body - if self.module.check_mode: - params["dry_run"] = "All" try: k8s_obj = self.client.delete(resource, **params).to_dict() except Exception as e: diff --git a/tests/integration/targets/k8s_check_mode/aliases b/tests/integration/targets/k8s_check_mode/aliases new file mode 100644 index 0000000000..078108247f --- /dev/null +++ b/tests/integration/targets/k8s_check_mode/aliases @@ -0,0 +1,3 @@ +time=30 +k8s +k8s_info \ No newline at end of file diff --git a/tests/integration/targets/k8s_check_mode/defaults/main.yml b/tests/integration/targets/k8s_check_mode/defaults/main.yml new file mode 100644 index 0000000000..2fd8c70c5c --- /dev/null +++ b/tests/integration/targets/k8s_check_mode/defaults/main.yml @@ -0,0 +1,10 @@ +--- +test_namespace: "check-mode-ns" + +test_config: + - k8s_release: "kubernetes < 18.20.0" + dry_run: false + virtualenv: 'env1' + - k8s_release: "kubernetes >= 18.20.0" + dry_run: true + virtualenv: 'env2' diff --git a/tests/integration/targets/k8s_check_mode/meta/main.yml b/tests/integration/targets/k8s_check_mode/meta/main.yml new file mode 100644 index 0000000000..3fec0259cb --- /dev/null +++ b/tests/integration/targets/k8s_check_mode/meta/main.yml @@ -0,0 +1,3 @@ +--- +# dependencies: +# - remove_namespace diff --git a/tests/integration/targets/k8s_check_mode/tasks/check_mode.yml b/tests/integration/targets/k8s_check_mode/tasks/check_mode.yml new file mode 100644 index 0000000000..5d9e2caa76 --- /dev/null +++ b/tests/integration/targets/k8s_check_mode/tasks/check_mode.yml @@ -0,0 +1,32 @@ +- name: Create virtualenv with kubernetes release + pip: + name: + - '{{ item.k8s_release }}' + virtualenv_command: "virtualenv --python {{ ansible_python_interpreter }}" + virtualenv: "{{ tmpdir }}/{{ item.virtualenv }}" + +- name: Create resource using check mode + k8s: + kind: Namespace + name: '{{ test_namespace }}' + check_mode: true + register: _create + +- name: Ensure namespace was not created + k8s_info: + kind: Namespace + name: '{{ test_namespace }}' + register: info + failed_when: info.resources | length > 0 + +- name: Ensure server side dry_run has being used + assert: + that: + - '"creationTimestamp" in _create.result.metadata' + when: item.dry_run + +- name: Ensure server side dry_run was not used + assert: + that: + - '"creationTimestamp" in _create.result.metadata' + when: not item.dry_run diff --git a/tests/integration/targets/k8s_check_mode/tasks/main.yml b/tests/integration/targets/k8s_check_mode/tasks/main.yml new file mode 100644 index 0000000000..60af294e5f --- /dev/null +++ b/tests/integration/targets/k8s_check_mode/tasks/main.yml @@ -0,0 +1,19 @@ +- name: create temporary directory for tests + tempfile: + suffix: .k8s + state: directory + register: _path + +- block: + - include_tasks: tasks/check_mode.yml + with_items: '{{ test_config }}' + + vars: + tmpdir: '{{ _path.path }}' + + always: + - name: Delete temporaray directory + file: + state: absent + path: '{{ tmpdir }}' + ignore_errors: true From bc2bb4f1af172eb9135b823c017f57ea074bdab5 Mon Sep 17 00:00:00 2001 From: Bikouo Aubin <79859644+abikouo@users.noreply.github.com> Date: Tue, 3 Jan 2023 18:46:50 +0100 Subject: [PATCH 2/3] add dependency for integration testing --- tests/integration/targets/k8s_check_mode/meta/main.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/targets/k8s_check_mode/meta/main.yml b/tests/integration/targets/k8s_check_mode/meta/main.yml index 3fec0259cb..2e3ba2fa37 100644 --- a/tests/integration/targets/k8s_check_mode/meta/main.yml +++ b/tests/integration/targets/k8s_check_mode/meta/main.yml @@ -1,3 +1,3 @@ --- -# dependencies: -# - remove_namespace +dependencies: + - remove_namespace From 97570d997e77ea30250ada0ba42058e712796685 Mon Sep 17 00:00:00 2001 From: Bikouo Aubin <79859644+abikouo@users.noreply.github.com> Date: Thu, 5 Jan 2023 16:54:23 +0100 Subject: [PATCH 3/3] Update 561-fix-dry-run.yml --- changelogs/fragments/561-fix-dry-run.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelogs/fragments/561-fix-dry-run.yml b/changelogs/fragments/561-fix-dry-run.yml index ace7ee9565..12fdd34036 100644 --- a/changelogs/fragments/561-fix-dry-run.yml +++ b/changelogs/fragments/561-fix-dry-run.yml @@ -1,3 +1,3 @@ --- bugfixes: - - Fix dry_run logic - Pass the value dry_run=All instead of dry_run=True to the client, add conditional check on kubernetes client version as this feature is supported only for kubernetes >= 18.20.0 (https://github.com/ansible-collections/kubernetes.core/issues/561). + - Fix dry_run logic - Pass the value dry_run=All instead of dry_run=True to the client, add conditional check on kubernetes client version as this feature is supported only for kubernetes >= 18.20.0 (https://github.com/ansible-collections/kubernetes.core/pull/561).