SecurityGroups Tests : invalid id must be valid uuid

Bug #1182384 reported by Jordan Pittier on 2013-05-21
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
High
Unassigned
tempest
Medium
Matt Riedemann

Bug Description

Hi,
When a try to run this test suite : nosetests tempest/tests/compute/security_groups/test_security_group_rules.py the tests test_security_group_rules_create_with_invalid_id and test_security_group_rules_delete_with_invalid_id fails with the following same stack trace :

_StringException: Traceback (most recent call last):
  File "/opt/stack/tempest/tempest/tests/compute/security_groups/test_security_group_rules.py", line 190, in test_security_group_rules_delete_with_invalid_id
    rand_name('999'))
BadRequest: Bad request
Details: {'message': 'Security group id should be uuid', 'code': '400'}

I think it is caused by this check : https://github.com/openstack/nova/blob/master/nova/network/security_group/quantum_driver.py#L137

I am working on a patch.

Jordan

------------------
Tempest master
Quantum v2.0
Openvswitch

Changed in tempest:
assignee: nobody → Jordan Pittier (jordan-pittier)
Changed in tempest:
status: New → In Progress
Sean Dague (sdague) wrote :

Adding to nova as well as this is a case where Nova is returning different error codes based on back end for the same input, which seems like a no no.

Changed in nova:
status: New → Confirmed
Changed in tempest:
importance: Undecided → Medium
milestone: none → havana-1
Sean Dague (sdague) on 2013-06-03
Changed in tempest:
milestone: havana-1 → havana-2

Fix proposed to branch: master
Review: https://review.openstack.org/35285

Changed in nova:
assignee: nobody → Jordan Pittier (jordan-pittier)
status: Confirmed → In Progress
Matt Riedemann (mriedem) wrote :

Proposed a patch to skip the tests until this is resolved: https://review.openstack.org/#/c/40015/

Matt Riedemann (mriedem) wrote :

Another patch to skip more tests for the same root issue in nova:

https://review.openstack.org/#/c/40669/

Attila Fazekas (afazekas) wrote :

This is one of the different behaviours when you use neutron instead of nova network.
I wonder why this issue does not have high/medium priority in nova.

Reviewed: https://review.openstack.org/40669
Committed: http://github.com/openstack/tempest/commit/31f28ddc22154375cc6435e7625ded1bad28c1a7
Submitter: Jenkins
Branch: master

commit 31f28ddc22154375cc6435e7625ded1bad28c1a7
Author: Matt Riedemann <email address hidden>
Date: Tue Aug 6 18:20:45 2013 -0700

    Skip more security group tests until bug 1182384 is fixed

    This patch builds on change I3f0d21c23661f556354c321476ba559d4925dccd
    which skipped a couple of tests that are failing in nova if neutron is
    configured. The previous patch missed a couple of other tests that fail
    for the same bug. This patch skips those other failing tests for the
    same bug.

    Related-Bug: #1182384

    Change-Id: I4f191f21232f9e58ad09456261fb6247d64f22b5

Matt Riedemann (mriedem) wrote :

I've changed the priority since we're skipping the tests in tempest now.

Changed in nova:
importance: Undecided → High
Miguel Lavalle (minsel) wrote :

mriedem,

I read all the comments in this bug. It is not clear to me what is the next step. Are we going to add uuid's to security groups in nova? I am interested in helping to solve this issue, but I want to undertsnad first how we plan to move forward

Cheers

Matt Riedemann (mriedem) wrote :

This was the conversation I had with Sean Dague in IRC back on 6/18:

http://paste.openstack.org/show/44543/

When Sean is talking about catching the 400 and translating it to a 404, I think that's tied to the second option in the mailing list thread here:

http://lists.openstack.org/pipermail/openstack-dev/2013-July/011666.html

"> 2) Encapsulate all calls to validate_id() in a try/catch HTTPBadRequest and raise a HTTPNotFound instead (exception translation)"

