[PATCH Conductor] Fix for https://bugzilla.redhat.com/show_bug.cgi?id=760377

gblomqui at redhat.com gblomqui at redhat.com
Fri Dec 16 16:42:16 UTC 2011


From: Greg Blomquist <gblomqui at redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=760377

Conductor no longer uses RestClient to connect to config server.  Instead is
uses the OAuth client library directly.  RestClient was not correctly signing
the entire post body when submitting an HTTP POST request to the Config Server.
The OAuth server library expected the entire post body to be signed.
---
 src/app/models/config_server.rb                |   72 +++++++++++-------------
 src/config/initializers/config_server_oauth.rb |   33 -----------
 src/spec/factories/config_server.rb            |   15 ++++-
 src/spec/models/deployment_spec.rb             |   25 ++++++++
 src/spec/models/instance_spec.rb               |   23 --------
 5 files changed, 71 insertions(+), 97 deletions(-)
 delete mode 100644 src/config/initializers/config_server_oauth.rb

diff --git a/src/app/models/config_server.rb b/src/app/models/config_server.rb
index 6444ee0..ab97e9b 100644
--- a/src/app/models/config_server.rb
+++ b/src/app/models/config_server.rb
@@ -84,34 +84,31 @@ class ConfigServer < ActiveRecord::Base
   # generated when testing the connection.
   def connection_valid?
     if status.untested? or (changed? and (changes.keys & @@status_fields).empty?)
+      error_str = nil
       begin
-        test_connection
-        status.success!
-      rescue => e
-        error_str = map_connection_exception_to_error(e)
-        status.fail!(error_str)
+        response = test_connection
+        if "200" == response.code
+          status.success!
+        else
+          error_str = map_response_to_error(response)
+        end
+      rescue => exception
+        error_str = map_exception_to_error(exception)
       end
+      status.fail!(error_str) if error_str
     end
     status.success?
   end
 
   def send_config(instance_config)
-    url = "#{endpoint}/configs/#{API_VERSION}/#{instance_config.uuid}"
-    args = get_connection_args(url, "post")
-    data = CGI::escape(instance_config.to_s)
-    args[:payload] = "data=#{data}"
-    RestClient::Request.execute(args)
+    uri = "/configs/#{API_VERSION}/#{instance_config.uuid}"
+    body = {:data => instance_config.to_s}
+    oauth.post(uri, body)
   end
 
   def delete_deployment_config(deployment_uuid)
-    url = "#{endpoint}/deployment/#{API_VERSION}/#{deployment_uuid}"
-    args = get_connection_args(url, "delete")
-    begin
-      RestClient::Request.execute(args)
-    rescue RestClient::ResourceNotFound => e
-      # allow for 404s, this means that the configs don't exist on this config
-      # server
-    end
+    uri = "/deployment/#{API_VERSION}/#{deployment_uuid}"
+    oauth.delete(uri)
   end
 
   private
@@ -127,32 +124,29 @@ class ConfigServer < ActiveRecord::Base
     end
   end
 
-  def get_connection_args(url, method="get")
-    args = {:method => method.to_sym}
-    args[:url] = url
-    # the :config_server_oauth parameter is inspected by one of the
-    # RestClient#before_execution_procs that gets added in
-    # config/initializers/config_server_oauth.rb.
-    args[:config_server_oauth] = true
-    args[:consumer_key] = key
-    args[:consumer_secret] = secret
-    args
+  def consumer
+    OAuth::Consumer.new(key, secret, :site => endpoint)
+  end
+
+  def oauth
+    OAuth::AccessToken.new(consumer)
   end
 
-  # Test the connection to this config server.  Return nil on success, or throw
-  # an exception on errors.  See
-  # http://rubydoc.info/gems/rest-client/1.6.3/RestClient#STATUSES-constant
-  # for more information on the types of exceptions.
+  # Test the connection to this config server.
+  # Return the http response
   def test_connection
-    args = get_connection_args("#{endpoint}/auth")
-    args[:raw_response] = true
-    RestClient::Request.execute(args)
+    oauth.get("/auth")
+  end
+
+  def map_response_to_error(response)
+    msg = "#{response.code}: #{response.message}"
+    error_string = I18n.translate("config_servers.errors.connection.generic_with_message", :url => endpoint, :msg => msg)
   end
 
-  def map_connection_exception_to_error(ex)
-    if ex.kind_of?(RestClient::ExceptionWithResponse) or ex.kind_of?(RestClient::Exception) or ex.class == Errno::ETIMEDOUT
-      error_string = I18n.translate("config_servers.errors.connection.generic_with_message", :url => endpoint, :msg => ex.message)
-    elsif not ex.nil?
+  def map_exception_to_error(exception)
+    if [Errno::ETIMEDOUT, Errno::ECONNREFUSED].include? exception.class
+      error_string = I18n.translate("config_servers.errors.connection.generic_with_message", :url => endpoint, :msg => exception.message)
+    else
       error_string = I18n.translate("config_servers.errors.connection.generic", :url => endpoint)
     end
   end
