Merge lp:~anso/nova/ec2-security-groups into lp:~soren/nova/ec2-security-groups

[Vish Ishaya]

Fixes deleted objects showing in relations and a few issues with too many rules getting revoked.

[Devin Carlen (devcamcar)]

Looks good except the syntax of the primaryjoins and secondaryjoins is strange. What's with the double quotes? It looks unnecessary based on the sqlalchemy docs, which gives this example of using primary join:

primaryjoin=and_(users_table.c.user_id==addresses_table.c.user_id,
                addresses_table.c.city=='Boston'))

Examples from patch:

219 + secondaryjoin="and_(SecurityGroup.id == SecurityGroupInstanceAssociation.security_group_id,"
220 + "Instance.id == SecurityGroupInstanceAssociation.instance_id,"
221 + "SecurityGroup.deleted == False,"
222 + "Instance.deleted == False)",

and

231 + primaryjoin="and_(SecurityGroupIngressRule.parent_group_id == SecurityGroup.id,"
232 + "SecurityGroupIngressRule.deleted == False)")

review: Needs Information

[Vish Ishaya (vishvananda)]

the primary and secondary were wrong, the quotes is fine though. If you surround it in quotes, you can refer to tables that haven't been defined yet or are in the process of being defined. Reproposing.

[lp:~anso/nova/ec2-security-groups]

318. By Vish Ishaya on 2010-09-29

fix ordering of rules to actually allow out and drop in

319. By Vish Ishaya on 2010-10-01

show project ids for groups instead of user ids

320. By Todd Willey on 2010-10-02

Merge to latest.

Unmerged revisions

320. By Todd Willey on 2010-10-02

Merge to latest.

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2010-09-29 17:25:57 +0000
+++ nova/api/ec2/cloud.py	2010-09-29 17:25:57 +0000
@@ -296,7 +296,7 @@
                     db.security_group_get_by_name(context,
                                                   source_project_id,
                                                   source_security_group_name)
-            values['group_id'] = source_security_group.id
+            values['group_id'] = source_security_group['id']
         elif cidr_ip:
             # If this fails, it throws an exception. This is what we want.
             IPy.IP(cidr_ip)
@@ -333,17 +333,19 @@
                                                        group_name)
 
         criteria = self._authorize_revoke_rule_args_to_dict(context, **kwargs)
+        if criteria == None:
+            raise exception.ApiError("No rule for the specified parameters.")
 
         for rule in security_group.rules:
+            match = True
             for (k,v) in criteria.iteritems():
                 if getattr(rule, k, False) != v:
-                    break
-            # If we make it here, we have a match
-            db.security_group_rule_destroy(context, rule.id)
-
-        self._trigger_refresh_security_group(security_group)
-
-        return True
+                    match = False
+            if match:
+                db.security_group_rule_destroy(context, rule['id'])
+                self._trigger_refresh_security_group(security_group)
+                return True
+        raise exception.ApiError("No rule for the specified parameters.")
 
     # TODO(soren): Dupe detection. Adding the same rule twice actually
     #              adds the same rule twice to the rule set, which is
@@ -387,7 +389,7 @@
 
     def create_security_group(self, context, group_name, group_description):
         self._ensure_default_security_group(context)
