From ee2b17d871e45e710f53664336929c77d9cb3825 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Wed, 16 Dec 2020 11:43:42 -0800 Subject: [PATCH 1/6] Fix predeploy_sequence when there are conflicts between built-ins and CRDs --- ...ify-github-io-ruby-style-guide-rubocop-yml | 1 + lib/krane/deploy_task.rb | 38 +++++++++---------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/.rubocop-http---shopify-github-io-ruby-style-guide-rubocop-yml b/.rubocop-http---shopify-github-io-ruby-style-guide-rubocop-yml index c68248c29..e77a04344 100644 --- a/.rubocop-http---shopify-github-io-ruby-style-guide-rubocop-yml +++ b/.rubocop-http---shopify-github-io-ruby-style-guide-rubocop-yml @@ -195,6 +195,7 @@ Style/FrozenStringLiteralComment: SupportedStyles: - always - never + SafeAutoCorrect: true Style/GlobalVars: AllowedVariables: [] diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index b95cede1f..4796bd0c8 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -57,24 +57,24 @@ class DeployTask ) def predeploy_sequence - default_group = { group: nil } - before_crs = %w( - ResourceQuota - NetworkPolicy - ConfigMap - PersistentVolumeClaim - ServiceAccount - Role - RoleBinding - Secret - ).map { |r| [r, default_group] } - - after_crs = %w( - Pod - ).map { |r| [r, default_group] } - - crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, { group: cr.group }] } - Hash[before_crs + crs + after_crs] + default_group = { groups: nil } + before_crs = { + 'ResourceQuota' => default_group, + 'NetworkPolicy' => { groups: %w(extensions networking.k8s.io) }, + 'ConfigMap' => default_group, + 'PersistentVolumeClaim' => default_group, + 'ServiceAccount' => default_group, + 'Role' => default_group, + 'RoleBinding' => default_group, + 'Secret' => default_group, + } + + after_crs = { 'Pod' => default_group } + + crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, { groups: [cr.group] }] }.to_h + predeploy_hash = before_crs + predeploy_hash.merge!(crs) { |_k, old, new| { groups: old[:groups] + new[:groups] } } + predeploy_hash.merge(after_crs) { |_k, old, new| { groups: old[:groups] + new[:groups] } } end def prune_whitelist @@ -216,7 +216,7 @@ def ejson_provisioners def deploy_has_priority_resources?(resources) resources.any? do |r| next unless (pr = predeploy_sequence[r.type]) - !pr[:group] || pr[:group] == r.group + !pr[:groups] || pr[:groups].include?(r.group) end end From 505f2cfbf1228c8bfecf82672daf821d87511ec2 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Wed, 16 Dec 2020 11:57:37 -0800 Subject: [PATCH 2/6] Don't predeploy all CRs --- lib/krane/deploy_task.rb | 44 +++++++++++++++++++--------------- lib/krane/resource_deployer.rb | 3 +-- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index 4796bd0c8..9ef8473d9 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -57,24 +57,30 @@ class DeployTask ) def predeploy_sequence - default_group = { groups: nil } - before_crs = { - 'ResourceQuota' => default_group, - 'NetworkPolicy' => { groups: %w(extensions networking.k8s.io) }, - 'ConfigMap' => default_group, - 'PersistentVolumeClaim' => default_group, - 'ServiceAccount' => default_group, - 'Role' => default_group, - 'RoleBinding' => default_group, - 'Secret' => default_group, - } - - after_crs = { 'Pod' => default_group } - - crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, { groups: [cr.group] }] }.to_h - predeploy_hash = before_crs - predeploy_hash.merge!(crs) { |_k, old, new| { groups: old[:groups] + new[:groups] } } - predeploy_hash.merge(after_crs) { |_k, old, new| { groups: old[:groups] + new[:groups] } } + default_group = { skip_groups: [] } + before_crs = %w( + ResourceQuota + NetworkPolicy + ConfigMap + PersistentVolumeClaim + ServiceAccount + Role + RoleBinding + Secret + ).map { |r| [r, default_group] } + + after_crs = %w( + Pod + ).map { |r| [r, default_group] } + + crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, default_group] } + predeploy_hash = Hash[before_crs + crs + after_crs] + + cluster_resource_discoverer.crds.reject(&:predeployed?).each do |cr| + predeploy_hash[cr.kind][:skip_groups] << cr.group if predeploy_hash[cr.kind] + end + + predeploy_hash end def prune_whitelist @@ -216,7 +222,7 @@ def ejson_provisioners def deploy_has_priority_resources?(resources) resources.any? do |r| next unless (pr = predeploy_sequence[r.type]) - !pr[:groups] || pr[:groups].include?(r.group) + !pr[:skip_groups].include?(r.group) end end diff --git a/lib/krane/resource_deployer.rb b/lib/krane/resource_deployer.rb index 3990fb0af..b0cbc7782 100644 --- a/lib/krane/resource_deployer.rb +++ b/lib/krane/resource_deployer.rb @@ -48,8 +48,7 @@ def predeploy_priority_resources(resource_list, predeploy_sequence) predeploy_sequence.each do |resource_type, attributes| matching_resources = resource_list.select do |r| - r.type == resource_type && - (!attributes[:group] || r.group == attributes[:group]) + r.type == resource_type && !attributes[:skip_groups].include?(r.group) end StatsD.client.gauge('priority_resources.count', matching_resources.size, tags: statsd_tags) From aa0a06544299898192b34d7465a70d0bebb2a943 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Wed, 16 Dec 2020 15:07:25 -0800 Subject: [PATCH 3/6] Track in and out --- lib/krane/deploy_task.rb | 14 +++++++++----- lib/krane/resource_deployer.rb | 4 +++- test/unit/krane/resource_deployer_test.rb | 2 +- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index 9ef8473d9..ba9557633 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -57,7 +57,7 @@ class DeployTask ) def predeploy_sequence - default_group = { skip_groups: [] } + default_group = { groups: [], skip_groups: [] } before_crs = %w( ResourceQuota NetworkPolicy @@ -71,10 +71,14 @@ def predeploy_sequence after_crs = %w( Pod - ).map { |r| [r, default_group] } + ) + + predeploy_hash = Hash[before_crs] + cluster_resource_discoverer.crds.select(&:predeployed?).each do |cr| + predeploy_hash[cr.kind] ||= { groups: [cr.group], skip_groups: [] } + end - crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, default_group] } - predeploy_hash = Hash[before_crs + crs + after_crs] + after_crs.each { |cr| predeploy_hash[cr] = default_group } cluster_resource_discoverer.crds.reject(&:predeployed?).each do |cr| predeploy_hash[cr.kind][:skip_groups] << cr.group if predeploy_hash[cr.kind] @@ -222,7 +226,7 @@ def ejson_provisioners def deploy_has_priority_resources?(resources) resources.any? do |r| next unless (pr = predeploy_sequence[r.type]) - !pr[:skip_groups].include?(r.group) + (pr[:groups].empty? || pr[:groups].include?(r.group)) && !pr[:skip_groups].include?(r.group) end end diff --git a/lib/krane/resource_deployer.rb b/lib/krane/resource_deployer.rb index b0cbc7782..0d9eed4c1 100644 --- a/lib/krane/resource_deployer.rb +++ b/lib/krane/resource_deployer.rb @@ -48,7 +48,9 @@ def predeploy_priority_resources(resource_list, predeploy_sequence) predeploy_sequence.each do |resource_type, attributes| matching_resources = resource_list.select do |r| - r.type == resource_type && !attributes[:skip_groups].include?(r.group) + r.type == resource_type && + ((attributes[:groups].empty? || attributes[:groups].include?(r.group)) && + !attributes[:skip_groups].include?(r.group)) end StatsD.client.gauge('priority_resources.count', matching_resources.size, tags: statsd_tags) diff --git a/test/unit/krane/resource_deployer_test.rb b/test/unit/krane/resource_deployer_test.rb index 7c15e56c7..31a040918 100644 --- a/test/unit/krane/resource_deployer_test.rb +++ b/test/unit/krane/resource_deployer_test.rb @@ -71,7 +71,7 @@ def test_predeploy_priority_resources_respects_pre_deploy_list # are deployed. See test_predeploy_priority_resources_respects_empty_pre_deploy_list # for counter example Krane::ResourceWatcher.expects(:new).returns(watcher) - priority_list = { kind => { group: "core" } } + priority_list = { kind => { groups: ["core"], skip_groups: [] } } resource_deployer.predeploy_priority_resources([resource], priority_list) end From 42301ecc96b6c680590eeb655eefe6a8f0030ca1 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Tue, 12 Jan 2021 16:35:16 -0800 Subject: [PATCH 4/6] Add a test --- test/fixtures/crd/network_policy.yml | 13 ++++++++++++ test/fixtures/crd/network_policy_cr.yml | 20 ++++++++++++++++++ test/integration-serial/serial_deploy_test.rb | 21 +++++++++++++++++++ 3 files changed, 54 insertions(+) create mode 100644 test/fixtures/crd/network_policy.yml create mode 100644 test/fixtures/crd/network_policy_cr.yml diff --git a/test/fixtures/crd/network_policy.yml b/test/fixtures/crd/network_policy.yml new file mode 100644 index 000000000..fa6751f4f --- /dev/null +++ b/test/fixtures/crd/network_policy.yml @@ -0,0 +1,13 @@ +apiVersion: apiextensions.k8s.io/v1beta1 +kind: CustomResourceDefinition +metadata: + name: networkpolicies.stable.example.io + labels: + app: krane +spec: + group: stable.example.io + version: v1 + names: + kind: NetworkPolicy + plural: networkpolicies + scope: Namespaced diff --git a/test/fixtures/crd/network_policy_cr.yml b/test/fixtures/crd/network_policy_cr.yml new file mode 100644 index 000000000..cbb8f4679 --- /dev/null +++ b/test/fixtures/crd/network_policy_cr.yml @@ -0,0 +1,20 @@ +--- +apiVersion: "stable.example.io/v1" +kind: NetworkPolicy +metadata: + name: cr-np +--- +apiVersion: networking.k8s.io/v1 +kind: NetworkPolicy +metadata: + name: built-in-policy +spec: + ingress: + - ports: + - port: 80 + protocol: TCP + podSelector: + matchLabels: + app: nginx-ingress-controller + policyTypes: + - Ingress diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb index 41a26ea5d..6130858bb 100644 --- a/test/integration-serial/serial_deploy_test.rb +++ b/test/integration-serial/serial_deploy_test.rb @@ -236,6 +236,27 @@ def test_cr_merging assert_deploy_success(result) end + def test_custom_resources_predeployed_correctly_with_non_unique_kinds + results = deploy_global_fixtures("crd", subset: %w(network_policy.yml)) do |f| + # revert name muning since we're explicitly testing conflicting names + np = f["network_policy.yml"]["CustomResourceDefinition"].first + np["metadata"]["name"] = "networkpolicies.stable.example.io" + np["spec"]["names"] = { "kind" => "NetworkPolicy", "plural" => "networkpolicies" } + end + assert_deploy_success(results) + reset_logger + + result = deploy_fixtures("crd", subset: %w(network_policy_cr.yml)) + assert_deploy_success(result) + assert_logs_match_all([ + /Phase 3: Predeploying priority resources/, + /Successfully deployed in \d.\ds: NetworkPolicy\/built-in-policy, NetworkPolicy\/cr-np/, + /Phase 4: Deploying all resources/, + /NetworkPolicy\/built-in-policy Created/, + /NetworkPolicy\/cr-np Exists/, + ], in_order: true) + end + def test_custom_resources_predeployed assert_deploy_success(deploy_global_fixtures("crd", subset: %w(mail.yml things.yml widgets.yml)) do |f| mail = f.dig("mail.yml", "CustomResourceDefinition").first From 6c00d1888820eb3ec5bb3ebcd47d474afbdce509 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Tue, 12 Jan 2021 16:35:35 -0800 Subject: [PATCH 5/6] Start of fixes for non unique kinds --- lib/krane/kubernetes_resource.rb | 14 +++++++++----- .../custom_resource_definition.rb | 5 +++++ lib/krane/resource_cache.rb | 8 +++++--- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/lib/krane/kubernetes_resource.rb b/lib/krane/kubernetes_resource.rb index a88c4f6a0..c202cd876 100644 --- a/lib/krane/kubernetes_resource.rb +++ b/lib/krane/kubernetes_resource.rb @@ -48,11 +48,11 @@ def build(namespace: nil, context:, definition:, logger:, statsd_tags:, crd: nil validate_definition_essentials(definition) opts = { namespace: namespace, context: context, definition: definition, logger: logger, statsd_tags: statsd_tags } - if (klass = class_for_kind(definition["kind"])) - return klass.new(**opts) - end - if crd + + if crd && definition['apiVersion'].start_with?("#{crd.group}/") CustomResource.new(crd: crd, **opts) + elsif (klass = class_for_kind(definition["kind"])) + klass.new(**opts) else type = definition["kind"] inst = new(**opts) @@ -164,12 +164,16 @@ def file_path end def sync(cache) - @instance_data = cache.get_instance(kubectl_resource_type, name, raise_if_not_found: true) + @instance_data = cache.get_instance(sync_group_kind, name, raise_if_not_found: true) rescue Krane::Kubectl::ResourceNotFoundError @disappeared = true if deploy_started? @instance_data = {} end + def sync_group_kind + "#{kubectl_resource_type}.#{group}" + end + def after_sync end diff --git a/lib/krane/kubernetes_resource/custom_resource_definition.rb b/lib/krane/kubernetes_resource/custom_resource_definition.rb index de0bf0c89..b2d103b79 100644 --- a/lib/krane/kubernetes_resource/custom_resource_definition.rb +++ b/lib/krane/kubernetes_resource/custom_resource_definition.rb @@ -87,6 +87,11 @@ def validate_rollout_conditions @rollout_conditions_validated = true end + def sync_group_kind + real_group = @definition.dig("apiVersion").split("/").first + "#{self.class.kind}.#{real_group}" + end + private def names_accepted_condition diff --git a/lib/krane/resource_cache.rb b/lib/krane/resource_cache.rb index baba9f8a8..2c556c209 100644 --- a/lib/krane/resource_cache.rb +++ b/lib/krane/resource_cache.rb @@ -36,9 +36,10 @@ def get_all(kind, selector = nil) [] end + # TODO: I've broken sync_dependencies def prewarm(resources) sync_dependencies = resources.flat_map { |r| r.class.const_get(:SYNC_DEPENDENCIES) } - kinds = (resources.map(&:type) + sync_dependencies).uniq + kinds = resources.map(&:sync_group_kind).uniq # sync_dependencies).uniq Krane::Concurrency.split_across_threads(kinds, max_threads: kinds.count) { |kind| get_all(kind) } end @@ -56,8 +57,9 @@ def use_or_populate_cache(kind) end def fetch_by_kind(kind) - resource_class = KubernetesResource.class_for_kind(kind) - global_kind = @task_config.global_kinds.map(&:downcase).include?(kind.downcase) + kind_only = kind.split(".").first.downcase # Busted for non unique kinds... + resource_class = KubernetesResource.class_for_kind(kind_only) + global_kind = @task_config.global_kinds.map(&:downcase).include?(kind_only) output_is_sensitive = resource_class.nil? ? false : resource_class::SENSITIVE_TEMPLATE_CONTENT raw_json, _, st = @kubectl.run("get", kind, "--chunk-size=0", attempts: 5, output: "json", output_is_sensitive: output_is_sensitive, use_namespace: !global_kind) From f1c83a781cd9713df68b1e13ec49d2cb914a2220 Mon Sep 17 00:00:00 2001 From: Daniel Turner Date: Tue, 19 Jan 2021 16:12:15 -0800 Subject: [PATCH 6/6] More fixup --- lib/krane/kubernetes_resource/daemon_set.rb | 2 +- lib/krane/kubernetes_resource/deployment.rb | 2 +- lib/krane/kubernetes_resource/replica_set.rb | 2 +- lib/krane/kubernetes_resource/service.rb | 2 +- lib/krane/kubernetes_resource/stateful_set.rb | 2 +- lib/krane/resource_cache.rb | 10 ++++++---- lib/krane/task_config.rb | 8 ++++++-- 7 files changed, 17 insertions(+), 11 deletions(-) diff --git a/lib/krane/kubernetes_resource/daemon_set.rb b/lib/krane/kubernetes_resource/daemon_set.rb index b296f633e..5cfe53665 100644 --- a/lib/krane/kubernetes_resource/daemon_set.rb +++ b/lib/krane/kubernetes_resource/daemon_set.rb @@ -3,7 +3,7 @@ module Krane class DaemonSet < PodSetBase TIMEOUT = 5.minutes - SYNC_DEPENDENCIES = %w(Pod) + SYNC_DEPENDENCIES = %w(Pod.apps) attr_reader :pods def sync(cache) diff --git a/lib/krane/kubernetes_resource/deployment.rb b/lib/krane/kubernetes_resource/deployment.rb index b7ed43b9f..0dfcf50fb 100644 --- a/lib/krane/kubernetes_resource/deployment.rb +++ b/lib/krane/kubernetes_resource/deployment.rb @@ -4,7 +4,7 @@ module Krane class Deployment < KubernetesResource TIMEOUT = 7.minutes - SYNC_DEPENDENCIES = %w(Pod ReplicaSet) + SYNC_DEPENDENCIES = %w(Pod.apps ReplicaSet.apps) REQUIRED_ROLLOUT_ANNOTATION = "required-rollout" REQUIRED_ROLLOUT_TYPES = %w(maxUnavailable full none).freeze DEFAULT_REQUIRED_ROLLOUT = 'full' diff --git a/lib/krane/kubernetes_resource/replica_set.rb b/lib/krane/kubernetes_resource/replica_set.rb index bb585bcef..204ac6dbe 100644 --- a/lib/krane/kubernetes_resource/replica_set.rb +++ b/lib/krane/kubernetes_resource/replica_set.rb @@ -4,7 +4,7 @@ module Krane class ReplicaSet < PodSetBase TIMEOUT = 5.minutes - SYNC_DEPENDENCIES = %w(Pod) + SYNC_DEPENDENCIES = %w(Pod.apps) attr_reader :pods def initialize(namespace:, context:, definition:, logger:, statsd_tags: nil, diff --git a/lib/krane/kubernetes_resource/service.rb b/lib/krane/kubernetes_resource/service.rb index 78a7f35c3..dbe84f82a 100644 --- a/lib/krane/kubernetes_resource/service.rb +++ b/lib/krane/kubernetes_resource/service.rb @@ -4,7 +4,7 @@ module Krane class Service < KubernetesResource TIMEOUT = 7.minutes - SYNC_DEPENDENCIES = %w(Pod Deployment StatefulSet) + SYNC_DEPENDENCIES = %w(Pod.apps Deployment.apps StatefulSet.apps) def sync(cache) super diff --git a/lib/krane/kubernetes_resource/stateful_set.rb b/lib/krane/kubernetes_resource/stateful_set.rb index e500a7271..ad8f2b3e6 100644 --- a/lib/krane/kubernetes_resource/stateful_set.rb +++ b/lib/krane/kubernetes_resource/stateful_set.rb @@ -4,7 +4,7 @@ module Krane class StatefulSet < PodSetBase TIMEOUT = 10.minutes ONDELETE = 'OnDelete' - SYNC_DEPENDENCIES = %w(Pod) + SYNC_DEPENDENCIES = %w(Pod.apps) attr_reader :pods def sync(cache) diff --git a/lib/krane/resource_cache.rb b/lib/krane/resource_cache.rb index 2c556c209..d4e955c89 100644 --- a/lib/krane/resource_cache.rb +++ b/lib/krane/resource_cache.rb @@ -36,10 +36,9 @@ def get_all(kind, selector = nil) [] end - # TODO: I've broken sync_dependencies def prewarm(resources) sync_dependencies = resources.flat_map { |r| r.class.const_get(:SYNC_DEPENDENCIES) } - kinds = resources.map(&:sync_group_kind).uniq # sync_dependencies).uniq + kinds = (resources.map(&:sync_group_kind) + sync_dependencies).uniq Krane::Concurrency.split_across_threads(kinds, max_threads: kinds.count) { |kind| get_all(kind) } end @@ -57,9 +56,12 @@ def use_or_populate_cache(kind) end def fetch_by_kind(kind) - kind_only = kind.split(".").first.downcase # Busted for non unique kinds... + kind_only, group = kind.downcase.split(".", 2) + global_kind = @task_config.global_resources.any? do |resource| + resource['kind'].downcase == kind_only && resource['apigroup'].downcase == group + end + resource_class = KubernetesResource.class_for_kind(kind_only) - global_kind = @task_config.global_kinds.map(&:downcase).include?(kind_only) output_is_sensitive = resource_class.nil? ? false : resource_class::SENSITIVE_TEMPLATE_CONTENT raw_json, _, st = @kubectl.run("get", kind, "--chunk-size=0", attempts: 5, output: "json", output_is_sensitive: output_is_sensitive, use_namespace: !global_kind) diff --git a/lib/krane/task_config.rb b/lib/krane/task_config.rb index 113304914..415bfa2d6 100644 --- a/lib/krane/task_config.rb +++ b/lib/krane/task_config.rb @@ -14,9 +14,13 @@ def initialize(context, namespace, logger = nil, kubeconfig = nil) end def global_kinds - @global_kinds ||= begin + global_resources.map { |g| g["kind"] } + end + + def global_resources + @global_resources ||= begin cluster_resource_discoverer = ClusterResourceDiscovery.new(task_config: self) - cluster_resource_discoverer.fetch_resources(namespaced: false).map { |g| g["kind"] } + cluster_resource_discoverer.fetch_resources(namespaced: false) end end