Merge branch 'gcp-fix' into 'master'10-4-stable-prepare-rc4

Fix GCP redirect

Closes #41867

See merge request gitlab-org/gitlab-ce!16355

(cherry picked from commit 59adc07f00f4f3ea326194d77c31580edfdfb2a4)

b44583e9 Extract GCP billing check as method
cf6258af Fix billing checking
0cdd56e6 Fix link to billing
e52bae3b Fix CheckGcpProjectBillingService spec
b8b2f5ff Fix CheckGcpProjectBillingWorker spec
8ba3e473 Fix GCP Controller spec
1f0a4fe6 Add missing user agent header to GCP client
6ef28ace Add API requirements to docs
0b294fc2 Use new tab for link in flash
cf95756a Refactor GCP redirect test suite
e6012d3e Change failed GCP billing check wording
35598274 Fix breadcumb of clusters show page
cf842986 Update links for GCP instructions

12 files changed, 135 insertions, 47 deletions

diff --git a/app/controllers/projects/clusters/gcp_controller.rb b/app/controllers/projects/clusters/gcp_controller.rb
index 25608df0b9c..4fc515bd03e 100644
--- a/app/controllers/projects/clusters/gcp_controller.rb
+++ b/app/controllers/projects/clusters/gcp_controller.rb
@@ -1,8 +1,9 @@
 class Projects::Clusters::GcpController < Projects::ApplicationController
   before_action :authorize_read_cluster!
   before_action :authorize_google_api, except: [:login]
-  before_action :authorize_google_project_billing, only: [:new]
+  before_action :authorize_google_project_billing, only: [:new, :create]
   before_action :authorize_create_cluster!, only: [:new, :create]
+  before_action :verify_billing, only: [:create]
 
   def login
     begin
@@ -23,24 +24,34 @@ class Projects::Clusters::GcpController < Projects::ApplicationController
   end
 
   def create
+    @cluster = ::Clusters::CreateService
+      .new(project, current_user, create_params)
+      .execute(token_in_session)
+
+    if @cluster.persisted?
+      redirect_to project_cluster_path(project, @cluster)
+    else
+      render :new
+    end
+  end
+
+  private
+
+  def verify_billing
     case google_project_billing_status
     when 'true'
-      @cluster = ::Clusters::CreateService
-        .new(project, current_user, create_params)
-        .execute(token_in_session)
-
-      return redirect_to project_cluster_path(project, @cluster) if @cluster.persisted?
+      return
     when 'false'
-      flash[:error] = _('Please enable billing for one of your projects to be able to create a cluster.')
+      flash[:alert] = _('Please <a href=%{link_to_billing} target="_blank" rel="noopener noreferrer">enable billing for one of your projects to be able to create a cluster</a>, then try again.').html_safe % { link_to_billing: "https://console.cloud.google.com/freetrial?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral" }
     else
-      flash[:error] = _('We could not verify that one of your projects on GCP has billing enabled. Please try again.')
+      flash[:alert] = _('We could not verify that one of your projects on GCP has billing enabled. Please try again.')
     end
 
+    @cluster = ::Clusters::Cluster.new(create_params)
+
     render :new
   end
 
