Race condition exists for default security group creation

Bug #1194579 reported by Maru Newby
58
This bug affects 7 people
Affects Status Importance Assigned to Milestone
neutron
Fix Released
Medium
Ann Taraday

Bug Description

A race condition exists in the creation of a tenant's default security group, since the uniqueness is guaranteed by the name and the check is done in code. As a result, multiple requests have the potential to create multiple default security groups.

It probably makes sense to ensure the uniqueness of the security group name and tenant id relationship via a constraint on the model.

Maru Newby (maru)
description: updated
Revision history for this message
yong sheng gong (gongysh) wrote :
Changed in quantum:
importance: Undecided → High
status: New → Confirmed
milestone: none → havana-2
tags: added: neutron-core
removed: quantum-core
Changed in neutron:
milestone: havana-2 → havana-3
Changed in neutron:
milestone: havana-3 → havana-rc1
tags: added: havana-rc-potential
Changed in neutron:
milestone: havana-rc1 → none
Thierry Carrez (ttx)
tags: added: havana-backport-potential
removed: havana-rc-potential
Revision history for this message
Marios Andreou (marios-b) wrote :

Hi, I just submitted https://review.openstack.org/#/c/91805/. Mark, I don't know if this is the bug I asked you about a while back, may well be as I recall you saying something about overhauling some code to enforce this. I mean as was envisaged/described by the bluprint [1]. I then came across [2] which deals with the corresponding mac issue (though in code, no just on the model as slightly different situation) and there wasn't too much pushback there (though was ultimately abandoned...).

In any case, I made the review to also learn about the alembic migrations as I've never touched it before. Let me know if I should abandon this for whatever it is you have brewing to address it (unless it's a different bug...)

thanks, marios

[1] https://blueprints.launchpad.net/neutron/+spec/distributed-lock-resource-allocation
[2] https://review.openstack.org/#/c/34823/

Revision history for this message
Marios Andreou (marios-b) wrote :

This bug is specifically about creation of the security group called 'default'. Some questions:

* Do we really want to enforce that security group names are unique _per_tenant_? A lot of tests are failing with this migration because of duplicate security group names (this can be fixed obviously).
* If we do, we probably need to document it (will add !DocImpact) [1]
* How do we handle the case when someone tries to migrate who already has duplicate sg names for a given tenant_id? [2]

I just came across a relevant review @ [3]"Duplicate security group name cause fail to start instance" but this tried to enforce uniqueness in code, rather than the db schema.

[1] http://docs.openstack.org/api/openstack-network/2.0/content/security_groups.html
[2] https://review.openstack.org/#/c/91805/1/neutron/db/migration/alembic_migrations/versions/591bbc0502b2_enforce_unique_tenan.py
[3] https://review.openstack.org/#/c/79270/2

Revision history for this message
Attila Fazekas (afazekas) wrote :
Revision history for this message
Eugene Nikanorov (enikanorov) wrote :

Hi, are we going to move on with the fix?
The patch is on gerrit, Marios, can you resurrect it?

Changed in neutron:
status: Confirmed → In Progress
importance: High → Medium
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (master)

Change abandoned by Salvatore Orlando (<email address hidden>) on branch: master
Review: https://review.openstack.org/91805
Reason: This patch has been dead enough that I think it's safe to abandon.
The author can resurrect it if needed.

Changed in neutron:
assignee: Mark McClain (markmcclain) → Ann Kamyshnikova (akamyshnikova)
Revision history for this message
Ann Taraday (akamyshnikova) wrote :
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Change abandoned by Ann Kamyshnikova (<email address hidden>) on branch: master
Review: https://review.openstack.org/135006
Reason: I continue working on existing change as I see that people share my opinion here in thread. Seems that I'm wrong and disadvantages listed by others are more important. I will work on alternative.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (master)

Reviewed: https://review.openstack.org/142101
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=79c97120de9cff4d0992b5d41ff4bbf05e890f89
Submitter: Jenkins
Branch: master

commit 79c97120de9cff4d0992b5d41ff4bbf05e890f89
Author: Ann Kamyshnikova <email address hidden>
Date: Fri Dec 12 15:30:06 2014 +0300

    Default security group table

    This change prevents the race condition by enforcing a single default
    security group via new table default_security_group. It has tenant_id
    as primary key and security_group_id, which is id of default
    security group. Migration that inroduces this table has sanity check that
    verifies that there is no duplicate default security group in any
    tenant.

    This idea has come up from discussion in comments to
    https://review.openstack.org/135006

    DocImpact

    Closes-bug: #1194579

    Change-Id: Ifa8fbddd22bce4c50836cf443ebe10dff37443ef

Changed in neutron:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in neutron:
milestone: none → kilo-2
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to neutron (stable/juno)

Related fix proposed to branch: stable/juno
Review: https://review.openstack.org/156596

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on neutron (stable/juno)

Change abandoned by Ihar Hrachyshka (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/156596
Reason: That's an unreasonable and incomplete approach that we cannot take for stable releases. There seems to be no solution for Juno without db migration rules that are not available for backports in upstream, so abandoning the patch.

tags: removed: havana-backport-potential
Thierry Carrez (ttx)
Changed in neutron:
milestone: kilo-2 → 2015.1.0
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers