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.
Hi, good points, going to resend. Jan