From e96ebd55da20dc6aaf58d60ab5a33a95671f2ea8 Mon Sep 17 00:00:00 2001 From: Kevin Paulisse Date: Sun, 14 Feb 2021 18:59:20 -0600 Subject: [PATCH] Compare file text changes and docs --- doc/advanced-compare-file-text.md | 79 +++++++++++++++++ doc/optionsref.md | 30 +++++++ .../catalog-util/fileresources.rb | 53 ++++++++---- .../cli/options/compare_file_text.rb | 18 ++++ .../catalogs/catalog-test-file-v4-tagged.json | 57 ++++++++++++ .../broken/modules/test/manifests/init.pp | 2 + .../convert_file_resources_spec.rb | 86 +++++++++++++++++-- .../tests/catalog-util/fileresources_spec.rb | 58 ++++++++++++- 8 files changed, 361 insertions(+), 22 deletions(-) create mode 100644 doc/advanced-compare-file-text.md create mode 100644 spec/octocatalog-diff/fixtures/catalogs/catalog-test-file-v4-tagged.json diff --git a/doc/advanced-compare-file-text.md b/doc/advanced-compare-file-text.md new file mode 100644 index 00000000..34a88240 --- /dev/null +++ b/doc/advanced-compare-file-text.md @@ -0,0 +1,79 @@ +# Compare file text + +`octocatalog-diff` contains functionality to detect changes in file content for file resources that use the `source` attribute like this: + +``` +file { '/etc/logrotate.d/my-service.conf': + source => 'puppet:///modules/logrotate/etc/logrotate.d/my-service.conf', +} +``` + +When the `source` attribute is used, the catalog contains the value of the attribute (in the example above, `source` = `puppet:///modules/logrotate/etc/logrotate.d/my-service.conf`). However, the catalog does not contain the content of the file. When an agent applies the catalog, the file found on the server or in the code base at `modules/logrotate/file/etc/logrotate.d/my-service.conf` will be installed into `/etc/logrotate.d/my-service.conf`. + +If a developer creates a change to this file, the catalog will not indicate this change. This means that so long as the `source` location does not change, any tool analyzing just the catalog would not detect a "diff" resulting from changes in the underlying file itself. + +However, since applying the catalog could change the content of a file on the target node, `octocatalog-diff` has a feature that will do the following for any `source => 'puppet:///modules/...'` files: + +- Locate the source file in the Puppet code +- Substitute the content of the file into the generated catalog (remove `source` and populate `content`) +- Display the "diff" in the now-populated `content` field + +This feature is available only when the catalogs are being compiled from local code. This feature is not available, and will be automatically disabled, when pulling catalogs from PuppetDB or a Puppet server. + +Note: In Puppet >= 4.4 there is an option in Puppet itself called "static catalogs" which if enabled will cause the checksum of the file to be included in the catalog. However, the `octocatalog-diff` feature described here is still useful because it can be used to display a "diff" of the change rather than just displaying a "diff" of a checksum. +## Command line options + +### `--compare-file-text` and `--no-compare-file-text` + +The feature described above is enabled by default, and no special setup is required. + +To disable this feature for a specific run, add `--no-compare-file-text` to the command line. + +To disable this feature by default, add the following to a [configuration](/doc/configuration.md) file: + +```ruby +settings[:compare_file_text] = false +``` + +If this feature is disabled by default in a configuration file, add `--compare-file-text` to enable the feature for this specific run. + +Note that the feature will be automatically disabled, regardless of configuration or command line options, if catalogs are being pulled from PuppetDB or a Puppet server. + +### `--compare-file-text-ignore-tags` + +To disable this feature for specific `file` resources, set a tag on the resources for which the comparison is undesired. For example: + +``` +file { '/etc/logrotate.d/my-service.conf': + source => 'puppet:///modules/logrotate/etc/logrotate.d/my-service.conf', +} + +file { '/etc/logrotate.d/other-service.conf': + tag => ['compare-file-text-disable'], + source => 'puppet:///modules/logrotate/etc/logrotate.d/other-service.conf', +} +``` + +Inform `octocatalog-diff` of the name of the tag either via the command line (`--compare-file-text-ignore-tags "compare-file-text-disable"`) or via a [configuration](/doc/configuration.md) file: + +```ruby +settings[:compare_file_text_ignore_tags] = %w(compare-file-text-disable) +``` + +With this example setup, the file text would be compared for `/etc/logrotate.d/my-service.conf` but would NOT be compared for `/etc/logrotate.d/other-service.conf`. + +Notes: + +1. `--compare-file-text-ignore-tags` can take comma-separated arguments if there are multiple tags, e.g.: `--compare-file-text-ignore-tags tag1,tag2`. + +1. When defining values in the configuration file, `settings[:compare_file_text_ignore_tags]` must be an array. (The default value is an empty array, `[]`.) + +1. "compare-file-text-disable" is used as the name of the tag in the example above, but it is not a magical value. Any valid tag name can be used. + +## Notes + +1. If the underlying file source cannot be found when building the "to" catalog, an exception will be raised. This will display the resource type and title, the value of `source` that is invalid, and the file name and line number of the Puppet manifest that declared the resource. + +1. If the underlying file source cannot be found when building the "from" catalog, no exception will be raised. This behavior allows `octocatalog-diff` to work correctly even if there is a problem in the "from" catalog that is being corrected in the "to" catalog. (Of course, if the underlying file source can't be found in the "to" catalog either, an exception is raised -- see #1.) + +1. No processing takes place for file `source` whose values do not start with `puppet:///modules/`. diff --git a/doc/optionsref.md b/doc/optionsref.md index 55f92cd4..963919ca 100644 --- a/doc/optionsref.md +++ b/doc/optionsref.md @@ -185,6 +185,8 @@ Usage: octocatalog-diff [command line options] --no-ignore-tags Disable ignoring based on tags --ignore-tags STRING1[,STRING2[,...]] Specify tags to ignore + --compare-file-text-ignore-tags STRING1[,STRING2[,...]] + Tags that exclude a file resource from text comparison --[no-]preserve-environments Enable or disable environment preservation --environment STRING Environment for catalog compilation globally --to-environment STRING Environment for catalog compilation for the to branch @@ -374,6 +376,21 @@ what is most often desired. (compare_file_text.rb) + + + 
--create-symlinks STRING1[,STRING2[,...]]
@@ -1897,6 +1914,19 @@ has no effect when `--display-detail-add` is not used. (use_lcs.rb) + + + 
--validate-references
diff --git a/lib/octocatalog-diff/catalog-util/fileresources.rb b/lib/octocatalog-diff/catalog-util/fileresources.rb
index 8621b0a0..0c8b24f9 100644
--- a/lib/octocatalog-diff/catalog-util/fileresources.rb
+++ b/lib/octocatalog-diff/catalog-util/fileresources.rb
@@ -13,9 +13,16 @@ class FileResources
       # Public method: Convert file resources to text. See the description of the class
       # just above for details.
       # @param obj [OctocatalogDiff::Catalog] Catalog object (will be modified)
