Refactor CLI Interface, Add Scan fail-on-warnings (#197)

* Refactor CLI Interface, Add Scan fail-on-warnings

[Finishes #165899506]; [Finishes #165899494]; factors out command line arguments reading; adds fail-on-warnings support to cfn_nag_scan; removes repeated code across cfn_nag_scan and cfn_nag by refactoring into an executor. Post-refactor, rubocop and testing required the following included supplmental changes: CfnNagExecutor is too large if read_cli_options is included, so factors that function out to its own file; CfnNag class is too large with the added fail_on_warnings option, refactors initialize parameters as a separate CfnNagConfig class; refactors the cfn_nag integration tests to use the new signatures

* Fix Missing validate_options Call

Fixes a missing call to validate_options in the executor; refactors to stay within the 15 line per function bounds of rubocop

* Executor Tests, Name Fixes

Adds naming corrections per @erickascic 's review; adds tests for CfnNagExecutor class at 88%

* Test Coverage Bump

Adds additional tests to get cfn_nag_executor and other introduced files to 100% coverage

* Keep cli_options Backwards Compatible

Updates cli_options to be backwards compatible -- removes alphabetical ordering, splits into two options categories for cfn_nag and cfn_nag_scan

* Factory Pattern, cleanup CfnNagExecutor arguments

Refactors cli_options and argf_supplier into a factory-pattern from lambdas; changes the CfnNagExecutor to have no initializer arguments and scan to properly have a cfn_nag_scan argument instead, for clarity

* Test Coverage

Raises Argf and Options test coverage to 100%

* Resolve Test Issues, Incorporate Eric's Changeset

Incorporates Eric's changeset from Slack; removes a test that is both no longer necessary and not working under the new changes; resolves rubocop issues

* No @ Allowed In Trollop.options

Removes @ items used in Trollop.options block -- those are unsupported and cause nil class exceptions

* Resolve Failing e2e Tests - Extraneous puts

An extraneous puts options call in cfn_nag_executor was causing failures in the e2e tests; removes

* Add output format option to cfn_nag

Complies with Jesse's PR #215, adding output format option to cfn_nag

* Rename execute_single_scan, options type, deprecated argf class

Per Eric Kascic's review, removes the Argf class (which was unused after prior changes); renames execute_single_scan execute_file_or_piped_scan since the argument can be an explicit file or piped input; changes the options type value for file/piped scans to 'file' from 'cli'

bin/cfn_nag

@@ -1,110 +1,11 @@
 #!/usr/bin/env ruby
 # frozen_string_literal: true
 
-require 'trollop'
 require 'cfn-nag'
 require 'logging'
 require 'json'
 require 'rubygems/specification'
 
-# rubocop:disable Metrics/BlockLength
-opts = Trollop.options do
-  options_message = '[options] <cloudformation template path ...>|' \
-                    '<cloudformation template in STDIN>'
-  custom_rule_exceptions_message = 'Isolate custom rule exceptions - ' \
-                                   'just emit the exception without stack ' \
-                                   ' trace and keep chugging'
-  usage options_message
-  version Gem::Specification.find_by_name('cfn-nag').version
+exec = CfnNagExecutor.new
 
-  opt :debug,
-      'Enable debug output',
-      type: :boolean,
-      required: false,
-      default: false
-  opt :allow_suppression,
-      'Allow using Metadata to suppress violations',
-      type: :boolean,
-      required: false,
-      default: true
-  opt :print_suppression,
-      'Emit suppressions to stderr',
-      type: :boolean,
-      required: false,
-      default: false
-  opt :rule_directory,
-      'Extra rule directory',
-      type: :io,
-      required: false,
-      default: nil
-  opt :profile_path,
-      'Path to a profile file',
-      type: :io,
-      required: false,
-      default: nil
-  opt :blacklist_path,
-      'Path to a blacklist file',
-      type: :io,
-      required: false,
-      default: nil
-  opt :parameter_values_path,
-      'Path to a JSON file to pull Parameter values from',
-      type: :io,
-      required: false,
-      default: nil
-  opt :isolate_custom_rule_exceptions,
-      custom_rule_exceptions_message,
-      type: :boolean,
-      required: false,
-      default: false
-  opt :fail_on_warnings,
-      'Treat warnings as failing violations',
-      type: :boolean,
-      required: false,
-      default: false
-end
-# rubocop:enable Metrics/BlockLength
-
-CfnNagLogging.configure_logging(opts)
-
-profile_definition = nil
-unless opts[:profile_path].nil?
-  profile_definition = IO.read(opts[:profile_path])
-end
-
-blacklist_definition = nil
-unless opts[:blacklist_path].nil?
-  blacklist_definition = IO.read(opts[:blacklist_path])
-end
-
-parameter_values_string = nil
-unless opts[:parameter_values_path].nil?
-  parameter_values_string = IO.read(opts[:parameter_values_path])
-end
-
-cfn_nag = CfnNag.new(
-  profile_definition: profile_definition,
-  blacklist_definition: blacklist_definition,
-  rule_directory: opts[:rule_directory],
-  allow_suppression: opts[:allow_suppression],
-  print_suppression: opts[:print_suppression],
-  isolate_custom_rule_exceptions: opts[:isolate_custom_rule_exceptions]
-)
-
-total_failure_count = 0
-until ARGF.closed? || ARGF.eof?
-  results = cfn_nag.audit(cloudformation_string: ARGF.file.read,
-                          parameter_values_string: parameter_values_string)
-  ARGF.close
-
-  total_failure_count += if opts[:fail_on_warnings]
-                           results[:violations].length
-                         else
-                           results[:failure_count]
-                         end
-
-  results[:violations] = results[:violations].map(&:to_h)
-  puts JSON.pretty_generate(results)
-end
-
-exit total_failure_count
+exit exec.scan(options_type: 'file')

bin/cfn_nag_scan

@@ -1,113 +1,13 @@
 #!/usr/bin/env ruby
 # frozen_string_literal: true
 
-require 'trollop'
 require 'cfn-nag'
 require 'logging'
 require 'json'
 require 'rubygems/specification'
 
-# rubocop:disable Metrics/BlockLength
-opts = Trollop.options do
-  version Gem::Specification.find_by_name('cfn-nag').version
+exec = CfnNagExecutor.new
 
-  input_path_message = 'CloudFormation template to nag on or directory of ' \
-                       'templates.  Default is all *.json, *.yaml, *.yml ' \
-                       'and *.template recursively, but can be constrained ' \
-                       'by --template-pattern'
-
-  custom_rule_exceptions_message = 'Isolate custom rule exceptions - just ' \
-                                   'emit the exception without stack trace ' \
-                                   'and keep chugging'
-
-  template_pattern_message = 'Within the --input-path, match files to scan ' \
-                             'against this regular expression'
-
-  opt :input_path,
-      input_path_message,
-      type: :io,
-      required: true
-  opt :output_format,
-      'Format of results: [txt, json]',
-      type: :string,
-      default: 'txt'
-  opt :debug,
-      'Enable debug output',
-      type: :boolean,
-      required: false,
-      default: false
-  opt :rule_directory,
-      'Extra rule directory',
-      type: :io,
-      required: false,
-      default: nil
-  opt :profile_path,
-      'Path to a profile file',
-      type: :io,
-      required: false,
-      default: nil
-  opt :blacklist_path,
-      'Path to a blacklist file',
-      type: :io,
-      required: false,
-      default: nil
-  opt :parameter_values_path,
-      'Path to a JSON file to pull Parameter values from',
-      type: :io,
-      required: false,
-      default: nil
-  opt :allow_suppression,
-      'Allow using Metadata to suppress violations',
-      type: :boolean,
-      required: false,
-      default: true
-  opt :print_suppression,
-      'Emit suppressions to stderr',
-      type: :boolean,
-      required: false,
-      default: false
-  opt :isolate_custom_rule_exceptions,
-      custom_rule_exceptions_message,
-      type: :boolean,
-      required: false,
-      default: false
-  opt :template_pattern,
-      template_pattern_message,
-      type: :string,
-      required: false,
-      default: '..*\.json|..*\.yaml|..*\.yml|..*\.template'
-end
-# rubocop:enable Metrics/BlockLength
-
-unless %w[txt json].include?(opts[:output_format])
-  Trollop.die(:output_format,
-              'Must be txt or json')
-end
-
-CfnNagLogging.configure_logging(opts)
-
-profile_definition = nil
-unless opts[:profile_path].nil?
-  profile_definition = IO.read(opts[:profile_path])
-end
-
-blacklist_definition = nil
-unless opts[:blacklist_path].nil?
-  blacklist_definition = IO.read(opts[:blacklist_path])
-end
-
-cfn_nag = CfnNag.new(
-  profile_definition: profile_definition,
-  blacklist_definition: blacklist_definition,
-  rule_directory: opts[:rule_directory],
-  allow_suppression: opts[:allow_suppression],
-  print_suppression: opts[:print_suppression],
-  isolate_custom_rule_exceptions: opts[:isolate_custom_rule_exceptions]
-)
-
-exit cfn_nag.audit_aggregate_across_files_and_render_results(
-  input_path: opts[:input_path],
-  output_format: opts[:output_format],
-  parameter_values_path: opts[:parameter_values_path],
-  template_pattern: opts[:template_pattern]
+exit exec.scan(
+  options_type: 'scan'
 )

lib/cfn-nag.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require 'cfn-nag/cfn_nag'
+require 'cfn-nag/cfn_nag_executor'
 require 'cfn-nag/cfn_nag_logging'
 require 'cfn-nag/violation'
 require 'cfn-nag/rule_dumper'

lib/cfn-nag/cfn_nag.rb

@@ -12,24 +12,9 @@
 class CfnNag
   include ViolationFiltering
 
-  # rubocop:disable Metrics/ParameterLists
-  def initialize(profile_definition: nil,
-                 blacklist_definition: nil,
-                 rule_directory: nil,
-                 allow_suppression: true,
-                 print_suppression: false,
-                 isolate_custom_rule_exceptions: false)
-    @rule_directory = rule_directory
-    @custom_rule_loader = CustomRuleLoader.new(
-      rule_directory: rule_directory,
-      allow_suppression: allow_suppression,
-      print_suppression: print_suppression,
-      isolate_custom_rule_exceptions: isolate_custom_rule_exceptions
-    )
-    @profile_definition = profile_definition
-    @blacklist_definition = blacklist_definition
+  def initialize(config:)
+    @config = config
   end
-  # rubocop:enable Metrics/ParameterLists
 
   ##
   # Given a file or directory path, emit aggregate results to stdout
@@ -48,7 +33,11 @@ def audit_aggregate_across_files_and_render_results(input_path:,
                    output_format: output_format)
 
     aggregate_results.inject(0) do |total_failure_count, results|
-      total_failure_count + results[:file_results][:failure_count]
+      if @config.fail_on_warnings
+        total_failure_count + results[:file_results][:violations].length
+      else
+        total_failure_count + results[:file_results][:failure_count]
+      end
     end
   end
 
@@ -87,7 +76,7 @@ def audit(cloudformation_string:, parameter_values_string: nil)
       cfn_model = CfnParser.new.parse cloudformation_string,
                                       parameter_values_string,
                                       true
-      violations += @custom_rule_loader.execute_custom_rules(cfn_model)
+      violations += @config.custom_rule_loader.execute_custom_rules(cfn_model)
 
       violations = filter_violations_by_blacklist_and_profile(violations)
       violations = mark_line_numbers(violations, cfn_model)
@@ -115,15 +104,15 @@ def mark_line_numbers(violations, cfn_model)
 
   def filter_violations_by_blacklist_and_profile(violations)
     violations = filter_violations_by_profile(
-      profile_definition: @profile_definition,
-      rule_definitions: @custom_rule_loader.rule_definitions,
+      profile_definition: @config.profile_definition,
+      rule_definitions: @config.custom_rule_loader.rule_definitions,
       violations: violations
     )
 
     # this must come after - blacklist should always win
     violations = filter_violations_by_blacklist(
-      blacklist_definition: @blacklist_definition,
-      rule_definitions: @custom_rule_loader.rule_definitions,
+      blacklist_definition: @config.blacklist_definition,
+      rule_definitions: @config.custom_rule_loader.rule_definitions,
       violations: violations
     )
     violations

lib/cfn-nag/cfn_nag_config.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class CfnNagConfig
+  # rubocop:disable Metrics/ParameterLists
+  def initialize(profile_definition: nil,
+                 blacklist_definition: nil,
+                 rule_directory: nil,
+                 allow_suppression: true,
+                 print_suppression: false,
+                 isolate_custom_rule_exceptions: false,
+                 fail_on_warnings: false)
+    @rule_directory = rule_directory
+    @custom_rule_loader = CustomRuleLoader.new(
+      rule_directory: rule_directory,
+      allow_suppression: allow_suppression,
+      print_suppression: print_suppression,
+      isolate_custom_rule_exceptions: isolate_custom_rule_exceptions
+    )
+    @profile_definition = profile_definition
+    @blacklist_definition = blacklist_definition
+    @fail_on_warnings = fail_on_warnings
+  end
+  # rubocop:enable Metrics/ParameterLists
+
+  attr_reader :rule_directory
+  attr_reader :custom_rule_loader
+  attr_reader :profile_definition
+  attr_reader :blacklist_definition
+  attr_reader :fail_on_warnings
+end