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

Red Hat Satellite subscription module #52578

Open
wants to merge 12 commits into
base: devel
from
@@ -0,0 +1,86 @@
# -*- coding: utf-8 -*-

# Copyright: (c) 2018, Luc Stroobant <luc.stroobant@wdc.com>
# GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)

from __future__ import absolute_import, division, print_function
__metaclass__ = type

try:
import requests

This comment has been minimized.

Copy link
@sivel

sivel Feb 21, 2019

Member

Just as a heads up. We will not accept this change switching to requests. requests is a blacklisted import and we require use of open_url.

I see the conversation below, but that is not sufficient.

Requests has too many versions that may not be compatible, there are likely to be issues with fullfilling pinned dependencies, and this adds an extra non-necessary dependency.

This comment has been minimized.

Copy link
@stroobl

stroobl Feb 21, 2019

Author

In that case the module (and specially the reporting module that we also want to contribute) becomes unusable for our use case. I can't spend on rewriting with the internal Ansible url modules if that bug is not resolved.

This comment has been minimized.

Copy link
@dagwieers

dagwieers Feb 21, 2019

Member

@sivel Maybe we should have a check for this in Shippable CI, as I see more and more modules using requests.

[dag@moria ansible.git]$ grep -rlE '(import requests|from requests)' lib/ansible | wc -l
35

This comment has been minimized.

Copy link
@sivel

sivel Feb 21, 2019

Member

We do actually, it's done in validate-modules. However validate-modules doesn't do a verification in module_utils. A code-smell may not be bad. However there is also strange logic for checking this, as a few modules snuck in after the re-combining core and extras into the main repo, when validate-modules had some broken functionality. So we would have to add skip lists.

There are also a few modules which by extension of another dependency use requests, such as some cloud modules, so they import to get access to exceptions.

from requests.auth import HTTPBasicAuth
requests.packages.urllib3.disable_warnings()
requests_available = True
This conversation was marked as resolved by stroobl

This comment has been minimized.

Copy link
@dagwieers

dagwieers Feb 20, 2019

Member

We tend to use HAS_REQUESTS as a standard way of doing this, but...

We actually prefer that modules use lib/ansible/module_utils/urls.py, specifically fetch_url() or open_url() for anything HTTP/REST based.

This comment has been minimized.

Copy link
@stroobl

stroobl Feb 21, 2019

Author

Ah, on that. I started with the built-in url modules at first. It has a serious performance issue, are you aware of that? I found out when running my Satellite reporting module (which executes 3-4000 requests to fetch info about all our hosts).
For every request you make, the module creates a temp file where it creates a single file with a bundle of all CA's on your system (about 1.4MB big in my case). After 1024 requests the process is killed since the user can not open any more files. It's here in the code:
https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/urls.py#L710

Since all tasks are delegated to the Ansible controller and requests also makes some other things (like the management of the params) easier, I think it should be acceptable to use it here.

This comment has been minimized.

Copy link
@dagwieers

dagwieers Feb 21, 2019

Member

Whoaw, I knew there was an impact, but did not suspect this. This is ugly.
cc @kbreit

Opened a ticket for this. #52717

except ImportError:
requests_available = False


def host_list(sat_url, user, password, org, validate_certs, content_host=None):
"""Get a list of hosts from the satellite"""

apicall = "{0}/api/hosts".format(sat_url)
params = dict(
organization_id=org,
sort_by="name",
sort_order="ASC"
)
if content_host:
if '=' in content_host:
# this is a already a search query, don't search for hostnames
params["search"] = str(content_host)
else:
# treat other patterns as hostnames
params["search"] = "name={0}".format(content_host)

hosts = request(apicall, user, password, validate_certs, True, params)

return hosts


def request(url, user, password, validate_certs=True, pagination=False, params=None):
""""Execute a Satellite GET API request"""

per_page = 200
if not params:
params = dict()
if params != {} or pagination:
params["page"] = 1
params["paged"] = True
params["per_page"] = per_page

items = []
done = 0
req = requests.get(url, timeout=60, verify=validate_certs, params=params,
auth=HTTPBasicAuth(user, password))
req.raise_for_status()
response = req.json()

done += per_page
# pagination: check if we need more requests
if pagination:
items.extend(response["results"])
if response["subtotal"]:
while done < response["subtotal"]:
response = None
params["page"] += 1
req = requests.get(url, timeout=60, verify=validate_certs, params=params,
auth=HTTPBasicAuth(user, password))
req.raise_for_status()
response = req.json()
items.extend(response["results"])
done += per_page
# not pagination: just return the response
else:
return response

return items


def put(url, user, password, data, validate_certs=True):
""""Execute a Satellite PUT API request"""

req = requests.put(url, timeout=60, verify=validate_certs, json=data,
auth=HTTPBasicAuth(user, password))
req.raise_for_status()

return req.json()
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.