4
\$\begingroup\$

I am attempting to learn Python. It was suggested to me to try a web scraper, so I thought to get myself to look at multiple Nagios instances. I have not programmed in Python before, but learned from other code reviews to adapt my standards to PEP8. The online checker says the code passes.

from bs4 import BeautifulSoup
import requests
from requests.auth import HTTPDigestAuth
from requests.auth import HTTPBasicAuth
import urllib.request
import time

allNagiosInfo = [
    ('http://example.com/nagios', 'user', 'password', 'Basic'),
    ('https://network2.example.com/nagios', 'user', 'password', 'Digest'),
    ('https://anothernetwork.example.com/nagios', 'user', 'password', 'Basic')]

for nagiosEntry in allNagiosInfo:
    nagiosBaseURL = nagiosEntry[0]
    nagiosUsername = nagiosEntry[1]
    nagiosPassword = nagiosEntry[2]
    nagiosAuthType = nagiosEntry[3]
    # print("Current values: {}\t{}\t{}\t{}".format(
    #      nagiosBaseURL, nagiosUsername, nagiosPassword, nagiosAuthType))

    if nagiosAuthType == "Basic":
        response = requests.get(nagiosBaseURL + "/cgi-bin/status.cgi?host=all",
                                auth=HTTPBasicAuth(nagiosUsername,
                                                   nagiosPassword))
    if nagiosAuthType == "Digest":
        response = requests.get(nagiosBaseURL + "/cgi-bin/status.cgi?host=all",
                                auth=HTTPDigestAuth(nagiosUsername,
                                                    nagiosPassword))

    html = BeautifulSoup(response.text, "html.parser")
    for i, items in enumerate(html.select('td')):
        if i == 3:
            hostsAll = items.text.split('\n')
            hostsUp = hostsAll[12]
            hostsDown = hostsAll[13]
            hostsUnreachable = hostsAll[14]
            hostsPending = hostsAll[15]
            hostsProblems = hostsAll[24]
            hostsTypes = hostsAll[25]
        if i == 12:
            serviceAll = items.text.split('\n')
            serviceOK = serviceAll[13]
            serviceWarning = serviceAll[14]
            serviceUnknown = serviceAll[15]
            serviceCritical = serviceAll[16]
            serviceProblems = serviceAll[26]
            serviceTypes = serviceAll[27]
        # print(i, items.text) ## To get the index and text
    print("Nagios Server at {}@{}:".format(nagiosUsername, nagiosBaseURL))
    print("    Hosts")
    print("\tUp          - {}".format(hostsUp))
    print("\tDown        - {}".format(hostsDown))
    print("\tUnreachable - {}".format(hostsUnreachable))
    print("\tPending     - {}".format(hostsPending))
    print("\tProblems    - {}".format(hostsProblems))
    print("\tTypes       - {}".format(hostsTypes))

    print("    Services")
    print("\tOK          - {}".format(serviceOK))
    print("\tWarning     - {}".format(serviceWarning))
    print("\tUnknown     - {}".format(serviceUnknown))
    print("\tCritical    - {}".format(serviceCritical))
    print("\tProblems    - {}".format(serviceProblems))
    print("\tTypes       - {}".format(serviceTypes))
    # print("Request returned:\n\n{}".format(html.text))
    # To get the full request

Currently, it outputs my three instances properly:

Nagios Server at user@http://example.com/nagios:
    Hosts
        Up          - 24
        Down        - 0
        Unreachable - 0
        Pending     - 0
        Problems    - 0
        Types       - 24
    Services
        OK          - 142
        Warning     - 0
        Unknown     - 0
        Critical    - 0
        Problems    - 0
        Types       - 142
Nagios Server at user@https://network2.example.com/nagios:
    Hosts
        Up          - 3
        Down        - 0
        Unreachable - 0
        Pending     - 0
        Problems    - 0
        Types       - 3
    Services
        OK          - 28
        Warning     - 0
        Unknown     - 0
        Critical    - 0
        Problems    - 0
        Types       - 28
Nagios Server at user@https://anothernetwork.example.com/nagios:
    Hosts
        Up          - 56
        Down        - 0
        Unreachable - 0
        Pending     - 0
        Problems    - 0
        Types       - 56
    Services
        OK          - 130
        Warning     - 0
        Unknown     - 0
        Critical    - 0
        Problems    - 0
        Types       - 130

I understand I do not have error checking (i.e. is the username/password correct? Auth Type correct? Server URL right?), so that will be my next thing to learn. I also understand I should not have variables (especially with passwords) hard-coded into the code, but I do not know what is the "proper" way of dealing with secure variables.

\$\endgroup\$
3
\$\begingroup\$

First of all, as per PEP8 I'd recommend you follow the best practices.

Naming conventions

  • allNagiosInfo -> this is most likely a constant, so: ALL_NAGIOS_INFO
  • nagiosEntry, nagiosBaseURL, nagiosUsername, nagiosPassword, nagiosAuthType -> nagios_entry, nagios_base_url, nagios_username, nagios_password, nagios_auth_type.

And so on with the naming, you got the idea.

Summary:

Variable names should be lowercase, with words separated by underscores as necessary to improve readability.

Imports

Imports should be grouped in the following order:

  • Standard library imports.
  • Related third party imports.
  • Local application/library specific imports.
  • You should put a blank line between each group of imports.

Also, don't import modules that you're not using (e.g: time, urllib). So, that means you could rewrite your imports as follows:

import requests
from requests.auth import HTTPBasicAuth, HTTPDigestAuth
from bs4 import BeautifulSoup

