[WIP] Capture admission hook changes of created resources

lib/krane/kubernetes_resource.rb

@@ -12,7 +12,7 @@ module Krane
   class KubernetesResource
     using PsychK8sCompatibility
 
-    attr_reader :name, :namespace, :context
+    attr_reader :name, :namespace, :context, :definition
     attr_writer :type, :deploy_started_at, :global
 
     GLOBAL = false
@@ -381,6 +381,18 @@ def use_generated_name(instance_data)
       @file = create_definition_tempfile
     end
 
+    # In the presence of admission controllers, an object may be modified by the server. If Krane tries to reapply its
+    # local definition (for example, pruning a Pod), it will fail because the server has already modified the object.
+    # To work around this, we update the local definition with the server's modified version.
+    def update_definition!(definition)
+      @definition = sanitize_definition(definition)
+      @file = create_definition_tempfile
+    end
+
+    def sanitize_definition(definition)
+      @definition = safe_deep_copy(@definition, definition)
+    end
+
     class Event
       EVENT_SEPARATOR = "ENDEVENT--BEGINEVENT"
       FIELD_SEPARATOR = "ENDFIELD--BEGINFIELD"
@@ -612,5 +624,32 @@ def statsd_tags
       tags = %W(context:#{context} namespace:#{namespace} type:#{type} status:#{status})
       tags | @optional_statsd_tags
     end
+
+    # Recursively construct a new definition from an updated document
+    def safe_deep_copy(current, incoming, acc={})
+      current.keys.each do |key|
+        case key
+        when Hash
+          acc[key] = _safe_deep_copy(current[key], incoming[key])
+        when Array
+          acc[key] = incoming[key].map { |v| _safe_deep_copy(v, incoming) }
+        else
+          acc[key] = incoming[key]
+        end
+      end
+      acc
+    end
+
+    def _safe_deep_copy(current, incoming, acc={})
+      case current
+      when Hash
+        acc[key] = _safe_deep_copy(current[key], incoming[key])
+      when Array
+        acc = incoming.map { |v| _safe_deep_copy(v, incoming)}
+      else
+        acc = incoming
+      end
+      acc
+    end
   end
 end

lib/krane/resource_deployer.rb

@@ -96,7 +96,6 @@ def deploy_resources(resources, prune: false, verify:, record_summary: true)
       applyables, individuals = resources.partition { |r| r.deploy_method == :apply }
       # Prunable resources should also applied so that they can  be pruned
       pruneable_types = @prune_allowlist.map { |t| t.split("/").last }
-      applyables += individuals.select { |r| pruneable_types.include?(r.type) && !r.deploy_method_override }
 
       individuals.each do |individual_resource|
         individual_resource.deploy_started_at = Time.now.utc
@@ -106,22 +105,21 @@ def deploy_resources(resources, prune: false, verify:, record_summary: true)
         when :replace
           err, status = replace_or_create_resource(individual_resource)
         when :replace_force
-          err, status = replace_or_create_resource(individual_resource, force: true)
+          updated_resource, err, status = replace_or_create_resource(individual_resource, force: true)
         else
           # Fail Fast! This is a programmer mistake.
           raise ArgumentError, "Unexpected deploy method! (#{individual_resource.deploy_method.inspect})"
         end
-
         next if status.success?
 
         raise FatalDeploymentError, <<~MSG
-          Failed to replace or create resource: #{individual_resource.id}
-          #{individual_resource.sensitive_template_content? ? '<suppressed sensitive output>' : err}
+        Failed to replace or create resource: #{individual_resource.id}
+        #{individual_resource.sensitive_template_content? ? '<suppressed sensitive output>' : err}
         MSG
       end
 
+      applyables += individuals.select { |r| pruneable_types.include?(r.type) && !r.deploy_method_override }
       apply_all(applyables, prune)
-
       if verify
         watcher = Krane::ResourceWatcher.new(resources: resources, deploy_started_at: deploy_started_at,
           timeout: @global_timeout, task_config: @task_config, sha: @current_sha)
@@ -229,8 +227,17 @@ def replace_or_create_resource(resource, force: false)
         ["replace", "-f", resource.file_path]
       end
 
-      _, err, status = kubectl.run(*args, log_failure: false, output_is_sensitive: resource.sensitive_template_content?,
-        raise_if_not_found: true, use_namespace: !resource.global?)
+      out, err, status = kubectl.run(*args, log_failure: false, output_is_sensitive: resource.sensitive_template_content?,
+        raise_if_not_found: true, output: 'json', use_namespace: !resource.global?)
+
+      updated_resource_definition = MultiJson.load(out)
+      # For resources that rely on a generateName attribute, we get the `name` from the result of the call to `create`
+      # We must explicitly set this name value so that the `apply` step for pruning can run successfully
+      if status.success? && resource.uses_generate_name?
+        resource.use_generated_name(updated_resource_definition)
+      end
+
+      resource.update_definition!(updated_resource_definition)
 
       [err, status]
     rescue Krane::Kubectl::ResourceNotFoundError
@@ -243,12 +250,15 @@ def create_resource(resource)
         output: 'json', output_is_sensitive: resource.sensitive_template_content?,
         use_namespace: !resource.global?)
 
