OpenStack Compute (nova)

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

  1. ec2-security-groups
  2. Merge into ec2-security-groups
Proposed by Vish Ishaya
Status: Merged
Merged at revision: 312
Proposed branch: lp:~anso/nova/ec2-security-groups
Merge into: lp:~soren/nova/ec2-security-groups
Prerequisite: lp:~hudson-openstack/nova/trunk
Diff against target: 346 lines (+69/-58)
6 files modified
nova/api/ec2/cloud.py (+14/-16)
nova/db/api.py (+1/-1)
nova/db/sqlalchemy/api.py (+31/-23)
nova/db/sqlalchemy/models.py (+18/-13)
nova/tests/virt_unittest.py (+1/-1)
nova/virt/libvirt_conn.py (+4/-4)
To merge this branch: bzr merge lp:~anso/nova/ec2-security-groups
Reviewer Review Type Date Requested Status
Devin Carlen (community) Approve
Review via email: mp+37030@code.launchpad.net

This proposal supersedes a proposal from 2010-09-29.

Description of the change

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

To post a comment you must log in.
Revision history for this message
Devin Carlen (devcamcar) wrote : Posted in a previous version of this proposal

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
Revision history for this message
Vish Ishaya (vishvananda) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Vish Ishaya (vishvananda) wrote :

