Merge branch 'ee-44496-save-project-id' into 'master'

Save instance administration project id in DB

See merge request gitlab-org/gitlab-ee!15018

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

app/models/concerns/cacheable_attributes.rb

@@ -5,6 +5,8 @@ module CacheableAttributes
 
   included do
     after_commit { self.class.expire }
+
+    private_class_method :request_store_cache_key
   end
 
   class_methods do
@@ -32,7 +34,11 @@ module CacheableAttributes
     end
 
     def cached
-      Gitlab::SafeRequestStore[:"#{name}_cached_attributes"] ||= retrieve_from_cache
+      Gitlab::SafeRequestStore[request_store_cache_key] ||= retrieve_from_cache
+    end
+
+    def request_store_cache_key
+      :"#{name}_cached_attributes"
     end
 
     def retrieve_from_cache
@@ -58,6 +64,7 @@ module CacheableAttributes
     end
 
     def expire
+      Gitlab::SafeRequestStore.delete(request_store_cache_key)
       cache_backend.delete(cache_key)
     rescue
       # Gracefully handle when Redis is not available. For example,

app/services/self_monitoring/project/create_service.rb

@@ -4,16 +4,22 @@ module SelfMonitoring
   module Project
     class CreateService < ::BaseService
       include Stepable
+      include Gitlab::Utils::StrongMemoize
 
-      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,
-        :add_project_members,
+        :save_project_id,
+        :add_group_members,
         :add_to_whitelist,
         :add_prometheus_manual_configuration
 
@@ -36,20 +42,60 @@ 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?
           success(project: project)
         else
-          log_error("Could not create self-monitoring project. Errors: #{project.errors.full_messages}")
-          error('Could not create project')
+          log_error(_("Could not create instance administration project. Errors: %{errors}") % { errors: project.errors.full_messages })
+          error(_('Could not create project'))
         end
       end
 
-      def add_project_members
-        members = project.add_users(project_maintainers, Gitlab::Access::MAINTAINER)
+      def save_project_id
+        return success if project_created?
+
+        result = ApplicationSettings::UpdateService.new(
+          application_settings,
+          group_owner,
+          { instance_administration_project_id: @project.id }
+        ).execute
+
+        if result
+          success
+        else
+          log_error(_("Could not save instance administration project ID, errors: %{errors}") % { errors: application_settings.errors.full_messages })
+          error(_('Could not save project ID'))
+        end
+      end
+
+      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?
@@ -68,14 +114,15 @@ module SelfMonitoring
         return error(_('Prometheus listen_address is not a valid URI')) unless uri
 
         result = ApplicationSettings::UpdateService.new(
-          Gitlab::CurrentSettings.current_application_settings,
-          project_owner,
-          outbound_local_requests_whitelist: [uri.normalized_host]
+          application_settings,
+          group_owner,
+          add_to_outbound_local_requests_whitelist: [uri.normalized_host]
         ).execute
 
         if result
           success
         else
+          log_error(_("Could not add prometheus URL to whitelist, errors: %{errors}") % { errors: application_settings.errors.full_messages })
           error(_('Could not add prometheus URL to whitelist'))
         end
       end
@@ -94,6 +141,17 @@ module SelfMonitoring
         success
       end
 
+      def application_settings
+        strong_memoize(:application_settings) do
+          Gitlab::CurrentSettings.expire_current_application_settings
+          Gitlab::CurrentSettings.current_application_settings
+        end
+      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
@@ -114,21 +172,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
 

db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb

+# frozen_string_literal: true
+
+class AddColumnForSelfMonitoringProjectId < ActiveRecord::Migration[5.2]
+  DOWNTIME = false
+
+  def change
+    add_reference(
+      :application_settings,
+      :instance_administration_project,
+      index: { name: 'index_applicationsettings_on_instance_administration_project_id' },
+      foreign_key: { to_table: :projects, on_delete: :nullify }
+    )
+  end
+end

db/schema.rb

@@ -231,8 +231,10 @@ ActiveRecord::Schema.define(version: 2019_08_02_235445) do
     t.integer "raw_blob_request_limit", default: 300, null: false
     t.boolean "allow_local_requests_from_web_hooks_and_services", default: false, null: false
     t.boolean "allow_local_requests_from_system_hooks", default: true, null: false
+    t.bigint "instance_administration_project_id"
     t.index ["custom_project_templates_group_id"], name: "index_application_settings_on_custom_project_templates_group_id"
     t.index ["file_template_project_id"], name: "index_application_settings_on_file_template_project_id"
+    t.index ["instance_administration_project_id"], name: "index_applicationsettings_on_instance_administration_project_id"
     t.index ["usage_stats_set_by_user_id"], name: "index_application_settings_on_usage_stats_set_by_user_id"
   end
 
@@ -3623,6 +3625,7 @@ ActiveRecord::Schema.define(version: 2019_08_02_235445) do
 
   add_foreign_key "application_settings", "namespaces", column: "custom_project_templates_group_id", on_delete: :nullify
   add_foreign_key "application_settings", "projects", column: "file_template_project_id", name: "fk_ec757bd087", on_delete: :nullify
+  add_foreign_key "application_settings", "projects", column: "instance_administration_project_id", on_delete: :nullify
   add_foreign_key "application_settings", "users", column: "usage_stats_set_by_user_id", name: "fk_964370041d", on_delete: :nullify
   add_foreign_key "approval_merge_request_rule_sources", "approval_merge_request_rules", on_delete: :cascade
   add_foreign_key "approval_merge_request_rule_sources", "approval_project_rules", on_delete: :cascade

ee/spec/services/ee/self_monitoring/project/create_service_spec.rb

@@ -13,17 +13,15 @@ describe SelfMonitoring::Project::CreateService do
 
     let(:result) { subject.execute }
     let(:project) { result[:project] }
+    let(:application_setting) { Gitlab::CurrentSettings.current_application_settings }
 
     let!(:user) { create(:user, :admin) }
 
     before do
       allow(Gitlab.config).to receive(:prometheus).and_return(prometheus_settings)
 
-      allow(ApplicationSetting)
-        .to receive(:current)
-        .and_return(
-          ApplicationSetting.build_from_defaults(allow_local_requests_from_hooks_and_services: true)
-        )
+      allow(ApplicationSetting).to receive(:current_without_cache) { application_setting }
+      application_setting.allow_local_requests_from_web_hooks_and_services = true
     end
 
     context 'with license' do

lib/gitlab/current_settings.rb

@@ -7,6 +7,11 @@ module Gitlab
         Gitlab::SafeRequestStore.fetch(:current_application_settings) { ensure_application_settings! }
       end
 
+      def expire_current_application_settings
+        ::ApplicationSetting.expire
+        Gitlab::SafeRequestStore.delete(:current_application_settings)
+      end
+
       def clear_in_memory_application_settings!
         @in_memory_application_settings = nil
       end

locale/gitlab.pot

@@ -4072,6 +4072,9 @@ msgstr ""
 msgid "Could not add prometheus URL to whitelist"
 msgstr ""
 
+msgid "Could not add prometheus URL to whitelist, errors: %{errors}"
+msgstr ""
+
 msgid "Could not authorize chat nickname. Try again!"
 msgstr ""
 
@@ -4087,6 +4090,12 @@ msgstr ""
 msgid "Could not create Wiki Repository at this time. Please try again later."
 msgstr ""
 
+msgid "Could not create instance administration project. Errors: %{errors}"
+msgstr ""
+
+msgid "Could not create project"
+msgstr ""
+
 msgid "Could not delete chat nickname %{chat_name}."
 msgstr ""
 
@@ -4102,6 +4111,12 @@ msgstr ""
 msgid "Could not revoke personal access token %{personal_access_token_name}."
 msgstr ""
 