+      # @param environment [String] Environment (defaults to production)
       def self.convert_file_resources(obj, environment = 'production')
         return unless obj.valid? && obj.compilation_dir.is_a?(String) && !obj.compilation_dir.empty?
-        _convert_file_resources(obj.resources, obj.compilation_dir, environment)
+        _convert_file_resources(
+          obj.resources,
+          obj.compilation_dir,
+          environment,
+          obj.options[:compare_file_text_ignore_tags],
+          obj.options[:tag]
+        )
         begin
           obj.catalog_json = ::JSON.generate(obj.catalog)
         rescue ::JSON::GeneratorError => exc
@@ -70,8 +77,11 @@ def self.module_path(dir)
       # Internal method: Static method to convert file resources. The compilation directory is
       # required, or else this is a no-op. The passed-in array of resources is modified by this method.
       # @param resources [Array] Array of catalog resources
-      # @param compilation_dir [String] Compilation directory (so files can be looked up)
-      def self._convert_file_resources(resources, compilation_dir, environment = 'production')
+      # @param compilation_dir [String] Compilation directory
+      # @param environment [String] Environment
+      # @param ignore_tags [Array] Tags that exempt a resource from conversion
+      # @param to_from_tag [String] Either "to" or "from" for catalog type
+      def self._convert_file_resources(resources, compilation_dir, environment, ignore_tags, to_from_tag)
         # Calculate compilation directory. There is not explicit error checking here because
         # there is on-demand, explicit error checking for each file within the modification loop.
         return unless compilation_dir.is_a?(String) && compilation_dir != ''
