HI, I am becoming aware of this patch just now, and I would like to thank Dan for raising the issue. Bug 1069782 was initially filed for adding convenience client-side commands for retrieving router interfaces, and I believe it was not supposed to have any server side change.
In general we should avoid being 'trigger-happy' with changes on the tenant-facing API, even when the change appears totally reasonable and improves the API. Keeping that in mind, this particular change won't break backward compatibility, so it's not super-bad. However, it will break the default policy engine rules - or, to be more precise, it will circumvent the policy engine. Indeed it won't matter what is the policy for ports on external network, you'll end up showing some details about it. You might argue that this is something that needs to be addressed in the policy engine, and I would agree with you. Dan proposes two solution which I believe are dictated by a "safety first" approach. They both seem reasonable to me. The client-side option looks cleaner but less efficient, where the admin-only option would be more efficient, and still safe (although I might have some reservation if the admin-only constraint becomes hard coded). Salvatore On 1 November 2012 22:11, Dan Wendlandt <[email protected]> wrote: > Hi team, > > I have two concerns with the commit below, one general, one specific. > > First off, the general concern: this change changes our tenant-facing API, > but it seems that there was no real public discussion or notification of > this change. Our tenant-facing APIs, including extensions, must be stable, > consistent, and documented. I would argue that changes to the API should > be part of blueprints, which will trigger a discussion about API > consistency, documentation, etc. > > Second, the specific one: I want to explain the reasoning why this field > was not included in the original version of the L3 API, and why my current > thinking is that it probably should not be added. The model we were going > with for L3 routers is that the details of the gateway are abstracted from > the tenant. In particular, the port used to "hold" the IP allocation for > gateway and floating-ip ports are not visible to a common tenant and should > not be manipulated directly. They are only visible to an admin user. As a > result, I would argue that include the gw port info in the JSON that is > visible to all tenants doesn't really make sense. We could consider adding > it as an admin only attribute (similar to how the provider network > attributes are included in a network, but only for admins), though this > info is already available to admin users if they search for ports that have > the device_owner=<router-id> and device_owner=network:router_gateway . We > could even has the CLI do this as part of the router-show command, if the > user was an admin. > > I think we can have a technical discussions about the second issue, but my > most major concern is actually the first issue in that we need have a more > deliberate process when it comes to making changes to tenant-facing APIs. > > Dan > > p.s. if we end up deciding to make the change to the API, I would also > argue that the field be named something like gw_port_id, rather than > gw_port, as we try to maintain consistency that UUID fields end with _id. > This is where Salvatore and the API police come into play :P > > > ---------- Forwarded message ---------- > From: OpenStack Hudson <[email protected]> > Date: Thu, Nov 1, 2012 at 2:40 AM > Subject: [Bug 1069782] Re: quantum router-show should return interfaces too > To: [email protected] > > > Reviewed: https://review.openstack.org/15101 > Committed: > http://github.com/openstack/quantum/commit/c1c19b11792e8e220b11466051739ea5b4279a7a > Submitter: Jenkins > Branch: master > > commit c1c19b11792e8e220b11466051739ea5b4279a7a > Author: gongysh <[email protected]> > Date: Wed Oct 31 22:01:35 2012 +0800 > > Put gw_port into router dict result. > > Bug #1069782 > > We put gw_port into router dict result so that client can get more > information for the result router. > > Change-Id: I54cec8a71441a9370c7ba95767a92190bf1c9c21 > > > ** Changed in: quantum > Status: In Progress => Fix Committed > > -- > You received this bug notification because you are a member of Netstack > Core Developers, which is subscribed to quantum. > https://bugs.launchpad.net/bugs/1069782 > > Title: > quantum router-show should return interfaces too > > Status in Python client library for Quantum: > In Progress > Status in OpenStack Quantum (virtual network service): > Fix Committed > > Bug description: > interfaces and external_gateway_info are two arms of a router, so we > should be able to show interface information too. > > quantum router-show myrouter1 > +-----------------------+--------------------------------------+ > | Field | Value | > +-----------------------+--------------------------------------+ > | admin_state_up | True | > | external_gateway_info | | > | id | c508d501-8019-4e12-b332-fd19ac3be79e | > | name | myrouter1 | > | status | ACTIVE | > | tenant_id | b7445f221cda4f4a8ac7db6b218b1339 | > +-----------------------+--------------------------------------+ > > To manage notifications about this bug go to: > https://bugs.launchpad.net/python-quantumclient/+bug/1069782/+subscriptions > > > > -- > ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Dan Wendlandt > Nicira, Inc: www.nicira.com > twitter: danwendlandt > ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > -- > Mailing list: https://launchpad.net/~quantum-core > Post to : [email protected] > Unsubscribe : https://launchpad.net/~quantum-core > More help : https://help.launchpad.net/ListHelp > >
-- Mailing list: https://launchpad.net/~quantum-core Post to : [email protected] Unsubscribe : https://launchpad.net/~quantum-core More help : https://help.launchpad.net/ListHelp