This version acually works! :(

Revision history for this message
Devin Carlen (devcamcar) wrote :

+ instance_filter = 'nova-instance-%s' % instance_ref['name']

should be

+ instance_filter = 'nova-instance-%s' % instance_ref['ec2_id']

review: Needs Fixing
Revision history for this message
Devin Carlen (devcamcar) wrote :

Talked with Vish, name is correct, lgtm

review: Approve
lp:~anso/nova/ec2-security-groups updated
319. By Vish Ishaya

show project ids for groups instead of user ids

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'nova/api/ec2/cloud.py'
2--- nova/api/ec2/cloud.py 2010-10-01 06:44:51 +0000
3+++ nova/api/ec2/cloud.py 2010-10-01 06:44:51 +0000
4@@ -260,7 +260,7 @@
5 g = {}
6 g['groupDescription'] = group.description
7 g['groupName'] = group.name
8- g['ownerId'] = context.user.id
9+ g['ownerId'] = group.project_id
10 g['ipPermissions'] = []
11 for rule in group.rules:
12 r = {}
13@@ -272,7 +272,7 @@
14 if rule.group_id:
15 source_group = db.security_group_get(context, rule.group_id)
16 r['groups'] += [{'groupName': source_group.name,
17- 'userId': source_group.user_id}]
18+ 'userId': source_group.project_id}]
19 else:
20 r['ipRanges'] += [{'cidrIp': rule.cidr}]
21 g['ipPermissions'] += [r]
22@@ -296,7 +296,7 @@
23 db.security_group_get_by_name(context,
24 source_project_id,
25 source_security_group_name)
26- values['group_id'] = source_security_group.id
27+ values['group_id'] = source_security_group['id']
28 elif cidr_ip:
29 # If this fails, it throws an exception. This is what we want.
30 IPy.IP(cidr_ip)
31@@ -333,17 +333,19 @@
32 group_name)
33
34 criteria = self._authorize_revoke_rule_args_to_dict(context, **kwargs)
35+ if criteria == None:
36+ raise exception.ApiError("No rule for the specified parameters.")
37
38 for rule in security_group.rules:
39+ match = True
40 for (k,v) in criteria.iteritems():
41 if getattr(rule, k, False) != v:
42- break
43- # If we make it here, we have a match
44- db.security_group_rule_destroy(context, rule.id)
45-
46- self._trigger_refresh_security_group(security_group)
47-
48- return True
49+ match = False
50+ if match:
51+ db.security_group_rule_destroy(context, rule['id'])
52+ self._trigger_refresh_security_group(security_group)
53+ return True
54+ raise exception.ApiError("No rule for the specified parameters.")
55
56 # TODO(soren): Dupe detection. Adding the same rule twice actually
57 # adds the same rule twice to the rule set, which is
58@@ -387,7 +389,7 @@
59
60 def create_security_group(self, context, group_name, group_description):
61 self._ensure_default_security_group(context)
62- if db.securitygroup_exists(context, context.project.id, group_name):
63+ if db.security_group_exists(context, context.project.id, group_name):
64 raise exception.ApiError('group %s already exists' % group_name)
65
66 group = {'user_id' : context.user.id,
67@@ -779,15 +781,11 @@
68 base_options['project_id'] = context.project.id
69 base_options['user_data'] = kwargs.get('user_data', '')
70
71- type_data = INSTANCE_TYPES[instance_type]
72- base_options['instance_type'] = instance_type
73-<<<<<<< TREE
74-=======
75 base_options['display_name'] = kwargs.get('display_name')
76 base_options['display_description'] = kwargs.get('display_description')
77
78 type_data = INSTANCE_TYPES[instance_type]
79->>>>>>> MERGE-SOURCE
80+ base_options['instance_type'] = instance_type
81 base_options['memory_mb'] = type_data['memory_mb']
82 base_options['vcpus'] = type_data['vcpus']
83 base_options['local_gb'] = type_data['local_gb']
84
85=== modified file 'nova/db/api.py'
86--- nova/db/api.py 2010-10-01 06:44:51 +0000
87+++ nova/db/api.py 2010-10-01 06:44:51 +0000
88@@ -600,7 +600,7 @@
89 return IMPL.security_group_get_by_instance(context, instance_id)
90
91
92-def securitygroup_exists(context, project_id, group_name):
93+def security_group_exists(context, project_id, group_name):
94 """Indicates if a group name exists in a project"""
95 return IMPL.security_group_exists(context, project_id, group_name)
96
97
98=== modified file 'nova/db/sqlalchemy/api.py'
99--- nova/db/sqlalchemy/api.py 2010-10-01 06:44:51 +0000
100+++ nova/db/sqlalchemy/api.py 2010-10-01 06:44:51 +0000
101@@ -28,14 +28,9 @@
102 from nova.db.sqlalchemy import models
103 from nova.db.sqlalchemy.session import get_session
104 from sqlalchemy import or_
105-<<<<<<< TREE
106-from sqlalchemy.orm import eagerload, joinedload_all
107-from sqlalchemy.sql import func
108-=======
109 from sqlalchemy.exc import IntegrityError
110 from sqlalchemy.orm import joinedload_all
111 from sqlalchemy.sql import exists, func
112->>>>>>> MERGE-SOURCE
113
114 FLAGS = flags.FLAGS
115
116@@ -415,8 +410,17 @@
117
118
119 def instance_get(context, instance_id):
120- return models.Instance().find(instance_id, deleted=_deleted(context),
121- options=eagerload('security_groups'))
122+ session = get_session()
123+ instance_ref = session.query(models.Instance
124+ ).filter_by(id=instance_id
125+ ).filter_by(deleted=_deleted(context)
126+ ).options(joinedload_all('security_groups')
127+ ).options(joinedload_all('fixed_ip.floating_ips')
128+ ).first()
129+ if not instance_ref:
130+ raise exception.NotFound('Instance %s not found' % (instance_id))
131+
132+ return instance_ref
133
134
135 def instance_get_all(context):
136@@ -950,16 +954,18 @@
137 def security_group_get_all(_context):
138 session = get_session()
139 return session.query(models.SecurityGroup
140- ).options(eagerload('rules')
141 ).filter_by(deleted=False
142+ ).options(joinedload_all('rules')
143 ).all()
144
145
146 def security_group_get(_context, security_group_id):
147 session = get_session()
148 result = session.query(models.SecurityGroup
149- ).options(eagerload('rules')
150- ).get(security_group_id)
151+ ).filter_by(deleted=False
152+ ).filter_by(id=security_group_id
153+ ).options(joinedload_all('rules')
154+ ).first()
155 if not result:
156 raise exception.NotFound("No secuity group with id %s" %
157 security_group_id)
158@@ -968,36 +974,38 @@
159
160 def security_group_get_by_name(context, project_id, group_name):
161 session = get_session()
162- group_ref = session.query(models.SecurityGroup
163- ).options(eagerload('rules')
164- ).options(eagerload('instances')
165+ result = session.query(models.SecurityGroup
166 ).filter_by(project_id=project_id
167 ).filter_by(name=group_name
168 ).filter_by(deleted=False
169+ ).options(joinedload_all('rules')
170+ ).options(joinedload_all('instances')
171 ).first()
172- if not group_ref:
173+ if not result:
174 raise exception.NotFound(
175 'No security group named %s for project: %s' \
176 % (group_name, project_id))
177- return group_ref
178+ return result
179
180
181 def security_group_get_by_project(_context, project_id):
182 session = get_session()
183 return session.query(models.SecurityGroup
184- ).options(eagerload('rules')
185 ).filter_by(project_id=project_id
186 ).filter_by(deleted=False
187+ ).options(joinedload_all('rules')
188 ).all()
189
190
191 def security_group_get_by_instance(_context, instance_id):
192 session = get_session()
193- with session.begin():
194- return session.query(models.Instance
195- ).join(models.Instance.security_groups
196- ).filter_by(deleted=False
197- ).all()
198+ return session.query(models.SecurityGroup
199+ ).filter_by(deleted=False
200+ ).options(joinedload_all('rules')
201+ ).join(models.SecurityGroup.instances
202+ ).filter_by(id=instance_id
203+ ).filter_by(deleted=False
204+ ).all()
205
206
207 def security_group_exists(_context, project_id, group_name):
208@@ -1023,7 +1031,7 @@
209 session = get_session()
210 with session.begin():
211 # TODO(vish): do we have to use sql here?
212- session.execute('update security_group set deleted=1 where id=:id',
213+ session.execute('update security_groups set deleted=1 where id=:id',
214 {'id': security_group_id})
215 session.execute('update security_group_rules set deleted=1 '
216 'where group_id=:id',
217@@ -1033,7 +1041,7 @@
218 session = get_session()
219 with session.begin():
220 # TODO(vish): do we have to use sql here?
221- session.execute('update security_group set deleted=1')
222+ session.execute('update security_groups set deleted=1')
223 session.execute('update security_group_rules set deleted=1')
224
225 ###################
226
227=== modified file 'nova/db/sqlalchemy/models.py'
228--- nova/db/sqlalchemy/models.py 2010-10-01 06:44:51 +0000
229+++ nova/db/sqlalchemy/models.py 2010-10-01 06:44:51 +0000
230@@ -25,7 +25,7 @@
231
232 # TODO(vish): clean up these imports
233 from sqlalchemy.orm import relationship, backref, exc, object_mapper
234-from sqlalchemy import Column, Integer, String, Table
235+from sqlalchemy import Column, Integer, String
236 from sqlalchemy import ForeignKey, DateTime, Boolean, Text
237 from sqlalchemy.ext.declarative import declarative_base
238
239@@ -340,16 +340,16 @@
240 uselist=False))
241
242
243-security_group_instance_association = Table('security_group_instance_association',
244- BASE.metadata,
245- Column('security_group_id', Integer,
246- ForeignKey('security_group.id')),
247- Column('instance_id', Integer,
248- ForeignKey('instances.id')))
249+class SecurityGroupInstanceAssociation(BASE, NovaBase):
250+ __tablename__ = 'security_group_instance_association'
251+ id = Column(Integer, primary_key=True)
252+ security_group_id = Column(Integer, ForeignKey('security_groups.id'))
253+ instance_id = Column(Integer, ForeignKey('instances.id'))
254+
255
256 class SecurityGroup(BASE, NovaBase):
257 """Represents a security group"""
258- __tablename__ = 'security_group'
259+ __tablename__ = 'security_groups'
260 id = Column(Integer, primary_key=True)
261
262 name = Column(String(255))
263@@ -358,7 +358,11 @@
264 project_id = Column(String(255))
265
266 instances = relationship(Instance,
267- secondary=security_group_instance_association,
268+ secondary="security_group_instance_association",
269+ primaryjoin="and_(SecurityGroup.id == SecurityGroupInstanceAssociation.security_group_id,"
270+ "SecurityGroup.deleted == False)",
271+ secondaryjoin="and_(SecurityGroupInstanceAssociation.instance_id == Instance.id,"
272+ "Instance.deleted == False)",
273 backref='security_groups')
274
275 @property
276@@ -375,10 +379,11 @@
277 __tablename__ = 'security_group_rules'
278 id = Column(Integer, primary_key=True)
279
280- parent_group_id = Column(Integer, ForeignKey('security_group.id'))
281+ parent_group_id = Column(Integer, ForeignKey('security_groups.id'))
282 parent_group = relationship("SecurityGroup", backref="rules",
283 foreign_keys=parent_group_id,
284- primaryjoin=parent_group_id==SecurityGroup.id)
285+ primaryjoin="and_(SecurityGroupIngressRule.parent_group_id == SecurityGroup.id,"
286+ "SecurityGroupIngressRule.deleted == False)")
287
288 protocol = Column(String(5)) # "tcp", "udp", or "icmp"
289 from_port = Column(Integer)
290@@ -387,7 +392,7 @@
291
292 # Note: This is not the parent SecurityGroup. It's SecurityGroup we're
293 # granting access for.
294- group_id = Column(Integer, ForeignKey('security_group.id'))
295+ group_id = Column(Integer, ForeignKey('security_groups.id'))
296
297
298 class KeyPair(BASE, NovaBase):
299@@ -541,7 +546,7 @@
300 from sqlalchemy import create_engine
301 models = (Service, Instance, Volume, ExportDevice, FixedIp, FloatingIp,
302 Network, NetworkIndex, SecurityGroup, SecurityGroupIngressRule,
303- AuthToken) # , Image, Host
304+ SecurityGroupInstanceAssociation, AuthToken) # , Image, Host
305 engine = create_engine(FLAGS.sql_connection, echo=False)
306 for model in models:
307 model.metadata.create_all(engine)
308
309=== modified file 'nova/tests/virt_unittest.py'
310--- nova/tests/virt_unittest.py 2010-09-29 07:46:37 +0000
311+++ nova/tests/virt_unittest.py 2010-10-01 06:44:51 +0000
312@@ -185,7 +185,7 @@
313 inst_id = instance_ref['id']
314
315 def _ensure_all_called(_):
316- instance_filter = 'nova-instance-%s' % instance_ref['str_id']
317+ instance_filter = 'nova-instance-%s' % instance_ref['name']
318 secgroup_filter = 'nova-secgroup-%s' % self.security_group['id']
319 for required in [secgroup_filter, 'allow-dhcp-server',
320 'no-arp-spoofing', 'no-ip-spoofing',
321
322=== modified file 'nova/virt/libvirt_conn.py'
323--- nova/virt/libvirt_conn.py 2010-09-28 07:47:25 +0000
324+++ nova/virt/libvirt_conn.py 2010-10-01 06:44:51 +0000
325@@ -527,8 +527,8 @@
326 def nova_base_ipv4_filter(self):
327 retval = "<filter name='nova-base-ipv4' chain='ipv4'>"
328 for protocol in ['tcp', 'udp', 'icmp']:
329- for direction,action,priority in [('out','accept', 400),
330- ('in','drop', 399)]:
331+ for direction,action,priority in [('out','accept', 399),
332+ ('inout','drop', 400)]:
333 retval += """<rule action='%s' direction='%s' priority='%d'>
334 <%s />
335 </rule>""" % (action, direction,
336@@ -540,8 +540,8 @@
337 def nova_base_ipv6_filter(self):
338 retval = "<filter name='nova-base-ipv6' chain='ipv6'>"
339 for protocol in ['tcp', 'udp', 'icmp']:
340- for direction,action,priority in [('out','accept',400),
341- ('in','drop',399)]:
342+ for direction,action,priority in [('out','accept',399),
343+ ('inout','drop',400)]:
344 retval += """<rule action='%s' direction='%s' priority='%d'>
345 <%s-ipv6 />
346 </rule>""" % (action, direction,

Subscribers

People subscribed via source and target branches