+      updated_resource_definition = MultiJson.load(out)
       # For resources that rely on a generateName attribute, we get the `name` from the result of the call to `create`
       # We must explicitly set this name value so that the `apply` step for pruning can run successfully
       if status.success? && resource.uses_generate_name?
-        resource.use_generated_name(MultiJson.load(out))
+        resource.use_generated_name(updated_resource_definition)
       end
 
+      resource.update_definition!(updated_resource_definition)
+
       [err, status]
     end
 

lib/krane/version.rb

@@ -1,4 +1,4 @@
 # frozen_string_literal: true
 module Krane
-  VERSION = "3.6.3"
+  VERSION = "3.7.0"
 end

test/test_helper.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
-require 'pry'
+# require 'pry'
 
 if ENV["COVERAGE"]
   require 'simplecov'
@@ -34,24 +34,24 @@
   c.stubbing_non_public_method = :prevent
 end
 
-if ENV["PARALLELIZE_ME"]
-  Minitest::Reporters.use!([
-    Minitest::Reporters::ParallelizableReporter.new(
-      fast_fail: ENV['VERBOSE'] == '1',
-      slow_count: 10,
-      detailed_skip: false,
-      verbose: ENV['VERBOSE'] == '1'
-    ),
-  ])
-else
-  Minitest::Reporters.use!([
-    Minitest::Reporters::DefaultReporter.new(
-      slow_count: 10,
-      detailed_skip: false,
-      verbose: ENV['VERBOSE'] == '1'
-    ),
-  ])
-end
+# if ENV["PARALLELIZE_ME"]
+#   Minitest::Reporters.use!([
+#     Minitest::Reporters::ParallelizableReporter.new(
+#       fast_fail: ENV['VERBOSE'] == '1',
+#       slow_count: 10,
+#       detailed_skip: false,
+#       verbose: ENV['VERBOSE'] == '1'
+#     ),
+#   ])
+# else
+#   Minitest::Reporters.use!([
+#     Minitest::Reporters::DefaultReporter.new(
+#       slow_count: 10,
+#       detailed_skip: false,
+#       verbose: ENV['VERBOSE'] == '1'
+#     ),
+#   ])
+# end
 
 module Krane
   class TestCase < ::Minitest::Test

test/unit/krane/kubernetes_resource_test.rb

@@ -367,6 +367,52 @@ def test_build_handles_hardcoded_and_core_and_dynamic_objects
     assert_equal(resource.class, Krane::KubernetesResource)
   end
 
+  def test_sanitized_definition_copy
+    definition = {
+      "kind" => "Unknown",
+      "metadata" => {
+        "annotations" => {
+          "one" => "val1",
+          "two" => "val2",
+        },
+        "name" => "test",
+      },
+      "spec" => {
+        "containers" => [
+          "fake" => "key"
+        ]
+      }
+    }
+
+    incoming = {
+      "kind" => "Unknown",
+      "metadata" => {
+        "annotations" => {
+          "one" => "val1",
+          "two" => "val2",
+          "three" => "val3"
+        },
+        "name" => "test",
+      },
+      "spec" => {
+        "containers" => [
+          "fake" => "key",
+          "new" => "value",
+          "nested" => {
+            "key" => "value"
+          }
+        ],
+        "should-not" => "appear"
+      }
+    }
+
+    resource = Krane::KubernetesResource.build(namespace: "test", context: "test", logger: @logger,
+                                               statsd_tags: [], definition: definition)
+    resource.sanitize_definition(incoming)
+    incoming["spec"].delete("should-not")
+    assert_equal(resource.definition, incoming)
+  end
+
   private
 
   def kubectl

tophat/configmap-data.yml

@@ -0,0 +1,10 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: hello-cloud-configmap-data
+  labels:
+    name: hello-cloud-configmap-data
+    app: hello-cloud
+data:
+  datapoint1: value1
+  datapoint2: value2

tophat/pod.yml

@@ -0,0 +1,23 @@
+apiVersion: v1
+kind: Pod
+metadata:
+  generateName: pod-using-generate-name-
+  # name: pod
+  annotations:
+    krane.shopify.io/timeout-override: 60s
+    # container.apparmor.security.beta.kubernetes.io/command-runner: runtime/default
+  labels:
+    type: pod-using-generate-name
+    name: pod-using-generate-name
+spec:
+  activeDeadlineSeconds: 60
+  restartPolicy: Never
+  containers:
+  - name: busybox-container
+    image: busybox
+    imagePullPolicy: IfNotPresent
+    command: ["sh", "-c", "echo 'Hello from the command runner!' && test 1 -eq 1"]
+    resources:
+      requests:
+        memory: 100Mi
+        cpu: 100m