deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "marios@redhat.com" <mandr...@redhat.com>
Subject Re: [PATCH] Openstack driver - adds forced_authentication for /api... checks whether Openstack provider supports buckets collection (is swift service deployed?)
Date Fri, 14 Sep 2012 08:33:57 GMT
Hi David - thanks for having a look - more inline:

On 13/09/12 23:18, David Lutterkort wrote:
> On Wed, 2012-09-12 at 15:32 +0300, marios@redhat.com wrote:
>> From: marios <marios@redhat.com>
>>
>> related to https://issues.apache.org/jira/browse/DTACLOUD-316
>> https://github.com/ruby-openstack/ruby-openstack/pull/13
>>
>> Signed-off-by: marios <marios@redhat.com>
>> ---
>>  .../deltacloud/drivers/openstack/openstack_driver.rb  | 19 +++++++++++++------
>>  server/lib/deltacloud/server.rb                       |  6 +++++-
>>  2 files changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> index 5b29840..ca824bd 100644
>> --- a/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> +++ b/server/lib/deltacloud/drivers/openstack/openstack_driver.rb
>> @@ -40,6 +40,19 @@ module Deltacloud
>>  
>>          define_hardware_profile('default')
>>  
>> +        def respond_to?(capability)#, credentials)
>> +          if capability == :buckets
>> +            begin
>> +              client = new_client(Deltacloud.config["openstack_creds"], capability)
>> +            rescue Deltacloud::ExceptionHandler::NotImplemented #OpenStack::Exception::NotImplemented...
>> +              return false
>> +            end
>> +            return true
>> +          else
>> +            super(capability)
>> +          end
>> +        end
> 
> This is not a good idea - it means that something as innocuous looking
> as driver.respond_to?(:buckets) has major sideeffects and fail for all
> kinds of reasons.
> 
> The right fix is to change the supported_collections method in
> deltacloud_helper.rb - this whol capability checking seems way too
> convoluted to me, anyway.

sure - the reason I used 'respond_to' rather than 'has_capability' was
because the check in the collections/buckets.rb looks like:

 16 module Deltacloud::Collections$
 17   class Buckets < Base$
 18 $
 19     include Deltacloud::Features$
 20 $
 21     set :capability, lambda { |m| driver.respond_to? m }$

as this is repeated across all collections I wanted to maintain the
consistency. At some point (and my original implementation) I changed
respond_to? to has_capability? on line 21 above and overrode that method
in the openstack driver instead. I can easily change this and look at
overriding supported_collections instead.
(more below)

 Why can't we have a
> supported_collections(credentials) method in the base driver which, for
> all I care, use introspection to figure out which collections are
> available, and override that in the OpenStack driver ?
> 
>>          def hardware_profiles(credentials, opts = {})
>>            os = new_client(credentials)
>>            results = []
>> @@ -351,12 +364,6 @@ private
>>            end
>>          end
>>  
>> -        def cloudfiles_client(credentials)
>> -          safely do
>> -            CloudFiles::Connection.new(:username => credentials.user, :api_key
=> credentials.password)
>> -          end
>> -        end
>> -
>>  #NOTE: for the convert_from_foo methods below... openstack-compute
>>  #gives Hash for 'flavors' but OpenStack::Compute::Flavor for 'flavor'
>>  #hence the use of 'send' to deal with both cases and save duplication
>> diff --git a/server/lib/deltacloud/server.rb b/server/lib/deltacloud/server.rb
>> index c7c6c41..4b9485e 100644
>> --- a/server/lib/deltacloud/server.rb
>> +++ b/server/lib/deltacloud/server.rb
>> @@ -44,8 +44,12 @@ module Deltacloud
>>      set :config, Deltacloud[:deltacloud]
>>  
>>      get '/' do
>> -      if params[:force_auth]
>> +      if driver.name == "openstack" or params[:force_auth]
> 
> Why is this check necessary ? The LazyCredentials helper already throws
> a 401 if you try to use credentials and they are not provided.
> 

It's necessary because of the point at which the :buckets capability is
checked - i.e. in the declaration of :buckets collection, rather than in
a request. The LazyCredentials aren't in scope (and I couldn't get
access to 'env' of the request context in order to initialise
LazyCredentials) at the point where these are needed to query the
Openstack provider. So the easiest way was to make the check at the
'first' request which is the entry point.

>>          return [401, 'Authentication failed'] unless driver.valid_credentials?(credentials)
>> +        if driver.name == "openstack"
>> +          Deltacloud.config["openstack_creds"] = credentials
> 
> You can't do that - that's a global object shared across all threads.
> 
>> +          #or here also works: Thread.current["openstack_creds"] = credentials
> 
> This would be better. Though even better would be a direct
> supported_collections(credentials) method on the drivers.

OK - will work on tidying this up today. This code is already pushed
after the initial ACK but I can just patch ontop of that,

thanks, marios


> David
> 
> 


Mime
View raw message