fix multiple issues with dry_run logic (#561)

fix multiple issues with dry_run logic

SUMMARY

Fix multiple issues with dry_run logic

The parameter value passed to the client set to dry_run=All instead of dry_run=True.
Add conditional check for Kubernetes release for the dry_run to be set
Add integration test that checks to ensure server side dry run is being used during check mode.


ISSUE TYPE


Bugfix Pull Request

Reviewed-by: Mike Graves <mgraves@redhat.com>
Reviewed-by: Jill R <None>

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/pull/561).

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 @@ class K8SClient:
 
     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:
+    if module and module.server_side_dry_run:
-        dry_run = module.params.get("dry_run", False)
+        dry_run = True
 
     k8s_client = K8SClient(
         configuration=configuration,

plugins/module_utils/k8s/core.py

@@ -47,6 +47,10 @@ class AnsibleK8SModule:
     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

plugins/module_utils/k8s/service.py

@@ -80,6 +80,10 @@ class K8sService:
         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 @@ class K8sService:
             )
         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 @@ class K8sService:
         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 @@ class K8sService:
         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 @@ class K8sService:
         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 @@ class K8sService:
         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 @@ class K8sService:
                     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 @@ class K8sService:
         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 @@ class K8sService:
             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 @@ class K8sService:
             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:

tests/integration/targets/k8s_check_mode/aliases

@@ -0,0 +1,3 @@
+time=30
+k8s
+k8s_info

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'

tests/integration/targets/k8s_check_mode/meta/main.yml

@@ -0,0 +1,3 @@
+---
+dependencies:
+  - remove_namespace

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

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