Fix issue with missing knative cluster role binding, and cleanup tests

6 files changed, 124 insertions, 44 deletions

diff --git a/app/models/clusters/applications/knative.rb b/app/models/clusters/applications/knative.rb
index d5cf3a4bb07..22549fd539d 100644
--- a/app/models/clusters/applications/knative.rb
+++ b/app/models/clusters/applications/knative.rb
@@ -18,21 +18,18 @@ module Clusters
       include ::Clusters::Concerns::ApplicationData
 
       default_value_for :version, VERSION
-      default_value_for :hostname, nil
+
+      validates :hostname, presence: true
 
       def chart
         'knative/knative'
       end
 
       def values
-        content_values.to_yaml
+        { domain: hostname }.to_yaml
       end
 
       def install_command
-        if hostname.nil?
-          raise 'Hostname is required'
-        end
-
         Gitlab::Kubernetes::Helm::InstallCommand.new(
           name: name,
           version: VERSION,
@@ -40,25 +37,60 @@ module Clusters
           chart: chart,
           files: files,
           repository: REPOSITORY,
-          script: install_script
+          preinstall: install_script,
+          postinstall: setup_knative_role
         )
       end
 
+      private
+
       def install_script
-        ['/usr/bin/kubectl', 'apply', '-f', ISTIO_CRDS]
+        ["/usr/bin/kubectl apply -f #{ISTIO_CRDS} >/dev/null"]
       end
 
-      private
-
-      def content_values
-        YAML.load_file(chart_values_file).deep_merge!(knative_configs)
+      def setup_knative_role
+        if !cluster.kubernetes_namespace.nil?
+          [
+            "echo \'#{create_rolebinding.to_yaml}\' > /tmp/rolebinding.yaml\n",
+            "/usr/bin/kubectl apply -f /tmp/rolebinding.yaml > /dev/null"
+          ]
+        else
+          nil
+        end
       end
 
-      def knative_configs
+      def create_rolebinding
         {
-          "domain" => hostname
+          "apiVersion" => "rbac.authorization.k8s.io/v1",
+          "kind" => "ClusterRoleBinding",
+          "metadata" => {
+            "name" => create_role_binding_name,
+            "namespace" => namespace
+          },
+          "roleRef" => {
+            "apiGroup" => "rbac.authorization.k8s.io",
+            "kind" => "ClusterRole",
+            "name" => "knative-serving-admin"
+          },
+          "subjects" => role_subject
         }
       end
+
+      def create_role_binding_name
+        "#{namespace}-knative-binding"
+      end
+
+      def service_account_name
+        cluster.kubernetes_namespace.service_account_name
+      end
+
+      def role_subject
+        [{ "kind" => 'ServiceAccount', "name" => service_account_name, "namespace" => namespace }]
+      end
+
+      def namespace
+        cluster.kubernetes_namespace.namespace
+      end
     end
   end
 end
diff --git a/lib/gitlab/kubernetes/helm/install_command.rb b/lib/gitlab/kubernetes/helm/install_command.rb
index 09a4e494b64..28173b65cb6 100644
--- a/lib/gitlab/kubernetes/helm/install_command.rb
+++ b/lib/gitlab/kubernetes/helm/install_command.rb
@@ -4,16 +4,17 @@ module Gitlab
       class InstallCommand
         include BaseCommand
 
-        attr_reader :name, :files, :chart, :version, :repository, :script
+        attr_reader :name, :files, :chart, :version, :repository, :preinstall, :postinstall
 
-        def initialize(name:, chart:, files:, rbac:, version: nil, repository: nil, script: nil)
+        def initialize(name:, chart:, files:, rbac:, version: nil, repository: nil, preinstall: nil, postinstall: nil)
           @name = name
           @chart = chart
           @version = version
           @rbac = rbac
           @files = files
           @repository = repository
-          @script = script
+          @preinstall = preinstall
+          @postinstall = postinstall
         end
 
         def generate_script
@@ -21,8 +22,9 @@ module Gitlab
             init_command,
             repository_command,
             repository_update_command,
-            script_command,
-            install_command
+            preinstall_command,
+            install_command,
+            postinstall_command
           ].compact.join("\n")
         end
 
@@ -50,9 +52,15 @@ module Gitlab
           command.shelljoin + " >/dev/null\n"
         end
 
-        def script_command
-          unless script.nil?
-            script.shelljoin + " >/dev/null\n"
+        def preinstall_command
+          unless preinstall.nil?
+            preinstall.join("\n")
+          end
+        end
+
+        def postinstall_command
+          unless postinstall.nil?
+            postinstall.join("\n")
           end
         end
 
diff --git a/spec/factories/clusters/applications/helm.rb b/spec/factories/clusters/applications/helm.rb
index 3fe088d47cc..ff65c76cf26 100644
--- a/spec/factories/clusters/applications/helm.rb
+++ b/spec/factories/clusters/applications/helm.rb
@@ -58,6 +58,7 @@ FactoryBot.define do
     end
 
     factory :clusters_applications_knative, class: Clusters::Applications::Knative do
+      hostname 'example.com'
       cluster factory: %i(cluster with_installed_helm provided_by_gcp)
     end
 
diff --git a/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb b/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb
index bbe7cbe05c5..ed879350004 100644
--- a/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb
+++ b/spec/lib/gitlab/kubernetes/helm/install_command_spec.rb
@@ -5,6 +5,8 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do
   let(:repository) { 'https://repository.example.com' }
   let(:rbac) { false }
   let(:version) { '1.2.3' }
+  let(:preinstall) { nil }
+  let(:postinstall) { nil }
 
   let(:install_command) do
     described_class.new(
@@ -13,7 +15,9 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do
       rbac: rbac,
       files: files,
       version: version,
-      repository: repository
+      repository: repository,
+      preinstall: preinstall,
+      postinstall: postinstall
     )
   end
 
@@ -101,6 +105,53 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do
     end
   end
 
+  context 'when there is a pre-install script' do
+    let(:preinstall) { ['/bin/date', '/bin/true'] }
+
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+        helm init --client-only >/dev/null
+        helm repo add app-name https://repository.example.com
+        helm repo update >/dev/null
+        #{helm_install_command}
+        EOS
+      end
+
+      let(:helm_install_command) do
+        <<~EOS.strip
+        /bin/date
+        /bin/true
+        helm install chart-name --name app-name --tls --tls-ca-cert /data/helm/app-name/config/ca.pem --tls-cert /data/helm/app-name/config/cert.pem --tls-key /data/helm/app-name/config/key.pem --version 1.2.3 --namespace gitlab-managed-apps -f /data/helm/app-name/config/values.yaml >/dev/null
+        EOS
+      end
+    end
+  end
+
+  context 'when there is a post-install script' do
+    let(:postinstall) { ['/bin/date', "/bin/false\n"] }
+
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+        helm init --client-only >/dev/null
+        helm repo add app-name https://repository.example.com
+        helm repo update >/dev/null
+        #{helm_install_command}
+        EOS
+      end
+
+      let(:helm_install_command) do
+        <<~EOS.strip
+        helm install chart-name --name app-name --tls --tls-ca-cert /data/helm/app-name/config/ca.pem --tls-cert /data/helm/app-name/config/cert.pem --tls-key /data/helm/app-name/config/key.pem --version 1.2.3 --namespace gitlab-managed-apps -f /data/helm/app-name/config/values.yaml >/dev/null
+
+        /bin/date
+        /bin/false
+        EOS
+      end
+    end
+  end
+
   context 'when there is no ca.pem file' do
     let(:files) { { 'file.txt': 'some content' } }
 
diff --git a/spec/models/clusters/applications/knative_spec.rb b/spec/models/clusters/applications/knative_spec.rb
index 6d72e3fba16..7849e29d546 100644
--- a/spec/models/clusters/applications/knative_spec.rb
+++ b/spec/models/clusters/applications/knative_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe Clusters::Applications::Knative do
-  let(:knative) { create(:clusters_applications_knative, hostname: 'example.com') }
+  let(:knative) { create(:clusters_applications_knative) }
 
   include_examples 'cluster application core specs', :clusters_applications_knative
   include_examples 'cluster application status specs', :clusters_applications_knative
@@ -47,7 +47,9 @@ describe Clusters::Applications::Knative do
   describe '#install_command' do
     subject { knative.install_command }
 
-    it { is_expected.to be_an_instance_of(Gitlab::Kubernetes::Helm::InstallCommand) }
+    it 'should be an instance of Helm::InstallCommand' do
+      expect(subject).to be_an_instance_of(Gitlab::Kubernetes::Helm::InstallCommand)
+    end
 
     it 'should be initialized with knative arguments' do
       expect(subject.name).to eq('knative')
@@ -55,14 +57,6 @@ describe Clusters::Applications::Knative do
       expect(subject.version).to eq('0.1.3')
       expect(subject.files).to eq(knative.files)
     end
-
-    context 'application failed to install previously' do
-      let(:knative) { create(:clusters_applications_knative, :errored, version: 'knative', hostname: 'example.com') }
-
-      it 'should be initialized with the locked version' do
-        expect(subject.version).to eq('0.1.3')
-      end
-    end
   end
 
   describe '#files' do
@@ -71,7 +65,7 @@ describe Clusters::Applications::Knative do
 
     subject { application.files }
 
-    it 'should include knative valid keys in values' do
+    it 'should include knative specific keys in the values.yaml file' do
       expect(values).to include('domain')
     end
 
@@ -80,20 +74,15 @@ describe Clusters::Applications::Knative do
         application.cluster.application_helm.ca_cert = nil
       end
 
-      it 'should not include cert files' do
-        expect(subject[:'ca.pem']).not_to be_present
-        expect(subject[:'cert.pem']).not_to be_present
-        expect(subject[:'key.pem']).not_to be_present
+      it 'should not include cert files when there is no ca_cert entry' do
+        expect(subject).not_to include(:'ca.pem', :'cert.pem', :'key.pem')
       end
     end
 
-    it 'should include cert files' do
-      expect(subject[:'ca.pem']).to be_present
+    it 'should include cert files when there is a ca_cert entry' do
+      expect(subject).to include(:'ca.pem', :'cert.pem', :'key.pem')
       expect(subject[:'ca.pem']).to eq(application.cluster.application_helm.ca_cert)
 
-      expect(subject[:'cert.pem']).to be_present
-      expect(subject[:'key.pem']).to be_present
-
       cert = OpenSSL::X509::Certificate.new(subject[:'cert.pem'])
       expect(cert.not_after).to be < 60.minutes.from_now
     end
diff --git a/vendor/knative/values.yaml b/vendor/knative/values.yaml
deleted file mode 100644
index b3472660fb0..00000000000
--- a/vendor/knative/values.yaml
+++ /dev/null
@@ -1 +0,0 @@
-domain: example.com