From af4a597d3fb687ed2841bb755403f66cf131bdff Mon Sep 17 00:00:00 2001 From: Reuben Pereira Date: Wed, 7 Aug 2019 18:40:36 +0000 Subject: [PATCH] Save instance administration project id in DB - This will make it easy to identify the project even if admins change the name of the project or move it. --- app/models/application_setting.rb | 2 + app/models/concerns/cacheable_attributes.rb | 9 +- .../self_monitoring/project/create_service.rb | 105 ++++++++++++++---- ...d_column_for_self_monitoring_project_id.rb | 14 +++ db/schema.rb | 3 + lib/gitlab/current_settings.rb | 5 + locale/gitlab.pot | 21 ++++ spec/lib/gitlab/current_settings_spec.rb | 10 ++ .../concerns/cacheable_attributes_spec.rb | 45 +++++--- .../project/create_service_spec.rb | 71 +++++++++--- 10 files changed, 233 insertions(+), 52 deletions(-) create mode 100644 db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb 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/models/concerns/cacheable_attributes.rb b/app/models/concerns/cacheable_attributes.rb index 53dff2adfc3..0c800621a55 100644 --- a/app/models/concerns/cacheable_attributes.rb +++ b/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, diff --git a/app/services/self_monitoring/project/create_service.rb b/app/services/self_monitoring/project/create_service.rb index 8ffd22de127..c925c6a1610 100644 --- a/app/services/self_monitoring/project/create_service.rb +++ b/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 diff --git a/db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb b/db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb new file mode 100644 index 00000000000..ce249a527e6 --- /dev/null +++ b/db/migrate/20190724112147_add_column_for_self_monitoring_project_id.rb @@ -0,0 +1,14 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 2fac2c01e00..890f4ab4d8a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -230,9 +230,11 @@ 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.string "snowplow_collector_hostname" 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 diff --git a/lib/gitlab/current_settings.rb b/lib/gitlab/current_settings.rb index 32d5e4b9ea3..6ce47650562 100644 --- a/lib/gitlab/current_settings.rb +++ b/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 diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 022d1745a1b..7586fff2e7d 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -3297,6 +3297,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 "" @@ -3309,6 +3312,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 "" @@ -3324,6 +3333,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 "" @@ -5787,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/lib/gitlab/current_settings_spec.rb b/spec/lib/gitlab/current_settings_spec.rb index 2759412add8..eced96a4c77 100644 --- a/spec/lib/gitlab/current_settings_spec.rb +++ b/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, diff --git a/spec/models/concerns/cacheable_attributes_spec.rb b/spec/models/concerns/cacheable_attributes_spec.rb index eeacdadab9c..da46effe411 100644 --- a/spec/models/concerns/cacheable_attributes_spec.rb +++ b/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 diff --git a/spec/services/self_monitoring/project/create_service_spec.rb b/spec/services/self_monitoring/project/create_service_spec.rb index 7d4faba526b..def20448bd9 100644 --- a/spec/services/self_monitoring/project/create_service_spec.rb +++ b/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