nagios-charmers team mailing list archive

[Joe Guo <joe.guo@xxxxxxxxxxxxx>]

Review: Needs Fixing

Hi Giuseppe:

Added some inline comments, most of them are non-blocker.

Diff comments:

> diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
> index bbbb8ac..cc20507 100755
> --- a/hooks/upgrade_charm.py
> +++ b/hooks/upgrade_charm.py
> @@ -58,6 +69,53 @@ def warn_legacy_relations():
>                  "WARNING")
>  
>  
> +# Parses a list of extra Nagios contacts from a YAML string
> +# Does basic sanitization only
> +def get_extra_contacts(yaml_string, log=False):

personal opinion: the log param seems pointless here.

> +    # Final result
> +    extra_contacts = []
> +
> +    # Valid characters for contact names
> +    valid_name_chars = (
> +        'abcdefghijklmnopqrstuvwxyz'
> +        'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
> +        '0123456789_-'
> +    )

non-blocker: this could be:  string.ascii_letters + string.digits + '_-'

> +
> +    try:
> +        extra_contacts_raw = yaml.load(yaml_string, Loader=yaml.SafeLoader) or []
> +        if not isinstance(extra_contacts_raw, list):
> +            raise ValueError('not a list')
> +
> +        for contact in extra_contacts_raw:
> +            if {'name', 'host', 'service'} > set(contact.keys()):
> +                if log:
> +                    hookenv.log(
> +                        'Contact {} is missing fields.'.format(contact))

should be `contact.name`, although contact will also work, as a dict.
log level should be a WARNING or ERROR.

> +                continue
> +
> +            if set(contact['name']) >= set(valid_name_chars):

`>=` --> `>`
equal should be fine here

> +                if log:
> +                    hookenv.log(
> +                        'Contact name {} is illegal'.format(contact['name']))

log level should be a WARNING or ERROR.

> +                continue
> +
> +            if '\n' in (contact['host'] + contact['service']):
> +                if log:
> +                    hookenv.log('Line breaks not allowed in commands')

log level should be a WARNING or ERROR.

> +                continue
> +            contact['name'] = contact['name'].lower()
> +            contact['alias'] = contact['name'].capitalize()
> +            extra_contacts.append(contact)
> +
> +    except (ValueError, yaml.error.YAMLError) as e:
> +        if log:
> +            hookenv.log(
> +                'Invalid "extra_contacts" configuration: {}'.format(e))

log level should be a WARNING or ERROR.

> +
> +    return extra_contacts
> +
> +
>  # If the charm has extra configuration provided, write that to the
>  # proper nagios3 configuration file, otherwise remove the config
>  def write_extra_config():
> @@ -204,6 +262,40 @@ def update_contacts():
>  
>      if forced_contactgroup_members:
>          resulting_members = resulting_members + ',' + ','.join(forced_contactgroup_members)
> +    # Multiple Email Contacts
> +    contacts = []
> +    admin_email = list(
> +        filter(None, set(hookenv.config('admin_email').split(',')))
> +    )
> +    if len(admin_email) == 0:
> +        hookenv.log("admin_email is unset, this isn't valid config")
> +        hookenv.status_set("blocked", "admin_email is not configured")
> +        exit(0)

personal opinion: maybe use `return` instead of `exit` here?
I know that will introduce some other adjustment, but exit in the middle of a util function seems not a good practice to me.

> +    hookenv.status_set("active", "ready")
> +    if len(admin_email) == 1:
> +        hookenv.log("Setting one admin email address '%s'" % admin_email[0])
> +        contacts = [{
> +            'contact_name': 'root',
> +            'alias': 'Root',
> +            'email': admin_email[0]
> +        }]
> +    elif len(admin_email) > 1:
> +        hookenv.log("Setting %d admin email addresses" % len(admin_email))
> +        contacts = [
> +            {
> +                'contact_name': email.lower(),
> +                'alias': email.capitalize(),
> +                'email': email
> +            }
> +            for email in admin_email
> +        ]
> +        resulting_members = ', '.join([
> +            c['contact_name'] for c in contacts
> +        ])