-  private
-
   def create_params
     params.require(:cluster).permit(
       :enabled,
diff --git a/app/services/check_gcp_project_billing_service.rb b/app/services/check_gcp_project_billing_service.rb
index 854adf2177d..ea82b61b279 100644
--- a/app/services/check_gcp_project_billing_service.rb
+++ b/app/services/check_gcp_project_billing_service.rb
@@ -2,7 +2,10 @@ class CheckGcpProjectBillingService
   def execute(token)
     client = GoogleApi::CloudPlatform::Client.new(token, nil)
     client.projects_list.select do |project|
-      client.projects_get_billing_info(project.name).billingEnabled
+      begin
+        client.projects_get_billing_info(project.project_id).billing_enabled
+      rescue
+      end
     end
   end
 end
diff --git a/app/views/projects/clusters/gcp/_header.html.haml b/app/views/projects/clusters/gcp/_header.html.haml
index e2d7326a312..bddb902115d 100644
--- a/app/views/projects/clusters/gcp/_header.html.haml
+++ b/app/views/projects/clusters/gcp/_header.html.haml
@@ -4,11 +4,11 @@
   = s_('ClusterIntegration|Please make sure that your Google account meets the following requirements:')
 %ul
   %li
-    - link_to_kubernetes_engine = link_to(s_('ClusterIntegration|access to Google Kubernetes Engine'), 'https://console.cloud.google.com', target: '_blank', rel: 'noopener noreferrer')
+    - link_to_kubernetes_engine = link_to(s_('ClusterIntegration|access to Google Kubernetes Engine'), 'https://console.cloud.google.com/freetrial?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral', target: '_blank', rel: 'noopener noreferrer')
     = s_('ClusterIntegration|Your account must have %{link_to_kubernetes_engine}').html_safe % { link_to_kubernetes_engine: link_to_kubernetes_engine }
     %li
-      - link_to_requirements = link_to(s_('ClusterIntegration|meets the requirements'), 'https://cloud.google.com/kubernetes-engine/docs/quickstart', target: '_blank', rel: 'noopener noreferrer')
+      - link_to_requirements = link_to(s_('ClusterIntegration|meets the requirements'), 'https://cloud.google.com/kubernetes-engine/docs/quickstart?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral', target: '_blank', rel: 'noopener noreferrer')
       = s_('ClusterIntegration|Make sure your account %{link_to_requirements} to create clusters').html_safe % { link_to_requirements: link_to_requirements }
     %li
-      - link_to_container_project = link_to(s_('ClusterIntegration|Google Kubernetes Engine project'), 'https://console.cloud.google.com/home/dashboard', target: '_blank', rel: 'noopener noreferrer')
+      - link_to_container_project = link_to(s_('ClusterIntegration|Google Kubernetes Engine project'), 'https://console.cloud.google.com/home/dashboard?utm_campaign=2018_cpanel&utm_source=gitlab&utm_medium=referral', target: '_blank', rel: 'noopener noreferrer')
       = s_('ClusterIntegration|This account must have permissions to create a cluster in the %{link_to_container_project} specified below').html_safe % { link_to_container_project: link_to_container_project }
diff --git a/app/views/projects/clusters/show.html.haml b/app/views/projects/clusters/show.html.haml
index c7c84b5a42c..2049105dff6 100644
--- a/app/views/projects/clusters/show.html.haml
+++ b/app/views/projects/clusters/show.html.haml
@@ -1,6 +1,6 @@
 - @content_class = "limit-container-width" unless fluid_layout
 - add_to_breadcrumbs "Clusters", project_clusters_path(@project)
-- breadcrumb_title @cluster.id
+- breadcrumb_title @cluster.name
 - page_title _("Cluster")
 
 - expanded = Rails.env.test?
diff --git a/app/workers/check_gcp_project_billing_worker.rb b/app/workers/check_gcp_project_billing_worker.rb
index 557af14ee57..5466ccdda59 100644
--- a/app/workers/check_gcp_project_billing_worker.rb
+++ b/app/workers/check_gcp_project_billing_worker.rb
@@ -4,7 +4,7 @@ class CheckGcpProjectBillingWorker
   include ApplicationWorker
   include ClusterQueue
 
-  LEASE_TIMEOUT = 15.seconds.to_i
+  LEASE_TIMEOUT = 3.seconds.to_i
   SESSION_KEY_TIMEOUT = 5.minutes
   BILLING_TIMEOUT = 1.hour
 
@@ -23,13 +23,13 @@ class CheckGcpProjectBillingWorker
   end
 
   def self.redis_shared_state_key_for(token)
-    "gitlab:gcp:#{token.hash}:billing_enabled"
+    "gitlab:gcp:#{Digest::SHA1.hexdigest(token)}:billing_enabled"
   end
 
   def perform(token_key)
     return unless token_key
 
-    token = self.get_session_token(token_key)
+    token = self.class.get_session_token(token_key)
     return unless token
     return unless try_obtain_lease_for(token)
 
diff --git a/doc/user/project/clusters/index.md b/doc/user/project/clusters/index.md
index d5619c7b563..218b9dee41d 100644
--- a/doc/user/project/clusters/index.md
+++ b/doc/user/project/clusters/index.md
@@ -25,11 +25,14 @@ prerequisites must be met:
   be enabled in GitLab at the instance level. If that's not the case, ask your
   administrator to enable it.
 - Your associated Google account must have the right privileges to manage
-  clusters on GKE. That would mean that a
-  [billing account](https://cloud.google.com/billing/docs/how-to/manage-billing-account)
-  must be set up.
-- You must have Master [permissions] in order to be able to access the **Cluster**
-  page.
+  clusters on GKE. That would mean that a [billing
+  account](https://cloud.google.com/billing/docs/how-to/manage-billing-account)
+  must be set up and that you have to have permissions to access it.
+- You must have Master [permissions] in order to be able to access the
+  **Cluster** page.
+- You must have [Cloud Billing API](https://cloud.google.com/billing/) enabled
+- You must have [Resource Manager
+  API](https://cloud.google.com/resource-manager/)
 
 If all of the above requirements are met, you can proceed to add a new GKE
 cluster.
diff --git a/lib/google_api/cloud_platform/client.rb b/lib/google_api/cloud_platform/client.rb
index f05d001fd02..ff638c07755 100644
--- a/lib/google_api/cloud_platform/client.rb
+++ b/lib/google_api/cloud_platform/client.rb
@@ -47,15 +47,15 @@ module GoogleApi
         service.authorization = access_token
 
         service.fetch_all(items: :projects) do |token|
-          service.list_projects(page_token: token)
+          service.list_projects(page_token: token, options: user_agent_header)
         end
       end
 
-      def projects_get_billing_info(project_name)
+      def projects_get_billing_info(project_id)
         service = Google::Apis::CloudbillingV1::CloudbillingService.new
         service.authorization = access_token
 
-        service.get_project_billing_info("projects/#{project_name}")
+        service.get_project_billing_info("projects/#{project_id}", options: user_agent_header)
       end
 
       def projects_zones_clusters_get(project_id, zone, cluster_id)
diff --git a/spec/controllers/projects/clusters/gcp_controller_spec.rb b/spec/controllers/projects/clusters/gcp_controller_spec.rb
index be19fa93183..775f9db1c6e 100644
--- a/spec/controllers/projects/clusters/gcp_controller_spec.rb
+++ b/spec/controllers/projects/clusters/gcp_controller_spec.rb
@@ -137,11 +137,14 @@ describe Projects::Clusters::GcpController do
       context 'when access token is valid' do
         before do
           stub_google_api_validate_token
+          allow_any_instance_of(described_class).to receive(:authorize_google_project_billing)
         end
 
         context 'when google project billing is enabled' do
           before do
-            stub_google_project_billing_status
+            redis_double = double
+            allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_double)
+            allow(redis_double).to receive(:get).with(CheckGcpProjectBillingWorker.redis_shared_state_key_for('token')).and_return('true')
           end
 
           it 'creates a new cluster' do
@@ -158,7 +161,7 @@ describe Projects::Clusters::GcpController do
           it 'renders the cluster form with an error' do
             go
 
-            expect(response).to set_flash[:error]
+            expect(response).to set_flash[:alert]
             expect(response).to render_template('new')
           end
         end
diff --git a/spec/features/projects/clusters/gcp_spec.rb b/spec/features/projects/clusters/gcp_spec.rb
index 523cc08496b..8953b30bebf 100644
--- a/spec/features/projects/clusters/gcp_spec.rb
+++ b/spec/features/projects/clusters/gcp_spec.rb
@@ -13,6 +13,8 @@ feature 'Gcp Cluster', :js do
   end
 
   context 'when user has signed with Google' do
+    let(:project_id) { 'test-project-1234' }
+
     before do
       allow_any_instance_of(Projects::Clusters::GcpController)
         .to receive(:token_in_session).and_return('token')
@@ -23,7 +25,7 @@ feature 'Gcp Cluster', :js do
     context 'when user has a GCP project with billing enabled' do
       before do
         allow_any_instance_of(Projects::Clusters::GcpController).to receive(:authorize_google_project_billing)
-        stub_google_project_billing_status
+        allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return('true')
       end
 
       context 'when user does not have a cluster and visits cluster index page' do
@@ -131,15 +133,41 @@ feature 'Gcp Cluster', :js do
 
     context 'when user does not have a GCP project with billing enabled' do
       before do
+        allow_any_instance_of(Projects::Clusters::GcpController).to receive(:authorize_google_project_billing)
+        allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return('false')
+
         visit project_clusters_path(project)
 
         click_link 'Add cluster'
         click_link 'Create on GKE'
+
+        fill_in 'cluster_provider_gcp_attributes_gcp_project_id', with: 'gcp-project-123'
+        fill_in 'cluster_name', with: 'dev-cluster'
+        click_button 'Create cluster'
+      end
+
+      it 'user sees form with error' do
+        expect(page).to have_content('Please enable billing for one of your projects to be able to create a cluster, then try again.')
+      end
+    end
+
+    context 'when gcp billing status is not in redis' do
+      before do
+        allow_any_instance_of(Projects::Clusters::GcpController).to receive(:authorize_google_project_billing)
+        allow_any_instance_of(Projects::Clusters::GcpController).to receive(:google_project_billing_status).and_return(nil)
+
+        visit project_clusters_path(project)
+
+        click_link 'Add cluster'
+        click_link 'Create on GKE'
+
+        fill_in 'cluster_provider_gcp_attributes_gcp_project_id', with: 'gcp-project-123'
+        fill_in 'cluster_name', with: 'dev-cluster'
+        click_button 'Create cluster'
       end
 
-      it 'user sees a check page' do
-        pending 'the frontend still has not been implemented'
-        expect(page).to have_link('Continue')
+      it 'user sees form with error' do
+        expect(page).to have_content('We could not verify that one of your projects on GCP has billing enabled. Please try again.')
       end
     end
   end
diff --git a/spec/services/check_gcp_project_billing_service_spec.rb b/spec/services/check_gcp_project_billing_service_spec.rb
index f0e39ba6f49..3e68d906e71 100644
--- a/spec/services/check_gcp_project_billing_service_spec.rb
+++ b/spec/services/check_gcp_project_billing_service_spec.rb
@@ -1,29 +1,30 @@
 require 'spec_helper'
 
 describe CheckGcpProjectBillingService do
+  include GoogleApi::CloudPlatformHelpers
+
   let(:service) { described_class.new }
-  let(:projects) { [double(name: 'first_project'), double(name: 'second_project')] }
+  let(:project_id) { 'test-project-1234' }
 
   describe '#execute' do
     before do
-      expect_any_instance_of(GoogleApi::CloudPlatform::Client)
-        .to receive(:projects_list).and_return(projects)
-
-      allow_any_instance_of(GoogleApi::CloudPlatform::Client)
-        .to receive_message_chain(:projects_get_billing_info, :billingEnabled)
-        .and_return(project_billing_enabled)
+      stub_cloud_platform_projects_list(project_id: project_id)
     end
 
     subject { service.execute('bogustoken') }
 
     context 'google account has a billing enabled gcp project' do
-      let(:project_billing_enabled) { true }
+      before do
+        stub_cloud_platform_projects_get_billing_info(project_id, true)
+      end
 
-      it { is_expected.to eq(projects) }
+      it { is_expected.to all(satisfy { |project| project.project_id == project_id }) }
     end
 
     context 'google account does not have a billing enabled gcp project' do
-      let(:project_billing_enabled) { false }
+      before do
+        stub_cloud_platform_projects_get_billing_info(project_id, false)
+      end
 
       it { is_expected.to eq([]) }
     end
diff --git a/spec/support/google_api/cloud_platform_helpers.rb b/spec/support/google_api/cloud_platform_helpers.rb
index 99752ed396e..2fdbddd40c2 100644
--- a/spec/support/google_api/cloud_platform_helpers.rb
+++ b/spec/support/google_api/cloud_platform_helpers.rb
@@ -10,10 +10,14 @@ module GoogleApi
       request.session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at] = 1.hour.ago.to_i.to_s
     end
 
-    def stub_google_project_billing_status
-      redis_double = double
-      allow(Gitlab::Redis::SharedState).to receive(:with).and_yield(redis_double)
-      allow(redis_double).to receive(:get).with(CheckGcpProjectBillingWorker.redis_shared_state_key_for('token')).and_return('true')
+    def stub_cloud_platform_projects_list(options)
+      WebMock.stub_request(:get, cloud_platform_projects_list_url)
+        .to_return(cloud_platform_response(cloud_platform_projects_body(options)))
+    end
+
+    def stub_cloud_platform_projects_get_billing_info(project_id, billing_enabled)
+      WebMock.stub_request(:get, cloud_platform_projects_get_billing_info_url(project_id))
+        .to_return(cloud_platform_response(cloud_platform_projects_billing_info_body(project_id, billing_enabled)))
     end
 
     def stub_cloud_platform_get_zone_cluster(project_id, zone, cluster_id, **options)
@@ -46,6 +50,14 @@ module GoogleApi
         .to_return(status: [500, "Internal Server Error"])
     end
 
+    def cloud_platform_projects_list_url
+      "https://cloudresourcemanager.googleapis.com/v1/projects"
+    end
+
+    def cloud_platform_projects_get_billing_info_url(project_id)
+      "https://cloudbilling.googleapis.com/v1/projects/#{project_id}/billingInfo"
+    end
+
     def cloud_platform_get_zone_cluster_url(project_id, zone, cluster_id)
       "https://container.googleapis.com/v1/projects/#{project_id}/zones/#{zone}/clusters/#{cluster_id}"
     end
@@ -121,5 +133,32 @@ module GoogleApi
         "endTime": options[:endTime] || ''
       }
     end
+
+    def cloud_platform_projects_body(**options)
+      {
+        "projects": [
+          {
+            "projectNumber": options[:project_number] || "1234",
+            "projectId": options[:project_id] || "test-project-1234",
+            "lifecycleState": "ACTIVE",
+            "name": options[:name] || "test-project",
+            "createTime": "2017-12-16T01:48:29.129Z",
+            "parent": {
+              "type": "organization",
+              "id": "12345"
+            }
+          }
+        ]
+      }
+    end
+
+    def cloud_platform_projects_billing_info_body(project_id, billing_enabled)
+      {
+        "name": "projects/#{project_id}/billingInfo",
+        "projectId": "#{project_id}",
+        "billingAccountName": "account-name",
+        "billingEnabled": billing_enabled
+      }
+    end
   end
 end
diff --git a/spec/workers/check_gcp_project_billing_worker_spec.rb b/spec/workers/check_gcp_project_billing_worker_spec.rb
index f52a903327c..7b7a7c1bc44 100644
--- a/spec/workers/check_gcp_project_billing_worker_spec.rb
+++ b/spec/workers/check_gcp_project_billing_worker_spec.rb
@@ -8,7 +8,7 @@ describe CheckGcpProjectBillingWorker do
 
     context 'when there is a token in redis' do
       before do
-        allow_any_instance_of(described_class).to receive(:get_session_token).and_return(token)
+        allow(described_class).to receive(:get_session_token).and_return(token)
       end
 
       context 'when there is no lease' do