Account for mutating webhooks with side effects when batch dry-run se…

…rver-side apply (#798)

CHANGELOG.md

@@ -1,5 +1,7 @@
 ## next
 
+- Remove resources that are targeted by side-effect-inducing mutating admission webhooks from the serverside dry run batch [#798](https://github.com/Shopify/krane/pull/798)
+
 ## 2.1.5
 
 - Fix bug where the wrong dry-run flag is used for kubectl if client version is below 1.18 AND server version is 1.18+ [#793](https://github.com/Shopify/krane/pull/793).

lib/krane/cluster_resource_discovery.rb

@@ -61,6 +61,19 @@ def fetch_resources(namespaced: false)
       end
     end
 
+    def fetch_mutating_webhook_configurations
+      command = %w(get mutatingwebhookconfigurations)
+      raw_json, err, st = kubectl.run(*command, output: "json", attempts: 5, use_namespace: false)
+      if st.success?
+        JSON.parse(raw_json)["items"].map do |definition|
+          Krane::MutatingWebhookConfiguration.new(namespace: namespace, context: context, logger: logger,
+            definition: definition, statsd_tags: @namespace_tags)
+        end
+      else
+        raise FatalKubeAPIError, "Error retrieving mutatingwebhookconfigurations: #{err}"
+      end
+    end
+
     private
 
     # kubectl api-versions returns a list of group/version strings e.g. autoscaling/v2beta2

lib/krane/deploy_task.rb

@@ -30,6 +30,7 @@
   custom_resource_definition
   horizontal_pod_autoscaler
   secret
+  mutating_webhook_configuration
 ).each do |subresource|
   require "krane/kubernetes_resource/#{subresource}"
 end
@@ -277,16 +278,25 @@ def validate_configuration(prune:)
     end
     measure_method(:validate_configuration)
 
+    def partition_dry_run_resources(resources)
+      individuals = []
+      mutating_webhook_configurations = cluster_resource_discoverer.fetch_mutating_webhook_configurations
+      mutating_webhook_configurations.each do |mutating_webhook_configuration|
+        mutating_webhook_configuration.webhooks.each do |webhook|
+          individuals = resources.select { |resource| webhook.matches_resource?(resource) }
+          resources -= individuals
+        end
+      end
+      [resources, individuals]
+    end
+
     def validate_resources(resources)
       validate_globals(resources)
-      batch_dry_run_success = kubectl.server_dry_run_enabled? && validate_dry_run(resources)
+      batchable_resources, individuals = partition_dry_run_resources(resources.dup)
+      batch_dry_run_success = kubectl.server_dry_run_enabled? && validate_dry_run(batchable_resources)
+      individuals += batchable_resources unless batch_dry_run_success
       Krane::Concurrency.split_across_threads(resources) do |r|
-        # No need to pass in kubectl (and do per-resource dry run apply) if batch dry run succeeded
-        if batch_dry_run_success
-          r.validate_definition(kubectl: nil, selector: @selector, dry_run: false)
-        else
-          r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: true)
-        end
+        r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: individuals.include?(r))
       end
       failed_resources = resources.select(&:validation_failed?)
       if failed_resources.present?

lib/krane/kubernetes_resource.rb

@@ -226,6 +226,11 @@ def group
       version ? grouping : "core"
     end
 
+    def version
+      prefix, version = @definition.dig("apiVersion").split("/")
+      version || prefix
+    end
+
     def kubectl_resource_type
       type
     end

lib/krane/kubernetes_resource/mutating_webhook_configuration.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+module Krane
+  class MutatingWebhookConfiguration < KubernetesResource
+    GLOBAL = true
+
+    class Webhook
+      EQUIVALENT = 'Equivalent'
+      EXACT = 'Exact'
+
+      class Rule
+        def initialize(definition)
+          @definition = definition
+        end
+
+        def matches_resource?(resource, accept_equivalent:)
+          groups.each do |group|
+            versions.each do |version|
+              resources.each do |kind|
+                return true if (resource.group == group || group == '*' || accept_equivalent) &&
+                  (resource.version == version || version == '*' || accept_equivalent) &&
+                  (resource.type.downcase == kind.downcase.singularize || kind == "*")
+              end
+            end
+          end
+          false
+        end
+
+        def groups
+          @definition.dig('apiGroups')
+        end
+
+        def versions
+          @definition.dig('apiVersions')
+        end
+
+        def resources
+          @definition.dig('resources')
+        end
+      end
+
+      def initialize(definition)
+        @definition = definition
+      end
+
+      def side_effects
+        @definition.dig('sideEffects')
+      end
+
+      def has_side_effects?
+        !%w(None NoneOnDryRun).include?(side_effects)
+      end
+
+      def match_policy
+        @definition.dig('matchPolicy')
+      end
+
+      def matches_resource?(resource, skip_rule_if_side_effect_none: true)
+        return false if skip_rule_if_side_effect_none && !has_side_effects?
+        rules.any? do |rule|
+          rule.matches_resource?(resource, accept_equivalent: match_policy == EQUIVALENT)
+        end
+      end
+
+      def rules
+        @definition.fetch('rules', []).map { |rule| Rule.new(rule) }
+      end
+    end
+
+    def initialize(namespace:, context:, definition:, logger:, statsd_tags:)
+      @webhooks = (definition.dig('webhooks') || []).map { |hook| Webhook.new(hook) }
+      super(namespace: namespace, context: context, definition: definition,
+        logger: logger, statsd_tags: statsd_tags)
+    end
+
+    TIMEOUT = 30.seconds
+
+    def deploy_succeeded?
+      exists?
+    end
+
+    def webhooks
+      @definition.fetch('webhooks', []).map { |webhook| Webhook.new(webhook) }
+    end
+  end
+end

test/fixtures/mutating_webhook_configurations/ingress_hook.yaml

@@ -0,0 +1,31 @@
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: MutatingWebhookConfiguration
+metadata:
+  name: ingress-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1beta1
+  clientConfig:
+    service:
+      name: ingress-hook
+      namespace: test
+      path: "/ingress-hook"
+      port: 443
+  failurePolicy: Ignore
+  matchPolicy: Exact
+  name: ingress-hook.hooks.admission.krane.com
+  reinvocationPolicy: Never
+  rules:
+  - apiGroups:
+    - extensions
+    apiVersions:
+    - v1beta1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - ingresses
+    scope: "*"
+  sideEffects: Unknown
+  timeoutSeconds: 1

test/fixtures/mutating_webhook_configurations/secret_hook.yaml

@@ -0,0 +1,31 @@
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: MutatingWebhookConfiguration
+metadata:
+  name: secret-hook-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1beta1
+  clientConfig:
+    service:
+      name: secret-hook
+      namespace: test
+      path: "/secret-hook"
+      port: 443
+  failurePolicy: Ignore
+  matchPolicy: Equivalent
+  name: secret-hook.hooks.admission.krane.com
+  reinvocationPolicy: Never
+  rules:
+  - apiGroups:
+    - core
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - secrets
+    scope: "*"
+  sideEffects: Unknown
+  timeoutSeconds: 1

test/integration-serial/serial_deploy_test.rb

@@ -525,15 +525,73 @@ def test_batch_dry_run_apply_failure_falls_back_to_individual_resource_dry_run_v
   end
 
   def test_batch_dry_run_apply_success_precludes_individual_resource_dry_run_validation
-    Krane::KubernetesResource.any_instance.expects(:validate_definition).with do |kwargs|
-      kwargs[:kubectl].nil? && !kwargs[:dry_run]
+    Krane::KubernetesResource.any_instance.expects(:validate_definition).with { |params| params[:dry_run] == false }
+    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml))
+    assert_deploy_success(result)
+    assert_logs_match_all([
+      "Result: SUCCESS",
+      "Successfully deployed 1 resource",
+    ], in_order: true)
+  end
+
+  def test_resources_with_side_effect_inducing_webhooks_are_not_batched_server_side_dry_run
+    result = deploy_global_fixtures("mutating_webhook_configurations", subset: %(ingress_hook.yaml))
+    assert_deploy_success(result)
+
+    Krane::ResourceDeployer.any_instance.expects(:dry_run).with do |params|
+      # We expect the ingress to not be included in the batch run
+      params.length == 3 && (params.map(&:type).sort == ["ConfigMap", "Deployment", "Service"])
+    end.returns(true)
+
+    [Krane::ConfigMap, Krane::Deployment, Krane::Service].each do |r|
+      r.any_instance.expects(:validate_definition).with { |params| params[:dry_run] == false }
     end
-    deploy_fixtures("hello-cloud", subset: %w(secret.yml))
+    Krane::Ingress.any_instance.expects(:validate_definition).with { |params| params[:dry_run] }
+    result = deploy_fixtures('hello-cloud', subset: %w(web.yml.erb configmap-data.yml), render_erb: true)
+    assert_deploy_success(result)
+    assert_logs_match_all([
+      "Result: SUCCESS",
+      "Successfully deployed 4 resources",
+    ], in_order: true)
+  end
+
+  def test_resources_with_side_effect_inducing_webhooks_with_transitive_dependency_does_not_fail_batch_running
+    result = deploy_global_fixtures("mutating_webhook_configurations", subset: %(secret_hook.yaml))
+    assert_deploy_success(result)
+
+    actual_dry_runs = 0
+    Krane::KubernetesResource.any_instance.expects(:validate_definition).with do |params|
+      actual_dry_runs += 1 if params[:dry_run]
+      true
+    end.times(5)
+    result = deploy_fixtures('hello-cloud', subset: %w(web.yml.erb secret.yml configmap-data.yml),
+      render_erb: true) do |fixtures|
+      container = fixtures['web.yml.erb']['Deployment'][0]['spec']['template']['spec']
+      container['volumes'] = [{
+        'name' => 'secret',
+        'secret' => {
+          'secretName' => fixtures['secret.yml']["Secret"][0]['metadata']['name'],
+        },
+      }]
+    end
+    assert_deploy_success(result)
+    assert_equal(actual_dry_runs, 1)
+    assert_logs_match_all([
+      "Result: SUCCESS",
+      "Successfully deployed 5 resources",
+    ], in_order: true)
   end
 
   private
 
   def rollout_conditions_annotation_key
     Krane::Annotation.for(Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION)
   end