This:

nagios_base_url = nagios_entry[0]
nagios_username = nagios_entry[1]
nagios_password = nagios_entry[2]
nagios_auth_type = nagios_entry[3]

Can be rewritten as:

nagios_base_url, nagios_username, nagios_password, nagios_auth_type = nagios_entry

Since all the requests that you're doing are suffixed with the same url, I'd just define that before the ifs:

# ...
for nagios_entry in ALL_NAGIOS_INFO:
    nagios_base_url, nagios_username, nagios_password, nagios_auth_type = nagios_entry
    full_url = "{}/cgi-bin/status.cgi?host=all".format(nagios_base_url)

    if nagios_auth_type == "Basic":
        response = requests.get(full_url, auth=HTTPBasicAuth(nagios_username, nagios_password))

    if nagios_auth_type == "Digest":
        response = requests.get(full_url, auth=HTTPDigestAuth(nagios_username, nagios_password))

You have way too many prints which can be rewritten as a single print:

print("""Nagios Server at {}@{}: 

Hosts:

    Up          - {}
    Down        - {}
    Unreachable - {}
    Pending     - {}
    Problems    - {}
    Types       - {}

Services:

    OK          - {}
    Warning     - {}
    Unknown     - {}
    Critical    - {}
    Problems    - {}
    Types       - {}
""".format(
        nagios_username, nagios_base_url, hosts_up, hosts_down, hosts_unreachable, hosts_pending,
        hosts_problems, hosts_types, service_ok, service_warning, service_unknown, service_critical,
        service_problems, service_types
    )
)

You can also look at textwrap.dedent if you want to play around with the \ts.


Security

As you said, it's not recommended to keep your passwords in the code. There's no perfect solution for this, but we can enumerate some of the most used ones:

  • config files which have to be added to .gitignore so they get excluded from the repo
  • environment variables (mostly used on Unix based distros)
  • some powerful hashing technique to generate a master password (master password -> secure key-> encrypt data by the key)

Scraping

When scraping things, you can usualy go a specific element by using an xpath or a selector. I usually use the lxml module when I have to quickly put together something because it allows me to use xpaths (though bs might as well offer that - didn't actually play with that for a while now)

Example for lxml usage

from lxml import html
import requests

res = requests.get(some_url)
tree = html.fromstring(res.content)
tds = tree.xpath('//table//td[@class="plaintext"]/text()[normalize-space()]')

Now, your code is not very portable and if you plan on adding more servers, you'll find yourself having a hard time integrating those as well. The first thing that I'd do is to find the proper data structure for my data. In your scenario, a dictionary looks like the best choice out there:

NAGIOS_DATA = {
    'http://example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Basic'
    },
    'https://network2.example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Digest'
    },
    'https://anothernetwork.example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Basic'
    },
}

INFO: don't forget that the password can be defined in a config file and imported here

Now, that's debatable if it's the best structure that you can use because I don't know the entire logic of your app, but it'll do for now.

Using the above, we can now do:

import requests
from requests.auth import HTTPBasicAuth, HTTPDigestAuth
from bs4 import BeautifulSoup


NAGIOS_DATA = {
    'http://example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Basic'
    },
    'https://network2.example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Digest'
    },
    'https://anothernetwork.example.com/nagios/cgi-bin/status.cgi?host=all': {
        'user': 'user',
        'password': 'password',
        'auth_type': 'Basic'
    },
}


def get_url_response(url, user, password, auth_type):
    """Get the response from a URL.

    Args:
        url (str): Nagios url
        user (str): Nagios username
        password (str): Nagios password
        auth_type (str): Nagios auth_type

    Returns: Response object
    """

    if auth_type == "Basic":
        return requests.get(url, auth=HTTPBasicAuth(user, password)) 
    return requests.get(url, auth=HTTPDigestAuth(user, password))


def main():
    """
    Main entry to the program.
    """

    for url, auth_data in NAGIOS_DATA.items():
        user, password, auth_type = auth_data["user"], auth_data["password"], auth_data["auth_type"]
        response = get_url_response(url, user, password, auth_type)

        if response.status_code == 200:
            html = BeautifulSoup(response.text, "html.parser")
            # ... and so on


if __name__ == '__main__':
    main()

I haven't finished the whole review because I want you to go ahead and do the rest. Some things that I've added to the program:

  • functions: writing functions will let you reuse the functionality they contain at a later point + it's easier to test the code inside them. Homework: could you think of a way of splitting them even further?
  • docstrings: having docstrings within a function will tell others (and most important, the future-you) what that function does.
\$\endgroup\$
  • 2
    \$\begingroup\$ Note, many put spaces between the different import groups, due to isort - the industry standard import hinter. Also the big print would be nicer as an f-string or with explicit format names. \$\endgroup\$ – Peilonrayz Jan 23 at 11:46
  • \$\begingroup\$ Thanks for the detailed review! I have it working with a couple new functions now, naming convention used, and the dictionary for variables. One question though, if I find out how to import settings from a config file, would the NAGIOS_DATA still be a constant? \$\endgroup\$ – Canadian Luke Jan 23 at 21:14
  • \$\begingroup\$ @CanadianLuke Yep. \$\endgroup\$ – Grajdeanu Alex. Jan 24 at 13:58
  • \$\begingroup\$ Awesome, got a config file going now! Thanks! Should I post a new question for further review? \$\endgroup\$ – Canadian Luke Jan 24 at 18:44
  • \$\begingroup\$ @CanadianLuke Yep \$\endgroup\$ – Grajdeanu Alex. Jan 24 at 18:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.