Repository: ansible/ansible
Pull request: Ensure k8s apply works with check mode
##### SUMMARY

Update the new predicted object with fields from the previous object
before applying in check mode

Don't log output of `file` with `state: absent` on huge virtualenvs!

Fixes #60510

##### ISSUE TYPE
- Bugfix Pull Request

##### COMPONENT NAME
module_utils/k8s/raw.py

Problem statement:
k8s module shows diff changes for metadata added by default when we changed nothing
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
Using the k8s module running in --check and --diff modes shows metadata that would be removed which is data automatically added by kubernetes.

```
--- before
+++ after
@@ -1,5 +1 @@
-metadata:
-  creationTimestamp: '2019-05-16T00:49:39Z'
-  resourceVersion: '120846'
-  selfLink: /apis/rbac.authorization.k8s.io/v1/clusterroles/podsecuritypolicy-default
-  uid: 7c5e2d29-7774-11e9-ad63-0050569d5531
+metadata: {}
```

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
k8s

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.9.0.dev0
  config file = /workspace/ansible.cfg
  configured module search path = ['/home/ansible/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python3.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 3.7.3 (default, May  3 2019, 11:24:39) [GCC 8.3.0
```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below
DEFAULT_ROLES_PATH(/workspace/ansible.cfg) = ['/workspace/ansible_remote_roles']
DEFAULT_STDOUT_CALLBACK(/workspace/ansible.cfg) = yaml
HOST_KEY_CHECKING(/workspace/ansible.cfg) = False
INVENTORY_ENABLED(/workspace/ansible.cfg) = ['yaml']
LOCALHOST_WARNING(/workspace/ansible.cfg) = False
RETRY_FILES_ENABLED(/workspace/ansible.cfg) = False
```

##### OS / ENVIRONMENT
Kubernetes 1.14.1

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->
Using the k8s module add kubernetes object. Run ansible again using check mode and it shows a diff of metadata that should be ignored as changing.

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: "default Pod Security Policy on {{ kubernetes_cluster_name }}"
  k8s:
    state: present
    definition: "{{ lookup('file', 'psp-default.yml') }}"
    kubeconfig: "{{ kubernetes_kubeconfig_file_path }}"
    validate_certs: no
```

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
Diff shows nothing changed since we didn't change anything. 

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
Diff shows changes of metadata that is automatically added by kubernetes

<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/clustering/k8s/k8s.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/clustering/k8s/k8s.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @chouseknecht @fabianvf @flaper87 @maxamillion @willthames
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
After some more testing it seems like anything added to any fields (metadata, spec, etc.) automatically will then be pulled back as a diff when ran in check mode so it isn't just limited to the metadata.
Thanks @mitchellmaler - looks like that might be a bug in `apply`, turned on by default in (future) 2.9

Can you confirm that by running it with `apply: no` set?
I've reproduced this with a new test in the test suite now.

Now I just have to fix the broken test!
@willthames I tested it with apply: no and it reverted to how it used to work and shows no changes as expected. 
There's similar issue with generated nodePorts for services:
```
    TASK [Create service] ************************************
    --- before
    +++ after
    @@ -2,10 +2,9 @@
         "spec": {
             "ports": [
                 {
    -                "nodePort": 31495,
                     "port": 80,
                     "protocol": "TCP",
                     "targetPort": 8080
                 }
             ]
         }
```
The definition for this service ports is:
```
        ports:
          - port: 80
            protocol: TCP
            targetPort: 8080
```
The issue is in common/dict_transformations.py dict_merge function:
```
diff --git a/lib/ansible/module_utils/common/dict_transformations.py b/lib/ansible/module_utils/common/dict_transformations.py
index 831fbc5385..ab27b53887 100644
--- a/lib/ansible/module_utils/common/dict_transformations.py
+++ b/lib/ansible/module_utils/common/dict_transformations.py
@@ -118,7 +118,10 @@ def dict_merge(a, b):
         if k in result and isinstance(result[k], dict):
             result[k] = dict_merge(result[k], v)
         else:
-            result[k] = deepcopy(v)
+            if isinstance(result[k], list):
+                result[k] = [dict_merge(result[k][i], v[i]) if isinstance(result[k][i], dict) else deepcopy(v[i]) for i in range(len(result[k]))]
+            else:
+                result[k] = deepcopy(v)
     return result
```
I haven't properly tested the patch, not sure it works in all cases.

Commit message: Add changelog fragment for k8s apply check mode fix

Patch to review:
diff --git a/changelogs/fragments/60510-k8s-apply-check-mode.yml b/changelogs/fragments/60510-k8s-apply-check-mode.yml
new file mode 100644
index 00000000000000..8bffb0d251e52f
--- /dev/null
+++ b/changelogs/fragments/60510-k8s-apply-check-mode.yml
@@ -0,0 +1,2 @@
+minor_bugfixes:
+  - k8s - ensure that apply works with check mode. Bumps minimum openshift version for apply to 0.9.2.
diff --git a/lib/ansible/module_utils/k8s/raw.py b/lib/ansible/module_utils/k8s/raw.py
index 7bb2f3c68d8afb..c8209928086f8e 100644
--- a/lib/ansible/module_utils/k8s/raw.py
+++ b/lib/ansible/module_utils/k8s/raw.py
@@ -53,6 +53,13 @@
     K8S_CONFIG_HASH_IMP_ERR = traceback.format_exc()
     HAS_K8S_CONFIG_HASH = False
 
+HAS_K8S_APPLY = None
+try:
+    from openshift.dynamic.apply import apply_object
+    HAS_K8S_APPLY = True
+except ImportError:
+    HAS_K8S_APPLY = False
+
 
 class KubernetesRawModule(KubernetesAnsibleModule):
 
@@ -117,11 +124,11 @@ def __init__(self, k8s_kind=None, *args, **kwargs):
             if LooseVersion(self.openshift_version) < LooseVersion("0.6.2"):
                 self.fail_json(msg=missing_required_lib("openshift >= 0.6.2", reason="for merge_type"))
         if self.params.get('apply') is not None:
-            if LooseVersion(self.openshift_version) < LooseVersion("0.9.0"):
-                self.fail_json(msg=missing_required_lib("openshift >= 0.9.0", reason="for apply"))
+            if not HAS_K8S_APPLY:
+                self.fail_json(msg=missing_required_lib("openshift >= 0.9.2", reason="for apply"))
             self.apply = self.params['apply']
         else:
-            self.apply = LooseVersion(self.openshift_version) >= LooseVersion("0.9.0")
+            self.apply = HAS_K8S_APPLY
 
         if resource_definition:
             if isinstance(resource_definition, string_types):
@@ -288,7 +295,7 @@ def perform_action(self, resource, definition):
         else:
             if self.apply:
                 if self.check_mode:
-                    k8s_obj = definition
+                    ignored, k8s_obj = apply_object(resource, definition)
                 else:
                     try:
                         k8s_obj = resource.apply(definition, namespace=namespace).to_dict()
diff --git a/test/integration/targets/k8s/tasks/apply.yml b/test/integration/targets/k8s/tasks/apply.yml
index 7a4abdfd6dad4a..abfda06acfc530 100644
--- a/test/integration/targets/k8s/tasks/apply.yml
+++ b/test/integration/targets/k8s/tasks/apply.yml
@@ -55,6 +55,27 @@
         that:
           - k8s_configmap_2 is not changed
 
+    - name: add same configmap again with check mode on
+      k8s:
+        definition:
+          kind: ConfigMap
+          apiVersion: v1
+          metadata:
+            name: "apply-configmap"
+            namespace: "{{ apply_namespace }}"
+          data:
+            one: "1"
+            two: "2"
+            three: "3"
+        apply: yes
+      check_mode: yes
+      register: k8s_configmap_check
+
+    - name: check nothing changed
+      assert:
+        that:
+          - k8s_configmap_check is not changed
+
     - name: add same configmap again but using name and namespace args
       k8s:
         name: "apply-configmap"
diff --git a/test/integration/targets/k8s/tasks/main.yml b/test/integration/targets/k8s/tasks/main.yml
index f1fc919b917711..bb11bbc6862459 100644
--- a/test/integration/targets/k8s/tasks/main.yml
+++ b/test/integration/targets/k8s/tasks/main.yml
@@ -12,7 +12,7 @@
 
 - pip:
     name:
-      - 'openshift>=0.9.0'
+      - openshift>=0.9.2
       - coverage
     virtualenv: "{{ virtualenv }}"
     virtualenv_command: "{{ virtualenv_command }}"
@@ -25,13 +25,14 @@
 - file:
     path: "{{ virtualenv }}"
     state: absent
+  no_log: yes
 
 # Test validate with kubernetes-validate
 
 - pip:
     name:
       - kubernetes-validate==1.12.0
-      - 'openshift>=0.9.0'
+      - openshift>=0.9.2
       - coverage
     virtualenv: "{{ virtualenv }}"
     virtualenv_command: "{{ virtualenv_command }}"
@@ -45,6 +46,7 @@
 - file:
     path: "{{ virtualenv }}"
     state: absent
+  no_log: yes
 
 # Test graceful failure for older versions of openshift
 
@@ -66,12 +68,13 @@
 - file:
     path: "{{ virtualenv }}"
     state: absent
+  no_log: yes
 
 # Run full test suite
 
 - pip:
     name:
-      - 'openshift>=0.9.0'
+      - openshift>=0.9.2
       - coverage
     virtualenv: "{{ virtualenv }}"
     virtualenv_command: "{{ virtualenv_command }}"
@@ -86,3 +89,4 @@
 - file:
     path: "{{ virtualenv }}"
     state: absent
+  no_log: yes
diff --git a/test/integration/targets/k8s/tasks/older_openshift_fail.yml b/test/integration/targets/k8s/tasks/older_openshift_fail.yml
index e9006f8597e58e..2acf3d2175c4c6 100644
--- a/test/integration/targets/k8s/tasks/older_openshift_fail.yml
+++ b/test/integration/targets/k8s/tasks/older_openshift_fail.yml
@@ -66,4 +66,4 @@
       assert:
         that:
           - k8s_apply is failed
-          - "k8s_apply.msg.startswith('Failed to import the required Python library (openshift >= 0.9.0)')"
+          - "k8s_apply.msg.startswith('Failed to import the required Python library (openshift >= 0.9.2)')"


Code review comments:
diff_hunk: @@ -0,0 +1,2 @@
+minor_bugfixes:
path: changelogs/fragments/60510-k8s-apply-check-mode.yml
text: @user1:
```suggestion
bugfixes:
```
There's `bugfixes` and `minor_changes`, but no `minor_bugfixes`.