From 9f1fe2a5d961a58cfd2fb0964d029974b9c4d10d Mon Sep 17 00:00:00 2001 From: Diego Alvarez Date: Wed, 28 Sep 2022 11:40:50 -0700 Subject: [PATCH] Revert https://github.com/Shopify/krane/pull/798/ --- lib/krane/cluster_resource_discovery.rb | 13 --- lib/krane/deploy_task.rb | 25 ++---- lib/krane/kubernetes_resource.rb | 5 -- .../mutating_webhook_configuration.rb | 87 ------------------- .../ingress_hook.yaml | 31 ------- .../secret_hook.yaml | 31 ------- test/integration-serial/serial_deploy_test.rb | 7 -- .../mutating_webhook_configuration_test.rb | 31 ------- 8 files changed, 8 insertions(+), 222 deletions(-) delete mode 100644 lib/krane/kubernetes_resource/mutating_webhook_configuration.rb delete mode 100644 test/fixtures/mutating_webhook_configurations/ingress_hook.yaml delete mode 100644 test/fixtures/mutating_webhook_configurations/secret_hook.yaml delete mode 100644 test/unit/krane/kubernetes_resource/mutating_webhook_configuration_test.rb diff --git a/lib/krane/cluster_resource_discovery.rb b/lib/krane/cluster_resource_discovery.rb index 00c2662d5..f23eb96ab 100644 --- a/lib/krane/cluster_resource_discovery.rb +++ b/lib/krane/cluster_resource_discovery.rb @@ -37,19 +37,6 @@ def fetch_resources(namespaced: false) end.compact.uniq { |r| "#{r['apigroup']}/#{r['kind']}" } 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 # During discovery, the api paths may not actually be at the root, so we must programatically find it. diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index 3b95e476a..738c6a754 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -30,7 +30,6 @@ custom_resource_definition horizontal_pod_autoscaler secret - mutating_webhook_configuration ).each do |subresource| require "krane/kubernetes_resource/#{subresource}" end @@ -285,26 +284,18 @@ 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 = (individuals + resources.select { |resource| webhook.matches_resource?(resource) }).uniq - resources -= individuals - end - end - [resources, individuals] - end - def validate_resources(resources) validate_globals(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 + batch_dry_run_success = validate_dry_run(resources) resources.select! { |r| r.selected?(@selector) } if @selector_as_filter + Krane::Concurrency.split_across_threads(resources) do |r| - r.validate_definition(kubectl: kubectl, selector: @selector, dry_run: individuals.include?(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 end failed_resources = resources.select(&:validation_failed?) if failed_resources.present? diff --git a/lib/krane/kubernetes_resource.rb b/lib/krane/kubernetes_resource.rb index 289876454..dae23bda1 100644 --- a/lib/krane/kubernetes_resource.rb +++ b/lib/krane/kubernetes_resource.rb @@ -226,11 +226,6 @@ def group version ? grouping : "core" end - def version - prefix, version = @definition.dig("apiVersion").split("/") - version || prefix - end - def kubectl_resource_type type end diff --git a/lib/krane/kubernetes_resource/mutating_webhook_configuration.rb b/lib/krane/kubernetes_resource/mutating_webhook_configuration.rb deleted file mode 100644 index 8d830fdf3..000000000 --- a/lib/krane/kubernetes_resource/mutating_webhook_configuration.rb +++ /dev/null @@ -1,87 +0,0 @@ -# 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? - # Note: After K8s 1.22, this should ALWAYS be false. - !%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 diff --git a/test/fixtures/mutating_webhook_configurations/ingress_hook.yaml b/test/fixtures/mutating_webhook_configurations/ingress_hook.yaml deleted file mode 100644 index d04616b06..000000000 --- a/test/fixtures/mutating_webhook_configurations/ingress_hook.yaml +++ /dev/null @@ -1,31 +0,0 @@ ---- -apiVersion: admissionregistration.k8s.io/v1 -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: - - networking.k8s.io - apiVersions: - - v1 - operations: - - CREATE - - UPDATE - resources: - - ingresses - scope: "*" - sideEffects: NoneOnDryRun - timeoutSeconds: 1 diff --git a/test/fixtures/mutating_webhook_configurations/secret_hook.yaml b/test/fixtures/mutating_webhook_configurations/secret_hook.yaml deleted file mode 100644 index b6f80ad2a..000000000 --- a/test/fixtures/mutating_webhook_configurations/secret_hook.yaml +++ /dev/null @@ -1,31 +0,0 @@ ---- -apiVersion: admissionregistration.k8s.io/v1 -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: NoneOnDryRun - timeoutSeconds: 1 diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb index 1eb321705..576e72b0d 100644 --- a/test/integration-serial/serial_deploy_test.rb +++ b/test/integration-serial/serial_deploy_test.rb @@ -551,11 +551,4 @@ def test_batch_dry_run_apply_success_precludes_individual_resource_dry_run_valid 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 diff --git a/test/unit/krane/kubernetes_resource/mutating_webhook_configuration_test.rb b/test/unit/krane/kubernetes_resource/mutating_webhook_configuration_test.rb deleted file mode 100644 index 7c77d5b09..000000000 --- a/test/unit/krane/kubernetes_resource/mutating_webhook_configuration_test.rb +++ /dev/null @@ -1,31 +0,0 @@ -# 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 - -end