Merge branch 'gitaly-feature-flag-per-project' into 'master'

Allow setting Gitaly feature flags for individual projects

See merge request gitlab-org/gitlab!53387

lib/api/helpers/internal_helpers.rb

@@ -124,7 +124,7 @@ module API
           repository: repository.gitaly_repository.to_h,
           address: Gitlab::GitalyClient.address(repository.shard),
           token: Gitlab::GitalyClient.token(repository.shard),
-          features: Feature::Gitaly.server_feature_flags
+          features: Feature::Gitaly.server_feature_flags(repository.project)
         }
       end
     end

lib/feature/gitaly.rb

@@ -5,25 +5,25 @@ class Feature
     PREFIX = "gitaly_"
 
     class << self
-      def enabled?(feature_flag)
+      def enabled?(feature_flag, project = nil)
         return false unless Feature::FlipperFeature.table_exists?
 
-        Feature.enabled?("#{PREFIX}#{feature_flag}")
+        Feature.enabled?("#{PREFIX}#{feature_flag}", project)
       rescue ActiveRecord::NoDatabaseError, PG::ConnectionBad
         false
       end
 
-      def server_feature_flags
+      def server_feature_flags(project = nil)
         # We need to check that both the DB connection and table exists
         return {} unless ::Gitlab::Database.cached_table_exists?(FlipperFeature.table_name)
 
         Feature.persisted_names
           .select { |f| f.start_with?(PREFIX) }
-          .map do |f|
+          .to_h do |f|
           flag = f.delete_prefix(PREFIX)
 
-          ["gitaly-feature-#{flag.tr('_', '-')}", enabled?(flag).to_s]
-        end.to_h
+          ["gitaly-feature-#{flag.tr('_', '-')}", enabled?(flag, project).to_s]
+        end
       end
     end
   end

lib/gitlab/workhorse.rb

@@ -32,7 +32,7 @@ module Gitlab
           GitalyServer: {
             address: Gitlab::GitalyClient.address(repository.storage),
             token: Gitlab::GitalyClient.token(repository.storage),
-            features: Feature::Gitaly.server_feature_flags
+            features: Feature::Gitaly.server_feature_flags(repository.project)
           }
         }
 
@@ -231,7 +231,7 @@ module Gitlab
         {
           address: Gitlab::GitalyClient.address(repository.shard),
           token: Gitlab::GitalyClient.token(repository.shard),
-          features: Feature::Gitaly.server_feature_flags
+          features: Feature::Gitaly.server_feature_flags(repository.project)
         }
       end
 

spec/lib/feature/gitaly_spec.rb

@@ -3,35 +3,78 @@
 require 'spec_helper'
 
 RSpec.describe Feature::Gitaly do
-  let(:feature_flag) { "mep_mep" }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:project_2) { create(:project) }
+
+  before do
+    skip_feature_flags_yaml_validation
+  end
 
   describe ".enabled?" do
-    context 'when the gate is closed' do
-      before do
-        stub_feature_flags(gitaly_mep_mep: false)
+    context 'when the flag is set globally' do
+      let(:feature_flag) { 'global_flag' }
+
+      context 'when the gate is closed' do
+        before do
+          stub_feature_flags(gitaly_global_flag: false)
+        end
+
+        it 'returns false' do
+          expect(described_class.enabled?(feature_flag)).to be(false)
+        end
       end
 
-      it 'returns false' do
-        expect(described_class.enabled?(feature_flag)).to be(false)
+      context 'when the flag defaults to on' do
+        it 'returns true' do
+          expect(described_class.enabled?(feature_flag)).to be(true)
+        end
       end
     end
 
-    context 'when the flag defaults to on' do
-      it 'returns true' do
-        expect(described_class.enabled?(feature_flag)).to be(true)
+    context 'when the flag is enabled for a particular project' do
+      let(:feature_flag) { 'project_flag' }
+
+      before do
+        stub_feature_flags(gitaly_project_flag: project)
+      end
+
+      it 'returns true for that project' do
+        expect(described_class.enabled?(feature_flag, project)).to be(true)
+      end
+
+      it 'returns false for any other project' do
+        expect(described_class.enabled?(feature_flag, project_2)).to be(false)
+      end
+
+      it 'returns false when no project is passed' do
+        expect(described_class.enabled?(feature_flag)).to be(false)
       end
     end
   end
 
   describe ".server_feature_flags" do
     before do
