From 2c96a485b5bd9ffa5617b181e0daf21d94adf443 Mon Sep 17 00:00:00 2001 From: Ivar Lazzaro Date: Wed, 18 May 2016 16:07:31 -0700 Subject: [PATCH] ptg attribute for sc enforcement Add enforce_service_chains attribute to PTGs as part of the proxy-group driver extension. When set to False, PTGs won't trigger service chain creation even when providing a PRS with a redirect rule. Change-Id: I78fb098ec4092f2c2b43f0eb41f35ab2fd5e01d9 --- etc/policy.json | 1 + .../grouppolicy/extensions/group_proxy_db.py | 1 + .../versions/7afacef00d31_enforce_chains.py | 38 ++++++++++++++++ .../alembic_migrations/versions/HEAD | 2 +- .../neutron/extensions/driver_proxy_group.py | 5 +++ .../grouppolicy/drivers/chain_mapping.py | 32 ++++++++++---- .../drivers/extensions/proxy_group_driver.py | 6 +++ gbpservice/neutron/tests/etc/test-policy.json | 1 + .../services/grouppolicy/test_apic_mapping.py | 14 ------ .../grouppolicy/test_group_proxy_extension.py | 15 +++++++ .../grouppolicy/test_resource_mapping.py | 43 +++++++++++++++++++ 11 files changed, 134 insertions(+), 24 deletions(-) create mode 100644 gbpservice/neutron/db/migration/alembic_migrations/versions/7afacef00d31_enforce_chains.py diff --git a/etc/policy.json b/etc/policy.json index 9c51ce854..90e8a833d 100644 --- a/etc/policy.json +++ b/etc/policy.json @@ -215,6 +215,7 @@ "create_policy_target_group": "", "create_policy_target_group:shared": "rule:admin_only", "create_policy_target_group:service_management": "rule:admin_only", + "create_policy_target_group:enforce_service_chains": "rule:admin_only", "get_policy_target_group": "rule:admin_or_owner or rule:shared_ptg", "update_policy_target_group:shared": "rule:admin_only", diff --git a/gbpservice/neutron/db/grouppolicy/extensions/group_proxy_db.py b/gbpservice/neutron/db/grouppolicy/extensions/group_proxy_db.py index c96c6d49f..c5066c56c 100644 --- a/gbpservice/neutron/db/grouppolicy/extensions/group_proxy_db.py +++ b/gbpservice/neutron/db/grouppolicy/extensions/group_proxy_db.py @@ -28,6 +28,7 @@ class GroupProxyMapping(model_base.BASEV2): sa.ForeignKey('gp_policy_target_groups.id', ondelete="SET NULL")) proxy_type = sa.Column(sa.String(24)) + enforce_service_chains = sa.Column(sa.Boolean) class ProxyGatewayMapping(model_base.BASEV2): diff --git a/gbpservice/neutron/db/migration/alembic_migrations/versions/7afacef00d31_enforce_chains.py b/gbpservice/neutron/db/migration/alembic_migrations/versions/7afacef00d31_enforce_chains.py new file mode 100644 index 000000000..088390940 --- /dev/null +++ b/gbpservice/neutron/db/migration/alembic_migrations/versions/7afacef00d31_enforce_chains.py @@ -0,0 +1,38 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +# + +"""enforce_service_chains attribute for PTGs + +Revision ID: 64fa77aca090 + +""" + +# revision identifiers, used by Alembic. +revision = '7afacef00d31' +down_revision = 'c1aab79622fe' + + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + + op.add_column( + 'gp_group_proxy_mappings', + sa.Column('enforce_service_chains', sa.Boolean, default=True) + ) + + +def downgrade(): + pass diff --git a/gbpservice/neutron/db/migration/alembic_migrations/versions/HEAD b/gbpservice/neutron/db/migration/alembic_migrations/versions/HEAD index d0c9e6d26..f99718d03 100644 --- a/gbpservice/neutron/db/migration/alembic_migrations/versions/HEAD +++ b/gbpservice/neutron/db/migration/alembic_migrations/versions/HEAD @@ -1 +1 @@ -c1aab79622fe +7afacef00d31 \ No newline at end of file diff --git a/gbpservice/neutron/extensions/driver_proxy_group.py b/gbpservice/neutron/extensions/driver_proxy_group.py index 3df925133..218584ce9 100644 --- a/gbpservice/neutron/extensions/driver_proxy_group.py +++ b/gbpservice/neutron/extensions/driver_proxy_group.py @@ -74,6 +74,11 @@ EXTENDED_ATTRIBUTES_2_0 = { 'allow_post': False, 'allow_put': False, 'validate': {'type:uuid_or_none': None}, 'is_visible': True, 'enforce_policy': True}, + 'enforce_service_chains': { + 'allow_post': True, 'allow_put': False, 'default': True, + 'convert_to': attr.convert_to_boolean, + 'is_visible': True, 'required_by_policy': True, + 'enforce_policy': True}, # TODO(ivar): The APIs should allow the creation of a group with a # custom subnet prefix length. It may be useful for both the proxy # groups and traditional ones. diff --git a/gbpservice/neutron/services/grouppolicy/drivers/chain_mapping.py b/gbpservice/neutron/services/grouppolicy/drivers/chain_mapping.py index b0b8cf8cc..2082b0c20 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/chain_mapping.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/chain_mapping.py @@ -160,8 +160,8 @@ class ChainMappingDriver(api.PolicyDriver, local_api.LocalAPI, @log.log_method_call def create_policy_target_group_postcommit(self, context): - if (context.current['provided_policy_rule_sets'] and not - context.current.get('proxied_group_id')): + if (context.current['provided_policy_rule_sets'] and + self._is_group_chainable(context, context.current)): self._handle_redirect_action( context, context.current['provided_policy_rule_sets'], providing_ptg=context.current) @@ -186,9 +186,9 @@ class ChainMappingDriver(api.PolicyDriver, local_api.LocalAPI, self._cleanup_redirect_action(context) # If the spec is changed, then update the chain with new spec # If redirect is newly added, create the chain - if self._is_redirect_in_policy_rule_sets( - context, new_provided_policy_rule_sets) and not ( - context.current.get('proxied_group_id')): + if (self._is_redirect_in_policy_rule_sets( + context, new_provided_policy_rule_sets) and + self._is_group_chainable(context, context.current)): self._handle_redirect_action( context, curr['provided_policy_rule_sets'], providing_ptg=context.current) @@ -497,7 +497,7 @@ class ChainMappingDriver(api.PolicyDriver, local_api.LocalAPI, # REVISIT(Magesh): There are concurrency issues here with # concurrent updates to the same PRS, Policy Rule or Action # value - if not ptg_providing_prs.get('proxied_group_id'): + if self._is_group_chainable(context, ptg_providing_prs): self._create_or_update_chain( context, ptg_providing_prs['id'], SCI_CONSUMER_NOT_AVAILABLE, spec_id, @@ -749,9 +749,10 @@ class ChainMappingDriver(api.PolicyDriver, local_api.LocalAPI, def _validate_ptg_prss(self, context, ptg): # If the PTG is providing a redirect PRS, it can't provide any more # redirect rules - if self._prss_redirect_rules(context._plugin_context.session, - ptg['provided_policy_rule_sets']) > 1: - raise exc.PTGAlreadyProvidingRedirectPRS(ptg_id=ptg['id']) + if self._is_group_chainable(context, ptg): + if self._prss_redirect_rules(context._plugin_context.session, + ptg['provided_policy_rule_sets']) > 1: + raise exc.PTGAlreadyProvidingRedirectPRS(ptg_id=ptg['id']) def _handle_classifier_update_notification(self, context): # Invoke Service chain update notify hook if protocol or port or @@ -809,3 +810,16 @@ class ChainMappingDriver(api.PolicyDriver, local_api.LocalAPI, provider_ptg_ids=prs[ 'providing_policy_target_groups'])]) return result + + def _is_group_chainable(self, context, group): + """Determines whether a group should trigger a chain. + + Non chainable groups: + - Proxy groups; + + :param context: + :param group: + :return: boolean + """ + return (not group.get('proxied_group_id') and + group.get('enforce_service_chains', True) is True) diff --git a/gbpservice/neutron/services/grouppolicy/drivers/extensions/proxy_group_driver.py b/gbpservice/neutron/services/grouppolicy/drivers/extensions/proxy_group_driver.py index 172b5fcc5..2d5d06a4b 100644 --- a/gbpservice/neutron/services/grouppolicy/drivers/extensions/proxy_group_driver.py +++ b/gbpservice/neutron/services/grouppolicy/drivers/extensions/proxy_group_driver.py @@ -59,6 +59,12 @@ class ProxyGroupDriver(api.ExtensionDriver): policy_target_group_id=result['id']).one()) record.proxy_type = data['proxy_type'] result['proxy_type'] = data['proxy_type'] + # Proxy PTGs can't have chains enforced + data['enforce_service_chains'] = False + record = (session.query(db.GroupProxyMapping).filter_by( + policy_target_group_id=result['id']).one()) + record.enforce_service_chains = data['enforce_service_chains'] + result['enforce_service_chains'] = data['enforce_service_chains'] elif attributes.is_attr_set(data.get('proxy_type')): raise driver_proxy_group.ProxyTypeSetWithoutProxiedPTG() diff --git a/gbpservice/neutron/tests/etc/test-policy.json b/gbpservice/neutron/tests/etc/test-policy.json index 9219eebd9..15ac42a8e 100644 --- a/gbpservice/neutron/tests/etc/test-policy.json +++ b/gbpservice/neutron/tests/etc/test-policy.json @@ -22,6 +22,7 @@ "shared_sp": "field:service_profiles:shared=True", "create_policy_target_group": "", + "create_policy_target_group:enforce_service_chains": "rule:admin_only", "get_policy_target_group": "rule:admin_or_owner or rule:shared_ptg", "create_policy_target_group:service_management": "rule:admin_only", diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py index 92a994b93..b4b71a552 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_apic_mapping.py @@ -281,20 +281,6 @@ class ApicMappingTestCase( self.driver.apic_manager.ext_net_dict.update( self._build_external_dict(x[0], x[1], is_edge_nat=is_edge_nat)) - def _create_simple_policy_rule(self, direction='bi', protocol='tcp', - port_range=80, shared=False, - action_type='allow', action_value=None): - cls = self.create_policy_classifier( - direction=direction, protocol=protocol, - port_range=port_range, shared=shared)['policy_classifier'] - - action = self.create_policy_action( - action_type=action_type, shared=shared, - action_value=action_value)['policy_action'] - return self.create_policy_rule( - policy_classifier_id=cls['id'], policy_actions=[action['id']], - shared=shared)['policy_rule'] - def _bind_port_to_host(self, port_id, host): data = {'port': {'binding:host_id': host, 'device_owner': 'compute:', diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_group_proxy_extension.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_group_proxy_extension.py index 89beea6ea..d9ae772ff 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_group_proxy_extension.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_group_proxy_extension.py @@ -37,11 +37,26 @@ class ExtensionDriverTestCaseMixin(object): self.assertEqual('192.168.0.0/16', l3p['proxy_ip_pool']) self.assertEqual(28, l3p['proxy_subnet_prefix_length']) + # No proxy + ptg_proxy = self.create_policy_target_group()['policy_target_group'] + self.assertTrue(ptg_proxy['enforce_service_chains']) + ptg_proxy = self.show_policy_target_group( + ptg_proxy['id'])['policy_target_group'] + self.assertTrue(ptg_proxy['enforce_service_chains']) + ptg_proxy = self.create_policy_target_group( + enforce_service_chains=False, + is_admin_context=True)['policy_target_group'] + self.assertFalse(ptg_proxy['enforce_service_chains']) + ptg_proxy = self.show_policy_target_group( + ptg_proxy['id'])['policy_target_group'] + self.assertFalse(ptg_proxy['enforce_service_chains']) + ptg_proxy = self.create_policy_target_group( proxied_group_id=ptg['id'])['policy_target_group'] self.assertIsNone(ptg_proxy['proxy_group_id']) self.assertEqual(ptg['id'], ptg_proxy['proxied_group_id']) self.assertEqual('l3', ptg_proxy['proxy_type']) + self.assertFalse(ptg_proxy['enforce_service_chains']) # Verify relationship added ptg = self.show_policy_target_group(ptg['id'])['policy_target_group'] diff --git a/gbpservice/neutron/tests/unit/services/grouppolicy/test_resource_mapping.py b/gbpservice/neutron/tests/unit/services/grouppolicy/test_resource_mapping.py index ba5f832a2..a160b976f 100644 --- a/gbpservice/neutron/tests/unit/services/grouppolicy/test_resource_mapping.py +++ b/gbpservice/neutron/tests/unit/services/grouppolicy/test_resource_mapping.py @@ -448,6 +448,20 @@ class ResourceMappingTestCase(test_plugin.GroupPolicyPluginTestCase): scs_id = spec['servicechain_spec']['id'] return scs_id + def _create_simple_policy_rule(self, direction='bi', protocol='tcp', + port_range=80, shared=False, + action_type='allow', action_value=None): + cls = self.create_policy_classifier( + direction=direction, protocol=protocol, + port_range=port_range, shared=shared)['policy_classifier'] + + action = self.create_policy_action( + action_type=action_type, shared=shared, + action_value=action_value)['policy_action'] + return self.create_policy_rule( + policy_classifier_id=cls['id'], policy_actions=[action['id']], + shared=shared)['policy_rule'] + class TestClusterIdMixin(object): @@ -1381,6 +1395,35 @@ class TestPolicyTargetGroup(ResourceMappingTestCase): self._unbind_port(port['port']['id']) self.delete_policy_target_group(ptg['id'], expected_res_status=204) + def test_ptg_only_participate_one_prs_when_redirect(self): + redirect_rule = self._create_simple_policy_rule(action_type='redirect') + simple_rule = self._create_simple_policy_rule() + prs_r = self.create_policy_rule_set( + policy_rules=[redirect_rule['id']])['policy_rule_set'] + prs = self.create_policy_rule_set( + policy_rules=[simple_rule['id']])['policy_rule_set'] + + # Creating PTG with provided redirect and multiple PRS fails + self.create_policy_target_group( + provided_policy_rule_sets={prs_r['id']: '', prs['id']: ''}, + consumed_policy_rule_sets={prs['id']: ''}, + expected_res_status=201) + + action = self.create_policy_action( + action_type='redirect')['policy_action'] + res = self.update_policy_rule( + simple_rule['id'], policy_actions=[action['id']], + expected_res_status=400) + self.assertEqual('PTGAlreadyProvidingRedirectPRS', + res['NeutronError']['type']) + # Verify everythin is fine for non chainable PTGs + with mock.patch.object( + chain_mapping.ChainMappingDriver, '_is_group_chainable', + return_value=False): + self.update_policy_rule( + simple_rule['id'], policy_actions=[action['id']], + expected_res_status=200) + class TestL2Policy(ResourceMappingTestCase):