Merge ~xavpaice/charm-nrpe:lp1937888 into charm-nrpe:master

[Xav Paice]

Bind NRPE to local address

    In cases where we specify to use public address and/or Juju returns a
    public address regardless of the setting, we cannot use that address for
    binding the NRPE service.

    This change ensures get_local_ingress_address returns the local address
    for binding the nrpe service, and if configured to do so returns the
    public address for relation data and NRPE host exports.

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

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

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:1332acc124eeb785c714fc78ee7b3b64f70d6171
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/20/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/116/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/492/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/20//rebuild

review: Needs Fixing (continuous-integration)

[James Troup (elmo)]

See comment inline.

review: Needs Fixing

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:6dd95bb4bd2d4eddb5ff61e3a61847b95fa7a1e7
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/21/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/117/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/493/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/21//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[Xav Paice (xavpaice)]

I've rewritten the change and added some tests.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:4f7b0a0882305480c2bd4878e5605899931f356d
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/22/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/118/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/494/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/22//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:4f7b0a0882305480c2bd4878e5605899931f356d
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/23/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/119/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/495/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/23//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

FAILED: Continuous integration, rev:2edd09aa334de795c2e5cc1dbedb76c433da712a
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/24/
Executed test runs:
    FAILURE: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/120/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/496/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/24//rebuild

