nagios-charmers team mailing list archive

[Drew Freiberger <drew.freiberger@xxxxxxxxxxxxx>]

Review: Needs Fixing

Suggest a slight rework of the extra_contacts validation function to allow for broken names and to find a way to block on truly bad configs.  See inline comments.

Diff comments:

> diff --git a/config.yaml b/config.yaml
> index 5221c7a..2e85997 100644
> --- a/config.yaml
> +++ b/config.yaml
> @@ -290,3 +289,21 @@ options:
>          description: |
>              Defines the IP or Host Name to send snmp traps to. Leave blank (empty) to disable
>              the traps functionality.
> +    extra_contacts:
> +        default: ''
> +        type: string
> +        description: |
> +            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$

would be nice to provide an additional, more useful, working examples like the following
- name: opsteam_email
  host: /usr/bin/sendmail -s "alert for $HOSTNAME$" opsteam@xxxxxxxxxxxxx
  service: /usr/bin/sendmail -s "alert for $SERVICENAME$" opsteam@xxxxxxxxxxxxx
- name: snmp_collector
  host: /path/to/snmp/helper ....
  service: /path/to/snmp/helper ....

Otherwise "Name" is misleading to me, the layperson, thinking it's "Drew Freiberger" and not "contact_mechanism_identifier"

> +              service: /custom/command/for/service $SERVICENAME$
> diff --git a/hooks/templates/contacts-cfg.tmpl b/hooks/templates/contacts-cfg.tmpl
> index f182fcf..1b34c3b 100644
> --- a/hooks/templates/contacts-cfg.tmpl
> +++ b/hooks/templates/contacts-cfg.tmpl
> @@ -31,6 +31,39 @@ define contact{
>          }
>  {% endfor %}
>  
> +###############################################################################
> +###############################################################################
> +#
> +# EXTRA CONTACTS
> +#
> +###############################################################################
> +###############################################################################
> +
> +{% for contact in extra_contacts %}
> +
> +define contact{
> +        contact_name                    {{ contact.name }}
> +        alias                           {{ contact.alias }}
> +        service_notification_period     24x7
> +        host_notification_period        24x7
> +        service_notification_options    w,u,c,r

might be nice to optionally parameterize these for contacts, with wucr being the fallback default if not specified, but that may be for a future request, since this code relies upon only the three fields now to provide proper validation.

> +        host_notification_options       d,r
> +        service_notification_commands   notify-service-by-{{ contact.name }}
> +        host_notification_commands      notify-host-by-{{ contact.name }}
> +        }
> +
> +
> +define command {
> +        command_name                    notify-service-by-{{ contact.name }}
> +        command_line                    {{ contact.service }}
> +}
> +
> +define command {
> +        command_name                    notify-host-by-{{ contact.name }}
> +        command_line                    {{ contact.host }}
> +}
> +
> +{% endfor %}
>  
>  ###############################################################################
>  ###############################################################################
> diff --git a/hooks/upgrade_charm.py b/hooks/upgrade_charm.py
> index bbbb8ac..c97315a 100755
> --- a/hooks/upgrade_charm.py
> +++ b/hooks/upgrade_charm.py
> @@ -46,10 +49,19 @@ contactgroup_members = hookenv.config("contactgroup-members")
>  # it will be changed by functions
>  forced_contactgroup_members = []
>  
> +HTTP_ENABLED = ssl_config not in ["only"]
> +SSL_CONFIGURED = ssl_config in ["on", "only"]

for historical config support, given that we used to allow True False for this variable, we should also allow for in 'true'
SSL_CONFIGURED = ssl_config in ["on", "only", "true"]

> +
> +
>  # Checks the charm relations for legacy relations
>  # Inserts warnings into the log about legacy relations, as they will be removed
>  # in the future

this comment block should be removed in favor of the docstring added.

>  def warn_legacy_relations():
> +    """Checks the charm relations for legacy relations.
> +
> +    Inserts warnings into the log about legacy relations, as they will be removed
> +    in the future
> +    """
>      if legacy_relations is not None:
>          hookenv.log("Relations have been radically changed."
>                      " The monitoring interface is not supported anymore.",
> @@ -58,6 +70,45 @@ def warn_legacy_relations():
>                  "WARNING")
>  
>  
> +# Parses a list of extra Nagios contacts from a YAML string
> +# Does basic sanitization only

