Use (relaxed) ISO8601 durations and add more tests

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
 

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

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

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
@@ -291,9 +294,18 @@ 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"
+      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

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 %>

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

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,118 @@ 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_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 '-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("sixty"))
+    customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0S"))
     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 '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("-1"))
+    customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("1S"))
     customized_resource.validate_definition
-    assert customized_resource.validation_failed?, "Annotation with '-1' was valid"
-    assert_equal error_msg, customized_resource.validation_error_msg
+    refute customized_resource.validation_failed?, "Annotation with '1' was invalid"
+  end
 
-    stub_kubectl_response(*expected_cmd, resp: "{}")
-    customized_resource = DummyResource.new(definition_extras: build_timeout_metadata("0"))
+  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" => {