diff --git a/src/config/initializers/config_server_oauth.rb b/src/config/initializers/config_server_oauth.rb
deleted file mode 100644
index 9bad1a7..0000000
--- a/src/config/initializers/config_server_oauth.rb
+++ /dev/null
@@ -1,33 +0,0 @@
-#
-#   Copyright 2011 Red Hat, Inc.
-#
-#   Licensed under the Apache License, Version 2.0 (the "License");
-#   you may not use this file except in compliance with the License.
-#   You may obtain a copy of the License at
-#
-#       http://www.apache.org/licenses/LICENSE-2.0
-#
-#   Unless required by applicable law or agreed to in writing, software
-#   distributed under the License is distributed on an "AS IS" BASIS,
-#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-#   See the License for the specific language governing permissions and
-#   limitations under the License.
-#
-
-# The :config_server_oauth parameter is added in the config_server model object
-# just before executing each request against a config server.
-# This is a little scary, since this proc is added at the global RestClient
-# scope.  The :config_server_oauth parameter should keep any other RestClient
-# library users from having this block accidentally executed.
-#
-RestClient.add_before_execution_proc do |request, params|
-  if params.key?(:config_server_oauth)
-    consumer = OAuth::Consumer.new(
-      params[:consumer_key],
-      params[:consumer_secret],
-      :site => params[:url]
-    )
-    access_token = OAuth::AccessToken.new(consumer)
-    access_token.sign!(request)
-  end
-end
diff --git a/src/spec/factories/config_server.rb b/src/spec/factories/config_server.rb
index 989f842..82fa572 100644
--- a/src/spec/factories/config_server.rb
+++ b/src/spec/factories/config_server.rb
@@ -13,6 +13,15 @@
 #   See the License for the specific language governing permissions and
 #   limitations under the License.
 #
+class FakeResponse
+  attr_reader :code, :message
+  def initialize(code, message)
+    @code = code
+    @message = message
+  end
+end
+
+
 Factory.define :config_server do |f|
   f.sequence(:endpoint) {|n| "config_server#{n}" }
 end
@@ -26,7 +35,8 @@ end
 Factory.define :mock_config_server, :parent => :base_config_server do |f|
   f.association :provider_account, :factory => :mock_provider_account
   f.after_build do |cs|
-    cs.stub!(:test_connection).and_return(nil) if cs.respond_to? :stub!
+    response = FakeResponse.new("200", "OK")
+    cs.stub!(:test_connection).and_return(response) if cs.respond_to? :stub!
   end
 end
 
@@ -38,7 +48,8 @@ Factory.define :invalid_credentials_config_server, :parent => :base_config_serve
     # the default_strategy initialization parameter seemed much better
   end
   f.after_build do |cs|
-    cs.stub!(:test_connection).and_raise(RestClient::Unauthorized) if cs.respond_to? :stub!
+    response = FakeResponse.new("401", "Unauthorized")
+    cs.stub!(:test_connection).and_return(response) if cs.respond_to? :stub!
   end
 end
 
diff --git a/src/spec/models/deployment_spec.rb b/src/spec/models/deployment_spec.rb
index 1a60dd7..39ee155 100644
--- a/src/spec/models/deployment_spec.rb
+++ b/src/spec/models/deployment_spec.rb
@@ -243,4 +243,29 @@ describe Deployment do
     d.stub(:instances){[instance]}
     d.instances[0].instance_parameters.count.should >= 0
   end
+
+  it "should find a single provider account to launch" do
+    account1 = FactoryGirl.create(:mock_provider_account, :label => "test_account1")
+    account2 = FactoryGirl.create(:mock_provider_account, :label => "test_account2")
+    account3 = FactoryGirl.create(:mock_provider_account, :label => "test_account3")
+    possible1 = Instance::Match.new(nil,account1,nil,nil,nil)
+    possible2 = Instance::Match.new(nil,account2,nil,nil,nil)
+    possible3 = Instance::Match.new(nil,account2,nil,nil,nil)
+    possible4 = Instance::Match.new(nil,account3,nil,nil,nil)
+    possible5 = Instance::Match.new(nil,account2,nil,nil,nil)
+
+    # not gonna test the individual instance "machtes" logic again
+    # just stub out the behavior
+    instance1 = Factory.build(:instance)
+    instance1.stub!(:matches).and_return([[possible1, possible2], []])
+    instance2 = Factory.build(:instance)
+    instance2.stub!(:matches).and_return([[possible3, possible4], []])
+    instance3 = Factory.build(:instance)
+    instance3.stub!(:matches).and_return([[possible5], []])
+
+    instances = [instance1, instance2, instance3]
+    matches, errors = @deployment.send(:common_provider_accounts_for, instances)
+    matches.should_not be_empty
+    matches.keys.first.should eql(account2)
+  end
 end
diff --git a/src/spec/models/instance_spec.rb b/src/spec/models/instance_spec.rb
index a1fe525..217f33f 100644
--- a/src/spec/models/instance_spec.rb
+++ b/src/spec/models/instance_spec.rb
@@ -299,27 +299,4 @@ describe Instance do
     errors.should_not be_empty
     errors.select {|e| e.include?("no config server available") }.should_not be_empty
   end
-
-  it "should match only the intersecting provider accounts for all instances" do
-    account1 = FactoryGirl.create(:mock_provider_account, :label => "test_account1")
-    possible1 = Instance::Match.new(nil,account1,nil,nil,nil)
-    account2 = FactoryGirl.create(:mock_provider_account, :label => "test_account2")
-    possible2 = Instance::Match.new(nil,account2,nil,nil,nil)
-    account3 = FactoryGirl.create(:mock_provider_account, :label => "test_account3")
-    possible3 = Instance::Match.new(nil,account3,nil,nil,nil)
-
-    # not gonna test the individual instance "machtes" logic again
-    # just stub out the behavior
-    instance1 = Factory.build(:instance)
-    instance1.stub!(:matches).and_return([[possible1, possible2], []])
-    instance2 = Factory.build(:instance)
-    instance2.stub!(:matches).and_return([[possible2, possible3], []])
-    instance3 = Factory.build(:instance)
-    instance3.stub!(:matches).and_return([[possible2], []])
-
-    instances = [instance1, instance2, instance3]
-    matches, errors = Instance.matches(instances)
-    matches.should_not be_empty
-    matches.first.provider_account.should eql(account2)
-  end
 end
-- 
1.7.7.4




More information about the aeolus-devel mailing list