x/quark
Code Issues Proposed changes

RM8802

Browse Source 
Removes the deprecated form of security group creation in the driver
logic. At the time of implementation, quark only supported one type of
network at a time, so we could make assumptions about what our security
groups were going to look like for a given implementation. However,
since then we added multiple-driver support, meaning the old assumptions
were now invalid.

Confining these changes to their own patch will reduce the overall
complexity of the final security groups implementation. While this
removes functionality, said functionality was already broken, so this
isn't strictly "more" broken.

Ensures basic functionality still exists with creation and deletion of
groups and rules.
This commit is contained in:
Matt Dietz
2014-09-03 20:07:41 +00:00
parent 9e11f12bdf
commit 4fc87b03d6
3 changed files with 51 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
quark/plugin.py
View File

@@ -122,35 +122,25 @@ class Plugin(neutron_plugin_base_v2.NeutronPluginBaseV2,
def delete_mac_address_range(self, context, id): def delete_mac_address_range(self, context, id):
mac_address_ranges.delete_mac_address_range(context, id) mac_address_ranges.delete_mac_address_range(context, id)
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine #2487
@sessioned @sessioned
def create_security_group(self, context, security_group, net_driver): def create_security_group(self, context, security_group):
self._fix_missing_tenant_id(context, security_group["security_group"]) self._fix_missing_tenant_id(context, security_group["security_group"])
return security_groups.create_security_group(context, security_group, return security_groups.create_security_group(context, security_group)
net_driver)
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine #2487
@sessioned @sessioned
def create_security_group_rule(self, context, security_group, def create_security_group_rule(self, context, security_group_rule):
security_group_rule, net_driver): self._fix_missing_tenant_id(context,
self._fix_missing_tenant_id(context, security_group["security_group"]) security_group_rule["security_group_rule"])
return security_groups.create_security_group_rule(context, return security_groups.create_security_group_rule(context,
security_group_rule, security_group_rule)
net_driver)
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine #2487
@sessioned @sessioned
def delete_security_group(self, context, id, net_driver): def delete_security_group(self, context, id):
security_groups.delete_security_group(context, id, net_driver) security_groups.delete_security_group(context, id)
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine #2487
@sessioned @sessioned
def delete_security_group_rule(self, context, id, net_driver): def delete_security_group_rule(self, context, id):
security_groups.delete_security_group_rule(context, id, net_driver) security_groups.delete_security_group_rule(context, id)
@sessioned @sessioned
def get_security_group(self, context, id, fields=None): def get_security_group(self, context, id, fields=None):
@@ -176,13 +166,10 @@ class Plugin(neutron_plugin_base_v2.NeutronPluginBaseV2,
fields, sorts, limit, fields, sorts, limit,
marker, page_reverse) marker, page_reverse)
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine #2487
@sessioned @sessioned
def update_security_group(self, context, id, security_group, net_driver): def update_security_group(self, context, id, security_group):
return security_groups.update_security_group(context, id, return security_groups.update_security_group(context, id,
security_group, security_group)
net_driver)
@sessioned @sessioned
def create_ip_policy(self, context, ip_policy): def create_ip_policy(self, context, ip_policy):

48
quark/plugin_modules/security_groups.py
View File

@@ -41,12 +41,13 @@ def _validate_security_group_rule(context, rule):
port_range_max = rule['port_range_max'] port_range_max = rule['port_range_max']
if protocol: if protocol:
if isinstance(protocol, str): proto = str(protocol).lower()
protocol = protocol.lower() if proto in PROTOCOLS:
protocol = PROTOCOLS.get(protocol) protocol = PROTOCOLS.get(proto)
if not protocol: if not protocol or not (protocol and isinstance(protocol, int)):
raise sg_ext.SecurityGroupRuleInvalidProtocol() raise sg_ext.SecurityGroupRuleInvalidProtocol(
protocol=protocol, values=PROTOCOLS.keys())
if protocol in ALLOWED_WITH_RANGE: if protocol in ALLOWED_WITH_RANGE:
if (port_range_min is None) != (port_range_max is None): if (port_range_min is None) != (port_range_max is None):
@@ -65,9 +66,7 @@ def _validate_security_group_rule(context, rule):
return rule return rule
def create_security_group(context, security_group, net_driver): def create_security_group(context, security_group):
# TODO(dietz/perkins): passing in net_driver as a stopgap,
# XXX DO NOT DEPLOY!! XXX see redmine # 2487
LOG.info("create_security_group for tenant %s" % LOG.info("create_security_group for tenant %s" %
(context.tenant_id)) (context.tenant_id))
group = security_group["security_group"] group = security_group["security_group"]
@@ -77,12 +76,6 @@ def create_security_group(context, security_group, net_driver):
group_id = uuidutils.generate_uuid() group_id = uuidutils.generate_uuid()
with context.session.begin(): with context.session.begin():
net_driver.create_security_group(
context,
group_name,
group_id=group_id,
**group)
group["id"] = group_id group["id"] = group_id
group["name"] = group_name group["name"] = group_name
group["tenant_id"] = context.tenant_id group["tenant_id"] = context.tenant_id
@@ -90,7 +83,7 @@ def create_security_group(context, security_group, net_driver):
return v._make_security_group_dict(dbgroup) return v._make_security_group_dict(dbgroup)
def _create_default_security_group(context, net_driver): def _create_default_security_group(context):
default_group = { default_group = {
"name": "default", "description": "", "name": "default", "description": "",
"group_id": DEFAULT_SG_UUID, "group_id": DEFAULT_SG_UUID,
@@ -104,11 +97,6 @@ def _create_default_security_group(context, net_driver):
{"ethertype": "IPv6", "protocol": 17}, {"ethertype": "IPv6", "protocol": 17},
]} ]}
net_driver.create_security_group(
context,
"default",
**default_group)
default_group["id"] = DEFAULT_SG_UUID default_group["id"] = DEFAULT_SG_UUID
default_group["tenant_id"] = context.tenant_id default_group["tenant_id"] = context.tenant_id
for rule in default_group.pop("port_ingress_rules"): for rule in default_group.pop("port_ingress_rules"):
@@ -119,7 +107,7 @@ def _create_default_security_group(context, net_driver):
db_api.security_group_create(context, **default_group) db_api.security_group_create(context, **default_group)
def create_security_group_rule(context, security_group_rule, net_driver): def create_security_group_rule(context, security_group_rule):
LOG.info("create_security_group for tenant %s" % LOG.info("create_security_group for tenant %s" %
(context.tenant_id)) (context.tenant_id))
@@ -138,13 +126,11 @@ def create_security_group_rule(context, security_group_rule, net_driver):
context, context.tenant_id, context, context.tenant_id,
security_rules_per_group=len(group.get("rules", [])) + 1) security_rules_per_group=len(group.get("rules", [])) + 1)
net_driver.create_security_group_rule(context, group_id, rule) new_rule = db_api.security_group_rule_create(context, **rule)
return v._make_security_group_rule_dict(new_rule)
return v._make_security_group_rule_dict(
db_api.security_group_rule_create(context, **rule))
def delete_security_group(context, id, net_driver): def delete_security_group(context, id):
LOG.info("delete_security_group %s for tenant %s" % LOG.info("delete_security_group %s for tenant %s" %
(id, context.tenant_id)) (id, context.tenant_id))
@@ -158,11 +144,10 @@ def delete_security_group(context, id, net_driver):
raise sg_ext.SecurityGroupCannotRemoveDefault() raise sg_ext.SecurityGroupCannotRemoveDefault()
if group.ports: if group.ports:
raise sg_ext.SecurityGroupInUse(id=id) raise sg_ext.SecurityGroupInUse(id=id)
net_driver.delete_security_group(context, id)
db_api.security_group_delete(context, group) db_api.security_group_delete(context, group)
def delete_security_group_rule(context, id, net_driver): def delete_security_group_rule(context, id):
LOG.info("delete_security_group %s for tenant %s" % LOG.info("delete_security_group %s for tenant %s" %
(id, context.tenant_id)) (id, context.tenant_id))
with context.session.begin(): with context.session.begin():
@@ -176,9 +161,6 @@ def delete_security_group_rule(context, id, net_driver):
if not group: if not group:
raise sg_ext.SecurityGroupNotFound(id=id) raise sg_ext.SecurityGroupNotFound(id=id)
net_driver.delete_security_group_rule(
context, group.id, v._make_security_group_rule_dict(rule))
rule["id"] = id rule["id"] = id
db_api.security_group_rule_delete(context, rule) db_api.security_group_rule_delete(context, rule)
@@ -220,13 +202,11 @@ def get_security_group_rules(context, filters=None, fields=None,
return [v._make_security_group_rule_dict(rule) for rule in rules] return [v._make_security_group_rule_dict(rule) for rule in rules]
def update_security_group(context, id, security_group, net_driver): def update_security_group(context, id, security_group):
if id == DEFAULT_SG_UUID: if id == DEFAULT_SG_UUID:
raise sg_ext.SecurityGroupCannotUpdateDefault() raise sg_ext.SecurityGroupCannotUpdateDefault()
new_group = security_group["security_group"] new_group = security_group["security_group"]
with context.session.begin(): with context.session.begin():
group = db_api.security_group_find(context, id=id, scope=db_api.ONE) group = db_api.security_group_find(context, id=id, scope=db_api.ONE)
net_driver.update_security_group(context, id, **new_group)
db_group = db_api.security_group_update(context, group, **new_group) db_group = db_api.security_group_update(context, group, **new_group)
return v._make_security_group_dict(db_group) return v._make_security_group_dict(db_group)

89
quark/tests/plugin_modules/test_security_groups.py
View File

@@ -21,7 +21,6 @@ from neutron.extensions import securitygroup as sg_ext
from oslo.config import cfg from oslo.config import cfg
from quark.db import models from quark.db import models
import quark.drivers.base
from quark.plugin_modules import security_groups from quark.plugin_modules import security_groups
from quark.tests import test_quark_plugin from quark.tests import test_quark_plugin
@@ -127,10 +126,6 @@ class TestQuarkGetSecurityGroupRules(test_quark_plugin.TestQuarkPlugin):
class TestQuarkUpdateSecurityGroup(test_quark_plugin.TestQuarkPlugin): class TestQuarkUpdateSecurityGroup(test_quark_plugin.TestQuarkPlugin):
def setUp(self):
super(TestQuarkUpdateSecurityGroup, self).setUp()
self.net_driver = quark.drivers.base.BaseDriver()
def test_update_security_group(self): def test_update_security_group(self):
rule = models.SecurityGroupRule() rule = models.SecurityGroupRule()
rule.update(dict(id=1)) rule.update(dict(id=1))
@@ -142,29 +137,24 @@ class TestQuarkUpdateSecurityGroup(test_quark_plugin.TestQuarkPlugin):
with contextlib.nested( with contextlib.nested(
mock.patch("quark.db.api.security_group_find"), mock.patch("quark.db.api.security_group_find"),
mock.patch("quark.db.api.security_group_update"), mock.patch("quark.db.api.security_group_update"),
mock.patch( ) as (db_find, db_update):
"quark.drivers.base.BaseDriver.update_security_group")
) as (db_find, db_update, update_sg):
db_find.return_value = group db_find.return_value = group
db_update.return_value = updated_group db_update.return_value = updated_group
update = dict(security_group=dict(name="bar")) update = dict(security_group=dict(name="bar"))
resp = self.plugin.update_security_group(self.context, 1, update, resp = self.plugin.update_security_group(self.context, 1, update)
self.net_driver)
self.assertEqual(resp["name"], updated_group["name"]) self.assertEqual(resp["name"], updated_group["name"])
def test_update_security_group_with_deault_security_group_id(self): def test_update_security_group_with_deault_security_group_id(self):
with self.assertRaises(sg_ext.SecurityGroupCannotUpdateDefault): with self.assertRaises(sg_ext.SecurityGroupCannotUpdateDefault):
self.plugin.update_security_group(self.context, self.plugin.update_security_group(self.context,
security_groups.DEFAULT_SG_UUID, security_groups.DEFAULT_SG_UUID,
None, None)
self.net_driver)
class TestQuarkCreateSecurityGroup(test_quark_plugin.TestQuarkPlugin): class TestQuarkCreateSecurityGroup(test_quark_plugin.TestQuarkPlugin):
def setUp(self, *args, **kwargs): def setUp(self, *args, **kwargs):
super(TestQuarkCreateSecurityGroup, self).setUp(*args, **kwargs) super(TestQuarkCreateSecurityGroup, self).setUp(*args, **kwargs)
cfg.CONF.set_override('quota_security_group', 1, 'QUOTAS') cfg.CONF.set_override('quota_security_group', 1, 'QUOTAS')
self.net_driver = quark.drivers.base.BaseDriver()
@contextlib.contextmanager @contextlib.contextmanager
def _stubs(self, security_group, other=0): def _stubs(self, security_group, other=0):
@@ -187,7 +177,7 @@ class TestQuarkCreateSecurityGroup(test_quark_plugin.TestQuarkPlugin):
'security_group_rules': []} 'security_group_rules': []}
with self._stubs(group) as group_create: with self._stubs(group) as group_create:
result = self.plugin.create_security_group( result = self.plugin.create_security_group(
self.context, {'security_group': group}, self.net_driver) self.context, {'security_group': group})
self.assertTrue(group_create.called) self.assertTrue(group_create.called)
for key in expected.keys(): for key in expected.keys():
self.assertEqual(result[key], expected[key]) self.assertEqual(result[key], expected[key])
@@ -198,15 +188,13 @@ class TestQuarkCreateSecurityGroup(test_quark_plugin.TestQuarkPlugin):
with self._stubs(group) as group_create: with self._stubs(group) as group_create:
with self.assertRaises(sg_ext.SecurityGroupDefaultAlreadyExists): with self.assertRaises(sg_ext.SecurityGroupDefaultAlreadyExists):
self.plugin.create_security_group( self.plugin.create_security_group(
self.context, {'security_group': group}, self.context, {'security_group': group})
self.net_driver)
self.assertTrue(group_create.called) self.assertTrue(group_create.called)
class TestQuarkDeleteSecurityGroup(test_quark_plugin.TestQuarkPlugin): class TestQuarkDeleteSecurityGroup(test_quark_plugin.TestQuarkPlugin):
@contextlib.contextmanager @contextlib.contextmanager
def _stubs(self, security_group=None): def _stubs(self, security_group=None):
self.net_driver = quark.drivers.base.BaseDriver()
dbgroup = None dbgroup = None
if security_group: if security_group:
dbgroup = models.SecurityGroup() dbgroup = models.SecurityGroup()
@@ -215,43 +203,37 @@ class TestQuarkDeleteSecurityGroup(test_quark_plugin.TestQuarkPlugin):
with contextlib.nested( with contextlib.nested(
mock.patch("quark.db.api.security_group_find"), mock.patch("quark.db.api.security_group_find"),
mock.patch("quark.db.api.security_group_delete"), mock.patch("quark.db.api.security_group_delete"),
mock.patch( ) as (group_find, db_group_delete):
"quark.drivers.base.BaseDriver.delete_security_group")
) as (group_find, db_group_delete, driver_group_delete):
group_find.return_value = dbgroup group_find.return_value = dbgroup
db_group_delete.return_value = dbgroup db_group_delete.return_value = dbgroup
yield db_group_delete, driver_group_delete yield db_group_delete
def test_delete_security_group(self): def test_delete_security_group(self):
group = {'name': 'foo', 'description': 'bar', 'id': 1, group = {'name': 'foo', 'description': 'bar', 'id': 1,
'tenant_id': self.context.tenant_id} 'tenant_id': self.context.tenant_id}
with self._stubs(group) as (db_delete, driver_delete): with self._stubs(group) as (db_delete):
self.plugin.delete_security_group(self.context, 1, self.net_driver) self.plugin.delete_security_group(self.context, 1)
self.assertTrue(db_delete.called) self.assertTrue(db_delete.called)
driver_delete.assert_called_once_with(self.context, 1)
def test_delete_default_security_group(self): def test_delete_default_security_group(self):
group = {'name': 'default', 'id': 1, group = {'name': 'default', 'id': 1,
'tenant_id': self.context.tenant_id} 'tenant_id': self.context.tenant_id}
with self._stubs(group) as (db_delete, driver_delete): with self._stubs(group):
with self.assertRaises(sg_ext.SecurityGroupCannotRemoveDefault): with self.assertRaises(sg_ext.SecurityGroupCannotRemoveDefault):
self.plugin.delete_security_group(self.context, 1, self.plugin.delete_security_group(self.context, 1)
self.net_driver)
def test_delete_security_group_with_ports(self): def test_delete_security_group_with_ports(self):
port = models.Port() port = models.Port()
group = {'name': 'foo', 'description': 'bar', 'id': 1, group = {'name': 'foo', 'description': 'bar', 'id': 1,
'tenant_id': self.context.tenant_id, 'ports': [port]} 'tenant_id': self.context.tenant_id, 'ports': [port]}
with self._stubs(group) as (db_delete, driver_delete): with self._stubs(group):
with self.assertRaises(sg_ext.SecurityGroupInUse): with self.assertRaises(sg_ext.SecurityGroupInUse):
self.plugin.delete_security_group(self.context, 1, self.plugin.delete_security_group(self.context, 1)
self.net_driver)
def test_delete_security_group_not_found(self): def test_delete_security_group_not_found(self):
with self._stubs() as (db_delete, driver_delete): with self._stubs():
with self.assertRaises(sg_ext.SecurityGroupNotFound): with self.assertRaises(sg_ext.SecurityGroupNotFound):
self.plugin.delete_security_group(self.context, 1, self.plugin.delete_security_group(self.context, 1)
self.net_driver)
class TestQuarkCreateSecurityGroupRule(test_quark_plugin.TestQuarkPlugin): class TestQuarkCreateSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
@@ -274,7 +256,6 @@ class TestQuarkCreateSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
'tenant_id': None, 'tenant_id': None,
'protocol': None, 'protocol': None,
'security_group_id': 1} 'security_group_id': 1}
self.net_driver = quark.drivers.base.BaseDriver()
@contextlib.contextmanager @contextlib.contextmanager
def _stubs(self, rule, group): def _stubs(self, rule, group):
@@ -301,18 +282,11 @@ class TestQuarkCreateSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
ruleset['tenant_id'] = self.context.tenant_id ruleset['tenant_id'] = self.context.tenant_id
rule = dict(self.rule, **ruleset) rule = dict(self.rule, **ruleset)
group = rule.pop('group') group = rule.pop('group')
if group:
sec_group = group['id']
else:
sec_group = None
expected = dict(self.expected, **ruleset) expected = dict(self.expected, **ruleset)
expected.pop('group', None) expected.pop('group', None)
hax1 = {'security_group': sec_group} hax = {'security_group_rule': rule}
hax2 = {'security_group_rule': rule}
with self._stubs(rule, group) as rule_create: with self._stubs(rule, group) as rule_create:
result = self.plugin.create_security_group_rule(self.context, result = self.plugin.create_security_group_rule(self.context, hax)
hax1, hax2,
self.net_driver)
self.assertTrue(rule_create.called) self.assertTrue(rule_create.called)
for key in expected.keys(): for key in expected.keys():
self.assertEqual(expected[key], result[key]) self.assertEqual(expected[key], result[key])
@@ -373,7 +347,6 @@ class TestQuarkCreateSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
class TestQuarkDeleteSecurityGroupRule(test_quark_plugin.TestQuarkPlugin): class TestQuarkDeleteSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
@contextlib.contextmanager @contextlib.contextmanager
def _stubs(self, rule={}, group={'id': 1}): def _stubs(self, rule={}, group={'id': 1}):
self.net_driver = quark.drivers.base.BaseDriver()
dbrule = None dbrule = None
dbgroup = None dbgroup = None
if group: if group:
@@ -387,40 +360,28 @@ class TestQuarkDeleteSecurityGroupRule(test_quark_plugin.TestQuarkPlugin):
mock.patch("quark.db.api.security_group_find"), mock.patch("quark.db.api.security_group_find"),
mock.patch("quark.db.api.security_group_rule_find"), mock.patch("quark.db.api.security_group_rule_find"),
mock.patch("quark.db.api.security_group_rule_delete"), mock.patch("quark.db.api.security_group_rule_delete"),
mock.patch( ) as (group_find, rule_find, db_group_delete):
"quark.drivers.base.BaseDriver.delete_security_group_rule")
) as (group_find, rule_find, db_group_delete, driver_group_delete):
group_find.return_value = dbgroup group_find.return_value = dbgroup
rule_find.return_value = dbrule rule_find.return_value = dbrule
yield db_group_delete, driver_group_delete yield db_group_delete
def test_delete_security_group_rule(self): def test_delete_security_group_rule(self):
rule = {'id': 1, 'security_group_id': 1, 'ethertype': 'IPv4', rule = {'id': 1, 'security_group_id': 1, 'ethertype': 'IPv4',
'protocol': 6, 'port_range_min': 0, 'port_range_max': 10, 'protocol': 6, 'port_range_min': 0, 'port_range_max': 10,
'direction': 'ingress', 'tenant_id': self.context.tenant_id} 'direction': 'ingress', 'tenant_id': self.context.tenant_id}
expected = {
'id': 1, 'ethertype': 'IPv4', 'security_group_id': 1,
'direction': 'ingress', 'port_range_min': 0, 'port_range_max': 10,
'remote_group_id': None, 'remote_ip_prefix': None,
'tenant_id': self.context.tenant_id, 'protocol': 6}
with self._stubs(dict(rule, group_id=1)) as (db_delete, driver_delete): with self._stubs(dict(rule, group_id=1)) as (db_delete):
self.plugin.delete_security_group_rule(self.context, 1, self.plugin.delete_security_group_rule(self.context, 1)
self.net_driver)
self.assertTrue(db_delete.called) self.assertTrue(db_delete.called)
driver_delete.assert_called_once_with(self.context, 1,
expected)
def test_delete_security_group_rule_rule_not_found(self): def test_delete_security_group_rule_rule_not_found(self):
with self._stubs() as (db_delete, driver_delete): with self._stubs():
with self.assertRaises(sg_ext.SecurityGroupRuleNotFound): with self.assertRaises(sg_ext.SecurityGroupRuleNotFound):
self.plugin.delete_security_group_rule(self.context, 1, self.plugin.delete_security_group_rule(self.context, 1)
self.net_driver)
def test_delete_security_group_rule_group_not_found(self): def test_delete_security_group_rule_group_not_found(self):
rule = {'id': 1, 'security_group_id': 1, 'ethertype': 'IPv4'} rule = {'id': 1, 'security_group_id': 1, 'ethertype': 'IPv4'}
with self._stubs(dict(rule, group_id=1), with self._stubs(dict(rule, group_id=1),
None) as (db_delete, driver_delete): None):
with self.assertRaises(sg_ext.SecurityGroupNotFound): with self.assertRaises(sg_ext.SecurityGroupNotFound):
self.plugin.delete_security_group_rule(self.context, 1, self.plugin.delete_security_group_rule(self.context, 1)
self.net_driver)