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

ansible.builtin.uri doesn't return json dict when python interpreter is python2. #79592

Open
1 task done
stenh0use opened this issue Dec 13, 2022 · 4 comments
Open
1 task done
Labels
affects_2.11 bug This issue/PR relates to a bug. module This issue/PR relates to a module. P3 Priority 3 - Approved, No Time Limitation verified This issue has been verified/reproduced by maintainer

Comments

@stenh0use
Copy link

stenh0use commented Dec 13, 2022

Summary

When upgrading some roles from ansible 2.11.1 to ansible 2.13.7 I discovered a regression in the behavior of the ansible.builtin.uri module.

Requirements for the regression to appear:

  • control host is running ansible 2.13+ with python3
  • target host is running python2 as the default python bin/interpreter
  • uri module / server returns a GET request with the response headers Content-type: application/json and the content is a valid json string, but the response code is not 200

The expected return values in ansible should include the key in the dictionary json with the content loaded. When the above requirements are met this does not happen.

The module documentation states the following about return_content key.

Independently of this option, if the reported Content-type is “application/json”, then the JSON is always loaded into a key called json in the dictionary results.

Issue Type

Bug Report

Component Name

ansible.builtin.uri

Ansible Version

Issue not present: ansible-core==2.11.1
Issue not present: ansible-core==2.12.10

Issue present: 
  - ansible-core==2.13.7
  - ansible-core==2.14.1

Configuration

see molecule / example role.

OS / Environment

Centos7 / Python2 interpreter

Steps to Reproduce

Run the below and see that the bug does not exist in 2.11 when the target host has python2 running

git clone https://github.com/stenh0use/ansible-uri-issue.git
python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements_2.11.1.txt
# see that the json dictionary is returned
molecule converge

Run the below and then see that the bug does exist once upgrading to 2.13+ and the target host has python2 running

rm -rf .venv
python3 -m venv .venv
. .venv/bin/activate
pip install -r requirements_2.13.7.txt
# see that the json dictionary is not returned
molecule converge

Expected Results

I expect the json dict key to be populated even when the response code is not 200, but is in the expected list as per the documentation.

Actual Results

😫 $ molecule converge
INFO     default scenario test matrix: dependency, create, prepare, converge
INFO     Performing prerun with role_name_check=0...
INFO     Set ANSIBLE_LIBRARY=/Users/jstenhouse/.cache/ansible-compat/70e5d7/modules:/Users/jstenhouse/.ansible/plugins/modules:/usr/share/ansible/plugins/modules
INFO     Set ANSIBLE_COLLECTIONS_PATH=/Users/jstenhouse/.cache/ansible-compat/70e5d7/collections:/Users/jstenhouse/alpha/infra/ansible/collections
INFO     Set ANSIBLE_ROLES_PATH=/Users/jstenhouse/.cache/ansible-compat/70e5d7/roles:/Users/jstenhouse/.ansible/roles:/usr/share/ansible/roles:/etc/ansible/roles:/Users/jstenhouse/alpha/infra/ansible/roles:/Users/jstenhouse/alpha/infra/ansible-cloud/roles
INFO     Running default > dependency
WARNING  Skipping, missing the requirements file.
WARNING  Skipping, missing the requirements file.
INFO     Running default > create
WARNING  Skipping, instances already created.
INFO     Running default > prepare
WARNING  Skipping, prepare playbook not configured.
INFO     Running default > converge
INFO     Sanity checks: 'docker'

PLAY [Converge] ****************************************************************

TASK [Gathering Facts] *********************************************************
ok: [centos8]
ok: [centos7]

TASK [uri : ansible.builtin.include_tasks] *************************************
included: /Users/jstenhouse/dev/github/uri/tasks/uri_get.yml for centos7, centos8 => (item=200)
included: /Users/jstenhouse/dev/github/uri/tasks/uri_get.yml for centos7, centos8 => (item=429)
included: /Users/jstenhouse/dev/github/uri/tasks/uri_get.yml for centos7, centos8 => (item=501)
included: /Users/jstenhouse/dev/github/uri/tasks/uri_get.yml for centos7, centos8 => (item=503)

TASK [uri : get json response with code 200] ***********************************
ok: [centos7]
ok: [centos8]

TASK [uri : ansible.builtin.debug] *********************************************
ok: [centos7] => {
    "response": {
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "cookies": {},
        "cookies_string": "",
        "date": "Tue, 13 Dec 2022 19:53:52 GMT",
        "elapsed": 0,
        "failed": false,
        "json": {
            "hello": "world"
        },
        "msg": "OK (20 bytes)",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 200,
        "url": "http://server:5000/200"
    }
}
ok: [centos8] => {
    "response": {
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "cookies": {},
        "cookies_string": "",
        "date": "Tue, 13 Dec 2022 19:53:53 GMT",
        "elapsed": 0,
        "failed": false,
        "json": {
            "hello": "world"
        },
        "msg": "OK (20 bytes)",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 200,
        "url": "http://server:5000/200"
    }
}

