From f17d8941567206522d20638505fbcb0326890a94 Mon Sep 17 00:00:00 2001 From: Alexander Zagaynov Date: Tue, 18 Dec 2018 18:39:53 +0100 Subject: [PATCH 1/3] fix endpoint url uniqueness validation, and disable it for cloud providers --- app/models/endpoint.rb | 6 +++++- spec/models/endpoint_spec.rb | 17 +++++++++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/models/endpoint.rb b/app/models/endpoint.rb index a1c76bd8ada..c3f8f27f01d 100644 --- a/app/models/endpoint.rb +++ b/app/models/endpoint.rb @@ -6,7 +6,7 @@ class Endpoint < ApplicationRecord default_value_for :verify_ssl, OpenSSL::SSL::VERIFY_PEER validates :verify_ssl, :inclusion => {:in => [OpenSSL::SSL::VERIFY_NONE, OpenSSL::SSL::VERIFY_PEER]} validates :port, :numericality => {:only_integer => true, :allow_nil => true, :greater_than => 0} - validates :url, :uniqueness => true, :if => :url + validates :url, :uniqueness => true, :allow_blank => true, :if => :should_validate_url? validate :validate_certificate_authority after_create :endpoint_created @@ -77,4 +77,8 @@ def validate_certificate_authority rescue OpenSSL::OpenSSLError => err errors.add(:certificate_authority, err.to_s) end + + def should_validate_url? + !resource.kind_of?(ManageIQ::Providers::CloudManager) + end end diff --git a/spec/models/endpoint_spec.rb b/spec/models/endpoint_spec.rb index a30c07ce70a..d147fff2aa5 100644 --- a/spec/models/endpoint_spec.rb +++ b/spec/models/endpoint_spec.rb @@ -48,14 +48,27 @@ context "Uniqueness validation on :url" do it "is not required" do - expect(Endpoint.create!).to be_truthy - expect(Endpoint.create!).to be_truthy + expect(Endpoint.create!(:url => nil)).to be_truthy + expect(Endpoint.create!(:url => nil)).to be_truthy + expect(Endpoint.create!(:url => '')).to be_truthy + expect(Endpoint.create!(:url => '')).to be_truthy end it "raises when provided and already exists" do Endpoint.create!(:url => "abc") expect { Endpoint.create!(:url => "abc") }.to raise_error("Validation failed: Endpoint: Url has already been taken") end + + it 'disabled for cloud providers' do + expect(Endpoint.create!(:url => 'defined', :resource => FactoryBot.create(:ems_cloud))).to be_truthy + expect(Endpoint.create!(:url => 'defined', :resource => FactoryBot.create(:ems_cloud))).to be_truthy + end + + it 'enabled for other emses' do + expect(Endpoint.create!(:url => 'defined', :resource => FactoryBot.create(:ext_management_system))).to be_truthy + expect { Endpoint.create!(:url => 'defined', :resource => FactoryBot.create(:ext_management_system)) } + .to raise_error("Validation failed: Endpoint: Url has already been taken") + end end context "certificate_authority" do From 272f11f30b44b911d81692e74a54262834f01c92 Mon Sep 17 00:00:00 2001 From: Alexander Zagaynov Date: Wed, 19 Dec 2018 19:00:26 +0100 Subject: [PATCH 2/3] ask endpoint owner about url uniqueness requirement --- app/models/endpoint.rb | 6 +++--- app/models/manageiq/providers/cloud_manager.rb | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/models/endpoint.rb b/app/models/endpoint.rb index c3f8f27f01d..7644fcf4a87 100644 --- a/app/models/endpoint.rb +++ b/app/models/endpoint.rb @@ -6,7 +6,7 @@ class Endpoint < ApplicationRecord default_value_for :verify_ssl, OpenSSL::SSL::VERIFY_PEER validates :verify_ssl, :inclusion => {:in => [OpenSSL::SSL::VERIFY_NONE, OpenSSL::SSL::VERIFY_PEER]} validates :port, :numericality => {:only_integer => true, :allow_nil => true, :greater_than => 0} - validates :url, :uniqueness => true, :allow_blank => true, :if => :should_validate_url? + validates :url, :uniqueness => true, :allow_blank => true, :if => :should_have_unique_url? validate :validate_certificate_authority after_create :endpoint_created @@ -78,7 +78,7 @@ def validate_certificate_authority errors.add(:certificate_authority, err.to_s) end - def should_validate_url? - !resource.kind_of?(ManageIQ::Providers::CloudManager) + def should_have_unique_url? + !resource.try(:allow_nonunique_endpoint_url?) end end diff --git a/app/models/manageiq/providers/cloud_manager.rb b/app/models/manageiq/providers/cloud_manager.rb index 354e9c841fc..b6bdd36b6cf 100644 --- a/app/models/manageiq/providers/cloud_manager.rb +++ b/app/models/manageiq/providers/cloud_manager.rb @@ -150,5 +150,7 @@ def destroy_mapped_tenants source_tenant.destroy end end + + define_method(:allow_nonunique_endpoint_url?) { true } end end From ab9cc07e04f40cd769cb6dac75b657e3a3e26e04 Mon Sep 17 00:00:00 2001 From: Alexander Zagaynov Date: Thu, 20 Dec 2018 18:04:58 +0100 Subject: [PATCH 3/3] change method names --- app/models/endpoint.rb | 6 +++--- app/models/ext_management_system.rb | 2 ++ app/models/manageiq/providers/cloud_manager.rb | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/endpoint.rb b/app/models/endpoint.rb index 7644fcf4a87..890cff3ab70 100644 --- a/app/models/endpoint.rb +++ b/app/models/endpoint.rb @@ -6,7 +6,7 @@ class Endpoint < ApplicationRecord default_value_for :verify_ssl, OpenSSL::SSL::VERIFY_PEER validates :verify_ssl, :inclusion => {:in => [OpenSSL::SSL::VERIFY_NONE, OpenSSL::SSL::VERIFY_PEER]} validates :port, :numericality => {:only_integer => true, :allow_nil => true, :greater_than => 0} - validates :url, :uniqueness => true, :allow_blank => true, :if => :should_have_unique_url? + validates :url, :uniqueness => true, :allow_blank => true, :unless => :allow_duplicate_url? validate :validate_certificate_authority after_create :endpoint_created @@ -78,7 +78,7 @@ def validate_certificate_authority errors.add(:certificate_authority, err.to_s) end - def should_have_unique_url? - !resource.try(:allow_nonunique_endpoint_url?) + def allow_duplicate_url? + resource.try(:allow_duplicate_endpoint_url?) end end diff --git a/app/models/ext_management_system.rb b/app/models/ext_management_system.rb index cee710e547e..7f5ebab2e52 100644 --- a/app/models/ext_management_system.rb +++ b/app/models/ext_management_system.rb @@ -844,4 +844,6 @@ def clear_association_cache @storages = nil super end + + define_method(:allow_duplicate_endpoint_url?) { false } end diff --git a/app/models/manageiq/providers/cloud_manager.rb b/app/models/manageiq/providers/cloud_manager.rb index b6bdd36b6cf..58263197728 100644 --- a/app/models/manageiq/providers/cloud_manager.rb +++ b/app/models/manageiq/providers/cloud_manager.rb @@ -151,6 +151,6 @@ def destroy_mapped_tenants end end - define_method(:allow_nonunique_endpoint_url?) { true } + define_method(:allow_duplicate_endpoint_url?) { true } end end