ansible-collections / ansible-collections/google.cloud

Incorrect logic in `gcp_utils` when comparing differences between `GcpRequest`

Open
#592 1 comment 1 reaction 0 assignees View on GitHub
stale
Dominant language
Python
Stars
105
Forks
144
Avg merge
5d 10h
Merged PRs (30d)
4

Description

##### SUMMARY

I believe there is a flaw in the logic that handles checking if the desired state and the actual state is different, and therefore whether any updates are required by the module that is calling it.

This means that certain changes aren't applied, even though the playbook will complete successfully (but incorrectly).

##### ISSUE TYPE

- Bug Report

##### COMPONENT NAME

`gcp_utils` affecting all modules that make use of the `GcpRequest` class's `difference()` method, for example `gcp_compute_engine`.

Related:
* https://github.com/ansible-collections/google.cloud/issues/307
* https://github.com/ansible-collections/google.cloud/issues/425

##### ANSIBLE VERSION

```paste below
$ ansible --version
ansible [core 2.15.3]
config file = /de-ansible/ansible.cfg
configured module search path = ['/de-ansible/library', '/de-ansible/roles/*/library', '/de-ansible/roles-vendored/*/library']
ansible python module location = /usr/local/lib/python3.10/dist-packages/ansible
ansible collection location = /de-ansible/collections:/usr/share/ansible/collections
executable location = /usr/local/bin/ansible
python version = 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0] (/usr/bin/python3)
jinja version = 3.1.2
libyaml = True
```

##### COLLECTION VERSION

```paste below
$ ansible-galaxy collection list google.cloud

# /usr/local/lib/python3.10/dist-packages/ansible_collections
Collection Version
------------ -------
google.cloud 1.2.0
```

##### CONFIGURATION

```paste below
ANY_ERRORS_FATAL(/de-ansible/ansible.cfg) = True
COLLECTIONS_ON_ANSIBLE_VERSION_MISMATCH(/de-ansible/ansible.cfg) = error
COLLECTIONS_PATHS(/de-ansible/ansible.cfg) = ['/de-ansible/collections', '/usr/share/ansible/collections']
CONFIG_FILE() = /de-ansible/ansible.cfg
DEFAULT_CALLBACK_PLUGIN_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/callback_plugins']
DEFAULT_FILTER_PLUGIN_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/filter_plugins']
DEFAULT_FORCE_HANDLERS(/de-ansible/ansible.cfg) = True
DEFAULT_INVENTORY_PLUGIN_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/inventory_plugins']
DEFAULT_JINJA2_NATIVE(/de-ansible/ansible.cfg) = True
DEFAULT_LOOKUP_PLUGIN_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/lookup_plugins']
DEFAULT_MODULE_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/library', '/de-ansible/roles/*/library', '/de-ansible/roles-vendored/*/library']
DEFAULT_PRIVATE_ROLE_VARS(/de-ansible/ansible.cfg) = True
DEFAULT_ROLES_PATH(/de-ansible/ansible.cfg) = ['/de-ansible/roles-vendored', '/de-ansible/roles', '/usr/share/ansible/collections']
DEFAULT_STDOUT_CALLBACK(/de-ansible/ansible.cfg) = yaml
DEFAULT_TIMEOUT(/de-ansible/ansible.cfg) = 10
DEPRECATION_WARNINGS(/de-ansible/ansible.cfg) = True
DIFF_ALWAYS(/de-ansible/ansible.cfg) = True
RETRY_FILES_ENABLED(/de-ansible/ansible.cfg) = False
TRANSFORM_INVALID_GROUP_CHARS(/de-ansible/ansible.cfg) = never
```
##### OS / ENVIRONMENT

Running in a Docker container based on Ubuntu 22.04:

```
$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description: Ubuntu 22.04.3 LTS
Release: 22.04
Codename: jammy
```

##### STEPS TO REPRODUCE

We can reproduce this issue with the `google.cloud.gcp_compute_instance` module. Create an instance with a set of network tags, then remove one of the tags, and re-run the playbook.

1. First create an instance like this:

```yaml
- name: "{{ 'Create' if gcp_compute_state == 'present' else 'Delete' }} instance {{ gcp_compute_name }}"
google.cloud.gcp_compute_instance:
name: "{{ gcp_compute_name }}"
state: "{{ gcp_compute_state }}"
machine_type: "{{ gcp_compute_type }}"
disks:
- auto_delete: 'true'
boot: 'true'
source:
selfLink: "https://www.googleapis.com/compute/v1/projects/{{ gcp_compute_project }}/zones/{{ gcp_compute_zone }}/disks/{{ gcp_compute_name }}-boot-disk"
tags:
items: ["tag_one", "tag_two", "tag_three"]
metadata:
startup-script: "{{ gcp_compute_metadata_script }}"
labels: {}
network_interfaces:
- subnetwork:
selfLink: "{{ gcp_compute_subnetwork }}"
zone: "{{ gcp_compute_zone }}"
project: "{{ gcp_compute_project }}"
auth_kind: serviceaccount
service_account_contents: "{{ gcp_compute_service_account }}"
```

2. Change the network tags in the playbook to the following and re-run the playbook:

```yaml
tags:
items: ["tag_two", "tag_three"]
```

##### EXPECTED RESULTS

In the first change the network tag `tag_one` has been removed from the playbook, and so the tag should be removed from the Compute instance, so the actual tags should be `["tag_two", "tag_three"]`.

##### ACTUAL RESULTS

The playbook succeeds, showing no changes, tags are actually still `["tag_one", "tag_two", "tag_three"]`.

##### ANALYSIS

There are two problems:

1. There is no logic to call the `setTags` API endpoint of the Compute Engine API.

This can be fixed by adding the following method to the `gcp_compute_instance` module:
```yaml
def tag_fingerprint_update(module, request, response):
auth = GcpSession(module, 'compute')
auth.post(
''.join(["https://compute.googleapis.com/compute/v1/", "projects/{project}/zones/{zone}/instances/{name}/setTags"]).format(**module.params),
{u'fingerprint': response.get('tags', {}).get('fingerprint'), u'items': request.get('tags', {}).get('items', [])},
)
```

and by modifying the `update_fields` method to add these two lines:

```yaml
if response.get('tags', {}).get('items', []) != request.get('tags', {}).get('items', []):
tag_fingerprint_update(module, request, response)
```

2. The logic responsible for checking if the desired state and actual state are different seems to be flawed. The code can be found in [`gcp_utils.py`, lines 395-423](https://github.com/ansible-collections/google.cloud/blob/master/plugins/module_utils/gcp_utils.py#L395-L423).

The logic here seems faulty, as it doesn't detect all the cases properly:
a. It will correctly detect if an item is defined in both lists, but has a different value.
b. It will detect if an item exists in the first list (representing the desired state) but does not exist in the second list (representing the actual state).
c. It *does not* detect if an item exists in the second list but does not exist in the first list, which is the use case we describe above, where a previously existing network tag is being removed.

Contributor guide

Open the contributing guide

Research direction

Start with plugins/module_utils/gcp_utils.py around lines 395-423 to inspect the difference() logic, then trace its use from the gcp_compute_instance module and update_fields. Reproduce the network-tag removal case from the issue and verify that an extra actual-state tag is detected and that the Compute Engine setTags operation leaves the instance with only the requested tags.

Written by the indexing model from the issue text.

Assessment

Tech stack
google-cloud, python
Domain
api, cloud
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.