-        if db.securitygroup_exists(context, context.project.id, group_name):
+        if db.security_group_exists(context, context.project.id, group_name):
             raise exception.ApiError('group %s already exists' % group_name)
 
         group = {'user_id' : context.user.id,
@@ -779,15 +781,11 @@
         base_options['project_id'] = context.project.id
         base_options['user_data'] = kwargs.get('user_data', '')
 
-        type_data = INSTANCE_TYPES[instance_type]
-        base_options['instance_type'] = instance_type
-<<<<<<< TREE
-=======
         base_options['display_name'] = kwargs.get('display_name')
         base_options['display_description'] = kwargs.get('display_description')
 
         type_data = INSTANCE_TYPES[instance_type]
->>>>>>> MERGE-SOURCE
+        base_options['instance_type'] = instance_type
         base_options['memory_mb'] = type_data['memory_mb']
         base_options['vcpus'] = type_data['vcpus']
         base_options['local_gb'] = type_data['local_gb']

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2010-09-29 17:25:57 +0000
+++ nova/db/api.py	2010-09-29 17:25:57 +0000
@@ -600,7 +600,7 @@
     return IMPL.security_group_get_by_instance(context, instance_id)
 
 
-def securitygroup_exists(context, project_id, group_name):
+def security_group_exists(context, project_id, group_name):
     """Indicates if a group name exists in a project"""
     return IMPL.security_group_exists(context, project_id, group_name)
 

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2010-09-29 17:25:57 +0000
+++ nova/db/sqlalchemy/api.py	2010-09-29 17:25:57 +0000
@@ -28,14 +28,9 @@
 from nova.db.sqlalchemy import models
 from nova.db.sqlalchemy.session import get_session
 from sqlalchemy import or_
-<<<<<<< TREE
-from sqlalchemy.orm import eagerload, joinedload_all
-from sqlalchemy.sql import func
-=======
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm import joinedload_all
 from sqlalchemy.sql import exists, func
->>>>>>> MERGE-SOURCE
 
 FLAGS = flags.FLAGS
 
@@ -415,8 +410,17 @@
 
 
 def instance_get(context, instance_id):
-    return models.Instance().find(instance_id, deleted=_deleted(context),
-                                    options=eagerload('security_groups'))
+    session = get_session()
+    instance_ref = session.query(models.Instance
+                    ).filter_by(id=instance_id
+                    ).filter_by(deleted=_deleted(context)
+                    ).options(joinedload_all('security_groups')
+                    ).options(joinedload_all('fixed_ip.floating_ips')
+                    ).first()
+    if not instance_ref:
+        raise exception.NotFound('Instance %s not found' % (instance_id))
+
+    return instance_ref
 
 
 def instance_get_all(context):
@@ -950,16 +954,18 @@
 def security_group_get_all(_context):
     session = get_session()
     return session.query(models.SecurityGroup
-                 ).options(eagerload('rules')
                  ).filter_by(deleted=False
+                 ).options(joinedload_all('rules')
                  ).all()
 
 
 def security_group_get(_context, security_group_id):
     session = get_session()
     result = session.query(models.SecurityGroup
-                   ).options(eagerload('rules')
-                   ).get(security_group_id)
+                   ).filter_by(deleted=False
+                   ).filter_by(id=security_group_id
+                   ).options(joinedload_all('rules')
+                   ).first()
     if not result:
         raise exception.NotFound("No secuity group with id %s" %
                                  security_group_id)
@@ -968,36 +974,38 @@
 
 def security_group_get_by_name(context, project_id, group_name):
     session = get_session()
