Merge branch 'feature-flags-rollout-session-random' into 'master'

Support flexible rollout strategy in the API

See merge request gitlab-org/gitlab!43340

app/models/operations/feature_flags/strategy.rb

@@ -6,14 +6,17 @@ module Operations
       STRATEGY_DEFAULT = 'default'
       STRATEGY_GITLABUSERLIST = 'gitlabUserList'
       STRATEGY_GRADUALROLLOUTUSERID = 'gradualRolloutUserId'
+      STRATEGY_FLEXIBLEROLLOUT = 'flexibleRollout'
       STRATEGY_USERWITHID = 'userWithId'
       STRATEGIES = {
         STRATEGY_DEFAULT => [].freeze,
         STRATEGY_GITLABUSERLIST => [].freeze,
         STRATEGY_GRADUALROLLOUTUSERID => %w[groupId percentage].freeze,
+        STRATEGY_FLEXIBLEROLLOUT => %w[groupId rollout stickiness].freeze,
         STRATEGY_USERWITHID => ['userIds'].freeze
       }.freeze
       USERID_MAX_LENGTH = 256
+      STICKINESS_SETTINGS = %w[DEFAULT USERID SESSIONID RANDOM].freeze
 
       self.table_name = 'operations_strategies'
 
@@ -67,16 +70,25 @@ module Operations
         case name
         when STRATEGY_GRADUALROLLOUTUSERID
           gradual_rollout_user_id_parameters_validation
+        when STRATEGY_FLEXIBLEROLLOUT
+          flexible_rollout_parameters_validation
         when STRATEGY_USERWITHID
           FeatureFlagUserXidsValidator.validate_user_xids(self, :parameters, parameters['userIds'], 'userIds')
         end
       end
 
+      def within_range?(value, min, max)
+        return false unless value.is_a?(String)
+        return false unless value.match?(/\A\d+\z/)
+
+        value.to_i.between?(min, max)
+      end
+
       def gradual_rollout_user_id_parameters_validation
         percentage = parameters['percentage']
         group_id = parameters['groupId']
 
-        unless percentage.is_a?(String) && percentage.match(/\A[1-9]?[0-9]\z|\A100\z/)
+        unless within_range?(percentage, 0, 100)
           parameters_error('percentage must be a string between 0 and 100 inclusive')
         end
 
@@ -85,6 +97,25 @@ module Operations
         end
       end
 
+      def flexible_rollout_parameters_validation
+        stickiness = parameters['stickiness']
+        group_id = parameters['groupId']
+        rollout = parameters['rollout']
+
+        unless STICKINESS_SETTINGS.include?(stickiness)
+          options = STICKINESS_SETTINGS.to_sentence(last_word_connector: ', or ')
+          parameters_error("stickiness parameter must be #{options}")
+        end
+
+        unless group_id.is_a?(String) && group_id.match(/\A[a-z]{1,32}\z/)
+          parameters_error('groupId parameter is invalid')
+        end
+
+        unless within_range?(rollout, 0, 100)
+          parameters_error('rollout must be a string between 0 and 100 inclusive')
+        end
+      end
+
       def parameters_error(message)
         errors.add(:parameters, message)
         false

doc/api/feature_flags.md

