[jclouds-labs-google] dabbleing with fixes, still broken (#116)

[danbroudy <no...@github.com>]

This is not ready to be merged. 

Some of the live tests are still failing. I wanted to ask if I am missing some way to pass credentials into these test. 

Currently I am overriding all the tests that require credentials to SSH into an instance and adding the following lines: 
`template.getOptions().overrideLoginCredentials(loginCredentials);`
`template.getOptions().authorizePublicKey(this.keyPair.get("public"));`

@nacx any better ways to pass credentials into these live test?
You can merge this Pull Request by running:

  git pull https://github.com/GoogleCloudPlatform/jclouds-labs-google overrideTestFix

Or you can view, comment on it, or merge it online at:

  https://github.com/jclouds/jclouds-labs-google/pull/116

-- Commit Summary --

  * dabbleing with fixes, still broken

-- File Changes --

    M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceAdapter.java (53)
    M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/options/GoogleComputeEngineTemplateOptions.java (3)
    M google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/domain/Instance.java (2)
    M google-compute-engine/src/test/java/org/jclouds/googlecomputeengine/compute/GoogleComputeEngineServiceLiveTest.java (403)

-- Patch Links --

https://github.com/jclouds/jclouds-labs-google/pull/116.patch
https://github.com/jclouds/jclouds-labs-google/pull/116.diff

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[Ignasi Barrera <no...@github.com>]

I forgot to mention: If users already have the SSH key populated in the global project metadata, then there is no need to authorize a new public key. They can just set the `autogenerateKeyPair` option to false, and provide the proper `overrideLoginPrivateKey` option (again, this is only needed if the key is not already added to the ssh-agent).

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67940626

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[Ignasi Barrera <no...@github.com>]

@danbroudy Good news! I have a [wip branch](https://github.com/nacx/jclouds-labs-google/compare/compute-auth) where all authentication related live tests are passing:

```bash
Failed tests:
  TargetHttpProxyApiLiveTest.testInsertTargetHttpProxy:52->BaseGoogleComputeEngineApiLiveTest.assertOperationDoneSuccessfully:90 » IllegalState
  ImageApiLiveTest.testCreateImageFromPD:80->BaseGoogleComputeEngineApiLiveTest.assertOperationDoneSuccessfully:90 » IllegalState
  InstanceApiLiveTest.testAddAccessConfig:130->BaseGoogleComputeEngineApiLiveTest.assertOperationDoneSuccessfully:90 » IllegalState

Tests run: 158, Failures: 3, Errors: 0, Skipped: 14
```

We still have to figure out why these three tests are still failing and why the others are skipped, but with the changes in the branch we'll have the compute tests working. Here is a summary of the changes there (they are all in the last commit):

* I've removed all custom strategies that populated the credentials. The jclouds default ones should be good to go in GCE too.
* I've added an option to the `GoogleComputeEngineTempalteOptions` to configure if a SSH key pair should be created if none is provided. By default is true.
* I've added the logic to create that SSH key pair when needed, and adapted the compute service to configure the `LoginCredentials` accordingly.

This way, if no specific authentication is provided, jclouds will create a default one so it can access the VM to bootstrap it. If users provide their custom auth config, however, jclouds will just use that one.

With this changes the VM authentication and the provider itself gets simpler and the ComputeService live tests are passing without having to change them. I'll play around with different combinations of the code snippet of my previous comment to make sure all combinations work as expected, cleanup a bit the branch and open a pull request.

Any comments on the code and/or feedback about this authentication configuration are very welcome!

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67907424

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[Ignasi Barrera <no...@github.com>]

@danbroudy see https://github.com/jclouds/jclouds-labs-google/pull/118.

There is no generic call in jclouds to add SSH keys to instances. The way to configure that is by using the `TemplateOptions.authorizePublicKey(String key)` option when creating the nodes with the ComputeService. The current implementation already reads that property and configures the SSH key in the instance accordingly.

So, with  https://github.com/jclouds/jclouds-labs-google/pull/118, users will be able to access an instance by:

* Not configuring credentials at all. jclouds will take care of creating a key pair so it can access the node to bootstrap it if needed. This default can be turned off by setting the `autoGenerateKeyPair` option to false in the template options.
* Using the `authorizePublicKey` option in the template options to install their SSH key. They can provide also the `overrideLoginPrivateKey` option to configure the private key, if they don't want to delegate authentication to the ssh-agent.



---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67940341

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[Ignasi Barrera <no...@github.com>]

I'll have a deep look at this over the weekend. In general, access to VMs is the main cause of Compute live test failures, and we need to make sure the "base" compute live test class works out of the box, to be sure the compute implementation meets the expected contract.

That said, configuring the credentials can be tricky. I'll have a deep look over the weekend, as I have to "refresh" how the provider works now and align it to how GCE expects the access to the VMs to be done. I'll comment in this PR to keep you up to date and to discuss the design/impl of a proper VM access configuration.

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67712065

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[Ignasi Barrera <no...@github.com>]

I've been doing some tests and will continue this week, but here's what I'd do to focus on the authentication issues and fix the *real* problems:

We should try to make this small code snippet work "out of the box":

```java
ComputeServiceContext ctx = ContextBuilder.newBuilder("google-compute-engine")
   .credentials("identity", "credential")
   .modules(ImmutableSet.of(new SshjSshClientModule())) //
   .buildView(ComputeServiceContext.class);

ComputeService compute = ctx.getComputeService();
TemplateOptions options = compute.templateOptions();

Template template = compute.templateBuilder()
   .options(options.runScript("uptime"))
   .build();

compute.createNodesInGroup("jclouds-auth", 1);
```

This will try to create a node with the default template, and access it to run the *uptime* command. Note that **we are not providing any credentials** here. Ideally, this piece of code should work, but we are now just trying to isolate the issue, so we can play with the *options* object, and override the login user, password, private key, etc.

Here is what will be going on, related to the credentials when running that code:

1. jclouds will list all images and try to find the "default" template, as defined by [the default template property](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/GoogleComputeEngineApiMetadata.java#L65) (which can be, btw, overridden when creating the context).
2. It will transform every GCE image to the jclouds *Image* domain model using the [configured Image transformation function](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/functions/GoogleComputeEngineImageToImage.java).
3. For each resulting image, jclouds will invoke the [PopulateDefaultLoginCredentialsForImageStrategy](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/PopulateDefaultLoginCredentialsForImageStrategy.java) to set the default credentials to access the image. In GCE, that strategy is bound to [this class](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/strategy/PopulateDefaultLoginCredentialsForImageStrategy.java), but I'm not sure if it is working as expected. We need to review this carefully and see if what it does is really needed.
4. With the list of images, it runs the [TemplateBuilder#build](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/domain/internal/TemplateBuilderImpl.java#L670) method to select the image to deploy.
5. Now the `compute.createNodesInGroup` method will create an instance of the selected image, and configure a firewall to open port 22, because we've configured a `runScript` option that requires that port.
6. Once the node is created, **it stores the node credentials** in an internal (in-memory by default) credential store to reuse them when needed. The stored credentials are determined by the [PrioritizeCredentialsFromTemplate](https://github.com/jclouds/jclouds/blob/master/compute/src/main/java/org/jclouds/compute/strategy/PrioritizeCredentialsFromTemplate.java) class, which prioritised the credentials extracted from the TemplateOptions, in case the user configured them (in GCE this strategy is bound to the [UseNodeCredentialsButOverrideFromTemplate](https://github.com/jclouds/jclouds-labs-google/blob/master/google-compute-engine/src/main/java/org/jclouds/googlecomputeengine/compute/strategy/UseNodeCredentialsButOverrideFromTemplate.java) class). If not, it will default to the ones returned `NodeAndInitialCredentials` object in the `ComputeServiceAdapter` implementation.
7. Finally, once the note has started, jclouds will block until the port 22 is open, and then try to authenticate with the resolved credentials.

I've tried to give an overview of the compute process involving the login credentials, but I might have missed something. In any case, this should give enough context to play with the given small code snippet being able to debug the relevant classes to determine what in the provider current implementation is not properly aligned with what jclouds expects.

In the end, I think we have to answer the following question:

*Do GCE images have default credentials, or an initial login user/password/privateKey must be always set?*

If the former is true (I think it's not), then we should be able to populate those default credentials when listing the images, so they already have the working credentials set. Users should still able to override them using the template options.

If credentials must be always configured, then we must make sure the provider works even if they're not provided:

* We should create default SSH key pairs for the instances, if none has been set (this is what AWS, DigitalOcean or OpenStackNova do). This is the tricky part, as we must decide *where* we do this. After listing the images (kind of what is being done now), or *before* creating the nodes (IMO where it should be).
* We should respect LoginCredentials configured by users in the template options.

Taking all this into account we should be able to have a flexible implementation that works with different combinations of the given code snippet, and live tests will pass (with some minimal changes, such as configuring some convenience credentials by overriding the [BaseComputeServiceLiveTest#buildTemplate](https://github.com/jclouds/jclouds/blob/master/compute/src/test/java/org/jclouds/compute/internal/BaseComputeServiceLiveTest.java#L514)) method.

Ok, it's been quite a long answer but wanted to give enough context so you can get an idea of how it works so we can come up with an implementation we're all happy with. I'll continue testing the provider this week and trying different implementation changes, and share keep sharing my progress here. Your input and thoughts are very welcome! 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67789965

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[danbroudy <no...@github.com>]

Sounds good! 

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67712167

Re: [jclouds-labs-google] dabbleing with fixes, still broken (#116)

[danbroudy <no...@github.com>]

This is great news!

Looks great to me. In GCP you can either set project wide ssh-keys that then are propagated into every instance you spin up OR you can put ssh keys in the project metadata like you are here. I just want to make sure that it is a conscious choice to do it this way, I think it is good to intentionally set the keys of individual instances. 

Is there some jclouds generic call for adding sshkeys to instances? If not it may make sense to add a call to the InstancesApi for adding ssh-keys. The call would provide logic for formatting the metadata correctly, and adding keys without repeats ect. 

Would it make sense to save the generated keys to a file for reuse?

---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs-google/pull/116#issuecomment-67911488