TASK [uri : assert json dict exists] *******************************************
ok: [centos7] => {
    "changed": false,
    "msg": "All assertions passed"
}
ok: [centos8] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [uri : ansible.builtin.set_fact] ******************************************
skipping: [centos7]
skipping: [centos8]

TASK [uri : get json response with code 429] ***********************************
ok: [centos7]
ok: [centos8]

TASK [uri : ansible.builtin.debug] *********************************************
ok: [centos7] => {
    "response": {
        "body": "{ \"hello\": \"world\" }",
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:53:55 GMT",
        "elapsed": 0,
        "failed": false,
        "msg": "HTTP Error 429: TOO MANY REQUESTS",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 429,
        "url": "http://server:5000/429"
    }
}
ok: [centos8] => {
    "response": {
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:53:55 GMT",
        "elapsed": 0,
        "failed": false,
        "json": {
            "hello": "world"
        },
        "msg": "HTTP Error 429: TOO MANY REQUESTS",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 429,
        "url": "http://server:5000/429"
    }
}

TASK [uri : assert json dict exists] *******************************************
fatal: [centos7]: FAILED! => {
    "assertion": "response.json is defined",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}
...ignoring
ok: [centos8] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [uri : ansible.builtin.set_fact] ******************************************
ok: [centos7]
skipping: [centos8]

TASK [uri : get json response with code 501] ***********************************
ok: [centos7]
ok: [centos8]

TASK [uri : ansible.builtin.debug] *********************************************
ok: [centos7] => {
    "response": {
        "body": "{ \"hello\": \"world\" }",
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:53:58 GMT",
        "elapsed": 0,
        "failed": false,
        "msg": "HTTP Error 501: NOT IMPLEMENTED",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 501,
        "url": "http://server:5000/501"
    }
}
ok: [centos8] => {
    "response": {
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:53:59 GMT",
        "elapsed": 0,
        "failed": false,
        "json": {
            "hello": "world"
        },
        "msg": "HTTP Error 501: NOT IMPLEMENTED",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 501,
        "url": "http://server:5000/501"
    }
}

TASK [uri : assert json dict exists] *******************************************
fatal: [centos7]: FAILED! => {
    "assertion": "response.json is defined",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}
...ignoring
ok: [centos8] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [uri : ansible.builtin.set_fact] ******************************************
skipping: [centos8]
ok: [centos7]

TASK [uri : get json response with code 503] ***********************************
ok: [centos7]
ok: [centos8]

TASK [uri : ansible.builtin.debug] *********************************************
ok: [centos7] => {
    "response": {
        "body": "{ \"hello\": \"world\" }",
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:54:01 GMT",
        "elapsed": 0,
        "failed": false,
        "msg": "HTTP Error 503: SERVICE UNAVAILABLE",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 503,
        "url": "http://server:5000/503"
    }
}
ok: [centos8] => {
    "response": {
        "changed": false,
        "connection": "close",
        "content_length": "20",
        "content_type": "application/json",
        "date": "Tue, 13 Dec 2022 19:54:02 GMT",
        "elapsed": 0,
        "failed": false,
        "json": {
            "hello": "world"
        },
        "msg": "HTTP Error 503: SERVICE UNAVAILABLE",
        "redirected": false,
        "server": "Werkzeug/2.2.2 Python/3.11.1",
        "status": 503,
        "url": "http://server:5000/503"
    }
}

TASK [uri : assert json dict exists] *******************************************
fatal: [centos7]: FAILED! => {
    "assertion": "response.json is defined",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}
...ignoring
ok: [centos8] => {
    "changed": false,
    "msg": "All assertions passed"
}

TASK [uri : ansible.builtin.set_fact] ******************************************
skipping: [centos8]
ok: [centos7]

TASK [uri : ansible.builtin.debug] *********************************************
ok: [centos7] => {
    "ansible_facts.python_version": "2.7.5"
}
ok: [centos8] => {
    "ansible_facts.python_version": "3.6.8"
}

TASK [uri : ansible.builtin.assert] ********************************************
fatal: [centos7]: FAILED! => {
    "assertion": "not (json_dict_missing | default(false) | bool)",
    "changed": false,
    "evaluated_to": false,
    "msg": "one or more uri responses had a missing json_dict.\nfailed status codes: \"[429, 501, 503]\"\n"
}
ok: [centos8] => {
    "changed": false,
    "msg": "all test cases passed"
}

PLAY RECAP *********************************************************************
centos7                    : ok=21   changed=0    unreachable=0    failed=1    skipped=1    rescued=0    ignored=3   
centos8                    : ok=19   changed=0    unreachable=0    failed=0    skipped=4    rescued=0    ignored=0

