From e848750a75c6fb7abbdc1ddf54f10e84e1d3c4b6 Mon Sep 17 00:00:00 2001 From: Stefan Breunig Date: Wed, 15 Nov 2023 14:39:41 +0100 Subject: [PATCH] allow specifying additional/default labels via command line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change adds the ability to set additional labels or provide default values for deployment commands. This also works for ejson secrets. `ejson-keys` as shared secret will not be labled. The functionality was not made available on `krane render` due to potentially confusing behaviour around labels on secrets when using `krane render … | krane deploy -f secrets.ejson -f -` . Allowing labels specified in the templates take precedence is an intentional choice. It is the more flexible approach and allows customization for edge cases like migrations and "nested" deployments. see https://github.com/Shopify/krane/issues/682 --- lib/krane/cli/deploy_command.rb | 6 ++ lib/krane/cli/global_deploy_command.rb | 6 ++ lib/krane/deploy_task.rb | 7 +- lib/krane/ejson_secret_provisioner.rb | 4 +- lib/krane/extra_labels.rb | 21 ++++++ lib/krane/global_deploy_task.rb | 6 +- lib/krane/kubernetes_resource.rb | 16 +++-- .../kubernetes_resource/custom_resource.rb | 4 +- lib/krane/kubernetes_resource/pod.rb | 4 +- lib/krane/kubernetes_resource/replica_set.rb | 4 +- lib/krane/render_task.rb | 2 +- test/exe/deploy_test.rb | 1 + test/exe/global_deploy_test.rb | 1 + test/unit/krane/extra_labels_test.rb | 32 +++++++++ .../kubernetes_resource_test.rb | 66 +++++++++++++++++++ 15 files changed, 164 insertions(+), 16 deletions(-) create mode 100644 lib/krane/extra_labels.rb create mode 100644 test/unit/krane/extra_labels_test.rb create mode 100644 test/unit/krane/kubernetes_resource/kubernetes_resource_test.rb diff --git a/lib/krane/cli/deploy_command.rb b/lib/krane/cli/deploy_command.rb index 361d89789..bb2b10eb3 100644 --- a/lib/krane/cli/deploy_command.rb +++ b/lib/krane/cli/deploy_command.rb @@ -29,6 +29,8 @@ class DeployCommand desc: "Use --selector as a label filter to deploy only a subset "\ "of the provided resources", default: false }, + "extra-labels" => { type: :string, banner: "'label=value,foo=bar'", + desc: "Labels to set on resources that don't have them" }, "verbose-log-prefix" => { type: :boolean, desc: "Add [context][namespace] to the log prefix", default: false }, "verify-result" => { type: :boolean, default: true, @@ -39,6 +41,7 @@ def self.from_options(namespace, context, options) require 'krane/deploy_task' require 'krane/options_helper' require 'krane/label_selector' + require 'krane/extra_labels' selector = ::Krane::LabelSelector.parse(options[:selector]) if options[:selector] selector_as_filter = options['selector-as-filter'] @@ -47,6 +50,8 @@ def self.from_options(namespace, context, options) raise(Thor::RequiredArgumentMissingError, '--selector must be set when --selector-as-filter is set') end + extra_labels = ::Krane::ExtraLabels.parse(options['extra-labels']) if options['extra-labels'] + logger = ::Krane::FormattedLogger.build(namespace, context, verbose_prefix: options['verbose-log-prefix']) @@ -71,6 +76,7 @@ def self.from_options(namespace, context, options) selector: selector, selector_as_filter: selector_as_filter, protected_namespaces: protected_namespaces, + extra_labels: extra_labels, ) deploy.run!( diff --git a/lib/krane/cli/global_deploy_command.rb b/lib/krane/cli/global_deploy_command.rb index 186c1603f..7ff3cdf82 100644 --- a/lib/krane/cli/global_deploy_command.rb +++ b/lib/krane/cli/global_deploy_command.rb @@ -20,6 +20,8 @@ class GlobalDeployCommand desc: "Use --selector as a label filter to deploy only a subset "\ "of the provided resources", default: false }, + "extra-labels" => { type: :string, banner: "'label=value,foo=bar'", + desc: "Labels to set on resources that don't have them" }, "prune" => { type: :boolean, desc: "Enable deletion of resources that match"\ " the provided selector and do not appear in the provided templates", default: true }, @@ -29,6 +31,7 @@ def self.from_options(context, options) require 'krane/global_deploy_task' require 'krane/options_helper' require 'krane/label_selector' + require 'krane/extra_labels' require 'krane/duration_parser' selector = ::Krane::LabelSelector.parse(options[:selector]) @@ -38,6 +41,8 @@ def self.from_options(context, options) raise(Thor::RequiredArgumentMissingError, '--selector must be set when --selector-as-filter is set') end + extra_labels = ::Krane::ExtraLabels.parse(options['extra-labels']) if options['extra-labels'] + filenames = options[:filenames].dup filenames << "-" if options[:stdin] if filenames.empty? @@ -51,6 +56,7 @@ def self.from_options(context, options) global_timeout: ::Krane::DurationParser.new(options["global-timeout"]).parse!.to_i, selector: selector, selector_as_filter: selector_as_filter, + extra_labels: extra_labels, ) deploy.run!( diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index 33dec99af..d4044607d 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -100,12 +100,13 @@ def server_version # @param global_timeout [Integer] Timeout in seconds # @param selector [Hash] Selector(s) parsed by Krane::LabelSelector # @param selector_as_filter [Boolean] Allow selecting a subset of Kubernetes resource templates to deploy + # @param extra_labels [Hash] labels to set on resources that don't have them # @param filenames [Array] An array of filenames and/or directories containing templates (*required*) # @param protected_namespaces [Array] Array of protected Kubernetes namespaces (defaults # to Krane::DeployTask::PROTECTED_NAMESPACES) # @param render_erb [Boolean] Enable ERB rendering def initialize(namespace:, context:, current_sha: nil, logger: nil, kubectl_instance: nil, bindings: {}, - global_timeout: nil, selector: nil, selector_as_filter: false, filenames: [], protected_namespaces: nil, + global_timeout: nil, selector: nil, selector_as_filter: false, extra_labels: {}, filenames: [], protected_namespaces: nil, render_erb: false, kubeconfig: nil) @logger = logger || Krane::FormattedLogger.build(namespace, context) @template_sets = TemplateSets.from_dirs_and_files(paths: filenames, logger: @logger, render_erb: render_erb) @@ -119,6 +120,7 @@ def initialize(namespace:, context:, current_sha: nil, logger: nil, kubectl_inst @global_timeout = global_timeout @selector = selector @selector_as_filter = selector_as_filter + @extra_labels = extra_labels @protected_namespaces = protected_namespaces || PROTECTED_NAMESPACES @render_erb = render_erb end @@ -211,6 +213,7 @@ def ejson_provisioners ejson_file: ejson_secret_file, statsd_tags: @namespace_tags, selector: @selector, + extra_labels: @extra_labels, ) end end @@ -241,7 +244,7 @@ def discover_resources @template_sets.with_resource_definitions(current_sha: @current_sha, bindings: @bindings) do |r_def| crd = crds_by_kind[r_def["kind"]]&.first r = KubernetesResource.build(namespace: @namespace, context: @context, logger: @logger, definition: r_def, - statsd_tags: @namespace_tags, crd: crd, global_names: @task_config.global_kinds) + statsd_tags: @namespace_tags, crd: crd, global_names: @task_config.global_kinds, extra_labels: @extra_labels) resources << r @logger.info(" - #{r.id}") end diff --git a/lib/krane/ejson_secret_provisioner.rb b/lib/krane/ejson_secret_provisioner.rb index e6bd09154..80afe355a 100644 --- a/lib/krane/ejson_secret_provisioner.rb +++ b/lib/krane/ejson_secret_provisioner.rb @@ -18,11 +18,12 @@ class EjsonSecretProvisioner delegate :namespace, :context, :logger, to: :@task_config - def initialize(task_config:, ejson_keys_secret:, ejson_file:, statsd_tags:, selector: nil) + def initialize(task_config:, ejson_keys_secret:, ejson_file:, statsd_tags:, selector: nil, extra_labels: {}) @ejson_keys_secret = ejson_keys_secret @ejson_file = ejson_file @statsd_tags = statsd_tags @selector = selector + @extra_labels = extra_labels @task_config = task_config @kubectl = Kubectl.new( task_config: @task_config, @@ -97,6 +98,7 @@ def generate_secret_resource(secret_name, secret_type, data) labels = { "name" => secret_name } labels.reverse_merge!(@selector.to_h) if @selector + labels.reverse_merge!(@extra_labels) secret = { 'kind' => 'Secret', diff --git a/lib/krane/extra_labels.rb b/lib/krane/extra_labels.rb new file mode 100644 index 000000000..3764e68d7 --- /dev/null +++ b/lib/krane/extra_labels.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Krane + class ExtraLabels + def self.parse(string) + extra_labels = {} + + string.split(',').each do |kvp| + key, value = kvp.split('=', 2) + + if key.blank? + raise ArgumentError, "key is blank" + end + + extra_labels[key] = value + end + + extra_labels + end + end +end diff --git a/lib/krane/global_deploy_task.rb b/lib/krane/global_deploy_task.rb index 559872424..5400b033c 100644 --- a/lib/krane/global_deploy_task.rb +++ b/lib/krane/global_deploy_task.rb @@ -34,9 +34,10 @@ class GlobalDeployTask # @param global_timeout [Integer] Timeout in seconds # @param selector [Hash] Selector(s) parsed by Krane::LabelSelector (*required*) # @param selector_as_filter [Boolean] Allow selecting a subset of Kubernetes resource templates to deploy + # @param extra_labels [Hash] labels to set on resources that don't have them # @param filenames [Array] An array of filenames and/or directories containing templates (*required*) def initialize(context:, global_timeout: nil, selector: nil, selector_as_filter: false, - filenames: [], logger: nil, kubeconfig: nil) + extra_labels: {}, filenames: [], logger: nil, kubeconfig: nil) template_paths = filenames.map { |path| File.expand_path(path) } @task_config = TaskConfig.new(context, nil, logger, kubeconfig) @@ -45,6 +46,7 @@ def initialize(context:, global_timeout: nil, selector: nil, selector_as_filter: @global_timeout = global_timeout @selector = selector @selector_as_filter = selector_as_filter + @extra_labels = extra_labels end # Runs the task, returning a boolean representing success or failure @@ -168,7 +170,7 @@ def discover_resources @template_sets.with_resource_definitions do |r_def| crd = crds_by_kind[r_def["kind"]]&.first r = KubernetesResource.build(context: context, logger: logger, definition: r_def, - crd: crd, global_names: global_kinds, statsd_tags: statsd_tags) + crd: crd, global_names: global_kinds, statsd_tags: statsd_tags, extra_labels: @extra_labels) resources << r logger.info(" - #{r.id}") end diff --git a/lib/krane/kubernetes_resource.rb b/lib/krane/kubernetes_resource.rb index 2636ae9b0..3ddf7b327 100644 --- a/lib/krane/kubernetes_resource.rb +++ b/lib/krane/kubernetes_resource.rb @@ -44,10 +44,10 @@ class KubernetesResource SYNC_DEPENDENCIES = [] class << self - def build(namespace: nil, context:, definition:, logger:, statsd_tags:, crd: nil, global_names: []) + def build(namespace: nil, context:, definition:, logger:, statsd_tags:, crd: nil, global_names: [], extra_labels: {}) validate_definition_essentials(definition) opts = { namespace: namespace, context: context, definition: definition, logger: logger, - statsd_tags: statsd_tags } + statsd_tags: statsd_tags, extra_labels: extra_labels } if (klass = class_for_kind(definition["kind"])) return klass.new(**opts) end @@ -115,14 +115,14 @@ def pretty_timeout_type "timeout: #{timeout}s" end - def initialize(namespace:, context:, definition:, logger:, statsd_tags: []) + def initialize(namespace:, context:, definition:, logger:, statsd_tags: [], extra_labels: {}) # subclasses must also set these if they define their own initializer @name = (definition.dig("metadata", "name") || definition.dig("metadata", "generateName")).to_s @optional_statsd_tags = statsd_tags @namespace = namespace @context = context @logger = logger - @definition = definition + @definition = set_extra_labels(definition, extra_labels) @statsd_report_done = false @disappeared = false @validation_errors = [] @@ -529,6 +529,14 @@ def krane_annotation_value(suffix) @definition.dig("metadata", "annotations", Annotation.for(suffix)) end + def set_extra_labels(definition, extra_labels) + return definition if extra_labels.nil? || extra_labels.empty? + definition["metadata"] ||= {} + definition["metadata"]["labels"] ||= {} + definition["metadata"]["labels"].reverse_merge!(extra_labels) + definition + end + def validate_selector(selector) if labels.nil? @validation_errors << "selector #{selector} passed in, but no labels were defined" diff --git a/lib/krane/kubernetes_resource/custom_resource.rb b/lib/krane/kubernetes_resource/custom_resource.rb index 453ce3f8d..f1e911c79 100644 --- a/lib/krane/kubernetes_resource/custom_resource.rb +++ b/lib/krane/kubernetes_resource/custom_resource.rb @@ -8,9 +8,9 @@ class CustomResource < KubernetesResource (.metadata.generation != .status.observedGeneration). MSG - def initialize(namespace:, context:, definition:, logger:, statsd_tags: [], crd:) + def initialize(namespace:, context:, definition:, logger:, statsd_tags: [], crd:, extra_labels: {}) super(namespace: namespace, context: context, definition: definition, - logger: logger, statsd_tags: statsd_tags) + logger: logger, statsd_tags: statsd_tags, extra_labels: extra_labels) @crd = crd end diff --git a/lib/krane/kubernetes_resource/pod.rb b/lib/krane/kubernetes_resource/pod.rb index 64a93f80a..f55d50645 100644 --- a/lib/krane/kubernetes_resource/pod.rb +++ b/lib/krane/kubernetes_resource/pod.rb @@ -13,7 +13,7 @@ class Pod < KubernetesResource attr_reader :definition def initialize(namespace:, context:, definition:, logger:, - statsd_tags: nil, parent: nil, deploy_started_at: nil, stream_logs: false) + statsd_tags: nil, parent: nil, deploy_started_at: nil, stream_logs: false, extra_labels: {}) @parent = parent @deploy_started_at = deploy_started_at @@ -25,7 +25,7 @@ def initialize(namespace:, context:, definition:, logger:, @containers += definition["spec"].fetch("initContainers", []).map { |c| Container.new(c, init_container: true) } @stream_logs = stream_logs super(namespace: namespace, context: context, definition: definition, - logger: logger, statsd_tags: statsd_tags) + logger: logger, statsd_tags: statsd_tags, extra_labels: extra_labels) end def sync(_cache) diff --git a/lib/krane/kubernetes_resource/replica_set.rb b/lib/krane/kubernetes_resource/replica_set.rb index bb585bcef..4caeced02 100644 --- a/lib/krane/kubernetes_resource/replica_set.rb +++ b/lib/krane/kubernetes_resource/replica_set.rb @@ -8,12 +8,12 @@ class ReplicaSet < PodSetBase attr_reader :pods def initialize(namespace:, context:, definition:, logger:, statsd_tags: nil, - parent: nil, deploy_started_at: nil) + parent: nil, deploy_started_at: nil, extra_labels: {}) @parent = parent @deploy_started_at = deploy_started_at @pods = [] super(namespace: namespace, context: context, definition: definition, - logger: logger, statsd_tags: statsd_tags) + logger: logger, statsd_tags: statsd_tags, extra_labels: extra_labels) end def sync(cache) diff --git a/lib/krane/render_task.rb b/lib/krane/render_task.rb index 26fb952ea..39865215f 100644 --- a/lib/krane/render_task.rb +++ b/lib/krane/render_task.rb @@ -14,7 +14,7 @@ class RenderTask # @param current_sha [String] The SHA of the commit # @param filenames [Array] An array of filenames and/or directories containing templates (*required*) # @param bindings [Hash] Bindings parsed by Krane::BindingsParser - def initialize(logger: nil, current_sha:, filenames: [], bindings:) + def initialize(logger: nil, current_sha:, filenames: [], bindings:, extra_labels: {}) @logger = logger || Krane::FormattedLogger.build @filenames = filenames.map { |path| File.expand_path(path) } @bindings = bindings diff --git a/test/exe/deploy_test.rb b/test/exe/deploy_test.rb index ce52fb4a5..02bea62ca 100644 --- a/test/exe/deploy_test.rb +++ b/test/exe/deploy_test.rb @@ -164,6 +164,7 @@ def default_options(new_args = {}, run_args = {}) selector: nil, selector_as_filter: false, protected_namespaces: ["default", "kube-system", "kube-public"], + extra_labels: nil, }.merge(new_args), run_args: { verify_result: true, diff --git a/test/exe/global_deploy_test.rb b/test/exe/global_deploy_test.rb index 9269b67c0..943e5b437 100644 --- a/test/exe/global_deploy_test.rb +++ b/test/exe/global_deploy_test.rb @@ -143,6 +143,7 @@ def default_options(new_args = {}, run_args = {}) global_timeout: 300, selector: 'name=web', selector_as_filter: false, + extra_labels: nil, }.merge(new_args), run_args: { verify_result: true, diff --git a/test/unit/krane/extra_labels_test.rb b/test/unit/krane/extra_labels_test.rb new file mode 100644 index 000000000..4051ba86d --- /dev/null +++ b/test/unit/krane/extra_labels_test.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true +require 'test_helper' +require 'krane/extra_labels' + +class ExtraLabelsTest < ::Minitest::Test + def test_parse_extra_labels + expected = { "foo" => "42", "bar" => "17" } + assert_equal(expected, parse("foo=42,bar=17")) + end + + def test_parse_extra_labels_with_equality_sign + expected = { "foo" => "1=2=3", "bar" => "3", "bla" => "4=7" } + assert_equal(expected, parse("foo=1=2=3,bar=3,bla=4=7")) + end + + def test_parse_extra_labels_with_no_value + expected = { "bla" => nil, "foo" => "" } + assert_equal(expected, parse("bla,foo=")) + end + + def test_parse_extra_labels_with_no_key + assert_raises(ArgumentError, "key is blank") do + parse("=17,foo=42") + end + end + + private + + def parse(string) + Krane::ExtraLabels.parse(string).to_h + end +end diff --git a/test/unit/krane/kubernetes_resource/kubernetes_resource_test.rb b/test/unit/krane/kubernetes_resource/kubernetes_resource_test.rb new file mode 100644 index 000000000..0ceae6287 --- /dev/null +++ b/test/unit/krane/kubernetes_resource/kubernetes_resource_test.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true +require 'test_helper' + +class KubernetesResourceTest < Krane::TestCase + def test_extra_labels + [ + {kind: "ConfigMap"}, + {kind: "CronJob"}, + {kind: "CustomResourceDefinition"}, + {kind: "DaemonSet"}, + {kind: "Deployment"}, + {kind: "HorizontalPodAutoscaler"}, + {kind: "Ingress"}, + {kind: "Job"}, + {kind: "NetworkPolicy"}, + {kind: "PersistentVolumeClaim"}, + {kind: "PodDisruptionBudget"}, + {kind: "PodSetBase"}, + {kind: "PodTemplate"}, + {kind: "ReplicaSet"}, + {kind: "ResourceQuota"}, + {kind: "Role"}, + {kind: "RoleBinding"}, + {kind: "Secret"}, + {kind: "Service"}, + {kind: "ServiceAccount"}, + {kind: "StatefulSet"}, + + {kind: "Pod", spec: {"containers" => [{"name" => "someContainer"}]}}, + {kind: "SomeCustomResource", init_args: {crd: "SomeCRD"}}, + {kind: "ResourceUnknownToKrane"}, + ].each do |resource| + args = { + namespace: 'test', + context: 'nope', + logger: @logger, + statsd_tags: [], + extra_labels: { + "extra" => "label", + "overwritten" => "yes" + }, + definition: { + "kind" => resource.fetch(:kind), + "metadata" => { + "name" => "testsuite", + "labels" => { + "overwritten" => "no" + }, + }, + "spec" => resource.fetch(:spec, {}) + } + } + args.merge!(resource.fetch(:init_args, {})) + + resource = begin + Krane::KubernetesResource.build(**args) + rescue ArgumentError => e + flunk("failed to build #{resource.fetch(:kind)}: #{e.message}") + end + + assert_equal(resource.send(:labels), + {"extra"=>"label", "overwritten"=>"no"}, + "expected #{resource} to apply extra_labels") + end + end +end