Merge lp:~stub/charm-helpers/bug-1557769-ensure-ip-address into lp:charm-helpers

[Stuart Bishop]

Juju 1.25.4 seems to be again returning names in some situations where we expect IP addresses. Fix this, even though it might be considered a Juju regression.

[Cory Johns (johnsca)]

From my understanding, private-address was never guaranteed to return an IP and in fact almost always returns some form of host name, meaning this function was always poorly named. I believe it depends on the provider what is actually returned.

For the big data charms (and originally for CloudFoundry), we use a similar helper[1] to force it to IPs, but that doesn't support IPv6 as yours does. We also ran into at least one instance where the "host name" returned wasn't resolvable and the IP was part of the host name string and had to be parsed out. My point is just that there a lot of corner cases in what should be a simple function.

It would be ideal if Juju handled this for us and had a private-ip field as well.

[1]: https://github.com/juju-solutions/jujubigdata/blob/master/jujubigdata/utils.py#L404

[Stuart Bishop (stub)]

I'm leaving it up to the juju-core bug squad to decide if this is a juju-core bug or not :) I'm fixing it here for old versions of Juju, and because I need the fix right now for a production deployment. We can open feature requests if they decline to accept Bug #1557769, and if they get implemented, update this function to use it when it is available.

I think we should ignore the case where what is returned by unit-info private-address is not even a resolvable name - that is certainly a provider bug (and one I don't think exists anywhere any more?). If getaddrinfo can't decode it (an unparsable or unresolvable name), it is useless to charms and their software and I think I'm doing the right thing by raising a ValueError rather than, say, attempting to munge it or pass it on through for the charm to choke on it. Call sites can of course catch the ValueError and deal with it if they really need to.

[Stuart Bishop (stub)]

It seems that juju-core has accepted the change in 1.25.4 under some providers as a regression.

Even if not, we should fix charm-helpers to return a consistent value across providers.

The Cassandra charm under 1.25.4 or later has been broken for a few months now under some providers. I'll try to get a release out with a forked charm-helpers next week since it is griefing one of our projects, but i would be nice to get this landed upstream in charm-helpers.

[Stuart Bishop (stub)]

This is still occurring under Juju 2.1, so I'd like to get this landed in charm-helpers.

Unmerged revisions

550. By Stuart Bishop on 2016-03-16

Ensure IP addresses are returned by methods claiming to return IP addresses

549. By Stuart Bishop on 2016-03-16

Missing dependencies

=== modified file 'Makefile'
--- Makefile	2015-12-08 05:07:07 +0000
+++ Makefile	2016-03-16 01:42:31 +0000
@@ -37,7 +37,7 @@
 	python setup.py install --user
 
 .venv:
-	sudo apt-get install -y gcc python-dev python-virtualenv python-apt
+	sudo apt-get install -y gcc python-dev python-virtualenv python-apt distro-info git bzr zip
 	virtualenv .venv --system-site-packages
 	.venv/bin/pip install -U pip
 	.venv/bin/pip install -U distribute
@@ -45,7 +45,7 @@
 	.venv/bin/pip install bzr
 
 .venv3:
-	sudo apt-get install -y gcc python3-dev python-virtualenv python3-apt
+	sudo apt-get install -y gcc python3-dev python-virtualenv python3-apt distro-info git bzr zip
 	virtualenv .venv3 --python=python3 --system-site-packages
 	.venv3/bin/pip install -U pip
 	.venv3/bin/pip install -U distribute
 
=== modified file 'charmhelpers/core/hookenv.py'
--- charmhelpers/core/hookenv.py	2016-03-07 18:02:12 +0000
+++ charmhelpers/core/hookenv.py	2016-03-16 01:42:31 +0000
@@ -28,6 +28,7 @@
 import os
 import json
 import yaml
+import socket
 import subprocess
 import sys
 import errno
@@ -628,12 +629,28 @@
 
 def unit_public_ip():
     """Get this unit's public IP address"""
-    return unit_get('public-address')
+    return _ensure_ip(unit_get('public-address'))
 
 
 def unit_private_ip():
     """Get this unit's private IP address"""
-    return unit_get('private-address')
+    return _ensure_ip(unit_get('private-address'))
+
+
+def _ensure_ip(addr):
+    """If addr is a hostname, resolve it to an IP address"""
+    if not addr:
+        return None
+    # We need to use socket.getaddrinfo for IPv6 support.
+    info = socket.getaddrinfo(addr, None)
+    if info is None:
+        # Should never happen
+        raise ValueError("Invalid result None from getaddinfo")
+    try:
+        return info[0][4][0]
+    except IndexError:
+        # Should never happen
+        raise ValueError("Invalid result {!r} from getaddinfo".format(info))
 
 
 @cached
 
=== modified file 'tests/core/test_hookenv.py'
--- tests/core/test_hookenv.py	2016-03-09 20:30:51 +0000
+++ tests/core/test_hookenv.py	2016-03-16 01:42:31 +0000
@@ -2,9 +2,10 @@
 import json
 from subprocess import CalledProcessError
 import shutil
+import socket
 import tempfile
 from mock import call, MagicMock, mock_open, patch, sentinel
-from testtools import TestCase
+from unittest import TestCase
 import yaml
 
 import six
@@ -361,17 +362,46 @@
 
         self.assertEqual(hookenv.local_unit(), 'foo')
 
+    @patch('charmhelpers.core.hookenv._ensure_ip')
     @patch('charmhelpers.core.hookenv.unit_get')
-    def test_gets_unit_public_ip(self, _unitget):
-        _unitget.return_value = sentinel.public_ip
+    def test_gets_unit_public_ip(self, _unitget, _ensure_ip):
+        _unitget.return_value = sentinel.public_address
+        _ensure_ip.return_value = sentinel.public_ip
         self.assertEqual(sentinel.public_ip, hookenv.unit_public_ip())
         _unitget.assert_called_once_with('public-address')
+        _ensure_ip.assert_called_once_with(sentinel.public_address)
 
+    @patch('charmhelpers.core.hookenv._ensure_ip')
     @patch('charmhelpers.core.hookenv.unit_get')
-    def test_gets_unit_private_ip(self, _unitget):
-        _unitget.return_value = sentinel.private_ip
+    def test_gets_unit_private_ip(self, _unitget, _ensure_ip):
+        _unitget.return_value = sentinel.private_address
+        _ensure_ip.return_value = sentinel.private_ip
         self.assertEqual(sentinel.private_ip, hookenv.unit_private_ip())
         _unitget.assert_called_once_with('private-address')
+        _ensure_ip.assert_called_once_with(sentinel.private_address)
+
+    @patch('socket.getaddrinfo')
+    def test_ensure_ip(self, info):
+        # Resolve names to ip addresses. Note that getaddrinfo
+        # accepts both names and ip addresses, passing through
+        # ip addresses unmolested.
+        info.return_value = [(socket.AF_INET, socket.SOCK_STREAM,
+                              6, '', (sentinel.ip, 0))]
+        self.assertEqual(hookenv._ensure_ip(sentinel.address), sentinel.ip)
+
+        # Deal with bogus input as best we can.
+        self.assertIsNone(hookenv._ensure_ip(None))
+        self.assertIsNone(hookenv._ensure_ip(''))
+
+        # Deal with weird getaddrinfo return values that should never
+        # occur as best we can.
+        info.return_value = None
+        with self.assertRaises(ValueError):
+            hookenv._ensure_ip(sentinel.address)
+
+        info.return_value = []
+        with self.assertRaises(ValueError):
+            hookenv._ensure_ip(sentinel.address)
 
     @patch('charmhelpers.core.hookenv.os')
     def test_checks_that_is_running_in_relation_hook(self, os_):