Skip to content

Adding capability for version negotiation in service endpoint URIs.#463

Merged
jamiehannaford merged 11 commits intorackspace:workingfrom
ycombinator:version-neg
Nov 21, 2014
Merged

Adding capability for version negotiation in service endpoint URIs.#463
jamiehannaford merged 11 commits intorackspace:workingfrom
ycombinator:version-neg

Conversation

@ycombinator
Copy link
Copy Markdown
Contributor

Certain services (e.g. OpenStack Neutron) do not publish a version as part of their endpoint URIs in the service catalog. Here is an example of the Neutron section of a OpenStack service catalog:

 {
  "endpoints": [
    {
      "adminURL": "http://23.253.159.142:9696/",
      "region": "RegionOne",
      "internalURL": "http://23.253.159.142:9696/",
      "id": "195e82869d6248cdaff43ea2b1e4e777",
      "publicURL": "http://23.253.159.142:9696/"
    }
  ],
  "endpoints_links": [],
  "type": "network",
  "name": "neutron"
}

Notice how the adminURL, internalURL, and publicURL do not contain a version at the end of their respective URIs. The expectation here is that the consumer of this response will issue a GET request to the specified URI for version negotiation. Here is an example response from making a GET request to http://23.253.159.142:9696/:

{
  "versions": [
    {
      "status": "CURRENT",
      "id": "v2.0",
      "links": [
        {
          "href": "http://23.253.159.142:9696/v2.0",
          "rel": "self"
        }
      ]
    }
  ]
}

This PR implements version negotiation. A couple of points to note:

  1. Since there is no indication in the service catalog as to which services require version negotiation vs. which ones do not, the SDK must speculatively make the additional GET request to the requested endpoint URI in case it demands version negotiation.
  2. As suggested in the sample version negotiation response JSON above, there might be multiple versions of the same service. This implementation chooses the first version that has "status": "CURRENT".

@gecampbell
Copy link
Copy Markdown
Contributor

LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$this is causing an error

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a static method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hahaha, rookie oops. Fixing.

@jamiehannaford
Copy link
Copy Markdown
Contributor

My concern with this PR is that we're already making additional requests where we don't have to. If Neutron is the only service that requires additional version determination, maybe we should implement a solution that affects this service only - since no other service seems to require it (but would be affected by the performance slow-down). I've done an initial check with a few Rackspace services, and they all fail to handle this request - which means it's a wasted request. Users seem to be very sensitive to these additional requests, so I just want to make sure we can avoid it where necessary.

Is there anything wrong with using a default version (append it to the base URL), and update the string when the service version gets updated? Or, for OpenStack, to enable users to pass in which version they want? Otherwise we'd be assuming that all users ever wanted was the CURRENT version - which might not necessarily be true.

@gecampbell
Copy link
Copy Markdown
Contributor

Neutron is the only service at this time that requires it; we can probably expect to do this more often in the future as multiple API versions are deployed (for compute, for example). The other option would be to transmit an Accept: header with the appropriate mime-type that specifies the version.

@gecampbell
Copy link
Copy Markdown
Contributor

For example,

Accept: application/vnd.openstack.compute.v2+son

See http://docs.openstack.org/api/openstack-compute/2/content/Versions-d1e1193.html

@jamiehannaford
Copy link
Copy Markdown
Contributor

Sure, but we're speculating now - I'd rather not jeopardise current performance for the sake of future possibilities, especially when we'll most likely be developing v2 next year (that will be able to handle multiple versions). IMHO one awkward service should not modify the underlying behaviour of 12 others.

Do any other OpenStack and Rackspace services use MIME-based versioning schemes?

@gecampbell
Copy link
Copy Markdown
Contributor

All of the OpenStack services except for Object Storage and Telemetry provide a versioning endpoint so, yes, they do.

@gecampbell
Copy link
Copy Markdown
Contributor

I think the issue is that most of the others only support one version, so that's what's in the service catalog. I'm not sure why the Neutron service doesn't do that, too.

@ycombinator
Copy link
Copy Markdown
Contributor Author

@gecampbell, @jamiehannaford, and @ycombinator had an out-of-PR discussion on this, and decided on the following:

  • Test the endpoint URI against a regular expression to determine if version negotiation is necessary. Rationale: This ensures continued high performance for those endpoints that already contain a version in their URI.
  • When negotiating the version, look for a specific version (e.g. v2.0) in the version negotiation API response. This specific version should be defined in the service class (e.g. OpenCloud\Network\Service). Rationale: This ensures that the SDK is asking for a version it knows it can support.
  • When negotiation the version, if the specific version is not found, fail by throwing an exception. Rationale: fail early.

Comment thread lib/OpenCloud/Networking/Service.php Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to be "network"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is now :) Thanks for catching. Fixing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be Url::factory($link->href);?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Fixing.

@gecampbell
Copy link
Copy Markdown
Contributor

shipit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants