Merge branch 'sk/211509-add-k8-object-validation' into 'master'

Add validation on params for Kubernetes and Elasticsearch PodLogs

See merge request gitlab-org/gitlab!30165

app/services/pod_logs/base_service.rb

@@ -58,6 +58,9 @@ module PodLogs
       result[:pod_name] = params['pod_name'].presence
       result[:container_name] = params['container_name'].presence
 
+      return error(_('Invalid pod_name')) if result[:pod_name] && !result[:pod_name].is_a?(String)
+      return error(_('Invalid container_name')) if result[:container_name] && !result[:container_name].is_a?(String)
+
       success(result)
     end
 

app/services/pod_logs/elasticsearch_service.rb

@@ -53,12 +53,16 @@ module PodLogs
     def check_search(result)
       result[:search] = params['search'] if params.key?('search')
 
+      return error(_('Invalid search parameter')) if result[:search] && !result[:search].is_a?(String)
+
       success(result)
     end
 
     def check_cursor(result)
       result[:cursor] = params['cursor'] if params.key?('cursor')
 
+      return error(_('Invalid cursor parameter')) if result[:cursor] && !result[:cursor].is_a?(String)
+
       success(result)
     end
 

app/services/pod_logs/kubernetes_service.rb

@@ -47,6 +47,10 @@ module PodLogs
           ' chars' % { max_length: K8S_NAME_MAX_LENGTH }))
       end
 
+      unless result[:pod_name] =~ Gitlab::Regex.kubernetes_dns_subdomain_regex
+        return error(_('pod_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character'))
+      end
+
       unless result[:pods].include?(result[:pod_name])
         return error(_('Pod does not exist'))
       end
@@ -70,6 +74,10 @@ module PodLogs
           ' %{max_length} chars' % { max_length: K8S_NAME_MAX_LENGTH }))
       end
 
+      unless result[:container_name] =~ Gitlab::Regex.kubernetes_dns_subdomain_regex
+        return error(_('container_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character'))
+      end
+
       unless container_names.include?(result[:container_name])
         return error(_('Container does not exist'))
       end

changelogs/unreleased/sk-add-validation-on-params-for-podlogs-service.yml

+---
+title: Return validation errors for invalid pod name or container name when viewing pod logs
+merge_request: 30165
+author: Sashi Kumar
+type: changed

lib/gitlab/regex.rb

@@ -79,6 +79,12 @@ module Gitlab
       "Must start with a letter, and cannot end with '-'"
     end
 
+    # Pod name adheres to DNS Subdomain Names(RFC 1123) naming convention
+    # https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names
+    def kubernetes_dns_subdomain_regex
+      /\A[a-z0-9]([a-z0-9\-\.]*[a-z0-9])?\z/
+    end
+
     def environment_slug_regex
       @environment_slug_regex ||= /\A[a-z]([a-z0-9-]*[a-z0-9])?\z/.freeze
     end

locale/gitlab.pot

@@ -11521,6 +11521,12 @@ msgstr ""
 msgid "Invalid URL"
 msgstr ""
 
+msgid "Invalid container_name"
+msgstr ""
+
+msgid "Invalid cursor parameter"
+msgstr ""
+
 msgid "Invalid cursor value provided"
 msgstr ""
 
@@ -11557,12 +11563,18 @@ msgstr ""
 msgid "Invalid pin code"
 msgstr ""
 
+msgid "Invalid pod_name"
+msgstr ""
+
 msgid "Invalid query"
 msgstr ""
 
 msgid "Invalid repository path"
 msgstr ""
 
+msgid "Invalid search parameter"
+msgstr ""
+
 msgid "Invalid server response"
 msgstr ""
 
@@ -24979,6 +24991,9 @@ msgstr ""
 msgid "connecting"
 msgstr ""
 
+msgid "container_name can contain only lowercase letters, digits, '-', and '.' and must start and end with an alphanumeric character"
+msgstr ""
+
 msgid "container_name cannot be larger than %{max_length} chars"
 msgstr ""
 
@@ -25686,6 +25701,9 @@ msgstr ""
 msgid "pipeline"
 msgstr ""
 
+msgid "pod_name can contain only lowercase letters, digits, '-', and '.' and must start and end with an alphanumeric character"
+msgstr ""
+
 msgid "pod_name cannot be larger than %{max_length} chars"
 msgstr ""
 

spec/lib/gitlab/regex_spec.rb

@@ -130,4 +130,37 @@ describe Gitlab::Regex do
     it { is_expected.not_to match('aa-1234-cc') }
     it { is_expected.not_to match('9/9/2018') }
   end
+
+  describe '.kubernetes_namespace_regex' do
+    subject { described_class.kubernetes_namespace_regex }
+
+    it { is_expected.to match('foo') }
+    it { is_expected.to match('foo-bar') }
+    it { is_expected.to match('1foo-bar') }
+    it { is_expected.to match('foo-bar2') }
+    it { is_expected.to match('foo-1bar') }
+    it { is_expected.not_to match('foo.bar') }
+    it { is_expected.not_to match('Foo') }
+    it { is_expected.not_to match('FoO') }
+    it { is_expected.not_to match('FoO-') }
+    it { is_expected.not_to match('-foo-') }
+    it { is_expected.not_to match('foo/bar') }
+  end
+
+  describe '.kubernetes_dns_subdomain_regex' do
+    subject { described_class.kubernetes_dns_subdomain_regex }
+
+    it { is_expected.to match('foo') }
+    it { is_expected.to match('foo-bar') }
+    it { is_expected.to match('foo.bar') }
+    it { is_expected.to match('foo1.bar') }
+    it { is_expected.to match('foo1.2bar') }
+    it { is_expected.to match('foo.bar1') }
+    it { is_expected.to match('1foo.bar1') }
+    it { is_expected.not_to match('Foo') }
+    it { is_expected.not_to match('FoO') }
+    it { is_expected.not_to match('FoO-') }
+    it { is_expected.not_to match('-foo-') }
+    it { is_expected.not_to match('foo/bar') }
+  end
 end

spec/services/pod_logs/base_service_spec.rb

@@ -103,6 +103,36 @@ describe ::PodLogs::BaseService do
         expect(result[:container_name]).to eq(container_name)
       end
     end
+
+    context 'when pod_name is not a string' do
+      let(:params) do
+        {
+            'pod_name' => { something_that_is: :not_a_string }
+        }
+      end
+
+      it 'returns error' do
+        result = subject.send(:check_arguments, {})
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Invalid pod_name')
+      end
+    end
+
+    context 'when container_name is not a string' do
+      let(:params) do
+        {
+            'container_name' => { something_that_is: :not_a_string }
+        }
+      end
+
+      it 'returns error' do
+        result = subject.send(:check_arguments, {})
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq('Invalid container_name')
+      end
+    end
   end
 
   describe '#get_pod_names' do

spec/services/pod_logs/elasticsearch_service_spec.rb

@@ -158,6 +158,21 @@ describe ::PodLogs::ElasticsearchService do
       end
     end
 
+    context 'with search provided and invalid' do
+      let(:params) do
+        {
+            'search' => { term: "foo-bar" }
+        }
+      end
+
+      it 'returns error' do
+        result = subject.send(:check_search, {})
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Invalid search parameter")
+      end
+    end
+
     context 'with search not provided' do
       let(:params) do
         {}
@@ -188,6 +203,21 @@ describe ::PodLogs::ElasticsearchService do
       end
     end
 
+    context 'with cursor provided and invalid' do
+      let(:params) do
+        {
+            'cursor' => { term: "foo-bar" }
+        }
+      end
+
+      it 'returns error' do
+        result = subject.send(:check_cursor, {})
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message]).to eq("Invalid cursor parameter")
+      end
+    end
+
     context 'with cursor not provided' do
       let(:params) do
         {}

spec/services/pod_logs/kubernetes_service_spec.rb

@@ -218,7 +218,7 @@ describe ::PodLogs::KubernetesService do
     end
 
     it 'returns error if pod_name was specified but does not exist' do
-      result = subject.send(:check_pod_name, pod_name: 'another_pod', pods: [pod_name])
+      result = subject.send(:check_pod_name, pod_name: 'another-pod', pods: [pod_name])
 
       expect(result[:status]).to eq(:error)
       expect(result[:message]).to eq('Pod does not exist')
@@ -230,6 +230,13 @@ describe ::PodLogs::KubernetesService do
       expect(result[:status]).to eq(:error)
       expect(result[:message]).to eq('pod_name cannot be larger than 253 chars')
     end
+
+    it 'returns error if pod_name is in invalid format' do
+      result = subject.send(:check_pod_name, pod_name: "Invalid_pod_name", pods: [pod_name])
+
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('pod_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character')
+    end
   end
 
   describe '#check_container_name' do
@@ -287,5 +294,16 @@ describe ::PodLogs::KubernetesService do
       expect(result[:status]).to eq(:error)
       expect(result[:message]).to eq('container_name cannot be larger than 253 chars')
     end
+
+    it 'returns error if container_name is in invalid format' do
+      result = subject.send(:check_container_name,
+        container_name: "Invalid_container_name",
+        pod_name: pod_name,
+        raw_pods: raw_pods
+      )
+
+      expect(result[:status]).to eq(:error)
+      expect(result[:message]).to eq('container_name can contain only lowercase letters, digits, \'-\', and \'.\' and must start and end with an alphanumeric character')
+    end
   end
 end