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 14:49:45 GMT
see what you think of http://tracker.deltacloud.org/set/62
(notes/intro are on the patches sent to this mailing list)

marios


On 14/09/12 11:33, marios@redhat.com wrote:
> 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