+
+  def mutating_webhook_fixture(path)
+    JSON.parse(File.read(path))['items'].map do |definition|
+      Krane::MutatingWebhookConfiguration.new(namespace: @namespace, context: @context, logger: @logger,
+        definition: definition, statsd_tags: @namespace_tags)
+    end
+  end
 end

test/unit/krane/kubernetes_resource/mutating_webhook_configuration_test.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+require 'test_helper'
+
+class MutatingWebhookConfigurationTest < Krane::TestCase
+  def test_load_from_json
+    definition = YAML.load_file(File.join(fixture_path("mutating_webhook_configurations"), "secret_hook.yaml"))
+    webhook_configuration = Krane::MutatingWebhookConfiguration.new(
+      namespace: 'test', context: 'nope', definition: definition,
+      logger: @logger, statsd_tags: nil
+    )
+    assert_equal(webhook_configuration.webhooks.length, 1)
+
+    raw_webhook = definition.dig('webhooks', 0)
+    webhook = webhook_configuration.webhooks.first
+    assert_equal(raw_webhook.dig('matchPolicy'), webhook.match_policy)
+    assert_equal(raw_webhook.dig('sideEffects'), webhook.side_effects)
+
+    assert_equal(webhook.rules.length, 1)
+    raw_rule = definition.dig('webhooks', 0, 'rules', 0)
+    rule = webhook.rules.first
+    assert_equal(raw_rule.dig('apiGroups'), ['core'])
+    assert_equal(rule.groups, ['core'])
+
+    assert_equal(raw_rule.dig('apiVersions'), ['v1'])
+    assert_equal(rule.versions, ['v1'])
+
+    assert_equal(raw_rule.dig('resources'), ['secrets'])
+    assert_equal(rule.resources, ['secrets'])
+  end
+
+  def test_webhook_configuration_matches_when_side_effects
+    secret_def = YAML.load_file(File.join(fixture_path('hello-cloud'), 'secret.yml'))
+    secret = Krane::Secret.new(namespace: 'test', context: 'nope', definition: secret_def,
+      logger: @logger, statsd_tags: nil)
+
+    definition = YAML.load_file(File.join(fixture_path("mutating_webhook_configurations"), "secret_hook.yaml"))
+    webhook_configuration = Krane::MutatingWebhookConfiguration.new(
+      namespace: 'test', context: 'nope', definition: definition,
+      logger: @logger, statsd_tags: nil
+    )
+    webhook = webhook_configuration.webhooks.first
+    assert(webhook.has_side_effects?)
+    assert(webhook.matches_resource?(secret))
+    assert(webhook.matches_resource?(secret, skip_rule_if_side_effect_none: true))
+    assert(webhook.matches_resource?(secret, skip_rule_if_side_effect_none: false))
+  end
+
+  def test_matches_webhook_configuration_doesnt_match_when_no_side_effects_and_flag
+    secret_def = YAML.load_file(File.join(fixture_path('hello-cloud'), 'secret.yml'))
+    secret = Krane::Secret.new(namespace: 'test', context: 'nope', definition: secret_def,
+      logger: @logger, statsd_tags: nil)
+
+    definition = YAML.load_file(File.join(fixture_path("mutating_webhook_configurations"), "secret_hook.yaml"))
+    webhook_configuration = Krane::MutatingWebhookConfiguration.new(
+      namespace: 'test', context: 'nope', definition: definition,
+      logger: @logger, statsd_tags: nil
+    )
+    webhook = webhook_configuration.webhooks.first
+    webhook.stubs(:has_side_effects?).returns(false).at_least_once
+    refute(webhook.matches_resource?(secret))
+    refute(webhook.matches_resource?(secret, skip_rule_if_side_effect_none: true))
+    assert(webhook.matches_resource?(secret, skip_rule_if_side_effect_none: false))
+  end
+
+  def test_no_match_when_policy_is_exact_and_resource_doesnt_match
+    secret_def = YAML.load_file(File.join(fixture_path('hello-cloud'), 'secret.yml'))
+    secret = Krane::Secret.new(namespace: 'test', context: 'nope', definition: secret_def,
+      logger: @logger, statsd_tags: nil)
+
+    definition = YAML.load_file(File.join(fixture_path("mutating_webhook_configurations"), "secret_hook.yaml"))
+    webhook_configuration = Krane::MutatingWebhookConfiguration.new(
+      namespace: 'test', context: 'nope', definition: definition,
+      logger: @logger, statsd_tags: nil
+    )
+
+    webhook = webhook_configuration.webhooks.first
+    assert(webhook.matches_resource?(secret))
+    webhook.expects(:match_policy).returns(Krane::MutatingWebhookConfiguration::Webhook::EXACT).at_least(1)
+    assert(webhook.matches_resource?(secret))
+    secret.expects(:group).returns('fake').once
+    refute(webhook.matches_resource?(secret))
+    secret.unstub(:group)
+    secret.expects(:type).returns('fake').once
+    refute(webhook.matches_resource?(secret))
+  end
+end