You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by jasdeep-hundal <no...@github.com> on 2014/03/12 07:19:58 UTC
[jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Fixes: https://issues.apache.org/jira/browse/JCLOUDS-494
This change adds a new endpoint parser that takes a base endpoint that serves a JSON formatted list of API versions and matching endpoints and returns the endpoint that matches the requested version. This solution made sense as this change certainly does not belong in the ImageAPI spec as those requests in fact depend on the right endpoint from this, and changes elsewhere would affect projects that aren't Openstack related.
This PR for now is PoC proposal that has been tested on a local Glance installation, still needs lots of work to be ready. It was also built w/ skipTests enabled, so tests are probably broken in some way in this branch. I think I particularly need answers to these questions:
- I'd like to inject apiVersion here, but trying to copy from LocationIdToURIFromAccessForTypeAndVersion in jclouds-core appeared to cause some sort of conflict and did not work.
- I'd also like to use an HttpCommandExecutor (esp. as dev installs of Glance don't use real certificates and I'd like Jclouds to handle ignoring cert verification for me.) I was able to successfully inject one, but the BaseHttpExecutor invoke method returns errors on an HTTP status code of 300 (which is what the GET on the Glance base URL returns).
- What's the right/jclouds way to handle the JSON response from Glance that contains the version list?
One more note: The forceSecure attribute was a hack because Glance was returning http endpoints in the version list when the actual endpoints were behind SSL. Not sure how necessary it will be in a final version.
You can merge this Pull Request by running:
git pull https://github.com/jasdeep-hundal/jclouds-labs-openstack glance_negotiation
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds-labs-openstack/pull/82
-- Commit Summary --
* JCLOUDS-494: Change EndpointParam parser to negotiate version
-- File Changes --
A openstack-glance/src/main/java/org/jclouds/location/functions/ZoneToEndpointNegotiateVersion.java (98)
M openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/GlanceApi.java (6)
-- Patch Links --
https://github.com/jclouds/jclouds-labs-openstack/pull/82.patch
https://github.com/jclouds/jclouds-labs-openstack/pull/82.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Success!
```
Starting test testCreateUpdateAndDeleteImage(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest)
[TestNG] Test testCreateUpdateAndDeleteImage(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest) succeeded: 1187ms
Test suite progress: tests succeeded: 1, failed: 0, skipped: 0.
Starting test testList(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest)
[TestNG] Test testList(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest) succeeded: 63ms
Test suite progress: tests succeeded: 2, failed: 0, skipped: 0.
Starting test testListInDetail(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest)
[TestNG] Test testListInDetail(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest) succeeded: 132ms
Test suite progress: tests succeeded: 3, failed: 0, skipped: 0.
Starting test testReserveUploadAndDeleteImage(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest)
[TestNG] Test testReserveUploadAndDeleteImage(org.jclouds.openstack.glance.v1_0.features.ImageApiLiveTest) succeeded: 448ms
Test suite progress: tests succeeded: 4, failed: 0, skipped: 0.
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.194 sec
Results :
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 12.744s
[INFO] Finished at: Sat Mar 29 12:49:21 PDT 2014
[INFO] Final Memory: 33M/439M
[INFO] ------------------------------------------------------------------------
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39007016
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #192](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/192/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39248959
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #995](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/995/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39282157
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #177](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/177/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37379338
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
The stack trace is:
```
org.jclouds.http.HttpResponseException: command: GET https://proxy.env11.nebula.com:9292/ HTTP/1.1 failed with response: HTTP/1.1 300 Multiple Choices; content: [{"versions": [{"status": "CURRENT", "id": "v2.1", "links": [{"href": "http://proxy.env11.nebula.com:9292/v2/", "rel": "self"}]}, {"status": "SUPPORTED", "id": "v2.0", "links": [{"href": "http://proxy.env11.nebula.com:9292/v2/", "rel": "self"}]}, {"status": "CURRENT", "id": "v1.1", "links": [{"href": "http://proxy.env11.nebula.com:9292/v1/", "rel": "self"}]}, {"status": "SUPPORTED", "id": "v1.0", "links": [{"href": "http://proxy.env11.nebula.com:9292/v1/", "rel": "self"}]}]}]
at org.jclouds.openstack.glance.v1_0.handlers.GlanceErrorHandler.handleError(GlanceErrorHandler.java:44)
at org.jclouds.http.handlers.DelegatingErrorHandler.handleError(DelegatingErrorHandler.java:65)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.shouldContinue(BaseHttpCommandExecutorService.java:180)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:150)
at org.jclouds.rest.internal.InvokeHttpMethod.invoke(InvokeHttpMethod.java:93)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:76)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:47)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.handle(DelegatesToInvocationFunction.java:156)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.invoke(DelegatesToInvocationFunction.java:123)
at com.sun.proxy.$Proxy64.get(Unknown Source)
at org.jclouds.location.functions.ZoneToEndpointNegotiateVersion.apply(ZoneToEndpointNegotiateVersion.java:89)
at org.jclouds.location.functions.ZoneToEndpointNegotiateVersion.apply(ZoneToEndpointNegotiateVersion.java:47)
```
Makes sense since Glance doesn't return a Location header here (makes the client figure out the endpoint from the returned JSON). Here are the returned headers of this request:
```
HTTP/1.1 300 Multiple Choices
Content-Type: application/json; charset=UTF-8
Content-Length: 478
Date: Thu, 13 Mar 2014 15:15:41 GMT
Strict-Transport-Security: max-age=31580000; includeSubDomains
X-Frame-Options: deny
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37546349
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> @@ -0,0 +1,44 @@
> +{
[minor] I think we normally use smaller indents for JSON files? I may be wrong, though.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724813
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
When the response code is >= 300, jclouds [checks if it should be retried or failed](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L175-L183). To do that, delegates in the [retry handlers](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/HttpRetryHandler.java) and [error handlers](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/HttpErrorHandler.java). By default, jclouds uses [this redirection retry handler](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/handlers/RedirectionRetryHandler.java) to check if the request should be retried.
If the response contains a `Location` header, then it will instruct to retry the request to that location; otherwise, it will return `false` and the request won't be retried. When the response can't be retried, it is passed to the configured error handler, and [this](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/handlers/CloseContentAndSetExceptionErrorHandler.java) is the one used by default to process redirection errors. That's why you are seeing the exception: there is a 300 response without a location header, and jclouds doesn't know where to redirect.
There are two things that I can think about you can do to fix this:
**Configure a custom retry error handler logic**
You can modify the [GalnceErrorHandler](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/handlers/GlanceErrorHandler.java) so it does not set an exception in the command if the response is a 300 and the request is the one to ask for the versioned endpoints. This way, the execution flow will continue without throwing an exception and your call will return the response normally. (The Glance error handler is configured [here](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/config/GlanceHttpApiModule.java#L87)).
**Add a method in the GlanceApi to request the versioned endpoints**
You could add a custom method to the Glance Api to request the versioned endpoints, and configure it with a custom `@Fallback`. Fallbacks instruct jclouds what to do upon errors, and you could write one that just returns normally if the request failed because of that 300 response. The [ImageApi](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/features/ImageApi.java) uses some built-in fallbacks, but you can write your own one and use it in your method.
HTH!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37723590
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #925](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/925/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37886473
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #197](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/197/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39341990
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> +
> + public void testNonExistentVersion() throws Exception {
> + // The referenced resource only an endpoint for v999.999 of the GlanceApi
> + HttpResponse localVersionNegotiationResponse = HttpResponse.builder().statusCode(300).message("HTTP/1.1 300 Multiple Choices").payload(
> + payloadFromResourceWithContentType("/glanceVersionResponseVersionUnavailable.json", "application/json")).build();
> +
> + GlanceApi apiWhenExist = requestsSendResponses(keystoneAuthWithUsernameAndPassword,
> + responseWithKeystoneAccess, versionNegotiationRequest, localVersionNegotiationResponse);
> +
> + try {
> + apiWhenExist.getImageApiForZone("az-1.region-a.geo-1").list();
> + } catch (UncheckedExecutionException e) {
> + if (e.getCause().getClass() == HttpException.class)
> + return;
> + }
> + fail("Did not throw expected HttpException");
That's what I tried initially, but the exception gets wrapped in an UncheckedExecutionException and it seemed unclear to use that as the expected exception in the Test annotation.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11096551
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #998](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/998/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39341818
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
Committed to [master](https://git-wip-us.apache.org/repos/asf?p=jclouds-labs-openstack.git;a=commit;h=7c1b681b6699025aa56e6b64c7da9c0136349b00)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39352640
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
@jasdeep-hundal Thanks! I'll have a look at it this weekend. Meanwhile, could you run the live tests against your devstack instance to make sure everything is working as expected?
```bash
mvm clean integration-test -Plive \
-Dtest.openstack-glance.endpoint=your-endpoint \
-Dtest.openstack-glance.identity=your-identity \
-Dtest.openstack-glance.credential=your-credential
```
Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38989537
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> Thinking about it some more, would it be possible/cleaner to use a retryHandler instead of an
> errorHandler to deal with the HttpClient behavior on HTTP responses with a 300 status code?
Well, the "cleanest" thing I can think of would be to hook in where the success/failure of the call is determined, and decide that 300 is actually a success response code for this particular call. But not sure how we would go about that.
> On the note of an explicit use of HttpClient, I don't think the Image/GlanceApi is where this should go
Agreed. But what is the OpenStack API that we're actually talking to here? Some kind of admin/config API?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38003773
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #922](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/922/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37751140
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -0,0 +1,44 @@
> +{
I think it was actually 4 in the keystoneAuthResponse.json, but I'm not consistent with that either here...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725757
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> @@ -46,6 +47,11 @@ public void handleError(HttpCommand command, HttpResponse response) {
> message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(),
> response.getStatusLine());
> switch (response.getStatusCode()) {
> + // This is exclusively to not throw exceptions on Glance version negotiation
> + case 300:
> + if (command.getCurrentRequest().getFirstHeaderOrNull(ZoneToEndpointNegotiateVersion.VERSION_NEGOTIATION_HEADER) != null)
300 is just what Glance returns as the status code when it offers up possible versions. Does seem a bit awkward, but it's what we're dealing with.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725824
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
I think we're missing some tests for this new functionality: what should happen if the API version is/is not found, validating the http -> https switch etc.
I'm not sure, though, whether the explicit use of HttpClient, as opposed to adding the version negotiation call as an explicit API call elsewhere and then invoking that, is what we want?
@nacx: thoughts on that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37986049
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #939](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/939/) FAILURE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38333755
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #195](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/195/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39282295
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + break;
> + }
> + }
> + } catch (Exception ex) {
> + throw Throwables.propagate(ex);
> + }
> + if (versionEndpointUri == null)
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + return versionEndpointUri;
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from != null && from instanceof String, "you must specify a zone, as a String argument");
We don't need the `!= null` check, null is not `instanceof String`
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724109
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@nacx @demobox @everett-toews : If you folks have time to give this a (hopefully) final review, would love to get this off my plate.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38977210
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@demobox @everett-toews @nacx : All comments addressed, thanks again for taking the time to review this. I definitely want to get this right, so keep the comments/concerns coming.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39248122
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #942](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/942/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38371981
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if (!apiVersionString.startsWith("v")) {
> + this.apiVersion = "v" + apiVersionString;
> + } else {
> + this.apiVersion = apiVersionString;
> + }
Name the constructor param `apiVersion` and do something like:
```
this.apiVersion = apiVersion;
if (!apiVersion.startsWith("v")) {
this.apiVersion = "v" + apiVersion;
}
```
which makes it a little clearer that this is "exceptional case handling"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095387
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if (!apiVersionString.startsWith("v")) {
> + this.apiVersion = "v" + apiVersionString;
> + } else {
> + this.apiVersion = apiVersionString;
> + }
The issue with that is the apiVersion gets used in the CacheLoader, so it needs to be be final. Since the parameter passed in may need to be modified, I couldn't give it the same name since it would hide the apiVersion field.
Suggestions to work around this and make it cleaner?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11096564
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + if (!baseEndpointPathParts.isEmpty() &&
> + baseEndpointPathParts.get(baseEndpointPathParts.size() - 1).matches("v[0-9]+(\\.[0-9])?[0-9]*")){
> + baseEndpointUri = new URI(baseEndpointUri.getScheme(), baseEndpointUri.getUserInfo(),
> + baseEndpointUri.getHost(), baseEndpointUri.getPort(),
> + Joiner.on('/').join(baseEndpointPathParts.subList(0, baseEndpointPathParts.size() - 1)) + "/",
> + baseEndpointUri.getQuery(), baseEndpointUri.getFragment());
> + }
> +
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
Haven't found a proper spec, but from talking to Mark I'm very confident this is the right thing and have yet to see an endpoint that returns multiple endpoints for a version.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11175754
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #179](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/179/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37751167
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> Is the right thing to do to make another PR to change the Glance endpoint returned?
Hmmm I think so. The mock json responses configured for the tests should be close to the real ones, so it would be great to have a PR to have those responses fixed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37796014
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Took a look at RedirectionRetryHandler and it is returning false the first time if the Location header is not present. This is the behavior I wanted. Looking at the code in <a href="https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/internal/BaseHttpCommandExecutorService.java#L175-L183">BaseHttpCommandExecutorService</a>, retry handlers only determine if another retry should be done and modify the HttpCommand to direct to a new endpoint if necessary. Since we don't want to redirect anywhere, the current modification to GlanceErrorHandler is sufficient and correct.
I've added the bits to strip off the version in the endpoint if present in order to perform the proper negotiation. Still need to write additional tests, which are:
- Test the GlanceRetryHandler
- Test the version negotiation if the Keystone catalog returns a versioned endpoint.
- Making sure the returned scheme for the URI matches the queried endpoint.
- Making sure an error is thrown if the required version does not exist.
If I missed a good test, please let me know.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38334182
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
The stack trace is:
```
org.jclouds.http.HttpResponseException: command: GET https://proxy:9292/ HTTP/1.1 failed with response: HTTP/1.1 300 Multiple Choices; content: [{"versions": [{"status": "CURRENT", "id": "v2.1", "links": [{"href": "https://proxy:9292/v2/", "rel": "self"}]}, {"status": "SUPPORTED", "id": "v2.0", "links": [{"href": "https://proxy:9292/v2/", "rel": "self"}]}, {"status": "CURRENT", "id": "v1.1", "links": [{"href": "https://proxy:9292/v1/", "rel": "self"}]}, {"status": "SUPPORTED", "id": "v1.0", "links": [{"href": "https://proxy:9292/v1/", "rel": "self"}]}]}]
at org.jclouds.openstack.glance.v1_0.handlers.GlanceErrorHandler.handleError(GlanceErrorHandler.java:44)
at org.jclouds.http.handlers.DelegatingErrorHandler.handleError(DelegatingErrorHandler.java:65)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.shouldContinue(BaseHttpCommandExecutorService.java:180)
at org.jclouds.http.internal.BaseHttpCommandExecutorService.invoke(BaseHttpCommandExecutorService.java:150)
at org.jclouds.rest.internal.InvokeHttpMethod.invoke(InvokeHttpMethod.java:93)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:76)
at org.jclouds.rest.internal.InvokeHttpMethod.apply(InvokeHttpMethod.java:47)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.handle(DelegatesToInvocationFunction.java:156)
at org.jclouds.rest.internal.DelegatesToInvocationFunction.invoke(DelegatesToInvocationFunction.java:123)
at com.sun.proxy.$Proxy64.get(Unknown Source)
at org.jclouds.location.functions.ZoneToEndpointNegotiateVersion.apply(ZoneToEndpointNegotiateVersion.java:89)
at org.jclouds.location.functions.ZoneToEndpointNegotiateVersion.apply(ZoneToEndpointNegotiateVersion.java:47)
```
Makes sense since Glance does not return a `Location` header for this request. It expects the client to figure out the endpoint solely from the JSON response. Here's the set of headers that it does return:
```
HTTP/1.1 300 Multiple Choices
Content-Type: application/json; charset=UTF-8
Content-Length: 478
Date: Thu, 13 Mar 2014 15:15:41 GMT
Strict-Transport-Security: max-age=31580000; includeSubDomains
X-Frame-Options: deny
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37546829
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
> + if(apiVersion.equals(version.id)) {
> + String newURIString = version.links.get(0).href;
> + if(newURIString.startsWith("http:") && baseEndpointUri.toString().startsWith("https:"))
> + newURIString = "https" + newURIString.substring(4);
> + versionEndpointUri = new URI(newURIString);
> + break;
> + }
> + }
> + } catch (Exception ex) {
> + throw Throwables.propagate(ex);
> + }
> + if (versionEndpointUri == null)
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + return versionEndpointUri;
Why can't we just return the first one we find in the loop?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724084
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.base.Throwables;
> +import com.google.common.cache.CacheBuilder;
> +import com.google.common.cache.CacheLoader;
> +import com.google.common.cache.LoadingCache;
> +
> +/**
> + *
> + * @author Jasdeep Hundal
> + */
> +@Singleton
> +public class ZoneToEndpointNegotiateVersion implements Function<Object, URI> {
> +
> + public static final String VERSION_NEGOTIATION_HEADER = "Is-Version-Negotiation-Request";
> +
> + private static class VersionsJsonResponse{
Nope, this was just something that works. Didn't know the JClouds way of doing things here, open to suggestions.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724655
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.http.HttpResponse;
> +import org.jclouds.openstack.glance.v1_0.GlanceApi;
> +import org.jclouds.openstack.glance.v1_0.internal.BaseGlanceApiExpectTest;
> +import org.jclouds.openstack.glance.v1_0.parse.ParseImagesTest;
> +import org.testng.annotations.Test;
> +
> +import com.google.common.util.concurrent.UncheckedExecutionException;
> +
> +/**
> + * @author Jasdeep Hundal
> + */
> +@Test(groups = "unit", testName = "GlanceVersionNegotiationExpectTest")
> +public class GlanceVersionNegotiationExpectTest extends BaseGlanceApiExpectTest {
> +
> + public void testSchemeMismatch() throws Exception {
Thanks for the update, @jasdeep-hundal!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11156769
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Hmm, test failures that I didn't see locally, will look into that soon.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39250229
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.base.Throwables;
> +import com.google.common.cache.CacheBuilder;
> +import com.google.common.cache.CacheLoader;
> +import com.google.common.cache.LoadingCache;
> +
> +/**
> + *
> + * @author Jasdeep Hundal
> + */
> +@Singleton
> +public class ZoneToEndpointNegotiateVersion implements Function<Object, URI> {
> +
> + public static final String VERSION_NEGOTIATION_HEADER = "Is-Version-Negotiation-Request";
> +
> + private static class VersionsJsonResponse{
I don't think we keep domain objects such as this one in functions much. Is this a pattern you've copied from elsewhere in the jclouds code?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10723873
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
JClouds PR for fixing Keystone catalog received in tests: https://github.com/jclouds/jclouds/pull/319
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37881237
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@nacx: Thanks for doing that. Should I squash/rebase this PR for merging as well?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37957831
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
OK, after testing it seems that the version number in fact does not start w/ a 'v' typically
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725390
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #193](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/193/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39253920
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Pushed a few of the indicated fixes. As for the caching, I now agree that it's certainly useful, but still a bit confused as where the right place for it is.
Started a bit of work on the testing, but would like one more pointer before continuing. The jclouds tests seem to generate a file at apis/openstack-keystone/target/test-classes/keystoneAuthResponse.json which contains the Keystone response used for the test. This returns the Glance versioned endpoint as part of the catalog, which isn't quite correct. Is the right thing to do to make another PR to change the Glance endpoint returned?
Again, thank you for your help and your patience.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37785201
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
> + return new URI(baseEndpointUri.getScheme(), versionedEndpointUri.getUserInfo(),
> + versionedEndpointUri.getHost(), versionedEndpointUri.getPort(),
> + versionedEndpointUri.getPath(), versionedEndpointUri.getQuery(),
> + versionedEndpointUri.getFragment());
> + }
> + }
> + } catch (URISyntaxException ex) {
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
Thanks for the pointer @nacx . @demobox : From that list I think UnsupportedOperationException makes the most sense, any opinion on that?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11175623
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
> + return new URI(baseEndpointUri.getScheme(), versionedEndpointUri.getUserInfo(),
> + versionedEndpointUri.getHost(), versionedEndpointUri.getPort(),
> + versionedEndpointUri.getPath(), versionedEndpointUri.getQuery(),
> + versionedEndpointUri.getFragment());
> + }
> + }
> + } catch (URISyntaxException ex) {
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new UnsupportedOperationException("Glance endpoint does not support API version: " + apiVersion);
Either this or `IllegalArgumentException`, but UOE is probably better, indeed.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11187283
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Everett Toews <no...@github.com>.
I ran the live tests with 100% success.
mvn clean integration-test -Plive -Dtest.openstack-glance.endpoint=http://162.242.246.90:5000/v2.0/ -Dtest.openstack-glance.identity=admin:admin -Dtest.openstack-glance.credential=devstack
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39140274
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
> + apiVersionString = "v" + apiVersionString;
> + this.apiVersion = apiVersionString;
> + this.endpointCache = CacheBuilder.newBuilder()
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI baseEndpointUri) {
> + URI versionEndpointUri = null;
> + try {
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
Formatting: "for (V...s)"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10723963
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.base.Throwables;
> +import com.google.common.cache.CacheBuilder;
> +import com.google.common.cache.CacheLoader;
> +import com.google.common.cache.LoadingCache;
> +
> +/**
> + *
> + * @author Jasdeep Hundal
> + */
> +@Singleton
> +public class ZoneToEndpointNegotiateVersion implements Function<Object, URI> {
> +
> + public static final String VERSION_NEGOTIATION_HEADER = "Is-Version-Negotiation-Request";
> +
> + private static class VersionsJsonResponse{
I'm personally ok with leaving this here. Given that we won't expose version negotiation and will do it transparently, I think it is ok to leave the domain object and the client used to perform that request hidden in this class.
This pattern is also used with the paged iterables, for example. In Openstack, Abiquo and others, we use internal classes to deserialize the pagination requests (see the [ParseImages](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/functions/internal/ParseImages.java#L55-L62) class in this project for an example) and keep the pagination internals hidden to the user.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11100358
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from instanceof String, "you must specify a zone, as a String argument");
> + Map<String, Supplier<URI>> zoneToEndpoint = zoneToEndpointSupplier.get();
> + checkState(!zoneToEndpoint.isEmpty(), "no zone name to endpoint mappings configured!");
> + checkArgument(zoneToEndpoint.containsKey(from),
> + "requested location %s, which is not in the configured locations: %s", from, zoneToEndpoint);
Do we need both of these checks? Yes, one throws an ISE and the other an IAE, but would e.g. the IAE be enough?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095379
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> @@ -46,6 +47,11 @@ public void handleError(HttpCommand command, HttpResponse response) {
> message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(),
> response.getStatusLine());
> switch (response.getStatusCode()) {
> + // This is exclusively to not throw exceptions on Glance version negotiation
> + case 300:
> + if (command.getCurrentRequest().getFirstHeaderOrNull(ZoneToEndpointNegotiateVersion.VERSION_NEGOTIATION_HEADER) != null)
Static import `ZoneToEndpointNegotiateVersion.VERSION_NEGOTIATION_HEADER`? But what causes the 300 response from Glance in the firest place?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724608
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + && versionRegex.matcher(baseEndpointPathParts.get(baseEndpointPathParts.size() - 1)).matches()) {
> + // Constructs a base URI Glance endpoint by stripping the version from the received URI
> + baseEndpointUri = new URI(baseEndpointUri.getScheme(), baseEndpointUri.getUserInfo(),
> + baseEndpointUri.getHost(), baseEndpointUri.getPort(),
> + Joiner.on('/').join(baseEndpointPathParts.subList(0, baseEndpointPathParts.size() - 1)) + "/",
> + baseEndpointUri.getQuery(), baseEndpointUri.getFragment());
> + }
> +
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
If we are pretty certain that only one element is returned, use [`Iterable.getOnlyElement`](http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/collect/Iterables.html#getOnlyElement(java.lang.Iterable\))? Otherwise, at least add a short comment to indicate that we really only expect one element here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11187328
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
Then some more elaborated configuration will be required. Could you paste the stack trace, to see the exact exception you're getting? jclouds automatically retries 300 requests to the host provided in the `Location` header. Just share the stack trace so we can properly configure jclouds to do the right thing in this case.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37512534
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + if(!apiVersionString.startsWith("v"))
> + apiVersionString = "v" + apiVersionString;
> + this.apiVersion = apiVersionString;
> + this.endpointCache = CacheBuilder.newBuilder()
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI baseEndpointUri) {
> + URI versionEndpointUri = null;
> + try {
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
> + if(apiVersion.equals(version.id)) {
Formatting: space after "if"
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10723978
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #991](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/991/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39256627
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + }
> + } catch (Exception ex) {
> + throw Throwables.propagate(ex);
> + }
> + if (versionEndpointUri == null)
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + return versionEndpointUri;
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from != null && from instanceof String, "you must specify a zone, as a String argument");
> + Map<String, Supplier<URI>> zoneToEndpoint = zoneToEndpointSupplier.get();
> + checkState(zoneToEndpoint.size() > 0, "no zone name to endpoint mappings configured!");
`!zoneToEndpoint.isEmpty()`?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724130
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Yup, it will just need an extension because the endpoint obtained from Keystone may actually already link to the version endpoint (HPCloud does this). So I will just strip off the version (if it exists) in the returned endpoint from Keystone to obtain the base endpoint and then perform version negotiation from there.
As for moving it to common openstack modules, this is Glance-only (at least right now), so I think it's best to leave it for now.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38097286
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
Probably was being too cautious here, didn't know if folks would ever specify a version with just the number.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724727
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
Could you squash and rebase the commits, @jasdeep-hundal? Then we can finally merge this change. Thanks!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39323732
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.base.Throwables;
> +import com.google.common.cache.CacheBuilder;
> +import com.google.common.cache.CacheLoader;
> +import com.google.common.cache.LoadingCache;
> +
> +/**
> + *
> + * @author Jasdeep Hundal
> + */
> +@Singleton
> +public class ZoneToEndpointNegotiateVersion implements Function<Object, URI> {
> +
> + public static final String VERSION_NEGOTIATION_HEADER = "Is-Version-Negotiation-Request";
> +
> + private static class VersionsJsonResponse{
@nacx: Ping...thought on this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095380
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if (!apiVersionString.startsWith("v")) {
> + this.apiVersion = "v" + apiVersionString;
> + } else {
> + this.apiVersion = apiVersionString;
> + }
> so it needs to be be final
Of course - doh! In that case, we could do something like this:
```
public ZoneToEndpointNegotiateVersion(..., @ApiVersion String apiVersion, ...) {
String versionPrefix = apiVersion.startsWith("v") ? "" : "v"; // or write using if...else, if preferred
this.apiVersion = versionPrefix + apiVersion;
}
```
We shouldn't have to worry about hiding if we use `this.apiVersion` to refer to the field elsewhere in the constructor. I agree, though, that that could be confusing. In which case, how about calling the parameter "apiVersion" and the field something like "apiVersionWithPrefix" or so, or call the parameter "rawApiVersion" and the field "apiVersion", to indicate that there's actually something different about the two?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11098697
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public void testNonExistentVersion() throws Exception {
> + // The referenced resource only an endpoint for v999.999 of the GlanceApi
> + HttpResponse localVersionNegotiationResponse = HttpResponse.builder().statusCode(300).message("HTTP/1.1 300 Multiple Choices").payload(
> + payloadFromResourceWithContentType("/glanceVersionResponseVersionUnavailable.json", "application/json")).build();
> +
> + GlanceApi apiWhenExist = requestsSendResponses(keystoneAuthWithUsernameAndPassword,
> + responseWithKeystoneAccess, versionNegotiationRequest, localVersionNegotiationResponse);
> +
> + try {
> + apiWhenExist.getImageApiForZone("az-1.region-a.geo-1").list();
> + } catch (UncheckedExecutionException e) {
> + if (e.getCause().getClass() == HttpException.class)
> + return;
> + }
> + fail("Did not throw expected HttpException");
Ah, OK, thanks for explaining. In that case, how about using the `@Test(expected = ...)` annotation in combination with:
```
try {
...
} catch (Unche... e) {
// extract the actual, expected exception from the UncheckedEE wrapper
throw e.getCause();
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11098646
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #184](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/184/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38372033
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Tests added, I think this is ready for a final review.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38372308
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
LGTM too. Thanks @jasdeep-hundal! It's good to see this kind of PRs that also take care of adding more tests!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39295055
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
> + return new URI(baseEndpointUri.getScheme(), versionedEndpointUri.getUserInfo(),
> + versionedEndpointUri.getHost(), versionedEndpointUri.getPort(),
> + versionedEndpointUri.getPath(), versionedEndpointUri.getQuery(),
> + versionedEndpointUri.getFragment());
> + }
> + }
> + } catch (URISyntaxException ex) {
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
Jclouds has a restricted set of [propagatable exceptions](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/util/Throwables2.java#L131-L140). Could you change it to one from that list? Otherwise the exception will get encapsulated.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11100300
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #988](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/988/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39249047
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
> OK, after testing it seems that the version number in fact does not start w/ a 'v' typically. (Tests quickly failed on
> that, their just using '1.0'
Ah, interesting. Any idea why the jclouds code was using a hard-coded `v1.0`? Is that supported in some kind of compatibility mode? Do we even need the `v` at all, then?
If we do need to keep this, we should at least add a test for it. And I think you can call the parameter `apiVersion` and simply do something like:
```
if (!apiVersion.startsWith("v")) {
this.apiVersion = "v" + apiVersion;
} else {
this.apiVersion = apiVersion;
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725682
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> + newURIString = "https" + newURIString.substring(4);
> + uri = new URI(newURIString);
> + break;
> + }
> + }
> + } catch (Exception ex) {
> + throw Throwables.propagate(ex);
> + }
> + return uri;
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(@Nullable Object from) {
> + checkArgument(from != null && from instanceof String, "you must specify a zone, as a String argument");
If `from` can't be null, don't declare it nullable.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10639104
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
+1 - looks good to me, pending a squash'n'rebase. Thanks, @jasdeep-hundal!
@everett-toews @nacx: Any further questions..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39293321
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #194](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/194/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39256561
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@nacx Thanks for the pointers!
With your suggestions I was able to inject what was needed, but the issues is that the HttpClient I received (presumably based off something that is based off the BaseHttpCommandExecutorService) tosses exceptions on receiving a 300 as the HTTP response status code, which is unfortunately exactly the code we're expecting from the base Glance endpoint. Any suggestions of a different way of doing things?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37451239
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #178](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/178/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37748681
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> */
> @Test(groups = "unit", testName = "GlanceErrorHandlerTest")
> public class GlanceErrorHandlerTest {
>
> + @Test
> + public void test300UnintendedVersionNegotiation() {
> + assertCodeMakes("GET", URI
> + .create("https://glance.jclouds.org:9292/"), 300, "Multiple Choices",
> + "", HttpResponseException.class);
[minor] Odd line breaks here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095411
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@demobox done! Thanks for the help!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39340891
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + break;
> + }
> + }
> + } catch (Exception ex) {
> + throw Throwables.propagate(ex);
> + }
> + if (versionEndpointUri == null)
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + return versionEndpointUri;
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from != null && from instanceof String, "you must specify a zone, as a String argument");
This and the next bit were copied from the ZoneToEndpoint class in jclouds core. (Just a note in case you want to make the fix there as well).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725241
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from instanceof String, "you must specify a zone, as a String argument");
> + Map<String, Supplier<URI>> zoneToEndpoint = zoneToEndpointSupplier.get();
> + checkState(!zoneToEndpoint.isEmpty(), "no zone name to endpoint mappings configured!");
> + checkArgument(zoneToEndpoint.containsKey(from),
> + "requested location %s, which is not in the configured locations: %s", from, zoneToEndpoint);
> This was copied from ZoneToEndpoint in JClouds core
Ah, OK, then let's keep as-is. Thanks for explaining!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11156760
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + && versionRegex.matcher(baseEndpointPathParts.get(baseEndpointPathParts.size() - 1)).matches()) {
> + // Constructs a base URI Glance endpoint by stripping the version from the received URI
> + baseEndpointUri = new URI(baseEndpointUri.getScheme(), baseEndpointUri.getUserInfo(),
> + baseEndpointUri.getHost(), baseEndpointUri.getPort(),
> + Joiner.on('/').join(baseEndpointPathParts.subList(0, baseEndpointPathParts.size() - 1)) + "/",
> + baseEndpointUri.getQuery(), baseEndpointUri.getFragment());
> + }
> +
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
Did both :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11190337
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #927](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/927/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37992059
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #181](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/181/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37886627
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #923](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/923/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37785256
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> HttpResponse response = HttpResponse.builder().statusCode(statusCode).message(message).payload(content).build();
> response.getPayload().getContentMetadata().setContentType(contentType);
>
> expect(command.getCurrentRequest()).andReturn(request).atLeastOnce();
> - command.setException(classEq(expected));
> + if (expected != null)
> + command.setException(classEq(expected));
[minor] Use
```
if (...) {
...
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095418
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + try {
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
> + if(apiVersion.equals(version.id)) {
> + String newURIString = version.links.get(0).href;
> + if(newURIString.startsWith("http:") && baseEndpointUri.toString().startsWith("https:"))
> + newURIString = "https" + newURIString.substring(4);
> + versionEndpointUri = new URI(newURIString);
> + break;
> + }
> + }
> + } catch (Exception ex) {
What Exceptions are we catching here? Why do we need this catch block, and can we catch a narrower exception?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724045
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
> + return new URI(baseEndpointUri.getScheme(), versionedEndpointUri.getUserInfo(),
> + versionedEndpointUri.getHost(), versionedEndpointUri.getPort(),
> + versionedEndpointUri.getPath(), versionedEndpointUri.getQuery(),
> + versionedEndpointUri.getFragment());
> + }
> + }
> + } catch (URISyntaxException ex) {
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
Is `HttpException` the best choice here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095394
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
Then I'd say "let's be tough", and simply add a `checkArgument` the requires an initial "v" and an appropriate doc comment (no need for a test, I think).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725056
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #182](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/182/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37992211
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #921](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/921/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37748677
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@everett-toews : This running on a stock Devstack (just pulled from master today) inside an Ubuntu Precise instance running in VirtualBox. The test runs on OS X, so there was a minimal amount of host configuration needed to make it work: https://gist.github.com/jasdeep-hundal/852739ae465398587144
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38643056
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if (!apiVersionString.startsWith("v")) {
> + this.apiVersion = "v" + apiVersionString;
> + } else {
> + this.apiVersion = apiVersionString;
> + }
> + this.endpointCache = CacheBuilder.newBuilder()
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI baseEndpointUri) {
> + try {
> + List<String> baseEndpointPathParts = Splitter.on('/').omitEmptyStrings().splitToList(baseEndpointUri.getPath());
> + if (!baseEndpointPathParts.isEmpty() &&
> + baseEndpointPathParts.get(baseEndpointPathParts.size() - 1).matches("v[0-9]+(\\.[0-9])?[0-9]*")){
Either break _before_ `&&` or break at `.matches`? Also, since this will be called a few times, extract the regex out into a constant `Pattern` and use a Matcher.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095389
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> jclouds » jclouds-labs-openstack #939 FAILURE
Hm.GitHub _still_ [timing out](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/939/console)? Wonder if BuildHive has a problem?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38337031
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
Thanks for the contribution @jasdeep-hundal!
>I'd like to inject apiVersion here, but trying to copy from LocationIdToURIFromAccessForTypeAndVersion in jclouds-core appeared to cause some sort of conflict and did not work.
You can add the following parameter to the constructor. That will inject the api version defined when creating the context, or the default one defined in the api metadata.
```java
..., @ApiVersion String apiVersion)
```
>I'd also like to use an HttpCommandExecutor (esp. as dev installs of Glance don't use real certificates and I'd like Jclouds to handle ignoring cert verification for me.) I was able to successfully inject one, but the BaseHttpExecutor invoke method returns errors on an HTTP status code of 300 (which is what the GET on the Glance base URL returns).
You can use the jclouds generic HTTP client to perform that request. That way all the command execution, retry logic, etc will be automatically executed. To so, you can also inject the `org.jclouds.rest.HttpClient` and use it to perform the request.
>What's the right/jclouds way to handle the JSON response from Glance that contains the version list?
You can use the jclouds `Json` utility. Just inject the `Json` class, and you should be able to execute the request and parse the results like this:
```java
InputStream response = httpClient.get(uri);
VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
```
Thinking out loud, could the results of these requests be cached? I mean, if the `zone -> version endpoints` are always the same, perhaps we could cache the results of the request, to avoid performing the same one many times. Anyway, I think the way to go should be to improve the code to fix the points you commented, add the proper tests (I can assist with that, and feel free to join the IRC channel to ask too), and once everything is working as expected we can think about introducing the cache improvement.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37384188
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Tests fixed after realizing they were running on top of https://github.com/jclouds/jclouds-labs-openstack/pull/84
Checkstyle violations also cleared out.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39255824
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
@demobox : Thanks for the eyes, pushed the minor fixes mentioned. Will get to the tests in a bit.
On the note of an explicit use of HttpClient, I don't think the Image/GlanceApi is where this should go. Determining the right versioned endpoint is something that needs to happen before and ImageApi request, and I'm not sure how we'd include that sort of logic by including this negotiation in the ImageApi itself.
Thinking about it some more, would it be possible/cleaner to use a retryHandler instead of an errorHandler to deal with the HttpClient behavior on HTTP responses with a 300 status code?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37991838
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
The tricky bit for me was that since we're using the apiVersion field in an anonymous class that we construct, the compiler needs to know that the field is final, and the compiler wasn't able to disambiguate between a nonfinal parameter in the constructor and the final field in the class that both had the same name.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10726147
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #196](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/196/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39341943
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #999](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/999/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39342874
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Everett Toews <no...@github.com>.
@jasdeep-hundal Have you tested this against a DevStack instance that's running both versions of the Glance API?
Can you please share your DevStack localrc in a gist?
Can you please share the jclouds code in a gist that you ran against a DevStack instance?
I'd like to try this in a realish env to help me understand it better. Thanks.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38492402
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #990](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/990/) SUCCESS
This pull request looks good
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-39254164
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI baseEndpointUri) {
> + URI versionEndpointUri = null;
> + try {
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
> + if(apiVersion.equals(version.id)) {
> + String newURIString = version.links.get(0).href;
> + if(newURIString.startsWith("http:") && baseEndpointUri.toString().startsWith("https:"))
> + newURIString = "https" + newURIString.substring(4);
> + versionEndpointUri = new URI(newURIString);
Can we use [Uris](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/Uris.java), for this, rather than doing string manipulation directly?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10724016
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by BuildHive <no...@github.com>.
[jclouds » jclouds-labs-openstack #911](https://buildhive.cloudbees.com/job/jclouds/job/jclouds-labs-openstack/911/) UNSTABLE
Looks like there's a problem with this pull request
[(what's this?)](https://www.cloudbees.com/what-is-buildhive)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37379319
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #180](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/180/) UNSTABLE
Looks like there's a problem with this pull request
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37785253
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Mark Washenberger <no...@github.com>.
Hi Folks!
Unfortunately the keystone catalog is not capable of doing version negotiation for clients. So the behavior is a bit variable from cloud to cloud. For example, HP returns a versioned string for their image endpoint [1]. But a recent devstack checkout (which is just about as canonical as you can get) returns an unversioned endpoint [2].
The way the python-glanceclient code deals with this problem is to strip off the version from the endpoint url if it is present, and then apply the version string it *knows* it wants [3] [4]. This is possible because the client knows which version it wants to use.
A better approach would be to strip off the version from the endpoint if present, and then do a get on the unversioned resource, which will return a versions response that indicates which versions are present and what links to follow to reach them. With that response from the server, it is possible to correctly construct the versioned endpoint you want (which will be especially useful if jclouds wants to adopt Glance v2 at some point).
I'm the Glance PTL, and I approve this message. ;-)
[1] - http://docs.hpcloud.com/api/v13/image/#3.3ServiceCatalog
[2] - Here's my output
```
Service: image
+-------------+----------------------------------+
| Property | Value |
+-------------+----------------------------------+
| adminURL | http://10.0.2.15:9292 |
| id | 25034db2ebbe45aea499fde42453503d |
| internalURL | http://10.0.2.15:9292 |
| publicURL | http://10.0.2.15:9292 |
| region | RegionOne |
+-------------+----------------------------------+
````
[3] - https://github.com/openstack/python-glanceclient/blob/master/glanceclient/v1/client.py#L34
[4] - https://github.com/openstack/python-glanceclient/blob/master/glanceclient/common/utils.py#L317
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38088893
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(!apiVersionString.startsWith("v"))
Why do we need this logic...backwards-compatibility? Can't we just enforce the fact that it has to start with a "v"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10723902
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> @@ -46,6 +46,9 @@ public void handleError(HttpCommand command, HttpResponse response) {
> message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(),
> response.getStatusLine());
> switch (response.getStatusCode()) {
> + // This is exclusively to not throw exceptions on Glance version negotiation
> + case 300:
> + return;
Can we add a check so this only applies to the version negotiation requests, and propagate the exception otherwise?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10639087
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +import com.google.common.base.Supplier;
> +import com.google.common.base.Throwables;
> +import com.google.common.cache.CacheBuilder;
> +import com.google.common.cache.CacheLoader;
> +import com.google.common.cache.LoadingCache;
> +
> +/**
> + *
> + * @author Jasdeep Hundal
> + */
> +@Singleton
> +public class ZoneToEndpointNegotiateVersion implements Function<Object, URI> {
> +
> + public static final String VERSION_NEGOTIATION_HEADER = "Is-Version-Negotiation-Request";
> +
> + private static class VersionsJsonResponse{
@nacx: do you have an example handy? ;-) But whether we need this or not will largely depend on the answer to the "use of HttpClient" question, I guess...
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10725021
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> + public String status;
> + public String id;
> + public List<Link> links;
> + }
> + public List<Version> versions;
> + }
> +
> + private final Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier;
> + private final String apiVersion;
> + private final LoadingCache<URI, URI> endpointCache;
> +
> + @Inject
> + public ZoneToEndpointNegotiateVersion(@Zone Supplier<Map<String, Supplier<URI>>> zoneToEndpointSupplier,
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if(apiVersionString != null) {
The api version should never be null. If users don't specify it when creating the context, the default one defined in the api metadata will be set.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10639093
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
Many thanks for the feedback!
If I undertand it properly, although keystone is not capable of doing content negotiation, the approach currently implemented is correct, isn't it?
We'll try to connect to the returned one, and negotiate the version if it returns a 300. Is this version negotiation standard so we can move that logic to the common openstack modules?
If not, please correct me, but I think it is better to inspect the body returned in the 300 respo se rather than directly replacing the api version?
Thx!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38094567
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
> + else {
> + this.apiVersion = "v1.0";
> + }
> + this.endpointCache = CacheBuilder.newBuilder()
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI uri) {
> + try {
> + InputStream response = client.get(uri);
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for( VersionsJsonResponse.Version version : versions.versions ) {
> + if(apiVersion.equals(version.id)) {
> + String newURIString = version.links.get(0).href;
> + if(newURIString.startsWith("http:") && uri.toString().startsWith("https:"));
> + newURIString = "https" + newURIString.substring(4);
> + uri = new URI(newURIString);
Better create a new URI object instead of modifying the one passed as a parameter.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r10639099
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
The error handler approach is OK.
Regarding the cache, the `apply` method will be actually called every time you call the `getExtensionApiForZone` method, so the cache might help saving some requests.
The `ZoneToEndpoint` already uses a cache. The supplier it delegates to is cached (and configured [here](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/location/config/LocationModule.java#L174-L182)). In this case, you could declare the cache in the [GlanceHttpApiModule](https://github.com/jclouds/jclouds-labs-openstack/blob/master/openstack-glance/src/main/java/org/jclouds/openstack/glance/v1_0/config/GlanceHttpApiModule.java).
Sounds good?
Nice job, btw! Next is to properly update the unit tests. As you'll see, the existing "expect" unit tests define a set of `request/response` pairs. They have to be modified to include this "version negotiation" in the request flow, in the form of a mocked request/response pair.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37755874
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + if (!baseEndpointPathParts.isEmpty() &&
> + baseEndpointPathParts.get(baseEndpointPathParts.size() - 1).matches("v[0-9]+(\\.[0-9])?[0-9]*")){
> + baseEndpointUri = new URI(baseEndpointUri.getScheme(), baseEndpointUri.getUserInfo(),
> + baseEndpointUri.getHost(), baseEndpointUri.getPort(),
> + Joiner.on('/').join(baseEndpointPathParts.subList(0, baseEndpointPathParts.size() - 1)) + "/",
> + baseEndpointUri.getQuery(), baseEndpointUri.getFragment());
> + }
> +
> + HttpRequest negotiationRequest = HttpRequest.builder()
> + .method("GET").endpoint(baseEndpointUri)
> + .addHeader(VERSION_NEGOTIATION_HEADER, "true").build();
> + InputStream response = client.invoke(negotiationRequest).getPayload().openStream();
> + VersionsJsonResponse versions = json.fromJson(Strings2.toStringAndClose(response), VersionsJsonResponse.class);
> + for (VersionsJsonResponse.Version version : versions.versions) {
> + if (apiVersion.equals(version.id)) {
> + URI versionedEndpointUri = new URI(version.links.get(0).href);
I'm guessing we know this is the correct one simply from the spec?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095391
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
With Mark's clarification, I'll work on changing this around to accept both, though I'll keep tests with the current changes as I'll take the devstack output to be what we'll usually see.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38089203
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> @@ -46,6 +47,12 @@ public void handleError(HttpCommand command, HttpResponse response) {
> message = message != null ? message : String.format("%s -> %s", command.getCurrentRequest().getRequestLine(),
> response.getStatusLine());
> switch (response.getStatusCode()) {
> + // This is exclusively to not throw exceptions on Glance version negotiation
[minor] Simply something like "// do not throw exceptions during Glance version negotiation"?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095398
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
Using the HttpClient directly or not will depend on whether we are OK about including that call as part of the GlanceApi or not. Do we want to expose that method to users? If it makes sense to expose it, we can add a method to the GlanceApi that returns the versioned endpoints, and call that method instead of directly using the HttpClient.
Although it seems that it introduces some overhead, Glance does the right thing returning a 300. By definition that is the status code to return in this kind of requests.
And regarding the retry handler, that would be possible. The default retry strategy for redirects is implemented in the [RedirectionRetryHandler](https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/http/handlers/RedirectionRetryHandler.java). As you see, after checking if the request should be retried, it updates the HttpCommand with the new request to be performed.
You could subclass it, and, if the request is a version negotiation request, update the HttpCommand accordingly so the request is re-sent to the right endpoint. For other requests, you can delegate to the super to keep the default retry logic. Once you have the custom retry handler, you can add the following to the `GlanceHttpApiModule` to instruct jclouds to use it:
```java
@Override
protected void bindRetryHandlers() {
bind(HttpRetryHandler.class).annotatedWith(Redirection.class).to(YourCustomRetryHandler.class);
}
```
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38030469
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by Ignasi Barrera <no...@github.com>.
Thanks @jasdeep-hundal! Will merge the Keystone PR later today.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37908051
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Everett Toews <no...@github.com>.
> +/*
> + * Copyright 2014 The Apache Software Foundation.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.jclouds.location.functions;
This is definitely not the right package for this. This is going to be common to all Glance versions so I think it belongs in `org.jclouds.openstack.glance.functions`.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11136161
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
> + throw Throwables.propagate(ex);
> + } catch (IOException ex) {
> + throw Throwables.propagate(ex);
> + }
> + throw new HttpException("Glance endpoint does not support API version: " + apiVersion);
> + }
> + });
> + }
> +
> + @Override
> + public URI apply(Object from) {
> + checkArgument(from instanceof String, "you must specify a zone, as a String argument");
> + Map<String, Supplier<URI>> zoneToEndpoint = zoneToEndpointSupplier.get();
> + checkState(!zoneToEndpoint.isEmpty(), "no zone name to endpoint mappings configured!");
> + checkArgument(zoneToEndpoint.containsKey(from),
> + "requested location %s, which is not in the configured locations: %s", from, zoneToEndpoint);
This was copied from ZoneToEndpoint in JClouds core, I'm fine with removing the ISE though.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11096566
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +
> + public void testNonExistentVersion() throws Exception {
> + // The referenced resource only an endpoint for v999.999 of the GlanceApi
> + HttpResponse localVersionNegotiationResponse = HttpResponse.builder().statusCode(300).message("HTTP/1.1 300 Multiple Choices").payload(
> + payloadFromResourceWithContentType("/glanceVersionResponseVersionUnavailable.json", "application/json")).build();
> +
> + GlanceApi apiWhenExist = requestsSendResponses(keystoneAuthWithUsernameAndPassword,
> + responseWithKeystoneAccess, versionNegotiationRequest, localVersionNegotiationResponse);
> +
> + try {
> + apiWhenExist.getImageApiForZone("az-1.region-a.geo-1").list();
> + } catch (UncheckedExecutionException e) {
> + if (e.getCause().getClass() == HttpException.class)
> + return;
> + }
> + fail("Did not throw expected HttpException");
Instead of the `try...catch...fail`, can we use `@Test(expected = HttpException)` here?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095403
Re: [jclouds-labs-openstack] WIP: JCLOUDS-494: Change EndpointParam
parser to negotiate version for Glance API (#82)
Posted by jasdeep-hundal <no...@github.com>.
Went the easy route and made the two line change in `GlanceErrorHandler`. Having a sort of circular dependency between the `ZoneToEndpointNegotiateVersion` object that gets created and the `GlanceApi` object seemed a bit awkward. It also seemed that Fallbacks in the ImageApi were typically used to return a result with the correct type when an image does not exist (as opposed to parse the guts of an error response), and since they receive a Throwable object, I'm not sure there would be enough information to recover and return the versioned Glance endpoint.
Added a cache with the latest commit, but I think I'd like an opinion on whether it's necessary (or whether this is the right place for it). The `ZoneToEndpoint` class that the `ZoneToEndpointNegotiateVersion` class is based on is a Singleton, so when we inject the `apiVersionString` value, it's there for the duration of the application. We're only caching on the base endpoint received from Keystone at that point, and I don't foresee this changing often. If this cache is useful (if each request is indeed calling the `apply` method in `ZoneToEndpointNegotiateVersion`), then perhaps the right architectural decision is to cache results from `ZoneToEndpoint`/`ZoneToEndpointNegotiateVersion` one level up the stack since the base endpoint for Glance that is returned from the Keystone catalog should not change often.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-37751271
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> + @ApiVersion String apiVersionString, final HttpClient client, final Json json) {
> + this.zoneToEndpointSupplier = checkNotNull(zoneToEndpointSupplier, "zoneToEndpointSupplier");
> + if (!apiVersionString.startsWith("v")) {
> + this.apiVersion = "v" + apiVersionString;
> + } else {
> + this.apiVersion = apiVersionString;
> + }
> + this.endpointCache = CacheBuilder.newBuilder()
> + .build(
> + new CacheLoader<URI, URI>() {
> + public URI load(URI baseEndpointUri) {
> + try {
> + List<String> baseEndpointPathParts = Splitter.on('/').omitEmptyStrings().splitToList(baseEndpointUri.getPath());
> + if (!baseEndpointPathParts.isEmpty() &&
> + baseEndpointPathParts.get(baseEndpointPathParts.size() - 1).matches("v[0-9]+(\\.[0-9])?[0-9]*")){
> + baseEndpointUri = new URI(baseEndpointUri.getScheme(), baseEndpointUri.getUserInfo(),
Quick comment here as to what this line is doing?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095390
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-labs-openstack-pull-requests #183](https://jclouds.ci.cloudbees.com/job/jclouds-labs-openstack-pull-requests/183/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38334147
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
Many thanks, @markwash! Guess we can't get more official input than that ;-) Looking forward to the changes, @jasdeep-hundal!
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82#issuecomment-38089395
Re: [jclouds-labs-openstack] JCLOUDS-494: Change EndpointParam parser
to negotiate version for Glance API (#82)
Posted by Andrew Phillips <no...@github.com>.
> +import org.jclouds.http.HttpRequest;
> +import org.jclouds.http.HttpResponse;
> +import org.jclouds.openstack.glance.v1_0.GlanceApi;
> +import org.jclouds.openstack.glance.v1_0.internal.BaseGlanceApiExpectTest;
> +import org.jclouds.openstack.glance.v1_0.parse.ParseImagesTest;
> +import org.testng.annotations.Test;
> +
> +import com.google.common.util.concurrent.UncheckedExecutionException;
> +
> +/**
> + * @author Jasdeep Hundal
> + */
> +@Test(groups = "unit", testName = "GlanceVersionNegotiationExpectTest")
> +public class GlanceVersionNegotiationExpectTest extends BaseGlanceApiExpectTest {
> +
> + public void testSchemeMismatch() throws Exception {
Could you explain the three tests, perhaps in short code comment? The first tests what happens if Glance returns the same version we expect, but a different scheme? And the second returns only different versions? Or am I misunderstanding this?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-openstack/pull/82/files#r11095407