-    group_ref = session.query(models.SecurityGroup
-                      ).options(eagerload('rules')
-                      ).options(eagerload('instances')
+    result = session.query(models.SecurityGroup
                       ).filter_by(project_id=project_id
                       ).filter_by(name=group_name
                       ).filter_by(deleted=False
+                      ).options(joinedload_all('rules')
+                      ).options(joinedload_all('instances')
                       ).first()
-    if not group_ref:
+    if not result:
         raise exception.NotFound(
             'No security group named %s for project: %s' \
              % (group_name, project_id))
-    return group_ref
+    return result
 
 
 def security_group_get_by_project(_context, project_id):
     session = get_session()
     return session.query(models.SecurityGroup
-                 ).options(eagerload('rules')
                  ).filter_by(project_id=project_id
                  ).filter_by(deleted=False
+                 ).options(joinedload_all('rules')
                  ).all()
 
 
 def security_group_get_by_instance(_context, instance_id):
     session = get_session()
-    with session.begin():
-        return session.query(models.Instance
-                      ).join(models.Instance.security_groups
-                      ).filter_by(deleted=False
-                      ).all()
+    return session.query(models.SecurityGroup
+                 ).filter_by(deleted=False
+                 ).options(joinedload_all('rules')
+                 ).join(models.SecurityGroup.instances
+                 ).filter_by(id=instance_id
+                 ).filter_by(deleted=False
+                 ).all()
 
 
 def security_group_exists(_context, project_id, group_name):
@@ -1023,7 +1031,7 @@
     session = get_session()
     with session.begin():
         # TODO(vish): do we have to use sql here?
-        session.execute('update security_group set deleted=1 where id=:id',
+        session.execute('update security_groups set deleted=1 where id=:id',
                         {'id': security_group_id})
         session.execute('update security_group_rules set deleted=1 '
                         'where group_id=:id',
@@ -1033,7 +1041,7 @@
     session = get_session()
     with session.begin():
         # TODO(vish): do we have to use sql here?
-        session.execute('update security_group set deleted=1')
+        session.execute('update security_groups set deleted=1')
         session.execute('update security_group_rules set deleted=1')
 
 ###################

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2010-09-29 17:25:57 +0000
+++ nova/db/sqlalchemy/models.py	2010-09-29 17:25:57 +0000
@@ -25,7 +25,7 @@
 
 # TODO(vish): clean up these imports
 from sqlalchemy.orm import relationship, backref, exc, object_mapper
-from sqlalchemy import Column, Integer, String, Table
+from sqlalchemy import Column, Integer, String
 from sqlalchemy import ForeignKey, DateTime, Boolean, Text
 from sqlalchemy.ext.declarative import declarative_base
 
@@ -340,16 +340,16 @@
                                                   uselist=False))
 
 
-security_group_instance_association = Table('security_group_instance_association',
-                                            BASE.metadata,
-                                            Column('security_group_id', Integer,
-                                                   ForeignKey('security_group.id')),
-                                            Column('instance_id', Integer,
-                                                   ForeignKey('instances.id')))
+class SecurityGroupInstanceAssociation(BASE, NovaBase):
+    __tablename__ = 'security_group_instance_association'
+    id = Column(Integer, primary_key=True)
+    security_group_id = Column(Integer, ForeignKey('security_groups.id'))
+    instance_id = Column(Integer, ForeignKey('instances.id'))
+
 
 class SecurityGroup(BASE, NovaBase):
     """Represents a security group"""
-    __tablename__ = 'security_group'
+    __tablename__ = 'security_groups'
     id = Column(Integer, primary_key=True)
 
     name = Column(String(255))
@@ -358,7 +358,11 @@
     project_id = Column(String(255))
 
     instances = relationship(Instance,
-                             secondary=security_group_instance_association,
+                             secondary="security_group_instance_association",
+                             primaryjoin="and_(SecurityGroup.id == SecurityGroupInstanceAssociation.security_group_id,"
+                                               "SecurityGroup.deleted == False)",
+                             secondaryjoin="and_(SecurityGroupInstanceAssociation.instance_id == Instance.id,"
+                                                 "Instance.deleted == False)",
                              backref='security_groups')
 
     @property
@@ -375,10 +379,11 @@
     __tablename__ = 'security_group_rules'
     id = Column(Integer, primary_key=True)
 
-    parent_group_id = Column(Integer, ForeignKey('security_group.id'))
+    parent_group_id = Column(Integer, ForeignKey('security_groups.id'))
     parent_group = relationship("SecurityGroup", backref="rules",
                          foreign_keys=parent_group_id,
-                         primaryjoin=parent_group_id==SecurityGroup.id)
+                         primaryjoin="and_(SecurityGroupIngressRule.parent_group_id == SecurityGroup.id,"
+                                          "SecurityGroupIngressRule.deleted == False)")
 
     protocol = Column(String(5)) # "tcp", "udp", or "icmp"
     from_port = Column(Integer)
@@ -387,7 +392,7 @@
 
     # Note: This is not the parent SecurityGroup. It's SecurityGroup we're
     # granting access for.
-    group_id = Column(Integer, ForeignKey('security_group.id'))
+    group_id = Column(Integer, ForeignKey('security_groups.id'))
 
 
 class KeyPair(BASE, NovaBase):
@@ -541,7 +546,7 @@
     from sqlalchemy import create_engine
     models = (Service, Instance, Volume, ExportDevice, FixedIp, FloatingIp,
               Network, NetworkIndex, SecurityGroup, SecurityGroupIngressRule,
-              AuthToken) # , Image, Host
+              SecurityGroupInstanceAssociation, AuthToken) # , Image, Host
     engine = create_engine(FLAGS.sql_connection, echo=False)
     for model in models:
         model.metadata.create_all(engine)

=== modified file 'nova/tests/virt_unittest.py'
--- nova/tests/virt_unittest.py	2010-09-29 07:46:37 +0000
+++ nova/tests/virt_unittest.py	2010-09-29 17:25:57 +0000
@@ -185,7 +185,7 @@
         inst_id = instance_ref['id']
 
         def _ensure_all_called(_):
-            instance_filter = 'nova-instance-%s' % instance_ref['str_id']
+            instance_filter = 'nova-instance-%s' % instance_ref['name']
             secgroup_filter = 'nova-secgroup-%s' % self.security_group['id']
             for required in [secgroup_filter, 'allow-dhcp-server',
                              'no-arp-spoofing', 'no-ip-spoofing',