Better logging when handling failures with sensitive resources

README.md

@@ -587,8 +587,9 @@ This gem uses subclasses of `KubernetesResource` to implement custom success/fai
 3. Adjust the `TIMEOUT` constant to an appropriate value for this type.
 4. Add the new class to list of resources in
    [`deploy_task.rb`](https://github.com/Shopify/kubernetes-deploy/blob/6a0dd662735bbcc0c0cf110d049a08a044a07dd1/lib/kubernetes-deploy/deploy_task.rb#L8)
-5. Add the a basic example of the type to the hello-cloud [fixture set](https://github.com/Shopify/kubernetes-deploy/tree/master/test/fixtures/hello-cloud) and appropriate assertions to `#assert_all_up` in [`hello_cloud.rb`](https://github.com/Shopify/kubernetes-deploy/blob/master/test/helpers/fixture_sets/hello_cloud.rb). This will get you coverage in several existing tests, such as `test_full_hello_cloud_set_deploy_succeeds`.
-6. Add tests for any edge cases you foresee.
+5. Add the new resource to the [prune whitelist](https://github.com/Shopify/kubernetes-deploy/blob/faa72ec63ca8d63d7484dd58bea7f6bc0ae85ae5/lib/kubernetes-deploy/deploy_task.rb#L81)
+6. Add the a basic example of the type to the hello-cloud [fixture set](https://github.com/Shopify/kubernetes-deploy/tree/master/test/fixtures/hello-cloud) and appropriate assertions to `#assert_all_up` in [`hello_cloud.rb`](https://github.com/Shopify/kubernetes-deploy/blob/master/test/helpers/fixture_sets/hello_cloud.rb). This will get you coverage in several existing tests, such as `test_full_hello_cloud_set_deploy_succeeds`.
+7. Add tests for any edge cases you foresee.
 
 ## Code of Conduct
 Everyone is expected to follow our [Code of Conduct](https://github.com/Shopify/kubernetes-deploy/blob/master/CODE_OF_CONDUCT.md).

lib/kubernetes-deploy/deploy_task.rb

@@ -302,10 +302,10 @@ def split_templates(filename)
       raise FatalDeploymentError, "Failed to render and parse template"
     end
 
-    def record_invalid_template(err:, filename:, content:)
+    def record_invalid_template(err:, filename:, content: nil)
       debug_msg = ColorizedString.new("Invalid template: #{filename}\n").red
       debug_msg += "> Error message:\n#{FormattedLogger.indent_four(err)}"
-      debug_msg += "\n> Template content:\n#{FormattedLogger.indent_four(content)}"
+      debug_msg += "\n> Template content:\n#{FormattedLogger.indent_four(content)}" if content
       @logger.summary.add_paragraph(debug_msg)
     end
 
@@ -437,7 +437,7 @@ def apply_all(resources, prune)
         if st.success?
           log_pruning(out) if prune
         else
-          record_apply_failure(err, skip_unidentified: output_is_sensitive)
+          record_apply_failure(err, resources: resources)
           raise FatalDeploymentError, "Command failed: #{Shellwords.join(command)}"
         end
       end
@@ -452,27 +452,38 @@ def log_pruning(kubectl_output)
       @logger.summary.add_action("pruned #{pruned.length} #{'resource'.pluralize(pruned.length)}")
     end
 
-    def record_apply_failure(err, skip_unidentified: false)
+    def record_apply_failure(err, resources: [])
       warn_msg = "WARNING: Any resources not mentioned in the error(s) below were likely created/updated. " \
         "You may wish to roll back this deploy."
       @logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)
 
       unidentified_errors = []
+      sensitive_filenames = resources.select(&:kubectl_output_is_sensitive?).map { |r| File.basename(r.file_path) }
+
       err.each_line do |line|
         bad_files = find_bad_files_from_kubectl_output(line)
         if bad_files.present?
-          bad_files.each { |f| record_invalid_template(err: f[:err], filename: f[:filename], content: f[:content]) }
+          bad_files.each do |f|
+            if sensitive_filenames.include?(f[:filename])
+              # Hide the template contents in case it has senitive information
+              record_invalid_template(err: f[:err], filename: f[:filename], content: nil)
+            else
+              record_invalid_template(err: f[:err], filename: f[:filename], content: f[:content])
+            end
+          end
         else
           unidentified_errors << line
         end
       end
 
-      unless skip_unidentified
-        if unidentified_errors.present?
-          heading = ColorizedString.new('Unidentified error(s):').red
-          msg = FormattedLogger.indent_four(unidentified_errors.join)
-          @logger.summary.add_paragraph("#{heading}\n#{msg}")
-        end
+      if unidentified_errors.present? && sensitive_filenames.any?
+        warn_msg = "WARNING: There was an error applying some or all resources. The raw ouput may be sensitive and " \
+          "so cannot be displayed."
+        @logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)
+      elsif unidentified_errors.present?
+        heading = ColorizedString.new('Unidentified error(s):').red
+        msg = FormattedLogger.indent_four(unidentified_errors.join)
+        @logger.summary.add_paragraph("#{heading}\n#{msg}")
       end
     end
 

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -115,7 +115,7 @@ def validate_definition(kubectl)
       return true if st.success?
       if kubectl_output_is_sensitive?
         @validation_errors << <<-EOS
-          Validation for #{id} failed. Detailed information is unavailale as the raw error may contain sensitive data.
+          Validation for #{id} failed. Detailed information is unavailable as the raw error may contain sensitive data.
         EOS
       else
         @validation_errors << err

test/integration/kubernetes_deploy_test.rb

@@ -300,15 +300,44 @@ def test_invalid_k8s_spec_that_is_valid_yaml_but_has_no_template_path_in_error_p
     ], in_order: true)
   end
 
-  def test_apply_failure_with_sensitive_resources_hides_raw_output
+  # We want to be sure that failures to apply resources with potentially sensitive output don't leak any content.
+  # Currently our only sensitive resource is `Secret`, but we cannot reproduce a failure scenario where the kubectl
+  # output contains the filename (which would trigger some extra logging). This test stubs `Deployment` to be sensitive
+  # to recreate such a condition
+  def test_apply_failure_with_sensitive_resources_hides_template_content
     logger.level = 0
-    result = deploy_fixtures("hello-cloud", subset: ["web.yml.erb", "secret.yml"]) do |fixtures|
+    KubernetesDeploy::Deployment.any_instance.expects(:kubectl_output_is_sensitive?).returns(true).at_least_once
+    result = deploy_fixtures("hello-cloud", subset: ["web.yml.erb"]) do |fixtures|
+      bad_port_name = "http_test_is_really_long_and_invalid_chars"
       svc = fixtures["web.yml.erb"]["Service"].first
-      svc["spec"]["ports"].first["targetPort"] = "http_test_is_really_long_and_invalid_chars"
+      svc["spec"]["ports"].first["targetPort"] = bad_port_name
+      deployment = fixtures["web.yml.erb"]["Deployment"].first
+      deployment["spec"]["template"]["spec"]["containers"].first["ports"].first["name"] = bad_port_name
     end
     assert_deploy_failure(result)
     refute_logs_match(/Kubectl err:/)
-    refute_logs_match(/Unidentified error/)
+    assert_logs_match_all([
+      "Command failed: apply -f",
+      /Invalid template: Deployment-web.*\.yml/,
+    ])
+    refute_logs_match("kind: Deployment") # content of the sensitive template
+  end
+
+  def test_apply_failure_with_sensitive_resources_hides_raw_output
+    logger.level = 0
+    # An invalid PATCH produces the kind of error we want to catch, so first create a valid secret:
+    assert_deploy_success(deploy_fixtures("hello-cloud", subset: %w(secret.yml)))
+    # Then try to PATCH an immutable field
+    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml)) do |fixtures|
+      secret = fixtures["secret.yml"]["Secret"].first
+      secret["type"] = "something/invalid"
+    end
+    assert_deploy_failure(result)
+    refute_logs_match(/Kubectl err:/)
+    assert_logs_match_all([
+      "Command failed: apply -f",
+      /WARNING:.*The raw ouput may be sensitive and so cannot be displayed/,
+    ])
   end
 
   def test_bad_container_image_on_unmanaged_pod_halts_and_fails_deploy