Databse foreing key, index, encrypt password. Use short path. Improve error handling. Polish.

7 files changed, 110 insertions, 55 deletions

diff --git a/app/controllers/google_api/authorizations_controller.rb b/app/controllers/google_api/authorizations_controller.rb
index 1fafd7e88be..4b315181b7d 100644
--- a/app/controllers/google_api/authorizations_controller.rb
+++ b/app/controllers/google_api/authorizations_controller.rb
@@ -1,8 +1,6 @@
 module GoogleApi
   class AuthorizationsController < ApplicationController
-    # /google_api/authorizations/callback(.:format)
     def callback
-      # TODO: Error handling
       session[GoogleApi::CloudPlatform::Client.token_in_session] = 
         GoogleApi::Authentication.new(nil, callback_google_api_authorizations_url)
                                  .get_token(params[:code])
diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb
index f3dd55a6800..61bd28c65fe 100644
--- a/app/controllers/projects/clusters_controller.rb
+++ b/app/controllers/projects/clusters_controller.rb
@@ -7,14 +7,15 @@ class Projects::ClustersController < Projects::ApplicationController
     begin
       @authorize_url = api_client.authorize_url
     rescue GoogleApi::Authentication::ConfigMissingError
+      # Show an alert message that gitlab.yml is not configured properly
     end
   end
 
   def index
     if project.clusters.any?
-      redirect_to edit_namespace_project_cluster_path(project.namespace, project, project.clusters.last.id)
+      redirect_to edit_project_cluster_path(project, project.clusters.last.id)
     else
-      redirect_to action: 'new'
+      redirect_to new_project_cluster_path(project)
     end
   end
 
@@ -26,11 +27,11 @@ class Projects::ClustersController < Projects::ApplicationController
       Ci::CreateClusterService.new(project, current_user, params)
                               .create_cluster_on_gke(api_client)
     rescue Ci::CreateClusterService::UnexpectedOperationError => e
-      puts "#{self.class.name} - #{__callee__}: e: #{e}"
       # TODO: error
+      puts "#{self.class.name} - #{__callee__}: e: #{e}"
     end
 
-    redirect_to action: 'index'
+    redirect_to project_clusters_path(project)
   end
 
   ##
@@ -49,15 +50,36 @@ class Projects::ClustersController < Projects::ApplicationController
   end
 
   def update
-    cluster.update(enabled: params['enabled'])
-    cluster.service.update(active: params['enabled'])
-    # TODO: Do we overwrite KubernetesService parameter?
+    Ci::Cluster.transaction do
+      if params['enabled'] == 'true'
+
+        cluster.service.attributes = {
+          active: true,
+          api_url: cluster.endpoint,
+          ca_pem: cluster.ca_cert,
+          namespace: cluster.project_namespace,
+          token: cluster.token
+        }
+
+        cluster.service.save!
+      else
+        cluster.service.update(active: false)
+      end
+
+      cluster.update(enabled: params['enabled'])
+    end
+
     render :edit
   end
 
   def destroy
-    cluster.destroy
-    redirect_to action: 'index'
+    if cluster.destroy
+      redirect_to project_clusters_path(project), status: 302
+    else
+      redirect_to project_clusters_path(project),
+                  status: :forbidden,
+                  alert: _("Failed to remove the cluster")
+    end
   end
 
   private
diff --git a/app/models/ci/cluster.rb b/app/models/ci/cluster.rb
index f9a9d12d118..afb70a3ff4a 100644
--- a/app/models/ci/cluster.rb
+++ b/app/models/ci/cluster.rb
@@ -6,9 +6,15 @@ module Ci
     self.reactive_cache_key = ->(cluster) { [cluster.class.model_name.singular, cluster.project_id, cluster.id] }
 
     belongs_to :project
-    belongs_to :owner, class_name: 'User'
+    belongs_to :user
     belongs_to :service
 
+    attr_encrypted :password,
+       mode: :per_attribute_iv_and_salt,
+       insecure_mode: true,
+       key: Gitlab::Application.secrets.db_key_base,
+       algorithm: 'aes-256-cbc'
+
     # after_save :clear_reactive_cache!
 
     def creation_status(access_token)
