Merge branch '64689-add-instance-administrators-group' into '44496-save-project-id'

Create self-monitoring project within a group

See merge request gitlab-org/gitlab-ce!30948

4 files changed, 108 insertions, 33 deletions

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 9dbcef8abaa..cb6346421ec 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -10,6 +10,8 @@ class ApplicationSetting < ApplicationRecord
   add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption, default_enabled: true) ? :optional : :required }
   add_authentication_token_field :health_check_access_token
 
+  belongs_to :instance_administration_project, class_name: "Project"
+
   # Include here so it can override methods from
   # `add_authentication_token_field`
   # We don't prepend for now because otherwise we'll need to
diff --git a/app/services/self_monitoring/project/create_service.rb b/app/services/self_monitoring/project/create_service.rb
index 437a91ea18c..d211e399e11 100644
--- a/app/services/self_monitoring/project/create_service.rb
+++ b/app/services/self_monitoring/project/create_service.rb
@@ -5,16 +5,20 @@ module SelfMonitoring
     class CreateService < ::BaseService
       include Stepable
 
-      DEFAULT_VISIBILITY_LEVEL = Gitlab::VisibilityLevel::INTERNAL
-      DEFAULT_NAME = 'GitLab Instance Administration'
-      DEFAULT_DESCRIPTION = <<~HEREDOC
+      VISIBILITY_LEVEL = Gitlab::VisibilityLevel::INTERNAL
+      PROJECT_NAME = 'GitLab Instance Administration'
+      PROJECT_DESCRIPTION = <<~HEREDOC
         This project is automatically generated and will be used to help monitor this GitLab instance.
       HEREDOC
 
+      GROUP_NAME = 'GitLab Instance Administrators'
+      GROUP_PATH = 'gitlab-instance-administrators'
+
       steps :validate_admins,
+        :create_group,
         :create_project,
         :save_project_id,
-        :add_project_members,
+        :add_group_members,
         :add_to_whitelist,
         :add_prometheus_manual_configuration
 
@@ -37,8 +41,31 @@ module SelfMonitoring
         success
       end
 
+      def create_group
+        if project_created?
+          log_info(_('Instance administrators group already exists'))
+          @group = application_settings.instance_administration_project.owner
+          return success(group: @group)
+        end
+
+        admin_user = group_owner
+        @group = ::Groups::CreateService.new(admin_user, create_group_params).execute
+
+        if @group.persisted?
+          success(group: @group)
+        else
+          error('Could not create group')
+        end
+      end
+
       def create_project
-        admin_user = project_owner
+        if project_created?
+          log_info(_('Instance administration project already exists'))
+          @project = application_settings.instance_administration_project
+          return success(project: project)
+        end
+
+        admin_user = group_owner
         @project = ::Projects::CreateService.new(admin_user, create_project_params).execute
 
         if project.persisted?
@@ -50,9 +77,11 @@ module SelfMonitoring
       end
 
       def save_project_id
+        return success if project_created?
+
         result = ApplicationSettings::UpdateService.new(
           application_settings,
-          project_owner,
+          group_owner,
           { instance_administration_project_id: @project.id }
         ).execute
 
@@ -64,8 +93,8 @@ module SelfMonitoring
         end
       end
 
-      def add_project_members
-        members = project.add_users(project_maintainers, Gitlab::Access::MAINTAINER)
+      def add_group_members
+        members = @group.add_users(group_maintainers, Gitlab::Access::MAINTAINER)
         errors = members.flat_map { |member| member.errors.full_messages }
 
         if errors.any?
@@ -85,8 +114,8 @@ module SelfMonitoring
 
         result = ApplicationSettings::UpdateService.new(
           application_settings,
-          project_owner,
-          outbound_local_requests_whitelist: [uri.normalized_host]
+          group_owner,
+          add_to_outbound_local_requests_whitelist: [uri.normalized_host]
         ).execute
 
         if result
@@ -116,6 +145,10 @@ module SelfMonitoring
           ::ApplicationSetting.create_from_defaults
       end
 
+      def project_created?
+        application_settings.instance_administration_project.present?
+      end
+
       def parse_url(uri_string)
         Addressable::URI.parse(uri_string)
       rescue Addressable::URI::InvalidURIError, TypeError
@@ -136,21 +169,30 @@ module SelfMonitoring
         @instance_admins ||= User.admins.active
       end
 
-      def project_owner
+      def group_owner
         instance_admins.first
       end
 
-      def project_maintainers
-        # Exclude the first so that the project_owner is not added again as a member.
-        instance_admins - [project_owner]
+      def group_maintainers
+        # Exclude the first so that the group_owner is not added again as a member.
+        instance_admins - [group_owner]
+      end
+
+      def create_group_params
+        {
+          name: GROUP_NAME,
+          path: "#{GROUP_PATH}-#{SecureRandom.hex(4)}",
+          visibility_level: VISIBILITY_LEVEL
+        }
       end
 
       def create_project_params
         {
           initialize_with_readme: true,
-          visibility_level: DEFAULT_VISIBILITY_LEVEL,
-          name: DEFAULT_NAME,
-          description: DEFAULT_DESCRIPTION
+          visibility_level: VISIBILITY_LEVEL,
+          name: PROJECT_NAME,
+          description: PROJECT_DESCRIPTION,
+          namespace_id: @group.id
         }
       end
 
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 5ecaaa00c7b..071ea86405b 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -5802,6 +5802,12 @@ msgstr ""
 msgid "Instance Statistics visibility"
 msgstr ""
 