WARNING  Retrying execution failure 2 of: ansible-playbook --inventory /Users/jstenhouse/.cache/molecule/uri/default/inventory --skip-tags molecule-notest,notest /Users/jstenhouse/dev/github/uri/molecule/default/converge.yml
CRITICAL Ansible return code was 2, command was: ['ansible-playbook', '--inventory', '/Users/jstenhouse/.cache/molecule/uri/default/inventory', '--skip-tags', 'molecule-notest,notest', '/Users/jstenhouse/dev/github/uri/molecule/default/converge.yml']

Code of Conduct

  • I agree to follow the Ansible Code of Conduct
@ansibot
Copy link
Contributor

ansibot commented Dec 13, 2022

Files identified in the description:

If these files are incorrect, please update the component name section of the description or use the !component bot command.

click here for bot help

@ansibot ansibot added affects_2.11 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. labels Dec 13, 2022
@stenh0use stenh0use changed the title ansible.builtin.uri doesn't return json dict when python interpreter is python2. [regression] ansible.builtin.uri doesn't return json dict when python interpreter is python2. Dec 13, 2022
@stenh0use
Copy link
Author

-label affects_2.11
+label affects_2.13
+label affects_2.14

@sivel
Copy link
Member

sivel commented Dec 13, 2022

Ultimately this is caused by #76386

Oh they joys of supporting all the idiosyncrasies of Py2 and Py3.

Py3: We need to check if r.fp is None because if so that means that the object is partially initialized
Py2: We need to check if r has headers.

I'm thinking we probably need to create a helper called something like has_headers that looks something like:

diff --git a/lib/ansible/modules/uri.py b/lib/ansible/modules/uri.py
index c6b592d940..915f5c5671 100644
--- a/lib/ansible/modules/uri.py
+++ b/lib/ansible/modules/uri.py
@@ -553,6 +553,16 @@ def form_urlencoded(body):
     return body
 
 
+def has_headers(r):
+    if PY3 and r and r.fp:
+        # r may be None for some errors
+        # r.fp may be None depending on the error, which means there are no headers either
+        return True
+    if PY2 and r and hasattr(r, 'headers'):
+        # r.headers may be missing on certain exceptions
+        return True
+    return False
+
+
 def uri(module, url, dest, body, body_format, method, headers, socket_timeout, ca_path, unredirected_headers, decompress,
         ciphers, use_netrc):
     # is dest is set and is a directory, let's check if we get redirected and
@@ -690,9 +700,7 @@ def main():
         filename = get_response_filename(r) or 'index.html'
         dest = os.path.join(dest, filename)
 
-    if r and r.fp is not None:
-        # r may be None for some errors
-        # r.fp may be None depending on the error, which means there are no headers either
+    if has_headers(r):
         content_type, main_type, sub_type, content_encoding = parse_content_type(r)
     else:
         content_type = 'application/octet-stream'

This will require more extensive testing to ensure it doesn't regress the previously mentioned fix, and resolves the issues. Integration tests will need added as well. As far as integration tests are concerned, afaik httpbin doesn't have a means of returning a JSON body along with a status code. I created a simple flask app for a reproducer:

Flask App...

from flask import Flask

app = Flask(__name__)

@app.route('/<int:code>')
def status(code):
    return '{"hello": "world"}', code, {'Content-Type': 'application/json'}

if __name__ == '__main__':
    app.run(debug=True, host='0.0.0.0')

I'm not pursuing this further, as I'm about to be AFK for several weeks.

Also, the affects labels really have no useful meaning about what historic versions it affects. It's more "this issue was filed with X.Y was the most recent". In practice we'd only fix this specific bug for 2.14 and the upcoming 2.15.

@sivel sivel added the verified This issue has been verified/reproduced by maintainer label Dec 13, 2022
@stenh0use
Copy link
Author

stenh0use commented Dec 13, 2022

Yeah, I would love to get rid of python2 from my environment 😞

I've implemented fixes in my ansible roles to work around this via setting return_content to true and then using from_json filter against the content key.

Mainly wanted to raise this issue as no one else had and it took me a good chunk of my day to figure out why some CI jobs were failing with the updated versions.

I also created a simple flask app with molecule testing/tasks https://github.com/stenh0use/ansible-uri-issue.git, although your flask app is neater than mine ;)

As for the labels, I didn't read the bot help properly, turns out they don't work for everyone 😄

@bcoca bcoca changed the title [regression] ansible.builtin.uri doesn't return json dict when python interpreter is python2. ansible.builtin.uri doesn't return json dict when python interpreter is python2. Jan 3, 2023
@bcoca bcoca added P3 Priority 3 - Approved, No Time Limitation and removed needs_triage Needs a first human triage before being processed. labels Jan 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.11 bug This issue/PR relates to a bug. module This issue/PR relates to a module. P3 Priority 3 - Approved, No Time Limitation verified This issue has been verified/reproduced by maintainer
Projects
None yet
Development

No branches or pull requests

4 participants