From 5be0dbcd906352fdcac774a60954ed6cec7d8051 Mon Sep 17 00:00:00 2001 From: Kevin Deisz Date: Mon, 3 Aug 2020 15:24:59 -0400 Subject: [PATCH 1/3] Remove support for kubernetes-deploy.shopify.io annotations --- lib/krane/annotation.rb | 11 ++ lib/krane/concerns/template_reporting.rb | 6 - lib/krane/deploy_task.rb | 6 +- lib/krane/global_deploy_task.rb | 5 - lib/krane/kubernetes_resource.rb | 45 +----- .../kubernetes_resource/custom_resource.rb | 2 +- .../custom_resource_definition.rb | 17 +-- lib/krane/kubernetes_resource/deployment.rb | 8 +- test/fixtures/crd/widgets_deprecated.yml | 15 -- .../with_default_conditions_deprecated.yml | 17 --- test/integration-serial/serial_deploy_test.rb | 94 +----------- test/integration/krane_deploy_test.rb | 24 +-- .../custom_resource_definition_test.rb | 22 ++- .../kubernetes_resource/deployment_test.rb | 91 +++-------- test/unit/krane/kubernetes_resource_test.rb | 144 ++---------------- 15 files changed, 85 insertions(+), 422 deletions(-) create mode 100644 lib/krane/annotation.rb delete mode 100644 test/fixtures/crd/widgets_deprecated.yml delete mode 100644 test/fixtures/crd/with_default_conditions_deprecated.yml diff --git a/lib/krane/annotation.rb b/lib/krane/annotation.rb new file mode 100644 index 000000000..aeee787a4 --- /dev/null +++ b/lib/krane/annotation.rb @@ -0,0 +1,11 @@ +# frozen_string_literal: true + +module Krane + module Annotation + class << self + def for(suffix) + "krane.shopify.io/#{suffix}" + end + end + end +end diff --git a/lib/krane/concerns/template_reporting.rb b/lib/krane/concerns/template_reporting.rb index 76b40fda7..e654402c3 100644 --- a/lib/krane/concerns/template_reporting.rb +++ b/lib/krane/concerns/template_reporting.rb @@ -15,12 +15,6 @@ def record_invalid_template(logger:, err:, filename:, content: nil) logger.summary.add_paragraph(debug_msg) end - def record_warnings(logger:, warning:, filename:) - warn_msg = "Template warning: #{filename}\n" - warn_msg += "> Warning message:\n#{Krane::FormattedLogger.indent_four(warning)}" - logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow) - end - def add_para_from_list(logger:, action:, enum:) logger.summary.add_action(action) logger.summary.add_paragraph(enum.map { |e| "- #{e}" }.join("\n")) diff --git a/lib/krane/deploy_task.rb b/lib/krane/deploy_task.rb index 0a9089672..7142d0217 100644 --- a/lib/krane/deploy_task.rb +++ b/lib/krane/deploy_task.rb @@ -4,6 +4,7 @@ require 'tempfile' require 'fileutils' +require 'krane/annotation' require 'krane/common' require 'krane/concurrency' require 'krane/resource_cache' @@ -282,11 +283,6 @@ def validate_resources(resources) r.validate_definition(kubectl, selector: @selector) end - resources.select(&:has_warnings?).each do |resource| - record_warnings(logger: @logger, warning: resource.validation_warning_msg, - filename: File.basename(resource.file_path)) - end - failed_resources = resources.select(&:validation_failed?) if failed_resources.present? diff --git a/lib/krane/global_deploy_task.rb b/lib/krane/global_deploy_task.rb index 364ed37e4..db2fee29e 100644 --- a/lib/krane/global_deploy_task.rb +++ b/lib/krane/global_deploy_task.rb @@ -133,11 +133,6 @@ def validate_resources(resources) r.validate_definition(@kubectl, selector: @selector) end - resources.select(&:has_warnings?).each do |resource| - record_warnings(logger: logger, warning: resource.validation_warning_msg, - filename: File.basename(resource.file_path)) - end - failed_resources = resources.select(&:validation_failed?) if failed_resources.present? failed_resources.each do |r| diff --git a/lib/krane/kubernetes_resource.rb b/lib/krane/kubernetes_resource.rb index 7d8ec5c40..68fd56f4a 100644 --- a/lib/krane/kubernetes_resource.rb +++ b/lib/krane/kubernetes_resource.rb @@ -32,9 +32,7 @@ class KubernetesResource If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout. MSG - TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX = "timeout-override" - TIMEOUT_OVERRIDE_ANNOTATION_DEPRECATED = "kubernetes-deploy.shopify.io/#{TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX}" - TIMEOUT_OVERRIDE_ANNOTATION = "krane.shopify.io/#{TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX}" + TIMEOUT_OVERRIDE_ANNOTATION = "timeout-override" LAST_APPLIED_ANNOTATION = "kubectl.kubernetes.io/last-applied-configuration" SENSITIVE_TEMPLATE_CONTENT = false SERVER_DRY_RUNNABLE = false @@ -103,7 +101,7 @@ def timeout def timeout_override return @timeout_override if defined?(@timeout_override) - @timeout_override = DurationParser.new(krane_annotation_value(TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX)).parse!.to_i + @timeout_override = DurationParser.new(krane_annotation_value(TIMEOUT_OVERRIDE_ANNOTATION)).parse!.to_i rescue DurationParser::ParsingError @timeout_override = nil end @@ -123,7 +121,6 @@ def initialize(namespace:, context:, definition:, logger:, statsd_tags: []) @statsd_report_done = false @disappeared = false @validation_errors = [] - @validation_warnings = [] @instance_data = {} @server_dry_run_validated = false end @@ -134,22 +131,12 @@ def to_kubeclient_resource def validate_definition(kubectl, selector: nil) @validation_errors = [] - @validation_warnings = [] validate_selector(selector) if selector validate_timeout_annotation - validate_annotation_version validate_spec_with_kubectl(kubectl) @validation_errors.present? end - def validation_warning_msg - @validation_warnings.join("\n") - end - - def has_warnings? - @validation_warnings.present? - end - def validation_error_msg @validation_errors.join("\n") end @@ -500,8 +487,8 @@ def global? private def validate_timeout_annotation - timeout_override_value = krane_annotation_value(TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX) - timeout_annotation_key = krane_annotation_key(TIMEOUT_OVERRIDE_ANNOTATION_SUFFIX) + timeout_override_value = krane_annotation_value(TIMEOUT_OVERRIDE_ANNOTATION) + timeout_annotation_key = Annotation.for(TIMEOUT_OVERRIDE_ANNOTATION) return if timeout_override_value.nil? override = DurationParser.new(timeout_override_value).parse! @@ -514,30 +501,8 @@ def validate_timeout_annotation @validation_errors << "#{timeout_annotation_key} annotation is invalid: #{e}" end - def validate_annotation_version - return if validation_warning_msg.include?("annotations is deprecated") - annotation_keys = @definition.dig("metadata", "annotations")&.keys - annotation_keys&.each do |annotation| - if annotation.include?("kubernetes-deploy.shopify.io") - annotation_prefix = annotation.split('/').first - @validation_warnings << "#{annotation_prefix} as a prefix for annotations is deprecated: "\ - "Use the 'krane.shopify.io' annotation prefix instead" - return - end - end - end - def krane_annotation_value(suffix) - @definition.dig("metadata", "annotations", "kubernetes-deploy.shopify.io/#{suffix}") || - @definition.dig("metadata", "annotations", "krane.shopify.io/#{suffix}") - end - - def krane_annotation_key(suffix) - if @definition.dig("metadata", "annotations", "kubernetes-deploy.shopify.io/#{suffix}") - "kubernetes-deploy.shopify.io/#{suffix}" - elsif @definition.dig("metadata", "annotations", "krane.shopify.io/#{suffix}") - "krane.shopify.io/#{suffix}" - end + @definition.dig("metadata", "annotations", Annotation.for(suffix)) end def validate_selector(selector) diff --git a/lib/krane/kubernetes_resource/custom_resource.rb b/lib/krane/kubernetes_resource/custom_resource.rb index e23611883..453ce3f8d 100644 --- a/lib/krane/kubernetes_resource/custom_resource.rb +++ b/lib/krane/kubernetes_resource/custom_resource.rb @@ -70,7 +70,7 @@ def validate_definition(*, **) @validation_errors << "The CRD that specifies this resource is using invalid rollout conditions. " \ "Krane will not be able to continue until those rollout conditions are fixed.\n" \ "Rollout conditions can be found on the CRD that defines this resource (#{@crd.name}), " \ - "under the annotation #{CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION}.\n" \ + "under the annotation #{Annotation.for(CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION)}.\n" \ "Validation failed with: #{e}" end diff --git a/lib/krane/kubernetes_resource/custom_resource_definition.rb b/lib/krane/kubernetes_resource/custom_resource_definition.rb index da6a8fb2a..de0bf0c89 100644 --- a/lib/krane/kubernetes_resource/custom_resource_definition.rb +++ b/lib/krane/kubernetes_resource/custom_resource_definition.rb @@ -2,9 +2,8 @@ module Krane class CustomResourceDefinition < KubernetesResource TIMEOUT = 2.minutes - ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX = "instance-rollout-conditions" - ROLLOUT_CONDITIONS_ANNOTATION = "krane.shopify.io/#{ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX}" - TIMEOUT_FOR_INSTANCE_ANNOTATION = "krane.shopify.io/instance-timeout" + ROLLOUT_CONDITIONS_ANNOTATION = "instance-rollout-conditions" + TIMEOUT_FOR_INSTANCE_ANNOTATION = "instance-timeout" GLOBAL = true def deploy_succeeded? @@ -20,7 +19,7 @@ def timeout_message end def timeout_for_instance - timeout = krane_annotation_value("instance-timeout") + timeout = krane_annotation_value(TIMEOUT_FOR_INSTANCE_ANNOTATION) DurationParser.new(timeout).parse!.to_i rescue DurationParser::ParsingError nil @@ -63,8 +62,8 @@ def predeployed? def rollout_conditions return @rollout_conditions if defined?(@rollout_conditions) - @rollout_conditions = if krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX) - RolloutConditions.from_annotation(krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX)) + @rollout_conditions = if krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION) + RolloutConditions.from_annotation(krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION)) end rescue RolloutConditionsError @rollout_conditions = nil @@ -75,13 +74,13 @@ def validate_definition(*, **) validate_rollout_conditions rescue RolloutConditionsError => e - @validation_errors << "Annotation #{krane_annotation_key(ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX)} "\ + @validation_errors << "Annotation #{Annotation.for(ROLLOUT_CONDITIONS_ANNOTATION)} " \ "on #{name} is invalid: #{e}" end def validate_rollout_conditions - if krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX) && @rollout_conditions_validated.nil? - conditions = RolloutConditions.from_annotation(krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION_SUFFIX)) + if krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION) && @rollout_conditions_validated.nil? + conditions = RolloutConditions.from_annotation(krane_annotation_value(ROLLOUT_CONDITIONS_ANNOTATION)) conditions.validate! end diff --git a/lib/krane/kubernetes_resource/deployment.rb b/lib/krane/kubernetes_resource/deployment.rb index 71d7ccb80..b7ed43b9f 100644 --- a/lib/krane/kubernetes_resource/deployment.rb +++ b/lib/krane/kubernetes_resource/deployment.rb @@ -5,9 +5,7 @@ module Krane class Deployment < KubernetesResource TIMEOUT = 7.minutes SYNC_DEPENDENCIES = %w(Pod ReplicaSet) - REQUIRED_ROLLOUT_ANNOTATION_SUFFIX = "required-rollout" - REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED = "kubernetes-deploy.shopify.io/#{REQUIRED_ROLLOUT_ANNOTATION_SUFFIX}" - REQUIRED_ROLLOUT_ANNOTATION = "krane.shopify.io/#{REQUIRED_ROLLOUT_ANNOTATION_SUFFIX}" + REQUIRED_ROLLOUT_ANNOTATION = "required-rollout" REQUIRED_ROLLOUT_TYPES = %w(maxUnavailable full none).freeze DEFAULT_REQUIRED_ROLLOUT = 'full' @@ -106,7 +104,7 @@ def validate_definition(*, **) strategy = @definition.dig('spec', 'strategy', 'type').to_s if required_rollout.downcase == 'maxunavailable' && strategy.present? && strategy.downcase != 'rollingupdate' - @validation_errors << "'#{krane_annotation_key(REQUIRED_ROLLOUT_ANNOTATION_SUFFIX)}: #{required_rollout}' "\ + @validation_errors << "'#{Annotation.for(REQUIRED_ROLLOUT_ANNOTATION)}: #{required_rollout}' " \ "is incompatible with strategy '#{strategy}'" end @@ -151,7 +149,7 @@ def progress_deadline end def rollout_annotation_err_msg - "'#{krane_annotation_key(REQUIRED_ROLLOUT_ANNOTATION_SUFFIX)}: #{required_rollout}' is invalid. "\ + "'#{Annotation.for(REQUIRED_ROLLOUT_ANNOTATION)}: #{required_rollout}' is invalid. " \ "Acceptable values: #{REQUIRED_ROLLOUT_TYPES.join(', ')}" end diff --git a/test/fixtures/crd/widgets_deprecated.yml b/test/fixtures/crd/widgets_deprecated.yml deleted file mode 100644 index 9cea3b8f1..000000000 --- a/test/fixtures/crd/widgets_deprecated.yml +++ /dev/null @@ -1,15 +0,0 @@ -apiVersion: apiextensions.k8s.io/v1beta1 -kind: CustomResourceDefinition -metadata: - name: widgets.stable.example.io - labels: - app: krane - annotations: - kubernetes-deploy.shopify.io/prunable: "true" -spec: - group: stable.example.io - version: v1 - names: - kind: Widget - plural: widgets - scope: Namespaced diff --git a/test/fixtures/crd/with_default_conditions_deprecated.yml b/test/fixtures/crd/with_default_conditions_deprecated.yml deleted file mode 100644 index dc49f2d0e..000000000 --- a/test/fixtures/crd/with_default_conditions_deprecated.yml +++ /dev/null @@ -1,17 +0,0 @@ -apiVersion: apiextensions.k8s.io/v1beta1 -kind: CustomResourceDefinition -metadata: - name: parameterizeds.stable.example.io - labels: - app: krane - annotations: - kubernetes-deploy.shopify.io/instance-rollout-conditions: "true" -spec: - group: stable.example.io - names: - kind: Parameterized - listKind: ParameterizedList - plural: parameterizeds - singular: parameterized - scope: Namespaced - version: v1 diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb index 69d22cae5..7f2c6fb4c 100644 --- a/test/integration-serial/serial_deploy_test.rb +++ b/test/integration-serial/serial_deploy_test.rb @@ -236,48 +236,6 @@ def test_cr_merging assert_deploy_success(result) end - def test_custom_resources_predeployed_deprecated - assert_deploy_success(deploy_global_fixtures("crd", - subset: %w(mail.yml things.yml widgets_deprecated.yml)) do |f| - mail = f.dig("mail.yml", "CustomResourceDefinition").first - mail["metadata"]["annotations"] = {} - - things = f.dig("things.yml", "CustomResourceDefinition").first - things["metadata"]["annotations"] = { - "kubernetes-deploy.shopify.io/predeployed" => "true", - } - - widgets = f.dig("widgets_deprecated.yml", "CustomResourceDefinition").first - widgets["metadata"]["annotations"] = { - "kubernetes-deploy.shopify.io/predeployed" => "false", - } - end) - reset_logger - - result = deploy_fixtures("crd", subset: %w(mail_cr.yml things_cr.yml widgets_cr.yml)) do |f| - f.each do |_filename, contents| - contents.each do |_kind, crs| # all of the resources are CRs, so change all of them - crs.each { |cr| cr["kind"] = add_unique_prefix_for_test(cr["kind"]) } - end - end - end - assert_deploy_success(result) - - mail_cr_id = "#{add_unique_prefix_for_test('Mail')}/my-first-mail" - thing_cr_id = "#{add_unique_prefix_for_test('Thing')}/my-first-thing" - widget_cr_id = "#{add_unique_prefix_for_test('Widget')}/my-first-widget" - assert_logs_match_all([ - /Phase 3: Predeploying priority resources/, - /Successfully deployed in \d.\ds: #{mail_cr_id}/, - /Successfully deployed in \d.\ds: #{thing_cr_id}/, - /Phase 4: Deploying all resources/, - /Successfully deployed in \d.\ds: #{mail_cr_id}, #{thing_cr_id}, #{widget_cr_id}/, - ], in_order: true) - refute_logs_match( - /Successfully deployed in \d.\ds: #{widget_cr_id}/, - ) - 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 @@ -319,22 +277,6 @@ def test_custom_resources_predeployed ) end - def test_cr_deploys_without_rollout_conditions_when_none_present_deprecated - assert_deploy_success(deploy_global_fixtures("crd", subset: %(widgets_deprecated.yml))) - result = deploy_fixtures("crd", subset: %w(widgets_cr.yml)) do |fixtures| - cr = fixtures["widgets_cr.yml"]["Widget"].first - cr["kind"] = add_unique_prefix_for_test(cr["kind"]) - end - assert_deploy_success(result) - - prefixed_kind = add_unique_prefix_for_test("Widget") - assert_logs_match_all([ - "Don't know how to monitor resources of type #{prefixed_kind}.", - "Assuming #{prefixed_kind}/my-first-widget deployed successfully.", - %r{#{prefixed_kind}/my-first-widget\s+Exists}, - ]) - end - def test_cr_deploys_without_rollout_conditions_when_none_present assert_deploy_success(deploy_global_fixtures("crd", subset: %(widgets.yml))) result = deploy_fixtures("crd", subset: %w(widgets_cr.yml)) do |f| @@ -395,34 +337,6 @@ def test_cr_success_with_service build_kubectl.run("delete", "-f", filepath, use_namespace: false, log_failure: false) end - def test_cr_success_with_default_rollout_conditions_deprecated_annotation - assert_deploy_success(deploy_global_fixtures("crd", subset: %(with_default_conditions_deprecated.yml))) - success_conditions = { - "status" => { - "observedGeneration" => 1, - "conditions" => [ - { - "type" => "Ready", - "reason" => "test", - "message" => "test", - "status" => "True", - }, - ], - }, - } - - result = deploy_fixtures("crd", subset: ["with_default_conditions_cr.yml"]) do |resource| - cr = resource["with_default_conditions_cr.yml"]["Parameterized"].first - cr.merge!(success_conditions) - cr["kind"] = add_unique_prefix_for_test(cr["kind"]) - end - assert_deploy_success(result) - assert_logs_match_all([ - %r{Successfully deployed in .*: #{add_unique_prefix_for_test("Parameterized")}\/with-default-params}, - %r{Parameterized/with-default-params\s+Healthy}, - ]) - end - def test_cr_failure_with_default_rollout_conditions assert_deploy_success(deploy_global_fixtures("crd", subset: %(with_default_conditions.yml))) failure_conditions = { @@ -506,7 +420,7 @@ def test_deploying_crs_with_invalid_crd_conditions_fails # deployed, we need to model the case where poorly configured rollout_conditions are present before deploying a CR fixtures = load_fixtures("crd", "with_custom_conditions.yml") crd = fixtures["with_custom_conditions.yml"]["CustomResourceDefinition"].first - crd["metadata"]["annotations"].merge!(Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION => "blah") + crd["metadata"]["annotations"].merge!(rollout_conditions_annotation_key => "blah") apply_scope_to_resources(fixtures, labels: "app=krane,test=#{@namespace}") Tempfile.open([@namespace, ".yml"]) do |f| f.write(YAML.dump(crd)) @@ -599,4 +513,10 @@ def test_resource_discovery_stops_deploys_when_fetch_crds_kubectl_errs failure_msg, ], in_order: true) end + + private + + def rollout_conditions_annotation_key + Krane::Annotation.for(Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION) + end end diff --git a/test/integration/krane_deploy_test.rb b/test/integration/krane_deploy_test.rb index 1593d30ff..de23fc195 100644 --- a/test/integration/krane_deploy_test.rb +++ b/test/integration/krane_deploy_test.rb @@ -507,28 +507,12 @@ def test_deployment_with_progress_times_out_for_short_duration ]) end - def test_deployment_with_timeout_override_deprecated - result = deploy_fixtures("long-running", subset: ['undying-deployment.yml']) do |fixtures| - deployment = fixtures['undying-deployment.yml']['Deployment'].first - deployment['spec']['progressDeadlineSeconds'] = 5 - deployment["metadata"]["annotations"] = { - Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION_DEPRECATED => "10S", - } - container = deployment['spec']['template']['spec']['containers'].first - container['readinessProbe'] = { "exec" => { "command" => ['- ls'] } } - end - assert_deploy_failure(result, :timed_out) - assert_logs_match_all(Krane::KubernetesResource::STANDARD_TIMEOUT_MESSAGE.split("\n") + - ["timeout override: 10s"]) - end - def test_deployment_with_timeout_override result = deploy_fixtures("long-running", subset: ['undying-deployment.yml']) do |fixtures| deployment = fixtures['undying-deployment.yml']['Deployment'].first deployment['spec']['progressDeadlineSeconds'] = 5 - deployment["metadata"]["annotations"] = { - Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION => "10S", - } + deployment["metadata"]["annotations"] = { timeout_override_annotation_key => "10S" } + container = deployment['spec']['template']['spec']['containers'].first container['readinessProbe'] = { "exec" => { "command" => ['- ls'] } } end @@ -1780,4 +1764,8 @@ def expected_daemonset_pod_count !node.metadata.labels.to_h.keys.include?(:"node-role.kubernetes.io/master") end end + + def timeout_override_annotation_key + Krane::Annotation.for(Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION) + end end diff --git a/test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb b/test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb index 490fcef06..96080c45f 100644 --- a/test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb +++ b/test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb @@ -54,7 +54,7 @@ def test_rollout_conditions_invalid_when_path_or_value_missing assert(crd.validation_failed?, "Missing path/value keys should fail validation") assert_equal(crd.validation_error_msg, - "Annotation #{Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION} " \ + "Annotation #{rollout_conditions_annotation_key} " \ "on #{crd.name} is invalid: Missing required key(s) for success_condition: [:value], " \ "Missing required key(s) for failure_condition: [:path]") end @@ -67,7 +67,7 @@ def test_rollout_conditions_fails_validation_when_missing_condition_keys assert(crd.validation_failed?, "success_conditions requires at least one entry") assert_equal(crd.validation_error_msg, - "Annotation #{Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION} " \ + "Annotation #{rollout_conditions_annotation_key} " \ "on #{crd.name} is invalid: success_conditions must contain at least one entry") end @@ -76,7 +76,7 @@ def test_rollout_conditions_fails_validation_with_invalid_json crd.validate_definition(kubectl) assert(crd.validation_failed?, "Invalid rollout conditions were accepted") assert(crd.validation_error_msg.match( - "Annotation #{Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION} " \ + "Annotation #{rollout_conditions_annotation_key} " \ "on #{crd.name} is invalid: Rollout conditions are not valid JSON:" )) end @@ -149,9 +149,7 @@ def test_instance_timeout_annotation crd = build_crd(crd_spec.merge( "metadata" => { "name" => "unittests.stable.example.io", - "annotations" => { - Krane::CustomResourceDefinition::TIMEOUT_FOR_INSTANCE_ANNOTATION => "60S", - }, + "annotations" => { timeout_for_instance_annotation_key => "60S" }, } )) cr = Krane::KubernetesResource.build(namespace: "test", context: "test", @@ -165,7 +163,7 @@ def test_instance_timeout_messages_with_rollout_conditions "metadata" => { "name" => "unittests.stable.example.io", "annotations" => { - Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION => "true", + rollout_conditions_annotation_key => "true", }, }, )) @@ -220,7 +218,7 @@ def merge_rollout_annotation(rollout_conditions) "metadata" => { "name" => "unittests.stable.example.io", "annotations" => { - Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION => rollout_conditions, + rollout_conditions_annotation_key => rollout_conditions, }, }, ) @@ -230,4 +228,12 @@ def build_crd(spec) Krane::CustomResourceDefinition.new(namespace: 'test', context: 'nope', definition: spec, logger: @logger) end + + def rollout_conditions_annotation_key + Krane::Annotation.for(Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION) + end + + def timeout_for_instance_annotation_key + Krane::Annotation.for(Krane::CustomResourceDefinition::TIMEOUT_FOR_INSTANCE_ANNOTATION) + end end diff --git a/test/unit/krane/kubernetes_resource/deployment_test.rb b/test/unit/krane/kubernetes_resource/deployment_test.rb index c556c8151..e563cc22a 100644 --- a/test/unit/krane/kubernetes_resource/deployment_test.rb +++ b/test/unit/krane/kubernetes_resource/deployment_test.rb @@ -158,45 +158,20 @@ def test_deploy_succeeded_with_max_unavailable_as_percent refute(deploy.deploy_succeeded?) end - def test_deploy_succeeded_raises_with_invalid_rollout_annotation_deprecated - deploy = build_synced_deployment( - template: build_deployment_template(rollout: 'bad', use_deprecated: true), - replica_sets: [build_rs_template] - ) - msg = "'#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED}: bad' is "\ - "invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')}" - assert_raises_message(Krane::FatalDeploymentError, msg) do - deploy.deploy_succeeded? - end - end - def test_deploy_succeeded_raises_with_invalid_rollout_annotation deploy = build_synced_deployment( template: build_deployment_template(rollout: 'bad'), replica_sets: [build_rs_template] ) - msg = "'#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: bad' is "\ - "invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')}" + + msg = "'#{rollout_annotation_key}: bad' is invalid. " \ + "Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')}" + assert_raises_message(Krane::FatalDeploymentError, msg) do deploy.deploy_succeeded? end end - def test_validation_fails_with_invalid_rollout_annotation_deprecated - deploy = build_synced_deployment( - template: build_deployment_template(rollout: 'bad', use_deprecated: true), - replica_sets: [] - ) - stub_validation_dry_run(err: "super failed", status: SystemExit.new(1)) - refute(deploy.validate_definition(kubectl)) - - expected = <<~STRING.strip - super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED}: bad' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} - STRING - assert_equal(expected, deploy.validation_error_msg) - end - def test_validation_fails_with_invalid_rollout_annotation deploy = build_synced_deployment(template: build_deployment_template(rollout: 'bad'), replica_sets: []) stub_validation_dry_run(err: "super failed", status: SystemExit.new(1)) @@ -204,7 +179,7 @@ def test_validation_fails_with_invalid_rollout_annotation expected = <<~STRING.strip super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: bad' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} + '#{rollout_annotation_key}: bad' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} STRING assert_equal(expected, deploy.validation_error_msg) end @@ -216,21 +191,6 @@ def test_validation_with_percent_rollout_annotation assert_empty(deploy.validation_error_msg) end - def test_validation_with_number_rollout_annotation_deprecated - deploy = build_synced_deployment( - template: build_deployment_template(rollout: '10', use_deprecated: true), - replica_sets: [] - ) - stub_validation_dry_run(err: "super failed", status: SystemExit.new(1)) - - refute(deploy.validate_definition(kubectl)) - expected = <<~STRING.strip - super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED}: 10' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} - STRING - assert_equal(expected, deploy.validation_error_msg) - end - def test_validation_with_number_rollout_annotation deploy = build_synced_deployment(template: build_deployment_template(rollout: '10'), replica_sets: []) stub_validation_dry_run(err: "super failed", status: SystemExit.new(1)) @@ -238,22 +198,7 @@ def test_validation_with_number_rollout_annotation refute(deploy.validate_definition(kubectl)) expected = <<~STRING.strip super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: 10' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} - STRING - assert_equal(expected, deploy.validation_error_msg) - end - - def test_validation_fails_with_invalid_mix_of_annotation_deprecated - deploy = build_synced_deployment( - template: build_deployment_template(rollout: 'maxUnavailable', strategy: 'Recreate', use_deprecated: true), - replica_sets: [build_rs_template] - ) - stub_validation_dry_run(err: "super failed", status: SystemExit.new(1)) - refute(deploy.validate_definition(kubectl)) - - expected = <<~STRING.strip - super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED}: maxUnavailable' is incompatible with strategy 'Recreate' + '#{rollout_annotation_key}: 10' is invalid. Acceptable values: #{Krane::Deployment::REQUIRED_ROLLOUT_TYPES.join(', ')} STRING assert_equal(expected, deploy.validation_error_msg) end @@ -268,7 +213,7 @@ def test_validation_fails_with_invalid_mix_of_annotation expected = <<~STRING.strip super failed - '#{Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION}: maxUnavailable' is incompatible with strategy 'Recreate' + '#{rollout_annotation_key}: maxUnavailable' is incompatible with strategy 'Recreate' STRING assert_equal(expected, deploy.validation_error_msg) end @@ -370,7 +315,7 @@ def test_deploy_timed_out_based_on_timeout_override }], } ) - template["metadata"]["annotations"][Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION] = "15S" + template["metadata"]["annotations"][timeout_override_annotation_key] = "15S" template["spec"]["progressDeadlineSeconds"] = "10" deploy = build_synced_deployment( template: template, @@ -419,18 +364,10 @@ def stub_validation_dry_run(out: "", err: "", status: SystemExit.new(0)) .returns([out, err, status]) end - def build_deployment_template(status: { 'replicas' => 3 }, rollout: nil, - strategy: 'rollingUpdate', max_unavailable: nil, use_deprecated: false) - - required_rollout_annotation = if use_deprecated - Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED - else - Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION - end - + def build_deployment_template(status: { 'replicas' => 3 }, rollout: nil, strategy: 'rollingUpdate', max_unavailable: nil) base_deployment_manifest = fixtures.find { |fixture| fixture["kind"] == "Deployment" } result = base_deployment_manifest.deep_merge("status" => status) - result["metadata"]["annotations"][required_rollout_annotation] = rollout if rollout + result["metadata"]["annotations"][rollout_annotation_key] = rollout if rollout if (spec_override = status["replicas"].presence) # ignores possibility of surge; need a spec_replicas arg for that result["spec"]["replicas"] = spec_override @@ -482,4 +419,12 @@ def kubectl def fixtures @fixtures ||= YAML.load_stream(File.read(File.join(fixture_path('for_unit_tests'), 'deployment_test.yml'))) end + + def timeout_override_annotation_key + Krane::Annotation.for(Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION) + end + + def rollout_annotation_key + Krane::Annotation.for(Krane::Deployment::REQUIRED_ROLLOUT_ANNOTATION) + end end diff --git a/test/unit/krane/kubernetes_resource_test.rb b/test/unit/krane/kubernetes_resource_test.rb index c39349fd8..fcc0f2b03 100644 --- a/test/unit/krane/kubernetes_resource_test.rb +++ b/test/unit/krane/kubernetes_resource_test.rb @@ -98,49 +98,6 @@ def test_fetch_events_returns_empty_hash_when_kubectl_results_empty assert_operator(events, :empty?) end - def test_deprecated_annotation_generates_warning - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60S", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - assert(customized_resource.has_warnings?, "Deprecated annotation did not generate a warning") - assert_equal("kubernetes-deploy.shopify.io as a prefix for annotations is deprecated: "\ - "Use the 'krane.shopify.io' annotation prefix instead", - customized_resource.validation_warning_msg) - end - - def test_multiple_deprecated_annotations_generates_only_one_warning - definition_extras = build_timeout_metadata("60S", use_deprecated: true).dup - definition_extras['metadata']['annotations']['kubernetes-deploy.shopify.io/something'] = "thing" - customized_resource = DummyResource.new(definition_extras: definition_extras) - customized_resource.validate_definition(kubectl) - - warning_message = <<~MESSAGE.strip - kubernetes-deploy.shopify.io as a prefix for annotations is deprecated: Use the 'krane.shopify.io' annotation prefix instead - MESSAGE - - assert(customized_resource.has_warnings?, "Deprecated annotation did not generate a warning") - assert_equal(customized_resource.validation_warning_msg, warning_message) - end - - def test_valid_annotations_generate_no_warning - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60S")) - customized_resource.validate_definition(kubectl) - refute(customized_resource.has_warnings?, "Valid annotation generated a warning") - end - - def test_can_override_hardcoded_timeout_via_an_annotation_deprecated - basic_resource = DummyResource.new - assert_equal(300, basic_resource.timeout) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60S", use_deprecated: true)) - assert_equal(60, customized_resource.timeout) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60M", use_deprecated: true)) - assert_equal(3600, customized_resource.timeout) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("1H", use_deprecated: true)) - assert_equal(3600, customized_resource.timeout) - end - def test_can_override_hardcoded_timeout_via_an_annotation basic_resource = DummyResource.new assert_equal(300, basic_resource.timeout) @@ -155,14 +112,6 @@ def test_can_override_hardcoded_timeout_via_an_annotation assert_equal(3600, customized_resource.timeout) end - def test_blank_timeout_annotation_is_invalid_deprecated - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - assert(customized_resource.validation_failed?, "Blank annotation was valid") - assert_equal("#{timeout_override_err_prefix_deprecated}: Invalid ISO 8601 duration: \"\" is empty duration", - customized_resource.validation_error_msg) - end - def test_blank_timeout_annotation_is_invalid customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("")) customized_resource.validate_definition(kubectl) @@ -178,24 +127,6 @@ def test_lack_of_timeout_annotation_does_not_fail_validation refute(basic_resource.validation_failed?) end - def test_timeout_override_lower_bound_validation_deprecation - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1S", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - assert(customized_resource.validation_failed?, "Annotation with '-1' was valid") - assert_equal("#{timeout_override_err_prefix_deprecated}: Value must be greater than 0", - customized_resource.validation_error_msg) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0S", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - assert(customized_resource.validation_failed?, "Annotation with '0' was valid") - assert_equal("#{timeout_override_err_prefix_deprecated}: Value must be greater than 0", - customized_resource.validation_error_msg) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("1S", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - refute(customized_resource.validation_failed?, "Annotation with '1' was invalid") - end - def test_timeout_override_lower_bound_validation customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1S")) customized_resource.validate_definition(kubectl) @@ -214,18 +145,6 @@ def test_timeout_override_lower_bound_validation refute(customized_resource.validation_failed?, "Annotation with '1' was invalid") end - def test_timeout_override_upper_bound_validation_deprecated - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H1S", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - assert(customized_resource.validation_failed?, "Annotation with '24H1S' was valid") - expected_message = "#{timeout_override_err_prefix_deprecated}: Value must be less than 24h" - assert_equal(expected_message, customized_resource.validation_error_msg) - - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H", use_deprecated: true)) - customized_resource.validate_definition(kubectl) - refute(customized_resource.validation_failed?, "Annotation with '24H' was invalid") - end - def test_timeout_override_upper_bound_validation customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H1S")) customized_resource.validate_definition(kubectl) @@ -267,23 +186,6 @@ def test_validate_definition_ignores_server_dry_run_unsupported_by_webhook_respo #{Krane::KubernetesResource::SERVER_DRY_RUN_DISABLED_ERROR}") end - def test_annotation_and_kubectl_error_messages_are_combined_deprecated - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad", use_deprecated: true)) - kubectl.expects(:run).returns([ - "{}", - "Error from kubectl: Something else in this template was not valid", - stub(success?: false), - ]) - - customized_resource.validate_definition(kubectl) - assert(customized_resource.validation_failed?, "Expected resource to be invalid") - expected = <<~STRING.strip - #{timeout_override_err_prefix_deprecated}: Invalid ISO 8601 duration: "BAD" - Error from kubectl: Something else in this template was not valid - STRING - assert_equal(expected, customized_resource.validation_error_msg) - end - def test_annotation_and_kubectl_error_messages_are_combined customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad")) kubectl.expects(:run).returns([ @@ -301,12 +203,6 @@ def test_annotation_and_kubectl_error_messages_are_combined assert_equal(expected, customized_resource.validation_error_msg) end - def test_calling_timeout_before_validation_with_invalid_annotation_does_not_raise_deprecated - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad", use_deprecated: true)) - assert_equal(300, customized_resource.timeout) - assert_nil(customized_resource.timeout_override) - end - def test_calling_timeout_before_validation_with_invalid_annotation_does_not_raise customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad")) assert_equal(300, customized_resource.timeout) @@ -327,20 +223,6 @@ def test_deploy_timed_out_respects_hardcoded_timeouts end end - def test_deploy_timed_out_respects_annotation_based_timeouts_deprecated - Timecop.freeze do - custom_dummy = DummyResource.new(definition_extras: build_timeout_metadata("3s", use_deprecated: true)) - refute custom_dummy.deploy_timed_out? - assert_equal 3, custom_dummy.timeout - - custom_dummy.deploy_started_at = Time.now.utc - 3 - refute custom_dummy.deploy_timed_out? - - custom_dummy.deploy_started_at = Time.now.utc - 4 - assert custom_dummy.deploy_timed_out? - end - end - def test_deploy_timed_out_respects_annotation_based_timeouts Timecop.freeze do custom_dummy = DummyResource.new(definition_extras: build_timeout_metadata("3s")) @@ -487,27 +369,15 @@ def kubectl @kubectl ||= build_runless_kubectl end - def timeout_override_err_prefix_deprecated - "kubernetes-deploy.shopify.io/timeout-override annotation is invalid" - end - def timeout_override_err_prefix "krane.shopify.io/timeout-override annotation is invalid" end - def build_timeout_metadata(value, use_deprecated: false) - timeout_override_annotation = if use_deprecated - Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION_DEPRECATED - else - Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION - end - + def build_timeout_metadata(value) { "metadata" => { "name" => "customized", - "annotations" => { - timeout_override_annotation => value, - }, + "annotations" => { timeout_override_annotation_key => value }, }, } end @@ -581,8 +451,16 @@ def build_crd(name:, with_config: false) }, } if with_config - crd["metadata"]["annotations"][Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION] = "true" + crd["metadata"]["annotations"][rollout_conditions_annotation_key] = "true" end crd end + + def timeout_override_annotation_key + Krane::Annotation.for(Krane::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION) + end + + def rollout_conditions_annotation_key + Krane::Annotation.for(Krane::CustomResourceDefinition::ROLLOUT_CONDITIONS_ANNOTATION) + end end From ea1235b101c6e8cb74f20f5d0997c0c0084d7127 Mon Sep 17 00:00:00 2001 From: Kevin Deisz Date: Mon, 3 Aug 2020 15:28:50 -0400 Subject: [PATCH 2/3] Stop annotating ejson secrets --- lib/krane/ejson_secret_provisioner.rb | 3 +-- test/helpers/fixture_set.rb | 3 +-- test/helpers/fixture_sets/ejson_cloud.rb | 6 +++--- test/integration/krane_deploy_test.rb | 18 +++++++++--------- .../krane/ejson_secret_provisioner_test.rb | 10 +++------- 5 files changed, 17 insertions(+), 23 deletions(-) diff --git a/lib/krane/ejson_secret_provisioner.rb b/lib/krane/ejson_secret_provisioner.rb index 6f09fcb09..fef4290f4 100644 --- a/lib/krane/ejson_secret_provisioner.rb +++ b/lib/krane/ejson_secret_provisioner.rb @@ -12,10 +12,10 @@ def initialize(msg) end class EjsonSecretProvisioner - EJSON_SECRET_ANNOTATION = "kubernetes-deploy.shopify.io/ejson-secret" EJSON_SECRET_KEY = "kubernetes_secrets" EJSON_SECRETS_FILE = "secrets.ejson" EJSON_KEYS_SECRET = "ejson-keys" + delegate :namespace, :context, :logger, to: :@task_config def initialize(task_config:, ejson_keys_secret:, ejson_file:, statsd_tags:, selector: nil) @@ -106,7 +106,6 @@ def generate_secret_resource(secret_name, secret_type, data) "name" => secret_name, "labels" => labels, "namespace" => namespace, - "annotations" => { EJSON_SECRET_ANNOTATION => "true" }, }, "data" => encoded_data, } diff --git a/test/helpers/fixture_set.rb b/test/helpers/fixture_set.rb index b7e83d771..3586460d1 100644 --- a/test/helpers/fixture_set.rb +++ b/test/helpers/fixture_set.rb @@ -130,10 +130,9 @@ def assert_pod_templates_present(cm_name) assert_equal(1, pod_templates.size, "Expected 1 podtemplate, got #{pod_templates.size}") end - def assert_secret_present(secret_name, expected_data = nil, type: 'Opaque', ejson: false) + def assert_secret_present(secret_name, expected_data = nil, type: 'Opaque') secret = kubeclient.get_secret(secret_name, namespace) refute_nil(secret, "Secret `#{secret_name}` not found") - assert_annotated(secret, Krane::EjsonSecretProvisioner::EJSON_SECRET_ANNOTATION) if ejson assert_equal(type, secret["type"]) return unless expected_data diff --git a/test/helpers/fixture_sets/ejson_cloud.rb b/test/helpers/fixture_sets/ejson_cloud.rb index 0c4c504ac..24963351d 100644 --- a/test/helpers/fixture_sets/ejson_cloud.rb +++ b/test/helpers/fixture_sets/ejson_cloud.rb @@ -37,12 +37,12 @@ def catphotoscom_key_value def assert_all_secrets_present assert_secret_present("ejson-keys") cert_data = { "tls.crt" => "this-is-the-certificate", "tls.key" => catphotoscom_key_value } - assert_secret_present("catphotoscom", cert_data, type: "kubernetes.io/tls", ejson: true) + assert_secret_present("catphotoscom", cert_data, type: "kubernetes.io/tls") token_data = { "api-token" => "this-is-the-api-token", "service" => "Datadog" } # Impt: it isn't _service: Datadog - assert_secret_present("monitoring-token", token_data, ejson: true) + assert_secret_present("monitoring-token", token_data) - assert_secret_present("unused-secret", { "this-is-for-testing-deletion" => "true" }, ejson: true) + assert_secret_present("unused-secret", "this-is-for-testing-deletion" => "true") end def assert_web_resources_up diff --git a/test/integration/krane_deploy_test.rb b/test/integration/krane_deploy_test.rb index de23fc195..76fe5d01d 100644 --- a/test/integration/krane_deploy_test.rb +++ b/test/integration/krane_deploy_test.rb @@ -578,7 +578,7 @@ def test_pruning_of_secrets_created_from_ejson ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace) ejson_cloud.create_ejson_keys_secret assert_deploy_success(deploy_fixtures("ejson-cloud")) - ejson_cloud.assert_secret_present('unused-secret', ejson: true) + ejson_cloud.assert_secret_present('unused-secret') result = deploy_fixtures("ejson-cloud") do |fixtures| fixtures["secrets.ejson"]["kubernetes_secrets"].delete("unused-secret") @@ -589,10 +589,10 @@ def test_pruning_of_secrets_created_from_ejson # The removed secret was pruned ejson_cloud.refute_resource_exists('secret', 'unused-secret') # The remaining secrets exist - ejson_cloud.assert_secret_present('monitoring-token', ejson: true) - ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', ejson: true) + ejson_cloud.assert_secret_present('monitoring-token') + ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls') # The unmanaged secret was not pruned - ejson_cloud.assert_secret_present('ejson-keys', ejson: false) + ejson_cloud.assert_secret_present('ejson-keys') end def test_pruning_of_existing_managed_secrets_when_ejson_file_has_been_deleted @@ -1161,7 +1161,7 @@ def test_ejson_secrets_respects_no_prune_flag ejson_cloud = FixtureSetAssertions::EjsonCloud.new(@namespace) ejson_cloud.create_ejson_keys_secret assert_deploy_success(deploy_fixtures("ejson-cloud")) - ejson_cloud.assert_secret_present('unused-secret', ejson: true) + ejson_cloud.assert_secret_present('unused-secret') result = deploy_fixtures("ejson-cloud", prune: false) do |fixtures| fixtures["secrets.ejson"]["kubernetes_secrets"].delete("unused-secret") @@ -1169,11 +1169,11 @@ def test_ejson_secrets_respects_no_prune_flag assert_deploy_success(result) # The removed secret was not pruned - ejson_cloud.assert_secret_present('unused-secret', ejson: true) + ejson_cloud.assert_secret_present('unused-secret') # The remaining secrets also exist - ejson_cloud.assert_secret_present('monitoring-token', ejson: true) - ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls', ejson: true) - ejson_cloud.assert_secret_present('ejson-keys', ejson: false) + ejson_cloud.assert_secret_present('monitoring-token') + ejson_cloud.assert_secret_present('catphotoscom', type: 'kubernetes.io/tls') + ejson_cloud.assert_secret_present('ejson-keys') end def test_deploy_task_fails_when_ejson_keys_prunable diff --git a/test/unit/krane/ejson_secret_provisioner_test.rb b/test/unit/krane/ejson_secret_provisioner_test.rb index c9a5bdfec..8aa0f3a0e 100644 --- a/test/unit/krane/ejson_secret_provisioner_test.rb +++ b/test/unit/krane/ejson_secret_provisioner_test.rb @@ -170,15 +170,15 @@ def with_ejson_file(content) end def dummy_ejson_secret(data = correct_ejson_key_secret_data) - dummy_secret_hash(data: data, name: 'ejson-keys', ejson: false) + dummy_secret_hash(data: data, name: 'ejson-keys') end - def dummy_secret_hash(name: SecureRandom.hex(4), data: {}, ejson: true) + def dummy_secret_hash(name: SecureRandom.hex(4), data: {}) encoded_data = data.each_with_object({}) do |(key, value), encoded| encoded[key] = Base64.strict_encode64(value) end - secret = { + { 'kind' => 'Secret', 'apiVersion' => 'v1', 'type' => 'Opaque', @@ -189,10 +189,6 @@ def dummy_secret_hash(name: SecureRandom.hex(4), data: {}, ejson: true) }, "data" => encoded_data, } - if ejson - secret['metadata']['annotations'] = { Krane::EjsonSecretProvisioner::EJSON_SECRET_ANNOTATION => true } - end - secret end def dummy_version From 2d98f49c1a2187c58088519cbc6bcdc91e7b655b Mon Sep 17 00:00:00 2001 From: Kevin Deisz Date: Mon, 3 Aug 2020 15:36:38 -0400 Subject: [PATCH 3/3] Fix up rubocop violations for annotation removal --- test/unit/krane/kubernetes_resource/deployment_test.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/unit/krane/kubernetes_resource/deployment_test.rb b/test/unit/krane/kubernetes_resource/deployment_test.rb index e563cc22a..3cae4b90d 100644 --- a/test/unit/krane/kubernetes_resource/deployment_test.rb +++ b/test/unit/krane/kubernetes_resource/deployment_test.rb @@ -364,7 +364,9 @@ def stub_validation_dry_run(out: "", err: "", status: SystemExit.new(0)) .returns([out, err, status]) end - def build_deployment_template(status: { 'replicas' => 3 }, rollout: nil, strategy: 'rollingUpdate', max_unavailable: nil) + def build_deployment_template(status: { 'replicas' => 3 }, rollout: nil, + strategy: 'rollingUpdate', max_unavailable: nil) + base_deployment_manifest = fixtures.find { |fixture| fixture["kind"] == "Deployment" } result = base_deployment_manifest.deep_merge("status" => status) result["metadata"]["annotations"][rollout_annotation_key] = rollout if rollout