New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance mail notifications scripts and add support for command line parameters #5170

Merged
merged 3 commits into from May 10, 2017

Conversation

Projects
None yet
4 participants
@sysadmama
Copy link
Contributor

sysadmama commented Apr 17, 2017

…of ENV variables, giving the ability to integrate with Icinga Director

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented Apr 18, 2017

Thanks 👍

Review for @Al2Klimov

Tests

  • Linux (Debian/Ubuntu LTS, Centos/Fedora, SLES/OpenSUSE)
  • FreeBSD

Verify whether the notification works as expected.

@dnsmichi dnsmichi added the ITL label Apr 18, 2017

@dnsmichi dnsmichi added this to the 2.7.0 milestone Apr 18, 2017

@@ -62,11 +68,16 @@ template Notification "mail-host-notification" {
template Notification "mail-service-notification" {
command = "mail-service-notification"

states = [ OK, Warning, Critical, Unknown ]
states = [ Critical, OK, Unknown, Warning ]

This comment has been minimized.

@Al2Klimov

Al2Klimov Apr 18, 2017

Contributor

What in the world is this change for?

This comment has been minimized.

@sysadmama

sysadmama Apr 24, 2017

Contributor

That's the result of many many changes that ended up in "don't commit additional scripts, but replace the old ones". You can revert it, if you want (or I can).

This comment has been minimized.

@dnsmichi

dnsmichi Apr 28, 2017

Member

Minor thing, we'll handle this, thanks.

@Al2Klimov

This comment has been minimized.

Copy link
Contributor

Al2Klimov commented Apr 24, 2017

@sysadmama Please could you modify your commit like this?

--- etc/icinga2/scripts/mail-host-notification.sh                                                                                                                                            
+++ etc/icinga2/scripts/mail-host-notification.sh                                                                                                                                            
@@ -79 +79 @@ When?    $LONGDATETIME                                                                                                                                                         
-Host?    $HOSTALIAS (aka "$HOSTDISPLAYNAME)                                                                                                                                                 
+Host?    $HOSTALIAS (aka "$HOSTDISPLAYNAME")
@Al2Klimov

This comment has been minimized.

Copy link
Contributor

Al2Klimov commented Apr 25, 2017

@dnsmichi OSes successfully tested so far:

  • Debian 8
  • Ubuntu 16.04
  • CentOS 7
  • Fedora 25
  • SLES 12
  • openSUSE 42.2
  • FreeBSD 11

Would you also like some other versions?

  • Debian 7
  • Ubuntu 14.04
  • CentOS 6
  • Fedora 24 (no need)
  • SLES 11
  • openSUSE 42.1
  • FreeBSD 10 (no need)
@Al2Klimov

This comment has been minimized.

Copy link
Contributor

Al2Klimov commented Apr 25, 2017

How I tested

  • built packages
  • installed icinga2-bin
  • configured:
icinga2 feature enable command
perl -pi -e $'s/\\bicinga\\@localhost\\b/alexander.klimov\\@netways.de/g' /etc/icinga2/conf.d/users.conf
  • and...
echo "[$(date $'+%s')] SEND_CUSTOM_HOST_NOTIFICATION;$(hostname);3;aklimov;IT WORX" >/var/run/icinga2/cmd/icinga2.cmd
echo "[$(date $'+%s')] SEND_CUSTOM_SVC_NOTIFICATION;$(hostname);ping6;3;aklimov;IT WORX" >/var/run/icinga2/cmd/icinga2.cmd
***** Icinga 2 Host Monitoring on i2bsd *****

==> i2bsd (i2bsd) is DOWN! <==

Info?    execvpe(/usr/lib/nagios/plugins/check_ping) failed: No such file or directory

When?    2017-04-25 10:48:39 +0200
Host?    i2bsd (aka "i2bsd)
IPv4?	 127.0.0.1
IPv6?	 ::1

Comment by aklimov:
  IT WORX
***** Icinga 2 Service Monitoring on i2bsd *****

==> ping6 on i2bsd is UNKNOWN! <==

Info?    execvpe(/usr/lib/nagios/plugins/check_ping) failed: No such file or directory

When?    2017-04-25 10:48:45 +0200
Service? ping6 (aka "ping6")
Host?    i2bsd (aka "i2bsd")
IPv4?	 127.0.0.1
IPv6?    ::1

Comment by aklimov:
  IT WORX
Info? $HOSTOUTPUT
When? $LONGDATETIME
Host? $HOSTALIAS (aka "$HOSTDISPLAYNAME)

This comment has been minimized.

@dnsmichi

dnsmichi Apr 28, 2017

Member

Seems that there's a closing double quote missing.

This comment has been minimized.

@dnsmichi

dnsmichi Apr 28, 2017

Member

Nevermind, didn't see the other commit.

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented Apr 28, 2017

Thanks @Al2Klimov, we'll do a final review.

@dnsmichi dnsmichi assigned dnsmichi and unassigned Al2Klimov Apr 28, 2017

dnsmichi added a commit that referenced this pull request Apr 28, 2017

Add notification command parameters to the default mail notification …
…script

Replace the previous notification scheme - now using getops in place of ENV
variables, giving the ability to integrate with the Icinga Director.

refs #5170

Signed-off-by: Michael Friedrich <michael.friedrich@icinga.com>

dnsmichi added a commit that referenced this pull request Apr 28, 2017

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented Apr 28, 2017

I've rebased and squashed the patch into feature/notification-scripts

The following fixes are applied in a separate commit:

  • Replace ? with : in info listings, replace aka with Display Name:
  • I'd suggest to go for HOSTNAME instead of HOSTALIAS, that's a bug for years now
  • IPv4 used tabs instead of spaces
  • Some phrases and naming

Open for discussion @gunnarbeutner

  • verbose vs sendtosyslog parameter naming
  • Usage() shouldn't call exit(1) but leave that to the caller (help must call exit(0)).
  • Add more object attributes to the mail body (or at least add some commented examples)

Documentation is still missing, that's a TODO.

@Crunsher Crunsher force-pushed the Icinga:master branch from 90a16d6 to 7c70d51 May 2, 2017

@dnsmichi dnsmichi changed the title Replace the previous notification scheme - now using getops in place … Enhance mail notifications scripts and add support for command line parameters May 8, 2017

sysadmama and others added some commits Apr 5, 2017

Add notification command parameters to the default mail notification …
…script

Replace the previous notification scheme - now using getops in place of ENV
variables, giving the ability to integrate with the Icinga Director.

refs #5170

Signed-off-by: Michael Friedrich <michael.friedrich@icinga.com>
@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented May 9, 2017

The script also does not check for required parameters. This leads to errors thrown by the mail binary, not the script itself, which is irritating. We should capture that, I'll wrap my head around some script modifications.

@dnsmichi dnsmichi force-pushed the sysadmama:notification-scripts branch from 9622bdf to 7b9f478 May 10, 2017

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented May 10, 2017

I've squashed the original patch, added 2 review commits by myself and force pushed your repository branch. That allows for easily merging this PR.

I've added some function helpers to print help and errors. Further the scripts now check for required params to avoid errors thrown by the mail binary. The rest is more or less cosmetic changes.

Thanks a lot for the patches 👍

@dnsmichi dnsmichi merged commit ebdbca0 into Icinga:master May 10, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

dnsmichi added a commit that referenced this pull request May 24, 2017

Fix verbose mode in notifications scripts
Otherwise it just spams the syslog on frequent notifications

refs #5170

dnsmichi added a commit that referenced this pull request May 24, 2017

Merge pull request #5286 from Icinga/fix/notification-scripts-log
Fix verbose mode in notifications scripts

#5170
@gvde

This comment has been minimized.

-a option only works with bsd-mailx not with heirloom-mailx which is the implementation on RHEL/CentOS.

-a on CentOS attaches a file and -r sets the from-addr.

This comment has been minimized.

Copy link
Member

dnsmichi replied May 29, 2017

Is there any reliable way to test and selectively chose the correct one?

@Al2Klimov you've tested this feature extensively, can you confirm that behaviour?

This comment has been minimized.

Copy link

gvde replied May 29, 2017

If it's only to set the from address using the option "-r" should work with either variant.

This comment has been minimized.

Copy link
Contributor

Al2Klimov replied May 30, 2017

@dnsmichi No.

@dnsmichi

This comment has been minimized.

Copy link
Member

dnsmichi commented May 29, 2017

Thanks, please continue over in #5299.

dnsmichi added a commit to sysadmama/icinga2 that referenced this pull request Jun 16, 2017

@sysadmama sysadmama deleted the sysadmama:notification-scripts branch Jun 19, 2017

Crunsher added a commit that referenced this pull request Jul 28, 2017

@dnsmichi dnsmichi added the enhancement label Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment