Hi Jan,
I thought I was just going to ACK this, but I think there's enough small
issues below to warrant re-spinning it
On Wed, 2011-07-27 at 04:35 -0400, Chris Lalancette wrote:
From: Jan Provaznik <jprovazn(a)redhat.com>
I'd like to see your nice explanation referenced in the commit log:
https://fedorahosted.org/pipermail/aeolus-devel/2011-June/002518.html
Also, you mentioned that we still need to:
- remove polymorphic instance key association (not needed for provider account
anymore)
- delete key (on provider side) on instance stop
I'm fine with you leaving those until later, but please file redmine
tasks for them so that they aren't forgotten.
---
src/app/models/instance.rb | 35 ++++++++---------------------
src/app/models/instance_key.rb | 26 ----------------------
src/app/models/instance_observer.rb | 8 ------
src/app/models/provider_account.rb | 6 -----
src/app/util/condormatic.rb | 5 +--
src/features/support/custom.rb | 10 --------
src/spec/factories/provider_account.rb | 3 --
src/spec/models/provider_account_spec.rb | 2 -
8 files changed, 12 insertions(+), 83 deletions(-)
diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
index 4700660..af5ec85 100644
--- a/src/app/models/instance.rb
+++ b/src/app/models/instance.rb
@@ -228,31 +228,6 @@ 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!
- self.save!
- end
- end
-
def self.get_user_instances_stats(user)
stats = {
:running_instances => 0,
@@ -355,6 +330,16 @@ class Instance < ActiveRecord::Base
read_attribute(:public_addresses)
end
+ 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)
+ kname =
"#{self.name}_#{Time.now.to_i}_key_#{self.object_id}".gsub(/[^a-zA-Z0-9\.\-]/,
'_')
Would it be cleaner to split that out into a key_name helper method?
+ if key = client.create_key(:name => kname)
+ self.update_attribute(:instance_key, InstanceKey.create!(:pem => key.pem.first,
:name => key.id, :instance_key_owner => self))
Why update_attribute instead of:
self.instance_key = InstanceKey.create!(:pem => key.pem.first, :name => key.id,
:instance_key_owner => self)
self.save!
+ end
+ end
+
named_scope :with_hardware_profile, lambda {
{:include => :hardware_profile}
}
diff --git a/src/app/models/instance_key.rb b/src/app/models/instance_key.rb
index 0246c9e..d19c422 100644
--- a/src/app/models/instance_key.rb
+++ b/src/app/models/instance_key.rb
@@ -40,30 +40,4 @@ require 'base64'
You can remove the base64 and openssl requires now
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
It's awesome to be getting rid of this mess
You should also eliminate the requirement for the net-scp gem
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
This code is still used in provider_account_observer if this patch is
applied. The patch to remove its use in provider_account_observer should
be included in this patch.
Cheers,
Mark.