Resending updated patch (didn't run full e2e test now as changes are minor). Not sending second patch which removes "create_bucket" call as I want to make sure this is done by imagefactory first (this patch is not required for rails3 patchset).
Jan
From: Jan Provaznik jprovazn@redhat.com
As condor matching process (when starting an instance) was moved to conductor we can simplify instance SSH key generation for EC2 accounts. SSH keys are always associated with an EC2 account. A user generates SSH key pair for the account and gets back ID for this key pair. Then when the user starts an instance he passes ID of the key pair generated before.
With matching in condor we didn't know where an instance will be started (which provider/provider account will condor choose) so we didn't know for which provider account new unique key pair should be generated. So we had to use a default key for starting the instance and replace the default key with new unique key when instance was started.
Now as matching is done on conductor side we know which provider account will be used so we can simply generate unique SSH key pair for the new instance before a launch request is passed to condor. Then we can remove instance observer and delayed_job and ssh key replacement logic. --- src/app/models/instance.rb | 33 +++++++------------- src/app/models/instance_key.rb | 29 ------------------ src/app/models/instance_observer.rb | 8 ----- src/app/models/provider_account.rb | 6 ---- src/app/models/provider_account_observer.rb | 3 -- src/app/util/condormatic.rb | 4 +- src/config/environment.rb | 1 - src/features/support/custom.rb | 10 ------ src/spec/factories/provider_account.rb | 3 -- src/spec/models/provider_account_observer_spec.rb | 23 -------------- src/spec/models/provider_account_spec.rb | 18 ----------- 11 files changed, 14 insertions(+), 124 deletions(-) delete mode 100644 src/spec/models/provider_account_observer_spec.rb
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb index 4700660..280eea9 100644 --- a/src/app/models/instance.rb +++ b/src/app/models/instance.rb @@ -228,27 +228,12 @@ class Instance < ActiveRecord::Base end end
- def create_unique_key - return unless self.provider_account and self.provider_account.instance_key - # deltacloud/ec2 api's create_key method returns only private part of - # key -> we don't know public part, so we generate new ssh key - # and replace whole authorized_keys file - key_name = "#{self.name}_rsa_#{Time.now.to_f}" - self.instance_key = InstanceKey.new(self.provider_account.instance_key.attributes.merge({ - :instance_key_owner => self, - :name => key_name - })) - begin - self.instance_key.replace_key(self.public_addresses, self.provider_account.instance_key.pem) - self.events.create!(:summary => "successfully updated ssh key", :event_time => Time.now) - rescue - msg = "failed to upload ssh key: #{$!}" - self.last_error = msg - self.events.create!(:summary => msg, :event_time => Time.now) - ensure - # if replace_key fails, we still save instance_key - should be copy of - # provider_account key which was used when launching instance - self.instance_key.save! + def create_auth_key + raise "instance provider_account is not set" unless self.provider_account + client = self.provider_account.connect + return nil unless client && client.feature?(:instances, :authentication_key) + if key = client.create_key(:name => key_name) + self.instance_key = InstanceKey.create!(:pem => key.pem.first, :name => key.id, :instance_key_owner => self) self.save! end end @@ -358,4 +343,10 @@ class Instance < ActiveRecord::Base named_scope :with_hardware_profile, lambda { {:include => :hardware_profile} } + + private + + def key_name + "#{self.name}_#{Time.now.to_i}_key_#{self.object_id}".gsub(/[^a-zA-Z0-9.-]/, '_') + end end diff --git a/src/app/models/instance_key.rb b/src/app/models/instance_key.rb index 0246c9e..bec06ab 100644 --- a/src/app/models/instance_key.rb +++ b/src/app/models/instance_key.rb @@ -34,36 +34,7 @@ # Likewise, all the methods added will be available for all controllers. #
-require 'openssl' -require 'base64' - class InstanceKey < ActiveRecord::Base
belongs_to :instance_key_owner, :polymorphic => true - - def replace_key(addr, old_pem) - key = generate_ssh_key - replace_on_server(addr, old_pem, key[:public]) - self.pem = key[:private] - end - - private - - def replace_on_server(addr, old_pem, new_pub) - provider_type = self.instance_key_owner.provider_account.provider.provider_type - Net::SCP::start(addr, provider_type.ssh_user, :key_data => [old_pem], :keys => []) do |scp| - scp.upload! StringIO.new(new_pub), File.join(provider_type.home_dir, '/.ssh/authorized_keys') - end - end - - def generate_ssh_key - key = OpenSSL::PKey::RSA.generate(1024) - writer = Net::SSH::Buffer.new - writer.write_key key - ssh_key = Base64.encode64( writer.to_s ).strip.gsub( /[\n\r\t ]/, "" ) - { - :private => key.export, - :public => "#{key.ssh_type} #{ssh_key} #{ENV['USER']}@#{ENV['HOSTNAME']}" - } - end end diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb index c71454a..e80bd40 100644 --- a/src/app/models/instance_observer.rb +++ b/src/app/models/instance_observer.rb @@ -68,14 +68,6 @@ class InstanceObserver < ActiveRecord::Observer end
def after_update(instance) - # we try to generate unique key only when instance is running - # and provider_account for this instance has instance_key (provider account - # instance_key is used as default ssh key when instance is launched) - if instance.state_changed? and instance.state == Instance::STATE_RUNNING and - not instance.instance_key and instance.provider_account and instance.provider_account.instance_key - instance.delay.create_unique_key - end - if instance.state_changed? event = Event.new(:source => instance, :event_time => DateTime.now, :summary => "state changed to #{instance.state}") diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb index 02fdf1d..e817579 100644 --- a/src/app/models/provider_account.rb +++ b/src/app/models/provider_account.rb @@ -201,12 +201,6 @@ class ProviderAccount < ActiveRecord::Base label_value_pairs.sort { |a, b| a[:label] <=> b[:label] } end
- def generate_auth_key - client = connect - return nil unless client && client.feature?(:instances, :authentication_key) - client.create_key(:name => "#{self.name}_#{Time.now.to_i}_key_#{self.object_id}") - end - def credentials_hash @credentials_hash = {} # Credential.all(:conditions => {:provider_account_id => id}, :include => :credential_definition).each do |cred| diff --git a/src/app/models/provider_account_observer.rb b/src/app/models/provider_account_observer.rb index fd5b65a..e4b8288 100644 --- a/src/app/models/provider_account_observer.rb +++ b/src/app/models/provider_account_observer.rb @@ -6,9 +6,6 @@ class ProviderAccountObserver < ActiveRecord::Observer if account.provider.provider_type_id == ProviderType.find_by_codename("ec2").id create_bucket(account) end - if key = account.generate_auth_key - account.update_attribute(:instance_key, InstanceKey.create!(:pem => key.pem.first, :name => key.id, :instance_key_owner => account)) - end account.populate_hardware_profiles end
diff --git a/src/app/util/condormatic.rb b/src/app/util/condormatic.rb index b52ff46..431697a 100644 --- a/src/app/util/condormatic.rb +++ b/src/app/util/condormatic.rb @@ -75,12 +75,12 @@ def condormatic_instance_create(task)
overrides = HardwareProfile.generate_override_property_values(instance.hardware_profile, found.hwp) - keyname = found.account.instance_key ? found.account.instance_key.name : '' - pwfilename = write_pw_file(job_name, found.account.credentials_hash['password'])
instance.provider_account = found.account + instance.create_auth_key unless instance.instance_key + keyname = instance.instance_key ? instance.instance_key.name : ''
# I use the 2>&1 to get stderr and stdout together because popen3 does not # support the ability to get the exit value of the command in ruby 1.8. diff --git a/src/config/environment.rb b/src/config/environment.rb index eabb8a8..523bb0b 100644 --- a/src/config/environment.rb +++ b/src/config/environment.rb @@ -51,7 +51,6 @@ Rails::Initializer.run do |config| config.gem 'rest-client', :version => '>= 1.6.1' config.gem 'rack-restful_submit', :version => '1.1.2' config.gem 'delayed_job', :version => '~>2.0.4' - config.gem 'net-scp', :lib => 'net/scp' config.gem 'uuidtools'
config.middleware.swap Rack::MethodOverride, 'Rack::RestfulSubmit' diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb index 2f6c459..27e5ed1 100644 --- a/src/features/support/custom.rb +++ b/src/features/support/custom.rb @@ -11,20 +11,10 @@ end
ProviderAccount.class_eval do
- alias :generate_auth_key_original :generate_auth_key - def valid_credentials? credentials_hash['username'].to_s == 'mockuser' && credentials_hash['password'].to_s == 'mockpassword' end
- def generate_auth_key - key = OpenStruct.new(:pem => 'PEM') - def key.id - "mock_#{Time.now.to_i}_key_#{self.object_id}" - end - key - end - # def instance_key # @key = mock('Key', :null_object => true) # @key.stub!(:pem).and_return("PEM") diff --git a/src/spec/factories/provider_account.rb b/src/spec/factories/provider_account.rb index d3944bd..28006e9 100644 --- a/src/spec/factories/provider_account.rb +++ b/src/spec/factories/provider_account.rb @@ -2,9 +2,6 @@ Factory.define :provider_account do |f| f.sequence(:label) { |n| "test label#{n}" } f.association :provider f.association :quota - f.after_build do |acc| - acc.stub!(:generate_auth_key).and_return(nil) if acc.respond_to?(:stub!) - end end
Factory.define :mock_provider_account, :parent => :provider_account do |f| diff --git a/src/spec/models/provider_account_observer_spec.rb b/src/spec/models/provider_account_observer_spec.rb deleted file mode 100644 index 6b7d62a..0000000 --- a/src/spec/models/provider_account_observer_spec.rb +++ /dev/null @@ -1,23 +0,0 @@ -require 'spec_helper' - -describe ProviderAccountObserver do - fixtures :all - - it "should create an instance_key if provider is EC2" do - @client = mock('Conductor', :null_object => true) - @provider = Factory.build :ec2_provider - @key = mock('Key', :null_object => true) - @key.stub!(:pem).and_return("PEM") - @key.stub!(:id).and_return("1_user") - @client.stub!(:"feature?").and_return(true) - @client.stub!(:"create_key").and_return(@key) - - provider_account = Factory.build :ec2_provider_account - provider_account.stub!(:connect).and_return(@client) - provider_account.stub!(:generate_auth_key).and_return(@key) - provider_account.save! - provider_account.instance_key.should_not == nil - provider_account.instance_key.pem == "PEM" - provider_account.instance_key.id == "1_user" - end -end diff --git a/src/spec/models/provider_account_spec.rb b/src/spec/models/provider_account_spec.rb index 370a132..2ef563f 100644 --- a/src/spec/models/provider_account_spec.rb +++ b/src/spec/models/provider_account_spec.rb @@ -41,24 +41,6 @@ describe ProviderAccount do provider_account.save.should == false end
- it "should create an instance_key if provider is EC2" do - @client = mock('Conductor', :null_object => true) - @provider = Factory.build :ec2_provider - @key = mock('Key', :null_object => true) - @key.stub!(:pem).and_return("PEM") - @key.stub!(:id).and_return("1_user") - @client.stub!(:"feature?").and_return(true) - @client.stub!(:"create_key").and_return(@key) - - provider_account = Factory.build :ec2_provider_account - provider_account.stub!(:connect).and_return(@client) - provider_account.stub!(:generate_auth_key).and_return(@key) - provider_account.save - provider_account.instance_key.should_not == nil - provider_account.instance_key.pem == "PEM" - provider_account.instance_key.id == "1_user" - end - it "when calling connect and it fails with exception it will return nil" do DeltaCloud.should_receive(:new).and_raise(Exception.new)
On Thu, 2011-07-28 at 15:27 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
As condor matching process (when starting an instance) was moved to conductor we can simplify instance SSH key generation for EC2 accounts. SSH keys are always associated with an EC2 account. A user generates SSH key pair for the account and gets back ID for this key pair. Then when the user starts an instance he passes ID of the key pair generated before.
With matching in condor we didn't know where an instance will be started (which provider/provider account will condor choose) so we didn't know for which provider account new unique key pair should be generated. So we had to use a default key for starting the instance and replace the default key with new unique key when instance was started.
Now as matching is done on conductor side we know which provider account will be used so we can simply generate unique SSH key pair for the new instance before a launch request is passed to condor. Then we can remove instance observer and delayed_job and ssh key replacement logic.
Looks great to me now Jan, ACK
Cheers, Mark.
On 07/28/11 - 04:13:54PM, Mark McLoughlin wrote:
On Thu, 2011-07-28 at 15:27 +0200, jprovazn@redhat.com wrote:
From: Jan Provaznik jprovazn@redhat.com
As condor matching process (when starting an instance) was moved to conductor we can simplify instance SSH key generation for EC2 accounts. SSH keys are always associated with an EC2 account. A user generates SSH key pair for the account and gets back ID for this key pair. Then when the user starts an instance he passes ID of the key pair generated before.
With matching in condor we didn't know where an instance will be started (which provider/provider account will condor choose) so we didn't know for which provider account new unique key pair should be generated. So we had to use a default key for starting the instance and replace the default key with new unique key when instance was started.
Now as matching is done on conductor side we know which provider account will be used so we can simply generate unique SSH key pair for the new instance before a launch request is passed to condor. Then we can remove instance observer and delayed_job and ssh key replacement logic.
Looks great to me now Jan, ACK
Thanks guys, I've pushed this now.
aeolus-devel@lists.fedorahosted.org
-
Chris Lalancette -
Jan Provazník -
Mark McLoughlin