authorize in controller. validation in model.

9 files changed, 69 insertions, 41 deletions

diff --git a/app/controllers/projects/clusters_controller.rb b/app/controllers/projects/clusters_controller.rb
index 840c623e778..ebb17bca010 100644
--- a/app/controllers/projects/clusters_controller.rb
+++ b/app/controllers/projects/clusters_controller.rb
@@ -1,8 +1,7 @@
 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 :cluster_creation_lock, only: [:update, :destroy]
-  # before_action :authorize_admin_clusters! # TODO: Authentication
 
   def login
     begin
@@ -67,9 +66,7 @@ class Projects::ClustersController < Projects::ApplicationController
     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")
+      render :edit
     end
   end
 
@@ -94,12 +91,4 @@ class Projects::ClustersController < Projects::ApplicationController
   def token_in_session
     @token_in_session ||= session[GoogleApi::CloudPlatform::Client.session_key_for_token]
   end
-
-  def cluster_creation_lock
-    if cluster.on_creation?
-      redirect_to edit_project_cluster_path(project, cluster),
-                  status: :forbidden,
-                  alert: _("You can not modify cluster during creation")
-    end
-  end
 end
diff --git a/app/models/ci/cluster.rb b/app/models/ci/cluster.rb
index 594739075e4..93f582f565a 100644
--- a/app/models/ci/cluster.rb
+++ b/app/models/ci/cluster.rb
@@ -32,8 +32,26 @@ module Ci
       errored: 4
     }
 
-    def error!(reason)
-      update!(status: statuses[:errored], status_reason: reason, gcp_token: nil)
+    validates :gcp_project_id, presence: true
+    validates :cluster_zone, presence: true
+    validates :cluster_name, presence: true
+    validates :cluster_size, presence: true,
+              numericality: { only_integer: true, greater_than: 0 }
+    validate :restrict_modification, on: :update
+
+    def errored!(reason)
+      self.status = :errored
+      self.status_reason = reason
+      self.gcp_token = nil
+
+      save!(validate: false)
+    end
+
+    def creating!(gcp_operation_id)
+      self.status = :creating
+      self.gcp_operation_id = gcp_operation_id
+
+      save!(validate: false)
     end
 
     def on_creation?
@@ -43,5 +61,18 @@ module Ci
     def api_url
       'https://' + endpoint
     end
+
+    def restrict_modification
+      if on_creation?
+        errors.add(:base, "cannot modify during creation")
+        return false
+      end
+
+      true
+    end
+
+    def destroy
+      super if restrict_modification
+    end
   end
 end
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index b7b5bd34189..279b19eb576 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -188,6 +188,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_build
     enable :admin_container_image
     enable :admin_pipeline
+    enable :admin_cluster
     enable :admin_environment
     enable :admin_deployment
     enable :admin_pages
diff --git a/app/views/projects/clusters/_form.html.haml b/app/views/projects/clusters/_form.html.haml
index 4912abfb58f..d2a2bc0b6be 100644
--- a/app/views/projects/clusters/_form.html.haml
+++ b/app/views/projects/clusters/_form.html.haml
@@ -5,6 +5,7 @@
       = s_('ClusterIntegration|Use our %{link_to_help_page} on cluster integration.').html_safe % { link_to_help_page: link_to_help_page}
 
       = 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'
diff --git a/app/views/projects/clusters/edit.html.haml b/app/views/projects/clusters/edit.html.haml
index 7f8400d9ea1..fa5d14b514a 100644
--- a/app/views/projects/clusters/edit.html.haml
+++ b/app/views/projects/clusters/edit.html.haml
@@ -1,16 +1,23 @@
 edit/show cluster
 %br
 = @cluster.inspect
+%br
 = @cluster.service.inspect
-- unless @cluster.on_creation?
-  = link_to "Enable", namespace_project_cluster_path(@project.namespace, @project, @cluster.id, cluster: {enabled: 'true'}), method: :put
-  = link_to "Disable", namespace_project_cluster_path(@project.namespace, @project, @cluster.id, cluster: {enabled: 'false'}), method: :put
-  = link_to "Soft-delete the cluster", namespace_project_cluster_path(@project.namespace, @project, @cluster.id), method: :delete
+%br
+= form_errors(@cluster)
+%br
+= link_to "Enable", namespace_project_cluster_path(@project.namespace, @project, @cluster.id, cluster: {enabled: 'true'}), method: :put
+%br
+= link_to "Disable", namespace_project_cluster_path(@project.namespace, @project, @cluster.id, cluster: {enabled: 'false'}), method: :put
+%br
+= link_to "Soft-delete the cluster", namespace_project_cluster_path(@project.namespace, @project, @cluster.id), method: :delete
+%br
 -# status GET
 -# status: The current status of the operation.
 -# status_reason: If an error has occurred, a textual description of the error.
 = link_to 'Check status', status_namespace_project_cluster_path(@cluster.project.namespace, @cluster.project, @cluster.id), :remote => true
+%br
 -# simply rendering error, if it happened
