Fixing -u user issue using network_cli

[ktbyers]

Fixes #161

[ktbyers]

@targuan Do you see any issue with the action plugin changes that I made here?

[targuan]

Hello,

According to ansible source code, the value of remote_user is overwritten by the current user if the connection: local is set, which used to be mandatory to use napalm.

Could you elaborate the context were the current action plugin is not working ? I have been using it for a few years now without any issue. But looking at the the play_context source code, there may be an issue when connection is set to another value than local (network for instance but I don't know if napalm module would work with this one). We may want to use connection_user if available (connection: local) if set and fallback to remote_user if not.

Also, while looking at the eos.py local action, the native plugins seems to still use connection_user as the username when connection: local (well, they actually overwrite remote_user which was overwriten in play_context is set, so I would not use another one to keep the same "look and feel" as the native plugins.

[ktbyers]

Let me test the two main use cases i.e. connection: local and connection: network_cli and document the behavior here.

Yes, I think we might want to do some variation of this:

We may want to use connection_user if available (connection: local) if set and fallback to remote_user if not.

So that connection: local is the expected behavior, but connection: network_cli would also work.

But let me document the behavior.

[ktbyers]

# connection: network_cli 
# ansible 2.9rc3
# develop branch of napalm (does not include this PR)

ansible-playbook napalm_get_1_alt.yml -u pyclass -k -i ./ansible-hosts.ini --limit cisco
SSH password: 

PLAY [NAPALM test] ***************************************************************************************************************************

TASK [NAPALM facts] **************************************************************************************************************************
fatal: [cisco6]: FAILED! => {"changed": false, "msg": "username is required"}
fatal: [cisco5]: FAILED! => {"changed": false, "msg": "username is required"}
fatal: [cisco1]: FAILED! => {"changed": false, "msg": "username is required"}
fatal: [cisco2]: FAILED! => {"changed": false, "msg": "username is required"}

PLAY RECAP ***********************************************************************************************************************************
cisco1                     : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   
cisco2                     : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   
cisco5                     : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   
cisco6                     : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

[ktbyers]

Same setup but connection: local

ansible-playbook napalm_get_1_alt.yml -u pyclass -k -i ./ansible-hosts.ini --limit cisco
SSH password: 

PLAY [NAPALM test] ***************************************************************************************************************************

TASK [NAPALM facts] **************************************************************************************************************************
ok: [cisco5]
ok: [cisco6]
ok: [cisco1]
ok: [cisco2]

PLAY RECAP ***********************************************************************************************************************************
cisco1                     : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
cisco2                     : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
cisco5                     : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   
cisco6                     : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0   

[ktbyers]

Okay, so there is no issue if using ansible_connection=local, but it fails if ansible_connection=network_cli.

I propose we do this (to try to make it work for network_cli so fewer people will have issues/errors with their default inventories):

diff --git a/napalm_ansible/plugins/action/napalm.py b/napalm_ansible/plugins/action/napalm.py
index ac37522..fc01b46 100644
--- a/napalm_ansible/plugins/action/napalm.py
+++ b/napalm_ansible/plugins/action/napalm.py
@@ -8,12 +8,16 @@ class ActionModule(_ActionModule):
     def run(self, tmp=None, task_vars=None):
         pc = self._play_context

-        if hasattr(pc, "connection_user"):  # new in ansible 2.3
+        if hasattr(pc, "connection_user"):
             # populate provider values with context values if not set
             provider = self._task.args.get('provider', {})

             provider['hostname'] = provider.get('hostname', provider.get('host', pc.remote_addr))
-            provider['username'] = provider.get('username', pc.connection_user)
+            username = provider.get('username', pc.connection_user)
+            # Try to make ansible_connection=network_cli also work
+            if not username:
+                username = provider.get('username', pc.remote_user)
+            provider['username'] = username
             provider['password'] = provider.get('password', pc.password)
             # Timeout can't be passed via command-line as Ansible defaults to a 10 second timeout
             provider['timeout'] = provider.get('timeout', 60)

[ktbyers]

Tested using both network_cli and local against Ansible 2.9rc3, 2.8.5, 2.7.13.

[ktbyers]

Updated this PR to reflect this new change.

[ktbyers]

@targuan Okay, let me know what you think about the updated solution. Thanks.

[targuan]

Looks good.