Implement Policy. Use show instead of edit. Chnage db column. fix comments. dry up workers

app/controllers/projects/clusters_controller.rb

 class Projects::ClustersController < Projects::ApplicationController
   before_action :cluster, except: [:login, :index, :new, :create]
-  before_action :authorize_admin_cluster!
-  before_action :authorize_google_api, except: [:login]
+  before_action :authorize_read_cluster!
+  before_action :authorize_create_cluster!, only: [:new, :create]
+  before_action :authorize_google_api, only: [:new, :create]
+  before_action :authorize_update_cluster!, only: [:update]
+  before_action :authorize_admin_cluster!, only: [:destroy]
 
   def login
     begin
@@ -16,7 +19,7 @@ class Projects::ClustersController < Projects::ApplicationController
 
   def index
     if project.cluster
-      redirect_to edit_project_cluster_path(project, project.cluster)
+      redirect_to project_cluster_path(project, project.cluster)
     else
       redirect_to new_project_cluster_path(project)
     end
@@ -32,7 +35,6 @@ class Projects::ClustersController < Projects::ApplicationController
       .execute(token_in_session)
 
     if @cluster.persisted?
-      ClusterCreationWorker.perform_async(@cluster.id)
       redirect_to project_clusters_path(project)
     else
       render :new
@@ -52,7 +54,7 @@ class Projects::ClustersController < Projects::ApplicationController
     end
   end
 
-  def edit
+  def show
   end
 
   def update
@@ -60,14 +62,14 @@ class Projects::ClustersController < Projects::ApplicationController
       .new(project, current_user, cluster_params)
       .execute(cluster)
 
-    render :edit
+    render :show
   end
 
   def destroy
     if cluster.destroy
       redirect_to project_clusters_path(project), status: 302
     else
-      render :edit
+      render :show
     end
   end
 
@@ -79,8 +81,8 @@ class Projects::ClustersController < Projects::ApplicationController
 
   def cluster_params
     params.require(:cluster)
-      .permit(:gcp_project_id, :cluster_zone, :cluster_name, :cluster_size,
-              :machine_type, :project_namespace, :enabled)
+      .permit(:gcp_project_id, :gcp_cluster_zone, :gcp_cluster_name, :gcp_cluster_size,
+              :gcp_machine_type, :project_namespace, :enabled)
   end
 
   def authorize_google_api
@@ -99,4 +101,12 @@ class Projects::ClustersController < Projects::ApplicationController
     @expires_at_in_session ||=
       session[GoogleApi::CloudPlatform::Client.session_key_for_expires_at]
   end
+
+  def authorize_update_cluster!
+    return access_denied! unless can?(current_user, :update_cluster, cluster)
+  end
+
+  def authorize_admin_cluster!
+    return access_denied! unless can?(current_user, :admin_cluster, cluster)
+  end
 end

app/models/gcp/cluster.rb

@@ -8,19 +8,16 @@ module Gcp
 
     attr_encrypted :password,
       mode: :per_attribute_iv,
-      insecure_mode: true,
       key: Gitlab::Application.secrets.db_key_base,
       algorithm: 'aes-256-cbc'
 
     attr_encrypted :kubernetes_token,
       mode: :per_attribute_iv,
-      insecure_mode: true,
       key: Gitlab::Application.secrets.db_key_base,
       algorithm: 'aes-256-cbc'
 
     attr_encrypted :gcp_token,
       mode: :per_attribute_iv,
-      insecure_mode: true,
       key: Gitlab::Application.secrets.db_key_base,
       algorithm: 'aes-256-cbc'
 
@@ -33,9 +30,9 @@ module Gcp
     }
 
     validates :gcp_project_id, presence: true
-    validates :cluster_zone, presence: true
-    validates :cluster_name, presence: true
-    validates :cluster_size, presence: true,
+    validates :gcp_cluster_zone, presence: true
+    validates :gcp_cluster_name, presence: true
+    validates :gcp_cluster_size, presence: true,
               numericality: { only_integer: true, greater_than: 0 }
     validate :restrict_modification, on: :update
 

app/policies/gcp/cluster_policy.rb

+module Gcp
+  class ClusterPolicy < BasePolicy
+    alias_method :cluster, :subject
+
+    delegate { @subject.project }
+
+    condition(:safe_to_change) do
+      can?(:master_access) && !cluster.on_creation?
+    end
+
+    rule { safe_to_change }.policy do
+      enable :update_cluster
+      enable :admin_cluster
+    end
+  end
+end

app/policies/project_policy.rb

@@ -164,6 +164,7 @@ class ProjectPolicy < BasePolicy
     enable :create_pipeline
     enable :update_pipeline
     enable :create_pipeline_schedule
+    enable :read_cluster
     enable :create_merge_request
     enable :create_wiki
     enable :push_code
@@ -188,7 +189,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_build
     enable :admin_container_image
     enable :admin_pipeline
-    enable :admin_cluster
+    enable :create_cluster
     enable :admin_environment
     enable :admin_deployment
     enable :admin_pages

app/services/ci/create_cluster_service.rb

 module Ci
   class CreateClusterService < BaseService
     def execute(access_token)
-      if params['machine_type'].blank?
-        params['machine_type'] = GoogleApi::CloudPlatform::Client::DEFAULT_MACHINE_TYPE
-      end
+      params['gcp_machine_type'] ||= GoogleApi::CloudPlatform::Client::DEFAULT_MACHINE_TYPE
 
       project.create_cluster(
         params.merge(user: current_user,
                      status: Gcp::Cluster.statuses[:scheduled],
-                     gcp_token: access_token))
+                     gcp_token: access_token)).tap do |cluster|
+        ClusterCreationWorker.perform_async(cluster.id) if cluster.persisted?
+      end
     end
   end
 end

app/services/ci/create_gke_cluster_service.rb

+module Ci
+  class CreateGkeClusterService
+    def execute(cluster)
+      api_client =
+        GoogleApi::CloudPlatform::Client.new(cluster.gcp_token, nil)
+
+      begin
+        operation = api_client.projects_zones_clusters_create(
+            cluster.gcp_project_id,
+            cluster.gcp_cluster_zone,
+            cluster.gcp_cluster_name,
+            cluster.gcp_cluster_size,
+            machine_type: cluster.gcp_machine_type
+          )
+      rescue Google::Apis::ServerError, Google::Apis::ClientError, Google::Apis::AuthorizationError => e
+        return cluster.errored!("Failed to request to CloudPlatform; #{e.message}")
+      end
+        
+      unless operation.status == 'RUNNING' || operation.status == 'PENDING'
+        return cluster.errored!("Operation status is unexpected; #{operation.status_message}")
+      end
+
+      operation_id = api_client.parse_operation_id(operation.self_link)
+
+      unless operation_id
+        return cluster.errored!('Can not find operation_id from self_link')
+      end
+
+      if cluster.creating!(operation_id)
+        WaitForClusterCreationWorker.perform_in(
+          WaitForClusterCreationWorker::INITIAL_INTERVAL, cluster.id)
+      else
+        return cluster.errored!("Failed to update cluster record; #{cluster.errors}")
+      end
+    end
+  end
+end

app/services/ci/fetch_gcp_operation_service.rb

+module Ci
+  class FetchGcpOperationService
+    def execute(cluster)
+      api_client = 
+        GoogleApi::CloudPlatform::Client.new(cluster.gcp_token, nil)
+
+      operation = api_client.projects_zones_operations(
+        cluster.gcp_project_id,
+        cluster.gcp_cluster_zone,
+        cluster.gcp_operation_id)
+
+      yield(operation) if block_given?
+    rescue Google::Apis::ServerError, Google::Apis::ClientError, Google::Apis::AuthorizationError => e
+      return cluster.errored!("Failed to request to CloudPlatform; #{e.message}")
+    end
+  end
+end

app/services/ci/finalize_cluster_creation_service.rb

