Add ModifySubnetAttribute #1102

geomacy · 2017-05-17T21:13:29Z

This is a follow-up to #1091 (comment).

+1 to merging this but I have been trying this out and I think we will need to extend it for practical purposes; if you want to create a VPC and subnet and then depl>oy a machine on to it, you also need to jump through a few other hoops apart from creating the subnet:

modify the subnet attributes to permit auto-assign public IP ("ModifySubnetAttribute")
create an Internet Gateway on the VPC ("CreateInternetGateway")
get and then modify the routing table of the subnet to add a public (0.0.0.0/0) route through the newly added gateway ("CreateRoute" and friends)
But as mentioned let's merge this as-is, and do any such new stuff as a new PR!

This PR adds the capability to modify the subnet to auto-assign the public IP via ModifySubnetAttribute.

It turned out that the ModifySubnetAttribute is a method that has been added to the AWS API subsequent to the version 2012-06-01 of the other subnet methods; to allow the selective specification of a different version I am proposing in this PR the introduction of a new annotation ApiVersionOverride, q.v. and see its use in https://github.com/jclouds/jclouds/compare/master...geomacy:modify-subnet-attribute?expand=1#diff-aa0b3b1bb310c30b9f8fdbab82794412R145

The annotation is processed by FormSignerV4; should I do this in FormSignerV2 as well?

andreaturli

looks really useful as we discussed on IRC.
Some minor comments, but @geomacy your impl is even cleaner than expected using the annotation.
Thanks!

andreaturli · 2017-05-18T12:46:12Z

apis/sts/src/main/java/org/jclouds/aws/filters/FormSignerV4.java

@@ -101,7 +105,25 @@
      this.serviceAndRegion = serviceAndRegion;
   }