This whole admin_email section looks weird to me:
1) that filter+None+set+line is too fancy
2) why we have to call the only admin `root`?
3) we only allow [a-zA-Z_-] for extra_contact name, but here we use email, which has invalid chars like @ or dot. Not a issue but inconsistent.

Also, for current code, I will prefer to write it like this:

admin_email = hookenv.config('admin_email')
if not admin_email:
    ...
    return # or exit

if ',' in admin_email:
    admin_email = {email for email in admin_email.split(',') if email}
    ...
else:
    ...

> +
> +    # Parse extra_contacts
> +    extra_contacts = get_extra_contacts(
> +        hookenv.config('extra_contacts'), log=True)
>  
>      template_values = {'admin_service_notification_period': hookenv.config('admin_service_notification_period'),
>                         'admin_host_notification_period': hookenv.config('admin_host_notification_period'),
> @@ -371,27 +481,60 @@ def update_apache():
>          templateDef = f.read()
>  
>      t = Template(templateDef)
> -    with open('/etc/apache2/sites-available/default-ssl.conf', 'w') as f:
> +    ssl_conf = '/etc/apache2/sites-available/default-ssl.conf'
> +    with open(ssl_conf, 'w') as f:
>          f.write(t.render(template_values))
> -    print("Value of ssl is %s" % ssl)
> -    if ssl_config == "only":
> -        subprocess.call(['a2dissite', 'default'])
> -        hookenv.close_port(80)
> -        subprocess.call(['a2ensite', 'default-ssl'])
> -        subprocess.call(['a2enmod', 'ssl'])
> -    elif ssl_config == "on":
> -        subprocess.call(['a2ensite', 'default-ssl'])
> -        subprocess.call(['a2enmod', 'ssl'])
> -        hookenv.open_port(443)
> -    else:
> -        subprocess.call(['a2dissite', 'default-ssl'])
> -        hookenv.close_port(443)
> -        subprocess.call(['a2ensite', 'default'])
> -        hookenv.open_port(80)
>  
> +    # Create directory for extra *.include files installed by subordinates
> +    try:
> +        os.makedirs('/etc/apache2/vhost.d/')

For python>=3.2: os.makedirs(path, exist_ok=True)

> +    except OSError:
> +        pass
> +
> +    # Configure the behavior of http sites
> +    sites = glob.glob("/etc/apache2/sites-available/*.conf")
> +    non_ssl = set(sites) - {ssl_conf}
> +    for each in non_ssl:
> +        site = os.path.basename(each).strip('.conf')

'config.conf'.strip('.conf') --> 'ig'
NOTE: rstrip is not right either: 'my_conf.conf'.rstrip('.conf') --> 'my_'
correct way in my opinion:
site = os.path.basename(each).rsplit(sep='.', maxsplit=1)[0]

> +        Apache2Site(site).action(enabled=HTTP_ENABLED)
> +
> +    # Configure the behavior of https site
> +    Apache2Site("default-ssl").action(enabled=SSL_CONFIGURED)
> +
> +    # Finally, restart apache2
>      host.service_reload('apache2')
>  
>  
> +class Apache2Site:
> +    def __init__(self, site):
> +        self.site = site
> +        self.is_ssl = 'ssl' in site.lower()
> +        self.port = 443 if self.is_ssl else 80
> +
> +    def action(self, enabled):
> +        fn = self._enable if enabled else self._disable
> +        return fn()

return self._enable() if enabled else self._disabled()

> +
> +    def _call(self, args):
> +        try:
> +            subprocess.check_output(args, stderr=subprocess.STDOUT)
> +        except subprocess.CalledProcessError as e:
> +            hookenv.log("Apache2Site: `{}`, returned {}, stdout:\n{}"
> +                        .format(e.cmd, e.returncode, e.output), "ERROR")
> +
> +    def _enable(self):
> +        hookenv.log("Apache2Site: Enabling %s..." % self.site, "INFO")
> +        self._call(['a2ensite', self.site])
> +        if self.port == 443:
> +            self._call(['a2enmod', 'ssl'])
> +        hookenv.open_port(self.port)
> +
> +    def _disable(self):
> +        hookenv.log("Apache2Site: Disabling %s..." % self.site, "INFO")
> +        self._call(['a2dissite', self.site])
> +        hookenv.close_port(self.port)
> +
> +
>  def update_password(account, password):
>      """Update the charm and Apache's record of the password for the supplied account."""
>      account_file = ''.join(['/var/lib/juju/nagios.', account, '.passwd'])
> diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py
> index 2e1cab6..ede914d 100644
> --- a/tests/functional/test_config.py
> +++ b/tests/functional/test_config.py
> @@ -112,3 +124,21 @@ async def test_pager_duty(unit, enable_pagerduty, file_stat):
>      )
>      stat = await file_stat('/etc/nagios3/conf.d/pagerduty_nagios.cfg', unit.u)
>      assert stat['size'] != 0, "pagerduty_config wasn't a non-zero sized file"
> +
> +
> +async def test_extra_contacts(auth, unit, set_extra_contacts):
> +    contancts_url = "http://%s/cgi-bin/nagios3/config.cgi?"; \
> +                    "type=contacts" % unit.u.public_address
> +    contact_name = set_extra_contacts
> +    r = requests.get(contancts_url, auth=auth)
> +    assert r.status_code == 200, "Get Nagios config request failed"
> +    assert r.text.find(contact_name), "Nagios is not loading the extra contact."
> +    assert r.text.find(contact_name.capitalize()), "Contact name alias is not " \

`str.find` will return substr index, which could be 0. Use `in` for safe.

> +                                                   "the capitalized name."
> +    contactgroups_url = "http://%s/cgi-bin/nagios3/config.cgi"; \
> +                        "?type=contactgroups" % unit.u.public_address
> +
> +    r = requests.get(contactgroups_url, auth=auth)
> +    assert r.status_code == 200, "Get Nagios config request failed"
> +    assert r.text.find(contact_name), "Extra contact is not " \

`str.find` will return substr index, which could be 0. Use `in` for safe.

> +                                      "added to the contact groups."
> diff --git a/tests/functional/test_deploy.py b/tests/functional/test_deploy.py
> index 9a9e067..867a527 100644
> --- a/tests/functional/test_deploy.py
> +++ b/tests/functional/test_deploy.py
> @@ -25,13 +25,12 @@ async def test_hosts_being_monitored(auth, unit):
>      host_url = ("http://%s/cgi-bin/nagios3/status.cgi?";
>                  "hostgroup=all&style=hostdetail") % unit.u.public_address
>      r = requests.get(host_url, auth=auth)
> -    assert r.text.find('mysql') and r.text.find('mediawiki'), \
> -        "Nagios is not monitoring the hosts it supposed to."
> +    assert r.text.find('mysql'), "Nagios is not monitoring the hosts it supposed to."

`str.find` will return substr index, which could be 0. Use `in` for safe.

>  
>  
>  async def test_nrpe_monitors_config(relatives, unit, file_contents):
>      # look for disk root check in nrpe config
> -    mysql_unit = relatives['mysql'].units[0]
> +    mysql_unit = relatives['mysql']['app'].units[0]
>      contents = await file_contents(
>              '/etc/nagios/nrpe.d/check_disk_root.cfg',
>              mysql_unit


-- 
https://code.launchpad.net/~peppepetra86/charm-nagios/+git/charm-nagios/+merge/388119
Your team Nagios Charm developers is subscribed to branch charm-nagios:master.