+module Ci
+  class FinalizeClusterCreationService
+    def execute(cluster)
+      api_client = 
+        GoogleApi::CloudPlatform::Client.new(cluster.gcp_token, nil)
+
+      begin
+        gke_cluster = api_client.projects_zones_clusters_get(
+          cluster.gcp_project_id,
+          cluster.gcp_cluster_zone,
+          cluster.gcp_cluster_name)
+      rescue Google::Apis::ServerError, Google::Apis::ClientError, Google::Apis::AuthorizationError => e
+        return cluster.errored!("Failed to request to CloudPlatform; #{e.message}")
+      end
+
+      endpoint = gke_cluster.endpoint
+      api_url = 'https://' + endpoint
+      ca_cert = Base64.decode64(gke_cluster.master_auth.cluster_ca_certificate)
+      username = gke_cluster.master_auth.username
+      password = gke_cluster.master_auth.password
+
+      kubernetes_token = Ci::FetchKubernetesTokenService.new(
+        api_url, ca_cert, username, password).execute
+
+      unless kubernetes_token
+        return cluster.errored!('Failed to get a default token of kubernetes')
+      end
+
+      Ci::IntegrateClusterService.new.execute(
+        cluster, endpoint, ca_cert, kubernetes_token, username, password)
+    end
+  end
+end

app/views/projects/clusters/_form.html.haml

@@ -7,8 +7,8 @@
       = form_for [@project.namespace.becomes(Namespace), @project, @cluster] do |field|
         = form_errors(@cluster)
         .form-group
-          = field.label :cluster_name
-          = field.text_field :cluster_name, class: 'form-control'
+          = field.label :gcp_cluster_name
+          = field.text_field :gcp_cluster_name, class: 'form-control'
 
         .form-group
           = field.label :gcp_project_id
@@ -16,25 +16,25 @@
           = field.text_field :gcp_project_id, class: 'form-control'
 
         .form-group
-          = field.label :cluster_zone
+          = field.label :gcp_cluster_zone
           = link_to(s_('ClusterIntegration|See zones'), 'https://cloud.google.com/compute/docs/regions-zones/regions-zones', target: '_blank', rel: 'noopener noreferrer')
-          = field.text_field :cluster_zone, class: 'form-control'
+          = field.text_field :gcp_cluster_zone, class: 'form-control'
 
         .form-group
-          = field.label :cluster_size
-          = field.text_field :cluster_size, class: 'form-control'
+          = field.label :gcp_cluster_size
+          = field.text_field :gcp_cluster_size, class: 'form-control'
 
         .form-group
           = field.label :project_namespace
           = field.text_field :project_namespace, class: 'form-control'
 
         .form-group
-          = field.label :machine_type
+          = field.label :gcp_machine_type
           = link_to(s_('ClusterIntegration|Machine type'), 'https://cloud.google.com/compute/docs/machine-types', target: '_blank', rel: 'noopener noreferrer')
-          = field.text_field :machine_type, class: 'form-control'
+          = field.text_field :gcp_machine_type, class: 'form-control'
 
         .form-group
           = field.submit s_('ClusterIntegration|Create cluster'), class: 'btn btn-save'
 
           -# TODO: Remove before merge
-          = link_to "Create on Google Container Engine", namespace_project_clusters_path(@project.namespace, @project, cluster: {cluster_name: "gke-test-creation#{Random.rand(100)}", gcp_project_id: 'gitlab-internal-153318', cluster_zone: 'us-central1-a', cluster_size: '1', project_namespace: 'aaa', machine_type: 'n1-standard-1'}), method: :post
+          = link_to "Create on Google Container Engine", namespace_project_clusters_path(@project.namespace, @project, cluster: {gcp_cluster_name: "gke-test-creation#{Random.rand(100)}", gcp_project_id: 'gitlab-internal-153318', gcp_cluster_zone: 'us-central1-a', gcp_cluster_size: '1', project_namespace: 'aaa', gcp_machine_type: 'n1-standard-1'}), method: :post

app/views/projects/clusters/edit.html.haml → app/views/projects/clusters/show.html.haml

@@ -55,9 +55,9 @@
       %label
         = s_('ClusterIntegration|Cluster name')
       .input-group
-        %input.form-control{ value: @cluster.cluster_name, disabled: true}
+        %input.form-control{ value: @cluster.gcp_cluster_name, disabled: true}
         %span.input-group-addon.clipboard-addon
-          = clipboard_button(text: @cluster.cluster_name, title: s_('ClusterIntegration|Copy cluster name'))
+          = clipboard_button(text: @cluster.gcp_cluster_name, title: s_('ClusterIntegration|Copy cluster name'))
 
     %br
     -# - if can?(current_user, :admin_cluster, @cluster)

app/workers/cluster_creation_worker.rb

@@ -3,44 +3,8 @@ class ClusterCreationWorker
   include DedicatedSidekiqQueue
 
   def perform(cluster_id)
-    cluster = Gcp::Cluster.find_by_id(cluster_id)
-
-    unless cluster
-      return Rails.logger.error "Cluster object is not found; #{cluster_id}"
-    end
-
-    api_client =
-      GoogleApi::CloudPlatform::Client.new(cluster.gcp_token, nil)
-
-    operation = api_client.projects_zones_clusters_create(
-        cluster.gcp_project_id,
-        cluster.cluster_zone,
-        cluster.cluster_name,
-        cluster.cluster_size,
-        machine_type: cluster.machine_type
-      )
-
-    if operation.is_a?(StandardError)
-      return cluster.errored!("Failed to request to CloudPlatform; #{operation.message}")
-    end
-      
-    unless operation.status == 'RUNNING' || operation.status == 'PENDING'
-      return cluster.errored!("Operation status is unexpected; #{operation.status_message}")
-    end
-
-    operation_id = api_client.parse_operation_id(operation.self_link)
-
-    unless operation_id
-      return cluster.errored!('Can not find operation_id from self_link')
-    end
-
-    if cluster.creating!(operation_id)
-      WaitForClusterCreationWorker.perform_in(
-        WaitForClusterCreationWorker::INITIAL_INTERVAL,
-        cluster.id
-      )
-    else
-      return cluster.errored!("Failed to update cluster record; #{cluster.errors}")
+    Gcp::Cluster.find_by_id(cluster_id).try do |cluster|
+      Ci::CreateGkeClusterService.new.execute(cluster)
     end
   end
 end

app/workers/wait_for_cluster_creation_worker.rb

@@ -7,66 +7,21 @@ class WaitForClusterCreationWorker
   TIMEOUT = 20.minutes
 
   def perform(cluster_id)
-    cluster = Gcp::Cluster.find_by_id(cluster_id)
-
-    unless cluster
-      return Rails.logger.error "Cluster object is not found; #{cluster_id}"
-    end
-
-    api_client = 
-      GoogleApi::CloudPlatform::Client.new(cluster.gcp_token, nil)
-
-    operation = api_client.projects_zones_operations(
-      cluster.gcp_project_id,
-      cluster.cluster_zone,
-      cluster.gcp_operation_id)
-
-    if operation.is_a?(StandardError)
-      return cluster.errored!("Failed to request to CloudPlatform; #{operation.message}")
-    end
-
-    case operation.status
-    when 'RUNNING'
-      if Time.now < operation.start_time.to_time + TIMEOUT
-        WaitForClusterCreationWorker.perform_in(EAGER_INTERVAL, cluster.id)
-      else
-        return cluster.errored!("Cluster creation time exceeds timeout; #{TIMEOUT}")
+    Gcp::Cluster.find_by_id(cluster_id).try do |cluster|
+      Ci::FetchGcpOperationService.new.execute(cluster) do |operation|
+        case operation.status
+        when 'RUNNING'
+          if TIMEOUT < Time.now - operation.start_time.to_time
+            return cluster.errored!("Cluster creation time exceeds timeout; #{TIMEOUT}")
+          end
+
+          WaitForClusterCreationWorker.perform_in(EAGER_INTERVAL, cluster.id)
+        when 'DONE'
+          Ci::FinalizeClusterCreationService.new.execute(cluster)
+        else
+          return cluster.errored!("Unexpected operation status; #{operation.status} #{operation.status_message}")
+        end
       end
-    when 'DONE'
-      integrate(cluster, api_client)
-    else
-      return cluster.errored!("Unexpected operation status; #{operation.status} #{operation.status_message}")
     end
   end
-
-  def integrate(cluster, api_client)
-    gke_cluster = api_client.projects_zones_clusters_get(
-      cluster.gcp_project_id,
-      cluster.cluster_zone,
-      cluster.cluster_name)
-
-    if gke_cluster.is_a?(StandardError)
-      return cluster.errored!("Failed to request to CloudPlatform; #{gke_cluster.message}")
-    end
-
-    begin
-      endpoint = gke_cluster.endpoint
-      api_url = 'https://' + endpoint
-      ca_cert = Base64.decode64(gke_cluster.master_auth.cluster_ca_certificate)
-      username = gke_cluster.master_auth.username
-      password = gke_cluster.master_auth.password
-    rescue Exception => e
-      return cluster.errored!("Can not extract the expected data; #{e}")
-    end
-
-    kubernetes_token = Ci::FetchKubernetesTokenService.new(
-      api_url, ca_cert, username, password).execute
-
-    unless kubernetes_token
-      return cluster.errored!('Failed to get a default token of kubernetes')
-    end
-
-    Ci::IntegrateClusterService.new.execute(
-      cluster, endpoint, ca_cert, kubernetes_token, username, password)
-  end
 end

config/routes/project.rb

@@ -183,7 +183,7 @@ constraints(ProjectUrlConstrainer.new) do
         end
       end
 
-      resources :clusters, except: [:show] do
+      resources :clusters, except: [:edit] do
         collection do
           get :login
         end

db/migrate/20170924094327_create_gcp_clusters.rb

@@ -19,22 +19,19 @@ class CreateGcpClusters < ActiveRecord::Migration
       t.string :endpoint
       t.text :ca_cert
       t.string :encrypted_kubernetes_token
-      t.string :encrypted_kubernetes_token_salt
       t.string :encrypted_kubernetes_token_iv
       t.string :username
       t.string :encrypted_password
-      t.string :encrypted_password_salt
       t.string :encrypted_password_iv
 
       # GKE
       t.string :gcp_project_id, null: false
-      t.string :cluster_zone, null: false
-      t.string :cluster_name, null: false
-      t.integer :cluster_size, null: false
-      t.string :machine_type
+      t.string :gcp_cluster_zone, null: false
+      t.string :gcp_cluster_name, null: false
+      t.integer :gcp_cluster_size, null: false
+      t.string :gcp_machine_type
       t.string :gcp_operation_id
       t.string :encrypted_gcp_token
-      t.string :encrypted_gcp_token_salt
       t.string :encrypted_gcp_token_iv
 
       t.datetime_with_timezone :created_at, null: false

db/schema.rb

@@ -267,38 +267,6 @@ ActiveRecord::Schema.define(version: 20170928100231) do
   add_index "ci_builds", ["updated_at"], name: "index_ci_builds_on_updated_at", using: :btree
   add_index "ci_builds", ["user_id"], name: "index_ci_builds_on_user_id", using: :btree
 
-  create_table "ci_clusters", force: :cascade do |t|
-    t.integer "project_id", null: false
-    t.integer "user_id", null: false
-    t.integer "service_id"
-    t.boolean "enabled", default: true
-    t.integer "status"
-    t.string "status_reason"
-    t.string "project_namespace"
-    t.string "endpoint"
-    t.text "ca_cert"
-    t.string "encrypted_kubernetes_token"
-    t.string "encrypted_kubernetes_token_salt"
-    t.string "encrypted_kubernetes_token_iv"
-    t.string "username"
-    t.string "encrypted_password"
-    t.string "encrypted_password_salt"
-    t.string "encrypted_password_iv"
-    t.string "gcp_project_id", null: false
-    t.string "cluster_zone", null: false
-    t.string "cluster_name", null: false
-    t.integer "cluster_size", null: false
-    t.string "machine_type"
-    t.string "gcp_operation_id"
-    t.string "encrypted_gcp_token"
-    t.string "encrypted_gcp_token_salt"
-    t.string "encrypted_gcp_token_iv"
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
-  end
-
-  add_index "ci_clusters", ["project_id"], name: "index_ci_clusters_on_project_id", unique: true, using: :btree
-
   create_table "ci_group_variables", force: :cascade do |t|
     t.string "key", null: false
     t.text "value"
@@ -619,20 +587,17 @@ ActiveRecord::Schema.define(version: 20170928100231) do
     t.string "endpoint"
     t.text "ca_cert"
     t.string "encrypted_kubernetes_token"
-    t.string "encrypted_kubernetes_token_salt"
     t.string "encrypted_kubernetes_token_iv"
     t.string "username"
     t.string "encrypted_password"
