Merge lp:~thedac/charm-helpers/access-network into lp:charm-helpers

[David Ames]

Pass an access-network directly to get_relation_ip

get_relation_ip currently has a config parameter config_override.
However, there are times like with share-db when the network override
is not a config option. Allow the option to set an access-network directly.

[Alex Kavanagh (ajkavanagh)]

Is it correct that prefer-ipv6 overrides the access-space? This is more just a comment, to check that I understand the precedence for the various config options. Sorry if a daft question.

[Alex Kavanagh (ajkavanagh)]

Sorry, an additional comment. How are:

get_relation_id(interface, config_override='access-network')

and

get_relation_id(interface, access_network=config('access-network'))

different? I must be missing something in the intent, or how charms are configured.

[David Ames (thedac)]

> Is it correct that prefer-ipv6 overrides the access-space? This is more just
> a comment, to check that I understand the precedence for the various config
> options. Sorry if a daft question.

With the current implementation when prefer-ipv6 is set it overrides everything. Or at least that is how it has been implemented in the past.

Going forward we ultimately want IPv6 to just be another space. As soon as we have no Juju 1.x we can just rely on spaces for IPv6 or IPv4.

[David Ames (thedac)]

> Sorry, an additional comment. How are:
>
> get_relation_id(interface, config_override='access-network')
>
> and
>
> get_relation_id(interface, access_network=config('access-network'))
>
> different? I must be missing something in the intent, or how charms are
> configured.

Good question, and this is what I feel is clunky. In both cases we are selecting an address given a CIDR network.

config_override is from a configuration value like rabbitmq's "access-network" https://github.com/openstack/charm-rabbitmq-server/blob/master/config.yaml#L172

access_network is not a configuration value. In the primary shared-db case this is received over a relation and then passed to get_relation_ip.

[Alex Kavanagh (ajkavanagh)]

Ah, yes, of course. I see what you mean about 'clunky'. The reason it's clunky is that (from a technical perspective) we are 'mixing concerns'. The get_relation_id() function is already dependent on the implementation of config(...) rather than the override just being the CIDR string.

The proper 'fix' is to take out the config(config_override) call from the function and just pass in the CIDR as a string. The calling function knows that it's in config and so should do:

get_relation_id(interface, override_network=config('override-config-key'))

rather than:

get_relation_id(interface, override='override-config-key')

That's where the clunkiness is coming from. I'm not sure if it is fixable because it's a shared library and charms may be depending on the functionality. A solution is to provide another function that does it properly and then just proxy the get_relation_id to that function:

def get_relation_id(interface, override):
    return proper_func(interface, config(override) if override)

Thoughts?

[David Ames (thedac)]

That makes sense.

So the function is new enough that is only in a handful of places. As long as we update all of them we should be ok. I'll make the change and get the list of charms that use it that need to be updated.

[lp:~thedac/charm-helpers/access-network]

735. By David Ames on 2017-04-19

Simplify to only cidr_network

[David Ames (thedac)]

Simplified and updated. Much less clunky. And I thought harder about the fallback address which had me rearrange things a bit.

The good news is that only nova-compute uses get_relation_ip and it does not pass an override. So no changes required. The bad news is rabbitmq-server uses a bastardized version of this and should probably be updated.

Regardless this CH MP and the nova-cloud-controller PR are ready to go if you give the all clear.

[Alex Kavanagh (ajkavanagh)]

David, this is great! Nicely handles the fallback for Juju 1.25.x, deals with the ipv6 issue (until it becomes a space) and also handles the override with a cidr in a non-dependency way. I need this function for my nova-cc changes too! This is a function that should go on the mailing list.

review: Approve

=== modified file 'charmhelpers/contrib/network/ip.py'
--- charmhelpers/contrib/network/ip.py	2017-04-19 12:42:07 +0000
+++ charmhelpers/contrib/network/ip.py	2017-04-19 21:29:37 +0000
@@ -554,31 +554,38 @@
                         "versions less than Trusty 14.04")
 
 
-def get_relation_ip(interface, config_override=None):
-    """Return this unit's IP for the given relation.
+def get_relation_ip(interface, cidr_network=None):
+    """Return this unit's IP for the given interface.
 
     Allow for an arbitrary interface to use with network-get to select an IP.
-    Handle all address selection options including configuration parameter
-    override and IPv6.
+    Handle all address selection options including passed cidr network and
+    IPv6.
 
-    Usage: get_relation_ip('amqp', config_override='access-network')
+    Usage: get_relation_ip('amqp', cidr_network='10.0.0.0/8')
 
     @param interface: string name of the relation.
