[RFE] API extension framework: impossible to extend a sub-resource

[Thomas Morin]

The code at [1] assumes that the resource to update is a base resource and the code update its dictionary. When the resource is a sub-resource, ie. a {'parent': ... , 'parameters': {..} } dictionary, the result of the update() call is that the content of 'parameters' is overwritten, while the correct thing to here would be to update the content of parameters with the new/changed attributes introduced by the extension.

(This issue is blocking the implementation of the bgpvpn routes-control blueprint [2] which is about an API extension that adds attributes to the "router_association" sub-resource introduced by the bgpvpn API extension.)

[1] https://github.com/openstack/neutron/blob/master/neutron/api/extensions.py#L348
[2] https://blueprints.launchpad.net/bgpvpn/+spec/routes-control

[OpenStack Infra (hudson-openstack)]

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

[Armando Migliaccio (armando-migliaccio)]

If my memory doesn't fail me, this is by design. Extending subresources further complicates client API bindings etc. Have you considered working out a different way to represent the relationship between what the API expose and what you're trying to add?

[Armando Migliaccio (armando-migliaccio)]

I'd almost say this is RFE material, because it has API impact.

[Thomas Morin (tmmorin-orange)]

Can you develop on how having new attributes on an existing sub-resource complicates client API binding ?

About "considered working out a different way": you can have a look at [1] and see for yourself, but its not obvious to me where else we could put the new 'advertise_extra_routes' attribute on 'router_association' in a way that would be consistent with the rest of the BGPVPN API.

[1] https://github.com/openstack/neutron-lib/blob/master/neutron_lib/api/definitions/bgpvpn_routes_control.py

[Armando Migliaccio (armando-migliaccio)]

To make sure we're on the same page, it would be good to state the exact steps to repro the issue you're observing. If I understand correctly, are you saying that it is currently impossible to update the value of a sub-attribute by means of the REST API call:

PUT /v2.0/api_prefix/top_level_resource/{resource_id}/sub_resource

That said, everything else works fine, ie. DELETE, POST and GET?

Agreed with your point about lack of consistency. I can see the use of sub-attributes is _extensively_ used in other parts of the bgpvpn API, as well as APIs like QoS and Flavors. We definitely need to come up with an answer.

[Armando Migliaccio (armando-migliaccio)]

As for the client binding issue I was referring to, the issue is not insurmountable...it's just that the client side framework is designed with the same limitation you're facing, i.e. that you would not go too deep into the resource hierarchy and thus client-side modifications would be required as well. If then you consider that you may potentially write API tests for these sub-resources, the change is no longer limited to the Neutron API framework alone and potentially extend to Tempest, OSC, etc. Hence we need to make sure what we're biting into :)

[Thomas Morin (tmmorin-orange)]

Let me state the problem in an hopefully clearer way, based on an example that happens to be what I'm working on:
- the starting point is the BGPVPN API extension, that has introduced a bgpvpn' resource, with 'network_association' and 'router_association' sub-resources
- we are adding a BGPVPN-routes-control API extension, which:
  * introduces a new 'port_association' sub-resource (easy come, easy go, nothing more about this below)
  * introduces a new attribute for the 'bgpvpn' resources (easy come, easy go, nothing more about this below)
  * introduces a new attribute for the 'router_association' sub-resource: this is something, I think, the code in neutron.api.extensions does not currently support

(So the problem I bring up is not that it would be impossible to do a PUT on an attribute of a sub-resource)

Since, yesterday, I have been reading more code, and found that the 'qos-bw-limit-direction' extension actually does add an attribute to the 'bandwidth_limit_rules' sub-resource of the 'policy' resource (see [1]). Since this merged in pike and there are tempest API tests for it, I have to assume this works and that I got something wrong, but I don't know what yet.

[1] http://git.openstack.org/cgit/openstack/neutron/tree/neutron/extensions/qos_bw_limit_direction.py#n56

[Thomas Morin (tmmorin-orange)]

My WIP code [1] breaks like this:

Traceback (most recent call last):
  File "/home/jenkins/workspace/gate-networking-bgpvpn-python27-ubuntu-xenial/.tmp/tmp.GRhLS1h1OZ/openstack/neutron/neutron/tests/base.py", line 118, in func
    return f(self, *args, **kwargs)
  File "/home/jenkins/workspace/gate-networking-bgpvpn-python27-ubuntu-xenial/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "networking_bgpvpn/tests/unit/services/test_plugin.py", line 1031, in test_create_bgpvpn_router_assoc
    do_disassociate=False):
  File "/usr/lib/python2.7/contextlib.py", line 17, in __enter__
    return self.gen.next()
  File "networking_bgpvpn/tests/unit/services/test_plugin.py", line 175, in assoc_router
    raise http_client_error(req, res)
webob.exc.HTTPClientError: Request 'POST http://localhost/bgpvpn/bgpvpns/43e8dbe8-b68a-40cb-80af-16d3b2c63188/router_associations.json {"router_association": {"tenant_id": "46f70361-ba71-4bd0-9769-3573fd227c4b", "router_id": "99b48a70-4127-4dbd-8cfe-f063f219db66"}}' failed: {"NeutronError": {"type": "HTTPBadRequest", "detail": "", "message": "Unrecognized attribute(s) 'tenant_id, router_id, project_id'"}}

http://logs.openstack.org/25/481125/24/check/gate-networking-bgpvpn-python27-ubuntu-xenial/7560207/console.html#_2017-10-12_10_40_19_735120

