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$" [email protected] service: /usr/bin/sendmail -s "alert for $SERVICENAME$" [email protected] - 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. -- Mailing list: https://launchpad.net/~nagios-charmers Post to : [email protected] Unsubscribe : https://launchpad.net/~nagios-charmers More help : https://help.launchpad.net/ListHelp
