Backport EE changes to the Kubernetes service

5 files changed, 107 insertions, 57 deletions

diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb
index 8977a7cdafe..48e7802c557 100644
--- a/app/models/project_services/kubernetes_service.rb
+++ b/app/models/project_services/kubernetes_service.rb
@@ -116,30 +116,19 @@ class KubernetesService < DeploymentService
   # short time later
   def terminals(environment)
     with_reactive_cache do |data|
-      pods = data.fetch(:pods, nil)
-      filter_pods(pods, app: environment.slug).
-        flat_map { |pod| terminals_for_pod(api_url, actual_namespace, pod) }.
-        each { |terminal| add_terminal_auth(terminal, terminal_auth) }
+      pods = filter_by_label(data[:pods], app: environment.slug)
+      terminals = pods.flat_map { |pod| terminals_for_pod(api_url, actual_namespace, pod) }
+      terminals.each { |terminal| add_terminal_auth(terminal, terminal_auth) }
     end
   end
 
-  # Caches all pods in the namespace so other calls don't need to block on
-  # network access.
+  # Caches resources in the namespace so other calls don't need to block on
+  # network access
   def calculate_reactive_cache
     return unless active? && project && !project.pending_delete?
 
-    kubeclient = build_kubeclient!
-
-    # Store as hashes, rather than as third-party types
-    pods = begin
-      kubeclient.get_pods(namespace: actual_namespace).as_json
-    rescue KubeException => err
-      raise err unless err.error_code == 404
-      []
-    end
-
     # We may want to cache extra things in the future
-    { pods: pods }
+    { pods: read_pods }
   end
 
   TEMPLATE_PLACEHOLDER = 'Kubernetes namespace'.freeze
@@ -166,6 +155,16 @@ class KubernetesService < DeploymentService
     )
   end
 
+  # Returns a hash of all pods in the namespace
+  def read_pods
+    kubeclient = build_kubeclient!
+
+    kubeclient.get_pods(namespace: actual_namespace).as_json
+  rescue KubeException => err
+    raise err unless err.error_code == 404
+    []
+  end
+
   def kubeclient_ssl_options
     opts = { verify_ssl: OpenSSL::SSL::VERIFY_PEER }
 
@@ -181,11 +180,11 @@ class KubernetesService < DeploymentService
     { bearer_token: token }
   end
 
-  def join_api_url(*parts)
+  def join_api_url(api_path)
     url = URI.parse(api_url)
     prefix = url.path.sub(%r{/+\z}, '')
 
-    url.path = [prefix, *parts].join("/")
+    url.path = [prefix, api_path].join("/")
 
     url.to_s
   end
diff --git a/lib/gitlab/kubernetes.rb b/lib/gitlab/kubernetes.rb
index 4a6091488c8..c56c1a4322f 100644
--- a/lib/gitlab/kubernetes.rb
+++ b/lib/gitlab/kubernetes.rb
@@ -8,13 +8,13 @@ module Gitlab
     )
 
     # Filters an array of pods (as returned by the kubernetes API) by their labels
-    def filter_pods(pods, labels = {})
-      pods.select do |pod|
-        metadata = pod.fetch("metadata", {})
-        pod_labels = metadata.fetch("labels", nil)
-        next unless pod_labels
+    def filter_by_label(items, labels = {})
+      items.select do |item|
+        metadata = item.fetch("metadata", {})
+        item_labels = metadata.fetch("labels", nil)
+        next unless item_labels
 
-        labels.all? { |k, v| pod_labels[k.to_s] == v }
+        labels.all? { |k, v| item_labels[k.to_s] == v }
       end
     end
 
diff --git a/spec/lib/gitlab/kubernetes_spec.rb b/spec/lib/gitlab/kubernetes_spec.rb
index 91f9d06b85a..e8c599a95ee 100644
--- a/spec/lib/gitlab/kubernetes_spec.rb
+++ b/spec/lib/gitlab/kubernetes_spec.rb
@@ -1,6 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::Kubernetes do
+  include KubernetesHelpers
   include described_class
 
   describe '#container_exec_url' do
@@ -36,4 +37,13 @@ describe Gitlab::Kubernetes do
       it { expect(result.query).to match(/\Acontainer=container\+1&/) }
     end
   end
+
+  describe '#filter_by_label' do
+    it 'returns matching labels' do
+      matching_items = [kube_pod(app: 'foo')]
+      items = matching_items + [kube_pod]
+
+      expect(filter_by_label(items, app: 'foo')).to eq(matching_items)
+    end
+  end
 end
diff --git a/spec/models/project_services/kubernetes_service_spec.rb b/spec/models/project_services/kubernetes_service_spec.rb
index 0dcf4a4b5d6..35dd601351f 100644
--- a/spec/models/project_services/kubernetes_service_spec.rb
+++ b/spec/models/project_services/kubernetes_service_spec.rb
@@ -7,24 +7,6 @@ describe KubernetesService, models: true, caching: true do
   let(:project) { build_stubbed(:kubernetes_project) }
   let(:service) { project.kubernetes_service }
 
-  # We use Kubeclient to interactive with the Kubernetes API. It will
-  # GET /api/v1 for a list of resources the API supports. This must be stubbed
-  # in addition to any other HTTP requests we expect it to perform.
-  let(:discovery_url) { service.api_url + '/api/v1' }
-  let(:discovery_response) { { body: kube_discovery_body.to_json } }
-
-  let(:pods_url) { service.api_url + "/api/v1/namespaces/#{service.actual_namespace}/pods" }
-  let(:pods_response) { { body: kube_pods_body(kube_pod).to_json } }
-
-  def stub_kubeclient_discover
-    WebMock.stub_request(:get, discovery_url).to_return(discovery_response)
-  end
-
-  def stub_kubeclient_pods
-    stub_kubeclient_discover
-    WebMock.stub_request(:get, pods_url).to_return(pods_response)
-  end
-
   describe "Associations" do
     it { is_expected.to belong_to :project }
   end