@@ -26,12 +32,16 @@ module Ci
       api_client = GoogleApi::CloudPlatform::Client.new(access_token, nil)
       operation = api_client.projects_zones_operations(gcp_project_id, cluster_zone, gcp_operation_id)
 
-      if operation&.status == 'DONE'
+      return { status_message: 'Failed to get a status' } unless operation
+
+      if operation.status == 'DONE'
         # Get cluster details (end point, etc)
         gke_cluster = api_client.projects_zones_clusters_get(
           gcp_project_id, cluster_zone, cluster_name
         )
 
+        return { status_message: 'Failed to get a cluster info on gke' } unless gke_cluster
+
         # Get k8s token
         token = ''
         KubernetesService.new.tap do |ks|
@@ -50,34 +60,41 @@ module Ci
           end
         end
 
+        return { status_message: 'Failed to get a default token on kubernetes' } unless token
+
         # k8s endpoint, ca_cert
         endpoint = 'https://' + gke_cluster.endpoint
         cluster_ca_certificate = Base64.decode64(gke_cluster.master_auth.cluster_ca_certificate)
 
-        # Update service
-        kubernetes_service.attributes = {
-          active: true,
-          api_url: endpoint,
-          ca_pem: cluster_ca_certificate,
-          namespace: project_namespace,
-          token: token
-        }
+        begin
+          Ci::Cluster.transaction do
+            # Update service
+            kubernetes_service.attributes = {
+              active: true,
+              api_url: endpoint,
+              ca_pem: cluster_ca_certificate,
+              namespace: project_namespace,
+              token: token
+            }
 
-        kubernetes_service.save!
-
-        # Save info in cluster record
-        update(
-          enabled: true,
-          service: kubernetes_service,
-          username: gke_cluster.master_auth.username,
-          password: gke_cluster.master_auth.password,
-          token: token,
-          ca_cert: cluster_ca_certificate,
-          end_point: endpoint,
-        )
+            kubernetes_service.save!
+
+            # Save info in cluster record
+            update(
+              enabled: true,
+              service: kubernetes_service,
+              username: gke_cluster.master_auth.username,
+              password: gke_cluster.master_auth.password,
+              token: token,
+              ca_cert: cluster_ca_certificate,
+              endpoint: endpoint,
+            )
+          end
+        rescue ActiveRecord::RecordInvalid => exception
+          return { status_message: 'Failed to setup integration' }
+        end
       end
 
-      puts "#{self.class.name} - #{__callee__}: operation.to_json: #{operation.to_json}"
       operation.to_h
     end
 
diff --git a/app/services/ci/create_cluster_service.rb b/app/services/ci/create_cluster_service.rb
index bbf42ab2c8d..edae245ec38 100644
--- a/app/services/ci/create_cluster_service.rb
+++ b/app/services/ci/create_cluster_service.rb
@@ -10,11 +10,11 @@ module Ci
       )
 
       if operation&.status != ('RUNNING' || 'PENDING')
-        raise UnexpectedOperationError
+        raise UnexpectedOperationError.new(operation&.status_message)
       end
 
       api_client.parse_self_link(operation.self_link).tap do |project_id, zone, operation_id|
-        project.clusters.create(owner: current_user,
+        project.clusters.create(user: current_user,
                                 gcp_project_id: params['gcp_project_id'],
                                 cluster_zone: params['cluster_zone'],
                                 cluster_name: params['cluster_name'],
diff --git a/db/migrate/20170924094327_create_ci_clusters.rb b/db/migrate/20170924094327_create_ci_clusters.rb
index 33a67be46dc..2663130c7e6 100644
--- a/db/migrate/20170924094327_create_ci_clusters.rb
+++ b/db/migrate/20170924094327_create_ci_clusters.rb
@@ -3,9 +3,9 @@ class CreateCiClusters < ActiveRecord::Migration
 
   def up
     create_table :ci_clusters do |t|
-      t.integer :project_id
-      t.integer :owner_id
-      t.integer :service_id
+      t.references :project, null: false, index: { unique: true }, foreign_key: { on_delete: :cascade }
+      t.references :user, null: false, foreign_key: true
+      t.references :service, foreign_key: true
 
       # General
       t.boolean :enabled, default: true
@@ -14,11 +14,14 @@ class CreateCiClusters < ActiveRecord::Migration
       t.string :project_namespace
 
       # Cluster details
-      t.string :end_point
+      t.string :endpoint
       t.text :ca_cert
       t.string :token
       t.string :username
       t.string :password
+      t.string :encrypted_password
+      t.string :encrypted_password_salt
+      t.string :encrypted_password_iv
 
       # GKE
       t.string :gcp_project_id
@@ -29,12 +32,6 @@ class CreateCiClusters < ActiveRecord::Migration
       t.datetime_with_timezone :created_at, null: false
       t.datetime_with_timezone :updated_at, null: false
     end
-
-    # TODO: fk, index, attr_encrypted
-
-    add_foreign_key :ci_clusters, :projects
-    add_foreign_key :ci_clusters, :users, column: :owner_id
-    add_foreign_key :ci_clusters, :services
   end
 
   def down
diff --git a/db/schema.rb b/db/schema.rb
index 0ebce995cfd..af5367113a2 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -268,16 +268,19 @@ ActiveRecord::Schema.define(version: 20170924094327) do
   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"
-    t.integer "owner_id"
+    t.integer "project_id", null: false
+    t.integer "user_id", null: false
     t.integer "service_id"
     t.boolean "enabled", default: true
     t.string "project_namespace"
-    t.string "end_point"
+    t.string "endpoint"
     t.text "ca_cert"
     t.string "token"
     t.string "username"
     t.string "password"
+    t.string "encrypted_password"
+    t.string "encrypted_password_salt"
+    t.string "encrypted_password_iv"
     t.string "gcp_project_id"
     t.string "cluster_zone"
     t.string "cluster_name"
@@ -286,6 +289,8 @@ ActiveRecord::Schema.define(version: 20170924094327) do
     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"
@@ -1704,9 +1709,9 @@ ActiveRecord::Schema.define(version: 20170924094327) 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"
+  add_foreign_key "ci_clusters", "projects", on_delete: :cascade
   add_foreign_key "ci_clusters", "services"
-  add_foreign_key "ci_clusters", "users", column: "owner_id"
+  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
diff --git a/lib/google_api/cloud_platform/client.rb b/lib/google_api/cloud_platform/client.rb
index 61176e39464..0bc306a24e6 100644
--- a/lib/google_api/cloud_platform/client.rb
+++ b/lib/google_api/cloud_platform/client.rb
@@ -13,11 +13,22 @@ module GoogleApi
         'https://www.googleapis.com/auth/cloud-platform'
       end
 
+      ##
+      # Exception
+      # Google::Apis::ClientError:
+      # Google::Apis::AuthorizationError:
+      ##
+
       def projects_zones_clusters_get(project_id, zone, cluster_id)
         service = Google::Apis::ContainerV1::ContainerService.new
         service.authorization = access_token
 
-        cluster = service.get_zone_cluster(project_id, zone, cluster_id)
+        begin
+          cluster = service.get_zone_cluster(project_id, zone, cluster_id)
+        rescue Google::Apis::ClientError, Google::Apis::AuthorizationError => e
+          return nil
+        end
+
         puts "#{self.class.name} - #{__callee__}: cluster: #{cluster.inspect}"
         cluster
       end
@@ -40,9 +51,9 @@ module GoogleApi
         begin
           operation = service.create_cluster(project_id, zone, request_body)
         rescue Google::Apis::ClientError, Google::Apis::AuthorizationError => e
-          puts "#{self.class.name} - #{__callee__}: Could not create cluster #{cluster_name}: #{e}"
-          # TODO: Error
+          return nil
         end
+
         puts "#{self.class.name} - #{__callee__}: operation: #{operation.inspect}"
         operation
       end
@@ -51,7 +62,12 @@ module GoogleApi
         service = Google::Apis::ContainerV1::ContainerService.new
         service.authorization = access_token
 
-        operation = service.get_zone_operation(project_id, zone, operation_id)
+        begin
+          operation = service.get_zone_operation(project_id, zone, operation_id)
+        rescue Google::Apis::ClientError, Google::Apis::AuthorizationError => e
+          return nil
+        end
+
         puts "#{self.class.name} - #{__callee__}: operation: #{operation.inspect}"
         operation
       end