Merge branch 'add-feature-flag-scope' into 'master'

Multiple environments support for feature flags (Unleash API standpoint)

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

db/schema.rb

@@ -2035,6 +2035,15 @@ ActiveRecord::Schema.define(version: 20190115054216) do
     t.index ["access_grant_id"], name: "index_oauth_openid_requests_on_access_grant_id", using: :btree
   end
 
+  create_table "operations_feature_flag_scopes", id: :bigserial, force: :cascade do |t|
+    t.bigint "feature_flag_id", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
+    t.boolean "active", null: false
+    t.string "environment_scope", default: "*", null: false
+    t.index ["feature_flag_id", "environment_scope"], name: "index_feature_flag_scopes_on_flag_id_and_environment_scope", unique: true, using: :btree
+  end
+
   create_table "operations_feature_flags", id: :bigserial, force: :cascade do |t|
     t.integer "project_id", null: false
     t.boolean "active", null: false
@@ -3430,6 +3439,7 @@ ActiveRecord::Schema.define(version: 20190115054216) do
   add_foreign_key "notes", "reviews", name: "fk_2e82291620", on_delete: :nullify
   add_foreign_key "notification_settings", "users", name: "fk_0c95e91db7", on_delete: :cascade
   add_foreign_key "oauth_openid_requests", "oauth_access_grants", column: "access_grant_id", name: "fk_oauth_openid_requests_oauth_access_grants_access_grant_id"
+  add_foreign_key "operations_feature_flag_scopes", "operations_feature_flags", column: "feature_flag_id", on_delete: :cascade
   add_foreign_key "operations_feature_flags", "projects", on_delete: :cascade
   add_foreign_key "operations_feature_flags_clients", "projects", on_delete: :cascade
   add_foreign_key "packages_maven_metadata", "packages_packages", column: "package_id", name: "fk_be88aed360", on_delete: :cascade

ee/app/models/concerns/has_environment_scope.rb

@@ -11,7 +11,18 @@ module HasEnvironmentScope
                 message: ::Gitlab::Regex.environment_scope_regex_message }
     )
 
-    scope :on_environment, -> (environment_name) do
+    ##
+    # Select rows which have a scope that matches the given environment name.
+    # Rows are ordered by relevance, by default. The most relevant row is
+    # placed at the end of a list.
+    #
+    # options:
+    #   - relevant_only: (boolean)
+    #     You can get the most relevant row only. Other rows are not be
+    #     selected even if its scope matches the environment name.
+    #     This is equivalent to using `#last` from SQL standpoint.
+    #
+    scope :on_environment, -> (environment_name, relevant_only: false) do
       where = <<~SQL
         environment_scope IN (:wildcard, :environment_name) OR
           :environment_name LIKE
@@ -55,8 +66,12 @@ module HasEnvironmentScope
       # In this case, B, C, and D would match. We also want to prioritize
       # the exact matched name, and put * last, and everything else in the
       # middle. So the order should be: D < C < B
-      where(where, values)
+      relation = where(where, values)
         .order(order % quoted_values) # `order` cannot escape for us!
+
+      relation = relation.reverse_order.limit(1) if relevant_only
+
+      relation
     end
   end
 

ee/app/models/operations/feature_flag.rb

 # frozen_string_literal: true
 
 module Operations
+  ##
+  # NOTE:
+  # "operations_feature_flags.active" column is not used in favor of
+  # operations_feature_flag_scopes's override policy.
+  # You can calculate actual `active` values with `for_environment` method.
   class FeatureFlag < ActiveRecord::Base
     self.table_name = 'operations_feature_flags'
 
     belongs_to :project
 
+    has_many :scopes, class_name: 'Operations::FeatureFlagScope'
+
     validates :project, presence: true
     validates :name,
       presence: true,
@@ -21,6 +28,22 @@ module Operations
     scope :enabled, -> { where(active: true) }
     scope :disabled, -> { where(active: false) }
 