However, later when I talked to Sean on 7/24 he suggested I could try to fix the nova security group id validation issue by using this patch as a template:

https://review.openstack.org/#/c/34559/

In that patch nova-network and neutron are validating name/description differently (neutron wasn't actually validating name/description), and it tries to move the validation into the base class. I'm not actually sure how that applies to the id validation since that's implemented in both the nova-network and neutron SecurityGroupBase implementations in nova.

So it sounds like Sean thinks we should return 404 in either case when the id is invalid, but I've also seen the proposed tempest patch to fix the nova tests in tempest if neutron is being used, and I didn't understand the problem with those (maybe because the client is forced to know the api implementation in nova?).

Anyway, could probably follow up on the mailing list or try to get a discussion going in #openstack-nova IRC?

Miguel Lavalle (minsel) wrote :

Today, I spent some time running tempest/tests/compute/security_groups/test_security_group_rules.py (the test case mentioned in the bug description). I ran it in two different devstack instances: one with nova network and the other with neutron. This is the summary of the results:

1) In the instance with nova network: when method test_security_group_rules_create_with_invalid_id attempts to create rules in a security group that doesn't exist and the security group id is a string convertible to an integer, the response is a 404, which is what it expects. When the same method is modified to provide a security group id that is a string not convertible to integer, the response is a 400, and it fails.

2) In the instance with neutron: when method test_security_group_rules_create_with_invalid_id attempts to create rules in a security group that doesn't exist and the security group id is a string convertible to an integer, the response is a 400 and it fails. When the same method is modified to provide a security group id that is a valid uuid, the response is a 404, which is what it expects.

Based on this behavior, I propose that we don't have a bug. When this test provides a security group id in the correct format to the nova api (integer string for nova network, uuid for neutron) the response is the expected 404. Conversely, when this test provides a security group id in an incorrect format, both nova network and neutron return a 400. In my opinion, these are correct behaviors

Matt Riedemann (mriedem) wrote :

Note that for the floating IPs tests, tempest is doing something like this now (checking for neutron and using a fake UUID):

https://github.com/openstack/tempest/blob/master/tempest/api/compute/floating_ips/test_list_floating_ips.py#L93

https://github.com/openstack/tempest/blob/master/tempest/api/compute/floating_ips/test_floating_ips_actions.py#L52

Which is what Jordan was trying to do here: https://review.openstack.org/#/c/29899/

Looks like some other cores were OK with it here: https://review.openstack.org/#/c/46886/

Matt Riedemann (mriedem) on 2013-11-06
Changed in nova:
assignee: Jordan Pittier (jordan-pittier) → nobody
status: In Progress → Confirmed
Changed in tempest:
assignee: Jordan Pittier (jordan-pittier) → nobody
status: In Progress → Confirmed

Fix proposed to branch: master
Review: https://review.openstack.org/55455

Changed in tempest:
assignee: nobody → Matt Riedemann (mriedem)
status: Confirmed → In Progress

Reviewed: https://review.openstack.org/55455
Committed: http://github.com/openstack/tempest/commit/b0ede30709729b32c8bca6b0df581c236ac39c32
Submitter: Jenkins
Branch: master

commit b0ede30709729b32c8bca6b0df581c236ac39c32
Author: Matt Riedemann <email address hidden>
Date: Wed Nov 6 12:15:28 2013 -0800

    Remove skips for bug 1182384

    Commit a2ccca0 fixed some negative tests to check for neutron when
    determining the invalid ID to use for nova APIs that behave differently
    when using neutron vs nova-network.

    This patch uses the same idea to remove the skips in place for bug
    1182384 in the compute security group negative tests.

    Closes-Bug: #1182384

    Change-Id: I58ba125012a74ea49311a163e9bf7fd7af33c1fc

Changed in tempest:
status: In Progress → Fix Released
Changed in nova:
status: Confirmed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers