search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Install Salt from the Salt package repositories here:
https://docs.saltproject.io/salt/install-guide/en/latest/
Apache License 2.0
14.2k stars 5.48k forks source link

Tornado minion http proxy connection issues #55192

Closed Oloremo closed 3 years ago

Oloremo commented 5 years ago

Description of Issue

There is an ability in Salt to sent minion wide http proxy: https://github.com/saltstack/salt/pull/29322 https://github.com/saltstack/salt/pull/43764

Which supports both setting proxy_host and no_proxy args. https://docs.saltstack.com/en/latest/ref/configuration/minion.html#proxy-host https://docs.saltstack.com/en/latest/ref/configuration/minion.html#no-proxy

Setting this args will affect every module and state which is using a utils.http since it uses tornado by default and tornado is aware of those proxy options.

The problem is that if you set both proxy_host and no_proxy option and use it in the same execution run - it won't work with a weird bug.

Setup

Consider setup minion so it will go via http proxy everywhere except the 127.0.0.1 - which is a very normal approach.

Steps to Reproduce Issue

Now try to run this state in this order:

test_http_source:
  file.managed:
    - name: /tmp/test.txt
    - source: https://sentry.io
    - skip_verify: True

get_agent_info:
  module.run:
    - consul.agent_self:
      - consul_url: http://127.0.0.1:8500

file.managed will work just fine and download the contents of the url to the file via proxy. But consul agent which will execute a consul.get module which itself will execute the very same utils.http lib which will execute the very same tornado - will fail will the weird error:

'consul.agent_info' failed: initialize() got an unexpected keyword argument 'max_body_size'

The problem part of the utils/http.py is 

        if proxy_host and proxy_port:
            if HAS_CURL_HTTPCLIENT is False:
                ret['error'] = ('proxy_host and proxy_port has been set. This requires pycurl and tornado, '
                                'but the libraries does not seem to be installed')
                log.error(ret['error'])
                return ret

            tornado.httpclient.AsyncHTTPClient.configure('tornado.curl_httpclient.CurlAsyncHTTPClient')
            client_argspec = salt.utils.args.get_function_argspec(
                    tornado.curl_httpclient.CurlAsyncHTTPClient.initialize)
        else:
            client_argspec = salt.utils.args.get_function_argspec(
                    tornado.simple_httpclient.SimpleAsyncHTTPClient.initialize)
        supports_max_body_size = 'max_body_size' in client_argspec.args

As you can see we switch from SimpleAsyncHTTPClient to CurlAsyncHTTPClient only in case of using a proxy.

So file.managed which goes outside via proxy so is using a CurlAsyncHTTPClient and it supports_max_body_size is set to False in that case.

After that, we go to consul which is uses a connection to 127.0.0.1 which is set in no_proxy arg, so we fall into else statement and use SimpleAsyncHTTPClient and supports_max_body_size is True.

Yet we get the TypeError: initialize() got an unexpected keyword argument 'max_body_size' So I kinda assume the following:

  1. It's not related to consul, it's related to proxy logic vs non-proxy logic
  2. Error is triggered by a specific order. You need to run proxy connection first
  3. It feels like the non-proxy connection re-use the proxy socket\connection\object but with wrong arguments this time.

Versions Report

Salt Version:
           Salt: 2019.2.2

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 3.6.8 (default, Aug  7 2019, 17:28:10)
   python-gnupg: 0.4.5
         PyYAML: 3.11
          PyZMQ: 18.0.2
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.3.1

System Versions:
           dist: centos 7.5.1804 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-862.3.2.el7.x86_64
         system: Linux
        version: CentOS Linux 7.5.1804 Core
Oloremo commented 5 years ago

Originally it was reported here: https://github.com/saltstack/salt/issues/43798

But stale bot closed it.

Oloremo commented 5 years ago

@mrproper any chance you understand what is happening here?

Ch3LL commented 5 years ago

im wondering if what is going on is this tornado.httpclient.AsyncHTTPClient.configure('tornado.curl_httpclient.CurlAsyncHTTPClient') is setting the subclass of asynchttpclient for both calls for some reason.

Does this help at all?:

diff --git a/salt/utils/http.py b/salt/utils/http.py
index c75ca73a85..09449f6c11 100644
--- a/salt/utils/http.py
+++ b/salt/utils/http.py
@@ -529,6 +529,7 @@ def query(url,
             client_argspec = salt.utils.args.get_function_argspec(
                     tornado.curl_httpclient.CurlAsyncHTTPClient.initialize)
         else:
+            tornado.httpclient.AsyncHTTPClient.configure(None)
             client_argspec = salt.utils.args.get_function_argspec(
                     tornado.simple_httpclient.SimpleAsyncHTTPClient.initialize)
Oloremo commented 5 years ago

@Ch3LL I tried that in the linked issue: https://github.com/saltstack/salt/issues/43798#issuecomment-548572871

The answer is - yes, but not for all cases for some reason.

Oloremo commented 5 years ago

but not for all cases for some reason.

Clarification: This patch helps for the tests case highlighted above but don't help for my real orchestration which does proxy and non-proxy requests many times. So it seems the problem is a bit more complicated.

Ch3LL commented 5 years ago

can you share a use case for when that patch does not work so I can dive in?

Oloremo commented 5 years ago

@Ch3LL Hm, let me try to create a more complex example, but the original one is still valid and I think the root cause will be the same.

Oloremo commented 5 years ago

@Ch3LL Ok I managed to find a simplified reproducible example.

1) Setup a http proxy, I use tinyproxy 2) define it minion and master configs like:

proxy_host: IP_OF_PROXY
proxy_port: PORT_OF_PROXY
proxy_username: ""
proxy_password: ""
no_proxy: [ '127.0.0.1', 'localhost' ]

Create SLS for tests 55192.sls:

dir.delete:
  file.absent:
    - name: /tmp/salt

extract.salt:
  archive.extracted:
    - name: /tmp/salt
    - source: https://github.com/saltstack/salt/releases/download/v2019.2.2/salt-2019.2.2.tar.gz
    - skip_verify: True
    - keep_source: False
    - trim_output: 1

query_example:
  http.query:
    - name: http://127.0.0.1:80
    - status: 200

55192-orch.sls

orch:
  salt.state:
    - tgt: '*'
    - sls:
      - 55192

Run orchestration salt-run state.orch 55192-orch

N.B. You have to run orchestration, it salt-call was affected too initially but the patch above kinda fixed that.

Oloremo commented 5 years ago

@Ch3LL Do you need any more details of the provided info is enough?

Oloremo commented 5 years ago

@Ch3LL Any updates on this?..

ymasson commented 4 years ago

Hi,

I'm facing the same issue (more or less). https://github.com/saltstack/salt/issues/55426

Simple example:

Test 1:

root@2a25eb5dded6:/# salt-call --local http.query https://www.google.com
local:
    ----------
    body:
            [........]

Test 2:

campbellmc commented 4 years ago

Hi,

I'm facing the same issue (more or less).

55426

Simple example:

  • start a Debian 10 container
  • install salt-minion (Python 3)

Test 1:

root@2a25eb5dded6:/# salt-call --local http.query https://www.google.com
local:
    ----------
    body:
            [........]

Test 2:

  • Add proxy_host and proxy_port in minion configuration and restart it
root@2a25eb5dded6:/# salt-call --local http.query https://www.google.com
[ERROR   ] An un-handled exception was caught by salt's global exception handler:
NameError: name 'AsyncHTTPClient' is not defined
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 431, in salt_call
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 138, in run
    ret = self.call()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 237, in call
    ret['return'] = self.minion.executors[fname](self.opts, data, func, args, kwargs)
  File "/usr/lib/python3/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/modules/http.py", line 41, in query
    return salt.utils.http.query(url=url, opts=opts, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/utils/http.py", line 537, in query
    AsyncHTTPClient.configure('CurlAsyncHTTPClient')
NameError: name 'AsyncHTTPClient' is not defined
Traceback (most recent call last):
  File "/usr/bin/salt-call", line 11, in <module>
    salt_call()
  File "/usr/lib/python3/dist-packages/salt/scripts.py", line 431, in salt_call
    client.run()
  File "/usr/lib/python3/dist-packages/salt/cli/call.py", line 57, in run
    caller.run()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 138, in run
    ret = self.call()
  File "/usr/lib/python3/dist-packages/salt/cli/caller.py", line 237, in call
    ret['return'] = self.minion.executors[fname](self.opts, data, func, args, kwargs)
  File "/usr/lib/python3/dist-packages/salt/executors/direct_call.py", line 12, in execute
    return func(*args, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/modules/http.py", line 41, in query
    return salt.utils.http.query(url=url, opts=opts, **kwargs)
  File "/usr/lib/python3/dist-packages/salt/utils/http.py", line 537, in query
    AsyncHTTPClient.configure('CurlAsyncHTTPClient')
NameError: name 'AsyncHTTPClient' is not defined

This doesn't look like the same issue. Once you start using proxies - it will force salt to use tornado.curl_httpclient rather than tornado.simple_httpclient. Are you sure you all the deps installed in your Debian container? Curl is not installed by-default on Deb, from memory. Tornado must be there already for salt to work. What about pycurl?

ymasson commented 4 years ago

Ok, it is not exactly the same issue. I have opened #55426 . Sorry for the noise in this one.

python3-tornado4 is a salt-minion dependency. And python3-pycurl is installed.

Ch3LL commented 4 years ago

confirmed with @Oloremo that this patch fixes the issue:

diff --git a/salt/utils/http.py b/salt/utils/http.py
index c75ca73a85..f6351c0f55 100644
--- a/salt/utils/http.py
+++ b/salt/utils/http.py
@@ -516,6 +516,8 @@ def query(url,
         if urlparse(url_full).hostname in no_proxy:
             proxy_host = None
             proxy_port = None
+            proxy_username = None
+            proxy_password = None

         # We want to use curl_http if we have a proxy defined
         if proxy_host and proxy_port:
@@ -529,6 +531,7 @@ def query(url,
             client_argspec = salt.utils.args.get_function_argspec(
                     tornado.curl_httpclient.CurlAsyncHTTPClient.initialize)
         else:
+            tornado.httpclient.AsyncHTTPClient.configure(None)
             client_argspec = salt.utils.args.get_function_argspec(
                     tornado.simple_httpclient.SimpleAsyncHTTPClient.initialize)

Going to work with him to write some tests for the fix.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

stale[bot] commented 4 years ago

Thank you for updating this issue. It is no longer marked as stale.

sagetherage commented 3 years ago

Closing and fixed in Aluminium release