[Bug Fix] Grouping Cluster Resources by group and Kind

lib/krane/deploy_task.rb

@@ -74,7 +74,7 @@ def predeploy_sequence
         Pod
       ).map { |r| [r, default_group] }
 
-      crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.kind, { group: cr.group }] }
+      crs = cluster_resource_discoverer.crds.select(&:predeployed?).map { |cr| [cr.group_kind, { group: cr.group }] }
       Hash[before_crs + crs + after_crs]
     end
 
@@ -238,9 +238,10 @@ def secrets_from_ejson
     def discover_resources
       @logger.info("Discovering resources:")
       resources = []
-      crds_by_kind = cluster_resource_discoverer.crds.group_by(&:kind)
+      crds_by_group_kind = cluster_resource_discoverer.crds.group_by(&:group_kind)
       @template_sets.with_resource_definitions(current_sha: @current_sha, bindings: @bindings) do |r_def|
-        crd = crds_by_kind[r_def["kind"]]&.first
+        group, kind = group_kind_for_r_def(r_def)
+        crd = crds_by_group_kind["#{kind}.#{group}"]&.first
         r = KubernetesResource.build(namespace: @namespace, context: @context, logger: @logger, definition: r_def,
           statsd_tags: @namespace_tags, crd: crd, global_names: @task_config.global_kinds)
         resources << r
@@ -389,5 +390,12 @@ def with_retries(limit)
         retried += 1
       end
     end
+
+    def group_kind_for_r_def(r_def)
+      grouping, version = r_def.dig("apiVersion").split("/")
+      group = version ? grouping : "core"
+      kind = r_def["kind"].to_s
+      [group, kind]
+    end
   end
 end

lib/krane/kubernetes_resource/custom_resource.rb

@@ -59,7 +59,7 @@ def status
     end
 
     def type
-      kind
+      group_kind
     end
 
     def validate_definition(*, **)
@@ -77,7 +77,11 @@ def validate_definition(*, **)
     private
 
     def kind
-      @definition["kind"]
+      @crd.kind
+    end
+
+    def group_kind
+      @crd.group_kind
     end
 
     def rollout_conditions

lib/krane/kubernetes_resource/custom_resource_definition.rb

@@ -52,6 +52,10 @@ def group
       @definition.dig("spec", "group")
     end
 
+    def group_kind
+      "#{kind}.#{group}"
+    end
+
     def prunable?
       prunable = krane_annotation_value("prunable")
       prunable == "true"

test/fixtures/crd/for_group_kind_test.yml

@@ -0,0 +1,71 @@
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: same-kinds.with-rollout-conditions.example.io
+  labels:
+    app: krane
+  annotations:
+    krane.shopify.io/instance-rollout-conditions: "true"
+spec:
+  group: with-rollout-conditions.example.io
+  names:
+    kind: SameKind
+    listKind: SameKindList
+    plural: same-kinds
+    singular: same-kind
+  scope: Namespaced
+  versions:
+    - name: v1
+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:
+          type: object
+          properties:
+            spec:
+              type: object
+            status:
+              type: object
+              properties:
+                observedGeneration:
+                  type: integer
+                conditions:
+                  type: array
+                  items:
+                    type: object
+                    properties:
+                      type:
+                        type: string
+                      reason:
+                        type: string
+                      message:
+                        type: string
+                      status:
+                        type: string
+
+---
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: same-kinds.no-rollout-conditions.example.io
+  labels:
+    app: krane
+spec:
+  group: no-rollout-conditions.example.io
+  names:
+    kind: SameKind
+    listKind: SameKindList
+    plural: same-kinds
+    singular: same-kind
+  scope: Namespaced
+  versions:
+    - name: v1
+      served: true
+      storage: true
+      schema:
+        openAPIV3Schema:
+          type: object
+          properties:
+            spec:
+              type: object

test/fixtures/crd/for_group_kind_test_cr.yml

@@ -0,0 +1,10 @@
+---
+apiVersion: "with-rollout-conditions.example.io/v1"
+kind: SameKind
+metadata:
+  name: monitored
+---
+apiVersion: "no-rollout-conditions.example.io/v1"
+kind: SameKind
+metadata:
+  name: unmonitored

test/helpers/fixture_set.rb

@@ -30,8 +30,8 @@ def app_name
     def refute_resource_exists(type, name)
       client = if %w(daemonset deployment replicaset statefulset).include?(type)
         apps_v1_kubeclient
-     elsif %w(ingress networkpolicy).include?(type)
-       networking_v1_kubeclient
+      elsif %w(ingress networkpolicy).include?(type)
+        networking_v1_kubeclient
       else
         kubeclient
       end
@@ -96,12 +96,16 @@ def assert_pvc_status(pvc_name, status)
     end
 
     def assert_ingress_up(ing_name)