@@ -151,7 +151,7 @@ POST /projects/:id/feature_flags
 | `description`       | string           | no         | The description of the feature flag.                                                   |
 | `active`            | boolean          | no         | The active state of the flag. Defaults to true. [Supported](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38350) in GitLab 13.3 and later. |
 | `strategies`        | JSON             | no         | The feature flag [strategies](../operations/feature_flags.md#feature-flag-strategies). |
-| `strategies:name`   | JSON             | no         | The strategy name.                                                                     |
+| `strategies:name`   | JSON             | no         | The strategy name. Can be `default`, `gradualRolloutUserId`, `userWithId`, or `gitlabUserList`. In [GitLab 13.5](https://gitlab.com/gitlab-org/gitlab/-/issues/36380) and later, can be [`flexibleRollout`](https://unleash.github.io/docs/activation_strategy#flexiblerollout). |
 | `strategies:parameters` | JSON         | no         | The strategy parameters.                                                               |
 | `strategies:scopes` | JSON             | no         | The scopes for the strategy.                                                           |
 | `strategies:scopes:environment_scope` | string | no | The environment spec for the scope.                                                    |

ee/app/controllers/projects/feature_flags_controller.rb

@@ -123,7 +123,9 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
       .permit(:name, :description, :active, :version,
               scopes_attributes: [:environment_scope, :active,
                                   strategies: [:name, parameters: [:groupId, :percentage, :userIds]]],
-             strategies_attributes: [:name, :user_list_id, parameters: [:groupId, :percentage, :userIds], scopes_attributes: [:environment_scope]])
+              strategies_attributes: [:name, :user_list_id,
+                                      parameters: [:groupId, :percentage, :userIds, :rollout, :stickiness],
+                                      scopes_attributes: [:environment_scope]])
   end
 
   def update_params
@@ -132,7 +134,7 @@ class Projects::FeatureFlagsController < Projects::ApplicationController
                   scopes_attributes: [:id, :environment_scope, :active, :_destroy,
                                       strategies: [:name, parameters: [:groupId, :percentage, :userIds]]],
                  strategies_attributes: [:id, :name, :user_list_id, :_destroy,
-                                         parameters: [:groupId, :percentage, :userIds],
+                                         parameters: [:groupId, :percentage, :userIds, :rollout, :stickiness],
                                          scopes_attributes: [:id, :environment_scope, :_destroy]])
   end
 

ee/changelogs/unreleased/feature-flags-rollout-session-random.yml

+---
+title: Support flexible rollout strategy in the API
+merge_request: 43340
+author:
+type: added

ee/spec/controllers/projects/feature_flags_controller_spec.rb

@@ -693,6 +693,39 @@ RSpec.describe Projects::FeatureFlagsController do
       end
     end
 
+    context 'when creating a version 2 feature flag with a flexibleRollout strategy' do
+      let(:params) do
+        {
+          namespace_id: project.namespace,
+          project_id: project,
+          operations_feature_flag: {
+            name: 'my_feature_flag',
+            active: true,
+            version: 'new_version_flag',
+            strategies_attributes: [{
+              name: 'flexibleRollout',
+              parameters: { groupId: 'default', rollout: '15', stickiness: 'DEFAULT' },
+              scopes_attributes: [{ environment_scope: 'production' }]
+            }]
+          }
+        }
+      end
+
+      it 'creates the new strategy' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:ok)
+
+        strategy_json = json_response['strategies'].first
+        expect(strategy_json['name']).to eq('flexibleRollout')
+        expect(strategy_json['parameters']).to eq({ 'groupId' => 'default', 'rollout' => '15', 'stickiness' => 'DEFAULT' })
+        expect(strategy_json['scopes'].count).to eq(1)
+
+        scope_json = strategy_json['scopes'].first
+        expect(scope_json['environment_scope']).to eq('production')
+      end
+    end
+
     context 'when creating a version 2 feature flag with a gitlabUserList strategy' do
       let!(:user_list) do
         create(:operations_feature_flag_user_list, project: project,
@@ -1360,6 +1393,24 @@ RSpec.describe Projects::FeatureFlagsController do
         expect(strategy_json['scopes']).to eq([])
       end
 
+      it 'creates a flexibleRollout strategy' do
+        put_request(new_version_flag, strategies_attributes: [{
+          name: 'flexibleRollout',
+          parameters: { groupId: 'default', rollout: '30', stickiness: 'DEFAULT' }
+        }])
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['strategies'].count).to eq(1)
+        strategy_json = json_response['strategies'].first
+        expect(strategy_json['name']).to eq('flexibleRollout')
+        expect(strategy_json['parameters']).to eq({
+          'groupId' => 'default',
+          'rollout' => '30',
+          'stickiness' => 'DEFAULT'
+        })
+        expect(strategy_json['scopes']).to eq([])
+      end
+
       it 'creates a gitlabUserList strategy' do
         user_list = create(:operations_feature_flag_user_list, project: project, name: 'My List', user_xids: 'user1,user2')
 