+msgid "Could not save instance administration project ID, errors: %{errors}"
+msgstr ""
+
+msgid "Could not save project ID"
+msgstr ""
+
 msgid "Coverage"
 msgstr ""
 
@@ -7835,6 +7850,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 ""
 

spec/lib/gitlab/current_settings_spec.rb

@@ -14,6 +14,16 @@ describe Gitlab::CurrentSettings do
     end
   end
 
+  describe '.expire_current_application_settings', :use_clean_rails_memory_store_caching, :request_store do
+    include_context 'with settings in cache'
+
+    it 'expires the cache' do
+      described_class.expire_current_application_settings
+
+      expect(ActiveRecord::QueryRecorder.new { described_class.current_application_settings }.count).not_to eq(0)
+    end
+  end
+
   describe '#current_application_settings', :use_clean_rails_memory_store_caching do
     it 'allows keys to be called directly' do
       db_settings = create(:application_setting,

spec/models/concerns/cacheable_attributes_spec.rb

@@ -7,6 +7,7 @@ describe CacheableAttributes do
     Class.new do
       include ActiveModel::Model
       extend ActiveModel::Callbacks
+      include ActiveModel::AttributeMethods
       define_model_callbacks :commit
       include CacheableAttributes
 
@@ -34,44 +35,60 @@ describe CacheableAttributes do
     end
   end
 
+  before do
+    stub_const("MinimalTestClass", minimal_test_class)
+  end
+
   shared_context 'with defaults' do
     before do
-      minimal_test_class.define_singleton_method(:defaults) do
+      MinimalTestClass.define_singleton_method(:defaults) do
         { foo: 'a', bar: 'b', baz: 'c' }
       end
     end
   end
 
+  describe '.expire', :use_clean_rails_memory_store_caching, :request_store do
+    it 'wipes the cache' do
+      obj = MinimalTestClass.new
+      obj.cache!
+      expect(MinimalTestClass.cached).not_to eq(nil)
+
+      MinimalTestClass.expire
+
+      expect(MinimalTestClass.cached).to eq(nil)
+    end
+  end
+
   describe '.current_without_cache' do
     it 'defaults to last' do
-      expect(minimal_test_class.current_without_cache).to eq(minimal_test_class.last)
+      expect(MinimalTestClass.current_without_cache).to eq(MinimalTestClass.last)
     end
 
     it 'can be overridden' do
-      minimal_test_class.define_singleton_method(:current_without_cache) do
+      MinimalTestClass.define_singleton_method(:current_without_cache) do
         first
       end
 
-      expect(minimal_test_class.current_without_cache).to eq(minimal_test_class.first)
+      expect(MinimalTestClass.current_without_cache).to eq(MinimalTestClass.first)
     end
   end
 
   describe '.cache_key' do
     it 'excludes cache attributes' do
-      expect(minimal_test_class.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Rails.version}")
+      expect(MinimalTestClass.cache_key).to eq("TestClass:#{Gitlab::VERSION}:#{Rails.version}")
     end
   end
 
   describe '.defaults' do
     it 'defaults to {}' do
-      expect(minimal_test_class.defaults).to eq({})
+      expect(MinimalTestClass.defaults).to eq({})
     end
 
     context 'with defaults defined' do
       include_context 'with defaults'
 
       it 'can be overridden' do
-        expect(minimal_test_class.defaults).to eq({ foo: 'a', bar: 'b', baz: 'c' })
+        expect(MinimalTestClass.defaults).to eq({ foo: 'a', bar: 'b', baz: 'c' })
       end
     end
   end
@@ -81,13 +98,13 @@ describe CacheableAttributes do
 
     context 'without any attributes given' do
       it 'intializes a new object with the defaults' do
-        expect(minimal_test_class.build_from_defaults.attributes).to eq(minimal_test_class.defaults.stringify_keys)
+        expect(MinimalTestClass.build_from_defaults.attributes).to eq(MinimalTestClass.defaults.stringify_keys)
       end
     end
 
     context 'with attributes given' do
       it 'intializes a new object with the given attributes merged into the defaults' do
-        expect(minimal_test_class.build_from_defaults(foo: 'd').attributes['foo']).to eq('d')
+        expect(MinimalTestClass.build_from_defaults(foo: 'd').attributes['foo']).to eq('d')
       end
     end
 
@@ -108,8 +125,8 @@ describe CacheableAttributes do
   describe '.current', :use_clean_rails_memory_store_caching do
     context 'redis unavailable' do
       before do
-        allow(minimal_test_class).to receive(:last).and_return(:last)
-        expect(Rails.cache).to receive(:read).with(minimal_test_class.cache_key).and_raise(Redis::BaseError)
+        allow(MinimalTestClass).to receive(:last).and_return(:last)
+        expect(Rails.cache).to receive(:read).with(MinimalTestClass.cache_key).and_raise(Redis::BaseError)
       end
 
       context 'in production environment' do
@@ -120,7 +137,7 @@ describe CacheableAttributes do
         it 'returns an uncached record and logs a warning' do
           expect(Rails.logger).to receive(:warn).with("Cached record for TestClass couldn't be loaded, falling back to uncached record: Redis::BaseError")
 
-          expect(minimal_test_class.current).to eq(:last)
+          expect(MinimalTestClass.current).to eq(:last)
         end
       end
 
@@ -132,7 +149,7 @@ describe CacheableAttributes do
         it 'returns an uncached record and logs a warning' do
           expect(Rails.logger).not_to receive(:warn)
 
-          expect { minimal_test_class.current }.to raise_error(Redis::BaseError)
+          expect { MinimalTestClass.current }.to raise_error(Redis::BaseError)
         end
       end
     end
@@ -202,7 +219,7 @@ describe CacheableAttributes do
   describe '.cached', :use_clean_rails_memory_store_caching do
     context 'when cache is cold' do
       it 'returns nil' do
-        expect(minimal_test_class.cached).to be_nil
+        expect(MinimalTestClass.cached).to be_nil
       end
     end
 

spec/services/self_monitoring/project/create_service_spec.rb

@@ -30,15 +30,13 @@ 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) }
 
       before do