-    @param config_override: string name of the config option for network
-           override. Supports legacy network override configuration parameters.
+    @param cidr_network: string CIDR Network to select an address from.
     @raises Exception if prefer-ipv6 is configured but IPv6 unsupported.
     @returns IPv6 or IPv4 address
     """
+    # Select the interface address first
+    # For possible use as a fallback bellow with get_address_in_network
+    try:
+        # Get the interface specific IP
+        address = network_get_primary_address(interface)
+    except NotImplementedError:
+        # If network-get is not available
+        address = get_host_ip(unit_get('private-address'))
 
-    fallback = get_host_ip(unit_get('private-address'))
     if config('prefer-ipv6'):
+        # Currently IPv6 has priority, eventually we want IPv6 to just be
+        # another network space.
         assert_charm_supports_ipv6()
         return get_ipv6_addr()[0]
-    elif config_override and config(config_override):
-        return get_address_in_network(config(config_override),
-                                      fallback)
-    else:
-        try:
-            return network_get_primary_address(interface)
-        except NotImplementedError:
-            return fallback
+    elif cidr_network:
+        # If a specific CIDR network is passed get the address from that
+        # network.
+        return get_address_in_network(cidr_network, address)
+
+    # Return the interface address
+    return address

=== modified file 'tests/contrib/network/test_ip.py'
--- tests/contrib/network/test_ip.py	2017-04-19 12:42:07 +0000
+++ tests/contrib/network/test_ip.py	2017-04-19 21:29:37 +0000
@@ -803,54 +803,42 @@
     def test_get_relation_ip(self, assert_charm_supports_ipv6, get_ipv6_addr,
                              unit_get, get_address_in_network,
                              network_get_primary_address, config):
+        ACCESS_IP = '10.50.1.1'
+        ACCESS_NETWORK = '10.50.1.0/24'
         AMQP_IP = '10.200.1.1'
-        OVERRIDE_AMQP_IP = '10.250.1.1'
-        CLUSTER_IP = '10.100.1.1'
-        OVERRIDE_CLUSTER_IP = '10.150.1.1'
         IPV6_IP = '2001:DB8::1'
         DEFAULT_IP = '172.16.1.1'
         assert_charm_supports_ipv6.return_value = True
         get_ipv6_addr.return_value = [IPV6_IP]
         unit_get.return_value = DEFAULT_IP
         get_address_in_network.return_value = DEFAULT_IP
+        network_get_primary_address.return_value = AMQP_IP
+
+        # Network-get calls
+        _config = {'prefer-ipv6': False}
+        config.side_effect = lambda key: _config.get(key)
+
+        network_get_primary_address.side_effect = NotImplementedError
+        self.assertEqual(DEFAULT_IP, net_ip.get_relation_ip('amqp'))
+
+        network_get_primary_address.side_effect = None
+        self.assertEqual(AMQP_IP, net_ip.get_relation_ip('amqp'))
+
+        self.assertFalse(get_address_in_network.called)
+
+        # Specific CIDR network
+        get_address_in_network.return_value = ACCESS_IP
         network_get_primary_address.return_value = DEFAULT_IP
+        self.assertEqual(
+            ACCESS_IP,
+            net_ip.get_relation_ip('shared-db',
+                                   cidr_network=ACCESS_NETWORK))
+        get_address_in_network.assert_called_with(ACCESS_NETWORK, DEFAULT_IP)
+
+        self.assertFalse(assert_charm_supports_ipv6.called)
 
         # IPv6
-        _config = {'prefer-ipv6': True,
-                   'cluster-network': '10.100.1.0/24',
-                   'access-network': '10.200.1.0/24'}
+        _config = {'prefer-ipv6': True}
         config.side_effect = lambda key: _config.get(key)
         self.assertEqual(IPV6_IP, net_ip.get_relation_ip('amqp'))
-
-        # Overrides
-        _config = {'prefer-ipv6': False,
-                   'cluster-network': '10.100.1.0/24',
-                   'access-network': '10.200.1.0/24'}
-        config.side_effect = lambda key: _config.get(key)
-
-        get_address_in_network.return_value = OVERRIDE_AMQP_IP
-        self.assertEqual(
-            OVERRIDE_AMQP_IP,
-            net_ip.get_relation_ip('amqp',
-                                   config_override='access-network'))
-
-        get_address_in_network.return_value = OVERRIDE_CLUSTER_IP
-        self.assertEqual(OVERRIDE_CLUSTER_IP,
-                         net_ip.get_relation_ip(
-                             'cluster',
-                             config_override='cluster-network'))
-
-        # Network-get calls
-        _config = {'prefer-ipv6': False,
-                   'cluster-network': None,
-                   'access-network': None}
-        config.side_effect = lambda key: _config.get(key)
-
-        network_get_primary_address.return_value = AMQP_IP
-        self.assertEqual(AMQP_IP, net_ip.get_relation_ip('amqp'))
-
-        network_get_primary_address.return_value = CLUSTER_IP
-        self.assertEqual(CLUSTER_IP,
-                         net_ip.get_relation_ip(
-                             config_override='cluster-network',
-                             interface='cluster'))
+        assert_charm_supports_ipv6.assert_called_with()