Merge pull request #36 from github/kpaulisse-validate-references

Validate references

Author: kpaulisse

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
 

doc/optionsref.md

@@ -47,6 +47,8 @@ Usage: octocatalog-diff [command line options]
         --ignore-attr "attr1,attr2,..."
                                      Attributes to ignore
         --[no-]display-source        Show source file and line for each difference
+        --[no-]validate-references "before,require,subscribe,notify"
+                                     References to validate
         --[no-]compare-file-text     Compare text, not source location, of file resources
         --[no-]storeconfigs          Enable integration with puppetdb for collected resources
         --retry-failed-catalog N     Retry building a failed catalog N times
@@ -1028,4 +1030,19 @@ These files must exist and be in Puppet catalog format. (<a href="../lib/octocat
     </td>
   </tr>
 
+  <tr>
+    <td valign=top>
+      <pre><code>--validate-references
+--no-validate-references </code></pre>
+    </td>
+    <td valign=top>
+      References to validate
+    </td>
+    <td valign=top>
+      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. (<a href="../lib/octocatalog-diff/catalog-diff/cli/options/validate_references.rb">validate_references.rb</a>)
+    </td>
+  </tr>
+
 </table>

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,66 @@ 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 = resources_missing_from_catalog(x['parameters'][r])
+          next unless missing_resources.any?
+          missing.concat missing_resources.map { |missing_target| { source: x, target_type: r, target_value: missing_target } }
+        end
+      end
+      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: Given a list of resources to check, return the references from
+    # that list that are missing from the catalog. (An empty array returned would indicate
+    # all references are present in the catalog.)
+    # @param resources_to_check [String / Array] Resources to check
+    # @return [Array] References that are missing from catalog
+    def resources_missing_from_catalog(resources_to_check)
+      [resources_to_check].flatten.select 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?
+      end
+    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