Merge branch 'kpaulisse-validate-references' into kpaulisse-environment

Author: Kevin Paulisse

doc/advanced-catalog-validation.md

@@ -0,0 +1,41 @@
+# Catalog validation
+
+`octocatalog-diff` contains additional functionality to validate catalogs, based on configurable criteria.
+
+Catalog validation features include:
+
+- Validate references: Ensure resources targeted by `before`, `notify`, `require`, and/or `subscribe` exist in the catalog
+
+## Validate references
+
+`octocatalog-diff` includes the ability to validate references by ensuring resources targeted by `before`, `notify`, `require`, and/or `subscribe` parameters also exist in the catalog.
+
+Consider the following Puppet code:
+
+```
+file { '/usr/local/bin/some-script.sh':
+  source => 'puppet:///modules/test/usr/local/bin/some-script.sh',
+  notify => Exec['execute /usr/local/bin/some-script.sh'],
+}
+```
+
+The catalog for this code would build, whether or not the `exec { 'execute /usr/local/bin/some-script.sh': ... }` resource was part of the catalog. However, when the catalog is applied on the Puppet agent, it would fail if this resource is missing.
+
+With the `--validate-references` command line flag (or the `settings[:validate_references]` [configuration setting](/doc/configuration.md), you can instruct `octocatalog-diff` to confirm that any resource targeted by a `before`, `notify`, `require`, and `subscribe` parameter actually exists. If the resource is missing from the catalog, an error will be raised, just as if the catalog failed to compile.
+
+The command line argument is demonstrated here:
+
+```
+# Validate all references: before,notify,require,subscribe
+octocatalog-diff ... --validate-references before,notify,require,subscribe
+
+# Validate some references: only before and require
+octocatalog-diff ... --validate-references before,require
+
+# Validate no references
+octocatalog-diff ... --no-validate-references
+```
+
+By default, no references are validated.
+
+Note as well, when using `octocatalog-diff` to compare two catalogs, the references in the "from" catalog are not checked. The reason for this design decision is as follows: the "from" catalog is generally what is considered to be stable and is perhaps already deployed, so it adds no value (and perhaps inhibits the ability to develop further) if `octocatalog-diff` fails just because references in the "from" catalog are broken.

doc/advanced.md

@@ -21,6 +21,7 @@ See also:
 - [Overriding facts](/doc/advanced-override-facts.md)
 - [Puppet Enterprise node classification service](/doc/advanced-pe-enc.md)
 - [Using `octocatalog-diff` without git](/doc/advanced-using-without-git.md)
+- [Catalog validation](/doc/advanced-catalog-validation.md)
 
 ### Controlling output
 

examples/octocatalog-diff.cfg.rb

@@ -200,6 +200,16 @@ def self.config
 
       settings[:from_env] = 'origin/master'
 
+      ##############################################################################################
+      # validate_references
+      #   Set this to an array containing 1 or more of: `before`, `notify`, `require`, `subscribe`
+      #   to validate references in the "to" catalog. This will cause the catalog to fail if it
+      #   contains references to resources that aren't in the catalog. If you don't want to validate
+      #   any references, set this to an empty array, or just comment out the line.
+      ##############################################################################################
+
+      settings[:validate_references] = %w(before notify require subscribe)
+
       ##############################################################################################
       # Less commonly changed settings
       ##############################################################################################

lib/octocatalog-diff/catalog-diff/cli/catalogs.rb

@@ -157,6 +157,7 @@ def build_catalog_tasks
             task = OctocatalogDiff::Util::Parallel::Task.new(
               method: method(:build_catalog),
               validator: method(:catalog_validator),
+              validator_args: { task: key },
               description: "build_catalog for #{@options["#{key}_env".to_sym]}",
               args: args
             )
@@ -231,9 +232,12 @@ def build_catalog(opts, logger = @logger)
 
         # Validate a catalog in the parallel execution
         # @param catalog [OctocatalogDiff::Catalog] Catalog object
+        # @param logger [Logger] Logger object (presently unused)
+        # @param args [Hash] Additional arguments set specifically for validator
         # @return [Boolean] true if catalog is valid, false otherwise
-        def catalog_validator(catalog = nil, _logger = @logger)
+        def catalog_validator(catalog = nil, _logger = @logger, args = {})
           return false unless catalog.is_a?(OctocatalogDiff::Catalog)
+          catalog.validate_references if args[:task] == :to
           catalog.valid?
         end
       end

lib/octocatalog-diff/catalog-diff/cli/options/validate_references.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+# Confirm that each `before`, `require`, `subscribe`, and/or `notify` points to a valid
+# resource in the catalog. This value should be specified as an array of which of these
+# parameters are to be checked.
+# @param parser [OptionParser object] The OptionParser argument
+# @param options [Hash] Options hash being constructed; this is modified in this method.
+OctocatalogDiff::CatalogDiff::Cli::Options::Option.newoption(:validate_references) do
+  has_weight 205
+
+  def parse(parser, options)
+    parser.on('--[no-]validate-references "before,require,subscribe,notify"', Array, 'References to validate') do |res|
+      if res == false
+        options[:validate_references] = []
+      else
+        options[:validate_references] ||= []
+        res.each do |item|
+          unless %w(before require subscribe notify).include?(item)
+            raise ArgumentError, "Invalid reference validation #{item}"
+          end
+
+          options[:validate_references] << item
+        end
+      end
+    end
+  end
+end

lib/octocatalog-diff/catalog.rb

@@ -21,6 +21,7 @@ class Catalog
     # Error classes that we can throw
     class PuppetVersionError < RuntimeError; end
     class CatalogError < RuntimeError; end
+    class ReferenceValidationError < RuntimeError; end
 
     # Constructor
     # @param :backend [Symbol] If set, this will force a backend
@@ -157,7 +158,7 @@ def resources
       # This is a bug condition
       # :nocov:
       raise "BUG: catalog has no data::resources or ::resources array. Please report this. #{@catalog.inspect}"
-      # :nocov
+      # :nocov:
     end
 
     # This retrieves the number of retries necessary to compile the catalog. If the underlying catalog
@@ -173,8 +174,69 @@ def valid?
       !@catalog.nil?
     end
 
+    # Determine if all of the (before, notify, require, subscribe) targets are actually in the catalog.
+    # Raise a ReferenceValidationError for any found to be missing.
+    # Uses @options[:validate_references] to influence which references are checked.
+    def validate_references
+      # Skip out early if no reference validation has been requested.
+      unless @options[:validate_references].is_a?(Array) && @options[:validate_references].any?
+        return
+      end
+
+      # Iterate over all the resources and check each one that has one of the attributes being checked.
+      # Keep track of all references that are missing for ultimate inclusion in the error message.
+      missing = []
+      resources.each do |x|
+        @options[:validate_references].each do |r|
+          next unless x.key?('parameters')
+          next unless x['parameters'].key?(r)
+          missing_resources = remove_existing_resources(x['parameters'][r])
+          missing << missing_resources.map { |missing_target| { source: x, target_type: r, target_value: missing_target } }
+        end
+      end
+      missing.flatten!
+      missing.compact!
+      return if missing.empty?
+
+      # At this point there is at least one broken/missing reference. Format an error message and
+      # raise. Error message will look like this:
+      # ---
+      # Catalog has broken references: exec[subscribe caller 1] -> subscribe[Exec[subscribe target]];
+      # exec[subscribe caller 2] -> subscribe[Exec[subscribe target]]; exec[subscribe caller 2] ->
+      # subscribe[Exec[subscribe target 2]]
+      # ---
+      formatted_references = missing.map do |obj|
+        # obj[:target_value] can be a string or an array. If it's an array, create a
+        # separate error message per element of that array. This allows the total number
+        # of errors to be correct.
+        src = "#{obj[:source]['type'].downcase}[#{obj[:source]['title']}]"
+        target_val = obj[:target_value].is_a?(Array) ? obj[:target_value] : [obj[:target_value]]
+        target_val.map { |tv| "#{src} -> #{obj[:target_type].downcase}[#{tv}]" }
+      end
+      formatted_references.flatten!
+      plural = formatted_references.size == 1 ? '' : 's'
+      errors = formatted_references.join('; ')
+      raise ReferenceValidationError, "Catalog has broken reference#{plural}: #{errors}"
+    end
+
     private
 
+    # Private method: Determine if a catalog contains resource or resources, which may
+    # have been passed in as an array or a string. Return the references to resources
+    # that are missing from the catalog. (An empty array would indicate all references
+    # are present.)
+    # @param resources_to_check [String / Array] Resources to check
+    # @return [Array] References that are missing from catalog
+    def remove_existing_resources(resources_to_check)
+      rtc_array = resources_to_check.is_a?(Array) ? resources_to_check : [resources_to_check]
+      rtc_array.map do |res|
+        unless res =~ /\A([\w:]+)\[(.+)\]\z/
+          raise ArgumentError, "Resource #{res} is not in the expected format"
+        end
+        resource(type: Regexp.last_match(1), title: Regexp.last_match(2)).nil? ? res : nil
+      end.compact
+    end
+
     # Private method: Choose backend based on passed-in options
     # @param options [Hash] Options passed into constructor
     # @return [Object] OctocatalogDiff::Catalog::<whatever> object

lib/octocatalog-diff/util/parallel.rb

@@ -22,6 +22,7 @@ def initialize(opts = {})
           @args = opts.fetch(:args, {})
           @description = opts[:description] || @method.name
           @validator = opts[:validator]
+          @validator_args = opts[:validator_args] || {}
         end
 
         def execute(logger = Logger.new(StringIO.new))
@@ -30,7 +31,7 @@ def execute(logger = Logger.new(StringIO.new))
 
         def validate(result, logger = Logger.new(StringIO.new))
           return true if @validator.nil?
-          @validator.call(result, logger)
+          @validator.call(result, logger, @validator_args)
         end
       end
 

octocatalog-diff.gemspec

@@ -6,7 +6,7 @@ Gem::Specification.new do |s|
   s.required_ruby_version = '>= 2.0.0'
 
   s.name        = 'octocatalog-diff'
-  s.version     = OctocatalogDiff::Version::VERSION
+  s.version     = ENV['OCTOCATALOG_DIFF_VERSION'] || OctocatalogDiff::Version::VERSION
   s.license     = 'MIT'
   s.authors     = ['GitHub, Inc.', 'Kevin Paulisse']
   s.email       = 'opensource+octocatalog-diff@github.com'

rake/gem.rb

@@ -6,8 +6,17 @@
 module OctocatalogDiff
   # A class to contain methods and constants for cleaner code
   class Gem
+    # Override version number from the environment
+    def self.version
+      version = ENV['OCTOCATALOG_DIFF_VERSION'] || OctocatalogDiff::Version::VERSION
+      unless version == OctocatalogDiff::Version::VERSION
+        warn "WARNING: Using version #{version}, not #{OctocatalogDiff::Version::VERSION}"
+      end
+      version
+    end
+
     BASEDIR = File.expand_path('..', File.dirname(__FILE__)).freeze
-    VERSION = OctocatalogDiff::Version::VERSION
+    VERSION = version.freeze
     GEMFILE = "octocatalog-diff-#{VERSION}.gem".freeze
     PKGDIR = File.join(BASEDIR, 'pkg').freeze
     OUTFILE = File.join(BASEDIR, GEMFILE).freeze
@@ -77,8 +86,12 @@ def self.exec_command(command)
 
   task 'force-build' do
     branch = OctocatalogDiff::Gem.branch
-    warn "WARNING: Force-building from non-master branch #{branch}" unless branch == 'master'
-    OctocatalogDiff::Gem.build("octocatalog-diff-#{OctocatalogDiff::Gem::VERSION}-#{branch}.gem")
+    unless branch == 'master'
+      warn "WARNING: Force-building from non-master branch #{branch}"
+    end
+
+    version = OctocatalogDiff::Gem.version
+    OctocatalogDiff::Gem.build("octocatalog-diff-#{version}-#{branch}.gem")
   end
 
   task 'push' do
@@ -127,7 +140,7 @@ def self.exec_command(command)
 
     # Make sure the gem has been built
     branch = OctocatalogDiff::Gem.branch
-    version = OctocatalogDiff::Gem::VERSION
+    version = OctocatalogDiff::Gem.version
     gemfile = if branch == 'master'
       OctocatalogDiff::Gem::FINAL_GEMFILE
     else