+msgid "Instance administration project already exists"
+msgstr ""
+
+msgid "Instance administrators group already exists"
+msgstr ""
+
 msgid "Instance does not support multiple Kubernetes clusters"
 msgstr ""
 
diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb
index 2c53ef816d8..a0eb83340f0 100644
--- a/spec/services/self_monitoring/project/create_service_spec.rb
+++ b/spec/services/self_monitoring/project/create_service_spec.rb
@@ -30,6 +30,7 @@ describe SelfMonitoring::Project::CreateService do
 
     context 'with admin users' do
       let(:project) { result[:project] }
+      let(:group) { result[:group] }
       let(:application_setting) { Gitlab::CurrentSettings.current_application_settings }
 
       let!(:user) { create(:user, :admin) }
@@ -53,6 +54,15 @@ describe SelfMonitoring::Project::CreateService do
 
       it_behaves_like 'has prometheus service', 'http://localhost:9090'
 
+      it 'creates group' do
+        expect(result[:status]).to eq(:success)
+        expect(group).to be_persisted
+        expect(group.name).to eq(described_class::GROUP_NAME)
+        expect(group.path).to start_with(described_class::GROUP_PATH)
+        expect(group.path.split('-').last.length).to eq(8)
+        expect(group.visibility_level).to eq(described_class::VISIBILITY_LEVEL)
+      end
+
       it 'creates project with internal visibility' do
         expect(result[:status]).to eq(:success)
         expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
@@ -69,8 +79,8 @@ describe SelfMonitoring::Project::CreateService do
 
       it 'creates project with correct name and description' do
         expect(result[:status]).to eq(:success)
-        expect(project.name).to eq(described_class::DEFAULT_NAME)
-        expect(project.description).to eq(described_class::DEFAULT_DESCRIPTION)
+        expect(project.name).to eq(described_class::PROJECT_NAME)
+        expect(project.description).to eq(described_class::PROJECT_DESCRIPTION)
       end
 
       it 'adds all admins as maintainers' do
@@ -79,10 +89,10 @@ describe SelfMonitoring::Project::CreateService do
         create(:user)
 
         expect(result[:status]).to eq(:success)
-        expect(project.owner).to eq(user)
-        expect(project.members.collect(&:user)).to contain_exactly(user, admin1, admin2)
-        expect(project.members.collect(&:access_level)).to contain_exactly(
-          Gitlab::Access::MAINTAINER,
+        expect(project.owner).to eq(group)
+        expect(group.members.collect(&:user)).to contain_exactly(user, admin1, admin2)
+        expect(group.members.collect(&:access_level)).to contain_exactly(
+          Gitlab::Access::OWNER,
           Gitlab::Access::MAINTAINER,
           Gitlab::Access::MAINTAINER
         )
@@ -93,16 +103,22 @@ describe SelfMonitoring::Project::CreateService do
         expect(application_setting.instance_administration_project_id).to eq(project.id)
       end
 
-      context 'when saving project id fails' do
-        before do
-          allow(application_setting).to receive(:update) { false }
-        end
+      it 'returns error when saving project ID fails' do
+        allow(application_setting).to receive(:update) { false }
 
-        it 'returns error' do
-          expect(result[:status]).to eq(:error)
-          expect(result[:failed_step]).to eq(:save_project_id)
-          expect(result[:message]).to eq('Could not save project ID')
-        end
+        expect(result[:status]).to eq(:error)
+        expect(result[:failed_step]).to eq(:save_project_id)
+        expect(result[:message]).to eq('Could not save project ID')
+      end
+
+      it 'does not fail when a project already exists' do
+        expect(result[:status]).to eq(:success)
+
+        second_result = subject.execute
+
+        expect(second_result[:status]).to eq(:success)
+        expect(second_result[:project]).to eq(project)
+        expect(second_result[:group]).to eq(group)
       end
 
       context 'when local requests from hooks and services are not allowed' do
@@ -111,6 +127,15 @@ describe SelfMonitoring::Project::CreateService do
         end
 
         it_behaves_like 'has prometheus service', 'http://localhost:9090'
+
+        it 'does not overwrite the existing whitelist' do
+          application_setting.outbound_local_requests_whitelist = ['example.com']
+
+          expect(result[:status]).to eq(:success)
+          expect(application_setting.outbound_local_requests_whitelist).to contain_exactly(
+            'example.com', 'localhost'
+          )
+        end
       end
 
       context 'with non default prometheus address' do
@@ -186,7 +211,7 @@ describe SelfMonitoring::Project::CreateService do
           expect(result).to eq({
             status: :error,
             message: 'Could not add admins as members',
-            failed_step: :add_project_members
+            failed_step: :add_group_members
           })
         end
       end