-      stub_feature_flags(gitaly_mep_mep: true, foo: true)
+      stub_feature_flags(gitaly_global_flag: true, gitaly_project_flag: project, non_gitaly_flag: false)
     end
 
     subject { described_class.server_feature_flags }
 
-    it { is_expected.to be_a(Hash) }
-    it { is_expected.to eq("gitaly-feature-mep-mep" => "true") }
+    it 'returns a hash of flags starting with the prefix, with dashes instead of underscores' do
+      expect(subject).to eq('gitaly-feature-global-flag' => 'true',
+                            'gitaly-feature-project-flag' => 'false')
+    end
+
+    context 'when a project is passed' do
+      it 'returns the value for the flag on the given project' do
+        expect(described_class.server_feature_flags(project))
+          .to eq('gitaly-feature-global-flag' => 'true',
+                 'gitaly-feature-project-flag' => 'true')
+
+        expect(described_class.server_feature_flags(project_2))
+          .to eq('gitaly-feature-global-flag' => 'true',
+                 'gitaly-feature-project-flag' => 'false')
+      end
+    end
 
     context 'when table does not exist' do
       before do

spec/lib/gitlab/workhorse_spec.rb

@@ -15,9 +15,7 @@ RSpec.describe Gitlab::Workhorse do
   end
 
   before do
-    allow(Feature::Gitaly).to receive(:server_feature_flags).and_return({
-      'gitaly-feature-foobar' => 'true'
-    })
+    stub_feature_flags(gitaly_enforce_requests_limits: true)
   end
 
   describe ".send_git_archive" do
@@ -43,7 +41,7 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq('git-archive')
       expect(params).to eq({
         'GitalyServer' => {
-          features: { 'gitaly-feature-foobar' => 'true' },
+          features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
           address: Gitlab::GitalyClient.address(project.repository_storage),
           token: Gitlab::GitalyClient.token(project.repository_storage)
         },
@@ -73,7 +71,7 @@ RSpec.describe Gitlab::Workhorse do
         expect(command).to eq('git-archive')
         expect(params).to eq({
           'GitalyServer' => {
-            features: { 'gitaly-feature-foobar' => 'true' },
+            features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
             address: Gitlab::GitalyClient.address(project.repository_storage),
             token: Gitlab::GitalyClient.token(project.repository_storage)
           },
@@ -124,7 +122,7 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq("git-format-patch")
       expect(params).to eq({
         'GitalyServer' => {
-          features: { 'gitaly-feature-foobar' => 'true' },
+          features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
           address: Gitlab::GitalyClient.address(project.repository_storage),
           token: Gitlab::GitalyClient.token(project.repository_storage)
         },
@@ -187,7 +185,7 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq("git-diff")
       expect(params).to eq({
         'GitalyServer' => {
-          features: { 'gitaly-feature-foobar' => 'true' },
+          features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
           address: Gitlab::GitalyClient.address(project.repository_storage),
           token: Gitlab::GitalyClient.token(project.repository_storage)
         },
@@ -274,7 +272,7 @@ RSpec.describe Gitlab::Workhorse do
       let(:gitaly_params) do
         {
           GitalyServer: {
-            features: { 'gitaly-feature-foobar' => 'true' },
+            features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
             address: Gitlab::GitalyClient.address('default'),
             token: Gitlab::GitalyClient.token('default')
           }
@@ -310,6 +308,35 @@ RSpec.describe Gitlab::Workhorse do
 
           it { is_expected.to include(ShowAllRefs: true) }
         end
+
+        context 'when a feature flag is set for a single project' do
+          before do
+            stub_feature_flags(gitaly_mep_mep: project)
+          end
+
+          it 'sets the flag to true for that project' do
+            response = described_class.git_http_ok(repository, Gitlab::GlRepository::PROJECT, user, action)
+
+            expect(response.dig(:GitalyServer, :features)).to eq('gitaly-feature-enforce-requests-limits' => 'true',
+                                                                 'gitaly-feature-mep-mep' => 'true')
+          end
+
+          it 'sets the flag to false for other projects' do
+            other_project = create(:project, :public, :repository)
+            response = described_class.git_http_ok(other_project.repository, Gitlab::GlRepository::PROJECT, user, action)
+
+            expect(response.dig(:GitalyServer, :features)).to eq('gitaly-feature-enforce-requests-limits' => 'true',
+                                                                 'gitaly-feature-mep-mep' => 'false')
+          end
+
+          it 'sets the flag to false when there is no project' do
+            snippet = create(:personal_snippet, :repository)
+            response = described_class.git_http_ok(snippet.repository, Gitlab::GlRepository::SNIPPET, user, action)
+
+            expect(response.dig(:GitalyServer, :features)).to eq('gitaly-feature-enforce-requests-limits' => 'true',
+                                                                 'gitaly-feature-mep-mep' => 'false')
+          end
+        end
       end
 
       context "when git_receive_pack action is passed" do
@@ -423,7 +450,7 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq('git-blob')
       expect(params).to eq({
         'GitalyServer' => {
-          features: { 'gitaly-feature-foobar' => 'true' },
+          features: { 'gitaly-feature-enforce-requests-limits' => 'true' },
           address: Gitlab::GitalyClient.address(project.repository_storage),
           token: Gitlab::GitalyClient.token(project.repository_storage)
         },
@@ -485,7 +512,7 @@ RSpec.describe Gitlab::Workhorse do
       expect(command).to eq('git-snapshot')
       expect(params).to eq(
         'GitalyServer' => {
-          'features' => { 'gitaly-feature-foobar' => 'true' },
+          'features' => { 'gitaly-feature-enforce-requests-limits' => 'true' },
           'address' => Gitlab::GitalyClient.address(project.repository_storage),
           'token' => Gitlab::GitalyClient.token(project.repository_storage)
         },

spec/requests/api/internal/base_spec.rb

@@ -543,25 +543,51 @@ RSpec.describe API::Internal::Base do
       end
 
       context "git pull" do
-        before do
-          stub_feature_flags(gitaly_mep_mep: true)
+        context "with a feature flag enabled globally" do
+          before do
+            stub_feature_flags(gitaly_mep_mep: true)
+          end
+
+          it "has the correct payload" do
+            pull(key, project)
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response["status"]).to be_truthy
+            expect(json_response["gl_repository"]).to eq("project-#{project.id}")
+            expect(json_response["gl_project_path"]).to eq(project.full_path)
+            expect(json_response["gitaly"]).not_to be_nil
+            expect(json_response["gitaly"]["repository"]).not_to be_nil
+            expect(json_response["gitaly"]["repository"]["storage_name"]).to eq(project.repository.gitaly_repository.storage_name)
+            expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
+            expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
+            expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
+            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'true')
+            expect(user.reload.last_activity_on).to eql(Date.today)
+          end
         end
 
-        it "has the correct payload" do
-          pull(key, project)
+        context "with a feature flag enabled for a project" do
+          before do
+            stub_feature_flags(gitaly_mep_mep: project)
+          end
 
-          expect(response).to have_gitlab_http_status(:ok)
-          expect(json_response["status"]).to be_truthy
-          expect(json_response["gl_repository"]).to eq("project-#{project.id}")
-          expect(json_response["gl_project_path"]).to eq(project.full_path)
-          expect(json_response["gitaly"]).not_to be_nil
-          expect(json_response["gitaly"]["repository"]).not_to be_nil
-          expect(json_response["gitaly"]["repository"]["storage_name"]).to eq(project.repository.gitaly_repository.storage_name)
-          expect(json_response["gitaly"]["repository"]["relative_path"]).to eq(project.repository.gitaly_repository.relative_path)
-          expect(json_response["gitaly"]["address"]).to eq(Gitlab::GitalyClient.address(project.repository_storage))
-          expect(json_response["gitaly"]["token"]).to eq(Gitlab::GitalyClient.token(project.repository_storage))
-          expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'true')
-          expect(user.reload.last_activity_on).to eql(Date.today)
+          it "has the flag set to true for that project" do
+            pull(key, project)
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response["gl_repository"]).to eq("project-#{project.id}")
+            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'true')
+          end
+
+          it "has the flag set to false for other projects" do
+            other_project = create(:project, :public, :repository)
+
+            pull(key, other_project)
+
+            expect(response).to have_gitlab_http_status(:ok)
+            expect(json_response["gl_repository"]).to eq("project-#{other_project.id}")
+            expect(json_response["gitaly"]["features"]).to eq('gitaly-feature-mep-mep' => 'false')
+          end
         end
       end