Commit c56cb312 authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Shinya Maeda

Disallow of feature flags to use licensed names

This ensures that all feature flags are explicitly
used, and have a unique meaning.

This work does:
- hide `licensed` from `bin/feature-flag`
- discovers the feature flag names to collide with licensed feature
  names
- disallows to use licensed names in feature flag API
  (unless `force=1`)
- removes mentions about implicit feature flags

This does not:
- remove implicit feature flag check
- the removal of implicit feature flag will be done at some point later
parent 44f97ac3
......@@ -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
......
......@@ -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
......@@ -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
......
# 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
......@@ -32,6 +32,18 @@ RSpec.describe API::Features, stub_feature_flags: false do
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
......
......@@ -61,6 +61,8 @@ module API
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 @@ module API
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')
......@@ -10,6 +10,8 @@ class Feature
# 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 @@ class Feature
},
licensed: {
description: 'Permanent feature flags used to temporarily disable licensed features.',
deprecated: true,
optional: true,
rollout_issue: false,
ee_only: true,
......
......@@ -123,6 +123,29 @@ RSpec.describe 'bin/feature-flag' do
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',
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment