From 362a359e92c59a18d4153d73d4adb61581cbe548 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Thu, 21 Dec 2017 17:34:04 -0500 Subject: [PATCH 1/4] Enable per-resource timeout customization via an annotation --- README.md | 4 + lib/kubernetes-deploy/kubernetes_resource.rb | 29 ++++-- .../hello-cloud/unmanaged-pod.yml.erb | 2 + test/integration/kubernetes_deploy_test.rb | 1 + .../kubernetes_resource_test.rb | 88 ++++++++++++++++++- 5 files changed, 117 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index f9f361c46..bab566f54 100644 --- a/README.md +++ b/README.md @@ -39,6 +39,7 @@ This repo also includes related tools for [running tasks](#kubernetes-run) and [ * [Installation](#installation) * [Usage](#usage) * [Using templates and variables](#using-templates-and-variables) + * [Customizing behaviour with annotations](#customizing-behaviour-with-annotations) * [Running tasks at the beginning of a deploy](#running-tasks-at-the-beginning-of-a-deploy) * [Deploying Kubernetes secrets (from EJSON)](#deploying-kubernetes-secrets-from-ejson) @@ -120,6 +121,9 @@ All templates must be YAML formatted. You can also use ERB. The following local You can add additional variables using the `--bindings=BINDINGS` option. For example, `kubernetes-deploy my-app cluster1 --bindings=color=blue,size=large` will expose `color` and `size` in your templates. +### Customizing behaviour with annotations + +- `kubernetes-deploy.shopify.io/timeout-override-seconds` (any resource): Override the tool's hard timeout for one specific resource. Value must be a positive number of seconds (digits only). ### Running tasks at the beginning of a deploy diff --git a/lib/kubernetes-deploy/kubernetes_resource.rb b/lib/kubernetes-deploy/kubernetes_resource.rb index efbb17d5b..382b97f56 100644 --- a/lib/kubernetes-deploy/kubernetes_resource.rb +++ b/lib/kubernetes-deploy/kubernetes_resource.rb @@ -6,7 +6,7 @@ module KubernetesDeploy class KubernetesResource - attr_reader :name, :namespace, :context, :validation_error_msg + attr_reader :name, :namespace, :context attr_writer :type, :deploy_started_at TIMEOUT = 5.minutes @@ -22,6 +22,8 @@ class KubernetesResource If you have reason to believe it will succeed, retry the deploy to continue to monitor the rollout. MSG + TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override-seconds" + def self.build(namespace:, context:, definition:, logger:) opts = { namespace: namespace, context: context, definition: definition, logger: logger } if KubernetesDeploy.const_defined?(definition["kind"]) @@ -39,9 +41,14 @@ def self.timeout end def timeout + return timeout_override.to_i if timeout_override.present? self.class.timeout end + def timeout_override + @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION) + end + def pretty_timeout_type "timeout: #{timeout}s" end @@ -59,20 +66,26 @@ def initialize(namespace:, context:, definition:, logger:) @logger = logger @definition = definition @statsd_report_done = false - @validation_error_msg = nil + @validation_errors = [] end def validate_definition - @validation_error_msg = nil + @validation_errors = [] + validate_annotations + command = ["create", "-f", file_path, "--dry-run", "--output=name"] _, err, st = kubectl.run(*command, log_failure: false) return true if st.success? - @validation_error_msg = err + @validation_errors << err false end + def validation_error_msg + @validation_errors.join("\n") + end + def validation_failed? - @validation_error_msg.present? + @validation_errors.present? end def id @@ -277,6 +290,12 @@ def to_s private + def validate_annotations + return unless timeout_override.present? + return if timeout_override.strip.match(/\A\d+\z/) && timeout_override.to_i > 0 + @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation must contain digits only and must be > 0" + end + def file @file ||= create_definition_tempfile end diff --git a/test/fixtures/hello-cloud/unmanaged-pod.yml.erb b/test/fixtures/hello-cloud/unmanaged-pod.yml.erb index fdc75bdb9..bbbb3ebf2 100644 --- a/test/fixtures/hello-cloud/unmanaged-pod.yml.erb +++ b/test/fixtures/hello-cloud/unmanaged-pod.yml.erb @@ -2,6 +2,8 @@ apiVersion: v1 kind: Pod metadata: name: unmanaged-pod-<%= deployment_id %> + annotations: + kubernetes-deploy.shopify.io/timeout-override-seconds: "42" labels: type: unmanaged-pod name: unmanaged-pod-<%= deployment_id %> diff --git a/test/integration/kubernetes_deploy_test.rb b/test/integration/kubernetes_deploy_test.rb index 7bac0ed90..983fc434c 100644 --- a/test/integration/kubernetes_deploy_test.rb +++ b/test/integration/kubernetes_deploy_test.rb @@ -9,6 +9,7 @@ def test_full_hello_cloud_set_deploy_succeeds assert_logs_match_all([ "Deploying ConfigMap/hello-cloud-configmap-data (timeout: 30s)", + %r{Deploying Pod/unmanaged-pod-[-\w]+ \(timeout: 42s\)}, # annotation timeout override "Hello from the command runner!", # unmanaged pod logs "Result: SUCCESS", "Successfully deployed 17 resources" diff --git a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb index 110d907e5..275d0aaf6 100644 --- a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb +++ b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb @@ -3,14 +3,18 @@ class KubernetesResourceTest < KubernetesDeploy::TestCase class DummyResource < KubernetesDeploy::KubernetesResource - def initialize(*) - definition = { "kind" => "DummyResource", "metadata" => { "name" => "test" } } + def initialize(definition_extras: {}) + definition = { "kind" => "DummyResource", "metadata" => { "name" => "test" } }.merge(definition_extras) super(namespace: 'test', context: 'test', definition: definition, logger: @logger) end def exists? true end + + def file_path + "/tmp/foo/bar" + end end def test_service_and_deployment_timeouts_are_equal @@ -53,8 +57,88 @@ def test_fetch_events_returns_empty_hash_when_kubectl_results_empty assert_operator events, :empty? end + def test_can_override_hardcoded_timeout_via_an_annotation + basic_resource = DummyResource.new + assert_equal 5.minutes, basic_resource.timeout + + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60")) + assert_equal 60, customized_resource.timeout + + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata(" 60 ")) + assert_equal 60, customized_resource.timeout + end + + def test_blank_timeout_annotation_is_ignored + stub_kubectl_response("create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything, resp: "{}") + + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("")) + customized_resource.validate_definition + refute customized_resource.validation_failed?, "Blank annotation with was invalid" + assert_equal 5.minutes, customized_resource.timeout + end + + def test_validation_of_timeout_annotation + expected_cmd = ["create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything] + error_msg = "kubernetes-deploy.shopify.io/timeout-override-seconds annotation " \ + "must contain digits only and must be > 0" + + stub_kubectl_response(*expected_cmd, resp: "{}") + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("sixty")) + customized_resource.validate_definition + assert customized_resource.validation_failed?, "Annotation with 'sixty' was valid" + assert_equal error_msg, customized_resource.validation_error_msg + + stub_kubectl_response(*expected_cmd, resp: "{}") + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1")) + customized_resource.validate_definition + assert customized_resource.validation_failed?, "Annotation with '-1' was valid" + assert_equal error_msg, customized_resource.validation_error_msg + + stub_kubectl_response(*expected_cmd, resp: "{}") + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0")) + customized_resource.validate_definition + assert customized_resource.validation_failed?, "Annotation with '0' was valid" + assert_equal error_msg, customized_resource.validation_error_msg + + stub_kubectl_response(*expected_cmd, resp: "{}") + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("10m")) + customized_resource.validate_definition + assert customized_resource.validation_failed?, "Annotation with '10m' was valid" + assert_equal error_msg, customized_resource.validation_error_msg + end + + def test_annotation_and_kubectl_error_messages_are_combined + stub_kubectl_response( + "create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything, + resp: "{}", + err: "Error from kubectl: Something else in this template was not valid", + success: false + ) + + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad")) + customized_resource.validate_definition + assert customized_resource.validation_failed?, "Expected resource to be invalid" + + expected = <<~STRING.strip + kubernetes-deploy.shopify.io/timeout-override-seconds annotation must contain digits only and must be > 0 + Error from kubectl: Something else in this template was not valid + STRING + assert_equal expected, customized_resource.validation_error_msg + end + private + def build_timeout_metadata(value) + { + "metadata" => { + "name" => "customized", + "annotations" => { + KubernetesDeploy::KubernetesResource::TIMEOUT_OVERRIDE_ANNOTATION => value + } + } + } + end + def assert_includes_dummy_events(events, first:, second:) unless first || second assert_operator events, :empty? From 0e4eeb00e9415c5c6735bb488921385318588cb9 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Sat, 23 Dec 2017 00:03:52 -0500 Subject: [PATCH 2/4] Use (relaxed) ISO8601 durations and add more tests --- README.md | 2 +- lib/kubernetes-deploy.rb | 2 + lib/kubernetes-deploy/duration_parser.rb | 39 +++++ lib/kubernetes-deploy/kubernetes_resource.rb | 30 +++- .../hello-cloud/unmanaged-pod.yml.erb | 2 +- .../kubernetes-deploy/duration_parser_test.rb | 47 ++++++ .../kubernetes_resource_test.rb | 142 +++++++++++++----- 7 files changed, 213 insertions(+), 51 deletions(-) create mode 100644 lib/kubernetes-deploy/duration_parser.rb create mode 100644 test/unit/kubernetes-deploy/duration_parser_test.rb diff --git a/README.md b/README.md index bab566f54..ad50f303c 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,7 @@ You can add additional variables using the `--bindings=BINDINGS` option. For exa ### Customizing behaviour with annotations -- `kubernetes-deploy.shopify.io/timeout-override-seconds` (any resource): Override the tool's hard timeout for one specific resource. Value must be a positive number of seconds (digits only). +- `kubernetes-deploy.shopify.io/timeout-override` (any resource): Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours. Example values: 45s / 3m / 1h / PT0.25H ### Running tasks at the beginning of a deploy diff --git a/lib/kubernetes-deploy.rb b/lib/kubernetes-deploy.rb index 390ca087b..31918d791 100644 --- a/lib/kubernetes-deploy.rb +++ b/lib/kubernetes-deploy.rb @@ -8,6 +8,7 @@ require 'active_support/core_ext/string/strip' require 'active_support/core_ext/hash/keys' require 'active_support/core_ext/array/conversions' +require 'active_support/duration' require 'colorized_string' require 'kubernetes-deploy/version' @@ -17,6 +18,7 @@ require 'kubernetes-deploy/statsd' require 'kubernetes-deploy/concurrency' require 'kubernetes-deploy/bindings_parser' +require 'kubernetes-deploy/duration_parser' module KubernetesDeploy KubernetesDeploy::StatsD.build diff --git a/lib/kubernetes-deploy/duration_parser.rb b/lib/kubernetes-deploy/duration_parser.rb new file mode 100644 index 000000000..41de8208a --- /dev/null +++ b/lib/kubernetes-deploy/duration_parser.rb @@ -0,0 +1,39 @@ +# frozen_string_literal: true + +module KubernetesDeploy + ## + # This class is a less strict extension of ActiveSupport::Duration::ISO8601Parser. + # In addition to full ISO8601 durations, it can parse unprefixed ISO8601 time components (e.g. '1H'). + # It is also case-insensitive. + # For example, this class considers the values "1H", "1h" and "PT1H" to be valid and equivalent. + + class DurationParser + class ParsingError < ArgumentError; end + + def initialize(value) + @iso8601_str = value.to_s.strip.upcase + end + + def parse! + if @iso8601_str.blank? + raise ParsingError, "Cannot parse blank value" + end + + parser = ActiveSupport::Duration::ISO8601Parser.new(@iso8601_str) + parser.mode = :time unless @iso8601_str.start_with?("P") + parts = parser.parse! + ActiveSupport::Duration.new(calculate_total_seconds(parts), parts) + rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e + raise ParsingError, e.message + end + + private + + # https://github.com/rails/rails/blob/19c450d5d99275924254b2041b6ad470fdaa1f93/activesupport/lib/active_support/duration.rb#L79-L83 + def calculate_total_seconds(parts) + parts.inject(0) do |total, (part, value)| + total + value * ActiveSupport::Duration::PARTS_IN_SECONDS[part] + end + end + end +end diff --git a/lib/kubernetes-deploy/kubernetes_resource.rb b/lib/kubernetes-deploy/kubernetes_resource.rb index 382b97f56..d5f64564d 100644 --- a/lib/kubernetes-deploy/kubernetes_resource.rb +++ b/lib/kubernetes-deploy/kubernetes_resource.rb @@ -22,7 +22,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 = "kubernetes-deploy.shopify.io/timeout-override-seconds" + TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override" def self.build(namespace:, context:, definition:, logger:) opts = { namespace: namespace, context: context, definition: definition, logger: logger } @@ -41,12 +41,15 @@ def self.timeout end def timeout - return timeout_override.to_i if timeout_override.present? + return timeout_override if timeout_override.present? self.class.timeout end def timeout_override - @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION) + return @timeout_override if defined?(@timeout_override) + @timeout_override = DurationParser.new(timeout_annotation).parse!.to_i + rescue DurationParser::ParsingError + @timeout_override = nil end def pretty_timeout_type @@ -71,7 +74,7 @@ def initialize(namespace:, context:, definition:, logger:) def validate_definition @validation_errors = [] - validate_annotations + validate_timeout_annotation command = ["create", "-f", file_path, "--dry-run", "--output=name"] _, err, st = kubectl.run(*command, log_failure: false) @@ -290,10 +293,21 @@ def to_s private - def validate_annotations - return unless timeout_override.present? - return if timeout_override.strip.match(/\A\d+\z/) && timeout_override.to_i > 0 - @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation must contain digits only and must be > 0" + def validate_timeout_annotation + return if timeout_annotation.nil? + + override = DurationParser.new(timeout_annotation).parse! + if override <= 0 + @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be greater than 0" + elsif override > 24.hours + @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: Value must be less than 24h" + end + rescue DurationParser::ParsingError => e + @validation_errors << "#{TIMEOUT_OVERRIDE_ANNOTATION} annotation is invalid: #{e}" + end + + def timeout_annotation + @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION) end def file diff --git a/test/fixtures/hello-cloud/unmanaged-pod.yml.erb b/test/fixtures/hello-cloud/unmanaged-pod.yml.erb index bbbb3ebf2..83b3cd8bb 100644 --- a/test/fixtures/hello-cloud/unmanaged-pod.yml.erb +++ b/test/fixtures/hello-cloud/unmanaged-pod.yml.erb @@ -3,7 +3,7 @@ kind: Pod metadata: name: unmanaged-pod-<%= deployment_id %> annotations: - kubernetes-deploy.shopify.io/timeout-override-seconds: "42" + kubernetes-deploy.shopify.io/timeout-override: "42s" labels: type: unmanaged-pod name: unmanaged-pod-<%= deployment_id %> diff --git a/test/unit/kubernetes-deploy/duration_parser_test.rb b/test/unit/kubernetes-deploy/duration_parser_test.rb new file mode 100644 index 000000000..35fb6dde9 --- /dev/null +++ b/test/unit/kubernetes-deploy/duration_parser_test.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true +require 'test_helper' + +class DurationParserTest < KubernetesDeploy::TestCase + def test_parses_correct_iso_durations_with_prefixes + assert_equal 300, new_parser("PT300S").parse! + assert_equal 300, new_parser("PT5M").parse! + assert_equal 900, new_parser("PT0.25H").parse! + assert_equal 110839937, new_parser("P3Y6M4DT12H30M5S").parse! + end + + def test_parses_correct_iso_durations_without_prefixes + assert_equal 300, new_parser("300S").parse! + assert_equal 300, new_parser("5M").parse! + assert_equal 900, new_parser("0.25H").parse! + assert_equal(-60, new_parser("-1M").parse!) + end + + def test_parse_is_case_insensitive + assert_equal 30, new_parser("30S").parse! + assert_equal 30, new_parser("30s").parse! + end + + def test_parse_raises_expected_error_for_blank_values + ["", " ", nil].each do |blank_value| + assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, "Cannot parse blank value") do + new_parser(blank_value).parse! + end + end + end + + def test_extra_whitespace_is_stripped_from_values + assert_equal 30, new_parser(" 30S ").parse! + end + + def test_parse_raises_expected_error_when_value_is_invalid + assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, 'Invalid ISO 8601 duration: "FOO"') do + new_parser("foo").parse! + end + end + + private + + def new_parser(value) + KubernetesDeploy::DurationParser.new(value) + end +end diff --git a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb index 275d0aaf6..c75c6b350 100644 --- a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb +++ b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb @@ -3,18 +3,35 @@ class KubernetesResourceTest < KubernetesDeploy::TestCase class DummyResource < KubernetesDeploy::KubernetesResource + attr_writer :succeeded + def initialize(definition_extras: {}) definition = { "kind" => "DummyResource", "metadata" => { "name" => "test" } }.merge(definition_extras) - super(namespace: 'test', context: 'test', definition: definition, logger: @logger) + super(namespace: 'test', context: 'test', definition: definition, logger: ::Logger.new($stderr)) + @succeeded = false end def exists? true end + def deploy_succeeded? + @succeeded + end + def file_path "/tmp/foo/bar" end + + def kubectl + @kubectl_stub ||= begin + kubectl_stub = super + def kubectl_stub.run(*) + ["", "", SystemExit.new(0)] + end + kubectl_stub + end + end end def test_service_and_deployment_timeouts_are_equal @@ -31,7 +48,7 @@ def test_fetch_events_parses_tricky_events_correctly tricky_events = dummy_events(start_time) assert tricky_events.first[:message].count("\n") > 1, "Sanity check failed: inadequate newlines in test events" - stub_kubectl_response("get", "events", anything, resp: build_event_jsonpath(tricky_events), json: false) + dummy.kubectl.expects(:run).returns([build_event_jsonpath(tricky_events), "", SystemExit.new(0)]) events = dummy.fetch_events assert_includes_dummy_events(events, first: true, second: true) end @@ -43,7 +60,7 @@ def test_fetch_events_excludes_events_from_previous_deploys mixed_time_events = dummy_events(start_time) mixed_time_events.first[:last_seen] = 1.hour.ago - stub_kubectl_response("get", "events", anything, resp: build_event_jsonpath(mixed_time_events), json: false) + dummy.kubectl.expects(:run).returns([build_event_jsonpath(mixed_time_events), "", SystemExit.new(0)]) events = dummy.fetch_events assert_includes_dummy_events(events, first: false, second: true) end @@ -52,82 +69,125 @@ def test_fetch_events_returns_empty_hash_when_kubectl_results_empty dummy = DummyResource.new dummy.deploy_started_at = Time.now.utc - 10.seconds - stub_kubectl_response("get", "events", anything, resp: "", json: false) + dummy.kubectl.expects(:run).returns(["", "", SystemExit.new(0)]) events = dummy.fetch_events assert_operator events, :empty? end def test_can_override_hardcoded_timeout_via_an_annotation basic_resource = DummyResource.new - assert_equal 5.minutes, basic_resource.timeout + assert_equal 300, basic_resource.timeout - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60")) + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60S")) assert_equal 60, customized_resource.timeout - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata(" 60 ")) - assert_equal 60, customized_resource.timeout - end + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("60M")) + assert_equal 3600, customized_resource.timeout - def test_blank_timeout_annotation_is_ignored - stub_kubectl_response("create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything, resp: "{}") + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("1H")) + assert_equal 3600, customized_resource.timeout + end + def test_blank_timeout_annotation_is_invalid customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("")) customized_resource.validate_definition - refute customized_resource.validation_failed?, "Blank annotation with was invalid" - assert_equal 5.minutes, customized_resource.timeout + assert customized_resource.validation_failed?, "Blank annotation was valid" + assert_equal "#{timeout_override_err_prefix}: Cannot parse blank value", customized_resource.validation_error_msg end - def test_validation_of_timeout_annotation - expected_cmd = ["create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything] - error_msg = "kubernetes-deploy.shopify.io/timeout-override-seconds annotation " \ - "must contain digits only and must be > 0" + def test_lack_of_timeout_annotation_does_not_fail_validation + basic_resource = DummyResource.new + assert_equal 300, basic_resource.timeout + basic_resource.validate_definition + refute basic_resource.validation_failed? + end - stub_kubectl_response(*expected_cmd, resp: "{}") - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("sixty")) + def test_timeout_override_lower_bound_validation + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1S")) customized_resource.validate_definition - assert customized_resource.validation_failed?, "Annotation with 'sixty' was valid" - assert_equal error_msg, customized_resource.validation_error_msg + assert customized_resource.validation_failed?, "Annotation with '-1' was valid" + assert_equal "#{timeout_override_err_prefix}: Value must be greater than 0", + customized_resource.validation_error_msg - stub_kubectl_response(*expected_cmd, resp: "{}") - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("-1")) + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0S")) customized_resource.validate_definition - assert customized_resource.validation_failed?, "Annotation with '-1' was valid" - assert_equal error_msg, customized_resource.validation_error_msg + assert customized_resource.validation_failed?, "Annotation with '0' was valid" + assert_equal "#{timeout_override_err_prefix}: Value must be greater than 0", + customized_resource.validation_error_msg - stub_kubectl_response(*expected_cmd, resp: "{}") - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0")) + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("1S")) + customized_resource.validate_definition + refute customized_resource.validation_failed?, "Annotation with '1' was invalid" + end + + def test_timeout_override_upper_bound_validation + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H1S")) customized_resource.validate_definition assert customized_resource.validation_failed?, "Annotation with '0' was valid" - assert_equal error_msg, customized_resource.validation_error_msg + assert_equal "#{timeout_override_err_prefix}: Value must be less than 24h", customized_resource.validation_error_msg - stub_kubectl_response(*expected_cmd, resp: "{}") - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("10m")) + customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H")) customized_resource.validate_definition - assert customized_resource.validation_failed?, "Annotation with '10m' was valid" - assert_equal error_msg, customized_resource.validation_error_msg + refute customized_resource.validation_failed?, "Annotation with '24H' was invalid" end def test_annotation_and_kubectl_error_messages_are_combined - stub_kubectl_response( - "create", "-f", "/tmp/foo/bar", "--dry-run", "--output=name", anything, - resp: "{}", - err: "Error from kubectl: Something else in this template was not valid", - success: false - ) - customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("bad")) + customized_resource.kubectl.expects(:run).returns([ + "{}", + "Error from kubectl: Something else in this template was not valid", + stub(success?: false) + ]) + customized_resource.validate_definition assert customized_resource.validation_failed?, "Expected resource to be invalid" - expected = <<~STRING.strip - kubernetes-deploy.shopify.io/timeout-override-seconds annotation must contain digits only and must be > 0 + #{timeout_override_err_prefix}: 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_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 + assert_nil customized_resource.timeout_override + end + + def test_deploy_timed_out_respects_hardcoded_timeouts + Timecop.freeze do + dummy = DummyResource.new + refute dummy.deploy_timed_out? + assert_equal 300, dummy.timeout + + dummy.deploy_started_at = Time.now.utc - 300 + refute dummy.deploy_timed_out? + + dummy.deploy_started_at = Time.now.utc - 301 + assert 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")) + 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 + private + def timeout_override_err_prefix + "kubernetes-deploy.shopify.io/timeout-override annotation is invalid" + end + def build_timeout_metadata(value) { "metadata" => { From 40a9fd7ccd79a8afd7228b896535bfac7c99d83c Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Tue, 2 Jan 2018 18:35:31 -0500 Subject: [PATCH 3/4] Simplify parser --- lib/kubernetes-deploy/duration_parser.rb | 24 +++++-------------- .../kubernetes-deploy/duration_parser_test.rb | 5 +++- .../kubernetes_resource_test.rb | 5 ++-- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/lib/kubernetes-deploy/duration_parser.rb b/lib/kubernetes-deploy/duration_parser.rb index 41de8208a..662e8144d 100644 --- a/lib/kubernetes-deploy/duration_parser.rb +++ b/lib/kubernetes-deploy/duration_parser.rb @@ -15,24 +15,12 @@ def initialize(value) end def parse! - if @iso8601_str.blank? - raise ParsingError, "Cannot parse blank value" - end - - parser = ActiveSupport::Duration::ISO8601Parser.new(@iso8601_str) - parser.mode = :time unless @iso8601_str.start_with?("P") - parts = parser.parse! - ActiveSupport::Duration.new(calculate_total_seconds(parts), parts) - rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e - raise ParsingError, e.message - end - - private - - # https://github.com/rails/rails/blob/19c450d5d99275924254b2041b6ad470fdaa1f93/activesupport/lib/active_support/duration.rb#L79-L83 - def calculate_total_seconds(parts) - parts.inject(0) do |total, (part, value)| - total + value * ActiveSupport::Duration::PARTS_IN_SECONDS[part] + ActiveSupport::Duration.parse("PT#{@iso8601_str}") # By default assume it is just a time component + rescue ActiveSupport::Duration::ISO8601Parser::ParsingError + begin + ActiveSupport::Duration.parse(@iso8601_str) + rescue ActiveSupport::Duration::ISO8601Parser::ParsingError => e + raise ParsingError, e.message end end end diff --git a/test/unit/kubernetes-deploy/duration_parser_test.rb b/test/unit/kubernetes-deploy/duration_parser_test.rb index 35fb6dde9..429112d98 100644 --- a/test/unit/kubernetes-deploy/duration_parser_test.rb +++ b/test/unit/kubernetes-deploy/duration_parser_test.rb @@ -19,11 +19,14 @@ def test_parses_correct_iso_durations_without_prefixes def test_parse_is_case_insensitive assert_equal 30, new_parser("30S").parse! assert_equal 30, new_parser("30s").parse! + assert_equal 30, new_parser("pt30s").parse! + assert_equal 110839937, new_parser("p3y6M4dT12H30M5s").parse! end def test_parse_raises_expected_error_for_blank_values ["", " ", nil].each do |blank_value| - assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, "Cannot parse blank value") do + expected_msg = 'Invalid ISO 8601 duration: "" is empty duration' + assert_raises_message(KubernetesDeploy::DurationParser::ParsingError, expected_msg) do new_parser(blank_value).parse! end end diff --git a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb index c75c6b350..d28467aa6 100644 --- a/test/unit/kubernetes-deploy/kubernetes_resource_test.rb +++ b/test/unit/kubernetes-deploy/kubernetes_resource_test.rb @@ -92,7 +92,8 @@ def test_blank_timeout_annotation_is_invalid customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("")) customized_resource.validate_definition assert customized_resource.validation_failed?, "Blank annotation was valid" - assert_equal "#{timeout_override_err_prefix}: Cannot parse blank value", customized_resource.validation_error_msg + assert_equal "#{timeout_override_err_prefix}: Invalid ISO 8601 duration: \"\" is empty duration", + customized_resource.validation_error_msg end def test_lack_of_timeout_annotation_does_not_fail_validation @@ -123,7 +124,7 @@ def test_timeout_override_lower_bound_validation def test_timeout_override_upper_bound_validation customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H1S")) customized_resource.validate_definition - assert customized_resource.validation_failed?, "Annotation with '0' was valid" + assert customized_resource.validation_failed?, "Annotation with '24H1S' was valid" assert_equal "#{timeout_override_err_prefix}: Value must be less than 24h", customized_resource.validation_error_msg customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("24H")) From 05585b9af7f82fc0775333678c65a7a9f52a519e Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Thu, 4 Jan 2018 18:06:19 -0500 Subject: [PATCH 4/4] Improve readme description --- README.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index ad50f303c..c8f0438f3 100644 --- a/README.md +++ b/README.md @@ -123,7 +123,9 @@ You can add additional variables using the `--bindings=BINDINGS` option. For exa ### Customizing behaviour with annotations -- `kubernetes-deploy.shopify.io/timeout-override` (any resource): Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours. Example values: 45s / 3m / 1h / PT0.25H +- `kubernetes-deploy.shopify.io/timeout-override`: Override the tool's hard timeout for one specific resource. Both full ISO8601 durations and the time portion of ISO8601 durations are valid. Value must be between 1 second and 24 hours. + - _Example values_: 45s / 3m / 1h / PT0.25H + - _Compatibility_: all resource types (Note: `Deployment` timeouts are based on `spec.progressDeadlineSeconds` if present, and that field has a default value as of the `apps/v1beta1` group version. Using this annotation will have no effect on `Deployment`s that time out with "Timeout reason: ProgressDeadlineExceeded".) ### Running tasks at the beginning of a deploy