Fixing issues with httpapi (ansible#40388)

* I seem to have forgotten the back half of tests

* Set http timeout from persistent_command_timeout

* Tweak URL generation and provide URL on error

* Push var_options to connection process

* Don't wait forever if coming from persistent

* Don't send the entire contents of variables to ansible-connection

Author: Qalthos

bin/ansible-connection

@@ -69,7 +69,7 @@ class ConnectionProcess(object):
         self.connection = None
         self._ansible_playbook_pid = ansible_playbook_pid
 
-    def start(self):
+    def start(self, variables):
         try:
             messages = list()
             result = {}
@@ -83,7 +83,7 @@ class ConnectionProcess(object):
                 self.play_context.private_key_file = os.path.join(self.original_path, self.play_context.private_key_file)
             self.connection = connection_loader.get(self.play_context.connection, self.play_context, '/dev/null',
                                                     ansible_playbook_pid=self._ansible_playbook_pid)
-            self.connection.set_options()
+            self.connection.set_options(var_options=variables)
             self.connection._connect()
             self.connection._socket_path = self.socket_path
             self.srv.register(self.connection)
@@ -201,10 +201,21 @@ def main():
             init_data += cur_line
             cur_line = stdin.readline()
 
+        cur_line = stdin.readline()
+        vars_data = b''
+
+        while cur_line.strip() != b'#END_VARS#':
+            if cur_line == b'':
+                raise Exception("EOF found before vars data was complete")
+            vars_data += cur_line
+            cur_line = stdin.readline()
+
         if PY3:
             pc_data = cPickle.loads(init_data, encoding='bytes')
+            variables = cPickle.loads(vars_data, encoding='bytes')
         else:
             pc_data = cPickle.loads(init_data)
+            variables = cPickle.loads(vars_data)
 
         play_context = PlayContext()
         play_context.deserialize(pc_data)
@@ -242,7 +253,7 @@ def main():
                         os.close(r)
                         wfd = os.fdopen(w, 'w')
                         process = ConnectionProcess(wfd, play_context, socket_path, original_path, ansible_playbook_pid)
-                        process.start()
+                        process.start(variables)
                     except Exception:
                         messages.append(traceback.format_exc())
                         rc = 1

lib/ansible/executor/task_executor.py

@@ -813,7 +813,9 @@ def _get_connection(self, variables, templar):
             self._play_context.timeout = connection.get_option('persistent_command_timeout')
             display.vvvv('attempting to start connection', host=self._play_context.remote_addr)
             display.vvvv('using connection plugin %s' % connection.transport, host=self._play_context.remote_addr)
-            socket_path = self._start_connection()
+            # We don't need to send the entire contents of variables to ansible-connection
+            filtered_vars = dict((key, value) for key, value in variables.items() if key.startswith('ansible'))
+            socket_path = self._start_connection(filtered_vars)
             display.vvvv('local domain socket path is %s' % socket_path, host=self._play_context.remote_addr)
             setattr(connection, '_socket_path', socket_path)
 
@@ -886,7 +888,7 @@ def _get_action_handler(self, connection, templar):
 
         return handler
 
-    def _start_connection(self):
+    def _start_connection(self, variables):
         '''
         Starts the persistent connection
         '''
@@ -918,8 +920,12 @@ def find_file_in_path(filename):
         # that means only protocol=0 will work.
         src = cPickle.dumps(self._play_context.serialize(), protocol=0)
         stdin.write(src)
-
         stdin.write(b'\n#END_INIT#\n')
+
+        src = cPickle.dumps(variables, protocol=0)
+        stdin.write(src)
+        stdin.write(b'\n#END_VARS#\n')
+
         stdin.flush()
 
         (stdout, stderr) = p.communicate()

lib/ansible/plugins/connection/httpapi.py

@@ -16,7 +16,7 @@
 options:
   host:
     description:
-      - Specifies the remote device FQDN or IP address to establish the SSH
+      - Specifies the remote device FQDN or IP address to establish the HTTP(S)
         connection to.
     default: inventory_hostname
     vars:
@@ -25,14 +25,16 @@
     type: int
     description:
       - Specifies the port on the remote device to listening for connections
-        when establishing the SSH connection.
+        when establishing the HTTP(S) connection.
+        When unspecified, will pick 80 or 443 based on the value of use_ssl
     ini:
       - section: defaults
         key: remote_port
     env:
       - name: ANSIBLE_REMOTE_PORT
     vars:
       - name: ansible_port
+      - name: ansible_httpapi_port
   network_os:
     description:
       - Configures the device platform network operating system.  This value is
@@ -139,6 +141,7 @@
 from ansible.module_utils._text import to_bytes
 from ansible.module_utils.six import PY3
 from ansible.module_utils.six.moves import cPickle
+from ansible.module_utils.six.moves.urllib.error import URLError
 from ansible.module_utils.urls import open_url
 from ansible.playbook.play_context import PlayContext
 from ansible.plugins.loader import cliconf_loader, connection_loader, httpapi_loader
@@ -243,13 +246,13 @@ def _connect(self):
         network_os = self._play_context.network_os
 
         protocol = 'https' if self.get_option('use_ssl') else 'http'
-        host = self._play_context.remote_addr
-        port = self._play_context.port or 443 if protocol == 'https' else 80
+        host = self.get_option('host')
+        port = self.get_option('port') or (443 if protocol == 'https' else 80)
         self._url = '%s://%s:%s' % (protocol, host, port)
 
         self._cliconf = cliconf_loader.get(network_os, self)
         if self._cliconf:
-            display.vvvv('loaded cliconf plugin for network_os %s' % network_os, host=self._play_context.remote_addr)
+            display.vvvv('loaded cliconf plugin for network_os %s' % network_os, host=host)
         else:
             display.vvvv('unable to load cliconf for network_os %s' % network_os)
 
@@ -295,9 +298,17 @@ def send(self, path, data, **kwargs):
         '''
         Sends the command to the device over api
         '''
-        url_kwargs = dict(url_username=self._play_context.remote_user, url_password=self._play_context.password)
+        url_kwargs = dict(
+            url_username=self.get_option('remote_user'), url_password=self.get_option('password'),
+            timeout=self.get_option('timeout'),
+        )
         url_kwargs.update(kwargs)
-        response = open_url(self._url + path, data=data, **url_kwargs)
+
+        try:
+            response = open_url(self._url + path, data=data, **url_kwargs)
+        except URLError as exc:
+            raise AnsibleConnectionFailure('Could not connect to {0}: {1}'.format(self._url, exc.reason))
+
         self._auth = response.info().get('Set-Cookie')
 
         return response

lib/ansible/plugins/connection/persistent.py

@@ -117,8 +117,12 @@ def find_file_in_path(filename):
         # that means only protocol=0 will work.
         src = cPickle.dumps(self._play_context.serialize(), protocol=0)
         stdin.write(src)
-
         stdin.write(b'\n#END_INIT#\n')
+
+        src = cPickle.dumps({}, protocol=0)
+        stdin.write(src)
+        stdin.write(b'\n#END_VARS#\n')
+
         stdin.flush()
 
         (stdout, stderr) = p.communicate()

test/integration/targets/nxos_ntp/tests/common/sanity.yaml

@@ -11,7 +11,6 @@
     vrf_name: management
     source_addr: 5.5.5.5
     state: absent
-    provider: "{{ connection }}"
   ignore_errors: yes
 
 - block:
@@ -24,7 +23,6 @@
       vrf_name: management
       source_addr: 5.5.5.5
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: &true
@@ -47,7 +45,6 @@
       vrf_name: default
       source_addr: default
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -76,7 +73,6 @@
       peer: 1.2.3.4
       prefer: enabled
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -91,7 +87,6 @@
     nxos_ntp: &config3
       source_int: default
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true

test/integration/targets/nxos_ntp_auth/tests/common/sanity.yaml

@@ -8,7 +8,6 @@
     key_id: 32
     md5string: hello
     state: absent
-    provider: "{{ connection }}"
   ignore_errors: yes
 
 - block:
@@ -19,7 +18,6 @@
       md5string: hello
       authentication: off
       state: present
-      provider: "{{ connection }}"      
     register: result
 
   - assert: &true
@@ -32,7 +30,6 @@
       md5string: hello
       authentication: off
       state: absent
-      provider: "{{ connection }}"      
     register: result
 
   - assert: *true
@@ -43,7 +40,6 @@
       md5string: hello
       auth_type: encrypt
       state: present
-      provider: "{{ connection }}"      
     register: result
 
   - assert: *true
@@ -60,7 +56,6 @@
     nxos_ntp_auth: &authon
       authentication: on
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -75,7 +70,6 @@
     nxos_ntp_auth: &authoff
       authentication: off
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -91,7 +85,6 @@
       key_id: 32
       trusted_key: true
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -107,7 +100,6 @@
       key_id: 32
       trusted_key: false
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -125,7 +117,6 @@
       auth_type: encrypt
       authentication: on
       state: absent
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true

test/integration/targets/nxos_ntp_options/tests/common/sanity.yaml

@@ -6,7 +6,6 @@
 - name: "Apply default ntp config" 
   nxos_ntp_options: &default
     state: absent
-    provider: "{{ connection }}"
   ignore_errors: yes
 
 - block:
@@ -16,7 +15,6 @@
       master: true
       logging: true
       state: present
-      provider: "{{ connection }}"      
     register: result
 
   - assert: &true
@@ -36,7 +34,6 @@
       master: true
       stratum: 10
       state: present
-      provider: "{{ connection }}"      
     register: result
 
   - assert: *true
@@ -53,7 +50,6 @@
       stratum: 10
       logging: false
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true
@@ -69,7 +65,6 @@
       master: false
       logging: true
       state: present
-      provider: "{{ connection }}"
     register: result
 
   - assert: *true

test/integration/targets/nxos_nxapi/tests/cli/configure.yaml

@@ -7,21 +7,18 @@
 - name: Setup - put NXAPI in stopped state
   nxos_nxapi:
     state: absent
-    provider: "{{ cli }}"
 
 - name: Configure NXAPI
   nxos_nxapi:
     enable_http: no
     enable_sandbox: "{{nxapi_sandbox_option|default(omit)}}"
     enable_https: yes
     https_port: 9443
-    provider: "{{ cli }}"
   register: result
 
 - nxos_command:
     commands:
       - show nxapi | json
-    provider: "{{ cli }}"
   register: result
 
 - include: targets/nxos_nxapi/tasks/platform/n7k/assert_changes.yaml
@@ -39,7 +36,6 @@
     enable_sandbox: "{{nxapi_sandbox_option|default(omit)}}"
     enable_https: yes
     https_port: 9443
-    provider: "{{ cli }}"
   register: result
 
 - name: Assert configuration is idempotent

test/integration/targets/nxos_nxapi/tests/cli/disable.yaml

@@ -4,14 +4,12 @@
 - name: Disable NXAPI
   nxos_nxapi:
     state: absent
-    provider: "{{ cli }}"
   register: result
 
 - name: Check NXAPI state
   nxos_command:
     commands:
       - show feature | grep nxapi
-    provider: "{{ cli }}"
   register: result
 
 - name: Assert NXAPI is disabled
@@ -23,7 +21,6 @@
   nxos_nxapi:
     state:
       absent
-    provider: "{{ cli }}"
   register: result
 
 - name: Assert idempotence