Fix namespace/project removal elasticsearch limit

Send a -1 when a user purposefully removes all namespaces or projects
from their relevant lists

doc/api/settings.md

@@ -176,6 +176,9 @@ are listed in the descriptions of the relevant settings.
 | `elasticsearch_indexing`                 | boolean          | no                                   | **(Premium)** Enable Elasticsearch indexing |
 | `elasticsearch_search`                   | boolean          | no                                   | **(Premium)** Enable Elasticsearch search |
 | `elasticsearch_url`                      | string           | no                                   | **(Premium)** The url to use for connecting to Elasticsearch. Use a comma-separated list to support cluster (e.g., "http://localhost:9200, http://localhost:9201"). If your Elasticsearch instance is password protected, pass the `username:password` in the URL (e.g., `http://<username>:<password>@<elastic_host>:9200/`). |
+| `elasticsearch_limit_indexing`           | boolean          | no                                   | **(Premium)** Limit Elasticsearch to index certain namespaces and projects |
+| `elasticsearch_project_ids`              | array of integers | no                                  | **(Premium)** The projects to index via Elasticsearch if `elasticsearch_limit_indexing` is enabled. |
+| `elasticsearch_namespace_ids`            | array of integers | no                                  | **(Premium)** The namespaces to index via Elasticsearch if `elasticsearch_limit_indexing` is enabled. |
 | `email_additional_text`                  | string           | no                                   | **(Premium)** Additional text added to the bottom of every email for legal/auditing/compliance reasons |
 | `email_author_in_body`                   | boolean          | no                                   | Some email servers do not support overriding the email sender name. Enable this option to include the name of the author of the issue, merge request or comment in the email body instead. |
 | `enabled_git_access_protocol`            | string           | no                                   | Enabled protocols for Git access. Allowed values are: `ssh`, `http`, and `nil` to allow both protocols. |

ee/app/helpers/ee/application_settings_helper.rb

@@ -54,11 +54,11 @@ module EE
     end
 
     def elasticsearch_namespace_ids
-      ElasticsearchIndexedNamespace.namespace_ids.join(',')
+      ElasticsearchIndexedNamespace.target_ids.join(',')
     end
 
     def elasticsearch_project_ids
-      ElasticsearchIndexedProject.project_ids.join(',')
+      ElasticsearchIndexedProject.target_ids.join(',')
     end
 
     def self.repository_mirror_attributes

ee/app/models/concerns/elasticsearch_indexed_container.rb

+# frozen_string_literal: true
+
+module ElasticsearchIndexedContainer
+  extend ActiveSupport::Concern
+
+  included do
+    after_commit :index, on: :create
+    after_commit :delete_from_index, on: :destroy
+  end
+
+  class_methods do
+    def target_ids
+      pluck(target_attr_name)
+    end
+
+    def remove_all(except: [])
+      self.where.not(target_attr_name => except).each_batch do |batch, _index|
+        batch.destroy_all # #rubocop:disable Cop/DestroyAll
+      end
+    end
+  end
+end

ee/app/models/ee/application_setting.rb

@@ -14,11 +14,6 @@ module EE
       EMAIL_ADDITIONAL_TEXT_CHARACTER_LIMIT = 10_000
       INSTANCE_REVIEW_MIN_USERS = 100
 
-      attr_accessor :elasticsearch_namespace_ids, :elasticsearch_project_ids
-
-      after_save -> { update_elasticsearch_containers(ElasticsearchIndexedNamespace, :namespace_id, elasticsearch_namespace_ids) }, on: [:create, :update]
-      after_save -> { update_elasticsearch_containers(ElasticsearchIndexedProject, :project_id, elasticsearch_project_ids) }, on: [:create, :update]
-
       belongs_to :file_template_project, class_name: "Project"
 
       ignore_column :minimum_mirror_sync_time
@@ -95,24 +90,12 @@ module EE
       end
     end
 
-    def update_elasticsearch_containers(klass, attribute, container_ids)
-      return unless elasticsearch_limit_indexing?
-
-      container_ids = container_ids&.split(",")
-      return unless container_ids.present?
-
-      # Destroy any containers that have been removed. This runs callbacks, etc
-      # #rubocop:disable Cop/DestroyAll
-      klass.where.not(attribute => container_ids).each_batch do |batch, _index|
-        batch.destroy_all
-      end
-      # #rubocop:enable Cop/DestroyAll
-
-      # Disregard any duplicates that are already present
-      container_ids -= klass.pluck(attribute)
+    def elasticsearch_namespace_ids
+      ElasticsearchIndexedNamespace.target_ids
+    end
 
-      # Add new containers
-      container_ids.each { |id| klass.create(attribute => id) }
+    def elasticsearch_project_ids
+      ElasticsearchIndexedProject.target_ids
     end
 
     def elasticsearch_indexes_project?(project)

ee/app/models/elasticsearch_indexed_namespace.rb

 # frozen_string_literal: true
 
 class ElasticsearchIndexedNamespace < ApplicationRecord
+  include ElasticsearchIndexedContainer
   include EachBatch
 
-  self.primary_key = 'namespace_id'
-
-  after_commit :index, on: :create
-  after_commit :delete_from_index, on: :destroy
+  self.primary_key = :namespace_id
 
   belongs_to :namespace
 
   validates :namespace_id, presence: true, uniqueness: true
 
-  def self.namespace_ids
-    self.pluck(:namespace_id)
+  def self.target_attr_name
+    :namespace_id
   end
 
   private

ee/app/models/elasticsearch_indexed_project.rb

 # frozen_string_literal: true
 
 class ElasticsearchIndexedProject < ApplicationRecord
+  include ElasticsearchIndexedContainer
   include EachBatch
 
-  self.primary_key = 'project_id'
-
-  after_commit :index, on: :create
-  after_commit :delete_from_index, on: :destroy
+  self.primary_key = :project_id
 
   belongs_to :project
 
   validates :project_id, presence: true, uniqueness: true
 
-  def self.project_ids
-    self.pluck(:project_id)
+  def self.target_attr_name
+    :project_id
   end
 
   private

ee/app/services/ee/application_settings/update_service.rb

@@ -12,7 +12,31 @@ module EE
         limit = params.delete(:repository_size_limit)
         application_setting.repository_size_limit = ::Gitlab::Utils.try_megabytes_to_bytes(limit) if limit
 
-        super
+        elasticsearch_namespace_ids = params.delete(:elasticsearch_namespace_ids)
+        elasticsearch_project_ids = params.delete(:elasticsearch_project_ids)
+
+        if result = super
+          update_elasticsearch_containers(ElasticsearchIndexedNamespace, elasticsearch_namespace_ids)
+          update_elasticsearch_containers(ElasticsearchIndexedProject, elasticsearch_project_ids)
+        end
+
+        result
+      end
+
+      def update_elasticsearch_containers(klass, new_container_ids)
+        return unless application_setting.elasticsearch_limit_indexing?
+
+        new_container_ids = new_container_ids&.split(",") unless new_container_ids.is_a?(Array)
+        return if new_container_ids.nil?
+
+        # Destroy any containers that have been removed. This runs callbacks, etc
+        klass.remove_all(except: new_container_ids)
+
+        # Disregard any duplicates that are already present
+        new_container_ids -= klass.target_ids
+
+        # Add new containers
+        new_container_ids.each { |id| klass.create(klass.target_attr_name => id) }
       end
     end
   end

ee/changelogs/unreleased/11225-es_limited_namespaces_removal.yml

+---
+title: Fix for not being able to remove the last namespace/project from elasticsearch
+  limited namespaces/projects
+merge_request: 11989
+author:
+type: fixed

ee/spec/features/admin/admin_settings_spec.rb

@@ -100,11 +100,44 @@ describe 'Admin updates EE-only settings' do
         click_button 'Save changes'
       end
 
+      wait_for_all_requests
+
       expect(Gitlab::CurrentSettings.elasticsearch_limit_indexing).to be_truthy
       expect(ElasticsearchIndexedNamespace.exists?(namespace_id: namespace.id)).to be_truthy
       expect(ElasticsearchIndexedProject.exists?(project_id: project.id)).to be_truthy
       expect(page).to have_content "Application settings saved successfully"
     end
+
+    it 'Allows removing all namespaces and projects', :js do
+      stub_ee_application_setting(elasticsearch_limit_indexing: true)
+
+      namespace = create(:elasticsearch_indexed_namespace).namespace
+      project = create(:elasticsearch_indexed_project).project
+
+      visit integrations_admin_application_settings_path
+
+      expect(ElasticsearchIndexedNamespace.count).to be > 0
+      expect(ElasticsearchIndexedProject.count).to be > 0
+
+      page.within('.as-elasticsearch') do
+        expect(page).to have_content('Namespaces to index')
+        expect(page).to have_content('Projects to index')
+        expect(page).to have_content(namespace.full_path)
+        expect(page).to have_content(project.full_name)
+
+        find('.js-limit-namespaces .select2-search-choice-close').click
+        find('.js-limit-projects .select2-search-choice-close').click
+
+        expect(page).not_to have_content(namespace.full_path)
+        expect(page).not_to have_content(project.full_name)
+
+        click_button 'Save changes'
+      end
+
+      expect(ElasticsearchIndexedNamespace.count).to eq(0)
+      expect(ElasticsearchIndexedProject.count).to eq(0)
+      expect(page).to have_content "Application settings saved successfully"
+    end
   end
 
   it 'Enable Slack application' do

ee/spec/models/application_setting_spec.rb

@@ -193,23 +193,7 @@ describe ApplicationSetting do
       end
 
       context 'namespaces' do
-        let(:namespaces) { create_list(:namespace, 3) }
-
-        it 'creates ElasticsearchIndexedNamespace objects when given elasticsearch_namespace_ids' do
-          expect do
-            setting.update!(elasticsearch_namespace_ids: namespaces.map(&:id).join(','))
-          end.to change { ElasticsearchIndexedNamespace.count }.by(3)
-        end
-
-        it 'deletes ElasticsearchIndexedNamespace objects not in elasticsearch_namespace_ids' do
-          create :elasticsearch_indexed_namespace, namespace: namespaces.last
-
-          expect do
-            setting.update!(elasticsearch_namespace_ids: namespaces.first(2).map(&:id).join(','))
-          end.to change { ElasticsearchIndexedNamespace.count }.from(1).to(2)
-
-          expect(ElasticsearchIndexedNamespace.where(namespace_id: namespaces.last.id)).not_to exist
-        end
+        let(:namespaces) { create_list(:namespace, 2) }
 
         it 'tells you if a namespace is allowed to be indexed' do
           create :elasticsearch_indexed_namespace, namespace: namespaces.last
@@ -220,23 +204,7 @@ describe ApplicationSetting do
       end
 
       context 'projects' do
-        let(:projects) { create_list(:project, 3) }
-
-        it 'creates ElasticsearchIndexedProject objects when given elasticsearch_project_ids' do
-          expect do
-            setting.update!(elasticsearch_project_ids: projects.map(&:id).join(','))
-          end.to change { ElasticsearchIndexedProject.count }.by(3)
-        end
-
-        it 'deletes ElasticsearchIndexedProject objects not in elasticsearch_project_ids' do
-          create :elasticsearch_indexed_project, project: projects.last
-
-          expect do
-            setting.update!(elasticsearch_project_ids: projects.first(2).map(&:id).join(','))
-          end.to change { ElasticsearchIndexedProject.count }.from(1).to(2)
-
-          expect(ElasticsearchIndexedProject.where(project_id: projects.last.id)).not_to exist
-        end
+        let(:projects) { create_list(:project, 2) }
 
         it 'tells you if a project is allowed to be indexed' do
           create :elasticsearch_indexed_project, project: projects.last
@@ -250,10 +218,8 @@ describe ApplicationSetting do
         project1 = create(:project)
         projects = create_list(:project, 3)
 
-        setting.update!(
-          elasticsearch_project_ids: projects.map(&:id).join(','),
-          elasticsearch_namespace_ids: project1.namespace.id.to_s
-        )
+        create :elasticsearch_indexed_namespace, namespace: project1.namespace
+        projects.each { |project| create :elasticsearch_indexed_project, project: project }
 
         expect(setting.elasticsearch_limited_projects).to match_array(projects << project1)
       end

ee/spec/models/ee/namespace_spec.rb

@@ -51,7 +51,7 @@ describe Namespace do
 
       expect(namespace.use_elasticsearch?).to eq(false)
 
-      ::Gitlab::CurrentSettings.update!(elasticsearch_namespace_ids: namespace.id.to_s)
+      create :elasticsearch_indexed_namespace, namespace: namespace
 
       expect(namespace.use_elasticsearch?).to eq(true)
     end

ee/spec/requests/api/settings_spec.rb

@@ -33,6 +33,46 @@ describe API::Settings, 'EE Settings' do
       expect(json_response['snowplow_site_id']).to eq('site_id')
       expect(json_response['file_template_project_id']).to eq(project.id)
     end