@@ -111,6 +93,34 @@ describe KubernetesService, models: true, caching: true do
     it "returns the default namespace" do
       is_expected.to eq(service.send(:default_namespace))
     end
+
+    context 'when namespace is specified' do
+      before do
+        service.namespace = 'my-namespace'
+      end
+
+      it "returns the user-namespace" do
+        is_expected.to eq('my-namespace')
+      end
+    end
+
+    context 'when service is not assigned to project' do
+      before do
+        service.project = nil
+      end
+
+      it "does not return namespace" do
+        is_expected.to be_nil
+      end
+    end
+  end
+
+  describe '#actual_namespace' do
+    subject { service.actual_namespace }
+
+    it "returns the default namespace" do
+      is_expected.to eq(service.send(:default_namespace))
+    end
     
     context 'when namespace is specified' do
       before do
@@ -134,6 +144,8 @@ describe KubernetesService, models: true, caching: true do
   end
 
   describe '#test' do
+    let(:discovery_url) { 'https://kubernetes.example.com/api/v1' }
+
     before do
       stub_kubeclient_discover
     end
@@ -142,7 +154,8 @@ describe KubernetesService, models: true, caching: true do
       let(:discovery_url) { 'https://kubernetes.example.com/prefix/api/v1' }
 
       it 'tests with the prefix' do
-        service.api_url = 'https://kubernetes.example.com/prefix/'
+        service.api_url = 'https://kubernetes.example.com/prefix'
+        stub_kubeclient_discover
 
         expect(service.test[:success]).to be_truthy
         expect(WebMock).to have_requested(:get, discovery_url).once
@@ -170,9 +183,9 @@ describe KubernetesService, models: true, caching: true do
     end
 
     context 'failure' do
-      let(:discovery_response) { { status: 404 } }
-
       it 'fails to read the discovery endpoint' do
+        WebMock.stub_request(:get, service.api_url + '/api/v1').to_return(status: 404)
+
         expect(service.test[:success]).to be_falsy
         expect(WebMock).to have_requested(:get, discovery_url).once
       end
@@ -258,7 +271,6 @@ describe KubernetesService, models: true, caching: true do
   end
 
   describe '#calculate_reactive_cache' do
-    before { stub_kubeclient_pods }
     subject { service.calculate_reactive_cache }
 
     context 'when service is inactive' do
@@ -268,17 +280,25 @@ describe KubernetesService, models: true, caching: true do
     end
 
     context 'when kubernetes responds with valid pods' do
+      before do
+        stub_kubeclient_pods
+      end
+
       it { is_expected.to eq(pods: [kube_pod]) }
     end
 
-    context 'when kubernetes responds with 500' do
-      let(:pods_response) { { status: 500 } }
+    context 'when kubernetes responds with 500s' do
+      before do
+        stub_kubeclient_pods(status: 500)
+      end
 
       it { expect { subject }.to raise_error(KubeException) }
     end
 
-    context 'when kubernetes responds with 404' do
-      let(:pods_response) { { status: 404 } }
+    context 'when kubernetes responds with 404s' do
+      before do
+        stub_kubeclient_pods(status: 404)
+      end
 
       it { is_expected.to eq(pods: []) }
     end
diff --git a/spec/support/kubernetes_helpers.rb b/spec/support/kubernetes_helpers.rb
index 9280fad4ace..c92f78b324c 100644
--- a/spec/support/kubernetes_helpers.rb
+++ b/spec/support/kubernetes_helpers.rb
@@ -1,7 +1,26 @@
 module KubernetesHelpers
   include Gitlab::Kubernetes
 
-  def kube_discovery_body
+  def kube_response(body)
+    { body: body.to_json }
+  end
+
+  def kube_pods_response
+    kube_response(kube_pods_body)
+  end
+
+  def stub_kubeclient_discover
+    WebMock.stub_request(:get, service.api_url + '/api/v1').to_return(kube_response(kube_v1_discovery_body))
+  end
+
+  def stub_kubeclient_pods(response = nil)
+    stub_kubeclient_discover
+    pods_url = service.api_url + "/api/v1/namespaces/#{service.actual_namespace}/pods"
+
+    WebMock.stub_request(:get, pods_url).to_return(response || kube_pods_response)
+  end
+
+  def kube_v1_discovery_body
     {
       "kind" => "APIResourceList",
       "resources" => [
@@ -10,17 +29,19 @@ module KubernetesHelpers
     }
   end
 
-  def kube_pods_body(*pods)
-    { "kind" => "PodList",
-      "items" => [kube_pod] }
+  def kube_pods_body
+    {
+      "kind" => "PodList",
+      "items" => [kube_pod]
+    }
   end
 
   # This is a partial response, it will have many more elements in reality but
   # these are the ones we care about at the moment
-  def kube_pod(app: "valid-pod-label")
+  def kube_pod(name: "kube-pod", app: "valid-pod-label")
     {
       "metadata" => {
-        "name" => "kube-pod",
+        "name" => name,
         "creationTimestamp" => "2016-11-25T19:55:19Z",
         "labels" => { "app" => app }
       },