-      ing = networking_v1_kubeclient.get_ingresses(namespace: namespace, label_selector: "name=#{ing_name},app=#{app_name}")
+      ing = networking_v1_kubeclient.get_ingresses(
+        namespace: namespace, label_selector: "name=#{ing_name},app=#{app_name}"
+      )
       assert_equal(1, ing.size, "Expected 1 #{ing_name} ingress, got #{ing.size}")
     end
 
     def assert_configmap_present(cm_name, expected_data)
-      configmaps = kubeclient.get_config_maps(namespace: namespace, label_selector: "name=#{cm_name},app=#{app_name}")
+      configmaps = kubeclient.get_config_maps(
+        namespace: namespace, label_selector: "name=#{cm_name},app=#{app_name}"
+      )
       assert_equal(1, configmaps.size, "Expected 1 configmap, got #{configmaps.size}")
       assert_equal(expected_data, configmaps.first["data"].to_h)
     end

test/integration-serial/serial_deploy_test.rb

@@ -265,9 +265,9 @@ def test_custom_resources_predeployed
     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"
+    mail_cr_id = "#{add_unique_prefix_for_test('Mail')}.stable.example.io/my-first-mail"
+    thing_cr_id = "#{add_unique_prefix_for_test('Thing')}.stable.example.io/my-first-thing"
+    widget_cr_id = "#{add_unique_prefix_for_test('Widget')}.stable.example.io/my-first-widget"
     assert_logs_match_all([
       /Phase 3: Predeploying priority resources/,
       /Successfully deployed in \d.\ds: #{mail_cr_id}/,
@@ -291,11 +291,11 @@ def test_cr_deploys_without_rollout_conditions_when_none_present
     end
 
     assert_deploy_success(result)
-    prefixed_kind = add_unique_prefix_for_test("Widget")
+    prefixed_kind = "#{add_unique_prefix_for_test('Widget')}.stable.example.io"
     assert_logs_match_all([
       "Don't know how to monitor resources of type #{prefixed_kind}.",
       "Assuming #{prefixed_kind}/my-first-widget deployed successfully.",
-      %r{Widget/my-first-widget\s+Exists},
+      %r{#{prefixed_kind}/my-first-widget\s+Exists},
     ])
   end
 
@@ -320,10 +320,11 @@ def test_cr_success_with_default_rollout_conditions
       cr.merge!(success_conditions)
       cr["kind"] = add_unique_prefix_for_test(cr["kind"])
     end
+    prefixed_name = "#{add_unique_prefix_for_test('Parameterized')}.stable.example.io"
     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},
+      %r{Successfully deployed in .*: #{prefixed_name}\/with-default-params},
+      %r{Parameterized.stable.example.io/with-default-params\s+Healthy},
     ])
   end
 
@@ -374,12 +375,47 @@ def test_cr_failure_with_default_rollout_conditions
     assert_deploy_failure(result)
 
     assert_logs_match_all([
-      "Parameterized/with-default-params: FAILED",
+      "Parameterized.stable.example.io/with-default-params: FAILED",
       "custom resource rollout failed",
       "Final status: Unhealthy",
     ], in_order: true)
   end
 
+  # # Make 2 CRDs of same kind, different group. We expect the appropriate one to be monitored and
+  # # the other to be unmonitored for rollout conditions
+  def test_cr_references_parent_crd_by_group_kind
+    assert_deploy_success(deploy_global_fixtures("crd", subset: %(for_group_kind_test.yml)))
+    success_conditions = {
+      "status" => {
+        "observedGeneration" => 1,
+        "conditions" => [
+          {
+            "type" => "Ready",
+            "reason" => "test",
+            "message" => "test",
+            "status" => "True",
+          },
+        ],
+      },
+    }
+
+    result = deploy_fixtures("crd", subset: %(for_group_kind_test_cr.yml)) do |resource|
+      monitored = resource["for_group_kind_test_cr.yml"]["SameKind"][0]
+      monitored["kind"] = add_unique_prefix_for_test(monitored["kind"])
+      monitored.merge!(success_conditions)
+
+      unmonitored = resource["for_group_kind_test_cr.yml"]["SameKind"][1]
+      unmonitored["kind"] = add_unique_prefix_for_test(unmonitored["kind"])
+    end
+    assert_deploy_success(result)
+    prefixed_kind = add_unique_prefix_for_test("SameKind")
+    assert_logs_match_all([
+      /Successfully deployed in .*: /,
+      %r{#{prefixed_kind}.no-rollout-conditions.example.io/unmonitored Exists},
+      %r{#{prefixed_kind}.with-rollout-conditions.example.io/monitored Healthy},
+    ])
+  end
+
   def test_cr_success_with_arbitrary_rollout_conditions
     assert_deploy_success(deploy_global_fixtures("crd", subset: %(with_custom_conditions.yml)))
 
@@ -398,8 +434,9 @@ def test_cr_success_with_arbitrary_rollout_conditions
       cr.merge!(success_conditions)
     end
     assert_deploy_success(result)
+    prefixed_name = "#{add_unique_prefix_for_test('Customized')}.stable.example.io"
     assert_logs_match_all([
-      %r{Successfully deployed in .*: #{add_unique_prefix_for_test("Customized")}\/with-customized-params},
+      %r{Successfully deployed in .*: #{prefixed_name}\/with-customized-params},
     ])
   end
 
@@ -450,7 +487,7 @@ def test_deploying_crs_with_invalid_crd_conditions_fails
       end
     end
     assert_deploy_failure(result)
-    prefixed_name = add_unique_prefix_for_test("Customized-with-customized-params")
+    prefixed_name = add_unique_prefix_for_test('Customized.stable.example.io-with-customized-params').to_s
     assert_logs_match_all([
       /Invalid template: #{prefixed_name}/,
       /Rollout conditions are not valid JSON/,

test/integration/krane_deploy_test.rb

@@ -40,20 +40,22 @@ def test_deploy_fails_with_empty_yaml
     assert_deploy_failure(deploy_raw_fixtures("empty-resources", subset: %w[empty1.yml empty2.yml]))
 
     assert_logs_match_all([
-                            "All required parameters and files are present",
-                            "Result: FAILURE",
-                            "No deployable resources were found!",
-                          ], in_order: true)
+      "All required parameters and files are present",
+      "Result: FAILURE",
+      "No deployable resources were found!",
+    ], in_order: true)
   end
 
   def test_deploy_fails_with_empty_erb
-    assert_deploy_failure(deploy_raw_fixtures("empty-resources", subset: %w[empty3.yml.erb empty4.yml.erb], render_erb: true))
+    assert_deploy_failure(
+      deploy_raw_fixtures("empty-resources", subset: %w[empty3.yml.erb empty4.yml.erb], render_erb: true)
+    )
 
     assert_logs_match_all([
-                            "All required parameters and files are present",
-                            "Result: FAILURE",
-                            "No deployable resources were found!",
-                          ], in_order: true)
+      "All required parameters and files are present",
+      "Result: FAILURE",
+      "No deployable resources were found!",
+    ], in_order: true)
   end
 
   def test_service_account_predeployed_before_unmanaged_pod
@@ -135,7 +137,7 @@ def test_pruning_works
       prune_matcher("role", "rbac.authorization.k8s.io", "role"),
       prune_matcher("rolebinding", "rbac.authorization.k8s.io", "role-binding"),
       prune_matcher("persistentvolumeclaim", "", "hello-pv-claim"),
-      prune_matcher("ingress", %w(networking.k8s.io extensions), "web")
+      prune_matcher("ingress", %w(networking.k8s.io extensions), "web"),
     ] # not necessarily listed in this order
     expected_msgs = [/Pruned 2[013] resources and successfully deployed 6 resources/]
     expected_pruned.map do |resource|

test/unit/krane/kubernetes_resource/custom_resource_definition_test.rb

@@ -96,6 +96,7 @@ def test_cr_instance_fails_validation_when_rollout_conditions_for_crd_invalid
       logger: @logger, statsd_tags: @statsd_tags, crd: crd,
       definition: {
         "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
         "metadata" => { "name" => "test" },
       })
     cr.validate_definition(kubectl: kubectl)
@@ -129,6 +130,7 @@ def test_cr_instance_valid_when_rollout_conditions_for_crd_valid
       logger: @logger, statsd_tags: [], crd: crd,
       definition: {
         "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
         "metadata" => { "name" => "test" },
       })
     cr.validate_definition(kubectl: kubectl)
@@ -154,7 +156,11 @@ def test_instance_timeout_annotation
     ))
     cr = Krane::KubernetesResource.build(namespace: "test", context: "test",
       logger: @logger, statsd_tags: [], crd: crd,
-      definition: { "kind" => "UnitTest", "metadata" => { "name" => "test" } })
+      definition: {
+        "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
+        "metadata" => { "name" => "test" },
+      })
     assert_equal(cr.timeout, 60)
   end
 
@@ -171,6 +177,7 @@ def test_instance_timeout_messages_with_rollout_conditions
       logger: @logger, statsd_tags: [], crd: crd,
       definition: {
         "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
         "metadata" => {
           "name" => "test",
         },
@@ -195,6 +202,7 @@ def test_instance_timeout_messages_without_rollout_conditions
       logger: @logger, statsd_tags: [], crd: crd,
       definition: {
         "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
         "metadata" => {
           "name" => "test",
         },

test/unit/resource_cache_test.rb

@@ -125,12 +125,13 @@ def test_warming_the_resource_cache_works_for_custom_resources
       logger: @logger, statsd_tags: [], crd: crd,
       definition: {
         "kind" => "UnitTest",
+        "apiVersion" => "stable.example.io/v1",
         "metadata" => {
           "name" => "test",
         },
       })
 
-    stub_kind_get("UnitTest", times: 1)
+    stub_kind_get("UnitTest.stable.example.io", times: 1)
     stub_kind_get("CustomResourceDefinition", times: 1)
     resources = [cr, crd]
     @cache.prewarm(resources)