+    scope :for_environment, -> (environment) do
+      select("operations_feature_flags.*" \
+             ", (#{actual_active_sql(environment)}) AS active")
+    end
+
+    class << self
+      def actual_active_sql(environment)
+        Operations::FeatureFlagScope
+          .where('operations_feature_flag_scopes.feature_flag_id = ' \
+                 'operations_feature_flags.id')
+          .on_environment(environment, relevant_only: true)
+          .select('active')
+          .to_sql
+      end
+    end
+
     def strategies
       [
         { name: 'default' }

ee/app/models/operations/feature_flag_scope.rb

+# frozen_string_literal: true
+
+module Operations
+  class FeatureFlagScope < ActiveRecord::Base
+    prepend HasEnvironmentScope
+
+    self.table_name = 'operations_feature_flag_scopes'
+
+    belongs_to :feature_flag
+
+    validates :environment_scope, uniqueness: {
+      scope: :feature_flag,
+      message: "(%{value}) has already been taken"
+    }
+
+    scope :enabled, -> { where(active: true) }
+    scope :disabled, -> { where(active: false) }
+  end
+end

ee/changelogs/unreleased/add-feature-flag-scope.yml

+---
+title: Multiple environments support for feature flags (Unleash API standpoint)
+merge_request: 9110
+author:
+type: added

ee/db/migrate/20190110200434_create_feature_flag_scopes.rb

+# frozen_string_literal: true
+
+class CreateFeatureFlagScopes < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :operations_feature_flag_scopes, id: :bigserial do |t|
+      t.bigint :feature_flag_id, null: false
+      t.timestamps_with_timezone null: false
+      t.boolean :active, null: false
+      t.string :environment_scope, default: "*", null: false
+      t.foreign_key :operations_feature_flags, column: :feature_flag_id, on_delete: :cascade
+
+      t.index [:feature_flag_id, :environment_scope],
+        unique: true,
+        name: 'index_feature_flag_scopes_on_flag_id_and_environment_scope'
+    end
+  end
+end

ee/lib/api/unleash.rb

@@ -9,6 +9,7 @@ module API
         params do
           requires :project_id, type: String, desc: 'The ID of a project'
           optional :instance_id, type: String, desc: 'The Instance ID of Unleash Client'
+          optional :app_name, type: String, desc: 'The Application Name of Unleash Client'
         end
         route_param :project_id do
           before do
@@ -23,12 +24,14 @@ module API
 
           desc 'Get a list of features (deprecated, v2 client support)'
           get 'features' do
-            present project, with: ::EE::API::Entities::UnleashFeatures
+            present :version, 1
+            present :features, feature_flags, with: ::EE::API::Entities::UnleashFeature
           end
 
           desc 'Get a list of features'
           get 'client/features' do
-            present project, with: ::EE::API::Entities::UnleashFeatures
+            present :version, 1
+            present :features, feature_flags, with: ::EE::API::Entities::UnleashFeature
           end
 
           post 'client/register' do
@@ -50,7 +53,11 @@ module API
       end
 
       def unleash_instance_id
-        params[:instance_id] || env['HTTP_UNLEASH_INSTANCEID']
+        env['HTTP_UNLEASH_INSTANCEID'] || params[:instance_id]
+      end
+
+      def unleash_app_name
+        env['HTTP_UNLEASH_APPNAME'] || params[:app_name]
       end
 
       def authorize_by_unleash_instance_id!
@@ -61,6 +68,17 @@ module API
       def authorize_feature_flags_feature!
         forbidden! unless project.feature_available?(:feature_flags)
       end
+
+      def feature_flags
+        if Feature.enabled?(:feature_flags_environment_scope, project: project)
+          return [] unless unleash_app_name.present?
+
+          project.operations_feature_flags.for_environment(unleash_app_name)
+                                          .ordered
+        else
+          project.operations_feature_flags.ordered
+        end
+      end
     end
   end
 end

ee/lib/ee/api/entities.rb

@@ -459,21 +459,6 @@ module EE
         expose :strategies
       end
 
-      class UnleashFeatures < Grape::Entity
-        expose :version
-        expose :features, with: UnleashFeature
-
-        private
-
-        def version
-          1
-        end
-
-        def features
-          object.operations_feature_flags.ordered
-        end
-      end
-
       class GitlabSubscription < Grape::Entity
         expose :plan do
           expose :plan_name, as: :code

ee/spec/factories/operations/feature_flag_scopes.rb

+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :operations_feature_flag_scope, class: Operations::FeatureFlagScope do
+    association :feature_flag, factory: :operations_feature_flag
+    active true
+    sequence(:environment_scope) { |n| "review/patch-#{n}" }
+  end
+end

ee/spec/models/concerns/has_environment_scope_spec.rb

@@ -19,11 +19,18 @@ describe HasEnvironmentScope do
     let!(:cluster1) { create(:cluster, projects: [project], environment_scope: '*') }
     let!(:cluster2) { create(:cluster, projects: [project], environment_scope: 'product/*') }
     let!(:cluster3) { create(:cluster, projects: [project], environment_scope: 'staging/*') }
-    let(:environment_name) { 'product/*' }
+    let(:environment_name) { 'product/canary-1' }
 
     it 'returns scoped objects' do
       expect(project.clusters.on_environment(environment_name)).to eq([cluster1, cluster2])
     end
+
+    context 'when relevant_only option is specified' do
+      it 'returns only one relevant object' do
+        expect(project.clusters.on_environment(environment_name, relevant_only: true))
+          .to eq([cluster2])
+      end
+    end
   end
 
   describe '#environment_scope=' do

ee/spec/models/operations/feature_flag_scope_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Operations::FeatureFlagScope do
+  describe 'associations' do
+    it { is_expected.to belong_to(:feature_flag) }
+  end
+
+  describe 'validations' do
+    context 'when duplicate environment scope is going to be created' do
+      let!(:existing_feature_flag_scope) do
+        create(:operations_feature_flag_scope)
+      end
+
+      let(:new_feature_flag_scope) do
+        build(:operations_feature_flag_scope,
+          feature_flag: existing_feature_flag_scope.feature_flag,
+          environment_scope: existing_feature_flag_scope.environment_scope)
+      end
+
+      it 'validates uniqueness of environment scope' do
+        new_feature_flag_scope.save
+
+        expect(new_feature_flag_scope.errors[:environment_scope])
+          .to include("(#{existing_feature_flag_scope.environment_scope})" \
+                      " has already been taken")
+      end
+    end
+  end
+
+  describe '.enabled' do
+    subject { described_class.enabled }
+
+    let!(:feature_flag_scope) do
+      create(:operations_feature_flag_scope, active: active)
+    end
+
+    context 'when scope is active' do
+      let(:active) { true }
+
+      it 'returns the scope' do
+        is_expected.to eq([feature_flag_scope])
+      end
+    end
+
+    context 'when scope is inactive' do
+      let(:active) { false }
+
+      it 'returns an empty array' do
+        is_expected.to be_empty
+      end
+    end
+  end
+
+  describe '.disabled' do
+    subject { described_class.disabled }
+
+    let!(:feature_flag_scope) do
+      create(:operations_feature_flag_scope, active: active)
+    end
+
+    context 'when scope is active' do
+      let(:active) { true }
+
+      it 'returns an empty array' do
+        is_expected.to be_empty
+      end
+    end
+
+    context 'when scope is inactive' do
+      let(:active) { false }
+
+      it 'returns the scope' do
+        is_expected.to eq([feature_flag_scope])
+      end
+    end
+  end
+end

ee/spec/models/operations/feature_flag_spec.rb

 require 'spec_helper'
 
 describe Operations::FeatureFlag do
+  include FeatureFlagHelpers
+
   subject { create(:operations_feature_flag) }
 
   describe 'associations' do
     it { is_expected.to belong_to(:project) }
+    it { is_expected.to have_many(:scopes) }
   end
 
   describe 'validations' do
@@ -12,4 +15,79 @@ describe Operations::FeatureFlag do
     it { is_expected.to validate_presence_of(:name) }
     it { is_expected.to validate_uniqueness_of(:name).scoped_to(:project_id) }
   end
+
+  describe '.for_environment' do
+    subject { described_class.for_environment(environment_name) }
+
+    before do
+      stub_feature_flags(feature_flags_environment_scope: true)
+    end
+
+    context 'when feature flag is off on production' do
+      before do
+        feature_flag = create(:operations_feature_flag)
+        create_scope(feature_flag, '*', true)
+        create_scope(feature_flag, 'production', false)
+      end
+
+      context 'when environment is production' do
+        let(:environment_name) { 'production' }
+
+        it 'returns actual active value' do
+          expect(subject.first.active).to be_falsy
+        end
+      end
+
+      context 'when environment is staging' do
+        let(:environment_name) { 'staging' }
+
+        it 'returns actual active value' do
+          expect(subject.first.active).to be_truthy
+        end
+      end
+    end
+
+    context 'when feature flag is default disabled but enabled for review apps' do
+      before do
+        feature_flag = create(:operations_feature_flag)
+        create_scope(feature_flag, '*', false)
+        create_scope(feature_flag, 'review/*', true)
+      end
+
+      context 'when environment is review app' do
+        let(:environment_name) { 'review/patch-1' }
+
+        it 'returns actual active value' do
+          expect(subject.first.active).to be_truthy
+        end
+      end
+
+      context 'when environment is production' do
+        let(:environment_name) { 'production' }
+
+        it 'returns actual active value' do
+          expect(subject.first.active).to be_falsy
+        end
+      end
+    end
+
+    context 'when there are two flags' do
+      let!(:feature_flag_1) { create(:operations_feature_flag) }
+      let!(:feature_flag_2) { create(:operations_feature_flag) }
+
+      before do
+        create_scope(feature_flag_1, '*', true)
+        create_scope(feature_flag_1, 'production', false)
+        create_scope(feature_flag_2, '*', true)
+      end
+
+      context 'when environment is production' do
+        let(:environment_name) { 'production' }
+
+        it 'returns multiple actual active values' do
+          expect(subject.ordered.map(&:active)).to eq([false, true])
+        end
+      end
+    end
+  end
 end

ee/spec/requests/api/unleash_spec.rb

 require 'spec_helper'
 
 describe API::Unleash do
+  include FeatureFlagHelpers
+
   set(:project) { create(:project) }
   let(:project_id) { project.id }
   let(:feature_enabled) { true }
@@ -66,6 +68,77 @@ describe API::Unleash do
     end
   end
 
+  shared_examples_for 'support multiple environments' do
+    let(:client) { create(:operations_feature_flags_client, project: project) }
+    let(:base_headers) { { "UNLEASH-INSTANCEID" => client.token } }
+    let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "test" }) }
+
+    let!(:feature_flag_1) do
+      create(:operations_feature_flag, project: project)
+    end
+
+    let!(:feature_flag_2) do
+      create(:operations_feature_flag, project: project)
+    end
+
+    before do
+      stub_feature_flags(feature_flags_environment_scope: true)
+      create_scope(feature_flag_1, '*', true)
+      create_scope(feature_flag_1, 'production', false)
+      create_scope(feature_flag_2, '*', false)
+      create_scope(feature_flag_2, 'review/*', true)
+    end
+
+    it 'does not have N+1 problem' do
+      recorded = ActiveRecord::QueryRecorder.new { subject }
+
+      expect(recorded.count).to be_within(8).of(10)
+    end
+
+    context 'when app name is staging' do
+      let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "staging" }) }
+
+      it 'returns correct active values' do
+        subject
+
+        expect(json_response['features'].first['enabled']).to be_truthy
+        expect(json_response['features'].second['enabled']).to be_falsy
+      end
+    end
+
+    context 'when app name is production' do
+      let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "production" }) }
+
+      it 'returns correct active values' do
+        subject
+
+        expect(json_response['features'].first['enabled']).to be_falsy
+        expect(json_response['features'].second['enabled']).to be_falsy
+      end
+    end
+
+    context 'when app name is review/patch-1' do
+      let(:headers) { base_headers.merge({ "UNLEASH-APPNAME" => "review/patch-1" }) }
+
+      it 'returns correct active values' do
+        subject
+
+        expect(json_response['features'].first['enabled']).to be_truthy
+        expect(json_response['features'].second['enabled']).to be_truthy
+      end
+    end
+
+    context 'when app name is empty' do
+      let(:headers) { base_headers }
+
+      it 'returns empty list' do
+        subject
+
+        expect(json_response['features'].count).to eq(0)
+      end
+    end
+  end
+
   %w(/feature_flags/unleash/:project_id/features /feature_flags/unleash/:project_id/client/features).each do |features_endpoint|
     describe "GET #{features_endpoint}" do
       let(:features_url) { features_endpoint.sub(':project_id', project_id) }
