[PATCH conductor] RM 3628 - ensure that all params in a deployable can be resolved
jprovazn at redhat.com
jprovazn at redhat.com
Tue Aug 7 08:11:26 UTC 2012
From: Jan Provazník <jan.provaznik at gmail.com>
Adds validation for cyclic and not existing references in deployable XML.
Ii also resolves https://bugzilla.redhat.com/show_bug.cgi?id=841028
---
src/app/models/deployable.rb | 44 +++++++++++++-
src/app/util/deployable_xml.rb | 94 ++++++++++++++++++++++++++++++
src/config/locales/en.yml | 4 ++
src/spec/factories/deployable.rb | 111 ++++++++++++++++++++++++++++++++++++
src/spec/models/deployable_spec.rb | 71 ++++++++++++++++++++++-
5 files changed, 321 insertions(+), 3 deletions(-)
diff --git a/src/app/models/deployable.rb b/src/app/models/deployable.rb
index e1d80d5..374b4e3 100644
--- a/src/app/models/deployable.rb
+++ b/src/app/models/deployable.rb
@@ -29,7 +29,8 @@ class Deployable < ActiveRecord::Base
validates_length_of :name, :maximum => 1024
validates_presence_of :xml
- validate :valid_deployable_xml?, :if => Proc.new { |deployable| !deployable.xml.blank? }
+ validate :validate_deployable_xml, :if => Proc.new {|deployable|
+ !deployable.xml.blank? }
has_many :permissions, :as => :permission_object, :dependent => :destroy,
:include => [:role],
@@ -57,7 +58,7 @@ class Deployable < ActiveRecord::Base
super + catalogs + catalogs.collect{|c| c.pool}.uniq + [pool_family]
end
- def valid_deployable_xml?
+ def validate_deployable_xml
begin
deployable_xml = DeployableXML.new(xml)
if !deployable_xml.validate!
@@ -65,11 +66,50 @@ class Deployable < ActiveRecord::Base
elsif !deployable_xml.unique_assembly_names?
errors.add(:xml, I18n.t('catalog_entries.flash.warning.not_valid_duplicate_assembly_names'))
end
+ validate_cycles_in_deployable_xml(deployable_xml)
+ validate_references_in_deployable_xml(deployable_xml)
rescue Nokogiri::XML::SyntaxError => e
errors.add(:base, I18n.t("deployments.errors.not_valid_deployable_xml", :msg => "#{e.message}"))
end
end
+ def validate_cycles_in_deployable_xml(deployable_xml)
+ deployable_xml.dependency_graph.cycles.each do |cycle|
+ cycle_str = cycle.map do |node|
+ str = node[:assembly].to_s
+ str << "[#{node[:service]}]" if node[:service].present?
+ str
+ end.join(' -> ')
+ errors.add(:xml,
+ I18n.t('catalog_entries.flash.warning.not_valid_cyclic_reference',
+ :reference => cycle_str))
+ end
+ end
+
+ def validate_references_in_deployable_xml(deployable_xml)
+ deployable_xml.dependency_graph.not_existing_references.each do |reference|
+ locale_params = {:from_assembly => reference[:assembly],
+ :from_service => reference[:service],
+ :from_param => reference[:reference][:from_param],
+ :to_param => reference[:reference][:param],
+ :to_assembly => reference[:reference][:assembly],
+ :to_service => reference[:reference][:service]}
+ if reference[:no_return_param]
+ errors.add(:xml,
+ I18n.t('catalog_entries.flash.warning.not_valid_not_existing_param',
+ locale_params))
+ elsif reference[:to_service]
+ errors.add(:xml,
+ I18n.t('catalog_entries.flash.warning.not_valid_not_existing_service_reference',
+ locale_params))
+ else
+ errors.add(:xml,
+ I18n.t('catalog_entries.flash.warning.not_valid_not_existing_assembly_reference',
+ locale_params))
+ end
+ end
+ end
+
# Fetch the deployable contained at :url
def fetch_deployable
begin
diff --git a/src/app/util/deployable_xml.rb b/src/app/util/deployable_xml.rb
index cdbbc39..cb01cf5 100644
--- a/src/app/util/deployable_xml.rb
+++ b/src/app/util/deployable_xml.rb
@@ -18,6 +18,8 @@
XML Wrapper objects for the deployable XML format
=end
+require 'tsort'
+
class ValidationError < RuntimeError; end
class ParameterXML
@@ -96,6 +98,16 @@ class ServiceXML
ParameterXML.new(param_node)
end
end
+
+ def references
+ refs = parameters.map do |param|
+ next unless param.reference?
+ {:assembly => param.reference_assembly,
+ :service => param.reference_service,
+ :param => param.reference_parameter,
+ :from_param => param.name}
+ end.compact
+ end
end
class AssemblyXML
@@ -166,6 +178,20 @@ class AssemblyXML
@root.to_s
end
+ def dependency_nodes
+ nodes = services.map do |service|
+ {:assembly => name,
+ :service => service.name,
+ :references => service.references}
+ end
+ # whole assembly is added as a node too because a inter-assembly references
+ # reference whole assembly, not particular service
+ nodes << {:assembly => name,
+ :service => nil,
+ :references => nodes.map {|n|
+ n[:references]}.flatten}
+ end
+
private
def collect_services
# collect the service level tooling
@@ -270,8 +296,76 @@ class DeployableXML
end
end
+ def dependency_graph
+ @dependency_graph ||= DeployableDependencyGraph.new(assemblies)
+ end
+
private
+
def relax
@relax ||= File.open(@relax_file) {|f| Nokogiri::XML::RelaxNG(f)}
end
end
+
+class DeployableDependencyGraph
+ include TSort
+
+ def initialize(assemblies)
+ @assemblies = assemblies
+ end
+
+ def cycles
+ strongly_connected_components.find_all {|c| c.length > 1}
+ end
+
+ def dependency_nodes
+ @nodes ||= @assemblies.map {|assembly| assembly.dependency_nodes}.flatten
+ end
+
+ def tsort_each_node(&block)
+ dependency_nodes.each(&block)
+ end
+
+ def tsort_each_child(node, &block)
+ node[:references].map do |ref|
+ ref_node = dependency_nodes.find do |n|
+ n[:assembly] == ref[:assembly] && n[:service] == ref[:service]
+ end
+ ref[:not_existing_ref] = true unless ref_node
+ ref_node
+ end.compact.each(&block)
+ end
+
+ def not_existing_references
+ # not_existing references are detected when doing tsort
+ strongly_connected_components
+
+ invalid_refs = []
+ dependency_nodes.each do |node|
+ # skip "whole assembly" nodes
+ next unless node[:service]
+
+ node[:references].each do |ref|
+ if ref[:not_existing_ref]
+ # in this case, the referenced assembly/service doesn't exist
+ # at all
+ invalid_refs << {:assembly => node[:assembly],
+ :service => node[:service],
+ :reference => ref}
+ else
+ # in this case, the referenced assembly/service exists, but the
+ # referenced parameter is not listed in <returns> tag of the assembly
+ assembly = @assemblies.find {|a| a.name == ref[:assembly]}
+ next unless assembly # this shouldn't be needed
+ unless assembly.output_parameters.include?(ref[:param])
+ invalid_refs << {:assembly => node[:assembly],
+ :service => node[:service],
+ :no_return_param => true,
+ :reference => ref}
+ end
+ end
+ end
+ end
+ invalid_refs
+ end
+end
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index 9ae82e4..8cadf41 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -826,6 +826,10 @@ en:
no_url_provided: "No URL provided for the deployable XML file."
not_valid: "Deployable XML file doesn't resolve valid XML"
not_valid_duplicate_assembly_names: "Deployable XML must contain unique assembly names"
+ not_valid_cyclic_reference: "Contains cyclic reference between following assemblies or services: %{reference}"
+ not_valid_not_existing_assembly_reference: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references not existing assembly %{to_assembly}"
+ not_valid_not_existing_service_reference: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references not existing assembly %{to_assembly}, service %{to_service}"
+ not_valid_not_existing_param: "Assembly %{from_assembly}, service %{from_service}, parameter %{from_param} references param %{to_param} which is not returned by assembly %{to_assembly}"
failed: Deployable was not created.
notice:
added: Deployable added to catalog %{catalog}.
diff --git a/src/spec/factories/deployable.rb b/src/spec/factories/deployable.rb
index efbfc6e..8a4f3ba 100644
--- a/src/spec/factories/deployable.rb
+++ b/src/spec/factories/deployable.rb
@@ -72,4 +72,115 @@ FactoryGirl.define do
</assemblies>
</deployable>"
end
+
+ factory :deployable_with_cyclic_assembly_references, :parent => :deployable do
+ xml "<?xml version=\"1.0\"?>
+ <deployable version='1.0' name='deployable'>
+ <assemblies>
+ <assembly name='assembly1' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly2' parameter='hostname'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ <assembly name='assembly2' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly3' parameter='hostname'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ <assembly name='assembly3' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly1' parameter='hostname'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ </assemblies>
+ </deployable>"
+ end
+
+
+ factory :deployable_with_cyclic_service_references, :parent => :deployable do
+ xml "<?xml version=\"1.0\"?>
+ <deployable version='1.0' name='deployable'>
+ <assemblies>
+ <assembly name='assembly1' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly1' service='service2'/>
+ </parameter>
+ </parameters>
+ </service>
+ <service name='service2'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly1' service='service1'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ </assemblies>
+ </deployable>"
+ end
+
+ factory :deployable_with_not_existing_references, :parent => :deployable do
+ xml "<?xml version=\"1.0\"?>
+ <deployable version='1.0' name='deployable'>
+ <assemblies>
+ <assembly name='assembly1' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly2' service='service2'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ <assembly name='assembly2' hwp='front_hwp1'>
+ <image id='85653387-88b2-46b0-b7b2-c779d2af06c7'></image>
+ <services>
+ <service name='service1'>
+ <executable url='executable_url'/>
+ <parameters>
+ <parameter name='param1'>
+ <reference assembly='assembly3' service='service1'/>
+ </parameter>
+ </parameters>
+ </service>
+ </services>
+ </assembly>
+ </assemblies>
+ </deployable>"
+ end
end
diff --git a/src/spec/models/deployable_spec.rb b/src/spec/models/deployable_spec.rb
index 6bc8ffd..4aec099 100644
--- a/src/spec/models/deployable_spec.rb
+++ b/src/spec/models/deployable_spec.rb
@@ -17,6 +17,16 @@
require 'spec_helper'
describe Deployable do
+ shared_examples_for "deployable XML with cyclic reference" do
+ it "should not be valid" do
+ @deployable.valid?.should be_false
+ end
+
+ it "should detect one cycle" do
+ @cycles.length.should == 1
+ end
+ end
+
it "should generate xml when set from image" do
image = mock(Aeolus::Image::Warehouse::Image, :id => '3c58e0d6-d11a-4e68-8b12-233783e56d35', :name => 'image1', :uuid => '3c58e0d6-d11a-4e68-8b12-233783e56d35')
Aeolus::Image::Warehouse::Image.stub(:find).and_return(image)
@@ -40,7 +50,7 @@ describe Deployable do
it "should not be valid if xml has multiple assemblies with the same name" do
deployable = FactoryGirl.build(:deployable_unique_name_violation)
- deployable.valid_deployable_xml?
+ deployable.validate_deployable_xml
deployable.errors[:xml].should == [I18n.t('catalog_entries.flash.warning.not_valid_duplicate_assembly_names')]
end
@@ -72,6 +82,65 @@ describe Deployable do
deployable.should_not be_valid
end
+ describe "validation of deployable_xml with cyclic service references" do
+ before :each do
+ @deployable = FactoryGirl.build(:deployable_with_cyclic_service_references)
+ @cycles = DeployableXML.new(@deployable.xml).dependency_graph.cycles
+ end
+
+ it_behaves_like "deployable XML with cyclic reference"
+
+ it "should detect cycle between services of same assembly" do
+ cycle = @cycles.first
+ cycle.length.should == 2
+ cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service1'}
+ cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service2'}
+ end
+ end
+
+ describe "validation of deployable_xml with cyclic assembly references" do
+ before :each do
+ @deployable = FactoryGirl.build(:deployable_with_cyclic_assembly_references)
+ @cycles = DeployableXML.new(@deployable.xml).dependency_graph.cycles
+ end
+
+ it_behaves_like "deployable XML with cyclic reference"
+
+ it "should detect cycle between multiple assmebliess" do
+ cycle = @cycles.first
+ cycle.length.should == 3
+ cycle.find {|n| n[:assembly] == 'assembly1' && n[:service] == 'service1'}
+ cycle.find {|n| n[:assembly] == 'assembly2' && n[:service] == 'service1'}
+ cycle.find {|n| n[:assembly] == 'assembly3' && n[:service] == 'service1'}
+ end
+ end
+
+ describe "validation of deployable_xml with not existing references" do
+ before :each do
+ @deployable = FactoryGirl.build(:deployable_with_not_existing_references)
+ @invalid_refs = DeployableXML.new(@deployable.xml).
+ dependency_graph.not_existing_references
+ end
+
+ it "should not be valid" do
+ @deployable.should_not be_valid
+ end
+
+ it "should detect reference to not existing assembly" do
+ @invalid_refs.find {|r| r[:assembly] == 'assembly2' &&
+ r[:service] == 'service1' &&
+ r[:reference][:assembly] == 'assembly3' &&
+ r[:reference][:service] == 'service1'}.should_not be_nil
+ end
+
+ it "should detect reference to not existing service" do
+ @invalid_refs.find {|r| r[:assembly] == 'assembly1' &&
+ r[:service] == 'service1' &&
+ r[:reference][:assembly] == 'assembly2' &&
+ r[:reference][:service] == 'service2'}.should_not be_nil
+ end
+ end
+
it "should manage permissions on catalog changes" do
pool1 = FactoryGirl.create :pool
pool2 = FactoryGirl.create :pool
--
1.7.10.4
More information about the aeolus-devel
mailing list