Merge ~xavpaice/charm-nrpe:nrpe-address into ~nrpe-charmers/charm-nrpe:master

[Xav Paice]

This is to work around 1832671 - it's not a fix, but a handy addition for when we need it.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Wouter van Bommel (woutervb)]

For the moment this is handy, it will allow nagios servers to monitor, even if they are not part of the juju model

review: Approve

[Chris Sanders (chris.sanders)]

> For the moment this is handy, it will allow nagios servers to monitor, even if
> they are not part of the juju model

For a temporary fork this seems ok, but I tend to think this isn't the correct fix for the upstream charm. Wouldn't the correct fix here to be implement support for spaces so that relations work as expected rather than add a configuration to work without relations?

Is the root cause something that can't be fixed due to some limitation?

review: Needs Information

[Xav Paice (xavpaice)]

the limitation is that an openstack instance doesn't have any idea at all of it's floating IP address - only the cloud provider can tell Juju that - but the instance is what provides the list of addresses for the relation to Nagios. In this case, what's happening is that somehow the floating IP address of the Nagios unit is getting pumped into the relation data across the cross model relation, but not the floating IP of the nrpe unit - and that means the wrong address is added to nrpe.cfg. It should be fixable, I just am at a bit of a loss as to how right now.

Meantime, this change would allow us to have a non-related Nagios allowed for checks in addition to any related units - that might be handy for the future.

I've not checked if the change of nagios_address_type from private to public needs the relation removing and re-adding to trigger any change. Also, we might be able to use nagios_master rather than add a new config option.

There's a fork for this at cs:~xavpaice/nrpe at this point in time, to allow the site affected to be monitored.

[Stuart Bishop (stub)]

charmhelpers.core.hookenv.ingress_address(relid, charmhelpers.core.hookenv.local_unit()) provides the IP address that remote units in that particular relation should connect to the local unit. Juju maintains this, and should always be correct per the spaces, public ip addresses etc. chosen at deploy time.

[Xav Paice (xavpaice)]

great, thanks for that! I'll dig that a bit further wrt the bug for this.

[Tom Haddon (mthaddon)]

I'll mark this MP as work in progress for now. Please flip back to "Needs Review" when you're ready to progress, or let us know if it can be deleted and will be progressed in a different MP.

[Xav Paice (xavpaice)]

I think we can abandon this change, the bug was fixed in https://bugs.launchpad.net/charm-nrpe/+bug/1827703

review: Disapprove

Unmerged commits

7a45b98... by Xav Paice on 2019-06-13

add config param for Nagios hosts we cannot specify via relation

diff --git a/config.yaml b/config.yaml
index 44a6680..ffdc1c4 100644
--- a/config.yaml
+++ b/config.yaml
@@ -179,3 +179,9 @@ options:
         dmesg history length to check for xfs errors, in minutes
         .
         Defaults to disabled, set the time to enable.
+  nagios_hosts:
+    default: ""
+    type: string
+    description:
+      Additional Nagios hosts that should be allowed to query NRPE.  This
+      is not required if the the application is related to Nagios.
diff --git a/hooks/nrpe_helpers.py b/hooks/nrpe_helpers.py
index c3adc6e..8a103d9 100644
--- a/hooks/nrpe_helpers.py
+++ b/hooks/nrpe_helpers.py
@@ -161,6 +161,8 @@ class MonitorsRelation(helpers.RelationContext):
         if not hookenv.relation_ids(self.name):
             return
         addresses = [self.ingress_address(info) for info in self['monitors']]
+        if hookenv.config('nagios_hosts'):
+            addresses += hookenv.config('nagios_hosts').split(",")
         self['monitor_allowed_hosts'] = ','.join(addresses)
 
     def provide_data(self):