-    t.string "encrypted_password_salt"
     t.string "encrypted_password_iv"
     t.string "gcp_project_id", null: false
-    t.string "cluster_zone", null: false
-    t.string "cluster_name", null: false
-    t.integer "cluster_size", null: false
-    t.string "machine_type"
+    t.string "gcp_cluster_zone", null: false
+    t.string "gcp_cluster_name", null: false
+    t.integer "gcp_cluster_size", null: false
+    t.string "gcp_machine_type"
     t.string "gcp_operation_id"
     t.string "encrypted_gcp_token"
-    t.string "encrypted_gcp_token_salt"
     t.string "encrypted_gcp_token_iv"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
@@ -1749,9 +1714,6 @@ ActiveRecord::Schema.define(version: 20170928100231) do
   add_foreign_key "ci_builds", "ci_pipelines", column: "auto_canceled_by_id", name: "fk_a2141b1522", on_delete: :nullify
   add_foreign_key "ci_builds", "ci_stages", column: "stage_id", name: "fk_3a9eaa254d", on_delete: :cascade
   add_foreign_key "ci_builds", "projects", name: "fk_befce0568a", on_delete: :cascade
-  add_foreign_key "ci_clusters", "projects", on_delete: :cascade
-  add_foreign_key "ci_clusters", "services"
-  add_foreign_key "ci_clusters", "users"
   add_foreign_key "ci_group_variables", "namespaces", column: "group_id", name: "fk_33ae4d58d8", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedule_variables", "ci_pipeline_schedules", column: "pipeline_schedule_id", name: "fk_41c35fda51", on_delete: :cascade
   add_foreign_key "ci_pipeline_schedules", "projects", name: "fk_8ead60fcc4", on_delete: :cascade

lib/google_api/cloud_platform/client.rb

@@ -3,7 +3,8 @@ require 'google/apis/container_v1'
 module GoogleApi
   module CloudPlatform
     class Client < GoogleApi::Auth
-      DEFAULT_MACHINE_TYPE = 'n1-standard-1'
+      DEFAULT_MACHINE_TYPE = 'n1-standard-1'.freeze
+      SCOPE = 'https://www.googleapis.com/auth/cloud-platform'.freeze
 
       class << self
         def session_key_for_token
@@ -16,7 +17,7 @@ module GoogleApi
       end
 
       def scope
-        'https://www.googleapis.com/auth/cloud-platform'
+        SCOPE
       end
 
       def validate_token(expires_at)
@@ -35,14 +36,7 @@ module GoogleApi
         service = Google::Apis::ContainerV1::ContainerService.new
         service.authorization = access_token
 
-        begin
-          cluster = service.get_zone_cluster(project_id, zone, cluster_id)
-        rescue Google::Apis::ServerError, Google::Apis::ClientError, Google::Apis::AuthorizationError => e
-          return e
-        end
-
-        puts "#{self.class.name} - #{__callee__}: cluster: #{cluster.inspect}"
-        cluster
+        service.get_zone_cluster(project_id, zone, cluster_id)
       end
 
       def projects_zones_clusters_create(project_id, zone, cluster_name, cluster_size, machine_type:)
@@ -61,28 +55,14 @@ module GoogleApi
             }
           )
 
-        begin
-          operation = service.create_cluster(project_id, zone, request_body)
-        rescue Google::Apis::ServerError, Google::Apis::ClientError, Google::Apis::AuthorizationError => e
-          return e
-        end
-
-        puts "#{self.class.name} - #{__callee__}: operation: #{operation.inspect}"
-        operation
+        service.create_cluster(project_id, zone, request_body)
       end
 
       def projects_zones_operations(project_id, zone, operation_id)
         service = Google::Apis::ContainerV1::ContainerService.new
         service.authorization = access_token
 
-        begin
-          operation = service.get_zone_operation(project_id, zone, operation_id)
-        rescue Google::Apis::ClientError, Google::Apis::AuthorizationError => e
-          return e
-        end
-
-        puts "#{self.class.name} - #{__callee__}: operation: #{operation.inspect}"
-        operation
+        service.get_zone_operation(project_id, zone, operation_id)
       end
 
       def parse_operation_id(self_link)