[LIBCLOUD-614] multiple bugfixes and improvements to GCE

libcloud/compute/drivers/gce.py

@@ -910,7 +910,7 @@ def ex_list_zones(self):
         list_zones = [self._to_zone(z) for z in response['items']]
         return list_zones
 
-    def ex_create_address(self, name, region=None):
+    def ex_create_address(self, name, region=None, address=None):
         """
         Create a static address in a region.
 
@@ -920,6 +920,10 @@ def ex_create_address(self, name, region=None):
         :keyword  region: Name of region for the address (e.g. 'us-central1')
         :type     region: ``str`` or :class:`GCERegion`
 
+        :keyword  address: Ephemeral IP address to promote to a static one
+                           (e.g. 'xxx.xxx.xxx.xxx')
+        :type     address: ``str`` or ``None``
+
         :return:  Static Address object
         :rtype:   :class:`GCEAddress`
         """
@@ -930,6 +934,8 @@ def ex_create_address(self, name, region=None):
             raise ValueError('REGION_NOT_SPECIFIED',
                              'Region must be provided for an address')
         address_data = {'name': name}
+        if address:
+            address_data['address'] = address
         request = '/regions/%s/addresses' % (region.name)
         self.connection.async_request(request, method='POST',
                                       data=address_data)
@@ -938,7 +944,8 @@ def ex_create_address(self, name, region=None):
     def ex_create_healthcheck(self, name, host=None, path=None, port=None,
                               interval=None, timeout=None,
                               unhealthy_threshold=None,
-                              healthy_threshold=None):
+                              healthy_threshold=None,
+                              description=None):
         """
         Create an Http Health Check.
 
@@ -969,13 +976,18 @@ def ex_create_healthcheck(self, name, host=None, path=None, port=None,
                                      healthy.  Defaults to 2.
         :type     healthy_threshold: ``int``
 
+        :keyword  description: The description of the check.  Defaults to None.
+        :type     description: ``str`` or ``None``
+
         :return:  Health Check object
         :rtype:   :class:`GCEHealthCheck`
         """
         hc_data = {}
         hc_data['name'] = name
         if host:
             hc_data['host'] = host
+        if description:
+            hc_data['description'] = description
         # As of right now, the 'default' values aren't getting set when called
         # through the API, so set them explicitly
         hc_data['requestPath'] = path or '/'
@@ -1046,7 +1058,9 @@ def ex_create_firewall(self, name, allowed, network='default',
         firewall_data['name'] = name
         firewall_data['allowed'] = allowed
         firewall_data['network'] = nw.extra['selfLink']
-        firewall_data['sourceRanges'] = source_ranges or ['0.0.0.0/0']
+        if source_ranges is None:
+            source_ranges = ['0.0.0.0/0']
+        firewall_data['sourceRanges'] = source_ranges
         if source_tags is not None:
             firewall_data['sourceTags'] = source_tags
         if target_tags is not None:
@@ -1060,7 +1074,7 @@ def ex_create_firewall(self, name, allowed, network='default',
 
     def ex_create_forwarding_rule(self, name, targetpool, region=None,
                                   protocol='tcp', port_range=None,
-                                  address=None):
+                                  address=None, description=None):
         """
         Create a forwarding rule.
 
@@ -1085,6 +1099,10 @@ def ex_create_forwarding_rule(self, name, targetpool, region=None,
                            in same region.
         :type     address: ``str`` or :class:`GCEAddress`
 
+        :keyword  description: The description of the forwarding rule.
+                               Defaults to None.
+        :type     description: ``str`` or ``None``
+
         :return:  Forwarding Rule object
         :rtype:   :class:`GCEForwardingRule`
         """
@@ -1098,13 +1116,15 @@ def ex_create_forwarding_rule(self, name, targetpool, region=None,
         forwarding_rule_data['name'] = name
         forwarding_rule_data['region'] = region.extra['selfLink']
         forwarding_rule_data['target'] = targetpool.extra['selfLink']
-        forwarding_rule_data['protocol'] = protocol.upper()
+        forwarding_rule_data['IPProtocol'] = protocol.upper()
         if address:
             if not hasattr(address, 'name'):
                 address = self.ex_get_address(address, region)
-            forwarding_rule_data['IPAddress'] = address.extra['selfLink']
+            forwarding_rule_data['IPAddress'] = address.address
         if port_range:
             forwarding_rule_data['portRange'] = port_range
+        if description:
+            forwarding_rule_data['description'] = description
 
         request = '/regions/%s/forwardingRules' % (region.name)
 
@@ -1613,16 +1633,25 @@ def ex_targetpool_add_node(self, targetpool, node):
         """
         if not hasattr(targetpool, 'name'):
             targetpool = self.ex_get_targetpool(targetpool)
-        if not hasattr(node, 'name'):
-            node = self.ex_get_node(node, 'all')
+        if hasattr(node, 'name'):
+            node_uri = node.extra['selfLink']
+        else:
+            if node.startswith('https://'):
+                node_uri = node
+            else:
+                node = self.ex_get_node(node, 'all')
+                node_uri = node.extra['selfLink']
 
-        targetpool_data = {'instances': [{'instance': node.extra['selfLink']}]}
+        targetpool_data = {'instances': [{'instance': node_uri}]}
 
         request = '/regions/%s/targetPools/%s/addInstance' % (
             targetpool.region.name, targetpool.name)
         self.connection.async_request(request, method='POST',
                                       data=targetpool_data)
-        targetpool.nodes.append(node)
+        if all((node_uri != n) and
+               (not hasattr(n, 'extra') or n.extra['selfLink'] != node_uri)
+               for n in targetpool.nodes):
+            targetpool.nodes.append(node)
         return True
 
     def ex_targetpool_add_healthcheck(self, targetpool, healthcheck):
@@ -1643,7 +1672,9 @@ def ex_targetpool_add_healthcheck(self, targetpool, healthcheck):
         if not hasattr(healthcheck, 'name'):
             healthcheck = self.ex_get_healthcheck(healthcheck)
 
-        targetpool_data = {'healthCheck': healthcheck.extra['selfLink']}
+        targetpool_data = {
+            'healthChecks': [{'healthCheck': healthcheck.extra['selfLink']}]
+        }
 
         request = '/regions/%s/targetPools/%s/addHealthCheck' % (
             targetpool.region.name, targetpool.name)
@@ -1667,10 +1698,17 @@ def ex_targetpool_remove_node(self, targetpool, node):
         """
         if not hasattr(targetpool, 'name'):
             targetpool = self.ex_get_targetpool(targetpool)
-        if not hasattr(node, 'name'):
-            node = self.ex_get_node(node, 'all')
 
-        targetpool_data = {'instances': [{'instance': node.extra['selfLink']}]}
+        if hasattr(node, 'name'):
+            node_uri = node.extra['selfLink']
+        else:
+            if node.startswith('https://'):
+                node_uri = node
+            else:
+                node = self.ex_get_node(node, 'all')
+                node_uri = node.extra['selfLink']
+
+        targetpool_data = {'instances': [{'instance': node_uri}]}
 
         request = '/regions/%s/targetPools/%s/removeInstance' % (
             targetpool.region.name, targetpool.name)
@@ -1679,7 +1717,8 @@ def ex_targetpool_remove_node(self, targetpool, node):
         # Remove node object from node list
         index = None
         for i, nd in enumerate(targetpool.nodes):
-            if nd.name == node.name:
+            if nd == node_uri or (hasattr(nd, 'extra') and
+                                  nd.extra['selfLink'] == node_uri):
                 index = i
                 break
         if index is not None:
@@ -1704,7 +1743,9 @@ def ex_targetpool_remove_healthcheck(self, targetpool, healthcheck):
         if not hasattr(healthcheck, 'name'):
             healthcheck = self.ex_get_healthcheck(healthcheck)
 
-        targetpool_data = {'healthCheck': healthcheck.extra['selfLink']}
+        targetpool_data = {
+            'healthChecks': [{'healthCheck': healthcheck.extra['selfLink']}]
+        }
 
         request = '/regions/%s/targetPools/%s/removeHealthCheck' % (
             targetpool.region.name, targetpool.name)

libcloud/test/compute/fixtures/gce/aggregated_forwardingRules.json

@@ -34,6 +34,7 @@
           "id": "06342111469679701315",
           "kind": "compute#forwardingRule",
           "name": "lcforwardingrule",
+          "description": "test forwarding rule",
           "portRange": "8000-8500",
           "region": "https://www.googleapis.com/compute/v1/projects/project_name/regions/us-central1",
           "selfLink": "https://www.googleapis.com/compute/v1/projects/project_name/regions/us-central1/forwardingRules/lcforwardingrule",

libcloud/test/compute/fixtures/gce/global_httpHealthChecks.json

@@ -24,6 +24,7 @@
       "id": "022194976205566532",
       "kind": "compute#httpHealthCheck",
       "name": "lchealthcheck",
+      "description": "test healthcheck",
       "port": 9000,
       "requestPath": "/lc",
       "selfLink": "https://www.googleapis.com/compute/v1/projects/project_name/global/httpHealthChecks/lchealthcheck",

libcloud/test/compute/fixtures/gce/global_httpHealthChecks_lchealthcheck.json

@@ -1,6 +1,7 @@
 {
   "checkIntervalSec": 10,
   "creationTimestamp": "2013-09-02T22:18:01.180-07:00",
+  "description": "test healthcheck",
   "healthyThreshold": 3,
   "host": "lchost",
   "id": "06860603312991823381",

libcloud/test/compute/fixtures/gce/regions_us-central1_forwardingRules_lcforwardingrule.json

@@ -5,6 +5,7 @@
   "id": "10901665092293158938",
   "kind": "compute#forwardingRule",
   "name": "lcforwardingrule",
+  "description": "test forwarding rule",
   "portRange": "8000-8500",
   "region": "https://www.googleapis.com/compute/v1/projects/project_name/regions/us-central1",
   "selfLink": "https://www.googleapis.com/compute/v1/projects/project_name/regions/us-central1/forwardingRules/lcforwardingrule",

libcloud/test/compute/test_gce.py

@@ -231,13 +231,16 @@ def test_ex_create_healthcheck(self):
                   'interval': 10,
                   'timeout': 10,
                   'unhealthy_threshold': 4,
-                  'healthy_threshold': 3}
+                  'healthy_threshold': 3,
+                  'description': 'test healthcheck'}
         hc = self.driver.ex_create_healthcheck(healthcheck_name, **kwargs)
         self.assertTrue(isinstance(hc, GCEHealthCheck))
         self.assertEqual(hc.name, healthcheck_name)
         self.assertEqual(hc.path, '/lc')
         self.assertEqual(hc.port, 8000)
         self.assertEqual(hc.interval, 10)
+        self.assertEqual(hc.extra['host'], 'lchost')
+        self.assertEqual(hc.extra['description'], 'test healthcheck')
 
     def test_ex_create_firewall(self):
         firewall_name = 'lcfirewall'
@@ -252,11 +255,18 @@ def test_ex_create_forwarding_rule(self):
         fwr_name = 'lcforwardingrule'
         targetpool = 'lctargetpool'
         region = 'us-central1'
+        port_range='8000-8500'
+        description = 'test forwarding rule'
         fwr = self.driver.ex_create_forwarding_rule(fwr_name, targetpool,
                                                     region=region,
-                                                    port_range='8000-8500')
+                                                    port_range=port_range,
+                                                    description=description)
         self.assertTrue(isinstance(fwr, GCEForwardingRule))
         self.assertEqual(fwr.name, fwr_name)
+        self.assertEqual(fwr.region.name, region)
+        self.assertEqual(fwr.protocol, 'TCP')
+        self.assertEqual(fwr.extra['portRange'], port_range)
+        self.assertEqual(fwr.extra['description'], description)
 
     def test_ex_create_network(self):
         network_name = 'lcnetwork'
@@ -413,10 +423,23 @@ def test_ex_targetpool_remove_add_node(self):
         self.assertTrue(remove_node)
         self.assertEqual(len(targetpool.nodes), 1)
 
+        add_node = self.driver.ex_targetpool_add_node(targetpool, node.extra['selfLink'])
+        self.assertTrue(add_node)
+        self.assertEqual(len(targetpool.nodes), 2)
+
+        remove_node = self.driver.ex_targetpool_remove_node(targetpool, node.extra['selfLink'])
+        self.assertTrue(remove_node)
+        self.assertEqual(len(targetpool.nodes), 1)
+
         add_node = self.driver.ex_targetpool_add_node(targetpool, node)
         self.assertTrue(add_node)
         self.assertEqual(len(targetpool.nodes), 2)
 
+        # check that duplicates are filtered
+        add_node = self.driver.ex_targetpool_add_node(targetpool, node.extra['selfLink'])
+        self.assertTrue(add_node)
+        self.assertEqual(len(targetpool.nodes), 2)
+
     def test_ex_targetpool_remove_add_healthcheck(self):
         targetpool = self.driver.ex_get_targetpool('lctargetpool')
         healthcheck = self.driver.ex_get_healthcheck(