Repository: ansible/ansible
Pull request: nxos_vrf fix
Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>

##### SUMMARY
<!--- Describe the change, including rationale and design decisions -->
Fixes https://github.com/ansible/ansible/issues/36461
Also the module doesn't work when existing definition/property of vrf is attempted to be modified. The PR fixes that.

<!---
If you are fixing an existing issue, please include "Fixes #nnn" in your
commit message and your description; but you should still explain what
the change does.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest: -->
 - Bugfix Pull Request

##### COMPONENT NAME
<!--- Name of the module, plugin, module or task -->
modules/network/nxos/nxos_vrf.py
##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
devel
```

Problem statement:
nxos_vrf error when using vni option
<!---
Verify first that your issue/request is not already reported on GitHub.
Also test if the latest release, and devel branch are affected too.
-->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
 - Bug Report

##### COMPONENT NAME
<!---
Name of the module, plugin, task or feature
Do not include extra details here, e.g. "vyos_command" not "the network module vyos_command" or the full path
-->
nxos_vrf

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes below -->
```
ansible 2.6.0
  config file = /Users/jdacosta/Devel/ansible-netops/cisco/nxos/applications/fabric/europe-central-fr1/ansible.cfg
  configured module search path = [u'/Users/jdacosta/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Library/Python/2.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 2.7.10 (default, Feb  7 2017, 00:08:15) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)]
```

##### CONFIGURATION
<!---
If using Ansible 2.4 or above, paste the results of "ansible-config dump --only-changed"
Otherwise, mention any settings you have changed/added/removed in ansible.cfg
(or using the ANSIBLE_* environment variables).
-->

##### OS / ENVIRONMENT
<!---
Mention the OS you are running Ansible from, and the OS you are
managing, or say "N/A" for anything that is not platform-specific.
Also mention the specific version of what you are trying to control,
e.g. if this is a network bug the version of firmware on the network device.
-->

N/A
nxos 7.0(3)I7(1)

##### SUMMARY
<!--- Explain the problem briefly -->

##### STEPS TO REPRODUCE
<!---
For bugs, show exactly how to reproduce the problem, using a minimal test-case.
For new features, show how the feature would be used.
-->

<!--- Paste example playbooks or commands between quotes below -->
```yaml
- name: "Ensure state of VRF for VNIs"
  nxos_vrf:
    vrf: "tenant01"
    rd:  auto
    vni: "10150"
    state: "present"
    admin_state: "up"

```

<!--- You can also paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- What did you expect to happen when running the steps above? -->

##### ACTUAL RESULTS
<!--- What actually happened? If possible run with extra verbosity (-vvvv) -->
traceback error

<!--- Paste verbatim command output between quotes below -->
```
TASK [victorock.overlay-nxos-leaf : Ensure state of VRF for VNIs] ************************************************************
task path: /Users/jdacosta/Devel/ansible-netops/cisco/nxos/applications/fabric/europe-central-fr1/roles/victorock.overlay-nxos-leaf/tasks/vni.yml:14
<127.0.0.1> attempting to start connection
<127.0.0.1> using connection plugin network_cli
<127.0.0.1> found existing local domain socket, using it!
<127.0.0.1> updating play_context for connection
<127.0.0.1> 
<127.0.0.1> local domain socket path is /Users/jdacosta/.ansible/pc/32fbfb2e00
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/network/__init__.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/basic.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/network/nxos/__init__.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/network/common/utils.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/network/common/__init__.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/network/nxos/nxos.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/_text.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/parsing/convert_bool.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/parsing/__init__.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/pycompat24.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/six/__init__.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/connection.py
Using module_utils file /Library/Python/2.7/site-packages/ansible/module_utils/urls.py
Using module file /Library/Python/2.7/site-packages/ansible/modules/network/nxos/nxos_vrf.py
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: jdacosta
<127.0.0.1> EXEC /bin/sh -c 'echo ~ && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571 `" && echo ansible-tmp-1519162691.84-261532122578571="` echo /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571 `" ) && sleep 0'
<127.0.0.1> PUT /Users/jdacosta/.ansible/tmp/ansible-local-27233QmXSr2/tmp2bG1bG TO /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571/nxos_vrf.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571/ /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571/nxos_vrf.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/usr/bin/python /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571/nxos_vrf.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /Users/jdacosta/.ansible/tmp/ansible-tmp-1519162691.84-261532122578571/ > /dev/null 2>&1 && sleep 0'
The full traceback is:
Traceback (most recent call last):
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 467, in <module>
    main()
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 453, in main
    commands = map_obj_to_commands((want, have), module)
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 258, in map_obj_to_commands
    if have.get('vni') and have.get('vni') != '':
AttributeError: 'list' object has no attribute 'get'

failed: [leaf01] (item=nxos_vrf_vni tenant01) => {
    "changed": false, 
    "failed": true, 
    "nxos_vrf_vni": {
        "key": "l2_150_10150", 
        "value": {
            "multicast": {
                "group": "239.0.0.150"
            }, 
            "name": "l2_150_10150", 
            "route": {
                "distinguisher": "auto", 
                "target_both": "auto"
            }, 
            "states": {
                "admin": "up", 
                "feature": "enabled", 
                "shutdown": false, 
                "state": "present"
            }, 
            "svi": "10.1.150.1/24", 
            "vlan": 150, 
            "vni": 10150, 
            "vrf": "tenant01"
        }
    }, 
    "rc": 1
}

MSG:

MODULE FAILURE


MODULE_STDERR:

Traceback (most recent call last):
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 467, in <module>
    main()
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 453, in main
    commands = map_obj_to_commands((want, have), module)
  File "/var/folders/jv/29pmt_2j39ngz8ftz40j1mn00000gn/T/ansible_Tj2nE6/ansible_module_nxos_vrf.py", line 258, in map_obj_to_commands
    if have.get('vni') and have.get('vni') != '':
AttributeError: 'list' object has no attribute 'get'

```

Hints from issue discussion:
Files identified in the description:
* [lib/ansible/modules/network/nxos/nxos_vrf.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/network/nxos/nxos_vrf.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 @GGabriele @jedelman8 @kedarX @mikewiebe @privateip @rahushen @rcarrillocruz @trishnaguha
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->

Commit message: nxos_vrf fix

Signed-off-by: Trishna Guha <trishnaguha17@gmail.com>

Patch to review:
diff --git a/lib/ansible/modules/network/nxos/nxos_vrf.py b/lib/ansible/modules/network/nxos/nxos_vrf.py
index 291819453b6ab2..504a84c47eaecd 100644
--- a/lib/ansible/modules/network/nxos/nxos_vrf.py
+++ b/lib/ansible/modules/network/nxos/nxos_vrf.py
@@ -224,12 +224,12 @@ def map_obj_to_commands(updates, module):
     state = module.params['state']
     purge = module.params['purge']
 
+    args = ('rd', 'description', 'vni')
+
     for w in want:
         name = w['name']
-        description = w['description']
-        vni = w['vni']
-        rd = w['rd']
         admin_state = w['admin_state']
+        vni = w['vni']
         interfaces = w.get('interfaces') or []
         state = w['state']
         del w['state']
@@ -242,12 +242,11 @@ def map_obj_to_commands(updates, module):
         elif state == 'present':
             if not obj_in_have:
                 commands.append('vrf context {0}'.format(name))
-                if rd and rd != '':
-                    commands.append('rd {0}'.format(rd))
-                if description:
-                    commands.append('description {0}'.format(description))
-                if vni and vni != '':
-                    commands.append('vni {0}'.format(vni))
+                for item in args:
+                    candidate = w.get(item)
+                    if candidate:
+                        cmd = item + ' ' + str(candidate)
+                        commands.append(cmd)
                 if admin_state == 'up':
                     commands.append('no shutdown')
                 elif admin_state == 'down':
@@ -255,8 +254,9 @@ def map_obj_to_commands(updates, module):
 
                 if commands:
                     if vni:
-                        if have.get('vni') and have.get('vni') != '':
-                            commands.insert(1, 'no vni {0}'.format(have['vni']))
+                        for h in have:
+                            if h.get('vni'):
+                                commands.insert(1, 'no vni {0}'.format(h['vni']))
                 commands.append('exit')
                 if interfaces:
                     for i in interfaces:
@@ -265,6 +265,21 @@ def map_obj_to_commands(updates, module):
                         commands.append('vrf member {0}'.format(name))
 
             else:
+                for item in args:
+                    candidate = w.get(item)
+                    if candidate and candidate != obj_in_have.get(item):
+                        cmd = item + ' ' + str(candidate)
+                        commands.append(cmd)
+                if admin_state and admin_state != obj_in_have.get('admin_state'):
+                    if admin_state == 'up':
+                        commands.append('no shutdown')
+                    elif admin_state == 'down':
+                        commands.append('shutdown')
+
+                if commands:
+                    commands.insert(0, 'vrf context {0}'.format(name))
+                    commands.append('exit')
+
                 if interfaces:
                     if not obj_in_have['interfaces']:
                         for i in interfaces:


Code review comments:
diff_hunk: @@ -242,21 +242,21 @@ def map_obj_to_commands(updates, module):
         elif state == 'present':
             if not obj_in_have:
                 commands.append('vrf context {0}'.format(name))
-                if rd and rd != '':
-                    commands.append('rd {0}'.format(rd))
-                if description:
-                    commands.append('description {0}'.format(description))
-                if vni and vni != '':
-                    commands.append('vni {0}'.format(vni))
+                for item in args:
+                    candidate = w.get(item)
+                    if candidate:
+                        cmd = item + ' ' + str(candidate)
+                        commands.append(cmd)
                 if admin_state == 'up':
                     commands.append('no shutdown')
                 elif admin_state == 'down':
                     commands.append('shutdown')
 
                 if commands:
path: lib/ansible/modules/network/nxos/nxos_vrf.py
text: @user1:
Probably not related to this diff but what is the use of this if command when command is already declared so it seems like a  if True type of check.

@author:
Good point. I will remove this check.