Making security group refresh more specific
Fixes bug 1029495 The trigger_members_refresh method in compute.api.py specifies a group id in the call to refresh_security_group_members. This is just the last group id seen and ignores the fact that a refresh may impact members of multiple groups. This is masked by the fact that on the host the group id is ignored and all instances have their security rules refreshed regardless of if they are part of the changed group or not. This change modifies the logic surrounding refreshes so we send a refresh request for each instance which is affected by a security group change, this ensures we aren't spending time refreshing unaffected instances and also removes the possibility of refreshing an instance multiple times if it is a member of more than one group. Also changed to be instance-centric is the refresh carried out when a rule is added/removed to a security group. Change-Id: Iec98e9aed818fdc4ecc88c8dcdd4ee5fa9386e00
This commit is contained in:
David McNally
parent
55cf5c3085
commit
2afbbab23a
|
|
@ -2164,20 +2164,16 @@ class SecurityGroupAPI(base.Base):
|
|||
|
||||
security_group = self.db.security_group_get(context, id)
|
||||
|
||||
hosts = set()
|
||||
for instance in security_group['instances']:
|
||||
if instance['host'] is not None:
|
||||
hosts.add(instance['host'])
|
||||
|
||||
for host in hosts:
|
||||
self.security_group_rpcapi.refresh_security_group_rules(context,
|
||||
security_group.id, host=host)
|
||||
self.security_group_rpcapi.refresh_instance_security_rules(
|
||||
context, instance['host'], instance)
|
||||
|
||||
def trigger_members_refresh(self, context, group_ids):
|
||||
"""Called when a security group gains a new or loses a member.
|
||||
|
||||
Sends an update request to each compute node for whom this is
|
||||
relevant.
|
||||
Sends an update request to each compute node for each instance for
|
||||
which this is relevant.
|
||||
"""
|
||||
# First, we get the security group rules that reference these groups as
|
||||
# the grantee..
|
||||
|
|
@ -2188,7 +2184,7 @@ class SecurityGroupAPI(base.Base):
|
|||
context,
|
||||
group_id))
|
||||
|
||||
# ..then we distill the security groups to which they belong..
|
||||
# ..then we distill the rules into the groups to which they belong..
|
||||
security_groups = set()
|
||||
for rule in security_group_rules:
|
||||
security_group = self.db.security_group_get(
|
||||
|
|
@ -2202,17 +2198,11 @@ class SecurityGroupAPI(base.Base):
|
|||
for instance in security_group['instances']:
|
||||
instances.add(instance)
|
||||
|
||||
# ...then we find the hosts where they live...
|
||||
hosts = set()
|
||||
# ..then we send a request to refresh the rules for each instance.
|
||||
for instance in instances:
|
||||
if instance['host']:
|
||||
hosts.add(instance['host'])
|
||||
|
||||
# ...and finally we tell these nodes to refresh their view of this
|
||||
# particular security group.
|
||||
for host in hosts:
|
||||
self.security_group_rpcapi.refresh_security_group_members(context,
|
||||
group_id, host=host)
|
||||
self.security_group_rpcapi.refresh_instance_security_rules(
|
||||
context, instance['host'], instance)
|
||||
|
||||
def parse_cidr(self, cidr):
|
||||
if cidr:
|
||||
|
|
|
|||
|
|
@ -221,7 +221,7 @@ def _get_image_meta(context, image_ref):
|
|||
class ComputeManager(manager.SchedulerDependentManager):
|
||||
"""Manages the running instances from creation to destruction."""
|
||||
|
||||
RPC_API_VERSION = '1.40'
|
||||
RPC_API_VERSION = '1.41'
|
||||
|
||||
def __init__(self, compute_driver=None, *args, **kwargs):
|
||||
"""Load configuration options and connect to the hypervisor."""
|
||||
|
|
@ -355,6 +355,16 @@ class ComputeManager(manager.SchedulerDependentManager):
|
|||
"""
|
||||
return self.driver.refresh_security_group_members(security_group_id)
|
||||
|
||||
@exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
|
||||
def refresh_instance_security_rules(self, context, instance):
|
||||
"""Tell the virtualization driver to refresh security rules for
|
||||
an instance.
|
||||
|
||||
Passes straight through to the virtualization driver.
|
||||
|
||||
"""
|
||||
return self.driver.refresh_instance_security_rules(instance)
|
||||
|
||||
@exception.wrap_exception(notifier=notifier, publisher_id=publisher_id())
|
||||
def refresh_provider_fw_rules(self, context, **kwargs):
|
||||
"""This call passes straight through to the virtualization driver."""
|
||||
|
|
|
|||
|
|
@ -119,6 +119,7 @@ class ComputeAPI(nova.openstack.common.rpc.proxy.RpcProxy):
|
|||
- remove topic, it was unused
|
||||
1.39 - Remove instance_uuid, add instance argument to run_instance()
|
||||
1.40 - Remove instance_id, add instance argument to live_migration()
|
||||
1.41 - Adds refresh_instance_security_rules()
|
||||
'''
|
||||
|
||||
BASE_RPC_API_VERSION = '1.0'
|
||||
|
|
@ -521,6 +522,7 @@ class SecurityGroupAPI(nova.openstack.common.rpc.proxy.RpcProxy):
|
|||
API version history:
|
||||
|
||||
1.0 - Initial version.
|
||||
1.41 - Adds refresh_instance_security_rules()
|
||||
'''
|
||||
|
||||
BASE_RPC_API_VERSION = '1.0'
|
||||
|
|
@ -540,3 +542,11 @@ class SecurityGroupAPI(nova.openstack.common.rpc.proxy.RpcProxy):
|
|||
self.cast(ctxt, self.make_msg('refresh_security_group_members',
|
||||
security_group_id=security_group_id),
|
||||
topic=_compute_topic(self.topic, ctxt, host, None))
|
||||
|
||||
def refresh_instance_security_rules(self, ctxt, host, instance):
|
||||
instance_p = jsonutils.to_primitive(instance)
|
||||
self.cast(ctxt, self.make_msg('refresh_instance_security_rules',
|
||||
instance=instance_p),
|
||||
topic=_compute_topic(self.topic, ctxt, instance['host'],
|
||||
instance),
|
||||
version='1.41')
|
||||
|
|
|
|||
|
|
@ -52,6 +52,7 @@ import nova.policy
|
|||
from nova import quota
|
||||
from nova.scheduler import driver as scheduler_driver
|
||||
from nova import test
|
||||
from nova.tests.db.fakes import FakeModel
|
||||
from nova.tests import fake_network
|
||||
from nova.tests.image import fake as fake_image
|
||||
from nova import utils
|
||||
|
|
@ -3918,6 +3919,134 @@ class ComputeAPITestCase(BaseTestCase):
|
|||
"/tmp/test", "File Contents")
|
||||
db.instance_destroy(self.context, instance['uuid'])
|
||||
|
||||
def test_secgroup_refresh(self):
|
||||
instance = self._create_fake_instance()
|
||||
|
||||
def rule_get(*args, **kwargs):
|
||||
mock_rule = FakeModel({'parent_group_id': 1})
|
||||
return [mock_rule]
|
||||
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': [instance]})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(
|
||||
self.compute_api.db,
|
||||
'security_group_rule_get_by_security_group_grantee',
|
||||
rule_get)
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
topic = rpc.queue_get_for(self.context, FLAGS.compute_topic,
|
||||
instance['host'])
|
||||
rpc.cast(self.context, topic,
|
||||
{"method": "refresh_instance_security_rules",
|
||||
"args": {'instance': jsonutils.to_primitive(instance)},
|
||||
"version": '1.41'})
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_members_refresh(self.context, [1])
|
||||
|
||||
def test_secgroup_refresh_once(self):
|
||||
instance = self._create_fake_instance()
|
||||
|
||||
def rule_get(*args, **kwargs):
|
||||
mock_rule = FakeModel({'parent_group_id': 1})
|
||||
return [mock_rule]
|
||||
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': [instance]})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(
|
||||
self.compute_api.db,
|
||||
'security_group_rule_get_by_security_group_grantee',
|
||||
rule_get)
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
topic = rpc.queue_get_for(self.context, FLAGS.compute_topic,
|
||||
instance['host'])
|
||||
rpc.cast(self.context, topic,
|
||||
{"method": "refresh_instance_security_rules",
|
||||
"args": {'instance': jsonutils.to_primitive(instance)},
|
||||
"version": '1.41'})
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_members_refresh(self.context, [1, 2])
|
||||
|
||||
def test_secgroup_refresh_none(self):
|
||||
def rule_get(*args, **kwargs):
|
||||
mock_rule = FakeModel({'parent_group_id': 1})
|
||||
return [mock_rule]
|
||||
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': []})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(
|
||||
self.compute_api.db,
|
||||
'security_group_rule_get_by_security_group_grantee',
|
||||
rule_get)
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_members_refresh(self.context, [1])
|
||||
|
||||
def test_secrule_refresh(self):
|
||||
instance = self._create_fake_instance()
|
||||
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': [instance]})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
topic = rpc.queue_get_for(self.context, FLAGS.compute_topic,
|
||||
instance['host'])
|
||||
rpc.cast(self.context, topic,
|
||||
{"method": "refresh_instance_security_rules",
|
||||
"args": {'instance': jsonutils.to_primitive(instance)},
|
||||
"version": '1.41'})
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_rules_refresh(self.context, [1])
|
||||
|
||||
def test_secrule_refresh_once(self):
|
||||
instance = self._create_fake_instance()
|
||||
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': [instance]})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
topic = rpc.queue_get_for(self.context, FLAGS.compute_topic,
|
||||
instance['host'])
|
||||
rpc.cast(self.context, topic,
|
||||
{"method": "refresh_instance_security_rules",
|
||||
"args": {'instance': jsonutils.to_primitive(instance)},
|
||||
"version": '1.41'})
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_rules_refresh(self.context, [1, 2])
|
||||
|
||||
def test_secrule_refresh_none(self):
|
||||
def group_get(*args, **kwargs):
|
||||
mock_group = FakeModel({'instances': []})
|
||||
return mock_group
|
||||
|
||||
self.stubs.Set(self.compute_api.db, 'security_group_get', group_get)
|
||||
|
||||
self.mox.StubOutWithMock(rpc, 'cast')
|
||||
self.mox.ReplayAll()
|
||||
|
||||
self.security_group_api.trigger_rules_refresh(self.context, [1, 2])
|
||||
|
||||
|
||||
def fake_rpc_method(context, topic, msg, do_cast=True):
|
||||
pass
|
||||
|
|
|
|||
|
|
@ -655,6 +655,10 @@ class BareMetalDriver(driver.ComputeDriver):
|
|||
# Bare metal doesn't currently support security groups
|
||||
pass
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
# Bare metal doesn't currently support security groups
|
||||
pass
|
||||
|
||||
def update_available_resource(self, ctxt, host):
|
||||
"""Updates compute manager resource info on ComputeNode table.
|
||||
|
||||
|
|
|
|||
|
|
@ -213,6 +213,9 @@ class FakeDriver(driver.ComputeDriver):
|
|||
def refresh_security_group_members(self, security_group_id):
|
||||
return True
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
return True
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
pass
|
||||
|
||||
|
|
|
|||
|
|
@ -75,6 +75,14 @@ class FirewallDriver(object):
|
|||
the security group."""
|
||||
raise NotImplementedError()
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
"""Refresh security group rules from data store
|
||||
|
||||
Gets called when an instance gets added to or removed from
|
||||
the security group the instance is a member of or if the
|
||||
group gains or looses a rule."""
|
||||
raise NotImplementedError()
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
"""Refresh common rules for all hosts/instances from data store.
|
||||
|
||||
|
|
@ -391,12 +399,21 @@ class IptablesFirewallDriver(FirewallDriver):
|
|||
self.do_refresh_security_group_rules(security_group)
|
||||
self.iptables.apply()
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
self.do_refresh_instance_rules(instance)
|
||||
self.iptables.apply()
|
||||
|
||||
@utils.synchronized('iptables', external=True)
|
||||
def do_refresh_security_group_rules(self, security_group):
|
||||
for instance in self.instances.values():
|
||||
self.remove_filters_for_instance(instance)
|
||||
self.add_filters_for_instance(instance)
|
||||
|
||||
@utils.synchronized('iptables', external=True)
|
||||
def do_refresh_instance_rules(self, instance):
|
||||
self.remove_filters_for_instance(instance)
|
||||
self.add_filters_for_instance(instance)
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
"""See :class:`FirewallDriver` docs."""
|
||||
self._do_refresh_provider_fw_rules()
|
||||
|
|
|
|||
|
|
@ -2173,6 +2173,9 @@ class LibvirtDriver(driver.ComputeDriver):
|
|||
def refresh_security_group_members(self, security_group_id):
|
||||
self.firewall_driver.refresh_security_group_members(security_group_id)
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
self.firewall_driver.refresh_instance_security_rules(instance)
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
self.firewall_driver.refresh_provider_fw_rules()
|
||||
|
||||
|
|
|
|||
|
|
@ -516,6 +516,13 @@ class XenAPIDriver(driver.ComputeDriver):
|
|||
"""
|
||||
return self._vmops.refresh_security_group_members(security_group_id)
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
""" Updates security group rules for specified instance
|
||||
Invoked when instances are added/removed to a security group
|
||||
or when a rule is added/removed to a security group
|
||||
"""
|
||||
return self._vmops.refresh_instance_security_rules(instance)
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
return self._vmops.refresh_provider_fw_rules()
|
||||
|
||||
|
|
|
|||
|
|
@ -1456,6 +1456,10 @@ class VMOps(object):
|
|||
""" recreates security group rules for every instance """
|
||||
self.firewall_driver.refresh_security_group_members(security_group_id)
|
||||
|
||||
def refresh_instance_security_rules(self, instance):
|
||||
""" recreates security group rules for specified instance """
|
||||
self.firewall_driver.refresh_instance_security_rules(instance)
|
||||
|
||||
def refresh_provider_fw_rules(self):
|
||||
self.firewall_driver.refresh_provider_fw_rules()
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue