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

Proposed by Stuart Bishop
Status: Needs review
Proposed branch: lp:~stub/charm-helpers/bug-1557769-ensure-ip-address
Merge into: lp:charm-helpers
Diff against target: 131 lines (+56/-9)
3 files modified
Makefile (+2/-2)
charmhelpers/core/hookenv.py (+19/-2)
tests/core/test_hookenv.py (+35/-5)
To merge this branch: bzr merge lp:~stub/charm-helpers/bug-1557769-ensure-ip-address
Reviewer Review Type Date Requested Status
charmers Pending
Review via email: mp+289128@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
Revision history for this message
Cory Johns (johnsca) wrote :

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

Revision history for this message
Stuart Bishop (stub) wrote :

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.

Revision history for this message
Stuart Bishop (stub) wrote :

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.

Revision history for this message
Stuart Bishop (stub) wrote :

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

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

549. By Stuart Bishop

Missing dependencies

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Makefile'
2--- Makefile 2015-12-08 05:07:07 +0000
3+++ Makefile 2016-03-16 01:42:31 +0000
4@@ -37,7 +37,7 @@
5 python setup.py install --user
6
7 .venv:
8- sudo apt-get install -y gcc python-dev python-virtualenv python-apt
9+ sudo apt-get install -y gcc python-dev python-virtualenv python-apt distro-info git bzr zip
10 virtualenv .venv --system-site-packages
11 .venv/bin/pip install -U pip
12 .venv/bin/pip install -U distribute
13@@ -45,7 +45,7 @@
14 .venv/bin/pip install bzr
15
16 .venv3:
17- sudo apt-get install -y gcc python3-dev python-virtualenv python3-apt
18+ sudo apt-get install -y gcc python3-dev python-virtualenv python3-apt distro-info git bzr zip
19 virtualenv .venv3 --python=python3 --system-site-packages
20 .venv3/bin/pip install -U pip
21 .venv3/bin/pip install -U distribute
22
23=== modified file 'charmhelpers/core/hookenv.py'
24--- charmhelpers/core/hookenv.py 2016-03-07 18:02:12 +0000
25+++ charmhelpers/core/hookenv.py 2016-03-16 01:42:31 +0000
26@@ -28,6 +28,7 @@
27 import os
28 import json
29 import yaml
30+import socket
31 import subprocess
32 import sys
33 import errno
34@@ -628,12 +629,28 @@
35
36 def unit_public_ip():
37 """Get this unit's public IP address"""
38- return unit_get('public-address')
39+ return _ensure_ip(unit_get('public-address'))
40
41
42 def unit_private_ip():
43 """Get this unit's private IP address"""
44- return unit_get('private-address')
45+ return _ensure_ip(unit_get('private-address'))
46+
47+
48+def _ensure_ip(addr):
49+ """If addr is a hostname, resolve it to an IP address"""
50+ if not addr:
51+ return None
52+ # We need to use socket.getaddrinfo for IPv6 support.
53+ info = socket.getaddrinfo(addr, None)
54+ if info is None:
55+ # Should never happen
56+ raise ValueError("Invalid result None from getaddinfo")
57+ try:
58+ return info[0][4][0]
59+ except IndexError:
60+ # Should never happen
61+ raise ValueError("Invalid result {!r} from getaddinfo".format(info))
62
63
64 @cached
65
66=== modified file 'tests/core/test_hookenv.py'
67--- tests/core/test_hookenv.py 2016-03-09 20:30:51 +0000
68+++ tests/core/test_hookenv.py 2016-03-16 01:42:31 +0000
69@@ -2,9 +2,10 @@
70 import json
71 from subprocess import CalledProcessError
72 import shutil
73+import socket
74 import tempfile
75 from mock import call, MagicMock, mock_open, patch, sentinel
76-from testtools import TestCase
77+from unittest import TestCase
78 import yaml
79
80 import six
81@@ -361,17 +362,46 @@
82
83 self.assertEqual(hookenv.local_unit(), 'foo')
84
85+ @patch('charmhelpers.core.hookenv._ensure_ip')
86 @patch('charmhelpers.core.hookenv.unit_get')
87- def test_gets_unit_public_ip(self, _unitget):
88- _unitget.return_value = sentinel.public_ip
89+ def test_gets_unit_public_ip(self, _unitget, _ensure_ip):
90+ _unitget.return_value = sentinel.public_address
91+ _ensure_ip.return_value = sentinel.public_ip
92 self.assertEqual(sentinel.public_ip, hookenv.unit_public_ip())
93 _unitget.assert_called_once_with('public-address')
94+ _ensure_ip.assert_called_once_with(sentinel.public_address)
95
96+ @patch('charmhelpers.core.hookenv._ensure_ip')
97 @patch('charmhelpers.core.hookenv.unit_get')
98- def test_gets_unit_private_ip(self, _unitget):
99- _unitget.return_value = sentinel.private_ip
100+ def test_gets_unit_private_ip(self, _unitget, _ensure_ip):
101+ _unitget.return_value = sentinel.private_address
102+ _ensure_ip.return_value = sentinel.private_ip
103 self.assertEqual(sentinel.private_ip, hookenv.unit_private_ip())
104 _unitget.assert_called_once_with('private-address')
105+ _ensure_ip.assert_called_once_with(sentinel.private_address)
106+
107+ @patch('socket.getaddrinfo')
108+ def test_ensure_ip(self, info):
109+ # Resolve names to ip addresses. Note that getaddrinfo
110+ # accepts both names and ip addresses, passing through
111+ # ip addresses unmolested.
112+ info.return_value = [(socket.AF_INET, socket.SOCK_STREAM,
113+ 6, '', (sentinel.ip, 0))]
114+ self.assertEqual(hookenv._ensure_ip(sentinel.address), sentinel.ip)
115+
116+ # Deal with bogus input as best we can.
117+ self.assertIsNone(hookenv._ensure_ip(None))
118+ self.assertIsNone(hookenv._ensure_ip(''))
119+
120+ # Deal with weird getaddrinfo return values that should never
121+ # occur as best we can.
122+ info.return_value = None
123+ with self.assertRaises(ValueError):
124+ hookenv._ensure_ip(sentinel.address)
125+
126+ info.return_value = []
127+ with self.assertRaises(ValueError):
128+ hookenv._ensure_ip(sentinel.address)
129
130 @patch('charmhelpers.core.hookenv.os')
131 def test_checks_that_is_running_in_relation_hook(self, os_):

Subscribers

People subscribed via source and target branches