this should be converted to a proper docstring for the function.

> +def get_extra_contacts(yaml_string):

This should be called {parse|validate|filter}_extra_contacts, as it's mainly doing validation and sanitization, not fetching the value.

> +    # 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),

see comment in next block about name maybe not needing to be a required param

> +                            hookenv.WARNING)
> +                continue

Need to set a counter or append the contact to a "failed contact validation" list for reporting below (see comment in the except block below)

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

I feel like name is just an identifier and if the one passed is invalid or missing, I feel like substituting a generated UUID would work just as well if it is missing or invalid.

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

Need to set a counter or append the contact to a "failed contact validation" list for reporting below (see comment in the except block below)

> +            contact['name'] = contact['name'].lower()

a space before this block would be super helpful for reading

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

Any contacts being "continue"d past should be flagged and noted with the charm status state noting "invalid extra_contacts config, found len(extra_contacts_raw) contacts defined, only implemented len(extra_contacts), check unit logs for detailed errors"

Perhaps this is as simple as
if len(extra_contacts_raw) != len(extra_contacts):
    "$block_handler_here"
Unfortunately, it's quite unlikely this triggers in upgrade-charm as the definition of the config option likely doesn't yet exist.

> +
> +    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():
> diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py
> index fafe7f0..c14d60b 100644
> --- a/tests/functional/conftest.py
> +++ b/tests/functional/conftest.py
> @@ -62,29 +61,34 @@ async def current_model():
>  @pytest.fixture
>  async def get_app(model):
>      """Return the application requested."""
> +

these blank lines should go away, unless that's our new black settings.

>      async def _get_app(name):
>          try:
>              return model.applications[name]
>          except KeyError:
>              raise JujuError("Cannot find application {}".format(name))
> +

ditto

>      return _get_app
>  
>  
>  @pytest.fixture
>  async def get_unit(model):
>      """Return the requested <app_name>/<unit_number> unit."""
> +
>      async def _get_unit(name):
>          try:
>              (app_name, unit_number) = name.split('/')
>              return model.applications[app_name].units[unit_number]
>          except (KeyError, ValueError):
>              raise JujuError("Cannot find unit {}".format(name))
> +
>      return _get_unit
>  
>  
>  @pytest.fixture
>  async def get_entity(model, get_unit, get_app):
>      """Return a unit or an application."""
> +
>      async def _get_entity(name):
>          try:
>              return await get_unit(name)
> @@ -104,6 +109,7 @@ async def run_command(get_unit):
>      :param cmd: Command to be run
>      :param target: Unit object or unit name string
>      """

This doc string should move into the function.

> +
>      async def _run_command(cmd, target):
>          unit = (
>              target
> @@ -123,10 +130,12 @@ async def file_stat(run_command):
>      :param path: File path
>      :param target: Unit object or unit name string
>      """

this docstring should move into the function

> +
>      async def _file_stat(path, target):
>          cmd = STAT_FILE % path
>          results = await run_command(cmd, target)
>          return json.loads(results['Stdout'])
> +
>      return _file_stat
>  
>  
> @@ -138,16 +147,19 @@ async def file_contents(run_command):
>      :param path: File path
>      :param target: Unit object or unit name string
>      """

ditto

> +
>      async def _file_contents(path, target):
>          cmd = 'cat {}'.format(path)
>          results = await run_command(cmd, target)
>          return results['Stdout']
> +
>      return _file_contents
>  
>  
>  @pytest.fixture
>  async def reconfigure_app(get_app, model):
>      """Apply a different config to the requested app."""
> +
>      async def _reconfigure_app(cfg, target):
>          application = (
>              target
> @@ -157,15 +169,18 @@ async def reconfigure_app(get_app, model):
>          await application.set_config(cfg)
>          await application.get_config()
>          await model.block_until(lambda: application.status == 'active')
> +
>      return _reconfigure_app
>  
>  
>  @pytest.fixture
>  async def create_group(run_command):
>      """Create the UNIX group specified."""
> +

again, blank lines should be removed unless this is black

>      async def _create_group(group_name, target):
>          cmd = "sudo groupadd %s" % group_name
>          await run_command(cmd, target)
> +
>      return _create_group
>  
>  


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