(the bgpvpn-routes-control extension which should have been extending the router_association sub-resource, has replaced it, and the base attributes for the sub-resource do not exist anymore)

(I still don't know why the qos-bw-limit-direction does not have this issue)

[Thomas Morin (tmmorin-orange)]

> (I still don't know why the qos-bw-limit-direction does not have this issue)

Hmm, yes, I see:

# The resource attribute map for the extension.
SUB_RESOURCE_ATTRIBUTE_MAP = {
    qos.BANDWIDTH_LIMIT_RULES: {
        'parameters': dict(
> qos.SUB_RESOURCE_ATTRIBUTE_MAP[
> qos.BANDWIDTH_LIMIT_RULES]['parameters'],
>
            **{'direction': {
                'allow_post': True,
                'allow_put': True,
                'is_visible': True,
                'default': common_constants.EGRESS_DIRECTION,
                'validate': {
                    'type:values': common_constants.VALID_DIRECTIONS}}}
        )
    }
}

The extension copies the base attributes to avoid overwriting.

Of course the trick works only when all the extensions extending something know each other, and will fall apart with more.

Doesn't it make sense to support this in a cleaner way ?

(I'll be glad to update my change to remove this cludge in the qos-bw-limit-direction extension)

[Armando Migliaccio (armando-migliaccio)]

Nice catch! It's easy to miss the kludge across the entirety of API definition.

As for fixing it, I am 50/50. Personally I wouldn't want to touch the neutron api/extension code with a 10 foot pole :)

On the other hand, the kludge should be discouraged but it generally requires a rethinking of the API. For instance, the l3 and trunk APIs have stayed away from the subresource pattern that came to life with flavor, QoS and bgpvpn.

[Thomas Morin (tmmorin-orange)]

I can understand one could refrain from touching this part of the code too much, but the alternatives seem to be:
- kludges like the one in qos-bw-limit-direction: not reasonably workable if more than one extension extends a sub-resource, people lose time finding out what they need to do to make their thing work, and the underlying API code does not become easier to maintain
- not extending sub-resources, but losing this ability will make it hard to extend existing APIs using them without having to do a v2 API ; I don't think we want to do that

It seems to me that the fix is simple enough (the only part which isn't elegant is looking at dict content to know whether it's a sub-resource or not and this part can possibly be improved a bit) and there are plenty of tests on API so any regression should be spotted.

(Taking a step back, I would tend agree with you that sub-resources introduce complexity and that we should be cautious and stop using them when not required. Looking back, using them for BGPVPN looked to me as a nice RESTFUL way of representing what we needed, but I now think this wasn't the best choice: our sub-resource are associations between a BGPVPN and a network (or a router), and there is not particular reason to have them sit as sub-resource of a BGPVPN resource, not more than as sub-resources of a network, router or port. So they could have been plain resources instead. We would have had the ability to list all bgpvpn-network (or bgpvpn-router) associations in one GET request. Nevertheless, doing a v2 API is too high of a price to pay to get extensibility.)

[Armando Migliaccio (armando-migliaccio)]

That's why it's paramount to have the API discussed in one place :)

But what's done is done. As for the options you present, I don't believe that option 1 is that likely anyway, code reviews should happen in a single place, we should document what the best practice is, and move on.

As for option 2, I am actually confused. Anything you do to touch the API must be flagged with an extension label. We really have no concept of version numbers in Neutron so I don't understand what you mean.

[Thomas Morin (tmmorin-orange)]

Armando.

Yes, discussing APIs in neutron-lib is the right way to go, totally agree.

Clarifications on what I was saying above, the alternatives I see to find a resolution for what currently blocks us from implementing the bgpvpn-routes-control API extension merged in neutron-lib:
- solve the bug in the API extension framework (neutron.api.extensions), like, or similarly as, in change https://review.openstack.org/511280
- workaround the problem like how it was done for qos-bw-limit-direction; if unelegant, this roughly works mostly when one API extension adds (or possibly changes) an attribute in a sub-resource, but if more than one API extension wants to do that, each API extension definition will have to refer to the sub-resource attribute map of all the other API extensions touching this sub-resource (doing the review work in one place would allow the thing to work, but we would then pass the line between "merely inelegant" to "ugly")
- the not-a-solution-alternative: abandon the idea of having an API extension that adds or modify attributes in a sub-resource (in that case doing a "BGPVPNv2" API extension would be the only option left to go beyond the behavior we have in BGPVPN today)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/511280
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=605364ad78407e3176dd79d0e4fe84cf87c8ce8e
Submitter: Zuul
Branch: master

commit 605364ad78407e3176dd79d0e4fe84cf87c8ce8e
Author: Thomas Morin <email address hidden>
Date: Wed Oct 11 17:57:49 2017 +0200

    Support that an extension extends a sub-resource

    The neutron.api.extensions code assumes that the resource to update are
    base resources and updates its dictionary. When the resource
    is a sub-resource, ie. a {'parent': ... , 'parameters': {..} } dictionary,
    the result of the update is that the content of 'parameters' is
    overwritten.

    The correct thing to do here, in the case where the extended resource is
    a sub-resource, is to update the content of parameters with the new/changed
    attributes.

    This change also removes a workaround that was made in
    the qos-bw-limit-direction extension, and which after the change in API
    extension code, is not needed anymore.

    Needed-By: I263e1ee6cf4e1a91be91a4a78f4a160f64d33cc6
    Change-Id: I4cb61481205c3689c41e62670cec113adb2a0362
    Closes-Bug: 1722842

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron 12.0.0.0b2 development milestone.