Merge branch '211998-add-cluster-mangement-id-on-create' into 'master'

Add management_project_id to group and project cluster creation

See merge request gitlab-org/gitlab!28289

app/services/clusters/create_service.rb

@@ -23,6 +23,8 @@ module Clusters
         cluster.errors.add(:base, _('Instance does not support multiple Kubernetes clusters'))
       end
 
+      validate_management_project_permissions(cluster)
+
       return cluster if cluster.errors.present?
 
       cluster.tap do |cluster|
@@ -57,6 +59,11 @@ module Clusters
     def can_create_cluster?
       clusterable.clusters.empty?
     end
+
+    def validate_management_project_permissions(cluster)
+      Clusters::Management::ValidateManagementProjectPermissionsService.new(current_user)
+        .execute(cluster, params[:management_project_id])
+    end
   end
 end
 

app/services/clusters/management/validate_management_project_permissions_service.rb

+# frozen_string_literal: true
+
+module Clusters
+  module Management
+    class ValidateManagementProjectPermissionsService
+      attr_reader :current_user
+
+      def initialize(user = nil)
+        @current_user = user
+      end
+
+      def execute(cluster, management_project_id)
+        if management_project_id.present?
+          management_project = management_project_scope(cluster).find_by_id(management_project_id)
+
+          unless management_project && can_admin_pipeline_for_project?(management_project)
+            cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
+
+            return false
+          end
+        end
+
+        true
+      end
+
+      private
+
+      def can_admin_pipeline_for_project?(project)
+        Ability.allowed?(current_user, :admin_pipeline, project)
+      end
+
+      def management_project_scope(cluster)
+        return ::Project.all if cluster.instance_type?
+
+        group =
+          if cluster.group_type?
+            cluster.first_group
+          elsif cluster.project_type?
+            cluster.first_project&.namespace
+          end
+
+        # Prevent users from selecting nested projects until
+        # https://gitlab.com/gitlab-org/gitlab/issues/34650 is resolved
+        include_subgroups = cluster.group_type?
+
+        ::GroupProjectsFinder.new(
+          group: group,
+          current_user: current_user,
+          options: { only_owned: true, include_subgroups: include_subgroups }
+        ).execute
+      end
+    end
+  end
+end

app/services/clusters/update_service.rb

@@ -18,46 +18,9 @@ module Clusters
 
     private
 
-    def can_admin_pipeline_for_project?(project)
-      Ability.allowed?(current_user, :admin_pipeline, project)
-    end
-
     def validate_params(cluster)
-      if params[:management_project_id].present?
-        management_project = management_project_scope(cluster).find_by_id(params[:management_project_id])
-
-        unless management_project
-          cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
-
-          return false
-        end
-
-        unless can_admin_pipeline_for_project?(management_project)
-          # Use same message as not found to prevent enumeration
-          cluster.errors.add(:management_project_id, _('Project does not exist or you don\'t have permission to perform this action'))
-
-          return false
-        end
-      end
-
-      true
-    end
-
-    def management_project_scope(cluster)
-      return ::Project.all if cluster.instance_type?
-
-      group =
-        if cluster.group_type?
-          cluster.first_group
-        elsif cluster.project_type?
-          cluster.first_project&.namespace
-        end
-
-      # Prevent users from selecting nested projects until
-      # https://gitlab.com/gitlab-org/gitlab/issues/34650 is resolved
-      include_subgroups = cluster.group_type?
-
-      ::GroupProjectsFinder.new(group: group, current_user: current_user, options: { only_owned: true, include_subgroups: include_subgroups }).execute
+      ::Clusters::Management::ValidateManagementProjectPermissionsService.new(current_user)
+        .execute(cluster, params[:management_project_id])
     end
   end
 end

changelogs/unreleased/211998-add-cluster-mangement-id-on-create.yml

+---
+title: Add management_project_id to group and project cluster creation, clarifies
+  docs.
+merge_request: 28289
+author:
+type: fixed

doc/api/group_clusters.md

@@ -224,6 +224,7 @@ Parameters:
 | `cluster_id` | integer | yes | The ID of the cluster |
 | `name` | string | no | The name of the cluster |
 | `domain` | string | no | The [base domain](../user/group/clusters/index.md#base-domain) of the cluster |
+| `management_project_id` | integer | no | The ID of the [management project](../user/clusters/management_project.md) for the cluster |
 | `platform_kubernetes_attributes[api_url]` | string | no | The URL to access the Kubernetes API |
 | `platform_kubernetes_attributes[token]` | string | no | The token to authenticate against Kubernetes |
 | `platform_kubernetes_attributes[ca_cert]` | string | no | TLS certificate. Required if API is using a self-signed TLS certificate. |

doc/api/project_clusters.md

@@ -179,6 +179,7 @@ Parameters:
 | `id` | integer | yes | The ID of the project owned by the authenticated user |
 | `name` | string | yes | The name of the cluster |
 | `domain` | string | no | The [base domain](../user/project/clusters/index.md#base-domain) of the cluster |
+| `management_project_id` | integer | no | The ID of the [management project](../user/clusters/management_project.md) for the cluster |
 | `enabled` | boolean | no | Determines if cluster is active or not, defaults to true |
 | `managed` | boolean | no | Determines if GitLab will manage namespaces and service accounts for this cluster, defaults to true |
 | `platform_kubernetes_attributes[api_url]` | string | yes | The URL to access the Kubernetes API |

lib/api/group_clusters.rb

@@ -53,6 +53,7 @@ module API
         requires :name, type: String, desc: 'Cluster name'
         optional :enabled, type: Boolean, default: true, desc: 'Determines if cluster is active or not, defaults to true'
         optional :domain, type: String, desc: 'Cluster base domain'
+        optional :management_project_id, type: Integer, desc: 'The ID of the management project'
         optional :managed, type: Boolean, default: true, desc: 'Determines if GitLab will manage namespaces and service accounts for this cluster, defaults to true'
         requires :platform_kubernetes_attributes, type: Hash, desc: %q(Platform Kubernetes data) do
           requires :api_url, type: String, allow_blank: false, desc: 'URL to access the Kubernetes API'

lib/api/project_clusters.rb

@@ -56,6 +56,7 @@ module API
         requires :name, type: String, desc: 'Cluster name'
         optional :enabled, type: Boolean, default: true, desc: 'Determines if cluster is active or not, defaults to true'
         optional :domain, type: String, desc: 'Cluster base domain'
+        optional :management_project_id, type: Integer, desc: 'The ID of the management project'
         optional :managed, type: Boolean, default: true, desc: 'Determines if GitLab will manage namespaces and service accounts for this cluster, defaults to true'
         requires :platform_kubernetes_attributes, type: Hash, desc: %q(Platform Kubernetes data) do
           requires :api_url, type: String, allow_blank: false, desc: 'URL to access the Kubernetes API'

spec/requests/api/group_clusters_spec.rb

@@ -157,6 +157,7 @@ describe API::GroupClusters do
 
     let(:api_url) { 'https://kubernetes.example.com' }
     let(:authorization_type) { 'rbac' }
+    let(:management_project_id) { create(:project, group: group).id }
 
     let(:platform_kubernetes_attributes) do
       {
@@ -171,7 +172,8 @@ describe API::GroupClusters do
         name: 'test-cluster',
         domain: 'domain.example.com',
         managed: false,
-        platform_kubernetes_attributes: platform_kubernetes_attributes
+        platform_kubernetes_attributes: platform_kubernetes_attributes,
+        management_project_id: management_project_id
       }
     end
 
@@ -203,6 +205,7 @@ describe API::GroupClusters do
           expect(cluster_result.name).to eq('test-cluster')
           expect(cluster_result.domain).to eq('domain.example.com')
           expect(cluster_result.managed).to be_falsy
+          expect(cluster_result.management_project_id).to eq management_project_id
           expect(platform_kubernetes.rbac?).to be_truthy
           expect(platform_kubernetes.api_url).to eq(api_url)
           expect(platform_kubernetes.token).to eq('sample-token')
@@ -234,6 +237,18 @@ describe API::GroupClusters do
         end
       end
 
+      context 'current user does not have access to management_project_id' do
+        let(:management_project_id) { create(:project).id }
+
+        it 'responds with 400' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'returns validation errors' do
+          expect(json_response['message']['management_project_id'].first).to match('don\'t have permission')
+        end
+      end
+
       context 'with invalid params' do
         let(:api_url) { 'invalid_api_url' }
 

spec/requests/api/project_clusters_spec.rb

@@ -150,6 +150,12 @@ describe API::ProjectClusters do
     let(:api_url) { 'https://kubernetes.example.com' }
     let(:namespace) { project.path }
     let(:authorization_type) { 'rbac' }
+    let(:management_project) { create(:project, namespace: project.namespace) }
+    let(:management_project_id) { management_project.id }
+
+    before do
+      management_project.add_maintainer(current_user)
+    end
 
     let(:platform_kubernetes_attributes) do
       {
@@ -165,7 +171,8 @@ describe API::ProjectClusters do
         name: 'test-cluster',
         domain: 'domain.example.com',
         managed: false,
-        platform_kubernetes_attributes: platform_kubernetes_attributes
+        platform_kubernetes_attributes: platform_kubernetes_attributes,
+        management_project_id: management_project_id
       }
     end
 
@@ -194,6 +201,7 @@ describe API::ProjectClusters do
           expect(cluster_result.name).to eq('test-cluster')
           expect(cluster_result.domain).to eq('domain.example.com')
           expect(cluster_result.managed).to be_falsy
+          expect(cluster_result.management_project_id).to eq management_project_id
           expect(platform_kubernetes.rbac?).to be_truthy
           expect(platform_kubernetes.api_url).to eq(api_url)
           expect(platform_kubernetes.namespace).to eq(namespace)
@@ -227,6 +235,18 @@ describe API::ProjectClusters do
         end
       end
 
+      context 'current user does not have access to management_project_id' do
+        let(:management_project_id) { create(:project).id }
+
+        it 'responds with 400' do
+          expect(response).to have_gitlab_http_status(:bad_request)
+        end
+
+        it 'returns validation errors' do
+          expect(json_response['message']['management_project_id'].first).to match('don\'t have permission')
+        end
+      end
+
       context 'with invalid params' do
         let(:namespace) { 'invalid_namespace' }
 

spec/services/clusters/create_service_spec.rb

@@ -59,4 +59,92 @@ describe Clusters::CreateService do
       end
     end
   end
+
+  context 'when params includes :management_project_id' do
+    subject(:cluster) { described_class.new(user, params).execute(access_token: access_token) }
+
+    let(:params) do
+      {
+        name: 'test-cluster',
+        provider_type: :gcp,
+        provider_gcp_attributes: {
+          gcp_project_id: 'gcp-project',
+          zone: 'us-central1-a',
+          num_nodes: 1,
+          machine_type: 'machine_type-a',
+          legacy_abac: 'true'
+        },
+        clusterable: clusterable,
+        management_project_id: management_project_id
+      }
+    end
+
+    let(:clusterable) { project }
+    let(:management_project_id) { management_project.id }
+    let(:management_project_namespace) { project.namespace }
+    let(:management_project) { create(:project, namespace: management_project_namespace) }
+
+    shared_examples 'invalid project or cluster permissions' do
+      it 'does not persist the cluster and adds errors' do
+        expect(cluster).not_to be_persisted
+
+        expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+      end
+    end
+
+    shared_examples 'setting a management project' do
+      context 'when user is authorized to adminster manangement_project' do
+        before do
+          management_project.add_maintainer(user)
+        end
+
+        it 'persists the cluster' do
+          expect(cluster).to be_persisted
+
+          expect(cluster.management_project).to eq(management_project)
+        end
+      end
+
+      context 'when user is not authorized to adminster manangement_project' do
+        include_examples 'invalid project or cluster permissions'
+      end
+    end
+
+    shared_examples 'setting a management project outside of scope' do
+      context 'when manangement_project is outside of the namespace scope' do
+        let(:management_project_namespace) { create(:group) }
+
+        it 'does not persist the cluster' do
+          expect(cluster).not_to be_persisted
+
+          expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+        end
+      end
+    end
+
+    context 'management_project is non-existent' do
+      let(:management_project_id) { 0 }
+
+      include_examples 'invalid project or cluster permissions'
+    end
+
+    context 'project cluster' do
+      include_examples 'setting a management project'
+      include_examples 'setting a management project outside of scope'
+    end
+
+    context 'group cluster' do
+      let(:management_project_namespace) { create(:group) }
+      let(:clusterable) { management_project_namespace }
+
+      include_examples 'setting a management project'
+      include_examples 'setting a management project outside of scope'
+    end
+
+    context 'instance cluster' do
+      let(:clusterable) { Clusters::Instance.new }
+
+      include_examples 'setting a management project'
+    end
+  end
 end

spec/services/clusters/management/validate_management_project_permissions_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Clusters::Management::ValidateManagementProjectPermissionsService do
+  describe '#execute' do
+    subject { described_class.new(user).execute(cluster, management_project_id) }
+
+    let(:cluster) { build(:cluster, :project, projects: [create(:project)]) }
+    let(:user) { create(:user) }
+
+    context 'when management_project_id is nil' do
+      let(:management_project_id) { nil }
+
+      it { is_expected.to be true }
+    end
+
+    context 'when management_project_id is not nil' do
+      let(:management_project_id) { management_project.id }
+      let(:management_project_namespace) { create(:group) }
+      let(:management_project) { create(:project, namespace: management_project_namespace) }
+
+      context 'when management_project does not exist' do
+        let(:management_project_id) { 0 }
+
+        it 'adds errors to the cluster and returns false' do
+          is_expected.to eq false
+
+          expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+        end
+      end
+
+      shared_examples 'management project is in scope' do
+        context 'when user is authorized to administer manangement_project' do
+          before do
+            management_project.add_maintainer(user)
+          end
+
+          it 'adds no error and returns true' do
+            is_expected.to eq true
+
+            expect(cluster.errors).to be_empty
+          end
+        end
+
+        context 'when user is not authorized to adminster manangement_project' do
+          it 'adds an error and returns false' do
+            is_expected.to eq false
+
+            expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+          end
+        end
+      end
+
+      shared_examples 'management project is out of scope' do
+        context 'when manangement_project is outside of the namespace scope' do
+          let(:management_project_namespace) { create(:group) }
+
+          it 'adds an error and returns false' do
+            is_expected.to eq false
+
+            expect(cluster.errors[:management_project_id]).to include('Project does not exist or you don\'t have permission to perform this action')
+          end
+        end
+      end
+
+      context 'project cluster' do
+        let(:cluster) { build(:cluster, :project, projects: [create(:project, namespace: management_project_namespace)]) }
+
+        include_examples 'management project is in scope'
+        include_examples 'management project is out of scope'
+      end
+
+      context 'group cluster' do
+        let(:cluster) { build(:cluster, :group, groups: [management_project_namespace]) }
+
+        include_examples 'management project is in scope'
+        include_examples 'management project is out of scope'
+      end
+
+      context 'instance cluster' do
+        let(:cluster) { build(:cluster, :instance) }
+
+        include_examples 'management project is in scope'
+      end
+    end
+  end
+end