diff --git a/app/models/ext_management_system.rb b/app/models/ext_management_system.rb index faf46c6ca29..79440409621 100644 --- a/app/models/ext_management_system.rb +++ b/app/models/ext_management_system.rb @@ -176,18 +176,41 @@ def self.create_from_params(params, endpoints, authentications) def edit_with_params(params, endpoints, authentications) tap do |ems| + endpoints_changed = false + authentications_changed = false + + endpoint_changes = {} + authentication_changes = {} + transaction do # Remove endpoints/attributes that are not arriving in the arguments above - ems.endpoints.where.not(:role => nil).where.not(:role => endpoints.map { |ep| ep['role'] }).delete_all - ems.authentications.where.not(:authtype => nil).where.not(:authtype => authentications.map { |au| au['authtype'] }).delete_all + endpoints_to_delete = ems.endpoints.where.not(:role => nil).where.not(:role => endpoints.map { |ep| ep['role'] }) + authentications_to_delete = ems.authentications.where.not(:authtype => nil).where.not(:authtype => authentications.map { |au| au['authtype'] }) + + endpoints_changed ||= endpoints_to_delete.delete_all > 0 + authentications_changed ||= authentications_to_delete.delete_all > 0 ems.assign_attributes(params) - ems.endpoints = endpoints.map(&method(:assign_nested_endpoint)) - ems.authentications = authentications.map(&method(:assign_nested_authentication)) + ems.endpoints = endpoints.map { |ep| assign_nested_endpoint(ep) } + ems.authentications = authentications.map { |auth| assign_nested_authentication(auth) } + + endpoint_changes = ems.endpoints.select(&:changed?).to_h do |ep| + [ep.role.to_sym, ep.changes] + end + + authentication_changes = ems.authentications.select(&:changed?).to_h do |auth| + [auth.authtype.to_sym, auth.changes] + end + + endpoints_changed ||= endpoint_changes.present? + authentications_changed ||= authentication_changes.present? ems.provider.save! if ems.provider.present? && ems.provider.changed? ems.save! end + + after_update_endpoints(endpoint_changes) if endpoints_changed + after_update_authentication(authentication_changes) if authentications_changed end end @@ -834,8 +857,12 @@ def perf_capture_enabled? # Some workers hold open a connection to the provider and thus do not # automatically pick up authentication changes. These workers have to be # restarted manually for the new credentials to be used. - def after_update_authentication - stop_event_monitor_queue_on_credential_change + def after_update_authentication(changes) + stop_event_monitor_queue_on_credential_change(changes) + end + + def after_update_endpoints(changes) + stop_event_monitor_queue_on_change(changes) end ################################### @@ -847,6 +874,14 @@ def self.event_monitor_class end delegate :event_monitor_class, :to => :class + def endpoint_role_for_events + :default + end + + def authtype_for_events + default_authentication_type + end + def event_monitor return if event_monitor_class.nil? event_monitor_class.find_by_ems(self).first @@ -874,15 +909,15 @@ def stop_event_monitor_queue ) end - def stop_event_monitor_queue_on_change - if event_monitor_class && !self.new_record? && default_endpoint.changed.include_any?("hostname", "ipaddress") + def stop_event_monitor_queue_on_change(changes) + if event_monitor_class && !new_record? && changes[endpoint_role_for_events]&.keys&.include_any?("hostname", "ipaddress") _log.info("EMS: [#{name}], Hostname or IP address has changed, stopping Event Monitor. It will be restarted by the WorkerMonitor.") stop_event_monitor_queue end end - def stop_event_monitor_queue_on_credential_change - if event_monitor_class && !self.new_record? && self.credentials_changed? + def stop_event_monitor_queue_on_credential_change(changes) + if event_monitor_class && !new_record? && changes[authtype_for_events].present? _log.info("EMS: [#{name}], Credentials have changed, stopping Event Monitor. It will be restarted by the WorkerMonitor.") stop_event_monitor_queue end @@ -908,6 +943,14 @@ def self.refresh_worker_class end delegate :refresh_worker_class, :to => :class + def endpoint_role_for_refresh + :default + end + + def authtype_for_refresh + default_authentication_type + end + def refresh_worker return if refresh_worker_class.nil? @@ -938,15 +981,15 @@ def stop_refresh_worker_queue ) end - def stop_refresh_worker_queue_on_change - if refresh_worker_class && !self.new_record? && default_endpoint.changed.include_any?("hostname", "ipaddress") + def stop_refresh_worker_queue_on_change(changes) + if refresh_worker_class && !new_record? && changes["default"]&.keys&.include_any?("hostname", "ipaddress") _log.info("EMS: [#{name}], Hostname or IP address has changed, stopping Refresh Worker. It will be restarted by the WorkerMonitor.") stop_refresh_worker_queue end end - def stop_refresh_worker_queue_on_credential_change - if refresh_worker_class && !self.new_record? && self.credentials_changed? + def stop_refresh_worker_queue_on_credential_change(changes) + if refresh_worker_class && !new_record? && changes[authtype_for_refresh].present? _log.info("EMS: [#{name}], Credentials have changed, stopping Refresh Worker. It will be restarted by the WorkerMonitor.") stop_refresh_worker_queue end diff --git a/app/models/manageiq/providers/cloud_manager.rb b/app/models/manageiq/providers/cloud_manager.rb index 3204330f937..55435c299bc 100644 --- a/app/models/manageiq/providers/cloud_manager.rb +++ b/app/models/manageiq/providers/cloud_manager.rb @@ -76,8 +76,8 @@ def open_browser MiqSystem.open_browser(browser_url) end - def stop_event_monitor_queue_on_credential_change - if event_monitor_class && !self.new_record? && self.credentials_changed? + def stop_event_monitor_queue_on_credential_change(changes) + if event_monitor_class && !new_record? && changes["default"]&.keys&.include_any?("hostname", "ipaddress") _log.info("EMS: [#{name}], Credentials have changed, stopping Event Monitor. It will be restarted by the WorkerMonitor.") stop_event_monitor_queue network_manager.stop_event_monitor_queue if respond_to?(:network_manager) && network_manager diff --git a/app/models/mixins/authentication_mixin.rb b/app/models/mixins/authentication_mixin.rb index fae8e8c2788..5f57d83c926 100644 --- a/app/models/mixins/authentication_mixin.rb +++ b/app/models/mixins/authentication_mixin.rb @@ -183,12 +183,10 @@ def update_authentication(data, options = {}) options.reverse_merge!(:save => true) - @orig_credentials ||= auth_user_pwd || "none" - # Invoke before callback before_update_authentication if self.respond_to?(:before_update_authentication) && options[:save] - data.each_pair do |type, value| + authentication_changes = data.each_pair.map do |type, value| cred = authentication_type(type) current = {:new => nil, :old => nil} @@ -245,18 +243,17 @@ def update_authentication(data, options = {}) cred.auth_key = value[:auth_key] cred.service_account = value[:service_account].presence + changes = [type.to_sym, cred.changes] if cred.changed? + cred.save if options[:save] && id - end - # Invoke callback - after_update_authentication if self.respond_to?(:after_update_authentication) && options[:save] - @orig_credentials = nil if options[:save] - end + changes + end.compact.to_h + + return if authentication_changes.blank? || !options[:save] - def credentials_changed? - @orig_credentials ||= auth_user_pwd || "none" - new_credentials = auth_user_pwd || "none" - @orig_credentials != new_credentials + # Invoke callback + after_update_authentication(authentication_changes) if respond_to?(:after_update_authentication) end def authentication_type(type) diff --git a/spec/models/ext_management_system_spec.rb b/spec/models/ext_management_system_spec.rb index ccfeac5db96..1e91e529907 100644 --- a/spec/models/ext_management_system_spec.rb +++ b/spec/models/ext_management_system_spec.rb @@ -833,6 +833,163 @@ def deliver_queue_message(queue_message = MiqQueue.order(:id).first) end end + describe "#edit_with_params" do + let(:zone) { EvmSpecHelper.local_miq_server.zone } + let(:ems) do + FactoryBot.create(:ext_management_system, :with_authentication, :zone => zone).tap do |ems| + # assign_nested_authentication automatically sets the name to "#{self.class.name} #{name}" + # set it here to prevent spurious authentication record changes + ems.default_authentication.update!(:name => "#{ems.class.name} #{ems.name}") + end + end + + let(:params) { {"name" => ems.name, "zone" => ems.zone} } + let(:endpoints) { [default_endpoint] } + let(:authentications) { [default_authentication] } + + let(:default_endpoint) { {"role" => "default", "hostname" => ems.default_endpoint.hostname, "ipaddress" => ems.default_endpoint.ipaddress} } + let(:default_authentication) { {"authtype" => "default", "userid" => ems.default_authentication.userid, "password" => ems.default_authentication.password} } + + context "with no changes" do + it "doesn't call endpoints or authentications changed callbacks" do + expect(ems).not_to receive(:after_update_endpoints) + expect(ems).not_to receive(:after_update_authentication) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "changing an ext_management_system record attribute" do + let(:params) { {:name => "new-name", :zone => ems.zone} } + + it "changes the name" do + ems.edit_with_params(params, endpoints, authentications) + expect(ems.reload.name).to eq("new-name") + end + end + + context "adding an endpoint" do + let(:endpoints) { [default_endpoint, {"role" => "metrics", "hostname" => "metrics"}] } + + it "creates the new endpoint" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + + expect(ems.endpoints.pluck(:role)).to match_array(["default", "metrics"]) + end + + it "calls after_update_endpoints" do + expect(ems).to receive(:after_update_endpoints) + expect(ems).not_to receive(:after_update_authentication) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "deleting an endpoint" do + before { ems.endpoints.create!(:role => "metrics", :hostname => "metrics") } + + it "deletes the unused endpoint" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + expect(ems.endpoints.pluck(:role)).to match_array(["default"]) + end + + it "calls after_update_endpoints" do + expect(ems).to receive(:after_update_endpoints) + expect(ems).not_to receive(:after_update_authentication) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "updating an endpoint" do + let(:default_endpoint) { {"role" => "default", "hostname" => "new-hostname", "ipaddress" => ems.default_endpoint.ipaddress} } + + it "updates the default hostname" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + expect(ems.default_endpoint.hostname).to eq("new-hostname") + end + + it "calls after_update_endpoints" do + expect(ems).to receive(:after_update_endpoints) + expect(ems).not_to receive(:after_update_authentication) + + ems.edit_with_params(params, endpoints, authentications) + end + + it "stops the event monitor" do + expect(ems).to receive(:stop_event_monitor_queue) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "adding an authentication" do + let(:authentications) { [default_authentication, {:authtype => "metrics", :auth_key => "secret"}] } + + it "creates the authentication" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + expect(ems.authentications.pluck(:authtype)).to match_array(["default", "metrics"]) + end + + it "calls after_update_authentication" do + expect(ems).to receive(:after_update_authentication) + expect(ems).not_to receive(:after_update_endpoints) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "deleting an authentication" do + before { ems.authentications.create!(:authtype => "metrics", :auth_key => "secret") } + + it "deletes the authentication" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + expect(ems.authentications.pluck(:authtype)).to match_array(["default"]) + end + + it "calls after_update_authentication" do + expect(ems).to receive(:after_update_authentication) + expect(ems).not_to receive(:after_update_endpoints) + + ems.edit_with_params(params, endpoints, authentications) + end + end + + context "updating an authentication" do + let(:default_authentication) { {"authtype" => "default", "userid" => ems.default_authentication.userid, "password" => "more-secret"} } + + it "updates the password" do + ems.edit_with_params(params, endpoints, authentications) + + ems.reload + expect(ems.default_authentication.password).to eq("more-secret") + end + + it "calls after_update_authentication" do + expect(ems).to receive(:after_update_authentication) + expect(ems).not_to receive(:after_update_endpoints) + + ems.edit_with_params(params, endpoints, authentications) + end + + it "stops the event monitor" do + expect(ems).to receive(:stop_event_monitor_queue) + + ems.edit_with_params(params, endpoints, authentications) + end + end + end + context "virtual column :supports_block_storage (direct supports)" do it "returns false if block storage is not supported" do ems = FactoryBot.create(:ext_management_system)