Since [1], we introduced a way to skip the load of the OVO synthetic
fields depending on the resource fields retrieved. In the case of the
security groups (SG), the SG rules are child objects to the SGs. The SG
rules are retrieved when a SG OVO is created.
The improvement done in [1] is to make the SG rules load dynamically,
that means using the load mode "lazy='dynamic'". That will issue a SQL
query only if the SG rules are read; if not, the query is never issued.
However since [2] (and this is previous to the [1] optimization), we
always add the field "shared" to the filters and thus to the fields to
retrieve, because it is a policy required field. Because "shared" is a
synthetic field [3], that will force the SG "synthetic_fields" load and
the retrieval of the SG rules always. This is undoing any performance
improvement.
This patch is changing the loading method to "joined"; that will request
the SG rules in the same SQL query, instead of issuing separate queries
for the SG rules. Until a method to load the SG "shared" field,
independently of the synthetic OVO fields is implemented, this change
will improve the SG retrieval performance. In a testing environment
with around 5500K SG and 4 rules (default ones) per SG:
* lazy='dynamic': 38 seconds
* lazy='select': 20 seconds
* lazy='joined': 12 seconds
[1]https://review.opendev.org/q/topic:%22bug/1810563%22
[2]https://review.opendev.org/c/openstack/neutron/+/328313
[3]b85b19e384/neutron/objects/rbac_db.py (L349)
Related-Bug: #2052419
Change-Id: I300464472f2348d148f2a3ddac384c883d9d815b
The method ``delete_security_group_rule`` is publishing the
BEFORE_DELETE event before starting the security group rule deletion.
This event is published using a wrap method called
``SecurityGroupDbMixin._registry_publish``. This method is capturing
any ``CallbackFailure`` exception and raising a
``SecurityGroupRuleInUse`` one. That makes no sense because:
* We are hidding the real cause of the callback failure.
* The BEFORE_DELETE is not checking that the security group rule is
being used (NOTE 1).
* If any new implementation makes this check, the corresponding callback
should return explicitly this exception.
The method ``_create_security_group_rule`` is publishing the
BEFORE_CREATE event before starting the security group rule creation.
The same argument applies here: the callback manager should return the
exception raise by the callback method (NOTE 2).
In a follow-up patch, this events will be captured to check the
permissions related to the user creating or deleting the security group
rule. In case of error, it will be needed to raise a ``NotAuthorized``
derived exception, instead of a ``InUse`` one.
NOTE 1: this is the current use of BEFORE_DELETE event in the
OpenStack repository:
* [2] Omni project had no activity for the last 4 years.
* [3] networking-arista: the method ``run_cmds_on_all_switches``, that
calls ``run_openstack_sg_cmds``, returns its own exceptions.
* [4] networking-opencontrail: same justification.
* [5] The ML2/OVN mechanism driver, that will raise an exception if the
OVN ACL deletion doesn't succeed.
NOTE 2: this is the current use of BEFORE_DELETE event in the
OpenStack repository:
* [2] Omni project had no activity for the last 4 years.
[1]https://codesearch.openstack.org/?q=%5C.SECURITY_GROUP_RULE&i=nope&literal=nope&files=&excludeFiles=&repos=
[2]https://opendev.org/x/omni/src/branch/master/neutron/neutron/plugins/ml2/drivers/aws/callbacks.py
[3]https://opendev.org/x/networking-arista/src/branch/master/networking_arista/ml2/security_groups/arista_security_groups.py
[4]https://opendev.org/x/networking-opencontrail/src/branch/master/networking_opencontrail/ml2/opencontrail_sg_callback.py
[5]https://opendev.org/openstack/neutron/src/branch/master/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py
Partial-Bug: #2019960
Change-Id: I8d5f5392fb7a6ab9b20e9222c143f4e67c925cae
Default SG rules created as template in the Neutron DB are now used to
create security group rules for each new default and non-default SG
created in Neutron.
Closes-bug: #1983053
Change-Id: Iaf27deb955c3844409fcd36239511478e9607a82
This patch adds DB model, OVO class and DB migration script for
SG rules template used for every new SG created.
It also implements Create/Get/Delete actions for that new resource and
adds API policies for those APIs
Related-Bug: #1983053
Change-Id: Ib3cde1710edd400b972f493b13666d0679a7753c
This reverts commit 6358495720.
Reason for revert: This is generating a lot of
"SecurityGroupNotFound" errors in neutron-server.log in
the tempest-integrated-networking job.
Closes-Bug: #2019449
Related-Bug: #2008712
Change-Id: I077fe87435f61bd29d5c1efc979c2adebca26181
Based on bug #2008712 if we have a security-group which
is the remote group of a 2nd security-group, the backend
never deletes the rule of the 2nd group which
remote_group_id is the original security-group.
By AFTER_DELETE event for each rule that has the
security_group_id as remote_group_id, we can make the
mech drivers do their work and delete these rules in the
backend.
Change-Id: I207ecf7954b06507e03cb16b502ceb6e2807e0e7
Closes-Bug: #2008712
The attempt to list security groups for a project, or any
random string, can create a default SG for it. Only allow if
privileges support it.
Closes-bug: #1988026
Change-Id: Ieef7011f48cd2188d4254ff16d90a6465bbabfe3
Add the shared field to security group API responses and support
using shared as a query filter.
A follow-up patch will remove the temporary api def once it is merged
and released in neutron-lib.
Related-Bug: #1942615
Depends-On: https://review.opendev.org/c/openstack/neutron-lib/+/812617
Change-Id: Ic04be8f0b7097c8aed19365f06089aa7af333eb9
The goal of [1] is to, in case of failing when removing the quota
reservation, continue the operation. Any expired reservation will
be removed automatically in any driver.
If the DB transaction fails, it should affect only to the reservation
trying to be deleted. This is why this patch isolates the
"remove_reservation" method and guarantees it is called outside an
active DB session. That guarantees, in case of failure, no other DB
operation will be affected.
This patch also partially reverts [2] but still checks the security
group rule quota when a new security group is created. Instead of
creating and releasing a quota reservation for the security group
rules created, now only the available quota limit is checked before
creating them. That won't prevent another operation to create security
group rules in parallel, exceeding the available quota. However, this
is not even guaranteed with the current quota driver.
[1]https://review.opendev.org/c/openstack/neutron/+/805031
[2]https://review.opendev.org/c/openstack/neutron/+/701565
Closes-Bug: #1943714
Change-Id: Id73368576a948f78a043d7cf0be16661a65626a9
This patch switches over to callback payloads for SECURITY_GROUP
events. To do so a few shims are put into place the handle both
payload and kwarg style callbacks; these shims will be removed once
all events use payloads. In addition a few UT updates are included to
get the tests working properly with payloads.
Change-Id: I6161a8b387812808c4d679f882a3193c93235647
If security group API is disabled, there is no point to create default
security group for tenant when e.g. network is created.
Closes-Bug: #1913297
Change-Id: Ib73babdd563e3e8c21ce6f63456cc87af414c5aa
New API extension was added in [1] to extend security group rules with
"normalized_cidr" read only attribute.
This patch implements this API extension in Neutron ML2 plugin and
extends security group rules with "normalized_cidr" value.
[1] https://review.opendev.org/#/c/743630/
Related-Bug: #1869129
Change-Id: I65584817a22f952da8da979ab68cd6cfaa2143be
- Add api extension and db model changes to support remote_address_group_id
in SG rules.
- RPC and firewall agent changes will be in the follow-up patches.
Change-Id: I99681736d05eefd82bdba72b3866eab9468ef5dd
Implements: blueprint address-groups-in-sg-rules
Included standard attributes ID in some OVO dictionaries to improve
the OVN revision numbers operation. Having this ID, there is no need
to retrieve it from the database.
The following OVOs have been updated:
- port
- network
- subnet
- security group
- security group rule
- router
Closes-Bug: #1904188
Change-Id: Ia16335a2fc8f9324b9489692c76a73e4ef5bef96
Method _ensure_default_security_group wasn't atomic as it first tries to get
default SG and if that not exists in DB, it tries to create it.
It may happend, like e.g. in Calico plugin that between
get_default_sg_id method and create_security_group method, this default
SG will be created by other neutron worker. And in such case there will
be Duplicate entry exception raised.
So this patch is adding handling of such exception.
Change-Id: I515c310f221e7d9ae3be59a26260538d1bc591c2
Closes-Bug: #1883730
Allow the subscriber to know the deleted security group name. It can
help some downstream callback utilization to use the deleted sg name for
additional workflow.
Change-Id: Ia321ff96cf445d20f082779d3f6a96fac07b0943
Now that we are python3 only, we should move to using the built
in version of mock that supports all of our testing needs and
remove the dependency on the "mock" package.
This patch moves all references to "import mock" to
"from unittest import mock". It also cleans up some new line
inconsistency.
Fixed an inconsistency in the OVSBridge.deferred() definition
as it needs to also have an *args argument.
Fixed an issue where an l3-agent test was mocking
functools.partial, causing a python3.8 failure.
Unit tests only, removing from tests/base.py affects
functional tests which need additional work.
Change-Id: I40e8a8410840c3774c72ae1a8054574445d66ece
Retrieving the SG rules now is used the admin context. This allows to
get all possible rules, independently of the user calling. The filters
passed and the RBAC policies filter those results, returning only:
- The SG rules belonging to the user.
- The SG rules belonging to a SG owned by the user.
However, if the SG list is too long, the query can take a lot of time.
Instead of this, the filtering is done in the DB query. If no filters
are passed to "get_security_group_rules" and the context is not the
admin context, only the rules specified in the first paragraph will
be retrieved.
Because overwriting the method "get_objects" is too complex, an
intermediate query is done to retrieve the SG rule IDs. Those IDs
will be used as a filter in the "get_objects" call.
Closes-Bug: #1863201
Change-Id: I25d3da929f8d0b6ee15d7b90ec59b9d58a4ae6a5
The tracked resources quota check is done at the beginning of an API
call to the Neutron server. The API call contains a resource and an
action over the resource. In case of creation, the server checks if
the number of items requested fits in the existing quota.
In case of security group creation, the tracked resource checked is
"security_group". But "SecurityGroupDbMixin.create_security_group"
method also creates several default rules for the new group and the
quota for "security_group_rule" is not enforced.
This patch checks the number of security group rules being created
("delta") and checks in the plugin method (not in the API method) if
there is enough room for those new rules (tracked resource
"security_group_rule").
Change-Id: I0a9b91b09d6260ff96fdba2f0a455de53bbc1f00
Closes-Bug: #1858680
Removed E125 (continuation line does not distinguish itself
from next logical line) from the ignore list and fixed all
the indentation issues. Didn't think it was going to be
close to 100 files when I started.
Change-Id: I0a6f5efec4b7d8d3632dd9dbb43e0ab58af9dff3
The functionality within neutron.db.common_db_mixin is available via
neutron-lib APIs. This patch removes common_db_mixin and updates any
uses of it to use neutron-lib instead.
Depends-On: https://review.openstack.org/#/c/636159/
NeutronLibImpact
Change-Id: I2388f90b37abb09408809dda8c21da551bcd94bb
After taking a closer look at bug 1818385, I found a couple
of follow-on things to fix in the security group code.
First, there are very few protocols that accept ports,
especially via iptables. For this reason I think it's
acceptable that the API rejects them as invalid.
Second, UDPlite has some interesting support in iptables. It
does not support using --dport directly, but does using
'-m multiport --dports 123', and also supports port ranges using
'-m multiport --dports 123:124'. Added code for this special
case.
Change-Id: Ifb2e6bb6c7a2e2987ba95040ef5a98ed50aa36d4
Closes-Bug: #1818385
Sometime between liberty and pike, adding rules to SG's got
slow, and slower with every rule. Streamline the rule create path,
and get close to the old performance back.
Two performance fixes:
1. Get rid of an n^2 duplicate check, using a hash table instead,
on bulk creates. This is more memory intensive than the previous loop,
but usable far past where the other becomes too slow to be useful.
2. Use an object existence check in a few places where we do not
want to load all of the child rules.
Co-Authored-By: William Hager <whager@salesforce.com>
Change-Id: I34e41a128f28211f2e7ab814a2611ce22620fcf3
Closes-bug: 1810563
This patch switches callbacks over to the payload object style events
[1] for PRECOMMIT_UPDATE based notifications. To do so a DBEventPayload
object is used with the publish() method to pass along the related data.
In addition a few UTs are updated to work with the changes. Finally
a few shims are put into place to allow PRECOMMIT_UPDATE based events to
use payloads while still supporting the existing kwarg style events.
NeutronLibImpact
[1] https://docs.openstack.org/neutron-lib/latest/contributor/callbacks.html#event-payloads
Change-Id: Ie6d27df01cd7b87894efc80946d41eb1ebe25bef
Iptables only supports port-ranges for certain protocols,
others will generate failures, possibly leaving the agent
looping trying to apply rules. Change to not allow port
ranges outside of the list of known good protocols.
Change-Id: I5867f77fc5aedc169b42f50def0424ff209c164c
Closes-bug: #1749667
Port ranges validation has been done only for TCP and UDP.
Use the same validation logic for DCCP, SCTP and UDP-Lite, too.
APIImpact
DocImpact
Change-Id: Ife90be597d1a59a634d5474dad543dc1803e8242
By extending the black list to include the integer representation
for IPv6 we can succesfully block api requests to create security
group rules for IPv6 protocols with ehtertype IPv4.
Closes-Bug: #1706229
Change-Id: I5abeff178b3be18f1e93d00d9d546147b11c1a74
In reviews we usually check import grouping but it is boring.
By using flake8-import-order plugin, we can avoid this.
It enforces loose checking so it sounds good to use it.
This flake8 plugin is already used in tempest.
Note that flake8-import-order version is pinned to avoid unexpected
breakage of pep8 job.
Setup for unit tests of hacking rules is tweaked to disable
flake8-import-order checks. This extension assumes an actual file exists
and causes hacking rule unit tests.
Change-Id: Ib51bd97dc4394ef2b46d4dbb7fb36a9aa9f8fe3d
This patch is follow up for [1].
When updating security group, pass updated value to precommit.
old value is passed as original_security_group and new value
is passed as security_group.
[1] https://review.openstack.org/#/c/448420/
Although argument to callbacks will be unified by EventPayload
eventually, it's necessary to unbreak decomposed modules.
Change-Id: I5a27d1d218a0be4fae6f9740559bbbf773518821
Related-bug: #1546910
Fix a regression from the recent change. [1]
[1] I6f49f25eb2ad16221357024f45a6bb6175d5cd55
Closes-Bug: #1698812
Change-Id: Ifef2561ef4ff2a44068fc008475b216fdabe7095
PRECOMMIT_XXX events callback need completed sg info, like the sg id
and its related rules for registered driver.
Change-Id: I6f49f25eb2ad16221357024f45a6bb6175d5cd55
Co-Authored-By: Rui Wang <starwangrui@gmail.com>
Co-Authored-By: Manjeet Singh Bhatia <manjeet.s.bhatia@intel.com>
Co-Authored-By: Yalei Wang <yalei.wang@intel.com>
Closes-bug: #1546910
The callback modules have been available in neutron-lib since commit [1]
and are ready for consumption.
As the callback registry is implemented with a singleton manager
instance, sync complications can arise ensuring all consumers switch to
lib's implementation at the same time. Therefore this consumption has
been broken down:
1) Shim neutron's callbacks using lib's callback system and remove
existing neutron internals related to callbacks (devref, UTs, etc.).
2) Switch all neutron's callback imports over to neutron-lib's.
3) Have all sub-projects using callbacks move their imports over to use
neutron-lib's callbacks implementation.
4) Remove the callback shims in neutron-lib once sub-projects are moved
over to lib's callbacks.
5) Follow-on patches moving our existing uses of callbacks to the new
event payload model provided by neutron-lib.callback.events
This patch implements #2 from above, moving all neutron's callback
imports to use neutron-lib's callbacks.
There are also a few places in the UT code that still patch callbacks,
we can address those in step #4 which may need [2].
NeutronLibImpact
[1] fea8bb64ba7ff52632c2bd3e3298eaedf623ee4f
[2] I9966c90e3f90552b41ed84a68b19f3e540426432
Change-Id: I8dae56f0f5c009bdf3e8ebfa1b360756216ab886
Security group rule setting remote_ip_prefix 0.0.0.0/0 for ipv4 or
::/0 for ipv6 plays the same role as the sg rules without setting
remote_ip_prefix. We could treat them as duplicate.
Change-Id: Ic9213e77d3b03aded7fc34d486066c8af4a3b2a1
Closes-Bug: #1534113
In the AFTER_DELETE of sg_rule delete, the sg_rule is actually deleted.
This makes it impossible/hard to know which sg is affected.
This patch add the sg_id in the event. And this patch will be used in [1].
[1] https://review.openstack.org/#/c/367718
Change-Id: Ibdef6a703913b74504e402d225b1a0dffadb7aff
Closes-Bug: #1621698
Currently we only pass the group id to event callback when deleting, but
this is not sufficient for ODL which don't maintain the relationship
between sg and its rules.
Because some rules added into SG still exists and need be cascading
deleted, like what did in DB. Both AFTER_DELETE and PRECOMMIT_DELETE
events in SG deleting have the similar problem.
This patch adds the rules into SG when send delete event for SG.
Co-Authored-By: Manjeet Singh Bhatia <manjeet.s.bhatia@intel.com>
Change-Id: If66b78243c55aefa57cc4975a6f8ea4d05dc6afa
Close-Bug: #1557976
Since neutron_lib 0.2.0 contains PROTO_NAME_IPV6_ICMP_LEGACY, remove
it from neutron.common.constants
Change-Id: Idd150ce1cbe660fc9529e5d46678f37142490a28
Closes-bug: #1585047
If a plugin wants to specify the id resource for a security group rule
to the db_base_class the code currently does not ignore this id field
when checking if the rules is a duplicate. This patch updates the code
so that it ignores the id field when checking for duplicate rules
Change-Id: Id4906cbdebd820d3349a4a3211ebb34491516c68
If protocol was present in the dict, but was None, then it was never
re-instantiated after being popped out of the dict. This later resulted
in KeyError when trying to access the key on the dict.
Change-Id: I4985e7b54117bee3241d7365cb438197a09b9b86
Closes-Bug: #1566327
Rodolfo Alonso Hernandez