nagios-charmers team mailing list archive

[Paul Goins <paul.goins@xxxxxxxxxxxxx>]

Review: Needs Fixing

Minor changes desired, plus a few general comments.

Diff comments:

> diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
> index bbbb8ac..5b21c1c 100755
> --- a/hooks/upgrade_charm.py
> +++ b/hooks/upgrade_charm.py
> @@ -58,6 +67,54 @@ def warn_legacy_relations():
>                  "WARNING")
>  
>  
> +def parse_extra_contacts(yaml_string):
> +    """Parses a list of extra Nagios contacts from a YAML string.
> +
> +    Does basic sanitization only
> +    """
> +    # Final result
> +    extra_contacts = []
> +
> +    # Valid characters for contact names
> +    valid_name_chars = 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()):
> +                hookenv.log('Contact {} is missing fields.'.format(contact),
> +                            hookenv.WARNING)
> +                continue
> +
> +            if set(contact['name']) >= set(valid_name_chars):

Corner case, but this will actually fail if the set of characters in the name is _equal_ to the valid name chars.  In other words, the ">=" here should be a ">".

> +                hookenv.log('Contact name {} is illegal'.format(contact['name']),
> +                            hookenv.WARNING)
> +                continue
> +
> +            if '\n' in (contact['host'] + contact['service']):
> +                hookenv.log('Line breaks not allowed in commands', hookenv.WARNING)
> +                continue
> +
> +            contact['name'] = contact['name'].lower()
> +            contact['alias'] = contact['name'].capitalize()
> +            extra_contacts.append(contact)
> +
> +    except (ValueError, yaml.error.YAMLError) as e:
> +        hookenv.log('Invalid "extra_contacts" configuration: {}'.format(e),
> +                    hookenv.WARNING)
> +    if len(extra_contacts_raw) != len(extra_contacts):
> +        hookenv.log(
> +            'Invalid extra_contacts config, found {} contacts defined, '
> +            'only {} were valid, check unit logs for '
> +            'detailed errors'.format(len(extra_contacts_raw), len(extra_contacts))
> +        )
> +
> +    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():
> @@ -200,10 +258,51 @@ def enable_traps_config():
>  
>  
>  def update_contacts():
> +    # Multiple Email Contacts
> +    admin_members = ''
> +    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(1)
> +    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 = []
> +        for email in admin_email:
> +            contact_name = email.replace('@', '').replace('.','').lower()
> +            contact_alias = contact_name.capitalize()
> +            contacts.append({
> +                'contact_name': contact_name,
> +                'alias': contact_alias,
> +                'email': email
> +            })
> +
> +        admin_members = ', '.join([
> +            c['contact_name'] for c in contacts
> +        ])
> +
>      resulting_members = contactgroup_members
> +    if admin_members:
> +        # if multiple admin emails are passed ignore contactgroup_members
> +        resulting_members = admin_members
>  
>      if forced_contactgroup_members:
> -        resulting_members = resulting_members + ',' + ','.join(forced_contactgroup_members)
> +        resulting_members = resulting_members + ',' + ','.join(
> +            forced_contactgroup_members)

NOTE: No change required.

Alternate way to write this which _might_ be clearer:
resulting_members = ",".join([resulting_members] + forced_contactgroup_members)

> +
> +    # Parse extra_contacts
> +    extra_contacts = parse_extra_contacts(hookenv.config('extra_contacts'))
>  
>      template_values = {'admin_service_notification_period': hookenv.config('admin_service_notification_period'),
>                         'admin_host_notification_period': hookenv.config('admin_host_notification_period'),
> @@ -360,6 +460,23 @@ def update_cgi_config():
>  # note: i tried to use cheetah, and it barfed, several times. It can go play
>  # in a fire. I'm jusing jinja2.
>  def update_apache():
> +    """

I feel like, while these SSL changes may be needed, they seem unrelated to the stated purpose of this MR of adding support for extra_contacts.  My preference would have been for the SSL-related changes to be part of their own MR.

Given where the overall review is at this point, I'll let that go, but I feel I do need to note this for the sake of easing future reviews.

> +    Nagios3 is deployed as a global apache application from the archive.
> +    We'll get a little funky and add the SSL keys to the default-ssl config
> +    which sets our keys, including the self-signed ones, as the host keyfiles.
> +    """
> +
> +    # Start by Setting the ports.conf
> +
> +    with open('hooks/templates/ports-cfg.jinja2', 'r') as f:
> +        templateDef = f.read()

PEP8 suggests templatedef or template_def rather than templateDef (i.e. lowercase rather than camelCase)

> +    t = Template(templateDef)
> +    ports_conf = '/etc/apache2/ports.conf'
> +
> +    with open(ports_conf, 'w') as f:
> +        f.write(t.render({'enable_http': HTTP_ENABLED}))
> +
> +    # Next setup the default-ssl.conf
>      if os.path.exists(chain_file) and os.path.getsize(chain_file) > 0:
>          ssl_chain = chain_file
>      else:


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