nagios-charmers team mailing list archive

[Stuart Bishop <stuart.bishop@xxxxxxxxxxxxx>]

Review: Needs Fixing

Code looks fine, but there are some inline comments that will need to be addressed.

Is this change useful as is? Looking at the examples, extra binaries are needed, which makes me think adding in the extra contacts is a job for a subordinate charm. The charm has no support for installing extra deb packages or snaps, and I suspect for real world deployments it is expected that someone manually copies the custom notification binaries into place (in which case, why bother with extending the charm at all when we can just cowboy the config changes at the same time we inject the needed binaries). Will need a ~nagios-charmer to help here.

Diff comments:

> diff --git a/config.yaml b/config.yaml
> index 4ce0f18..88d03b8 100644
> --- a/config.yaml
> +++ b/config.yaml
> @@ -105,7 +105,7 @@ options:
>          type: string
>          default: pageroot@localhost
>          description: |

Can you change this from a | to a > while you are here? Your editor is trimming whitespace, which puts you on the hook ;)

> -            Email address used for the admin pager, used by $ADMINPAGER$ in 
> +            Email address used for the admin pager, used by $ADMINPAGER$ in
>              notification commands.
>      enable_pagerduty:
>          type: boolean
> @@ -168,7 +168,7 @@ options:
>          type: string
>          description: |

And here too, as this string will render better with normalized whitespace rather.

>              A string to pass to the Nagios load monitoring command.  Default is
> -            to report warning at 5.0, 4.0 and 3.0 averages, critical at 10.0, 
> +            to report warning at 5.0, 4.0 and 3.0 averages, critical at 10.0,
>              6.0 and 4.0.
>      pagerduty_notification_levels:
>          default: u,c,r
> @@ -206,4 +206,21 @@ options:
>              u - Unknown
>              w - Warning
>              o - OK
> -
> +    extra_contacts:
> +        default: ''
> +        type: string
> +        description: |

This | is correct though.

> +            Define extra contacts for Nagios. They will be appended in the
> +            default administrators contact group. This is useful for adding
> +            notification integrations with external services.
> +            Contacts are passed as a YAML array, and must have a contact name,
> +            a command to be executed for host notifications and a command to
> +            be executed for service notifications. These commands may use all
> +            relevant Nagios macros ($HOSTNAME$, $SERVICENAME$, etc). Refer to
> +            the Nagios documentation for more information.
> +            Only [A-Za-z0-9_-] contact names are valid. Be careful with
> +            commands, since they are not sanitized.
> +            Example
> +            - name: contact_name_1
> +              host: /custom/command/for/host $HOSTNAME$
> +              service: /custom/command/for/service $SERVICENAME$
> diff --git a/hooks/upgrade-charm b/hooks/upgrade-charm
> index 19e736c..94eaaa2 100755
> --- a/hooks/upgrade-charm
> +++ b/hooks/upgrade-charm
> @@ -52,6 +53,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):
> +    # Final result
> +    extra_contacts = []
> +
> +    # Valid characters for contact names
> +    valid_name_chars = (
> +        'abcdefghijklmnopqrstuvwxyz'
> +        'ABCDEFGHIJKLMNOPQRSTUVWXYZ'
> +        '0123456789_-'
> +    )
> +
> +    try:
> +        extra_contacts_raw = yaml.load(yaml_string, Loader=yaml.Loader) or []

We want safe_load here, or Loader=yaml.SafeLoader.

Since you are doing all the error checking, you should probably start by catching any yaml.error.YAMLError exceptions raised from safe_load; a common problem with this style of configuration is people putting in invalid YAML or JSON.

> +        if not isinstance(extra_contacts_raw, list):
> +            raise ValueError('not a list')
> +
> +        required_fields = ['name', 'host', 'service']
> +        for contact in extra_contacts_raw:
> +            if {'name', 'host', 'service'} > set(contact.keys()):
> +                if log:
> +                    hookenv.log(
> +                        'Contact {} is missing fields.'.format(contact))
> +                continue
> +
> +            if not (set(contact['name']) < set(valid_name_chars)):
> +                if log:
> +                    hookenv.log(
> +                        'Contact name {} is illegal'.format(contact['name']))
> +                continue
> +
> +            if '\n' in (contact['host'] + contact['service']):
> +                if log:
> +                    hookenv.log('Line breaks not allowed in commands')
> +                continue
> +
> +            extra_contacts.append(contact)
> +
> +    except ValueError as e:
> +        if log:
> +            hookenv.log(
> +                'Invalid "extra_contacts" configuration: {}'.format(e))

Rather than log an error most people will never see, it would be better for this helper to raise a documented exception. The call site can catch it, and set a blocked workload state.

> +
> +    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():


-- 
https://code.launchpad.net/~aggkolaitis/nagios-charm/+git/nagios-charm/+merge/374074
Your team Nagios Charm developers is requested to review the proposed merge of ~aggkolaitis/nagios-charm:extra_contacts into nagios-charm:master.