-        allow(ApplicationSetting)
-          .to receive(:current)
-          .and_return(
-            ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: true)
-          )
+        application_setting.allow_local_requests_from_web_hooks_and_services = true
       end
 
       shared_examples 'has prometheus service' do |listen_address|
@@ -55,6 +53,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)
@@ -62,7 +69,7 @@ describe SelfMonitoring::Project::CreateService do
       end
 
       it 'creates project with internal visibility even when internal visibility is restricted' do
-        stub_application_setting(restricted_visibility_levels: [Gitlab::VisibilityLevel::INTERNAL])
+        application_setting.restricted_visibility_levels = [Gitlab::VisibilityLevel::INTERNAL]
 
         expect(result[:status]).to eq(:success)
         expect(project.visibility_level).to eq(Gitlab::VisibilityLevel::INTERNAL)
@@ -71,8 +78,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
@@ -81,25 +88,53 @@ 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
         )
       end
 
+      it 'saves the project id' do
+        expect(result[:status]).to eq(:success)
+        expect(application_setting.instance_administration_project_id).to eq(project.id)
+      end
+
+      it 'returns error when saving project ID fails' do
+        allow(application_setting).to receive(:update) { false }
+
+        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
         before do
-          allow(ApplicationSetting)
-            .to receive(:current)
-            .and_return(
-              ApplicationSetting.build_from_defaults(allow_local_requests_from_web_hooks_and_services: false)
-            )
+          application_setting.allow_local_requests_from_web_hooks_and_services = false
         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
@@ -175,7 +210,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