-   @Override public HttpRequest filter(HttpRequest request) throws HttpException {
+   private String getAnnotatedApiVersion(HttpRequest request) {


[minor] Although private could you make return Optional<String> instead of null ?

andreaturli · 2017-05-18T12:49:52Z

core/src/main/java/org/jclouds/rest/annotations/ApiVersionOverride.java

+@Target({ METHOD })
+@Retention(RUNTIME)
+@Qualifier
+public @interface ApiVersionOverride {


I like this idea, although maybe ARM approach -- which is meant to solve the same issue -- is slightly more flexible as you can configure the version without re-building the project.

Anyway I think it is really unlikely that a new version would not require a change at level of java implementation, so I think the Annotation is sensible here.

@nacx any further comments?

andreaturli · 2017-05-18T12:50:07Z

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/features/AWSSubnetApi.java

+    * @param options The options containing the attribute to modify. You can only modify one attribute at a time.
+    * @return true if the modification was successful
+    */
+   @ApiVersionOverride("2014-06-15")


neat, I like it!

andreaturli · 2017-05-18T12:52:04Z

I think we should edit the FormSignerV2 as well

nacx · 2017-05-18T14:26:38Z

I like the annotation approach and agree that the flexibility needed in ARM, where they deploy new versions of the APIs quite often (but keeping backward compatibility) might not be needed here.

A couple questions:

It would be worth considering the annotation at class level too. Could you implement that?
I think it makes sense to reuse the @SinceApiVersion annotation for this purpose. It may affect existing methods, but it should be safe, as it just defines the versions where the annotated methods can work. jclouds is already using that annotation in a similar way. For example, providers that have a root API that delegates to feature APIs (such as the AWSEC2Api) can return an Optional. If those methods are annotated with the @SinceApiVersion, jclouds will return absent instead of the API class (magic is done here). So, for me, letting the FormSigner (v2 and V4) override the API version for those methods annotated with @SinceApiVersion makes sense and I'd prefer this approach instead of introducing a new annotation to jclouds-core.

geomacy · 2017-05-18T15:51:12Z

@nacx

I can add the annotation at class level.

I originally started using @SinceApiVersion, and having the annotation at both method and class level, but found that various tests broke, because various APIs are annotated @SinceApiVersion. I will take a look at that again, but switched to having a separate annotation as I thought it was safer.

nacx · 2017-05-18T17:52:56Z

Unit tests may fail because they hardcode the expected version. Live tests will tell us if the change is safe. Let's run them for the classes that use the annotation.

geomacy · 2017-05-18T19:11:47Z

Updates made.

I was able to switch to using@SinceApiVersion, very pleased with that. The key thing is that I needed to change the override from a just-override-it approach to one that will allow the annotation to upgrade the version as compared to apiVersion but not downgrade it, which is much safer. All the unit tests now pass, and my live subnet test too.

By the way how is the @ApiVersion actually injected in places like FormSigner, for example? I think the value is being taken from AWSEC2ApiMetadata but I couldn't work out how jclouds configures Guice (or whatever) to actually wire them together.

nacx · 2017-05-18T22:18:17Z

Good!

Have you been able to run the live tests for the affected APIs?

By the way how is the @apiversion actually injected in places like FormSigner, for example? I think the value is being taken from AWSEC2ApiMetadata but I couldn't work out how jclouds configures Guice (or whatever) to actually wire them together.

The default value is taken from the metadata. It can be overridden when creating the context, and it is configured and bound in a Guice module when creating the context.

geomacy · 2017-05-19T11:47:31Z

hi @nacx

thanks for the explanation of the wiring.

I've tried to run live tests, but may not be set up for it - the tests that have @SinceApiVersion are

find . -name '*Api.java' | xargs grep -l SinceApiVersion | sed 's#.*/##' | while read file ; do   find . -name ${file%.java}LiveTest.java; done
./apis/chef/src/test/java/org/jclouds/chef/ChefApiLiveTest.java
./apis/chef/src/test/java/org/jclouds/chef/features/OrganizationApiLiveTest.java
./apis/ec2/src/test/java/org/jclouds/ec2/features/ElasticIPAddressApiLiveTest.java
./apis/ec2/src/test/java/org/jclouds/ec2/features/InstanceApiLiveTest.java
./apis/openstack-trove/src/test/java/org/jclouds/openstack/trove/v1/features/InstanceApiLiveTest.java
./providers/google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/features/InstanceApiLiveTest.java
./apis/ec2/src/test/java/org/jclouds/ec2/features/SubnetApiLiveTest.java
./apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/TagApiLiveTest.java
./apis/ec2/src/test/java/org/jclouds/ec2/features/WindowsApiLiveTest.java
./providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/features/AWSSubnetApiLiveTest.java

I can run /providers/aws-ec2/.../AWSSubnetApiLiveTest and /apis/ec2/.../SubnetApiLiveTest successfully.

I've tried running /apis/ec2/.../ElasticIPAddressApiLiveTest and InstanceApiLiveTest with the same credentials, and I get org.jclouds.rest.AuthorizationException: POST https://ec2.eu-west-2.amazonaws.com/ HTTP/1.1 -> HTTP/1.1 401 Unauthorized results for both operations; I have also tried this with a different Amazon account, with the same result. I suspect I need to do something else to run the test correctly but I'm not sure what? I'm running them as e.g:

mvn clean install  -Dtest.ec2.identity=.... -Dtest.ec2.credential="..." -Dtest=InstanceApiLiveTest -Plive

When running the Chef tests I get errors complaining about No such file or directory for my .chef folder, so must need some more setup there, but haven't found any instructions on how to set up the chef tests.

I haven't tried the tests in /apis/openstack-trove, /providers/google-compute-engine, or /apis/cloudstack as the FormSignerV4 is only used for AWS.

Have you any links to info that I might be able to use to get those ec2 tests going?

geomacy · 2017-05-22T07:56:23Z

p.s. I have checked the same tests as above in master and they fail in the same way for me, so it is something to do with my environment or AWS account, not a feature of the PR itself.

nacx · 2017-05-22T09:53:49Z

I don't know why it raises that unauthorized error. I tried with my account with the same results.
Also tried modifying the test to bind the form4 signer by default, without success, and creating a specific test in the awsec2 provider that extends the one in the generic ec2, but with the same auth errors.
@andreaturli Have you recently tested the ec2 api alone in a similar way and can help here?

geomacy · 2017-05-22T11:46:00Z

Ah, so it's something to do with the tests themselves then, not just my environment being wrong? I take it you get the error in master too?

nacx · 2017-05-22T12:41:56Z

I take it you get the error in master too?

Yes. Something to investigate. But I'd rather keep this PR on hold for a while, just to make sure auth failures don't come (apart from other things) from signature issues?

geomacy · 2017-05-22T12:56:05Z

@nacx ok; I'll try to take a look at the tests to see if I can contribute anything to a fix. I'd be keen to get this merged so I can start using it.

The intention is to use @SinceApiVersion for this purpose, but that would affect a number of APIs, and we would want to have good test coverage before merging that change (in FormSignerUtils#getAnnotatedApiVersion). However, there is some issue wth certain tests at resent that means we cannot successfully test all APIs that make use of @SinceApiVersion in order to assure ourselves that FormSignerUtils will not introduce some problem. See jclouds#1102 (comment) for details. This annotation is introduced as a temporary measure in order to decouple the functionality of FormSignerUtils#getAnnotatedApiVersion from @SinceApiVersion and the tests in question. It can be removed and replaced by @SinceApiVersion when those tests are fixed. Designates that a method overrides the {@link ApiVersion} on the class with a specific value.

geomacy · 2017-05-26T14:27:48Z

hi @nacx, per our conversation on IRC I have reverted to using a custom annotation, but put a comment on it explaining that it is meant only as a temporary measure to decouple the new function in FormSigner* from other APIs for the moment.

I have squashed the other commits from this PR into one, and left the custom annotation as a separate commit, so that it's easy to tell it apart, and hopefully even just revert it when the tests are working again.

How do you think that looks?

geomacy · 2017-05-26T15:25:56Z

p.s. I tested the Live tests for AWSEC2SubnetApi again successfully with this, as well as the mock tests

nacx · 2017-05-29T08:41:52Z

Thanks, @geomacy! I've merged it to master and 2.0.x.

The intention is to use @SinceApiVersion for this purpose, but that would affect a number of APIs, and we would want to have good test coverage before merging that change (in FormSignerUtils#getAnnotatedApiVersion). However, there is some issue wth certain tests at resent that means we cannot successfully test all APIs that make use of @SinceApiVersion in order to assure ourselves that FormSignerUtils will not introduce some problem. See #1102 (comment) for details. This annotation is introduced as a temporary measure in order to decouple the functionality of FormSignerUtils#getAnnotatedApiVersion from @SinceApiVersion and the tests in question. It can be removed and replaced by @SinceApiVersion when those tests are fixed. Designates that a method overrides the {@link ApiVersion} on the class with a specific value.

andreaturli reviewed May 18, 2017

View reviewed changes

geomacy added 2 commits May 26, 2017 14:35

Add ModifySubnetAttribute

cc4ece5

geomacy force-pushed the modify-subnet-attribute branch from 70860c7 to 59f593a Compare May 26, 2017 14:23

nacx approved these changes May 29, 2017

View reviewed changes

nacx closed this May 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ModifySubnetAttribute #1102

Add ModifySubnetAttribute #1102

geomacy commented May 17, 2017

andreaturli left a comment

andreaturli May 18, 2017

andreaturli May 18, 2017

andreaturli May 18, 2017

andreaturli commented May 18, 2017

nacx commented May 18, 2017

geomacy commented May 18, 2017

nacx commented May 18, 2017

geomacy commented May 18, 2017 •

edited

nacx commented May 18, 2017

geomacy commented May 19, 2017 •

edited

geomacy commented May 22, 2017

nacx commented May 22, 2017

geomacy commented May 22, 2017

nacx commented May 22, 2017 •

edited

geomacy commented May 22, 2017

geomacy commented May 26, 2017

geomacy commented May 26, 2017

nacx commented May 29, 2017

Add ModifySubnetAttribute #1102

Add ModifySubnetAttribute #1102

Conversation

geomacy commented May 17, 2017

andreaturli left a comment

Choose a reason for hiding this comment

andreaturli May 18, 2017

Choose a reason for hiding this comment

andreaturli May 18, 2017

Choose a reason for hiding this comment

andreaturli May 18, 2017

Choose a reason for hiding this comment

andreaturli commented May 18, 2017

nacx commented May 18, 2017

geomacy commented May 18, 2017

nacx commented May 18, 2017

geomacy commented May 18, 2017 • edited

nacx commented May 18, 2017

geomacy commented May 19, 2017 • edited

geomacy commented May 22, 2017

nacx commented May 22, 2017

geomacy commented May 22, 2017

nacx commented May 22, 2017 • edited

geomacy commented May 22, 2017

geomacy commented May 26, 2017

geomacy commented May 26, 2017

nacx commented May 29, 2017

geomacy commented May 18, 2017 •

edited

geomacy commented May 19, 2017 •

edited

nacx commented May 22, 2017 •

edited