From a2e5daccb389619dbd2c7ed271fd1e4d84d71823 Mon Sep 17 00:00:00 2001 From: Ihar Hrachyshka Date: Wed, 5 May 2021 21:50:51 -0400 Subject: [PATCH] Add support for OVN allow-stateless ACLs Map OpenStack SG stateful=False to OVN ACL allow-stateless action verb. The verb is added in the latest OVN release, 21.06. Inspect db schema to determine if the new action is supported by OVN before trying to create it. Fall back to allow-related when it's not supported yet. Also-Needs: I7343fb609fab91c20490842378747f7265241e82 This will require ovsdbapp version bump with the patch mentioned above to make it work. Change-Id: Ic1c36fb71a9d03e8697583a1ea9453d4c0052f74 --- neutron/common/ovn/acl.py | 32 ++++++++++---- neutron/common/ovn/constants.py | 1 + .../ovn/mech_driver/ovsdb/impl_idl_ovn.py | 8 ++++ .../ovn/mech_driver/ovsdb/ovn_client.py | 14 +++++-- .../ovn/mech_driver/ovsdb/ovn_db_sync.py | 26 +++++++++--- neutron/services/logapi/drivers/ovn/driver.py | 2 + .../ovn/mech_driver/ovsdb/test_ovn_db_sync.py | 2 +- neutron/tests/unit/fake_resources.py | 2 + .../ovn/mech_driver/test_mech_driver.py | 42 +++++++++++++++---- ...support-stateless-sg-849170c5d320487e.yaml | 6 +++ 10 files changed, 109 insertions(+), 26 deletions(-) create mode 100644 releasenotes/notes/ovn-support-stateless-sg-849170c5d320487e.yaml diff --git a/neutron/common/ovn/acl.py b/neutron/common/ovn/acl.py index ce36866f23e..8a5cbb86db9 100644 --- a/neutron/common/ovn/acl.py +++ b/neutron/common/ovn/acl.py @@ -172,12 +172,16 @@ def drop_all_ip_traffic_for_port(port): return acl_list -def add_sg_rule_acl_for_port_group(port_group, r, match): +def add_sg_rule_acl_for_port_group(port_group, r, stateful, match): dir_map = {const.INGRESS_DIRECTION: 'to-lport', const.EGRESS_DIRECTION: 'from-lport'} + if stateful: + action = ovn_const.ACL_ACTION_ALLOW_RELATED + else: + action = ovn_const.ACL_ACTION_ALLOW_STATELESS acl = {"port_group": port_group, "priority": ovn_const.ACL_PRIORITY_ALLOW, - "action": ovn_const.ACL_ACTION_ALLOW_RELATED, + "action": action, "log": False, "name": [], "severity": [], @@ -266,7 +270,7 @@ def acl_remote_group_id(r, ip_version): return ' && %s.%s == $%s' % (ip_version, src_or_dst, addrset_name) -def _add_sg_rule_acl_for_port_group(port_group, r): +def _add_sg_rule_acl_for_port_group(port_group, stateful, r): # Update the match based on which direction this rule is for (ingress # or egress). match = acl_direction(r, port_group=port_group) @@ -286,7 +290,7 @@ def _add_sg_rule_acl_for_port_group(port_group, r): match += acl_protocol_and_ports(r, icmp) # Finally, create the ACL entry for the direction specified. - return add_sg_rule_acl_for_port_group(port_group, r, match) + return add_sg_rule_acl_for_port_group(port_group, r, stateful, match) def _acl_columns_name_severity_supported(nb_idl): @@ -294,10 +298,15 @@ def _acl_columns_name_severity_supported(nb_idl): return ('name' in columns) and ('severity' in columns) -def add_acls_for_sg_port_group(ovn, security_group, txn): +def add_acls_for_sg_port_group(ovn, security_group, txn, + stateless_supported=True): + if stateless_supported: + stateful = security_group.get("stateful", True) + else: + stateful = True for r in security_group['security_group_rules']: acl = _add_sg_rule_acl_for_port_group( - utils.ovn_port_group_name(security_group['id']), r) + utils.ovn_port_group_name(security_group['id']), stateful, r) txn.add(ovn.pg_acl_add(**acl, may_exist=True)) @@ -306,7 +315,8 @@ def update_acls_for_security_group(plugin, ovn, security_group_id, security_group_rule, - is_add_acl=True): + is_add_acl=True, + stateless_supported=True): # Skip ACLs if security groups aren't enabled if not is_sg_enabled(): @@ -315,9 +325,15 @@ def update_acls_for_security_group(plugin, # Check if ACL log name and severity supported or not keep_name_severity = _acl_columns_name_severity_supported(ovn) + if stateless_supported: + sg = plugin.get_security_group(admin_context, security_group_id) + stateful = sg.get("stateful", True) + else: + stateful = True + acl = _add_sg_rule_acl_for_port_group( utils.ovn_port_group_name(security_group_id), - security_group_rule) + stateful, security_group_rule) # Remove ACL log name and severity if not supported if is_add_acl: if not keep_name_severity: diff --git a/neutron/common/ovn/constants.py b/neutron/common/ovn/constants.py index 4ad1845eeb2..038f08bd93a 100644 --- a/neutron/common/ovn/constants.py +++ b/neutron/common/ovn/constants.py @@ -79,6 +79,7 @@ ACL_PRIORITY_DROP = 1001 ACL_ACTION_DROP = 'drop' ACL_ACTION_REJECT = 'reject' ACL_ACTION_ALLOW_RELATED = 'allow-related' +ACL_ACTION_ALLOW_STATELESS = 'allow-stateless' ACL_ACTION_ALLOW = 'allow' # When a OVN L3 gateway is created, it needs to be bound to a chassis. In diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py index 1f2f8166ec3..4a0fe709f80 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/impl_idl_ovn.py @@ -127,6 +127,14 @@ class Backend(ovs_idl.Backend): return self.is_table_present(table_name) and ( col_name in self._tables[table_name].columns) + def is_col_supports_value(self, table_name, col_name, value): + if not self.is_col_present(table_name, col_name): + return False + enum = self._tables[table_name].columns[col_name].type.key.enum + if not enum: + return False + return value in {k.value for k in enum.values} + def create_transaction(self, check_error=False, log_errors=True): return idl_trans.Transaction( self, self.ovsdb_connection, self.ovsdb_connection.timeout, diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py index 608d4b24021..c4a06da5d5f 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_client.py @@ -106,6 +106,11 @@ class OVNClient(object): return self._nb_idl.is_col_present( 'Logical_Switch_Port', 'ha_chassis_group') + # TODO(ihrachys) remove when min OVN version >= 21.06 + def is_allow_stateless_supported(self): + return self._nb_idl.is_col_supports_value('ACL', 'action', + 'allow-stateless') + def _get_allowed_addresses_from_port(self, port): if not port.get(psec.PORTSECURITY): return [], [] @@ -2001,8 +2006,9 @@ class OVNClient(object): name=name, acls=[], external_ids=ext_ids)) # When a SG is created, it comes with some default rules, # so we'll apply them to the Port Group. - ovn_acl.add_acls_for_sg_port_group(self._nb_idl, - security_group, txn) + ovn_acl.add_acls_for_sg_port_group( + self._nb_idl, security_group, txn, + self.is_allow_stateless_supported()) db_rev.bump_revision( context, security_group, ovn_const.TYPE_SECURITY_GROUPS) @@ -2026,7 +2032,9 @@ class OVNClient(object): admin_context = n_context.get_admin_context() ovn_acl.update_acls_for_security_group( self._plugin, admin_context, self._nb_idl, - rule['security_group_id'], rule, is_add_acl=is_add_acl) + rule['security_group_id'], rule, + is_add_acl=is_add_acl, + stateless_supported=self.is_allow_stateless_supported()) def create_security_group_rule(self, context, rule): self._process_security_group_rule(rule) diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py index 315b31bc08b..643870a939f 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py @@ -247,10 +247,24 @@ class OvnNbSynchronizer(OvnDbSynchronizer): LOG.debug('ACL-SYNC: started @ %s', str(datetime.now())) neutron_acls = [] - for sgr in self.core_plugin.get_security_group_rules(ctx): - pg_name = utils.ovn_port_group_name(sgr['security_group_id']) - neutron_acls.append(acl_utils._add_sg_rule_acl_for_port_group( - pg_name, sgr)) + # if allow-stateless supported, we have to fetch groups to determine if + # stateful is set + if self._ovn_client.is_allow_stateless_supported(): + for sg in self.core_plugin.get_security_groups(ctx): + stateful = sg.get("stateful", True) + pg_name = utils.ovn_port_group_name(sg['id']) + for sgr in self.core_plugin.get_security_group_rules( + ctx, {'security_group_id': sg['id']}): + neutron_acls.append( + acl_utils._add_sg_rule_acl_for_port_group( + pg_name, stateful, sgr) + ) + else: + # TODO(ihrachys) remove when min OVN version >= 21.06 + for sgr in self.core_plugin.get_security_group_rules(ctx): + pg_name = utils.ovn_port_group_name(sgr['security_group_id']) + neutron_acls.append(acl_utils._add_sg_rule_acl_for_port_group( + pg_name, True, sgr)) neutron_acls += acl_utils.add_acls_for_drop_port_group( ovn_const.OVN_DROP_PORT_GROUP_NAME) @@ -1166,7 +1180,9 @@ class OvnNbSynchronizer(OvnDbSynchronizer): ext_ids = {ovn_const.OVN_SG_EXT_ID_KEY: sg['id']} txn.add(self.ovn_api.pg_add( name=pg_name, acls=[], external_ids=ext_ids)) - acl_utils.add_acls_for_sg_port_group(self.ovn_api, sg, txn) + acl_utils.add_acls_for_sg_port_group( + self.ovn_api, sg, txn, + self._ovn_client.is_allow_stateless_supported()) for port in db_ports: for sg in port['security_groups']: txn.add(self.ovn_api.pg_add_ports( diff --git a/neutron/services/logapi/drivers/ovn/driver.py b/neutron/services/logapi/drivers/ovn/driver.py index 1a5ce941964..7f97111b289 100644 --- a/neutron/services/logapi/drivers/ovn/driver.py +++ b/neutron/services/logapi/drivers/ovn/driver.py @@ -141,6 +141,7 @@ class OVNDriver(base.DriverBase): return set() if log_obj.event == log_const.ACCEPT_EVENT: return {ovn_const.ACL_ACTION_ALLOW_RELATED, + ovn_const.ACL_ACTION_ALLOW_STATELESS, ovn_const.ACL_ACTION_ALLOW} if log_obj.event == log_const.DROP_EVENT: return {ovn_const.ACL_ACTION_DROP, @@ -149,6 +150,7 @@ class OVNDriver(base.DriverBase): return {ovn_const.ACL_ACTION_DROP, ovn_const.ACL_ACTION_REJECT, ovn_const.ACL_ACTION_ALLOW_RELATED, + ovn_const.ACL_ACTION_ALLOW_STATELESS, ovn_const.ACL_ACTION_ALLOW} def _remove_acls_log(self, pgs, ovn_txn, log_name=None): diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py index 0687661eefb..b37af937ba3 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovn_db_sync.py @@ -1133,7 +1133,7 @@ class TestOvnNbSync(base.TestOVNFunctionalBase): for sg in self._list('security-groups')['security_groups']: for sgr in sg['security_group_rules']: acl = acl_utils._add_sg_rule_acl_for_port_group( - utils.ovn_port_group_name(sg['id']), sgr) + utils.ovn_port_group_name(sg['id']), True, sgr) db_acls.append(TestOvnNbSync._build_acl_for_pgs(**acl)) for acl in acl_utils.add_acls_for_drop_port_group( diff --git a/neutron/tests/unit/fake_resources.py b/neutron/tests/unit/fake_resources.py index f4e85860b33..890c53a0b00 100644 --- a/neutron/tests/unit/fake_resources.py +++ b/neutron/tests/unit/fake_resources.py @@ -119,6 +119,8 @@ class FakeOvsdbNbOvnIdl(object): self.get_router_floatingip_lbs.return_value = [] self.is_col_present = mock.Mock() self.is_col_present.return_value = False + self.is_col_supports_value = mock.Mock() + self.is_col_supports_value.return_value = False self.get_lrouter = mock.Mock() self.get_lrouter.return_value = None self.delete_lrouter_ext_gw = mock.Mock() diff --git a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py index 3bff20c276a..0894e626b5c 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py +++ b/neutron/tests/unit/plugins/ml2/drivers/ovn/mech_driver/test_mech_driver.py @@ -199,19 +199,43 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): self._test__validate_network_segments_id_succeed, 300) @mock.patch.object(ovn_revision_numbers_db, 'bump_revision') - def test__create_security_group(self, mock_bump): - self.mech_driver._create_security_group( - resources.SECURITY_GROUP, events.AFTER_CREATE, {}, - security_group=self.fake_sg, context=self.context) + def _test__create_security_group( + self, stateful, stateless_supported, mock_bump): + self.fake_sg["stateful"] = stateful + with mock.patch.object(self.mech_driver._ovn_client, + 'is_allow_stateless_supported', + return_value=stateless_supported): + self.mech_driver._create_security_group( + resources.SECURITY_GROUP, events.AFTER_CREATE, {}, + security_group=self.fake_sg, context=self.context) external_ids = {ovn_const.OVN_SG_EXT_ID_KEY: self.fake_sg['id']} pg_name = ovn_utils.ovn_port_group_name(self.fake_sg['id']) self.nb_ovn.pg_add.assert_called_once_with( name=pg_name, acls=[], external_ids=external_ids) + if stateful or not stateless_supported: + expected = ovn_const.ACL_ACTION_ALLOW_RELATED + else: + expected = ovn_const.ACL_ACTION_ALLOW_STATELESS + for c in self.nb_ovn.pg_acl_add.call_args_list: + self.assertEqual(expected, c[1]["action"]) + mock_bump.assert_called_once_with( mock.ANY, self.fake_sg, ovn_const.TYPE_SECURITY_GROUPS) + def test__create_security_group_stateful_supported(self): + self._test__create_security_group(True, True) + + def test__create_security_group_stateful_not_supported(self): + self._test__create_security_group(True, False) + + def test__create_security_group_stateless_supported(self): + self._test__create_security_group(False, True) + + def test__create_security_group_stateless_not_supported(self): + self._test__create_security_group(False, False) + @mock.patch.object(ovn_revision_numbers_db, 'delete_revision') def test__delete_security_group(self, mock_del_rev): self.mech_driver._delete_security_group( @@ -239,7 +263,7 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): has_same_rules.assert_not_called() ovn_acl_up.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, - 'sg_id', rule, is_add_acl=True) + 'sg_id', rule, is_add_acl=True, stateless_supported=False) mock_bump.assert_called_once_with( mock.ANY, rule, ovn_const.TYPE_SECURITY_GROUP_RULES) @@ -259,7 +283,7 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): has_same_rules.assert_not_called() ovn_acl_up.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, - 'sg_id', rule, is_add_acl=True) + 'sg_id', rule, is_add_acl=True, stateless_supported=False) mock_bump.assert_called_once_with( mock.ANY, rule, ovn_const.TYPE_SECURITY_GROUP_RULES) @@ -276,7 +300,7 @@ class TestOVNMechanismDriver(TestOVNMechanismDriverBase): security_group_rule=rule, context=self.context) ovn_acl_up.assert_called_once_with( mock.ANY, mock.ANY, mock.ANY, - 'sg_id', rule, is_add_acl=False) + 'sg_id', rule, is_add_acl=False, stateless_supported=False) mock_delrev.assert_called_once_with( mock.ANY, rule['id'], ovn_const.TYPE_SECURITY_GROUP_RULES) @@ -2967,8 +2991,8 @@ class TestOVNMechanismDriverSecurityGroup( for r in res['security_group_rules']: self._delete('security-group-rules', r['id']) - def _create_sg(self, sg_name): - sg = self._make_security_group(self.fmt, sg_name, '') + def _create_sg(self, sg_name, **kwargs): + sg = self._make_security_group(self.fmt, sg_name, '', **kwargs) return sg['security_group'] def _create_empty_sg(self, sg_name): diff --git a/releasenotes/notes/ovn-support-stateless-sg-849170c5d320487e.yaml b/releasenotes/notes/ovn-support-stateless-sg-849170c5d320487e.yaml new file mode 100644 index 00000000000..39feeb0a0d0 --- /dev/null +++ b/releasenotes/notes/ovn-support-stateless-sg-849170c5d320487e.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Support stateless security groups with the latest OVN 21.06+. + The stateful=False security groups are mapped to the new + "allow-stateless" OVN ACL verb.