Refactor: model errors for multi cluster validation

The current approach requires catching exceptions to handle these errors
and callers are already handling model validations so it seems more
appropriate.  Also it seemed to convoluted to add this logic directly to
the model since the model needs to check too many possible associations
to determine whether or not there are more than one cluster since the
model doesn't know what it's being created on. Additionally we only
wanted to validate during create to avoid the risk of existing models
becoming invalid by many different edge cases.

app/policies/clusters/instance_policy.rb

@@ -2,11 +2,6 @@
 
 module Clusters
   class InstancePolicy < BasePolicy
-    include ClusterableActions
-
-    condition(:has_clusters, scope: :subject) { clusterable_has_clusters? }
-    condition(:can_have_multiple_clusters) { multiple_clusters_available? }
-
     rule { admin }.policy do
       enable :read_cluster
       enable :add_cluster
@@ -14,7 +9,5 @@ module Clusters
       enable :update_cluster
       enable :admin_cluster
     end
-
-    rule { ~can_have_multiple_clusters & has_clusters }.prevent :add_cluster
   end
 end

app/policies/concerns/clusterable_actions.rb

-# frozen_string_literal: true
-
-module ClusterableActions
-  private
-
-  # Overridden on EE module
-  def multiple_clusters_available?
-    false
-  end
-
-  def clusterable_has_clusters?
-    !subject.clusters.empty?
-  end
-end

app/policies/group_policy.rb

 # frozen_string_literal: true
 
 class GroupPolicy < BasePolicy
-  include ClusterableActions
-
   desc "Group is public"
   with_options scope: :subject, score: 0
   condition(:public_group) { @subject.public? }
@@ -29,9 +27,6 @@ class GroupPolicy < BasePolicy
     GroupProjectsFinder.new(group: @subject, current_user: @user, options: { include_subgroups: true, only_owned: true }).execute.any?
   end
 
-  condition(:has_clusters, scope: :subject) { clusterable_has_clusters? }
-  condition(:can_have_multiple_clusters) { multiple_clusters_available? }
-
   with_options scope: :subject, score: 0
   condition(:request_access_enabled) { @subject.request_access_enabled }
 
@@ -121,8 +116,6 @@ class GroupPolicy < BasePolicy
 
   rule { owner & (~share_with_group_locked | ~has_parent | ~parent_share_with_group_locked | can_change_parent_share_with_group_lock) }.enable :change_share_with_group_lock
 
-  rule { ~can_have_multiple_clusters & has_clusters }.prevent :add_cluster
-
   rule { developer & developer_maintainer_access }.enable :create_projects
   rule { create_projects_disabled }.prevent :create_projects
 

app/policies/project_policy.rb

@@ -2,7 +2,6 @@
 
 class ProjectPolicy < BasePolicy
   extend ClassMethods
-  include ClusterableActions
 
   READONLY_FEATURES_WHEN_ARCHIVED = %i[
     issue
@@ -114,9 +113,6 @@ class ProjectPolicy < BasePolicy
     @subject.feature_available?(:merge_requests, @user)
   end
 
-  condition(:has_clusters, scope: :subject) { clusterable_has_clusters? }
-  condition(:can_have_multiple_clusters) { multiple_clusters_available? }
-
   condition(:internal_builds_disabled) do
     !@subject.builds_enabled?
   end
@@ -430,8 +426,6 @@ class ProjectPolicy < BasePolicy
     (~guest & can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request)
   end.enable :read_merge_request_iid
 
-  rule { ~can_have_multiple_clusters & has_clusters }.prevent :add_cluster
-
   rule { ~can?(:read_cross_project) & ~classification_label_authorized }.policy do
     # Preventing access here still allows the projects to be listed. Listing
     # projects doesn't check the `:read_project` ability. But instead counts

app/presenters/clusterable_presenter.rb

@@ -13,7 +13,8 @@ class ClusterablePresenter < Gitlab::View::Presenter::Delegated
   end
 
   def can_add_cluster?
-    can?(current_user, :add_cluster, clusterable)
+    can?(current_user, :add_cluster, clusterable) &&
+      (has_no_clusters? || multiple_clusters_available?)
   end
 
   def can_create_cluster?
@@ -63,4 +64,15 @@ class ClusterablePresenter < Gitlab::View::Presenter::Delegated
   def learn_more_link
     raise NotImplementedError
   end
+
+  private
+
+  # Overridden on EE module
+  def multiple_clusters_available?
+    false
+  end
+
+  def has_no_clusters?
+    clusterable.clusters.empty?
+  end
 end

app/services/clusters/create_service.rb

@@ -10,23 +10,26 @@ module Clusters
 
     def execute(access_token: nil)
       raise ArgumentError, 'Unknown clusterable provided' unless clusterable
-      raise ArgumentError, _('Instance does not support multiple Kubernetes clusters') unless can_create_cluster?
 
       cluster_params = params.merge(user: current_user).merge(clusterable_params)
       cluster_params[:provider_gcp_attributes].try do |provider|
         provider[:access_token] = access_token
       end
 
-      create_cluster(cluster_params).tap do |cluster|
-        ClusterProvisionWorker.perform_async(cluster.id) if cluster.persisted?
-      end
+      cluster = Clusters::Cluster.new(cluster_params)
+
+      unless can_create_cluster?
+        cluster.errors.add(:base, _('Instance does not support multiple Kubernetes clusters'))
       end
 
-    private
+      return cluster if cluster.errors.present?
 
-    def create_cluster(cluster_params)
-      Clusters::Cluster.create(cluster_params)
+      cluster.tap do |cluster|
+        cluster.save && ClusterProvisionWorker.perform_async(cluster.id)
       end
+    end
+
+    private
 
     def clusterable
       @clusterable ||= params.delete(:clusterable)

lib/api/project_clusters.rb

@@ -65,7 +65,7 @@ module API
         use :create_params_ee
       end
       post ':id/clusters/user' do
-        authorize! :add_cluster, user_project, 'Instance does not support multiple Kubernetes clusters'
+        authorize! :add_cluster, user_project
 
         user_cluster = ::Clusters::CreateService
           .new(current_user, create_cluster_user_params)

spec/requests/api/project_clusters_spec.rb

@@ -257,12 +257,22 @@ describe API::ProjectClusters do
         post api("/projects/#{project.id}/clusters/user", current_user), params: cluster_params
       end
 
+      it 'responds with 400' do
+        expect(response).to have_gitlab_http_status(400)
+
+        expect(json_response['message']['base'].first).to eq('Instance does not support multiple Kubernetes clusters')
+      end
+    end
+
+    context 'non-authorized user' do
+      before do
+        post api("/projects/#{project.id}/clusters/user", developer_user), params: cluster_params
+      end
+
       it 'responds with 403' do
         expect(response).to have_gitlab_http_status(403)
-      end
 
-      it 'returns an appropriate message' do
-        expect(json_response['message']).to include('Instance does not support multiple Kubernetes clusters')
+        expect(json_response['message']).to eq('403 Forbidden')
       end
     end
   end

spec/support/shared_examples/policies/clusterable_shared_examples.rb

@@ -24,14 +24,6 @@ shared_examples 'clusterable policies' do
       context 'with no clusters' do
         it { expect_allowed(:add_cluster) }
       end
-
-      context 'with an existing cluster' do
-        before do
-          cluster
-        end
-
-        it { expect_disallowed(:add_cluster) }
-      end
     end
   end
 end