-- if @cluster.status_reason
-  %p= status_reason
+status_reason
+%p= @cluster.status_reason
 -# Even if we got an error during the creation process, we don't delete cluster objects automatically, because we don't have a method to delete the cluster on gke. So users move to edit page from new page **regardless of validation errors**, and they have to delete the record manually. This is for giving users headsup that a new cluster might remain in gke. /cc @ayufan
diff --git a/app/workers/cluster_creation_worker.rb b/app/workers/cluster_creation_worker.rb
index 0b547089b94..40e40005022 100644
--- a/app/workers/cluster_creation_worker.rb
+++ b/app/workers/cluster_creation_worker.rb
@@ -21,27 +21,26 @@ class ClusterCreationWorker
       )
 
     if operation.is_a?(StandardError)
-      return cluster.error!("Failed to request to CloudPlatform; #{operation.message}")
+      return cluster.errored!("Failed to request to CloudPlatform; #{operation.message}")
     end
       
     unless operation.status == 'RUNNING' || operation.status == 'PENDING'
-      return cluster.error!("Operation status is unexpected; #{operation.status_message}")
+      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.error!('Can not find operation_id from self_link')
+      return cluster.errored!('Can not find operation_id from self_link')
     end
 
-    if cluster.update(status: Ci::Cluster.statuses[:creating],
-                      gcp_operation_id: operation_id)
+    if cluster.creating!(operation_id)
       WaitForClusterCreationWorker.perform_in(
         WaitForClusterCreationWorker::INITIAL_INTERVAL,
         cluster.id
       )
     else
-      return cluster.error!("Failed to update cluster record; #{cluster.errors}")
+      return cluster.errored!("Failed to update cluster record; #{cluster.errors}")
     end
   end
 end
diff --git a/app/workers/wait_for_cluster_creation_worker.rb b/app/workers/wait_for_cluster_creation_worker.rb
index 8d84f28c0cf..5ffe70d4ba6 100644
--- a/app/workers/wait_for_cluster_creation_worker.rb
+++ b/app/workers/wait_for_cluster_creation_worker.rb
@@ -22,7 +22,7 @@ class WaitForClusterCreationWorker
       cluster.gcp_operation_id)
 
     if operation.is_a?(StandardError)
-      return cluster.error!("Failed to request to CloudPlatform; #{operation.message}")
+      return cluster.errored!("Failed to request to CloudPlatform; #{operation.message}")
     end
 
     case operation.status
@@ -30,12 +30,12 @@ class WaitForClusterCreationWorker
       if Time.now < operation.start_time.to_time + TIMEOUT
         WaitForClusterCreationWorker.perform_in(EAGER_INTERVAL, cluster.id)
       else
-        return cluster.error!("Cluster creation time exceeds timeout; #{TIMEOUT}")
+        return cluster.errored!("Cluster creation time exceeds timeout; #{TIMEOUT}")
       end
     when 'DONE'
       integrate(cluster, api_client)
     else
-      return cluster.error!("Unexpected operation status; #{operation.status} #{operation.status_message}")
+      return cluster.errored!("Unexpected operation status; #{operation.status} #{operation.status_message}")
     end
   end
 
@@ -46,7 +46,7 @@ class WaitForClusterCreationWorker
       cluster.cluster_name)
 
     if gke_cluster.is_a?(StandardError)
-      return cluster.error!("Failed to request to CloudPlatform; #{gke_cluster.message}")
+      return cluster.errored!("Failed to request to CloudPlatform; #{gke_cluster.message}")
     end
 
     begin
@@ -56,14 +56,14 @@ class WaitForClusterCreationWorker
       username = gke_cluster.master_auth.username
       password = gke_cluster.master_auth.password
     rescue Exception => e
-      return cluster.error!("Can not extract the extected data; #{e}")
+      return cluster.errored!("Can not extract the extected data; #{e}")
     end
 
     kubernetes_token = Ci::FetchKubernetesTokenService.new(
       api_url, ca_cert, username, password).execute
 
     unless kubernetes_token
-      return cluster.error!('Failed to get a default token of kubernetes')
+      return cluster.errored!('Failed to get a default token of kubernetes')
     end
 
     Ci::IntegrateClusterService.new.execute(
diff --git a/db/migrate/20170924094327_create_ci_clusters.rb b/db/migrate/20170924094327_create_ci_clusters.rb
index 798c8e03b37..bef39b9808d 100644
--- a/db/migrate/20170924094327_create_ci_clusters.rb
+++ b/db/migrate/20170924094327_create_ci_clusters.rb
@@ -27,10 +27,10 @@ class CreateCiClusters < ActiveRecord::Migration
       t.string :encrypted_password_iv
 
       # GKE
-      t.string :gcp_project_id
-      t.string :cluster_zone
-      t.string :cluster_name
-      t.string :cluster_size
+      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
diff --git a/db/schema.rb b/db/schema.rb
index de844583d56..61245f3f666 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -284,10 +284,10 @@ ActiveRecord::Schema.define(version: 20170924094327) do
     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"
-    t.string "cluster_size"
+    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"