diff --git a/bin/feature-flag b/bin/feature-flag
index b5e7889be1d7bc85f7ff40a1765e4bfd26bba1dd..43e273be1fa914dc255565366b74540155fce191 100755
--- a/bin/feature-flag
+++ b/bin/feature-flag
@@ -126,6 +126,8 @@ class FeatureFlagOptionParser
$stdout.puts ">> Specify the feature flag type:"
$stdout.puts
TYPES.each do |type, data|
+ next if data[:deprecated]
+
$stdout.puts "#{type.to_s.rjust(15)}#{' '*6}#{data[:description]}"
end
@@ -133,7 +135,7 @@ class FeatureFlagOptionParser
$stdout.print "?> "
type = $stdin.gets.strip.to_sym
- return type if TYPES[type]
+ return type if TYPES[type] && !TYPES[type][:deprecated]
$stderr.puts "Invalid type specified '#{type}'"
end
diff --git a/config/initializers/0_inject_feature_flags.rb b/config/initializers/0_inject_feature_flags.rb
index 5b33b3bb4eab555d181e35b9caf30df25003c3a3..1e0c3c01abae7b3b9118a3db95a24a8a746172e7 100644
--- a/config/initializers/0_inject_feature_flags.rb
+++ b/config/initializers/0_inject_feature_flags.rb
@@ -4,3 +4,49 @@
Feature.register_feature_groups
Feature.register_definitions
Feature.register_hot_reloader unless Rails.configuration.cache_classes
+
+# This disallows usage of licensed feature names with the same name
+# as feature flags. This naming collision creates confusion and it was
+# decided to be removed in favor of explicit check.
+# https://gitlab.com/gitlab-org/gitlab/-/issues/259611
+if Gitlab.ee? && Gitlab.dev_or_test_env?
+ # These are the names of feature flags that do violate the constraint of
+ # being unique to licensed names. These feature flags should be reworked to
+ # be "development" with explicit check
+ IGNORED_FEATURE_FLAGS = %i[
+ resource_access_token
+ ci_secrets_management
+ feature_flags_related_issues
+ group_coverage_reports
+ group_wikis
+ incident_sla
+ swimlanes
+ minimal_access_role
+ ].to_set
+
+ # First, we validate a list of overrides to ensure that these overrides
+ # are removed if feature flag is gone
+ missing_feature_flags = IGNORED_FEATURE_FLAGS.reject do |feature_flag|
+ Feature::Definition.definitions[feature_flag]
+ end
+
+ if missing_feature_flags.any?
+ raise "The following feature flags were added as an override for discovering licensed features. " \
+ "Since these feature flags seems to be gone, ensure to remove them from \`IGNORED_FEATURE_FLAGS\` " \
+ "in \`#{__FILE__}'`: #{missing_feature_flags.join(", ")}"
+ end
+
+ # Second, we validate that there's no feature flag under the name as licensed feature
+ # flag, to ensure that the name used, is unique
+ licensed_features = License::PLANS_BY_FEATURE.keys.select do |licensed_feature_name|
+ IGNORED_FEATURE_FLAGS.exclude?(licensed_feature_name) &&
+ Feature::Definition.definitions[licensed_feature_name]
+ end
+
+ if licensed_features.any?
+ raise "The following feature flags do use a licensed feature. " \
+ "To avoid the confusion between their usage it is disallowed to use feature flag " \
+ "with exact the same name as licensed feature name. Use a different name to create " \
+ "a distinction: #{licensed_features.join(", ")}"
+ end
+end
diff --git a/doc/development/feature_flags/development.md b/doc/development/feature_flags/development.md
index e6668b0fd038dc5a87e0356472a7c881bb5f99ec..6c672663baed4e74586c180b27e8b181fc35bd4c 100644
--- a/doc/development/feature_flags/development.md
+++ b/doc/development/feature_flags/development.md
@@ -61,30 +61,6 @@ Feature.disabled?(:my_ops_flag, project, type: :ops)
push_frontend_feature_flag(:my_ops_flag, project, type: :ops)
```
-### `licensed` type
-
-`licensed` feature flags are used to temporarily disable licensed features. There
-should be a one-to-one mapping of `licensed` feature flags to licensed features.
-
-`licensed` feature flags must be `default_enabled: true`, because that's the only
-supported option in the current implementation. This is under development as per
-the [related issue](https://gitlab.com/gitlab-org/gitlab/-/issues/218667).
-
-The `licensed` type has a dedicated set of functions to check if a licensed
-feature is available for a project or namespace. This check validates
-if the license is assigned to the namespace and feature flag itself.
-The `licensed` feature flag has the same name as a licensed feature name:
-
-```ruby
-# Good: checks if feature flag is enabled
-project.feature_available?(:my_licensed_feature)
-namespace.feature_available?(:my_licensed_feature)
-
-# Bad: licensed flag must be accessed via `feature_available?`
-Feature.enabled?(:my_licensed_feature, type: :licensed)
-push_frontend_feature_flag(:my_licensed_feature, type: :licensed)
-```
-
## Feature flag definition and validation
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/229161) in GitLab 13.3.
@@ -309,25 +285,16 @@ used as an actor for `Feature.enabled?`.
### Feature flags for licensed features
-The [`Project#feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/app/models/project_feature.rb#L63-68),
-[`Namespace#feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/ee/namespace.rb#L71-85) (EE), and
-[`License.feature_available?`](https://gitlab.com/gitlab-org/gitlab/blob/4cc1c62918aa4c31750cb21dfb1a6c3492d71080/ee/app/models/license.rb#L293-300) (EE) methods all implicitly check for
-a by default enabled feature flag with the same name as the provided argument.
-
-**An important side-effect of the implicit feature flags mentioned above is that
-unless the feature is explicitly disabled or limited to a percentage of users,
-the feature flag check defaults to `true`.**
-
-NOTE: **Note:**
-Due to limitations with `feature_available?`, the YAML definition for `licensed` feature
-flags accepts only `default_enabled: true`. This is under development as per the
-[related issue](https://gitlab.com/gitlab-org/gitlab/-/issues/218667).
+You can't use a feature flag with the same name as a licensed feature name, because
+it would cause a naming collision. This was [widely discussed and removed](https://gitlab.com/gitlab-org/gitlab/-/issues/259611)
+because it is confusing.
-If you want a licensed feature to be disabled by default or enabled only for a given gate, you can use a feature flag with a different name. The feature checks would then
-look like:
+To check for licensed features, add a dedicated feature flag under a different name
+and check it explicitly, for example:
```ruby
-Feature.enabled?(:licensed_feature_feature_flag, project) && project.feature_available?(:licensed_feature)
+Feature.enabled?(:licensed_feature_feature_flag, project) &&
+ project.feature_available?(:licensed_feature)
```
### Feature groups
diff --git a/ee/lib/ee/api/features.rb b/ee/lib/ee/api/features.rb
new file mode 100644
index 0000000000000000000000000000000000000000..a7d2cf644adb7f8923f7e39d15703d0826fcc15d
--- /dev/null
+++ b/ee/lib/ee/api/features.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module EE
+ module API
+ module Features
+ extend ActiveSupport::Concern
+
+ prepended do
+ helpers do
+ extend ::Gitlab::Utils::Override
+
+ override :validate_licensed_name!
+ def validate_feature_flag_name!(name)
+ super
+
+ if License::PLANS_BY_FEATURE[name.to_sym]
+ bad_request!(
+ "The '#{name}' is a licensed feature name, " \
+ "and thus it cannot be used as a feature flag name. " \
+ "Use `rails console` to set this feature flag state."
+ )
+ end
+ end
+ end
+ end
+ end
+ end
+end
diff --git a/ee/spec/requests/api/features_spec.rb b/ee/spec/requests/api/features_spec.rb
index ed9ef598cb31ef162050805d038ac9de3bb388c5..920abc8e4b70c6fb295163ae322133166049f43d 100644
--- a/ee/spec/requests/api/features_spec.rb
+++ b/ee/spec/requests/api/features_spec.rb
@@ -32,6 +32,18 @@
end.to change(Geo::CacheInvalidationEvent, :count).by(1)
end
end
+
+ context 'when licensed feature name is given' do
+ let(:feature_name) do
+ License::PLANS_BY_FEATURE.each_key.first
+ end
+
+ it 'returns bad request' do
+ post api("/features/#{feature_name}", admin), params: { value: 'true' }
+
+ expect(response).to have_gitlab_http_status(:bad_request)
+ end
+ end
end
describe 'DELETE /feature/:name' do
diff --git a/lib/api/features.rb b/lib/api/features.rb
index 5d2e545abd623594c2c0d546c8bf26f7a375f540..70feef12cebd1880a7ed416e7feb5ca360d7fe6b 100644
--- a/lib/api/features.rb
+++ b/lib/api/features.rb
@@ -61,6 +61,8 @@ def gate_specified?(params)
mutually_exclusive :key, :project
end
post ':name' do
+ validate_feature_flag_name!(params[:name])
+
feature = Feature.get(params[:name]) # rubocop:disable Gitlab/AvoidFeatureGet
targets = gate_targets(params)
value = gate_value(params)
@@ -97,5 +99,13 @@ def gate_specified?(params)
no_content!
end
end
+
+ helpers do
+ def validate_feature_flag_name!(name)
+ # no-op
+ end
+ end
end
end
+
+API::Features.prepend_if_ee('EE::API::Features')
diff --git a/lib/feature/shared.rb b/lib/feature/shared.rb
index 9ec56ee6b52508315b2b679acd960c9e2ceb6b16..5716bc007a60bac4817f1194dfb8b301af81216a 100644
--- a/lib/feature/shared.rb
+++ b/lib/feature/shared.rb
@@ -10,6 +10,8 @@ module Shared
# rollout_issue: defines if `bin/feature-flag` asks for rollout issue
# default_enabled: defines a default state of a feature flag when created by `bin/feature-flag`
# ee_only: defines that a feature flag can only be created in a context of EE
+ # deprecated: defines if a feature flag type that is deprecated and to be removed,
+ # the deprecated types are hidden from all interfaces
# example: usage being shown when exception is raised
TYPES = {
development: {
@@ -37,6 +39,7 @@ module Shared
},
licensed: {
description: 'Permanent feature flags used to temporarily disable licensed features.',
+ deprecated: true,
optional: true,
rollout_issue: false,
ee_only: true,
diff --git a/spec/bin/feature_flag_spec.rb b/spec/bin/feature_flag_spec.rb
index 185a03fc587953d6c69024eb264e8a989d5525fa..8e0bed5b769053a6621bb6898c767a2723b9b7ca 100644
--- a/spec/bin/feature_flag_spec.rb
+++ b/spec/bin/feature_flag_spec.rb
@@ -123,6 +123,29 @@
end
end
+ context 'when there is deprecated feature flag type' do
+ before do
+ stub_const('FeatureFlagOptionParser::TYPES',
+ development: { description: 'short' },
+ deprecated: { description: 'deprecated', deprecated: true }
+ )
+ end
+
+ context 'and deprecated type is given' do
+ let(:type) { 'deprecated' }
+
+ it 'shows error message and retries' do
+ expect($stdin).to receive(:gets).and_return(type)
+ expect($stdin).to receive(:gets).and_raise('EOF')
+
+ expect do
+ expect { described_class.read_type }.to raise_error(/EOF/)
+ end.to output(/Specify the feature flag type/).to_stdout
+ .and output(/Invalid type specified/).to_stderr
+ end
+ end
+ end
+
context 'when there are many types defined' do
before do
stub_const('FeatureFlagOptionParser::TYPES',