Merge pull request #3021 from zendesk/jylee/kindless

consolidate kind duplication logic

plugins/kubernetes/app/models/kubernetes/release_doc.rb

@@ -56,7 +56,7 @@ def resources
     # Temporary template we run validations on ... so can be cheap / not fully fleshed out
     # and only be the primary since services/configmaps are not very interesting anyway
     def verification_template
-      primary_config = raw_template.detect { |e| Kubernetes::RoleConfigFile::PRIMARY_KINDS.include?(e.fetch(:kind)) }
+      primary_config = raw_template.detect { |e| Kubernetes::RoleConfigFile.primary?(e) }
       Kubernetes::TemplateFiller.new(self, primary_config, index: 0)
     end
 

plugins/kubernetes/app/models/kubernetes/resource.rb

@@ -32,7 +32,7 @@ def prerequisite?
       end
 
       def primary?
-        Kubernetes::RoleConfigFile::PRIMARY_KINDS.include?(@template.fetch(:kind))
+        Kubernetes::RoleConfigFile.primary?(@template)
       end
 
       def deploy

plugins/kubernetes/app/models/kubernetes/role_config_file.rb

@@ -7,10 +7,28 @@ class RoleConfigFile
     attr_reader :path, :elements
 
     DEPLOY_KINDS = ['Deployment', 'DaemonSet', 'StatefulSet'].freeze
-    PRIMARY_KINDS = (DEPLOY_KINDS + ['Job', 'CronJob', 'Pod']).freeze
     SERVICE_KINDS = ['Service'].freeze
     PREREQUISITE = [:metadata, :annotations, :'samson/prerequisite'].freeze
 
+    def self.primary?(resource)
+      templates(resource).any?
+    end
+
+    def self.templates(resource)
+      spec = resource[:spec]
+      if !spec
+        []
+      elsif spec[:containers]
+        [resource]
+      else
+        spec.values_at(*template_keys(resource)).flat_map { |r| templates(r) }
+      end
+    end
+
+    def self.template_keys(resource)
+      (resource[:spec] || {}).keys.grep(/template$/i)
+    end
+
     def initialize(content, path)
       @path = path
 
@@ -30,17 +48,11 @@ def initialize(content, path)
     end
 
     def primary
-      find_by_kind(PRIMARY_KINDS).first
+      @elements.detect { |e| self.class.primary?(e) }
     end
 
     def services
-      find_by_kind(SERVICE_KINDS)
-    end
-
-    private
-
-    def find_by_kind(kinds)
-      @elements.select { |doc| kinds.include?(doc[:kind]) }
+      @elements.select { |doc| SERVICE_KINDS.include?(doc[:kind]) }
     end
   end
 end

plugins/kubernetes/app/models/kubernetes/role_validator.rb

@@ -2,8 +2,11 @@
 module Kubernetes
   class RoleValidator
     VALID_LABEL = /\A[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?\z/ # also used in js ... cannot use /i
-    ALLOWED_DUPLICATE_KINDS = ['ConfigMap', 'Service'].freeze
     NAMESPACELESS_KINDS = ['APIService', 'ClusterRoleBinding', 'ClusterRole', 'CustomResourceDefinition'].freeze
+    IMMUTABLE_NAME_KINDS = ['APIService', 'CustomResourceDefinition', 'ConfigMap'].freeze
+
+    # we either generate multiple names or allow custom names
+    ALLOWED_DUPLICATE_KINDS = ((['Service'] + IMMUTABLE_NAME_KINDS)).freeze
 
     def initialize(elements)
       @elements = elements.compact
@@ -70,10 +73,10 @@ def validate_namespace
     end
 
     # multiple pods in a single role will make validations misbehave (recommend they all have the same role etc)
+    # also template filler won't know how to set images/resources
     def validate_single_primary_kind
-      kinds = map_attributes([:kind])
-      return if kinds.count { |k| RoleConfigFile::PRIMARY_KINDS.include?(k) } < 2
-      @errors << "Only use a maximum of 1 primary kind in a role (#{RoleConfigFile::PRIMARY_KINDS.join(", ")})"
+      return if templates.size <= 1
+      @errors << "Only use a maximum of 1 template with containers, found: #{templates.size}"
     end
 
     # template_filler.rb sets name for everything except for ConfigMaps and Service so we need to make sure
@@ -101,20 +104,13 @@ def validate_numeric_limits
     def validate_project_and_role_consistent
       labels = @elements.flat_map do |resource|
         kind = resource[:kind]
-        allow_cross_match = resource.dig(:metadata, :annotations, :"samson/service_selector_across_roles") == "true"
 
+        allow_cross_match = resource.dig(:metadata, :annotations, :"samson/service_selector_across_roles") == "true"
         label_paths = metadata_paths(resource).map { |p| p + [:labels] } +
-          case kind
-          when 'Service'
-            allow_cross_match ? [] : [[:spec, :selector]]
-          when 'PodDisruptionBudget'
-            [
-              [:spec, :selector, :matchLabels]
-            ]
-          when *RoleConfigFile::DEPLOY_KINDS
-            [
-              [:spec, :selector, :matchLabels],
-            ]
+          if resource.dig(:spec, :selector, :matchLabels)
+            [[:spec, :selector, :matchLabels]]
+          elsif resource.dig(:spec, :selector) && !allow_cross_match
+            [[:spec, :selector]]
           else
             [] # ignore unknown / unsupported types
           end
@@ -178,16 +174,13 @@ def validate_stateful_set_restart_policy
     end
 
     def validate_containers
-      primary_kinds = RoleConfigFile::PRIMARY_KINDS
-      containered = templates.select { |t| primary_kinds.include?(t[:kind]) }
-      containers = map_attributes([:spec, :containers], elements: containered)
-      bad = containers.select { |c| !c.is_a?(Array) || c.empty? }
-      return if bad.empty?
-      @errors << "#{containered.map { |r| r[:kind] }.join("/")} needs at least 1 container"
+      containers = map_attributes([:spec, :containers], elements: templates)
+      return if containers.all? { |c| c.is_a?(Array) && c.any? }
+      @errors << "All templates need spec.containers"
     end
 
     def validate_container_name
-      names = map_attributes([:spec, :containers], elements: templates).compact.flatten(1).map { |c| c[:name] }
+      names = map_attributes([:spec, :containers], elements: templates).flatten(1).map { |c| c[:name] }
       if names.any?(&:nil?)
         @errors << "Containers need a name"
       elsif bad = names.grep_v(VALID_LABEL).presence
@@ -237,8 +230,8 @@ def validate_env_values
 
     def validate_prerequisites
       allowed = ["Job", "Pod"]
-      bad = templates.any? do |t|
-        t.dig(*RoleConfigFile::PREREQUISITE) && !allowed.include?(t[:kind])
+      bad = (@elements + templates).any? do |e|
+        e.dig(*RoleConfigFile::PREREQUISITE) && !allowed.include?(e[:kind])
       end
       @errors << "Only elements with type #{allowed.join(", ")} can be prerequisites." if bad
     end
@@ -259,29 +252,15 @@ def find_stateful_set
       @elements.detect { |t| t[:kind] == "StatefulSet" }
     end
 
-    def templates
-      @elements.map do |e|
-        kind = e[:kind]
-        if kind == "Pod"
-          e
-        else
-          template = e.dig(:spec, :template) || e.dig(:spec, :jobTemplate, :spec, :template) || {}
-          template[:kind] = kind
-          template
-        end
-      end
+    def templates(elements = @elements)
+      elements.flat_map { |e| RoleConfigFile.templates(e) }
     end
 
     def metadata_paths(e)
-      [[:metadata]] +
-        case e[:kind]
-        when "CronJob"
-          [[:spec, :jobTemplate, :metadata], [:spec, :jobTemplate, :spec, :template, :metadata]]
-        when "Job", *RoleConfigFile::DEPLOY_KINDS
-          [[:spec, :template, :metadata]]
-        else
-          []
-        end
+      [[:metadata]] + RoleConfigFile.template_keys(e).flat_map do |k|
+        template = e.dig_fetch(:spec, k)
+        metadata_paths(template).map { |p| [:spec, k] + p }
+      end
     end
 
     def map_attributes(path, elements: @elements)

plugins/kubernetes/app/models/kubernetes/template_filler.rb

@@ -22,20 +22,16 @@ def to_hash(verification: false)
         set_project_labels if template.dig(:metadata, :annotations, :"samson/override_project_label")
         set_deploy_url
 
-        case kind
-        when 'APIService', 'CustomResourceDefinition' # rubocop:disable Lint/EmptyWhen
+        if RoleValidator::IMMUTABLE_NAME_KINDS.include?(kind)
           # names have a fixed pattern so we cannot override them
-        when 'HorizontalPodAutoscaler'
+        elsif kind == 'HorizontalPodAutoscaler'
           set_name
           set_hpa_scale_target_name
-        when 'ConfigMap' # rubocop:disable Lint/EmptyWhen
-          # referenced in other resources so we cannot change the name
-          # NOTE: may cause multiple projects to override each others ConfigMaps if they chose duplicate names
-        when *Kubernetes::RoleConfigFile::SERVICE_KINDS
+        elsif Kubernetes::RoleConfigFile::SERVICE_KINDS.include?(kind)
           set_service_name
           prefix_service_cluster_ip
           set_service_blue_green if blue_green_color
-        when *Kubernetes::RoleConfigFile::PRIMARY_KINDS
+        elsif Kubernetes::RoleConfigFile.primary?(template)
           if kind != 'Pod'
             set_rc_unique_label_key
             set_history_limit
@@ -59,7 +55,7 @@ def to_hash(verification: false)
           set_image_pull_secrets
           set_resource_blue_green if blue_green_color
           set_init_containers
-        when 'PodDisruptionBudget'
+        elsif kind == 'PodDisruptionBudget'
           set_name
           set_match_labels_blue_green if blue_green_color
         else
@@ -104,9 +100,7 @@ def samson_container_config(container, key)
     end
 
     def set_deploy_url
-      templates = [template]
-      templates << pod_template if Kubernetes::RoleConfigFile::PRIMARY_KINDS.include?(template[:kind])
-      templates.each do |t|
+      [template, pod_template].compact.each do |t|
         annotations = (t[:metadata][:annotations] ||= {})
         annotations[:"samson/deploy_url"] = @doc.kubernetes_release.deploy&.url
       end
@@ -214,11 +208,7 @@ def pod_annotations
     end
 
     def pod_template
-      case template[:kind]
-      when 'Pod' then template
-      when 'CronJob' then template.dig_fetch(:spec, :jobTemplate, :spec, :template)
-      else template.dig_fetch(:spec, :template)
-      end
+      @pod_template ||= RoleConfigFile.templates(template).first
     end
 
     def secret_annotations

plugins/kubernetes/test/models/kubernetes/resource_test.rb

@@ -295,7 +295,7 @@ def assert_create_and_delete_requests(**args, &block)
       end
 
       it "is not primary when it is a secondary resource" do
-        template[:kind] = "Service"
+        template[:spec][:template][:spec].delete :containers
         refute resource.primary?
       end
     end

plugins/kubernetes/test/models/kubernetes/role_config_file_test.rb

@@ -54,4 +54,30 @@
       config_file.services.must_equal []
     end
   end
+
+  describe ".primary?" do
+    it "is primary when template has containers" do
+      Kubernetes::RoleConfigFile.primary?(YAML.safe_load(content).with_indifferent_access).must_equal true
+    end
+
+    it "is not primary when template has no containers" do
+      Kubernetes::RoleConfigFile.primary?({}).must_equal false
+    end
+  end
+
+  describe ".templates" do
+    it "is empty when there is no spec" do
+      Kubernetes::RoleConfigFile.templates({}).must_equal []
+    end
+
+    it "finds simple template" do
+      Kubernetes::RoleConfigFile.templates(spec: {containers: 'foo'}).must_equal [{spec: {containers: 'foo'}}]
+    end
+
+    it "finds nested template" do
+      Kubernetes::RoleConfigFile.templates(spec: {fooTemplate: {spec: {containers: 'foo'}}}).must_equal(
+        [{spec: {containers: 'foo'}}]
+      )
+    end
+  end
 end

plugins/kubernetes/test/models/kubernetes/role_test.rb

@@ -274,7 +274,7 @@ def commit
     end
 
     it "raises when a role is invalid so the deploy is cancelled" do
-      assert config_content_yml.sub!('Service', 'Pod')
+      assert config_content_yml.sub!('project: some-project', 'project: project-invalid')
       write_config role.config_file, config_content_yml
 
       assert_raises Samson::Hooks::UserError do

plugins/kubernetes/test/models/kubernetes/role_validator_test.rb

@@ -81,9 +81,9 @@
     end
 
     it "does not allow multiple deployables" do
-      role[1][:kind] = "Pod"
+      role[1][:spec] = {containers: []}
       errors.must_include(
-        "Only use a maximum of 1 primary kind in a role (Deployment, DaemonSet, StatefulSet, Job, CronJob, Pod)"
+        "Only use a maximum of 1 template with containers, found: 2"
       )
     end
 
@@ -177,6 +177,7 @@
 
     it "allows duplicate ConfigMaps" do
       role[0][:kind] = "ConfigMap"
+      role[0][:spec][:template][:spec].delete :containers
       role << role[0].dup
       errors.must_be_nil
     end
@@ -186,9 +187,9 @@
       errors.must_include "Numeric cpu resources are not supported"
     end
 
-    it "reports missing containers" do
+    it "does not fail on missing containers" do
       role.first[:spec][:template][:spec].delete(:containers)
-      errors.must_include "Deployment needs at least 1 container"
+      errors.must_be_nil
     end
 
     it "ignores unknown types" do
@@ -258,7 +259,7 @@
     it "fails when there are duplicate kinds" do
       role[0][:kind] = "foo"
       role[1][:kind] = "foo"
-      errors.must_include "Only use a maximum of 1 of each kind in a role (except ConfigMap and Service)"
+      errors.to_s.must_include "Only use a maximum of 1 of each kind in a role"
     end
 
     describe "#validate_team_labels" do
@@ -302,7 +303,7 @@
         role.pop
         role.first[:kind] = "Job"
         role.first[:spec][:template][:spec][:restartPolicy] = "Never"
-        role.first[:spec][:template][:metadata][:annotations] = {"samson/prerequisite": 'true'}
+        role.first[:metadata][:annotations] = {"samson/prerequisite": 'true'}
       end
 
       it "does not report valid prerequisites" do
@@ -332,7 +333,7 @@
 
       it "fails without containers" do
         role[0][:spec][:containers].clear
-        errors.must_equal ["Pod needs at least 1 container"]
+        errors.must_equal ["All templates need spec.containers"]
       end
 
       it "fails without name" do

plugins/kubernetes/test/models/kubernetes/template_filler_test.rb

@@ -138,8 +138,9 @@ def add_init_container(container)
         must_equal doc.kubernetes_release.deploy.url
     end
 
-    it "sets name for unknown kinds" do
+    it "sets name for unknown non-primary kinds" do
       raw_template[:kind] = "foobar"
+      raw_template[:spec][:template][:spec].delete(:containers)
       template.to_hash[:metadata][:name].must_equal "test-app-server"
     end
 
@@ -171,6 +172,8 @@ def add_init_container(container)
 
       it "overrides project label in pod" do
         raw_template.replace(raw_template.dig(:spec, :template).merge(raw_template.slice(:metadata)))
+        raw_template[:kind] = "Pod"
+        doc.replica_target = 1
         raw_template[:spec].delete(:template)
         raw_template[:spec].delete(:selector)
         labels.must_equal ["foo", nil, nil, nil]
@@ -773,8 +776,12 @@ def secret_annotations(hash)
     end
 
     describe "PodDisruptionBudget" do
-      it "modified name" do
+      before do
         raw_template[:kind] = 'PodDisruptionBudget'
+        raw_template[:spec][:template][:spec].delete(:containers)
+      end
+
+      it "modified name" do
         hash = template.to_hash
         hash.dig_fetch(:metadata, :name).must_equal 'test-app-server'
         refute hash.dig(:spec, :selector, :matchLabels).key?(:blue_green)
@@ -803,6 +810,7 @@ def secret_annotations(hash)
 
       it "modified budgets so we do not get errors when 2 budgets match the same pod" do
         raw_template[:kind] = 'PodDisruptionBudget'
+        raw_template[:spec].delete(:template)
         hash = template.to_hash
         hash.dig_fetch(:metadata, :name).must_equal 'test-app-server-green'
         hash.dig_fetch(:spec, :selector, :matchLabels, :blue_green).must_equal 'green'