ee/spec/requests/api/feature_flags_spec.rb

@@ -446,7 +446,7 @@ RSpec.describe API::FeatureFlags do
         }])
       end
 
-      it 'creates a new feature flag with strategies with scopes' do
+      it 'creates a new feature flag with gradual rollout strategy with scopes' do
         params = {
           name: 'new-feature',
           version: 'new_version_flag',
@@ -476,6 +476,36 @@ RSpec.describe API::FeatureFlags do
         }])
       end
 
+      it 'creates a new feature flag with flexible rollout strategy with scopes' do
+        params = {
+          name: 'new-feature',
+          version: 'new_version_flag',
+          strategies: [{
+            name: 'flexibleRollout',
+            parameters: { groupId: 'default', rollout: '50', stickiness: 'DEFAULT' },
+            scopes: [{
+              environment_scope: 'staging'
+            }]
+          }]
+        }
+
+        post api("/projects/#{project.id}/feature_flags", user), params: params
+
+        expect(response).to have_gitlab_http_status(:created)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+
+        feature_flag = project.operations_feature_flags.last
+        expect(feature_flag.name).to eq(params[:name])
+        expect(feature_flag.version).to eq('new_version_flag')
+        expect(feature_flag.strategies.map { |s| s.slice(:name, :parameters).deep_symbolize_keys }).to eq([{
+          name: 'flexibleRollout',
+          parameters: { groupId: 'default', rollout: '50', stickiness: 'DEFAULT' }
+        }])
+        expect(feature_flag.strategies.first.scopes.map { |s| s.slice(:environment_scope).deep_symbolize_keys }).to eq([{
+          environment_scope: 'staging'
+        }])
+      end
+
       it 'returns a 422 when the feature flag is disabled' do
         stub_feature_flags(feature_flags_new_version: false)
         params = {
@@ -769,7 +799,7 @@ RSpec.describe API::FeatureFlags do
         expect(feature_flag.reload.description).to eq('old description')
       end
 
-      it 'returns an error for an invalid update' do
+      it 'returns an error for an invalid update of gradual rollout' do
         strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
         params = {
           strategies: [{
@@ -791,6 +821,28 @@ RSpec.describe API::FeatureFlags do
         }])
       end
 
+      it 'returns an error for an invalid update of flexible rollout' do
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        params = {
+          strategies: [{
+            id: strategy.id,
+            name: 'flexibleRollout',
+            parameters: { bad: 'params' }
+          }]
+        }
+
+        put api("/projects/#{project.id}/feature_flags/feature1", user), params: params
+
+        expect(response).to have_gitlab_http_status(:bad_request)
+        expect(json_response['message']).not_to be_nil
+        result = feature_flag.reload.strategies.map { |s| s.slice(:id, :name, :parameters).deep_symbolize_keys }
+        expect(result).to eq([{
+          id: strategy.id,
+          name: 'default',
+          parameters: {}
+        }])
+      end
+
       it 'updates the feature flag' do
         params = { description: 'new description' }
 
@@ -853,7 +905,7 @@ RSpec.describe API::FeatureFlags do
         })
       end
 
-      it 'updates an existing feature flag strategy' do
+      it 'updates an existing feature flag strategy to be gradual rollout strategy' do
         strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
         params = {
           strategies: [{
@@ -875,7 +927,29 @@ RSpec.describe API::FeatureFlags do
         }])
       end
 
-      it 'creates a new feature flag strategy' do
+      it 'updates an existing feature flag strategy to be flexible rollout strategy' do
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        params = {
+          strategies: [{
+            id: strategy.id,
+            name: 'flexibleRollout',
+            parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+          }]
+        }
+
+        put api("/projects/#{project.id}/feature_flags/feature1", user), params: params
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+        result = feature_flag.reload.strategies.map { |s| s.slice(:id, :name, :parameters).deep_symbolize_keys }
+        expect(result).to eq([{
+          id: strategy.id,
+          name: 'flexibleRollout',
+          parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+        }])
+      end
+
+      it 'adds a new gradual rollout strategy to a feature flag' do
         strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
         params = {
           strategies: [{
@@ -901,6 +975,32 @@ RSpec.describe API::FeatureFlags do
         }])
       end
 