+
+    context 'elasticsearch settings' do
+      it 'limits namespaces and projects properly' do
+        namespace_ids = create_list(:namespace, 2).map(&:id)
+        project_ids = create_list(:project, 2).map(&:id)
+
+        put api('/application/settings', admin),
+            params: {
+              elasticsearch_limit_indexing: true,
+              elasticsearch_project_ids: project_ids.join(','),
+              elasticsearch_namespace_ids: namespace_ids.join(',')
+            }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['elasticsearch_limit_indexing']).to eq(true)
+        expect(json_response['elasticsearch_project_ids']).to eq(project_ids)
+        expect(json_response['elasticsearch_namespace_ids']).to eq(namespace_ids)
+        expect(ElasticsearchIndexedNamespace.count).to eq(2)
+        expect(ElasticsearchIndexedProject.count).to eq(2)
+      end
+
+      it 'removes namespaces and projects properly' do
+        stub_ee_application_setting(elasticsearch_limit_indexing: true)
+        create(:elasticsearch_indexed_namespace).namespace.id
+        create(:elasticsearch_indexed_project).project.id
+
+        put api('/application/settings', admin),
+            params: {
+              elasticsearch_namespace_ids: []
+            }.to_json,
+            headers: {
+              'CONTENT_TYPE' => 'application/json'
+            }
+
+        expect(response).to have_gitlab_http_status(200)
+        expect(json_response['elasticsearch_namespace_ids']).to eq([])
+        expect(ElasticsearchIndexedNamespace.count).to eq(0)
+        expect(ElasticsearchIndexedProject.count).to eq(1)
+      end
+    end
   end
 
   shared_examples 'settings for licensed features' do

ee/spec/services/application_settings/update_service_spec.rb

@@ -60,6 +60,61 @@ describe ApplicationSettings::UpdateService do
           expect(setting.repository_size_limit).to be_nil
         end
       end
+
+      context 'elasticsearch' do
+        context 'limiting namespaces and projects' do
+          before do
+            setting.update!(elasticsearch_indexing: true)
+            setting.update!(elasticsearch_limit_indexing: true)
+          end
+
+          context 'namespaces' do
+            let(:namespaces) { create_list(:namespace, 3) }
+
+            it 'creates ElasticsearchIndexedNamespace objects when given elasticsearch_namespace_ids' do
+              opts = { elasticsearch_namespace_ids: namespaces.map(&:id).join(',') }
+
+              expect do
+                described_class.new(setting, user, opts).execute
+              end.to change { ElasticsearchIndexedNamespace.count }.by(3)
+            end
+
+            it 'deletes ElasticsearchIndexedNamespace objects not in elasticsearch_namespace_ids' do
+              create :elasticsearch_indexed_namespace, namespace: namespaces.last
+              opts = { elasticsearch_namespace_ids: namespaces.first(2).map(&:id).join(',') }
+
+              expect do
+                described_class.new(setting, user, opts).execute
+              end.to change { ElasticsearchIndexedNamespace.count }.from(1).to(2)
+
+              expect(ElasticsearchIndexedNamespace.where(namespace_id: namespaces.last.id)).not_to exist
+            end
+          end
+
+          context 'projects' do
+            let(:projects) { create_list(:project, 3) }
+
+            it 'creates ElasticsearchIndexedProject objects when given elasticsearch_project_ids' do
+              opts = { elasticsearch_project_ids: projects.map(&:id).join(',') }
+
+              expect do
+                described_class.new(setting, user, opts).execute
+              end.to change { ElasticsearchIndexedProject.count }.by(3)
+            end
+
+            it 'deletes ElasticsearchIndexedProject objects not in elasticsearch_project_ids' do
+              create :elasticsearch_indexed_project, project: projects.last
+              opts = { elasticsearch_project_ids: projects.first(2).map(&:id).join(',') }
+
+              expect do
+                described_class.new(setting, user, opts).execute
+              end.to change { ElasticsearchIndexedProject.count }.from(1).to(2)
+
+              expect(ElasticsearchIndexedProject.where(project_id: projects.last.id)).not_to exist
+            end
+          end
+        end
+      end
     end
   end
 end

lib/api/settings.rb

@@ -150,6 +150,12 @@ module API
         given elasticsearch_indexing: ->(val) { val } do
           optional :elasticsearch_search, type: Boolean, desc: 'Enable Elasticsearch search'
           requires :elasticsearch_url, type: String, desc: 'The url to use for connecting to Elasticsearch. Use a comma-separated list to support clustering (e.g., "http://localhost:9200, http://localhost:9201")'
+          optional :elasticsearch_limit_indexing, type: Boolean, desc: 'Limit Elasticsearch to index certain namespaces and projects'
+        end
+
+        given elasticsearch_limit_indexing: ->(val) { val } do
+          optional :elasticsearch_namespace_ids, type: Array[Integer], coerce_with: Validations::Types::LabelsList.coerce, desc: 'The namespace ids to index with Elasticsearch.'
+          optional :elasticsearch_project_ids, type: Array[Integer], coerce_with: Validations::Types::LabelsList.coerce, desc: 'The project ids to index with Elasticsearch.'
         end
 
         optional :email_additional_text, type: String, desc: 'Additional text added to the bottom of every email for legal/auditing/compliance reasons'