@@ -88,14 +98,10 @@ def self._convert_file_resources(resources, compilation_dir, environment = 'prod
         resources.map! do |resource|
           if resource_convertible?(resource)
             path = file_path(resource['parameters']['source'], modulepaths)
-            if path.nil?
-              # Pass this through as a wrapped exception, because it's more likely to be something wrong
-              # in the catalog itself than it is to be a broken setup of octocatalog-diff.
-              message = "Errno::ENOENT: Unable to resolve '#{resource['parameters']['source']}'!"
-              raise OctocatalogDiff::Errors::CatalogError, message
-            end
 
-            if File.file?(path)
+            if resource['tags'] && ignore_tags && (resource['tags'] & ignore_tags).any?
+              # Resource tagged not to be converted -- do nothing.
+            elsif path && File.file?(path)
               # If the file is found, read its content. If the content is all ASCII, substitute it into
               # the 'content' parameter for easier comparison. If not, instead populate the md5sum.
               # Delete the 'source' attribute as well.
@@ -103,23 +109,40 @@ def self._convert_file_resources(resources, compilation_dir, environment = 'prod
               is_ascii = content.force_encoding('UTF-8').ascii_only?
               resource['parameters']['content'] = is_ascii ? content : '{md5}' + Digest::MD5.hexdigest(content)
               resource['parameters'].delete('source')
-            elsif File.exist?(path)
+            elsif path && File.exist?(path)
               # We are not handling recursive file installs from a directory or anything else.
               # However, the fact that we found *something* at this location indicates that the catalog
               # is probably correct. Hence, the very general .exist? check.
+            elsif to_from_tag == 'from'
+              # Don't raise an exception for an invalid source in the "from"
+              # catalog, because the developer may be fixing this in the "to"
+              # catalog. If it's broken in the "to" catalog as well, the
+              # exception will be raised when this code runs on that catalog.
             else
-              # This is probably a bug
-              # :nocov:
-              raise "Unable to find '#{resource['parameters']['source']}' at #{path}!"
-              # :nocov:
+              # Pass this through as a wrapped exception, because it's more likely to be something wrong
+              # in the catalog itself than it is to be a broken setup of octocatalog-diff.
+              #
+              # Example error: 'puppet:///modules/test/tmp/bar' in File[/tmp/bar]
+              # (/x/modules/test/manifests/init.pp:46)>
+              source = resource['parameters']['source']
+              type = resource['type']
+              title = resource['title']
+              file = resource['file'].sub(Regexp.new('^' + Regexp.escape(env_dir) + '/'), '')
+              line = resource['line']
+              message = "Unable to resolve source=>'#{source}' in #{type}[#{title}] (#{file}:#{line})"
+              raise OctocatalogDiff::Errors::CatalogError, message
             end
           end
+
           resource
         end
       end
 
       # Internal method: Determine if a resource is convertible. It is convertible if it
       # is a file resource with no declared 'content' and with a declared and parseable 'source'.
+      # It is not convertible if the resource is tagged with one of the tags declared by
+      # the option `--compare-file-text-ignore-tags`.
       # @param resource [Hash] Resource to check
       # @return [Boolean] True of resource is convertible, false if not
       def self.resource_convertible?(resource)
diff --git a/lib/octocatalog-diff/cli/options/compare_file_text.rb b/lib/octocatalog-diff/cli/options/compare_file_text.rb
index 9db998e6..051d21de 100644
--- a/lib/octocatalog-diff/cli/options/compare_file_text.rb
+++ b/lib/octocatalog-diff/cli/options/compare_file_text.rb
@@ -15,3 +15,21 @@ def parse(parser, options)
     end
   end
 end
+
+# Sometimes there is a particular file resource for which the file text
+# comparison is not desired, while not disabling that option globally. Similar
+# to --ignore_tags, it's possible to tag the file resource and exempt it from
+# the --compare_file_text checks.
+# @param parser [OptionParser object] The OptionParser argument
+# @param options [Hash] Options hash being constructed; this is modified in this method.
+OctocatalogDiff::Cli::Options::Option.newoption(:compare_file_text_ignore_tags) do
+  has_weight 415
+
+  def parse(parser, options)
+    description = 'Tags that exclude a file resource from text comparison'
+    parser.on('--compare-file-text-ignore-tags STRING1[,STRING2[,...]]', Array, description) do |x|
+      options[:compare_file_text_ignore_tags] ||= []
+      options[:compare_file_text_ignore_tags].concat x
+    end
+  end
+end
diff --git a/spec/octocatalog-diff/fixtures/catalogs/catalog-test-file-v4-tagged.json b/spec/octocatalog-diff/fixtures/catalogs/catalog-test-file-v4-tagged.json
new file mode 100644
index 00000000..b4cc2db6
--- /dev/null
+++ b/spec/octocatalog-diff/fixtures/catalogs/catalog-test-file-v4-tagged.json
@@ -0,0 +1,57 @@
+{
+  "document_type": "Catalog",
+  "tags": ["settings","test"],
+  "name": "my.rspec.node",
+  "version": "production",
+  "environment": "production",
+  "resources": [
+    {
+      "type": "Stage",
+      "title": "main",
+      "tags": ["stage"],
+      "exported": false,
+      "parameters": {
+        "name": "main"
+      }
+    },
+    {
+      "type": "Class",
+      "title": "Settings",
+      "tags": ["class","settings"],
+      "exported": false
+    },
+    {
+      "type": "File",
+      "title": "/tmp/foo",
+      "tags": ["file","class"],
+      "file": "/x/modules/test/manifests/init.pp",
+      "line": 37,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "mode": "0440",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/test/tmp/foo"
+      }
+    },
+    {
+      "type": "File",
+      "title": "/tmp/bar",
+      "tags": ["file","class","no_compare_tag"],
+      "file": "/x/modules/test/manifests/init.pp",
+      "line": 46,
+      "exported": false,
+      "parameters": {
+        "backup": false,
+        "mode": "0440",
+        "owner": "root",
+        "group": "root",
+        "source": "puppet:///modules/test/tmp/bar"
+      }
+    }
+ ],
+  "classes": [
+    "test"
+  ]
+}
diff --git a/spec/octocatalog-diff/fixtures/repos/convert-resources/broken/modules/test/manifests/init.pp b/spec/octocatalog-diff/fixtures/repos/convert-resources/broken/modules/test/manifests/init.pp
index 5be6fa10..6f436741 100644
--- a/spec/octocatalog-diff/fixtures/repos/convert-resources/broken/modules/test/manifests/init.pp
+++ b/spec/octocatalog-diff/fixtures/repos/convert-resources/broken/modules/test/manifests/init.pp
@@ -1,9 +1,11 @@
 class test {
   file { '/tmp/foo1':
+    tag    => ['_convert_file_resources_foo1_'],
     source => 'puppet:///modules/test/foo-new',
   }
 
   file { '/tmp/foo2':
+    tag    => ['_convert_file_resources_foo2_'],
     source => 'puppet:///modules/test/foo-old',
   }
 }
diff --git a/spec/octocatalog-diff/integration/convert_file_resources_spec.rb b/spec/octocatalog-diff/integration/convert_file_resources_spec.rb
index c7327599..811acaa6 100644
--- a/spec/octocatalog-diff/integration/convert_file_resources_spec.rb
+++ b/spec/octocatalog-diff/integration/convert_file_resources_spec.rb
@@ -147,7 +147,7 @@
     end
   end
 
-  context 'with broken repo' do
+  context 'with broken reference in to-catalog' do
     it 'should fail' do
       result = OctocatalogDiff::Integration.integration(
         spec_fact_file: 'facts.yaml',
@@ -160,8 +160,84 @@
       )
       expect(result[:exitcode]).to eq(-1)
       expect(result[:exception]).to be_a_kind_of(OctocatalogDiff::Errors::CatalogError)
-      expect(result[:exception].message).to match(/Errno::ENOENT/)
-      expect(result[:exception].message).to match(%r{Unable to resolve 'puppet:///modules/test/foo-new'})
+      expect(result[:exception].message).to match(%r{\AUnable to resolve source=>'puppet:///modules/test/foo-new' in File\[/tmp/foo1\] \(modules/test/manifests/init.pp:\d+\)\z}) # rubocop:disable Metrics/LineLength
+    end
+
+    context 'with --convert-file-resources-ignore-tags' do
+      before(:all) do
+        @result = OctocatalogDiff::Integration.integration(
+          spec_fact_file: 'facts.yaml',
+          spec_repo_old: 'convert-resources/old',
+          spec_repo_new: 'convert-resources/broken',
+          argv: [
+            '-n', 'rspec-node.github.net',
+            '--compare-file-text',
+            '--compare-file-text-ignore-tags', '_convert_file_resources_foo1_'
+          ]
+        )
+      end
+
+      it 'should compile the catalog' do
+        expect(@result[:exitcode]).not_to eq(-1), OctocatalogDiff::Integration.format_exception(@result)
+        expect(@result[:exitcode]).to eq(2), "Runtime error: #{@result[:logs]}"
+        expect(@result[:diffs]).to be_a_kind_of(Array)
+      end
+
+      it 'should leave /tmp/foo1 parameters:source alone in the new catalog' do
+        resource = {
+          diff_type: '!',
+          type: 'File',
+          title: '/tmp/foo1',
+          structure: %w(parameters source),
+          old_value: nil,
+          new_value: 'puppet:///modules/test/foo-new'
+        }
+        expect(OctocatalogDiff::Spec.diff_match?(@result[:diffs], resource)).to eq(true)
+      end
+    end
+  end
+
+  context 'with broken reference in from-catalog' do
+    before(:all) do
+      @result = OctocatalogDiff::Integration.integration(
+        spec_fact_file: 'facts.yaml',
+        spec_repo_old: 'convert-resources/broken',
+        spec_repo_new: 'convert-resources/old',
+        argv: [
+          '-n', 'rspec-node.github.net',
+          '--compare-file-text'
+        ]
+      )
+    end
+
+    it 'should succeed' do
+      expect(@result[:exitcode]).not_to eq(-1), OctocatalogDiff::Integration.format_exception(@result)
+      expect(@result[:exitcode]).to eq(2), "Runtime error: #{@result[:logs]}"
+      expect(@result[:diffs]).to be_a_kind_of(Array)
+    end
+
+    it 'should leave /tmp/foo1 parameters:source alone in the old catalog' do
+      resource = {
+        diff_type: '!',
+        type: 'File',
+        title: '/tmp/foo1',
+        structure: %w(parameters source),
+        old_value: 'puppet:///modules/test/foo-new',
+        new_value: nil
+      }
+      expect(OctocatalogDiff::Spec.diff_match?(@result[:diffs], resource)).to eq(true)
+    end
+
+    it 'should populate /tmp/foo1 parameters:content in the new catalog' do
+      resource = {
+        diff_type: '!',
+        type: 'File',
+        title: '/tmp/foo1',
+        structure: %w(parameters content),
+        old_value: nil,
+        new_value: "content of foo-old\n"
+      }
+      expect(OctocatalogDiff::Spec.diff_match?(@result[:diffs], resource)).to eq(true)
     end
   end
 
@@ -256,9 +332,7 @@
     it 'should fail' do
       expect(@result[:exitcode]).to eq(-1)
       expect(@result[:exception]).to be_a_kind_of(OctocatalogDiff::Errors::CatalogError)
-      expect(@result[:exception].message).to match(/Errno::ENOENT/)
-      rexp = Regexp.new(Regexp.escape("Unable to resolve '[\"puppet:///modules/test/foo-bar\""))
-      expect(@result[:exception].message).to match(rexp)
+      expect(@result[:exception].message).to match(%r{\AUnable to resolve source=>'\["puppet:///modules/test/foo-bar", "puppet:///modules/test/foo-baz"\]' in File\[/tmp/foo\] \(modules/test/manifests/array3.pp:\d+\)\z}) # rubocop:disable Metrics/LineLength
     end
   end
 end
diff --git a/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb b/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb
index 1bb4cef3..10d37898 100644
--- a/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb
+++ b/spec/octocatalog-diff/tests/catalog-util/fileresources_spec.rb
@@ -225,9 +225,10 @@ def catalog_from_fixture(path)
       obj.resources[2]['parameters']['source'] = 'puppet:///modules/this/does/not/exist'
 
       # Perform test
+      expected_error = "Unable to resolve source=>'puppet:///modules/this/does/not/exist' in File[/tmp/foo] (/x/modules/test/manifests/init.pp:37)" # rubocop:disable Metrics/LineLength
       expect do
         OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
-      end.to raise_error(OctocatalogDiff::Errors::CatalogError, %r{Unable to resolve 'puppet:///modules/this/does/not/exist'})
+      end.to raise_error(OctocatalogDiff::Errors::CatalogError, expected_error)
     end
 
     it 'should return original if compilation_dir is not a string' do
@@ -276,6 +277,61 @@ def catalog_from_fixture(path)
       OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
       expect(obj.error_message).to eq('Failed to generate JSON: test')
     end
+
+    it 'should pass options[:compare_file_text_ignore_tags] properly' do
+      # Set up test
+      obj = catalog_from_fixture('catalogs/catalog-test-file-v4-tagged.json')
+      obj.compilation_dir = @tmpdir
+      obj.options[:compare_file_text_ignore_tags] = %w[no_compare_tag]
+      resources_save = obj.resources.dup
+      File.open(File.join(@tmpdir, 'modules', 'test', 'files', 'tmp', 'foo'), 'w') { |f| f.write "\u0256" }
+
+      # Perform test
+      OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
+      expect(obj.resources).to be_a_kind_of(Array)
+      expect(obj.resources.size).to eq(4)
+      expect(obj.resources[0]).to eq(resources_save[0])
+      expect(obj.resources[1]).to eq(resources_save[1])
+      expect(obj.resources[2]['parameters']['content']).to eq('{md5}165406e473f38ababa17a05696e2ef70')
+      expect(obj.resources[2]['parameters'].key?('source')).to eq(false)
+      expect(obj.resources[3]).to eq(resources_save[3])
+    end
+
+    it 'should raise the proper error when a file is missing' do
+      # Set up test
+      obj = catalog_from_fixture('catalogs/catalog-test-file-v4-tagged.json')
+      obj.compilation_dir = @tmpdir
+      File.open(File.join(@tmpdir, 'modules', 'test', 'files', 'tmp', 'foo'), 'w') { |f| f.write "\u0256" }
+
+      # Perform test
+      expected_error = "Unable to resolve source=>'puppet:///modules/test/tmp/bar' in File[/tmp/bar] (/x/modules/test/manifests/init.pp:46)" # rubocop:disable Metrics/LineLength
+      expect do
+        OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
+      end.to raise_error(OctocatalogDiff::Errors::CatalogError, expected_error)
+    end
+
+    it 'should not raise due to a missing file when error is suppressed by a tag' do
+      obj = catalog_from_fixture('catalogs/catalog-test-file-v4.json')
+      obj.compilation_dir = @tmpdir
+      obj.options[:compare_file_text_ignore_tags] = %w(suppress-error-test-tag)
+      obj.resources[2]['parameters']['source'] = 'puppet:///modules/this/does/not/exist'
+      obj.resources[2]['tags'] = %w(suppress-error-test-tag)
+
+      expect do
+        OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
+      end.not_to raise_error
+    end
+
+    it 'should not raise due to a missing file in the "from" catalog' do
+      obj = catalog_from_fixture('catalogs/catalog-test-file-v4.json')
+      obj.compilation_dir = @tmpdir
+      obj.resources[2]['parameters']['source'] = 'puppet:///modules/this/does/not/exist'
+      obj.options[:tag] = 'from'
+
+      expect do
+        OctocatalogDiff::CatalogUtil::FileResources.convert_file_resources(obj)
+      end.not_to raise_error
+    end
   end
 
   describe '#resource_convertible?' do