From fe2491a4819b4f6c3031b3685f673f4155b3eda9 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Mon, 31 Mar 2025 08:10:00 +1300 Subject: [PATCH 1/2] Add Cop against dynamic FF keys Feature flag calls should be static as dynamic keys make it hard to be detect usage. --- .rubocop.yml | 3 + .../gitlab/feature_flag_key_dynamic.yml | 32 +++++++ .../groups/settings/work_items_controller.rb | 2 +- .../finders/issuables/custom_fields_finder.rb | 2 +- .../issuables/custom_field_resolver.rb | 2 +- .../custom_fields/archive_service.rb | 2 +- .../issuables/custom_fields/create_service.rb | 2 +- .../custom_fields/unarchive_service.rb | 2 +- .../issuables/custom_fields/update_service.rb | 2 +- ee/lib/api/ai/duo_workflows/workflows.rb | 2 +- .../cop/gitlab/feature_flag_key_dynamic.rb | 60 +++++++++++++ .../gitlab/feature_flag_key_dynamic_spec.rb | 90 +++++++++++++++++++ 12 files changed, 193 insertions(+), 8 deletions(-) create mode 100644 .rubocop_todo/gitlab/feature_flag_key_dynamic.yml create mode 100644 rubocop/cop/gitlab/feature_flag_key_dynamic.rb create mode 100644 spec/rubocop/cop/gitlab/feature_flag_key_dynamic_spec.rb diff --git a/.rubocop.yml b/.rubocop.yml index d829fbfc06c239..f929fba3fa0544 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -637,6 +637,9 @@ Cop/ActiveModelErrorsDirectManipulation: Gitlab/AvoidFeatureGet: Enabled: true +Gitlab/FeatureFlagKeyDynamic: + Enabled: true + RSpec/WebMockEnable: Enabled: true Include: diff --git a/.rubocop_todo/gitlab/feature_flag_key_dynamic.yml b/.rubocop_todo/gitlab/feature_flag_key_dynamic.yml new file mode 100644 index 00000000000000..e56cffff60c661 --- /dev/null +++ b/.rubocop_todo/gitlab/feature_flag_key_dynamic.yml @@ -0,0 +1,32 @@ +--- +# Cop supports --autocorrect. +Gitlab/FeatureFlagKeyDynamic: + Details: grace period + Exclude: + - 'app/graphql/resolvers/app_config/gitlab_instance_feature_flags_resolver.rb' + - 'app/graphql/resolvers/feature_flag_resolver.rb' + - 'app/services/concerns/measurable.rb' + - 'app/services/service_desk_settings/update_service.rb' + - 'app/workers/concerns/worker_attributes.rb' + - 'app/workers/loose_foreign_keys/cleanup_worker.rb' + - 'ee/app/controllers/remote_development/workspaces_feature_flag_controller.rb' + - 'ee/app/graphql/resolvers/ai/user_available_features_resolver.rb' + - 'ee/app/graphql/resolvers/ai/user_code_suggestions_contexts_resolver.rb' + - 'ee/app/models/concerns/geo/verifiable_replicator.rb' + - 'ee/app/services/search/zoekt/info_service.rb' + - 'ee/lib/gitlab/ai_gateway.rb' + - 'ee/lib/gitlab/geo/replicator.rb' + - 'ee/lib/gitlab/llm/chain/concerns/use_ai_gateway_agent_prompt.rb' + - 'ee/lib/gitlab/llm/completions_factory.rb' + - 'ee/lib/tasks/gitlab/nav/variant_generator.rb' + - 'ee/spec/graphql/resolvers/ai/user_available_features_resolver_spec.rb' + - 'ee/spec/models/gitlab_subscriptions/features_spec.rb' + - 'lib/feature/gitaly.rb' + - 'lib/gitlab/gon_helper.rb' + - 'lib/gitlab/redis/multi_store.rb' + - 'lib/gitlab/sidekiq_middleware/skip_jobs.rb' + - 'lib/gitlab/sidekiq_sharding/router.rb' + - 'lib/web_ide/extension_marketplace.rb' + - 'spec/lib/feature_spec.rb' + - 'spec/requests/api/features_spec.rb' + - 'spec/support_specs/helpers/stub_feature_flags_spec.rb' diff --git a/ee/app/controllers/groups/settings/work_items_controller.rb b/ee/app/controllers/groups/settings/work_items_controller.rb index 7980e267ad2a30..5a04e0d41e7345 100644 --- a/ee/app/controllers/groups/settings/work_items_controller.rb +++ b/ee/app/controllers/groups/settings/work_items_controller.rb @@ -17,7 +17,7 @@ def show; end def check_feature_availability render_404 unless group.licensed_feature_available?(:custom_fields) && - Feature.enabled?('custom_fields_feature', group) + Feature.enabled?(:custom_fields_feature, group) end def authorize_admin_work_item_settings diff --git a/ee/app/finders/issuables/custom_fields_finder.rb b/ee/app/finders/issuables/custom_fields_finder.rb index 355ba852f1818b..1d023cbf7a31d8 100644 --- a/ee/app/finders/issuables/custom_fields_finder.rb +++ b/ee/app/finders/issuables/custom_fields_finder.rb @@ -30,7 +30,7 @@ def initialize( end def execute - return Issuables::CustomField.none unless Feature.enabled?('custom_fields_feature', @group) + return Issuables::CustomField.none unless Feature.enabled?(:custom_fields_feature, @group) return Issuables::CustomField.none unless @group&.licensed_feature_available?(:custom_fields) return Issuables::CustomField.none unless @skip_permissions_check || diff --git a/ee/app/graphql/resolvers/issuables/custom_field_resolver.rb b/ee/app/graphql/resolvers/issuables/custom_field_resolver.rb index cfdec99068ec40..fb09cfb770159f 100644 --- a/ee/app/graphql/resolvers/issuables/custom_field_resolver.rb +++ b/ee/app/graphql/resolvers/issuables/custom_field_resolver.rb @@ -16,7 +16,7 @@ class CustomFieldResolver < BaseResolver def resolve(id:) custom_field = authorized_find!(id: id) - return unless Feature.enabled?('custom_fields_feature', custom_field.namespace) + return unless Feature.enabled?(:custom_fields_feature, custom_field.namespace) custom_field end diff --git a/ee/app/services/issuables/custom_fields/archive_service.rb b/ee/app/services/issuables/custom_fields/archive_service.rb index fda97323e91851..b324d74707aeeb 100644 --- a/ee/app/services/issuables/custom_fields/archive_service.rb +++ b/ee/app/services/issuables/custom_fields/archive_service.rb @@ -22,7 +22,7 @@ def initialize(custom_field:, **kwargs) end def execute - return FeatureNotAvailableError unless Feature.enabled?('custom_fields_feature', group) + return FeatureNotAvailableError unless Feature.enabled?(:custom_fields_feature, group) return NotAuthorizedError unless can?(current_user, :admin_custom_field, group) return AlreadyArchivedError unless custom_field.active? diff --git a/ee/app/services/issuables/custom_fields/create_service.rb b/ee/app/services/issuables/custom_fields/create_service.rb index f6002d72e6bb86..a8f5482bbdc8c7 100644 --- a/ee/app/services/issuables/custom_fields/create_service.rb +++ b/ee/app/services/issuables/custom_fields/create_service.rb @@ -13,7 +13,7 @@ class CreateService < BaseGroupService ) def execute - return FeatureNotAvailableError unless Feature.enabled?('custom_fields_feature', group) + return FeatureNotAvailableError unless Feature.enabled?(:custom_fields_feature, group) return NotAuthorizedError unless can?(current_user, :admin_custom_field, group) custom_field = Issuables::CustomField.new( diff --git a/ee/app/services/issuables/custom_fields/unarchive_service.rb b/ee/app/services/issuables/custom_fields/unarchive_service.rb index 459eb4ca9c561f..fe82b2ef78a63f 100644 --- a/ee/app/services/issuables/custom_fields/unarchive_service.rb +++ b/ee/app/services/issuables/custom_fields/unarchive_service.rb @@ -22,7 +22,7 @@ def initialize(custom_field:, **kwargs) end def execute - return FeatureNotAvailableError unless Feature.enabled?('custom_fields_feature', group) + return FeatureNotAvailableError unless Feature.enabled?(:custom_fields_feature, group) return NotAuthorizedError unless can?(current_user, :admin_custom_field, group) return AlreadyActiveError if custom_field.active? diff --git a/ee/app/services/issuables/custom_fields/update_service.rb b/ee/app/services/issuables/custom_fields/update_service.rb index a4f0ff66ad3bc4..88a2a9ed3f863d 100644 --- a/ee/app/services/issuables/custom_fields/update_service.rb +++ b/ee/app/services/issuables/custom_fields/update_service.rb @@ -21,7 +21,7 @@ def initialize(custom_field:, **kwargs) end def execute - return FeatureNotAvailableError unless Feature.enabled?('custom_fields_feature', group) + return FeatureNotAvailableError unless Feature.enabled?(:custom_fields_feature, group) return NotAuthorizedError unless can?(current_user, :admin_custom_field, group) store_old_associations! diff --git a/ee/lib/api/ai/duo_workflows/workflows.rb b/ee/lib/api/ai/duo_workflows/workflows.rb index fa8326a9c9dcb5..adb1f16aae047d 100644 --- a/ee/lib/api/ai/duo_workflows/workflows.rb +++ b/ee/lib/api/ai/duo_workflows/workflows.rb @@ -100,7 +100,7 @@ def create_workflow_params end post do - not_found! unless Feature.enabled?('duo_workflow', current_user) + not_found! unless Feature.enabled?(:duo_workflow, current_user) check_rate_limit!(:duo_workflow_direct_access, scope: current_user) do render_api_error!(_('This endpoint has been requested too many times. Try again later.'), 429) diff --git a/rubocop/cop/gitlab/feature_flag_key_dynamic.rb b/rubocop/cop/gitlab/feature_flag_key_dynamic.rb new file mode 100644 index 00000000000000..0cd0364535c101 --- /dev/null +++ b/rubocop/cop/gitlab/feature_flag_key_dynamic.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Gitlab + # The first argument to `Feature.enabled?` and `Feature.disabled?` should be a literal symbol. + # Dynamic keys are discouraged because it prevents detection of removed feature flags. + # + # @example + # + # # bad + # Feature.enabled?('some_flag') + # Feature.enabled?(flag_name) + # Feature.disabled?(flag) + # + # # good + # Feature.enabled?(:some_flag) + # Feature.disabled?(:other_flag) + class FeatureFlagKeyDynamic < RuboCop::Cop::Base + extend AutoCorrector + + MSG = 'First argument to `Feature.%s` must be a literal symbol.' + + FEATURE_METHODS = %i[enabled? disabled?].freeze + + # @!method feature_flag_method?(node) + def_node_matcher :feature_flag_method?, <<~PATTERN + (send + (const nil? :Feature) + ${:enabled? :disabled?} + ... + ) + PATTERN + + def on_send(node) + method_name = feature_flag_method?(node) + return unless method_name + + first_arg = node.first_argument + return if first_arg&.sym_type? + + message = format(MSG, method: method_name) + + add_offense(first_arg, message: message) do |corrector| + autocorrect(corrector, first_arg) if first_arg&.str_type? + end + end + alias_method :on_csend, :on_send + + private + + def autocorrect(corrector, arg_node) + return unless arg_node.str_type? + + corrector.replace(arg_node, ":#{arg_node.value}") + end + end + end + end +end diff --git a/spec/rubocop/cop/gitlab/feature_flag_key_dynamic_spec.rb b/spec/rubocop/cop/gitlab/feature_flag_key_dynamic_spec.rb new file mode 100644 index 00000000000000..08319a089151a5 --- /dev/null +++ b/spec/rubocop/cop/gitlab/feature_flag_key_dynamic_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'rubocop_spec_helper' + +require_relative '../../../../rubocop/cop/gitlab/feature_flag_key_dynamic' + +RSpec.describe RuboCop::Cop::Gitlab::FeatureFlagKeyDynamic, feature_category: :scalability do + context 'when calling Feature.enabled?' do + it 'registers an offense when using a variable as the first argument' do + expect_offense(<<~RUBY) + flag_name = :some_flag + Feature.enabled?(flag_name) + ^^^^^^^^^ First argument to `Feature.enabled?` must be a literal symbol. + RUBY + end + + it 'registers an offense when using a method call as the first argument' do + expect_offense(<<~RUBY) + Feature.enabled?(get_flag_name) + ^^^^^^^^^^^^^ First argument to `Feature.enabled?` must be a literal symbol. + RUBY + end + + it 'registers an offense when using a string as the first argument' do + expect_offense(<<~RUBY) + Feature.enabled?('some_flag') + ^^^^^^^^^^^ First argument to `Feature.enabled?` must be a literal symbol. + RUBY + + expect_correction(<<~RUBY) + Feature.enabled?(:some_flag) + RUBY + end + + it 'does not register an offense when using a literal symbol' do + expect_no_offenses(<<~RUBY) + Feature.enabled?(:some_flag) + RUBY + end + + it 'does not register an offense when using a literal symbol with additional arguments' do + expect_no_offenses(<<~RUBY) + Feature.enabled?(:some_flag, project) + RUBY + end + end + + context 'when calling Feature.disabled?' do + it 'registers an offense when using a variable as the first argument' do + expect_offense(<<~RUBY) + flag_name = :some_flag + Feature.disabled?(flag_name) + ^^^^^^^^^ First argument to `Feature.disabled?` must be a literal symbol. + RUBY + end + + it 'registers an offense when using a method call as the first argument' do + expect_offense(<<~RUBY) + Feature.disabled?(get_flag_name) + ^^^^^^^^^^^^^ First argument to `Feature.disabled?` must be a literal symbol. + RUBY + end + + it 'registers an offense when using a string as the first argument' do + expect_offense(<<~RUBY) + Feature.disabled?('some_flag') + ^^^^^^^^^^^ First argument to `Feature.disabled?` must be a literal symbol. + RUBY + + expect_correction(<<~RUBY) + Feature.disabled?(:some_flag) + RUBY + end + + it 'does not register an offense when using a literal symbol' do + expect_no_offenses(<<~RUBY) + Feature.disabled?(:some_flag) + RUBY + end + end + + context 'with non-Feature methods' do + it 'does not register an offense for methods on other objects' do + expect_no_offenses(<<~RUBY) + OtherClass.enabled?(flag_name) + something.disabled?(flag_name) + RUBY + end + end +end -- GitLab From cfe0caf0090fbab28f743165cfcb061f78f575a6 Mon Sep 17 00:00:00 2001 From: Thong Kuah Date: Wed, 2 Apr 2025 12:47:20 +1300 Subject: [PATCH 2/2] Clarify why ability to search is important It is because we want FF to be short lived, so being able to search any usage is important. --- rubocop/cop/gitlab/feature_flag_key_dynamic.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rubocop/cop/gitlab/feature_flag_key_dynamic.rb b/rubocop/cop/gitlab/feature_flag_key_dynamic.rb index 0cd0364535c101..8407fda1e66880 100644 --- a/rubocop/cop/gitlab/feature_flag_key_dynamic.rb +++ b/rubocop/cop/gitlab/feature_flag_key_dynamic.rb @@ -4,7 +4,12 @@ module RuboCop module Cop module Gitlab # The first argument to `Feature.enabled?` and `Feature.disabled?` should be a literal symbol. - # Dynamic keys are discouraged because it prevents detection of removed feature flags. + # Dynamic keys are discouraged because they are harder to explicitly search for in the codebase by name. + # Strings are similarly discouraged to simplify exact matching when searching for flag usage. + # + # Feature flags are technical debt (should be short lived), so it is important to ensure we can find all usages in + # order to remove the flag safely. + # More information at https://docs.gitlab.com/development/feature_flags/#feature-flag-definition-and-validation # # @example # -- GitLab