PR Feedback

Shopify · Sep 9, 2019 · 3098276 · 3098276
1 parent f643581
commit 3098276
Show file tree

Hide file tree

Showing 9 changed files with 74 additions and 119 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,8 @@
 ## next
 
+*Other*
+- Deprecate `kubernetes-deploy.shopify.io` annotations in favour of `krane.shopify.io`.
+
 ## 0.27.0
 *Enhancements*
 - (alpha) Introduce a new `-f` flag for `kubernetes-deploy`. Allows passing in of multiple directories and/or filenames. Currently only usable by `kubernetes-deploy`, not `kubernetes-render`. [#514](https://github.com/Shopify/kubernetes-deploy/pull/514)
@@ -8,9 +11,6 @@
 - **[Breaking change]** Added ServiceAccount, PodTemplate, ReplicaSet, Role, and RoleBinding to the prune whitelist.
   * To see what resources may be affected, run `kubectl get $RESOURCE -o jsonpath='{ range .items[*] }{.metadata.namespace}{ "\t" }{.metadata.name}{ "\t" }{.metadata.annotations}{ "\n" }{ end }' --all-namespaces | grep "last-applied"`
   * To exclude a resource from kubernetes-deploy (and kubectl apply) management, remove the last-applied annotation `kubectl annotate $RESOURCE $SECRET_NAME kubectl.kubernetes.io/last-applied-configuration-`.
-
-*Other*
-- Deprecate `kubernetes-deploy.shopify.io` annotations in favour of `krane.shopify.io`.
 
 *Bug Fixes*
 - StatefulSets with 0 replicas explicitly specified don't fail deploy. [#540](https://github.com/Shopify/kubernetes-deploy/pull/540)

diff --git a/lib/kubernetes-deploy/kubernetes_resource.rb b/lib/kubernetes-deploy/kubernetes_resource.rb
@@ -95,7 +95,7 @@ def timeout
     def timeout_override
       return @timeout_override if defined?(@timeout_override)
 
-      @timeout_override = DurationParser.new(timeout_annotation).parse!.to_i
+      @timeout_override = DurationParser.new(krane_annotation_value("timeout-override")).parse!.to_i
     rescue DurationParser::ParsingError
       @timeout_override = nil
     end
@@ -409,33 +409,35 @@ def global?
     private
 
     def validate_timeout_annotation
-      return if timeout_annotation.nil?
+      return if krane_annotation_value("timeout-override").nil?
 
-      override = DurationParser.new(timeout_annotation).parse!
+      override = DurationParser.new(krane_annotation_value("timeout-override")).parse!
       if override <= 0
-        @validation_errors << "#{annotation_prefix} annotation is invalid: Value must be greater than 0"
+        @validation_errors << "#{timeout_annotation_key} annotation is invalid: Value must be greater than 0"
       elsif override > 24.hours
-        @validation_errors << "#{annotation_prefix} annotation is invalid: Value must be less than 24h"
+        @validation_errors << "#{timeout_annotation_key} annotation is invalid: Value must be less than 24h"
       end
     rescue DurationParser::ParsingError => e
-      @validation_errors << "#{annotation_prefix} annotation is invalid: #{e}"
+      @validation_errors << "#{timeout_annotation_key} annotation is invalid: #{e}"
     end
 
     def validate_annotation_version
       annotation_keys = @definition.dig("metadata", "annotations")&.keys
-      annotation_keys&.any? do |annotation|
+      annotation_keys&.each do |annotation|
         if annotation.include?("kubernetes-deploy.shopify.io")
-          @validation_warnings << "#{annotation} is deprecated: Use 'krane.shopify.io' annotations instead"
+          annotation_prefix = annotation.split('/').first
+          @validation_warnings << "#{annotation_prefix} as a prefix for annotations is deprecated: "\
+            "Use the 'krane.shopify.io' annotation prefix instead"
         end
       end
     end
 
-    def timeout_annotation
-      @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION_DEPRECATED) ||
-        @definition.dig("metadata", "annotations", TIMEOUT_OVERRIDE_ANNOTATION)
+    def krane_annotation_value(suffix)
+      @definition.dig("metadata", "annotations", "kubernetes-deploy.shopify.io/#{suffix}") ||
+        @definition.dig("metadata", "annotations", "krane.shopify.io/#{suffix}")
     end
 
-    def annotation_prefix
+    def timeout_annotation_key
       if @definition.dig("metadata", "annotations").key?(TIMEOUT_OVERRIDE_ANNOTATION)
         TIMEOUT_OVERRIDE_ANNOTATION
       else

diff --git a/lib/kubernetes-deploy/kubernetes_resource/custom_resource_definition.rb b/lib/kubernetes-deploy/kubernetes_resource/custom_resource_definition.rb
@@ -4,7 +4,6 @@ class CustomResourceDefinition < KubernetesResource
     TIMEOUT = 2.minutes
     ROLLOUT_CONDITIONS_ANNOTATION_DEPRECATED = "kubernetes-deploy.shopify.io/instance-rollout-conditions"
     ROLLOUT_CONDITIONS_ANNOTATION = "krane.shopify.io/instance-rollout-conditions"
-    TIMEOUT_FOR_INSTANCE_ANNOTATION_DEPRECATED = "kubernetes-deploy.shopify.io/instance-timeout"
     TIMEOUT_FOR_INSTANCE_ANNOTATION = "krane.shopify.io/instance-timeout"
     GLOBAL = true
 
@@ -21,7 +20,7 @@ def timeout_message
     end
 
     def timeout_for_instance
-      timeout = timeout_for_instance_annotation
+      timeout = krane_annotation_value("instance-timeout")
       DurationParser.new(timeout).parse!.to_i
     rescue DurationParser::ParsingError
       nil
@@ -52,36 +51,20 @@ def name
     end
 
     def prunable?
-      prunable = prunable_annotation
+      prunable = krane_annotation_value("prunable")
       prunable == "true"
     end
 
     def predeployed?
-      predeployed = predeployed_annotation
+      predeployed = krane_annotation_value("predeployed")
       predeployed.nil? || predeployed == "true"
     end
 
-    def prunable_annotation
-      if @definition.dig("metadata", "annotations").key?("kubernetes-deploy.shopify.io/prunable")
-        @definition.dig("metadata", "annotations", "kubernetes-deploy.shopify.io/prunable")
-      elsif @definition.dig("metadata", "annotations").key?("krane.shopify.io/prunable")
-        @definition.dig("metadata", "annotations", "krane.shopify.io/prunable")
-      end
-    end
-
-    def predeployed_annotation
-      if @definition.dig("metadata", "annotations").key?("kubernetes-deploy.shopify.io/predeployed")
-        @definition.dig("metadata", "annotations", "kubernetes-deploy.shopify.io/predeployed")
-      elsif @definition.dig("metadata", "annotations").key?("krane.shopify.io/predeployed")
-        @definition.dig("metadata", "annotations", "krane.shopify.io/predeployed")
-      end
-    end
-
     def rollout_conditions
       return @rollout_conditions if defined?(@rollout_conditions)
 
-      @rollout_conditions = if rollout_conditions_annotation
-        RolloutConditions.from_annotation(rollout_conditions_annotation)
+      @rollout_conditions = if krane_annotation_value("instance-rollout-conditions")
+        RolloutConditions.from_annotation(krane_annotation_value("instance-rollout-conditions"))
       end
     rescue RolloutConditionsError
       @rollout_conditions = nil
@@ -92,12 +75,12 @@ def validate_definition(*)
 
       validate_rollout_conditions
     rescue RolloutConditionsError => e
-      @validation_errors << "Annotation #{rollout_conditions_annotation_prefix} on #{name} is invalid: #{e}"
+      @validation_errors << "Annotation #{rollout_conditions_annotation_key} on #{name} is invalid: #{e}"
     end
 
     def validate_rollout_conditions
-      if rollout_conditions_annotation && @rollout_conditions_validated.nil?
-        conditions = RolloutConditions.from_annotation(rollout_conditions_annotation)
+      if krane_annotation_value("instance-rollout-conditions") && @rollout_conditions_validated.nil?
+        conditions = RolloutConditions.from_annotation(krane_annotation_value("instance-rollout-conditions"))
         conditions.validate!
       end
 
@@ -115,22 +98,12 @@ def names_accepted_status
       names_accepted_condition["status"]
     end
 
-    def rollout_conditions_annotation_prefix
+    def rollout_conditions_annotation_key
       if @definition.dig("metadata", "annotations").key?(ROLLOUT_CONDITIONS_ANNOTATION_DEPRECATED)
         ROLLOUT_CONDITIONS_ANNOTATION_DEPRECATED
       elsif @definition.dig("metadata", "annotations").key?(ROLLOUT_CONDITIONS_ANNOTATION)
         ROLLOUT_CONDITIONS_ANNOTATION
       end
     end
-
-    def rollout_conditions_annotation
-      @definition.dig("metadata", "annotations", ROLLOUT_CONDITIONS_ANNOTATION_DEPRECATED) ||
-        @definition.dig("metadata", "annotations", ROLLOUT_CONDITIONS_ANNOTATION)
-    end
-
-    def timeout_for_instance_annotation
-      @definition.dig("metadata", "annotations", TIMEOUT_FOR_INSTANCE_ANNOTATION_DEPRECATED) ||
-        @definition.dig("metadata", "annotations", TIMEOUT_FOR_INSTANCE_ANNOTATION)
-    end
   end
 end
diff --git a/lib/kubernetes-deploy/kubernetes_resource/deployment.rb b/lib/kubernetes-deploy/kubernetes_resource/deployment.rb
@@ -104,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 << "'#{rollout_annotation_prefix}: #{required_rollout}' is incompatible "\
+        @validation_errors << "'#{rollout_annotation_key}: #{required_rollout}' is incompatible "\
         "with strategy '#{strategy}'"
       end
 
@@ -149,14 +149,14 @@ def progress_deadline
     end
 
     def rollout_annotation_err_msg
-      "'#{rollout_annotation_prefix}: #{required_rollout}' is invalid. "\
+      "'#{rollout_annotation_key}: #{required_rollout}' is invalid. "\
         "Acceptable values: #{REQUIRED_ROLLOUT_TYPES.join(', ')}"
     end
 
-    def rollout_annotation_prefix
+    def rollout_annotation_key
       if @definition.dig("metadata", "annotations").key?(REQUIRED_ROLLOUT_ANNOTATION)
         REQUIRED_ROLLOUT_ANNOTATION
-      elsif @definition.dig("metadata", "annotations").key?(REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED)
+      else
         REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED
       end
     end
@@ -210,8 +210,7 @@ def max_unavailable
     end
 
     def required_rollout
-      @definition.dig('metadata', 'annotations', REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED).presence ||
-        @definition.dig('metadata', 'annotations', REQUIRED_ROLLOUT_ANNOTATION).presence || DEFAULT_REQUIRED_ROLLOUT
+      krane_annotation_value("required-rollout") || DEFAULT_REQUIRED_ROLLOUT
     end
 
     def percent?(value)

diff --git a/test/fixtures/pvc/pod.yml b/test/fixtures/pvc/pod.yml
@@ -2,7 +2,7 @@ apiVersion: v1
 kind: Pod
 metadata:
   annotations:
-    kubernetes-deploy.shopify.io/timeout-override: 10s
+    krane.shopify.io/timeout-override: 10s
   name: pvc
   labels:
     type: unmanaged-pod

diff --git a/test/fixtures/pvc/pvc.yml b/test/fixtures/pvc/pvc.yml
@@ -2,7 +2,7 @@ apiVersion: v1
 kind: PersistentVolumeClaim
 metadata:
   annotations:
-    kubernetes-deploy.shopify.io/timeout-override: 10s
+    krane.shopify.io/timeout-override: 10s
   name: with-storage-class
 spec:
   accessModes:

diff --git a/test/integration-serial/serial_deploy_test.rb b/test/integration-serial/serial_deploy_test.rb
@@ -144,6 +144,8 @@ def test_crd_pruning_deprecated
       "Phase 3: Deploying all resources",
       "CustomResourceDefinition/mail.stable.example.io (timeout: 120s)",
       %r{CustomResourceDefinition/mail.stable.example.io\s+Names accepted},
+      "Template warning:",
+      "kubernetes-deploy.shopify.io as a prefix for annotations is deprecated:",
     ])
     assert_deploy_success(deploy_fixtures("crd", subset: %w(mail_cr.yml widgets_cr.yml configmap-data.yml)))
     # Deploy any other non-priority (predeployable) resource to trigger pruning

diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb
@@ -160,7 +160,7 @@ def test_deploy_succeeded_with_max_unavailable_as_percent
 
   def test_deploy_succeeded_raises_with_invalid_rollout_annotation_deprecated
     deploy = build_synced_deployment(
-      template: build_deployment_template_deprecated(rollout: 'bad'),
+      template: build_deployment_template(rollout: 'bad', use_deprecated: true),
       replica_sets: [build_rs_template]
     )
     msg = "'#{KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED}: bad' is "\
@@ -183,7 +183,10 @@ def test_deploy_succeeded_raises_with_invalid_rollout_annotation
   end
 
   def test_validation_fails_with_invalid_rollout_annotation_deprecated
-    deploy = build_synced_deployment(template: build_deployment_template_deprecated(rollout: 'bad'), replica_sets: [])
+    deploy = build_synced_deployment(
+      template: build_deployment_template(rollout: 'bad', use_deprecated: true),
+      replica_sets: []
+    )
     kubectl.expects(:run).with('create', '-f', anything, '--dry-run', '--output=name', anything).returns(
       ["", "super failed", SystemExit.new(1)]
     )
@@ -220,7 +223,10 @@ def test_validation_with_percent_rollout_annotation
   end
 
   def test_validation_with_number_rollout_annotation_deprecated
-    deploy = build_synced_deployment(template: build_deployment_template_deprecated(rollout: '10'), replica_sets: [])
+    deploy = build_synced_deployment(
+      template: build_deployment_template(rollout: '10', use_deprecated: true),
+      replica_sets: []
+    )
     kubectl.expects(:run).with('create', '-f', anything, '--dry-run', '--output=name', anything).returns(
       ["", "super failed", SystemExit.new(1)]
     )
@@ -249,7 +255,7 @@ def test_validation_with_number_rollout_annotation
 
   def test_validation_fails_with_invalid_mix_of_annotation_deprecated
     deploy = build_synced_deployment(
-      template: build_deployment_template_deprecated(rollout: 'maxUnavailable', strategy: 'Recreate'),
+      template: build_deployment_template(rollout: 'maxUnavailable', strategy: 'Recreate', use_deprecated: true),
       replica_sets: [build_rs_template]
     )
     kubectl.expects(:run).with('create', '-f', anything, '--dry-run', '--output=name', anything).returns(
@@ -422,40 +428,18 @@ def test_deploy_timed_out_based_on_progress_deadline_ignores_statuses_for_older_
 
   private
 
-  def build_deployment_template_deprecated(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)
-    if rollout
-      result["metadata"]["annotations"][KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED] = rollout
-    end
-
-    if (spec_override = status["replicas"].presence) # ignores possibility of surge; need a spec_replicas arg for that
-      result["spec"]["replicas"] = spec_override
-    end
-
-    if strategy == "Recreate"
-      result["spec"]["strategy"] = { "type" => strategy }
-    end
+  def build_deployment_template(status: { 'replicas' => 3 }, rollout: nil,
+    strategy: 'rollingUpdate', max_unavailable: nil, use_deprecated: false)
 
-    if strategy.nil?
-      result["spec"]["strategy"] = nil
+    required_rollout_annotation = if use_deprecated
+      KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION_DEPRECATED
+    else
+      KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION
     end
 
-    if max_unavailable
-      result["spec"]["strategy"]["rollingUpdate"] = { "maxUnavailable" => max_unavailable }
-    end
-
-    result
-  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"][KubernetesDeploy::Deployment::REQUIRED_ROLLOUT_ANNOTATION] = rollout if rollout
+    result["metadata"]["annotations"][required_rollout_annotation] = 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