Some comments:
On Thu, 2010-05-06 at 14:43 +0200, [email protected] wrote:
> ---
> .../lib/deltacloud/drivers/gogrid/gogrid_driver.rb | 52
> +++++++++++++++-----
> server/server.rb | 5 ++-
> server/views/instances/new.html.haml | 2 +-
> server/views/instances/show.html.haml | 9 +++
> server/views/instances/show.xml.haml | 5 ++
> 5 files changed, 59 insertions(+), 14 deletions(-)
>
> diff --git a/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> b/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> index 875b876..2538da7 100644
> --- a/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> +++ b/server/lib/deltacloud/drivers/gogrid/gogrid_driver.rb
> @@ -18,12 +18,19 @@
> require 'deltacloud/base_driver'
> require 'deltacloud/drivers/gogrid/gogrid_client'
>
> +class Instance
> + attr_accessor :username
> + attr_accessor :password
> +end
> +
> module Deltacloud
> module Drivers
> module Gogrid
>
> class GogridDriver < Deltacloud::BaseDriver
>
> + feature :instances, :authentification_password
> +
> define_hardware_profile 'server' do
> cpu 2
> memory [512, 1024, 2048, 4096, 8192]
> @@ -78,12 +85,20 @@ class GogridDriver < Deltacloud::BaseDriver
> end
> name = (opts[:name] && opts[:name]!='') ? opts[:name] :
> get_random_instance_name
> safely do
> - convert_instance(new_client(credentials).request('grid/server/add', {
> + instance = new_client(credentials).request('grid/server/add', {
> 'name' => name,
> 'image' => image_id,
> 'server.ram' => server_ram,
> 'ip' => get_next_free_ip(credentials)
> - })['list'].first, credentials.user)
> + })['list'].first
> + if instance
> + login_data = get_login_data(instance[:id])
> + instance['username'] = login_data['username']
> + instance['password'] = login_data['password']
What happens if get_login_data fails ? We should still return an
instance, but somehow indicate in it that we failed to get the
username/password.
> + return convert_instance(instance, credentials.user)
> + else
> + return nil
> + end
> end
> end
>
> @@ -91,7 +106,10 @@ class GogridDriver < Deltacloud::BaseDriver
> instances = []
> if opts and opts[:id]
> safely do
> - instance = new_client(credentials).request('grid/server/get', { 'id'
> => opts[:id]})['list'].first
> + instance = new_client(credentials).request('grid/server/get', {
> 'name' => opts[:id]})['list'].first
> + login_data = get_login_data(credentials, instance['name'])
> + instance['username'] = login_data['username']
> + instance['password'] = login_data['password']
Same comment here on failure of get_login_data.
> instances = [convert_instance(instance, credentials.user)]
> end
> else
> @@ -107,22 +125,18 @@ class GogridDriver < Deltacloud::BaseDriver
>
> def reboot_instance(credentials, id)
> safely do
> - new_client(credentials).request('grid/server/power', { 'id' => id,
> 'power' => 'reboot'})
> - end
> - end
> -
> - def stop_instance(credentials, id)
> - safely do
> - new_client(credentials).request('grid/server/power', { 'id' => id,
> 'power' => 'off'})
> + new_client(credentials).request('grid/server/power', { 'name' => id,
> 'power' => 'reboot'})
> end
> end
>
> def destroy_instance(credentials, id)
> safely do
> - new_client(credentials).request('grid/server/delete', { 'id' => id})
> + new_client(credentials).request('grid/server/delete', { 'name' => id})
> end
> end
>
> + alias :stop_instance :destroy_instance
> +
> define_instance_states do
> start.to( :pending ) .automatically
> pending.to( :running ) .automatically
This hunk isn't connected to authentication - please split it out into a
separate patch.
> @@ -137,6 +151,18 @@ class GogridDriver < Deltacloud::BaseDriver
> GoGridClient.new('https://api.gogrid.com/api', credentials.user,
> credentials.password)
> end
>
> + def get_login_data(credentials, instance_id)
> + login_data = {}
> + new_client(credentials).request('support/password/list')['list'].each do
> |passwd|
> + next unless passwd['server']
> + if passwd['server']['name'] == instance_id
> + login_data['username'], login_data['password'] = passwd['username'],
> passwd['password']
> + break
> + end
> + end
> + return login_data
> + end
Would be nicer to pass the client in so we don't create a new one all in
several places.
> diff --git a/server/server.rb b/server/server.rb
> index 9f91d86..c4a4125 100644
> --- a/server/server.rb
> +++ b/server/server.rb
> @@ -242,7 +242,10 @@ collection :instances do
> @instance = instance
> haml :"instances/show"
> end
> - format.html { redirect instance_url(instance.id) }
> + format.html do
> + redirect instance_url(instance.id) if instance
> + redirect instances_url
> + end
> end
> end
> end
Not connected to authentication - please split into separate patch.
> diff --git a/server/views/instances/new.html.haml
> b/server/views/instances/new.html.haml
> index 25d8689..debcda8 100644
> --- a/server/views/instances/new.html.haml
> +++ b/server/views/instances/new.html.haml
> @@ -9,7 +9,7 @@
> %label
> Instance Name:
> %input{ :name => 'name', :size => 30 }/
> - -if driver_has_feature(:authentification_key)
> + -if driver_has_feature?(:authentification_key)
> %p
> %label
> Instance Keyname:
This hunk should go into the EC2 patch.
> diff --git a/server/views/instances/show.html.haml
> b/server/views/instances/show.html.haml
> index ebbdd2a..e000041 100644
> --- a/server/views/instances/show.html.haml
> +++ b/server/views/instances/show.html.haml
> @@ -41,6 +41,15 @@
> %dt Keyname
> %dd
> = @instance.keyname
> + - if driver_has_feature?(:authentification_password)
> + %di
> + %dt Username
> + %dd
> + = @instance.username
> + %di
> + %dt Password
> + %dd
> + = @instance.password
> %di
> %dt
> %dd
> diff --git a/server/views/instances/show.xml.haml
> b/server/views/instances/show.xml.haml
> index fd9d01d..5b7ccb4 100644
> --- a/server/views/instances/show.xml.haml
> +++ b/server/views/instances/show.xml.haml
> @@ -29,3 +29,8 @@
> - if driver_has_feature?(:authentification_key)
> %keyname<
> [email protected]
> + - if driver_has_feature?(:authentification_password)
> + %username<
> + [email protected]
> + %password<
> + [email protected]
We should wrap this and the output for authentication_key into a
container tag so that it's easy to tell from looking at the XML what
kind of auth scheme is in use.
For EC2:
<authentication type="key">
<keyname>12345</keyname>
</authentication>
For GoGrid:
<authentication type="password">
<username>root</username>
<password>secret</password>
</authentication>
and for the case when we fail to retrieve the details:
<authentication type="...">
<retrieval_failed>
... details about the failure if possible ..
</retrieval_failed>
</authentication>
Also, we need to add this to the Rackspace driver - could we just use
authentication_password there ? It's a little different in that we can't
report the password on GET /api/images/:id
David
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
