Multiple environments support for feature flags

Support for Unleash API

Elaborate a comment in has environment scope

Fix specs

Complete test for unleash API

Add changelog

Fix coding offence

Fix coding offence

Fix ee specific line check

Fix MySQL problem

Fix typo

Improve comment

Fix spec location

Override active value with alias

Add test and feature flag

Add frozen string literal

Default enable true

Decouple default scope related things

Decouple FeatureFlagsFinder change

Fix unleash api app name handling

Add helper for feature flag tests

Reorder columns

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