+      it 'adds a new gradual flexible strategy to a feature flag' do
+        strategy = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
+        params = {
+          strategies: [{
+            name: 'flexibleRollout',
+            parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+          }]
+        }
+
+        put api("/projects/#{project.id}/feature_flags/feature1", user), params: params
+
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(response).to match_response_schema('public_api/v4/feature_flag', dir: 'ee')
+        result = feature_flag.reload.strategies
+          .map { |s| s.slice(:id, :name, :parameters).deep_symbolize_keys }
+          .sort_by { |s| s[:name] }
+        expect(result.first[:id]).to eq(strategy.id)
+        expect(result.map { |s| s.slice(:name, :parameters) }).to eq([{
+          name: 'default',
+          parameters: {}
+        }, {
+          name: 'flexibleRollout',
+          parameters: { groupId: 'default', rollout: '10', stickiness: 'DEFAULT' }
+        }])
+      end
+
       it 'deletes a feature flag strategy' do
         strategy_a = create(:operations_strategy, feature_flag: feature_flag, name: 'default', parameters: {})
         strategy_b = create(:operations_strategy, feature_flag: feature_flag,

spec/models/operations/feature_flags/strategy_spec.rb

@@ -8,7 +8,7 @@ RSpec.describe Operations::FeatureFlags::Strategy do
   describe 'validations' do
     it do
       is_expected.to validate_inclusion_of(:name)
-        .in_array(%w[default gradualRolloutUserId userWithId gitlabUserList])
+        .in_array(%w[default gradualRolloutUserId flexibleRollout userWithId gitlabUserList])
         .with_message('strategy name is invalid')
     end
 
@@ -55,9 +55,9 @@ RSpec.describe Operations::FeatureFlags::Strategy do
 
         describe 'percentage' do
           where(:invalid_value) do
-            [50, 40.0, { key: "value" }, "garbage", "00", "01", "101", "-1", "-10", "0100",
-             "1000", "10.0", "5%", "25%", "100hi", "e100", "30m", " ", "\r\n", "\n", "\t",
-             "\n10", "20\n", "\n100", "100\n", "\n  ", nil]
+            [50, 40.0, { key: "value" }, "garbage", "101", "-1", "-10", "1000", "10.0", "5%", "25%",
+             "100hi", "e100", "30m", " ", "\r\n", "\n", "\t", "\n10", "20\n", "\n100", "100\n",
+             "\n  ", nil]
           end
           with_them do
             it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
@@ -117,6 +117,134 @@ RSpec.describe Operations::FeatureFlags::Strategy do
         end
       end
 
+      context 'when the strategy name is flexibleRollout' do
+        valid_parameters = { rollout: '40', groupId: 'mygroup', stickiness: 'DEFAULT' }
+        where(invalid_parameters: [
+          nil,
+          {},
+          *valid_parameters.to_a.combination(1).to_a.map { |p| p.to_h },
+          *valid_parameters.to_a.combination(2).to_a.map { |p| p.to_h },
+          { **valid_parameters, userIds: '4' },
+          { **valid_parameters, extra: nil }
+        ])
+        with_them do
+          it 'must have valid parameters for the strategy' do
+            feature_flag = create(:operations_feature_flag, project: project)
+            strategy = described_class.create(feature_flag: feature_flag,
+                                              name: 'flexibleRollout',
+                                              parameters: invalid_parameters)
+
+            expect(strategy.errors[:parameters]).to eq(['parameters are invalid'])
+          end
+        end
+
+        [
+          [:rollout, '10'],
+          [:stickiness, 'DEFAULT'],
+          [:groupId, 'mygroup']
+        ].permutation(3).each do |parameters|
+          it "allows the parameters in the order #{parameters.map { |p| p.first }.join(', ')}" do
+            feature_flag = create(:operations_feature_flag, project: project)
+            strategy = described_class.create(feature_flag: feature_flag,
+                                              name: 'flexibleRollout',
+                                              parameters: Hash[parameters])
+
+            expect(strategy.errors[:parameters]).to be_empty
+          end
+        end
+
+        describe 'rollout' do
+          where(invalid_value: [50, 40.0, { key: "value" }, "garbage", "101", "-1", " ", "-10",
+                                "1000", "10.0", "5%", "25%", "100hi", "e100", "30m", "\r\n",
+                                "\n", "\t", "\n10", "20\n", "\n100", "100\n", "\n  ", nil])
+          with_them do
+            it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: 'DEFAULT', groupId: 'mygroup', rollout: invalid_value }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq([
+                'rollout must be a string between 0 and 100 inclusive'
+              ])
+            end
+          end
+
+          where(valid_value: %w[0 1 10 38 100 93])
+          with_them do
+            it 'must be a string value between 0 and 100 inclusive and without a percentage sign' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: 'DEFAULT', groupId: 'mygroup', rollout: valid_value }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq([])
+            end
+          end
+        end
+
+        describe 'groupId' do
+          where(invalid_value: [nil, 4, 50.0, {}, 'spaces bad', 'bad$', '%bad', '<bad', 'bad>',
+                                '!bad', '.bad', 'Bad', 'bad1', "", " ", "b" * 33, "ba_d", "ba\nd"])
+          with_them do
+            it 'must be a string value of up to 32 lowercase characters' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: 'DEFAULT', groupId: invalid_value, rollout: '40' }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq(['groupId parameter is invalid'])
+            end
+          end
+
+          where(valid_value: ["somegroup", "anothergroup", "okay", "g", "a" * 32])
+          with_them do
+            it 'must be a string value of up to 32 lowercase characters' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: 'DEFAULT', groupId: valid_value, rollout: '40' }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq([])
+            end
+          end
+        end
+
+        describe 'stickiness' do
+          where(invalid_value: [nil, " ", "default", "DEFAULT\n", "UserId", "USER", "USERID "])
+          with_them do
+            it 'must be a string representing a supported stickiness setting' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: invalid_value, groupId: 'mygroup', rollout: '40' }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq([
+                'stickiness parameter must be DEFAULT, USERID, SESSIONID, or RANDOM'
+              ])
+            end
+          end
+
+          where(valid_value: %w[DEFAULT USERID SESSIONID RANDOM])
+          with_them do
+            it 'must be a string representing a supported stickiness setting' do
+              feature_flag = create(:operations_feature_flag, project: project)
+              parameters = { stickiness: valid_value, groupId: 'mygroup', rollout: '40' }
+              strategy = described_class.create(feature_flag: feature_flag,
+                                                name: 'flexibleRollout',
+                                                parameters: parameters)
+
+              expect(strategy.errors[:parameters]).to eq([])
+            end
+          end
+        end
+      end
+
       context 'when the strategy name is userWithId' do
         where(:invalid_parameters) do
           [nil, { userIds: 'sam', percentage: '40' }, { userIds: 'sam', some: 'param' }, { percentage: '40' }, {}]
@@ -318,6 +446,32 @@ RSpec.describe Operations::FeatureFlags::Strategy do
           expect(strategy.errors[:user_list]).to be_empty
         end
       end
+
+      context 'when name is flexibleRollout' do
+        it 'is invalid when associated with a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          user_list = create(:operations_feature_flag_user_list, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'flexibleRollout',
+                                            user_list: user_list,
+                                            parameters: { groupId: 'default',
+                                                          rollout: '10',
+                                                          stickiness: 'DEFAULT' })
+
+          expect(strategy.errors[:user_list]).to eq(['must be blank'])
+        end
+
+        it 'is valid without a user list' do
+          feature_flag = create(:operations_feature_flag, project: project)
+          strategy = described_class.create(feature_flag: feature_flag,
+                                            name: 'flexibleRollout',
+                                            parameters: { groupId: 'default',
+                                                          rollout: '10',
+                                                          stickiness: 'DEFAULT' })
+
+          expect(strategy.errors[:user_list]).to be_empty
+        end
+      end
     end
   end
 end