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.
Fix according to jay's suggestions
Fix the tests --- src/app/models/provider_account.rb | 44 +++++++++++-------- src/app/models/push_job.rb | 2 +- ...19142700_change_credential_definitions_names.rb | 30 +++++++++++++ src/db/seeds.rb | 10 ++-- src/spec/models/provider_account_spec.rb | 4 +- 5 files changed, 63 insertions(+), 27 deletions(-) create mode 100644 src/db/migrate/20110419142700_change_credential_definitions_names.rb
diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index 9858219..b1c1bb8 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -170,25 +170,31 @@ 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 + + creds_label_hash.each do |h| + element = Nokogiri::XML::Node.new(h[:label], doc) + element.content = h[:value] + credential_node << element + end + doc.to_xml + end + + def creds_label_hash + label_value_pairs = credentials.map do |c| + { :label => c.credential_definition.label.downcase.split.join('_'), + :value => c.value } + end + + # The list is ordered by labels. That way we guarantee that the resulting + # XML is always the same which makes it easier to verify in tests. + label_value_pairs.sort { |a, b| a[:label] <=> b[:label] } end
def generate_auth_key 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/db/migrate/20110419142700_change_credential_definitions_names.rb b/src/db/migrate/20110419142700_change_credential_definitions_names.rb new file mode 100644 index 0000000..a237e9e --- /dev/null +++ b/src/db/migrate/20110419142700_change_credential_definitions_names.rb @@ -0,0 +1,30 @@ +class ChangeCredentialDefinitionsNames < ActiveRecord::Migration + def self.up + CredentialDefinition.all.each do |cred| + if name_mapping.has_key? cred.label + cred.label = name_mapping[cred.label] + cred.save! + end + end + end + + def self.down + reverse_mapping = name_mapping.invert + CredentialDefinition.all.each do |cred| + if reverse_mapping.has_key? cred.label + cred.label = reverse_mapping[cred.label] + cred.save! + end + end + end + + def self.name_mapping + { + "API Key" => "Access Key", + "Secret" => "Secret Access Key", + "AWS Account ID" => "Account Number", + "EC2 x509 private key" => "Key", + "EC2 x509 public key" => "Certificate", + } + end +end diff --git a/src/db/seeds.rb b/src/db/seeds.rb index 69be3f4..363a08f 100644 --- a/src/db/seeds.rb +++ b/src/db/seeds.rb @@ -133,9 +133,9 @@ if CredentialDefinition.all.empty?
#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 => 'username', :label => 'Access Key', :input_type => 'text', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'password', :label => 'Secret Access Key', :input_type => 'password', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'account_id', :label => 'Account Number', :input_type => 'text', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'x509private', :label => 'Key', :input_type => 'file', :provider_type_id => ec2.id) + CredentialDefinition.create!(:name => 'x509public', :label => 'Certificate', :input_type => 'file', :provider_type_id => ec2.id) end diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb index df49708..2c3c090 100644 --- a/src/spec/models/provider_account_spec.rb +++ b/src/spec/models/provider_account_spec.rb @@ -68,11 +68,11 @@ 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
On Tue, 2011-04-19 at 16:47 +0200, tsedovic@redhat.com wrote:
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.
Fix according to jay's suggestions
Ok, the xml generation part of this patch is now perfect. I do have a couple minor problems with the migration though.
* There should also be an update to seeds.rb, so an initial run of this works w/o the migration. * The migration should conditionally run depending on if data is there, as we have done in other migrations of late. * The migration did not seem to work correctly on my database with existing providers. When I did a db:migrate, then checked the console for what xml was generated with build_credentials, I still got the old values, so instead of 'access_key', the xml generated 'username'. When I dropped the db and ran the migrations from scratch, it seemed to work as expected.
Lastly, I talked to Ian on the factory team (and scott on conductor side), and I think the easiest solution to the 'key' and 'credential' names being unintelligible to a user is to change the value in our db, and have factory look for that new name in the xml as well. The net of this is, leave the confusing name for now, in the next sprint we will have the new xml expected on factory side, and we will have a task to update the value (so the patch will be basically a migration).
So, hate to say it, but I think this will need one more rev to get the migration/seed bit right, but it is really just about there.
-j
On 04/20/2011 11:26 PM, Jason Guiditta wrote:
On Tue, 2011-04-19 at 16:47 +0200, tsedovic@redhat.com wrote:
From: Tomas Sedovictsedovic@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.
Fix according to jay's suggestions
Ok, the xml generation part of this patch is now perfect. I do have a couple minor problems with the migration though.
- There should also be an update to seeds.rb, so an initial run of this
works w/o the migration.
- The migration should conditionally run depending on if data is there,
as we have done in other migrations of late.
- The migration did not seem to work correctly on my database with
existing providers. When I did a db:migrate, then checked the console for what xml was generated with build_credentials, I still got the old values, so instead of 'access_key', the xml generated 'username'. When I dropped the db and ran the migrations from scratch, it seemed to work as expected.
Lastly, I talked to Ian on the factory team (and scott on conductor side), and I think the easiest solution to the 'key' and 'credential' names being unintelligible to a user is to change the value in our db, and have factory look for that new name in the xml as well. The net of this is, leave the confusing name for now, in the next sprint we will have the new xml expected on factory side, and we will have a task to update the value (so the patch will be basically a migration).
So, hate to say it, but I think this will need one more rev to get the migration/seed bit right, but it is really just about there.
-j
Thanks for looking at the patch Jay.
I must say I'm surprised by your experience however. The patch does include an update to the seeds.rb and the migrations looked and seemed to work right for me.
What do you mean by running the migrations conditionally? The code in this patch leaves any records that don't have the old values untouched. I tried to look at other migrations we have to see if there's something else I should have done but I didn't find anything.
Anyway, I tested the whole thing again to try and replicate your behaviour but everything worked as it should.
For the record, here's what I did:
First, I installed Conductor RPMs without this patch, run aeolus-configure and added EC2 and mock provider accounts.
The data and behaviour was as expected.
Then I built and installed RPMs with the patch included and run db:migrate in the production environment. I did not run aeolus-cleanup or configure so this should test just the migration code.
When looked at into the production console, it was as it should -- the CredentialDefinition records had their `label` fields updated and the `build_credential` method returned proper XML.
So then I run aeolus-cleanup followed by aeolus-configure which should invoke the seeds and run all the migrations.
Again, everything was as it should -- the CredentialDefinition.label fields were of the new value and the credential XMLs correct.
Lastly, I did db:clean, db:create and db:setup in my development environment. If I understand it correctly, this should invoke the database seed without running the migrations.
After adding the provider accounts, everything was working correctly again.
I'll be the last person to assert that my code is bug-free and I did look at the code and tried to see where it may be failing, but I couldn't find anything.
Could you please take a look at it again and see if you can notice anything being amiss?
Thanks a lot, Thomas
On Fri, 2011-04-22 at 14:01 +0200, Tomas Sedovic wrote:
On 04/20/2011 11:26 PM, Jason Guiditta wrote:
On Tue, 2011-04-19 at 16:47 +0200, tsedovic@redhat.com wrote:
From: Tomas Sedovictsedovic@redhat.com
Thanks for looking at the patch Jay.
I must say I'm surprised by your experience however. The patch does include an update to the seeds.rb and the migrations looked and seemed to work right for me.
What do you mean by running the migrations conditionally? The code in this patch leaves any records that don't have the old values untouched. I tried to look at other migrations we have to see if there's something else I should have done but I didn't find anything.
Anyway, I tested the whole thing again to try and replicate your behaviour but everything worked as it should.
Tomas, I am unable to replicate the behavior I saw either, so I think we can ignore it. You are right on the other points, not sure what I was thinking there, so ACK.
-j
aeolus-devel@lists.fedorahosted.org