[PATCH conductor] RM 3628 - ensure that all params in a deployable can be resolved
Tomas Hrcka
thrcka at redhat.com
Thu Aug 9 12:38:14 UTC 2012
On Tue, 2012-08-07 at 10:11 +0200, jprovazn at redhat.com wrote:
> 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
ACK.
More information about the aeolus-devel
mailing list