@@ -73,13 +146,18 @@ describe API::Unleash do
       subject { get api("/feature_flags/unleash/#{project_id}/features"), params: params, headers: headers }
 
       it_behaves_like 'authenticated request'
+      it_behaves_like 'support multiple environments'
 
       context 'with a list of feature flag' do
         let(:client) { create(:operations_feature_flags_client, project: project) }
-        let(:headers) { { "UNLEASH-INSTANCEID" => client.token }}
+        let(:headers) { { "UNLEASH-INSTANCEID" => client.token, "UNLEASH-APPNAME" => "production" }}
         let!(:enable_feature_flag) { create(:operations_feature_flag, project: project, name: 'feature1', active: true) }
         let!(:disabled_feature_flag) { create(:operations_feature_flag, project: project, name: 'feature2', active: false) }
 
+        before do
+          stub_feature_flags(feature_flags_environment_scope: false)
+        end
+
         it 'responds with a list' do
           subject
 

ee/spec/support/helpers/feature_flag_helpers.rb

+# frozen_string_literal: true
+
+module FeatureFlagHelpers
+  def create_scope(feature_flag, environment_scope, active)
+    create(:operations_feature_flag_scope,
+      feature_flag: feature_flag,
+      environment_scope: environment_scope,
+      active: active)
+  end
+end