Wrong tenant-id field of bgpvpn-create operation can be registered

Thanks for the report.

Indeed it seems we need additional code in the service plugin to check the validity of the tenant_id.

We need to at least prevent giving an empty string, or a string longer than a UUID, like for instance what is done in other places in RESOURCE_ATTRIBUTE_MAP.

About checking that the tenant actually exists: I haven't found out what is done for other API resources (and I haven't a devstack ready with me to check now).

Since backend can use different DB, do you think it is better to check the tenant ID at the driver level instead of the service plugin level?

(Currently I have a patch for the OpenContrail driver that works)

I think it should be done in the plugin, so that we don't have to write it again for each driver.

And anyway, Keystone is authoritative on whether or not the tenant exists or not, independently of whether or not the BGPVPN driver is using Neutron DB.

But, before doing it, I think it would be better to check how other components do it.
I actually would do it right now, but I'm traveling without enough no time to dig.

I don't know if it is a good example but I saw how it was made in neutron net-create (with --tenant-id in admin) and so it is done in the Opencontrail plugin (in the case of Contrail backend). But it is not the same level since opencontrail does not work with ML2 ... So what I want to say, it seems like the tenant ID check is made by the backend plugin/driver.
But I can also check that in other project :) to be more confident.

What I found was that neutron does minimal checks at DB insertion:

https://github.com/openstack/neutron/blob/master/neutron/db/model_base.py#L30

Do you know a piece of code where Neutron actually checks that the tenant exists ?

Note directly related, but worth mentioning: a transition is ongoing to use project_id rather than tenant_id.

See https://review.openstack.org/344383

I'm not sure to understand what we want to re-enforce here.

Admin can create a bgpvpn with any tenant-id, as he can create a network with any tenant-id, but nothing can be done with it since we enforce that tenants are identical during an association to a router or a network :

http://git.openstack.org/cgit/openstack/networking-bgpvpn/tree/networking_bgpvpn/neutron/services/plugin.py#n150

http://git.openstack.org/cgit/openstack/networking-bgpvpn/tree/networking_bgpvpn/neutron/services/plugin.py#n150

We could enforce some constraints on the DB schema and model to prevent an empty or too long uuid, but I think it's already done, at least in the schema :

http://git.openstack.org/cgit/openstack/networking-bgpvpn/tree/networking_bgpvpn/neutron/db/migration/alembic_migrations/versions/liberty/expand/17d9fd4fddee_initial.py#n40

So I found something quite interesting. In fact by using the ml2 plugin stuff, I noticed that there is no tenant-id check, indeed any neutron operation done by the admin with --tenant-id works, and the wrong information is registered in the DB (of course the following logical operations to proceed with neutron resource didn't work).

But within the opencontrail plugin, there are some tenant-id check for each neutron resource creation.
https://github.com/Juniper/contrail-neutron-plugin/blob/R3.0/neutron_plugin_contrail/plugins/opencontrail/vnc_client/vn_res_handler.py#L166

So if we want to spread the same behaviour to bgpvpn, just to patch the opencontrail driver is required.

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

Reviewed: https://review.openstack.org/354640
Committed: https://git.openstack.org/cgit/openstack/networking-bgpvpn/commit/?id=fcfdd8376cad5872589947d2331a0a97c130d60c
Submitter: Jenkins
Branch: master

commit fcfdd8376cad5872589947d2331a0a97c130d60c
Author: bfernando <email address hidden>
Date: Fri Aug 12 12:30:14 2016 +0200

    Add a tenant ID check to create a bgpvpn resource

    Only in the opencontrail driver (as other neutron resource creation)
    Improve the opecontrail_client by adding the 'project' resource
    Add a function that check the tenant ID by reading (show) the project
    Made the check in bgpvpn 'create' and 'update' functions

    Change-Id: Iedea7863bed39d9a8f4bff1e2fa647bdd45ba3a5
    Closes-Bug: #1604748

Fix proposed to branch: stable/mitaka
Review: https://review.openstack.org/356315

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

Reviewed: https://review.openstack.org/356315
Committed: https://git.openstack.org/cgit/openstack/networking-bgpvpn/commit/?id=a9d4479fe30bf5c81b091813a8b9580bee471e07
Submitter: Jenkins
Branch: stable/mitaka

commit a9d4479fe30bf5c81b091813a8b9580bee471e07
Author: bfernando <email address hidden>
Date: Fri Aug 12 12:30:14 2016 +0200

    Add a tenant ID check to create a bgpvpn resource

    Only in the opencontrail driver (as other neutron resource creation)
    Improve the opecontrail_client by adding the 'project' resource
    Add a function that check the tenant ID by reading (show) the project
    Made the check in bgpvpn 'create' and 'update' functions

    Change-Id: Iedea7863bed39d9a8f4bff1e2fa647bdd45ba3a5
    Closes-Bug: #1604748
    (cherry picked from commit fcfdd8376cad5872589947d2331a0a97c130d60c)

Fix proposed to branch: stable/mitaka
Review: https://review.openstack.org/360974

Reviewed: https://review.openstack.org/360656
Committed: https://git.openstack.org/cgit/openstack/networking-bgpvpn/commit/?id=c0eefd72c1122377f34b5fc8b5d671c41f5bf5f3
Submitter: Jenkins
Branch: master

commit c0eefd72c1122377f34b5fc8b5d671c41f5bf5f3
Author: bfernando <email address hidden>
Date: Thu Aug 25 17:58:51 2016 +0200

    Add error management regarding malformed UUID

    Catch the exception raised by the UUID library in the case of a
    malformed UUID and raise an OpenContrailMalformedUUID managed by
    Neutron server logs.

    Change-Id: I05dc8a380a3e5a8d54f21cef186cb532ec9461f2
    Closes-Bug: #1604748

Reviewed: https://review.openstack.org/360974
Committed: https://git.openstack.org/cgit/openstack/networking-bgpvpn/commit/?id=c25a34038f838b4d55ec5f088e8e8720380c61da
Submitter: Jenkins
Branch: stable/mitaka

commit c25a34038f838b4d55ec5f088e8e8720380c61da
Author: bfernando <email address hidden>
Date: Thu Aug 25 17:58:51 2016 +0200

    Add error management regarding malformed UUID

    Catch the exception raised by the UUID library in the case of a
    malformed UUID and raise an OpenContrailMalformedUUID managed by
    Neutron server logs.

    Change-Id: I05dc8a380a3e5a8d54f21cef186cb532ec9461f2
    Closes-Bug: #1604748
    (cherry picked from commit c0eefd72c1122377f34b5fc8b5d671c41f5bf5f3)

This issue was fixed in the openstack/networking-bgpvpn 4.0.2 release.

This issue was fixed in the openstack/networking-bgpvpn 5.0.0 release.