Merge lp:~hazmat/pyjuju/ostack-diablo-hp-compat into lp:pyjuju

[Kapil Thangavelu]

Openstack security group api compat with hpcloud

It looks like the issue is the server security group details don't have rule
information in the diablo/hpcloud api fallback which caused problems for juju
when trying to determine what ports where open in the group (couldn't close
ports, and attempted to open ports multiple times). The simple workaround is a
specific path for hpcloud that fetches the group detail info separately if
server group details don't have rule info.

Verified against folsom/canonistack and hpcloud/diablo.

https://bugs.launchpad.net/juju/+bug/1092343

https://codereview.appspot.com/6966047/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+140815_code.launchpad.net,

Message:
Please take a look.

Description:
Openstack security group api compat with hpcloud

It looks like the issue is the server security group details don't have
rule
information in the diablo/hpcloud api fallback which caused problems for
juju
when trying to determine what ports where open in the group (couldn't
close
ports, and attempted to open ports multiple times). The simple
workaround is a
specific path for hpcloud that fetches the group detail info separately
if
server group details don't have rule info.

https://bugs.launchpad.net/juju/+bug/1092343

https://code.launchpad.net/~hazmat/juju/ostack-diablo-hp-compat/+merge/140815

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/6966047/

Affected files:
   A [revision details]
   M juju/providers/openstack/client.py
   M juju/providers/openstack/ports.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/providers/openstack/client.py
=== modified file 'juju/providers/openstack/client.py'
--- juju/providers/openstack/client.py 2012-10-26 14:52:35 +0000
+++ juju/providers/openstack/client.py 2012-12-20 03:54:46 +0000
@@ -411,12 +411,18 @@
          d = self.get(
              ["servers", server_id, "os-security-groups"],
              root="security_groups")
- # 2012-07-12: Workaround lack of this api in HP cloud
- d.addErrback(
- lambda f: self.get_server(server_id).addCallback(
- operator.itemgetter("security_groups")))
+
+ # 2012-07-12: kt Workaround lack of this api in HP cloud
+ def _get_group_fallback(f):
+ log.debug("Falling back to older/diablo sec groups api")
+ return self.get_server(server_id).addCallback(
+ operator.itemgetter("security_groups"))
+ d.addErrback(_get_group_fallback)
          return d

+ def get_security_group_details(self, group_id):
+ return self.get(["os-security-groups", group_id], "security_group")
+
      def list_security_groups(self):
          return self.get(["os-security-groups"], "security_groups")

Index: juju/providers/openstack/ports.py
=== modified file 'juju/providers/openstack/ports.py'
--- juju/providers/openstack/ports.py 2012-09-19 05:34:00 +0000
+++ juju/providers/openstack/ports.py 2012-12-20 03:54:46 +0000
@@ -55,9 +55,20 @@
          group_name = self._machine_group_name(machine_id)
          server_id = machine.instance_id
          groups = yield self.nova.get_server_security_groups(server_id)
+
+ found = False
          for group in groups:
              if group['name'] == group_name:
- returnValue(group)
+ found = group
+ break
+
+ # 2012/12/19: kt diablo/hpcloud compatibility
+ if found and not 'rules' in group:
+ group = yield self.nova.get_security_group_details(group['id'])
+
+...

[Jim Baker (jimbaker)]

+1, LGTM. The workaround would be clearer if inlineCallbacks were used in the original code, but no point changing the style now with this fix. Assumes the obvious trivial test that has been promised on #juju.

review: Approve

[Martin Packman (gz)]

With the latest rev looks okay, would really like a test of some kind though. Am more than a little worried about deployment quirks like this being missed in the reimplimentation in juju-core, a clear-ish test with the observed behaviour would make for reasonably easy porting.

[Kapil Thangavelu (hazmat)]

added test and pushed again.

On Tue, Jan 8, 2013 at 11:14 AM, Martin Packman <
<email address hidden>> wrote:

> With the latest rev looks okay, would really like a test of some kind
> though. Am more than a little worried about deployment quirks like this
> being missed in the reimplimentation in juju-core, a clear-ish test with
> the observed behaviour would make for reasonably easy porting.
> --
>
> https://code.launchpad.net/~hazmat/juju/ostack-diablo-hp-compat/+merge/140815
> You are the owner of lp:~hazmat/juju/ostack-diablo-hp-compat.
>

[Kapil Thangavelu (hazmat)]

There's a caveat to this work around which needs consideration/verification
namely that empty groups on essex/folsom still return a rules entry in the
dict.

On Tue, Jan 8, 2013 at 1:21 PM, Kapil Thangavelu <
<email address hidden>> wrote:

> added test and pushed again.
>
>
> On Tue, Jan 8, 2013 at 11:14 AM, Martin Packman <
> <email address hidden>> wrote:
>
>> With the latest rev looks okay, would really like a test of some kind
>> though. Am more than a little worried about deployment quirks like this
>> being missed in the reimplimentation in juju-core, a clear-ish test with
>> the observed behaviour would make for reasonably easy porting.
>> --
>>
>> https://code.launchpad.net/~hazmat/juju/ostack-diablo-hp-compat/+merge/140815
>> You are the owner of lp:~hazmat/juju/ostack-diablo-hp-compat.
>>
>
>

[Kapil Thangavelu (hazmat)]

Please take a look.

https://codereview.appspot.com/6966047/

[lp:~hazmat/pyjuju/ostack-diablo-hp-compat]

609. By Kapil Thangavelu on 2013-01-13

pep8/pyflakes cleanups

[Kapil Thangavelu (hazmat)]

*** Submitted:

Openstack security group api compat with hpcloud

It looks like the issue is the server security group details don't have
rule
information in the diablo/hpcloud api fallback which caused problems for
juju
when trying to determine what ports where open in the group (couldn't
close
ports, and attempted to open ports multiple times). The simple
workaround is a
specific path for hpcloud that fetches the group detail info separately
if
server group details don't have rule info.

Verified against folsom/canonistack and hpcloud/diablo.

https://bugs.launchpad.net/juju/+bug/1092343

R=
CC=
https://codereview.appspot.com/6966047

https://codereview.appspot.com/6966047/

=== modified file 'juju/providers/openstack/client.py'
--- juju/providers/openstack/client.py	2012-10-26 14:52:35 +0000
+++ juju/providers/openstack/client.py	2013-01-13 18:47:19 +0000
@@ -391,7 +391,7 @@
         return self.delete(["servers", server_id], code=204)
 
     def run_server(self, image_id, flavor_id, name, security_group_names=None,
-            user_data=None, scheduler_hints=None):
+                   user_data=None, scheduler_hints=None):
         server = {
             'name': name,
             'flavorRef': flavor_id,
@@ -411,12 +411,18 @@
         d = self.get(
             ["servers", server_id, "os-security-groups"],
             root="security_groups")
-        # 2012-07-12: Workaround lack of this api in HP cloud
-        d.addErrback(
-            lambda f: self.get_server(server_id).addCallback(
-                operator.itemgetter("security_groups")))
+
+        # 2012-07-12: kt Workaround lack of this api in HP cloud
+        def _get_group_fallback(f):
+            log.debug("Falling back to older/diablo sec groups api")
+            return self.get_server(server_id).addCallback(
+                operator.itemgetter("security_groups"))
+        d.addErrback(_get_group_fallback)
         return d
 
+    def get_security_group_details(self, group_id):
+        return self.get(["os-security-groups", group_id], "security_group")
+
     def list_security_groups(self):
         return self.get(["os-security-groups"], "security_groups")
 

=== modified file 'juju/providers/openstack/ports.py'
--- juju/providers/openstack/ports.py	2012-09-19 05:34:00 +0000
+++ juju/providers/openstack/ports.py	2013-01-13 18:47:19 +0000
@@ -55,21 +55,34 @@
         group_name = self._machine_group_name(machine_id)
         server_id = machine.instance_id
         groups = yield self.nova.get_server_security_groups(server_id)
+
+        found = False
         for group in groups:
             if group['name'] == group_name:
-                returnValue(group)
+                found = group
+                break
+
+        # 2012/12/19: kt diablo/hpcloud compatibility
+        if found and not 'rules' in group:
+            group = yield self.nova.get_security_group_details(group['id'])
+            found = True
+
+        if found:
+            returnValue(group)
+
         raise errors.ProviderInteractionError(
             "Missing security group %r for machine %r" %
-                (group_name, server_id))
+            (group_name, server_id))
 
     @inlineCallbacks
     def open_port(self, machine, machine_id, port, protocol="tcp"):
         """Allow access to a port for the given machine only"""
         group = yield self._get_machine_group(machine, machine_id)
-        yield self.nova.add_security_group_rule(group['id'],
+        yield self.nova.add_security_group_rule(
+            group['id'],
             ip_protocol=protocol, from_port=port, to_port=port)
         log.debug("Opened %s/%s on machine %r",
-            port, protocol, machine.instance_id)
+                  port, protocol, machine.instance_id)
 
     @inlineCallbacks
     def close_port(self, machine, machine_id, port, protocol="tcp"):
@@ -78,13 +91,14 @@
         for rule in group["rules"]:
             if (port == rule["from_port"] == rule["to_port"] and
                 rule["ip_protocol"] == protocol):
+
                 yield self.nova.delete_security_group_rule(rule["id"])
                 log.debug("Closed %s/%s on machine %r",
                           port, protocol, machine.instance_id)
                 return
         raise errors.ProviderInteractionError(
             "Couldn't close unopened %s/%s on machine %r",
-                  port, protocol, machine.instance_id)
+            port, protocol, machine.instance_id)
 
     @inlineCallbacks
     def get_opened_ports(self, machine, machine_id):
@@ -113,11 +127,12 @@
         juju_group = self._juju_group_name()
         if not juju_group in groups_by_name:
             log.debug("Creating juju security group %s", juju_group)
-            sg = yield self.nova.create_security_group(juju_group,
+            sg = yield self.nova.create_security_group(
+                juju_group,
                 "juju group for %s" % (self.tag,))
             # Add external ssh access
-            yield self.nova.add_security_group_rule(sg['id'],
-                ip_protocol="tcp", from_port=22, to_port=22)
+            yield self.nova.add_security_group_rule(
+                sg['id'], ip_protocol="tcp", from_port=22, to_port=22)
             # Add internal group access
             yield self.nova.add_security_group_rule(
                 parent_group_id=sg['id'], group_id=sg['id'],
@@ -131,7 +146,8 @@
             yield self.nova.delete_security_group(
                 groups_by_name[machine_group])
         log.debug("Creating machine security group %s", machine_group)
-        yield self.nova.create_security_group(machine_group,
+        yield self.nova.create_security_group(
+            machine_group,
             "juju group for %s machine %s" % (self.tag, machine_id))
 
         returnValue([juju_group, machine_group])
@@ -157,7 +173,8 @@
         server_id = machine.instance_id
         groups = yield self.nova.get_server_security_groups(server_id)
         juju_group = self._juju_group_name()
-        groups_by_name = dict((g['name'], g['id']) for g in groups
+        groups_by_name = dict(
+            (g['name'], g['id']) for g in groups
             if g['name'].startswith(juju_group))
         if juju_group not in groups_by_name:
             # Not a juju machine, shouldn't touch

=== modified file 'juju/providers/openstack/tests/test_ports.py'
--- juju/providers/openstack/tests/test_ports.py	2012-09-19 05:34:00 +0000
+++ juju/providers/openstack/tests/test_ports.py	2013-01-13 18:47:19 +0000
@@ -39,10 +39,10 @@
 
     def test_open_port(self):
         """Opening a port adds the rule to the appropriate security group"""
-        self.expect_nova_get("servers/1000/os-security-groups",
+        self.expect_nova_get(
+            "servers/1000/os-security-groups",
             response={'security_groups': [
-                    {'name': "juju-testing-1", 'id': 1},
-                ]})
+                {'name': "juju-testing-1", 'id': 1, 'rules': []}]})
         self.expect_create_rule(1, "tcp", 80)
         self.mocker.replay()
 
@@ -52,9 +52,34 @@
 
         def _check_log(_):
             self.assertIn("Opened 80/tcp on machine '1000'",
-                log.getvalue())
+                          log.getvalue())
         return deferred.addCallback(_check_log)
 
+    def test_diablo_hpcloud_ccompatbility(self):
+        """Verify compatibility workarounds for hpcloud/diablo."""
+        self.expect_nova_get(
+            "servers/1000/os-security-groups",
+            response={'security_groups': [
+                {'name': "juju-testing-1", 'id': 1}]})
+
+        self.expect_nova_get(
+            "os-security-groups/1",
+            response={'security_group': {
+                'name': "juju-testing-1",
+                'id': 1,
+                'rules': [{
+                    'id': 1,
+                    'parent_group_id': 1,
+                    'ip_protocol': 'tcp',
+                    'from_port': 80,
+                    'to_port': 80}]}})
+
+        self.mocker.replay()
+
+        machine = NovaProviderMachine('1000', "server1000.testing.invalid")
+        deferred = self.get_provider().get_opened_ports(machine, "1")
+        return deferred.addCallback(self.assertEqual, set([(80, "tcp")]))
+
     def test_open_port_missing_group(self):
         """Missing security group raises an error on deleting port"""
         self.expect_nova_get("servers/1000/os-security-groups",