This fixes the task #692.
Since this changed things in the factory and condor interfaces, please make sure that you do a full end-end testing (building a new image, uploading it to the cloud and starting an instance) when reviewing this.
From: Tomas Sedovic tsedovic@redhat.com
This fixes Redmine #692
We're building the credentials XML that's passed to Image Factory dynamically using the credential_definitions table now.
The advantage is that as we add new providers, we won't have to update the build_credentials function. We populate the credential_definitions with the new entries and that will be it. --- src/app/models/credential_definition.rb | 2 +- src/app/models/provider_account.rb | 35 +++++++++----------- ...31142700_change_credential_definitions_names.rb | 25 ++++++++++++++ src/db/seeds.rb | 14 ++++---- 4 files changed, 49 insertions(+), 27 deletions(-) create mode 100644 src/db/migrate/20110331142700_change_credential_definitions_names.rb
diff --git a/src/app/models/credential_definition.rb b/src/app/models/credential_definition.rb index b809425..556ebdf 100644 --- a/src/app/models/credential_definition.rb +++ b/src/app/models/credential_definition.rb @@ -20,5 +20,5 @@ class CredentialDefinition < ActiveRecord::Base validates_presence_of :label validates_presence_of :input_type validates_presence_of :provider_type_id - CREDENTIAL_DEFINITIONS_ORDER = ["username", "password", "account_id", "x509private", "x509public"] + CREDENTIAL_DEFINITIONS_ORDER = ['access_key', 'secret_access_key', 'account_number', 'key', 'certificate'] end diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index 33c7d2e..5ff9e7e 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -167,25 +167,22 @@ class ProviderAccount < ActiveRecord::Base end
def build_credentials - xml = Nokogiri::XML <<EOT -<?xml version="1.0"?> -<provider_credentials> - <ec2_credentials> - <account_number></account_number> - <access_key></access_key> - <secret_access_key></secret_access_key> - <certificate></certificate> - <key></key> - </ec2_credentials> -</provider_credentials> -EOT - node = xml.at_xpath('/provider_credentials/ec2_credentials') - node.at_xpath('./account_number').content = credentials_hash['account_id'] - node.at_xpath('./access_key').content = credentials_hash['username'] - node.at_xpath('./secret_access_key').content = credentials_hash['password'] - node.at_xpath('./certificate').content = credentials_hash['x509public'] - node.at_xpath('./key').content = credentials_hash['x509private'] - xml + doc = Nokogiri::XML('') + doc.root = Nokogiri::XML::Node.new('provider_credentials', doc) + root = doc.root.at_xpath('/provider_credentials') + + credential_node_name = provider.provider_type.codename + '_credentials' + credential_node = Nokogiri::XML::Node.new(credential_node_name, doc) + root << credential_node + + # The elements are sorted. That way we guarantee that the resulting XML is + # always the same which makes it easier to verify in tests. + credentials_hash.keys.sort.each do |k| + element = Nokogiri::XML::Node.new(k, doc) + element.content = credentials_hash[k] + credential_node << element + end + doc.to_xml end
def generate_auth_key diff --git a/src/db/migrate/20110331142700_change_credential_definitions_names.rb b/src/db/migrate/20110331142700_change_credential_definitions_names.rb new file mode 100644 index 0000000..2536a54 --- /dev/null +++ b/src/db/migrate/20110331142700_change_credential_definitions_names.rb @@ -0,0 +1,25 @@ +class ChangeCredentialDefinitionsNames < ActiveRecord::Migration + def self.up + CredentialDefinition.all.each do |cred| + cred.name = name_mapping[cred.name] + cred.save! + end + end + + def self.down + CredentialDefinition.all.each do |cred| + cred.name = name_mapping.invert[cred.name] + cred.save! + end + end + + def self.name_mapping + { + "username" => "access_key", + "password" => "secret_access_key", + "account_id" => "account_number", + "x509private" => "key", + "x509public" => "certificate", + } + end +end diff --git a/src/db/seeds.rb b/src/db/seeds.rb index 69be3f4..ab23138 100644 --- a/src/db/seeds.rb +++ b/src/db/seeds.rb @@ -126,16 +126,16 @@ end if CredentialDefinition.all.empty? ProviderType.all.each do |provider_type| unless provider_type.codename == 'ec2' - CredentialDefinition.create!(:name => 'username', :label => 'Username', :input_type => 'text', :provider_type_id => provider_type.id) - CredentialDefinition.create!(:name => 'password', :label => 'Password', :input_type => 'password', :provider_type_id => provider_type.id) + CredentialDefinition.create!(:name => 'access_key', :label => 'Username', :input_type => 'text', :provider_type_id => provider_type.id) + CredentialDefinition.create!(:name => 'secret_access_key', :label => 'Password', :input_type => 'password', :provider_type_id => provider_type.id) end end
#for ec2 provider type ec2 = ProviderType.find_by_codename 'ec2' - CredentialDefinition.create!(:name => 'username', :label => 'API Key', :input_type => 'text', :provider_type_id => ec2.id) - CredentialDefinition.create!(:name => 'password', :label => 'Secret', :input_type => 'password', :provider_type_id => ec2.id) - CredentialDefinition.create!(:name => 'account_id', :label => 'AWS Account ID', :input_type => 'text', :provider_type_id => ec2.id) - CredentialDefinition.create!(:name => 'x509private', :label => 'EC2 x509 private key', :input_type => 'file', :provider_type_id => ec2.id) - CredentialDefinition.create!(:name => 'x509public', :label => 'EC2 x509 public key', :input_type => 'file', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'access_key', :label => 'API Key', :input_type => 'text', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'secret_access_key', :label => 'Secret', :input_type => 'password', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'account_number', :label => 'AWS Account ID', :input_type => 'text', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'key', :label => 'EC2 x509 private key', :input_type => 'file', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'certificate', :label => 'EC2 x509 public key', :input_type => 'file', :provider_type_id => ec2.id) end
From: Tomas Sedovic tsedovic@redhat.com
The update to the build_credentials method broke a lot of tests. --- src/app/models/provider_account.rb | 10 +++++----- src/app/models/provider_account_observer.rb | 2 +- src/app/models/push_job.rb | 2 +- src/app/services/data_service_active_record.rb | 2 +- src/app/util/condormatic.rb | 4 ++-- src/app/views/admin/provider_accounts/_list.haml | 2 +- src/dbomatic/dbomatic | 2 +- src/features/provider_account.feature | 8 ++++---- .../provider_accounts_controller_spec.rb | 6 +++--- src/spec/factories/credential.rb | 14 +++++++------- src/spec/models/provider_account_spec.rb | 18 +++++++++--------- .../services/data_service_active_record_spec.rb | 6 +++--- 12 files changed, 38 insertions(+), 38 deletions(-)
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index 5ff9e7e..71b126c 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -41,7 +41,7 @@ class ProviderAccount < ActiveRecord::Base
searchable do text :name, :as => :code_substring - text(:username, :as => :code_substring) { credentials_hash['username'] } + text(:username, :as => :code_substring) { credentials_hash['access_key'] } end
# Relations @@ -83,7 +83,7 @@ class ProviderAccount < ActiveRecord::Base
def validate_unique_username cid = CredentialDefinition.find_by_name('username', :conditions => {:provider_type_id => provider.provider_type.id}) - Credential.all(:conditions => {:value => credentials_hash['username'], :credential_definition_id => cid}).each do |c| + Credential.all(:conditions => {:value => credentials_hash['access_key'], :credential_definition_id => cid}).each do |c| if c.provider_account.provider == self.provider && c.provider_account_id != self.id errors.add(:base, "Username has already been taken") return false @@ -122,7 +122,7 @@ class ProviderAccount < ActiveRecord::Base
def connect begin - return DeltaCloud.new(credentials_hash['username'], credentials_hash['password'], provider.url) + return DeltaCloud.new(credentials_hash['access_key'], credentials_hash['secret_access_key'], provider.url) rescue Exception => e logger.error("Error connecting to framework: #{e.message}") logger.error("Backtrace: #{e.backtrace.join("\n")}") @@ -138,7 +138,7 @@ class ProviderAccount < ActiveRecord::Base end
def name - label.blank? ? credentials_hash['username'] : label + label.blank? ? credentials_hash['access_key'] : label end
# FIXME: for already-mapped accounts, update rather than add new @@ -163,7 +163,7 @@ class ProviderAccount < ActiveRecord::Base end
def valid_credentials? - DeltaCloud::valid_credentials?(credentials_hash['username'].to_s, credentials_hash['password'].to_s, provider.url) + DeltaCloud::valid_credentials?(credentials_hash['access_key'].to_s, credentials_hash['secret_access_key'].to_s, provider.url) end
def build_credentials diff --git a/src/app/models/provider_account_observer.rb b/src/app/models/provider_account_observer.rb index 0f76ee4..b57f38e 100644 --- a/src/app/models/provider_account_observer.rb +++ b/src/app/models/provider_account_observer.rb @@ -15,7 +15,7 @@ class ProviderAccountObserver < ActiveRecord::Observer
def create_bucket(account) client = account.connect - bucket_name = "#{account.credentials_hash['account_id']}-imagefactory-amis" + bucket_name = "#{account.credentials_hash['account_number']}-imagefactory-amis" # TODO (jprovazn): getting particular bucket takes long time (core fetches all # buckets from provider), so we call directly create_bucket, if bucket exists, # exception should be thrown (actually existing bucket is returned - this diff --git a/src/app/models/push_job.rb b/src/app/models/push_job.rb index 9983953..e028b84 100644 --- a/src/app/models/push_job.rb +++ b/src/app/models/push_job.rb @@ -28,7 +28,7 @@ class PushJob < Struct.new(:provider_image_id, :hydra) # TODO: what if a provider has multiple accounts # for now pick first account provider_account = provider_image.provider.provider_accounts.first - cred_block = provider_account.build_credentials.to_xml.html_safe + cred_block = provider_account.build_credentials.html_safe response = RestClient.post(YAML.load_file("#{RAILS_ROOT}/config/image_factory_console.yml")['pushurl'], :image_id => provider_image.image.uuid, :provider => provider_image.provider.name, :credentials => cred_block diff --git a/src/app/services/data_service_active_record.rb b/src/app/services/data_service_active_record.rb index 0eb6ddd..6257977 100644 --- a/src/app/services/data_service_active_record.rb +++ b/src/app/services/data_service_active_record.rb @@ -69,7 +69,7 @@ class DataServiceActiveRecord provider_accounts.each do |provider_account| quota = provider_account.quota if quota - data_points << TotalQuotaUsagePoint.new(provider_account.credentials_hash['username'], quota.total_instances) + data_points << TotalQuotaUsagePoint.new(provider_account.credentials_hash['access_key'], quota.total_instances) free_instances += (quota.maximum_total_instances - quota.total_instances) end end diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb index 6a28657..63282fd 100644 --- a/src/app/util/condormatic.rb +++ b/src/app/util/condormatic.rb @@ -270,8 +270,8 @@ def condormatic_classads_sync pipe.puts "hardware_profile_cpu="#{overrides[:cpu]}"" pipe.puts "realm_key="#{realm ? realm.external_key : ''}"" pipe.puts "provider_url="#{account.provider.url}"" - pipe.puts "username="#{account.credentials_hash['username']}"" - pipe.puts "password="#{account.credentials_hash['password']}"" + pipe.puts "username="#{account.credentials_hash['access_key']}"" + pipe.puts "password="#{account.credentials_hash['secret_access_key']}"" pipe.puts "provider_account_id="#{account.id}"" pipe.puts "keypair="#{account.instance_key.name}"" rescue Exception => ex diff --git a/src/app/views/admin/provider_accounts/_list.haml b/src/app/views/admin/provider_accounts/_list.haml index b431f9b..86a1561 100644 --- a/src/app/views/admin/provider_accounts/_list.haml +++ b/src/app/views/admin/provider_accounts/_list.haml @@ -15,6 +15,6 @@ %input{:name => 'accounts_selected[]', :type => 'checkbox', :value => account.id, :id => "account_checkbox_#{account.id}", :checked => selected } = link_to account.name, admin_provider_account_path(account) %td - =account.credentials_hash['username'] + =account.credentials_hash['access_key'] %td =account.provider.provider_type.name diff --git a/src/dbomatic/dbomatic b/src/dbomatic/dbomatic index 1cd1229..9350e58 100755 --- a/src/dbomatic/dbomatic +++ b/src/dbomatic/dbomatic @@ -206,7 +206,7 @@ class CondorEventLog < Nokogiri::XML::SAX::Document return end
- provider_account = provider.provider_accounts.detect {|a| a.credentials_hash['username'] == @username} + provider_account = provider.provider_accounts.detect {|a| a.credentials_hash['access_key'] == @username} if provider_account.nil? @logger.error "Could not find the cloud account corresponding to #{link} - #{@username}, skipping cloud id update" return diff --git a/src/features/provider_account.feature b/src/features/provider_account.feature index 6a06d32..c15f353 100644 --- a/src/features/provider_account.feature +++ b/src/features/provider_account.feature @@ -24,8 +24,8 @@ Feature: Manage Provider Accounts And I should see "New Account" When I select "testprovider" from "provider_account_provider_id" And I fill in "provider_account[label]" with "testaccount" - And I fill in "provider_account[credentials_hash][username]" with "mockuser" - And I fill in "provider_account[credentials_hash][password]" with "mockpassword" + And I fill in "provider_account[credentials_hash][access_key]" with "mockuser" + And I fill in "provider_account[credentials_hash][secret_access_key]" with "mockpassword" And I fill in "quota[maximum_running_instances]" with "13" And I press "Add" Then I should be on testaccount's provider account page @@ -43,8 +43,8 @@ Feature: Manage Provider Accounts And I should see "New Account" When I select "testprovider" from "provider_account_provider_id" And I fill in "provider_account[label]" with "testaccount" - When I fill in "provider_account[credentials_hash][username]" with "mockuser" - And I fill in "provider_account[credentials_hash][password]" with "wrongpassword" + When I fill in "provider_account[credentials_hash][access_key]" with "mockuser" + And I fill in "provider_account[credentials_hash][secret_access_key]" with "wrongpassword" And I fill in "quota[maximum_running_instances]" with "13" And I press "Add" Then I should see "Credentials are invalid!" diff --git a/src/spec/controllers/provider_accounts_controller_spec.rb b/src/spec/controllers/provider_accounts_controller_spec.rb index c6887cf..9216fd9 100644 --- a/src/spec/controllers/provider_accounts_controller_spec.rb +++ b/src/spec/controllers/provider_accounts_controller_spec.rb @@ -38,13 +38,13 @@ describe Admin::ProviderAccountsController do
it "should allow users with account modify password to update a cloud account" do UserSession.create(@admin) - @provider_account.credentials_hash = {:username => 'mockuser2', :password => "foobar"} + @provider_account.credentials_hash = {:access_key => 'mockuser2', :secret_access_key => "foobar"} @provider_account.stub!(:valid_credentials?).and_return(true) @provider_account.quota = Quota.new @provider_account.save.should be_true - post :update, :id => @provider_account.id, :provider_account => { :credentials_hash => {:username => 'mockuser', :password => 'mockpassword'} } + post :update, :id => @provider_account.id, :provider_account => { :credentials_hash => {:access_key => 'mockuser', :secret_access_key => 'mockpassword'} } response.should redirect_to admin_provider_account_path(@provider_account) - ProviderAccount.find(@provider_account.id).credentials_hash['password'].should == "mockpassword" + ProviderAccount.find(@provider_account.id).credentials_hash['secret_access_key'].should == "mockpassword" end
it "should allow users with account modify permission to delete a cloud account" do diff --git a/src/spec/factories/credential.rb b/src/spec/factories/credential.rb index 8cde3f8..81175a8 100644 --- a/src/spec/factories/credential.rb +++ b/src/spec/factories/credential.rb @@ -6,36 +6,36 @@ end # EC2 credentials Factory.define :ec2_username_credential, :parent => :credential do |c| c.value "mockuser" - c.credential_definition { CredentialDefinition.find_by_name('username',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} + c.credential_definition { CredentialDefinition.find_by_name('access_key',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} end
Factory.define :ec2_password_credential, :parent => :credential do |c| c.value "mockpassword" - c.credential_definition { CredentialDefinition.find_by_name('password',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} + c.credential_definition { CredentialDefinition.find_by_name('secret_access_key',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} end
Factory.define :ec2_account_id_credential, :parent => :credential do |c| c.value "3141" - c.credential_definition { CredentialDefinition.find_by_name('account_id',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} + c.credential_definition { CredentialDefinition.find_by_name('account_number',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} end
Factory.define :ec2_x509private_credential, :parent => :credential do |c| c.value "x509 private key" - c.credential_definition { CredentialDefinition.find_by_name('x509private',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} + c.credential_definition { CredentialDefinition.find_by_name('key',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} end
Factory.define :ec2_x509public_credential, :parent => :credential do |c| c.value "x509 public key" - c.credential_definition { CredentialDefinition.find_by_name('x509public',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} + c.credential_definition { CredentialDefinition.find_by_name('certificate',:conditions => {:provider_type_id => ProviderType.find_by_codename('ec2')})} end
#Mock & Others credentials Factory.define :username_credential, :parent => :credential do |c| c.value "mockuser" - c.credential_definition { CredentialDefinition.find_by_name('username',:conditions => {:provider_type_id => ProviderType.find_by_codename('mock')})} + c.credential_definition { CredentialDefinition.find_by_name('access_key',:conditions => {:provider_type_id => ProviderType.find_by_codename('mock')})} end
Factory.define :password_credential, :parent => :credential do |c| c.value "mockpassword" - c.credential_definition { CredentialDefinition.find_by_name('password',:conditions => {:provider_type_id => ProviderType.find_by_codename('mock')})} + c.credential_definition { CredentialDefinition.find_by_name('secret_access_key',:conditions => {:provider_type_id => ProviderType.find_by_codename('mock')})} end diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb index 6845ed2..308b8a1 100644 --- a/src/spec/models/provider_account_spec.rb +++ b/src/spec/models/provider_account_spec.rb @@ -20,7 +20,7 @@ describe ProviderAccount do mock_provider = Factory :mock_provider
invalid_provider_account = Factory.build(:mock_provider_account, :provider => mock_provider) - invalid_provider_account.credentials_hash = {'username' => "wrong_username", 'password' => "wrong_password"} + invalid_provider_account.credentials_hash = {'access_key' => "wrong_username", 'secret_access_key' => "wrong_password"} invalid_provider_account.should_not be_valid
valid_provider_account = Factory.build(:mock_provider_account, :provider => mock_provider) @@ -29,7 +29,7 @@ describe ProviderAccount do
it "should fail to create a cloud account if the provider credentials are invalid" do provider_account = Factory.build(:mock_provider_account) - provider_account.credentials_hash = {'password' => "wrong_password"} + provider_account.credentials_hash = {'secret_access_key' => "wrong_password"} provider_account.save.should == false end
@@ -62,21 +62,21 @@ describe ProviderAccount do <?xml version="1.0"?> <provider_credentials> <ec2_credentials> - <account_number>1234</account_number> <access_key>user</access_key> - <secret_access_key>pass</secret_access_key> + <account_number>1234</account_number> <certificate>cert</certificate> <key>priv_key</key> + <secret_access_key>pass</secret_access_key> </ec2_credentials> </provider_credentials> EOT provider_account = Factory.build(:ec2_provider_account) provider_account.credentials_hash = { - 'username' => 'user', - 'password' => 'pass', - 'account_id' => '1234', - 'x509private' => 'priv_key', - 'x509public' => 'cert' + 'access_key' => 'user', + 'secret_access_key' => 'pass', + 'account_number' => '1234', + 'key' => 'priv_key', + 'certificate' => 'cert' } provider_account.build_credentials.to_s.should eql(expected_xml) end diff --git a/src/spec/services/data_service_active_record_spec.rb b/src/spec/services/data_service_active_record_spec.rb index d108e01..b1510ed 100644 --- a/src/spec/services/data_service_active_record_spec.rb +++ b/src/spec/services/data_service_active_record_spec.rb @@ -13,7 +13,7 @@ describe DataServiceActiveRecord do for i in 0..2 quota = Factory(:quota, :maximum_total_instances => data[i][0], :total_instances => data[i][1]) provider_account = Factory.build(:provider_account, :provider => provider, :quota => quota) - provider_account.credentials_hash = {:username => "username" + i.to_s, :password => 'mockpassword'} + provider_account.credentials_hash = {:access_key => "username" + i.to_s, :secret_access_key => 'mockpassword'} provider_account.stub!(:valid_credentials?).and_return(true) provider_account.save!
@@ -42,7 +42,7 @@ describe DataServiceActiveRecord do :total_instances => 20)
provider_account = Factory.build(:provider_account, :provider => provider, :quota => quota) - provider_account.credentials_hash = {:username => 'test', :password =>'test'} + provider_account.credentials_hash = {:access_key => 'test', :secret_access_key =>'test'} provider_account.stub!(:valid_credentials?).and_return(true) provider_account.save!
@@ -129,7 +129,7 @@ describe DataServiceActiveRecord do provider_accounts = [] expected_averages.each do |expected_average| provider_account = Factory.build(:provider_account, :provider => provider) - provider_account.credentials_hash = { :username => "username" + expected_average[0].to_s, :password => 'mockpassword' } + provider_account.credentials_hash = { :access_key => "username" + expected_average[0].to_s, :secret_access_key => 'mockpassword' } provider_account.stub!(:valid_credentials?).and_return(true) provider_account.save!
On Fri, 2011-04-01 at 14:08 +0200, tsedovic@redhat.com wrote:
This fixes the task #692.
Since this changed things in the factory and condor interfaces, please make sure that you do a full end-end testing (building a new image, uploading it to the cloud and starting an instance) when reviewing this.
If we are going to spin a build today, I would suggest we hold off pushing this until next week, once tested, as the possibility for breakage is a bit on the high side this close to a release, imo.
-j
aeolus-devel mailing list aeolus-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/aeolus-devel
aeolus-devel@lists.fedorahosted.org