review: Needs Fixing (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:a46e8c68dea509cb00946a1269763484314f7c14
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/25/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/121/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/497/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/25//rebuild

review: Approve (continuous-integration)

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

A CI job is currently in progress. A follow up comment will be added when it completes.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:4344ba9b531ddeabd017b53ebaa468900679315c
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/26/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/122/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/508/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-nrpe-ci/26//rebuild

review: Approve (continuous-integration)

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

Change successfully merged at revision 74ca861645689f8eeeeebea7175d7c4b7a35f68d

diff --git a/hooks/nrpe_helpers.py b/hooks/nrpe_helpers.py
index 8384d76..b4f40b9 100644
--- a/hooks/nrpe_helpers.py
+++ b/hooks/nrpe_helpers.py
@@ -77,42 +77,42 @@ class Monitors(dict):
         return mons
 
 
-def get_local_ingress_address(binding):
+def get_ingress_address(binding, external=False):
     """Get ingress IP address for a binding.
 
-    binding - e.g. 'monitors'
+    Returns a local IP address for incoming requests to NRPE.
+
+    :param binding: name of the binding, e.g. 'monitors'
+    :param external: bool, if True return the public address if charm config requests
+                     otherwise return the local address which would be used for incoming
+                     nrpe requests.
     """
     # using network-get to retrieve the address details if available.
     hookenv.log("Getting ingress IP address for binding %s" % binding)
+    if hookenv.config("nagios_address_type").lower() == "public" and external:
+        return hookenv.unit_get("public-address")
+
+    ip_address = None
     try:
         network_info = hookenv.network_get(binding)
         if network_info is not None and "ingress-addresses" in network_info:
-            hookenv.log("Using ingress-addresses")
-            # workaround lp#1897261
             try:
                 ip_address = network_info["bind-addresses"][0]["addresses"][0][
                     "address"
                 ]
+                hookenv.log("Using ingress-addresses, found %s" % ip_address)
             except KeyError:
-                # ignore KeyError and populate ip_address per old method
-                ip_address = None
-            if ip_address not in network_info["ingress-addresses"]:
-                ip_address = network_info["ingress-addresses"][0]
-            hookenv.log(ip_address)
-            return ip_address
-    except (NotImplementedError, FileNotFoundError):
-        # We'll fallthrough to the Pre 2.3 code below.
-        pass
-
-    # Pre 2.3 output
-    try:
-        ip_address = hookenv.network_get_primary_address(binding)
-        hookenv.log("Using primary-addresses")
-    except NotImplementedError:
-        # pre Juju 2.0
-        ip_address = hookenv.unit_private_ip()
-        hookenv.log("Using unit_private_ip")
-    hookenv.log(ip_address)
+                hookenv.log("Using primary-addresses")
+                ip_address = hookenv.network_get_primary_address(binding)
+
+    except (NotImplementedError, FileNotFoundError) as e:
+        hookenv.log(
+            "Unable to determine inbound IP address for binding {} with {}".format(
+                binding, e
+            ),
+            level=hookenv.ERROR,
+        )
+
     return ip_address
 
 
@@ -200,7 +200,8 @@ class MonitorsRelation(helpers.RelationContext):
 
     def provide_data(self):
         """Return relation info."""
-        address = get_local_ingress_address("monitors")
+        # get the address to send to Nagios for host definition
+        address = get_ingress_address("monitors", external=True)
 
         relation_info = {
             "target-id": self.principal_relation.nagios_hostname(),
@@ -299,24 +300,10 @@ class NagiosInfo(dict):
             )
         self["nagios_hostname"] = self.principal_relation.nagios_hostname()
 
-        address = None
-        if hookenv.config("nagios_address_type").lower() == "public":
-            address = hookenv.unit_get("public-address")
-        elif hookenv.config("nagios_master") != "None":
-            # Try to work out the correct interface/IP. We can't use both
-            # network-get nor 'unit-get private-address' because both can
-            # return the wrong IP on systems with more than one interface
-            # (LP: #1736050).
-            s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
-            s.connect((hookenv.config("nagios_master").split(",")[0], 80))
-            address = s.getsockname()[0]
-            s.close()
-        # Fallback to unit-get private-address
-        if not address:
-            address = hookenv.unit_get("private-address")
-
-        self["nagios_ipaddress"] = address
-        self["nrpe_ipaddress"] = get_local_ingress_address("monitors")
+        # export_host.cfg.tmpl host definition for Nagios
+        self["nagios_ipaddress"] = get_ingress_address("monitors", external=True)
+        # Address configured for NRPE to listen on
+        self["nrpe_ipaddress"] = get_ingress_address("monitors")
 
         self["dont_blame_nrpe"] = "1" if hookenv.config("dont_blame_nrpe") else "0"
         self["debug"] = "1" if hookenv.config("debug") else "0"
diff --git a/tests/functional/tests/bundles/focal.yaml b/tests/functional/tests/bundles/focal.yaml
index 2689a08..33bea11 100644
--- a/tests/functional/tests/bundles/focal.yaml
+++ b/tests/functional/tests/bundles/focal.yaml
@@ -3,6 +3,7 @@ applications:
   rabbitmq-server:
     charm: cs:rabbitmq-server
     num_units: 1
+    constraints: root-disk=8G
   container:
     charm: cs:ubuntu
     num_units: 1
diff --git a/tests/functional/tests/tests.yaml b/tests/functional/tests/tests.yaml
index 8034f51..a661597 100644
--- a/tests/functional/tests/tests.yaml
+++ b/tests/functional/tests/tests.yaml
@@ -1,7 +1,9 @@
 charm_name: nrpe
 gate_bundles:
-  - xenial
+  - focal
   - bionic
+  - xenial
+smoke_bundles:
   - focal
 tests:
   - tests.nrpe_tests.TestNrpe
diff --git a/tests/unit/test_nrpe_helpers.py b/tests/unit/test_nrpe_helpers.py
index fc17454..949eb92 100644
--- a/tests/unit/test_nrpe_helpers.py
+++ b/tests/unit/test_nrpe_helpers.py
@@ -4,6 +4,7 @@ from unittest import mock
 
 import netifaces
 
+import nrpe_helpers
 from nrpe_helpers import match_cidr_to_ifaces
 
 
@@ -11,39 +12,107 @@ class TestMatchCidrToIfaces(unittest.TestCase):
     """Test match_cidr_to_ifaces helper function."""
 
     mock_iface_ip_data = {
-        'lo': '127.0.0.1',
-        'eno1': '10.0.0.0',
-        'eno2': '10.1.0.0',
-        'fan-252': '252.0.0.1',
+        "lo": "127.0.0.1",
+        "eno1": "10.0.0.0",
+        "eno2": "10.1.0.0",
+        "fan-252": "252.0.0.1",
     }
 
     def test_single_dev_match(self):
         """Test single interface match."""
-        self._run_mocked_test('10.0.0.0/16', ['eno1'])
+        self._run_mocked_test("10.0.0.0/16", ["eno1"])
 
     def test_multi_dev_match(self):
         """Test multiple interface match."""
-        self._run_mocked_test('10.0.0.0/8', ['eno1', 'eno2'])
+        self._run_mocked_test("10.0.0.0/8", ["eno1", "eno2"])
 
     def test_no_dev_match(self):
         """Test no interface match."""
-        self._run_mocked_test('192.168.0.0/16', [])
+        self._run_mocked_test("192.168.0.0/16", [])
 
     def test_cidr_with_host_bits_set(self):
         """Test invalid CIDR input (e.g. "eno1")."""
         with self.assertRaises(Exception):
-            match_cidr_to_ifaces('10.1.2.3/8')  # Should be 10.0.0.0/8
+            match_cidr_to_ifaces("10.1.2.3/8")  # Should be 10.0.0.0/8
 
     def test_iface_passed_in_as_cidr(self):
         """Test invalid CIDR input (e.g. "eno1")."""
         with self.assertRaises(Exception):
-            match_cidr_to_ifaces('eno1')
+            match_cidr_to_ifaces("eno1")
 
-    @mock.patch('netifaces.ifaddresses')
-    @mock.patch('netifaces.interfaces')
+    @mock.patch("netifaces.ifaddresses")
+    @mock.patch("netifaces.interfaces")
     def _run_mocked_test(self, cidr, matches, ifaces_mock, addrs_mock):
         iface_ip_tuples = list(self.mock_iface_ip_data.items())
         ifaces_mock.return_value = [t[0] for t in iface_ip_tuples]
-        addrs_mock.side_effect = [{netifaces.AF_INET: [{'addr': t[1]}]} for t in
-                                  iface_ip_tuples]
+        addrs_mock.side_effect = [
+            {netifaces.AF_INET: [{"addr": t[1]}]} for t in iface_ip_tuples
+        ]
         self.assertEqual(match_cidr_to_ifaces(cidr), matches)
+
+
+class TestIngressAddress(unittest.TestCase):
+    """Test functions to provide a suitable ingress address."""
+
+    @mock.patch("nrpe_helpers.hookenv.config")
+    @mock.patch("nrpe_helpers.hookenv.network_get")
+    def test_get_bind_address(self, mock_network_get, mock_config):
+        """Prove we get a local IP address for interface binding."""
+        mock_config.return_value = "private"
+        mock_network_get.return_value = {
+            "bind-addresses": [
+                {
+                    "mac-address": "06:f1:3a:74:ad:fe",
+                    "interface-name": "ens5",
+                    "addresses": [
+                        {
+                            "hostname": "",
+                            "address": "172.31.29.247",
+                            "cidr": "172.31.16.0/20",
+                        }
+                    ],
+                    "macaddress": "06:f1:3a:74:ad:fe",
+                    "interfacename": "ens5",
+                }
+            ],
+            "egress-subnets": ["3.8.134.119/32"],
+            "ingress-addresses": ["3.8.134.119"],
+        }
+        self.assertEqual(nrpe_helpers.get_ingress_address("mockbinding"),
+                         "172.31.29.247")
+
+    @mock.patch("nrpe_helpers.hookenv.config")
+    @mock.patch("nrpe_helpers.hookenv.network_get")
+    def test_get_private_address(self, mock_network_get, mock_config):
+        """Prove we get a local IP address for Nagios relation."""
+        mock_config.return_value = "private"
+        mock_network_get.return_value = {
+            "bind-addresses": [
+                {
+                    "mac-address": "06:f1:3a:74:ad:fe",
+                    "interface-name": "ens5",
+                    "addresses": [
+                        {
+                            "hostname": "",
+                            "address": "172.31.29.247",
+                            "cidr": "172.31.16.0/20",
+                        }
+                    ],
+                    "macaddress": "06:f1:3a:74:ad:fe",
+                    "interfacename": "ens5",
+                }
+            ],
+            "egress-subnets": ["3.8.134.119/32"],
+            "ingress-addresses": ["3.8.134.119"],
+        }
+        self.assertEqual(nrpe_helpers.get_ingress_address("mockbinding", external=True),
+                         "172.31.29.247")
+
+    @mock.patch("nrpe_helpers.hookenv.config")
+    @mock.patch("nrpe_helpers.hookenv.unit_get")
+    def test_get_public_address(self, mock_unit_get, mock_config):
+        """Prove we get a public IP address for Nagios relation."""
+        mock_config.return_value = "public"
+        mock_unit_get.return_value = "1.2.3.4"
+        self.assertEqual(nrpe_helpers.get_ingress_address("mockbinding", external=True),
+                         "1.2.3.4")