Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue of the vmware_cluster_info module does not decode of a clus… #366

Merged

Conversation

sky-joker
Copy link
Collaborator

@sky-joker sky-joker commented Aug 28, 2020

Depends-On: #374

SUMMARY

This PR is to fix the vmware_cluster_info module does not urldecode of a cluster name.
fixes: #365

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

plugins/modules/vmware_cluster_info

ADDITIONAL INFORMATION

tested on vCenter/ESXi 6.7

@RudneiBertolJr
Copy link

Hey @sky-joker,

Thank you for fix it.

I have a question, if it may be impacting other ansible resources that collect those facts from VMware, like Datacenter, Datastore, VM Guest name.

regards
rbertol

@sky-joker sky-joker changed the title [WIP] Fix issue of the vmware_cluster_info module does not decode of a clus… Fix issue of the vmware_cluster_info module does not decode of a clus… Aug 28, 2020
@sky-joker
Copy link
Collaborator Author

Hi @RudneiBertolJr

Thank you for reporting this issue.
When creating objects haven't this issue.
This issue occurs only when gathering information.
So some _info modules have the same issue.
I think the other _info modules need to be fixed in the same way.

First of all, can you please try this bug fixed module work at your environment, and please let me know the result?

@RudneiBertolJr
Copy link

Hey @sky-joker,

I have not tested yet, but I can tell you that it will work because it is very similar to the change that I did to work in my lab.

[root@dhcp181-132 ~]# cat /tmp/vmware_cluster_info.patch 
diff --git a/cloud/vmware/vmware_cluster_info.py b/cloud/vmware/vmware_cluster_info.py
index c1cf399..b3b1021 100644
--- a/cloud/vmware/vmware_cluster_info.py
+++ b/cloud/vmware/vmware_cluster_info.py
@@ -121,6 +121,11 @@ clusters:
 """
 
 try:
+    from urllib import unquote as urldecode
+except:
+    from urllib.parse import unquote as urldecode
+
+try:
     from pyVmomi import vim
 except ImportError:
     pass
@@ -213,7 +218,7 @@ class VmwreClusterInfoManager(PyVmomi):
                 vmware_client = VmwareRestClient(self.module)
                 tag_info = vmware_client.get_tags_for_cluster(cluster_mid=cluster._moId)
 
-            results['clusters'][cluster.name] = dict(
+            results['clusters'][urldecode(cluster.name)] = dict(
                 enable_ha=das_config.enabled,
                 ha_failover_level=ha_failover_level,
                 ha_vm_monitoring=das_config.vmMonitoring,

I will test it on Monday, and let you know if everything worked as expected.

regards
rbertol

@ansibullbot
Copy link

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug cloud community_review has_issue integration tests/integration module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_triage Needs a first human triage before being processed. plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests labels Aug 28, 2020
@sky-joker
Copy link
Collaborator Author

By the way, you can even get around this issue with the following like playbook.
e.g., used urlencode filter

- name: Workaround Playbook Example
  hosts: localhost
  gather_facts: no
  vars:
    # urlencode will not convert from ‘/‘ to ‘%2f'
    # workaround: https://jinja.palletsprojects.com/en/2.11.x/templates/#urlencode
    cluster_name: "Cluster{{ '/\\%' | urlencode | replace('/', '%2f') | lower }}"
  tasks:
    - name: "Gather all cluster info"
      vmware_cluster_info:
        hostname: "{{ vcenter_hostname }}"
        username: "{{ vcenter_username }}"
        password: "{{ vcenter_password }}"
        validate_certs: no
        datacenter: "{{ datacenter_name }}"
      register: gather_all_cluster

    - debug:
        msg:
          - "{{ gather_all_cluster.clusters[cluster_name] }}"

@goneri @Akasurde

There has been an issue with VMware modules not doing URL decoding for some time now, do you want to continue to respond with a workaround?

@RudneiBertolJr
Copy link

Hey @sky-joker,

I have tried to test on Ansible 2.9.10, however, the code on the file is a little bit different than the current version on Git.

I imported the unquote lib from ansible and changed the result line to unquote the cluster name, and it worked like a charm.

@sky-joker
Copy link
Collaborator Author

Thank you @RudneiBertolJr for taking the time out of checking the module work :)

@goneri
Copy link
Member

goneri commented Aug 31, 2020

Can we get the same problem with some other resource type?

@sky-joker
Copy link
Collaborator Author

Hi @goneri

I think some modules will be the same problem that occurs (mainly the _info-based modules).
I'll find the modules that have the same problem when I have time.

@goneri goneri added the gate label Sep 1, 2020
@goneri
Copy link
Member

goneri commented Sep 1, 2020

Thank you @sky-joker for the patch.

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

@goneri

Can you please check why the following error occurs by CI?

        "line": 567,
        "column": 4,
        "path": "scripts/inventory/vmware_inventory.py",
        "symbol": "too-complex",
        "message": "'facts_from_proplist' is too complex. The McCabe rating is 22",
        "message-id": "R1260"
    },
    {
        "type": "refactor",
        "module": "inventory.vmware_inventory",
        "obj": "VMWareInventory._process_object_types",
        "line": 690,
        "column": 4,
        "path": "scripts/inventory/vmware_inventory.py",
        "symbol": "too-complex",
        "message": "'_process_object_types' is too complex. The McCabe rating is 28",
        "message-id": "R1260"
    },
    {
        "type": "error",
        "module": "inventory.vmware_inventory",
        "obj": "VMWareInventory.get_instances",
        "line": 352,
        "column": 12,
        "path": "scripts/inventory/vmware_inventory.py",
        "symbol": "assigning-non-slot",
        "message": "Assigning to attribute 'check_hostname' not defined in class slots",
        "message-id": "E0237"
    },
    {
        "type": "error",
        "module": "inventory.vmware_inventory",
        "obj": "VMWareInventory.get_instances",
        "line": 360,
        "column": 12,
        "path": "scripts/inventory/vmware_inventory.py",
        "symbol": "assigning-non-slot",
        "message": "Assigning to attribute 'check_hostname' not defined in class slots",
        "message-id": "E0237"
    }
]�[0m
Cleaning up temporary python directory: /tmp/python-h4xmjwzp-ansible

https://dashboard.zuul.ansible.com/t/ansible/build/b9c74b5e85b44f2c919e876b6b183244

@goneri
Copy link
Member

goneri commented Sep 2, 2020

@goneri

Can you please check why the following error occurs by CI?

It should be better now.

@goneri
Copy link
Member

goneri commented Sep 2, 2020

recheck

4 similar comments
@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@sky-joker
Copy link
Collaborator Author

recheck

@ansible-zuul ansible-zuul bot merged commit 10310e7 into ansible-collections:main Sep 5, 2020
@sky-joker sky-joker deleted the fix_vmware_cluster_info branch September 5, 2020 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 bug This issue/PR relates to a bug cloud community_review has_issue integration tests/integration module module needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_triage Needs a first human triage before being processed. plugins plugin (any type) stale_ci CI is older than 7 days, rerun before merging tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vmware_cluster_info: Is not getting the correct VMware cluster name when the cluster name has '/' on the name.
4 participants