On Fri, 2011-04-15 at 17:56 +0200, tsedovic(a)redhat.com wrote:
From: Tomas Sedovic <tsedovic(a)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.
---
OK, so, parts of this look good like the updated build_credentials
method, but I think there was a misunderstanding on some other parts
(and this will affect the 2/2 patch, so I will not comment on that
directly). I'll explain inline, so hopefully it makes more sense.
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 9858219..c6f0079 100644
--- a/src/app/models/provider_account.rb
+++ b/src/app/models/provider_account.rb
@@ -170,25 +170,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)
The old values for 'name' here were almost correct in the old version
(name and label are the reverse of what I designed, but otherwise had
approximately correct values). I'll try to illustrate what is desired
here with an example.
'username' is what dc api always uses, no matter what the provider is.
EC2 may call it 'Access Key', but core still expects the access key
value to be in the username field on the core http auth form. So, _if_
our form in the conductor view is using CredentialDefiniton.name as the
id for the form fields, the id in this case should be 'username'. The
CredentialDefiniton.label value should be used as the label _for that
field in the form_, so in this instance 'Access Key'.
Now for the next level down. What we need to do when building the xml
to send to the factory, is to transform (on the fly) 'Access Key' into
'access_key', which is (one of) the xml key(s) that the factory will
parse out and use to build the actual image. The old xml in the
build_credentials method is a good example of what is expected for ec2,
so the final version of this should output exactly that when the method
is called (which you do seem to have right now). Other providers may
have some unknowns as far as what all these values are, but the values
can easily be added as we start using/building for another provider.
The main thing here is the convention factory is expecting is a tag with
whatever the provider calls this api key _transformed_ to lowercase,
with each word separated by an underscore. I am starting to wonder if
perhaps it would be clearer if the definition fields were call core_name
and provider_name, but maybe that would be confusing for the credentials
core does not give us.
I hope this makes more sense, and should actually not be a lot of work
to update your implementation to work this way, as you already have a
large chunk correct of what is needed.
-j
end