Automatic merge of gitlab-org/gitlab master

config/feature_flags/development/vulnerability_finding_replace_metadata.yml

+---
+name: vulnerability_finding_replace_metadata
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66868
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/337253
+group: group::threat insights
+type: development
+default_enabled: false
\ No newline at end of file

config/metrics/counts_all/20210216182006_source_code_pushes.yml

@@ -10,6 +10,10 @@ value_type: number
 status: data_available
 time_frame: all
 data_source: redis
+instrumentation_class: RedisMetric
+options:
+  counter_class: SourceCodeCounter
+  event: pushes
 distribution:
 - ce
 - ee

doc/development/service_ping/metrics_instrumentation.md

@@ -11,7 +11,7 @@ This guide describes how to develop Service Ping metrics using metrics instrumen
 ## Nomenclature
 
 - **Instrumentation class**:
-  - Inherits one of the metric classes: `DatabaseMetric`, `RedisHLLMetric` or `GenericMetric`.
+  - Inherits one of the metric classes: `DatabaseMetric`, `RedisMetric`, `RedisHLLMetric` or `GenericMetric`.
   - Implements the logic that calculates the value for a Service Ping metric.
 
 - **Metric definition**
@@ -24,7 +24,7 @@ This guide describes how to develop Service Ping metrics using metrics instrumen
 
 A metric definition has the [`instrumentation_class`](metrics_dictionary.md) field, which can be set to a class.
 
-The defined instrumentation class should have one of the existing metric classes: `DatabaseMetric`, `RedisHLLMetric`, or `GenericMetric`.
+The defined instrumentation class should have one of the existing metric classes: `DatabaseMetric`, `RedisMetric`, `RedisHLLMetric`, or `GenericMetric`.
 
 Using the instrumentation classes ensures that metrics can fail safe individually, without breaking the entire
  process of Service Ping generation.
@@ -51,6 +51,26 @@ module Gitlab
 end
 ```
 
+## Redis metrics
+
+[Example of a merge request that adds a `Redis` metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/66582).
+
+Count unique values for `source_code_pushes` event.
+
+Required options:
+
+- `event`: the event name.
+- `counter_class`: one of the counter classes from the `Gitlab::UsageDataCounters` namespace; it should implement `read` method or inherit it from `BaseCounter`.
+
+```yaml
+time_frame: all
+data_source: redis
+instrumentation_class: 'RedisMetric'
+options:
+  event: pushes
+  counter_class: SourceCodeCounter
+```
+
 ## Redis HyperLogLog metrics
 
 [Example of a merge request that adds a `RedisHLL` metric](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/61685).
@@ -60,7 +80,7 @@ Count unique values for `i_quickactions_approve` event.
 ```yaml
 time_frame: 28d
 data_source: redis_hll
-instrumentation_class: 'Gitlab::Usage::Metrics::Instrumentations::RedisHLLMetric'
+instrumentation_class: 'RedisHLLMetric'
 options:
   events:
     - i_quickactions_approve
@@ -91,13 +111,13 @@ end
 There is support for:
 
 - `count`, `distinct_count` for [database metrics](#database-metrics).
+- [Redis metrics](#redis-metrics).
 - [Redis HLL metrics](#redis-hyperloglog-metrics).
 - [Generic metrics](#generic-metrics), which are metrics based on settings or configurations.
 
 Currently, there is no support for:
 
 - `add`, `sum`, `histogram`, `estimate_batch_distinct_count` for database metrics.
-- Regular Redis counters.
 
 You can [track the progress to support these](https://gitlab.com/groups/gitlab-org/-/epics/6118).
 
@@ -107,7 +127,7 @@ To create a stub instrumentation for a Service Ping metric, you can use a dedica
 
 The generator takes the class name as an argument and the following options:
 
-- `--type=TYPE` Required. Indicates the metric type. It must be one of: `database`, `generic`.
+- `--type=TYPE` Required. Indicates the metric type. It must be one of: `database`, `generic`, `redis`.
 - `--operation` Required for `database` type. It must be one of: `count`, `distinct_count`.
 - `--ee` Indicates if the metric is for EE.
 

ee/app/models/vulnerabilities/finding.rb

@@ -209,7 +209,7 @@ module Vulnerabilities
     end
 
     def links
-      return metadata.fetch('links', []) if finding_links.load.empty?
+      return metadata.fetch('links', []) if Feature.disabled?(:vulnerability_finding_replace_metadata) || finding_links.load.empty?
 
       finding_links.as_json(only: [:name, :url])
     end

ee/spec/models/vulnerabilities/finding_spec.rb

@@ -12,6 +12,7 @@ RSpec.describe Vulnerabilities::Finding do
   with_them do
     before do
       stub_feature_flags(vulnerability_finding_tracking_signatures: vulnerability_finding_signatures_enabled)
+      stub_feature_flags(vulnerability_finding_replace_metadata: false)
       stub_licensed_features(vulnerability_finding_signatures: vulnerability_finding_signatures_enabled)
     end
 
@@ -360,7 +361,7 @@ RSpec.describe Vulnerabilities::Finding do
         create(
           :vulnerabilities_finding,
           raw_metadata: {
-            links: [{ url: 'https://raw.gitlab.com', name: 'raw_metadata_link' }]
+            links: [{ url: 'https://raw.example.com', name: 'raw_metadata_link' }]
           }.to_json
         )
       end
@@ -369,15 +370,27 @@ RSpec.describe Vulnerabilities::Finding do
 
       context 'when there are no finding links' do
         it 'returns links from raw_metadata' do
-          expect(links).to eq([{ 'url' => 'https://raw.gitlab.com', 'name' => 'raw_metadata_link' }])
+          expect(links).to eq([{ 'url' => 'https://raw.example.com', 'name' => 'raw_metadata_link' }])
         end
       end
 
       context 'when there are finding links assigned to given finding' do
-        let_it_be(:finding_link) { create(:finding_link, name: 'finding_link', url: 'https://link.gitlab.com', finding: finding) }
+        let_it_be(:finding_link) { create(:finding_link, name: 'finding_link', url: 'https://link.example.com', finding: finding) }
 
-        it 'returns links from finding link' do
-          expect(links).to eq([{ 'url' => 'https://link.gitlab.com', 'name' => 'finding_link' }])
+        context 'when the feature flag is enabled' do
+          before do
+            stub_feature_flags(vulnerability_finding_replace_metadata: true)
+          end
+
+          it 'returns links from finding link' do
+            expect(links).to eq([{ 'url' => 'https://link.example.com', 'name' => 'finding_link' }])
+          end
+        end
+
+        context 'when the feature flag is disabled' do
+          it 'returns links from raw_metadata' do
+            expect(links).to eq([{ 'url' => 'https://raw.example.com', 'name' => 'raw_metadata_link' }])
+          end
         end
       end
     end

lib/backup/manager.rb

@@ -232,7 +232,7 @@ module Backup
     end
 
     def folders_to_backup
-      FOLDERS_TO_BACKUP.reject { |name| skipped?(name) }
+      FOLDERS_TO_BACKUP.select { |name| !skipped?(name) && Dir.exist?(File.join(backup_path, name)) }
     end
 
     def disabled_features

lib/generators/gitlab/usage_metric/usage_metric_generator.rb

@@ -11,7 +11,8 @@ module Gitlab
 
     ALLOWED_SUPERCLASSES = {
       generic: 'Generic',
-      database: 'Database'
+      database: 'Database',
+      redis: 'Redis'
     }.freeze
 
     ALLOWED_OPERATIONS = %w(count distinct_count).freeze

lib/gitlab/usage/metrics/instrumentations/redis_metric.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Usage
+    module Metrics
+      module Instrumentations
+        # Usage example
+        #
+        # In metric YAML definition:
+        #
+        # instrumentation_class: RedisMetric
+        # options:
+        #   event: pushes
+        #   counter_class: SourceCodeCounter
+        #
+        class RedisMetric < BaseMetric
+          def initialize(time_frame:, options: {})
+            super
+
+            raise ArgumentError, "'event' option is required" unless metric_event.present?
+            raise ArgumentError, "'counter class' option is required" unless counter_class.present?
+          end
+
+          def metric_event
+            options[:event]
+          end
+
+          def counter_class_name
+            options[:counter_class]
+          end
+
+          def counter_class
+            "Gitlab::UsageDataCounters::#{counter_class_name}".constantize
+          end
+
+          def value
+            redis_usage_data do
+              counter_class.read(metric_event)
+            end
+          end
+
+          def suggested_name
+            Gitlab::Usage::Metrics::NameSuggestion.for(:redis)
+          end
+        end
+      end
+    end
+  end
+end

lib/sidebars/projects/menus/packages_registries_menu.rb

@@ -31,7 +31,7 @@ module Sidebars
         private
 
         def packages_registry_menu_item
-          if !::Gitlab.config.packages.enabled || !can?(context.current_user, :read_package, context.project)
+          if packages_registry_disabled?
             return ::Sidebars::NilMenuItem.new(item_id: :packages_registry)
           end
 
@@ -58,7 +58,7 @@ module Sidebars
         end
 
         def infrastructure_registry_menu_item
-          if Feature.disabled?(:infrastructure_registry_page, context.current_user, default_enabled: :yaml)
+          if Feature.disabled?(:infrastructure_registry_page, context.current_user, default_enabled: :yaml) || packages_registry_disabled?
             return ::Sidebars::NilMenuItem.new(item_id: :infrastructure_registry)
           end
 
@@ -69,6 +69,10 @@ module Sidebars
             item_id: :infrastructure_registry
           )
         end
+
+        def packages_registry_disabled?
+          !::Gitlab.config.packages.enabled || !can?(context.current_user, :read_package, context.project)
+        end
       end
     end
   end

rubocop/rubocop-usage-data.yml

@@ -122,3 +122,4 @@ UsageData/InstrumentationSuperclass:
     - :DatabaseMetric
     - :GenericMetric
     - :RedisHLLMetric
+    - :RedisMetric

spec/lib/backup/manager_spec.rb

@@ -12,20 +12,13 @@ RSpec.describe Backup::Manager do
   before do
     allow(progress).to receive(:puts)
     allow(progress).to receive(:print)
-
-    @old_progress = $progress # rubocop:disable Style/GlobalVars
-    $progress = progress # rubocop:disable Style/GlobalVars
-  end
-
-  after do
-    $progress = @old_progress # rubocop:disable Style/GlobalVars
   end
 
   describe '#pack' do
-    let(:backup_contents) { ['backup_contents'] }
+    let(:expected_backup_contents) { %w(repositories db uploads.tar.gz builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz backup_information.yml) }
+    let(:tar_file) { '1546300800_2019_01_01_12.3_gitlab_backup.tar' }
     let(:tar_system_options) { { out: [tar_file, 'w', Gitlab.config.backup.archive_permissions] } }
-    let(:tar_cmdline) { ['tar', '-cf', '-', *backup_contents, tar_system_options] }
-
+    let(:tar_cmdline) { ['tar', '-cf', '-', *expected_backup_contents, tar_system_options] }
     let(:backup_information) do
       {
         backup_created_at: Time.zone.parse('2019-01-01'),
@@ -36,20 +29,20 @@ RSpec.describe Backup::Manager do
     before do
       allow(ActiveRecord::Base.connection).to receive(:reconnect!)
       allow(Kernel).to receive(:system).and_return(true)
+      allow(YAML).to receive(:load_file).and_return(backup_information)
+
+      ::Backup::Manager::FOLDERS_TO_BACKUP.each do |folder|
+        allow(Dir).to receive(:exist?).with(File.join(Gitlab.config.backup.path, folder)).and_return(true)
+      end
 
-      allow(subject).to receive(:backup_contents).and_return(backup_contents)
       allow(subject).to receive(:backup_information).and_return(backup_information)
       allow(subject).to receive(:upload)
     end
 
-    context 'when BACKUP is not set' do
-      let(:tar_file) { '1546300800_2019_01_01_12.3_gitlab_backup.tar' }
-
-      it 'uses the default tar file name' do
-        subject.pack
+    it 'executes tar' do
+      subject.pack
 
-        expect(Kernel).to have_received(:system).with(*tar_cmdline)
-      end
+      expect(Kernel).to have_received(:system).with(*tar_cmdline)
     end
 
     context 'when BACKUP is set' do
@@ -62,6 +55,37 @@ RSpec.describe Backup::Manager do
         expect(Kernel).to have_received(:system).with(*tar_cmdline)
       end
     end
+
+    context 'when skipped is set in backup_information.yml' do
+      let(:expected_backup_contents) { %w{db uploads.tar.gz builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz backup_information.yml} }
+      let(:backup_information) do
+        {
+          backup_created_at: Time.zone.parse('2019-01-01'),
+          gitlab_version: '12.3',
+          skipped: ['repositories']
+        }
+      end
+
+      it 'executes tar' do
+        subject.pack
+
+        expect(Kernel).to have_received(:system).with(*tar_cmdline)
+      end
+    end
+
+    context 'when a directory does not exist' do
+      let(:expected_backup_contents) { %w{db uploads.tar.gz builds.tar.gz artifacts.tar.gz pages.tar.gz lfs.tar.gz backup_information.yml} }
+
+      before do
+        expect(Dir).to receive(:exist?).with(File.join(Gitlab.config.backup.path, 'repositories')).and_return(false)
+      end
+
+      it 'executes tar' do
+        subject.pack
+
+        expect(Kernel).to have_received(:system).with(*tar_cmdline)
+      end
+    end
   end
 
   describe '#remove_old' do

spec/lib/gitlab/usage/metrics/instrumentations/redis_metric_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Usage::Metrics::Instrumentations::RedisMetric, :clean_gitlab_redis_shared_state do
+  before do
+    4.times do
+      Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes)
+    end
+  end
+
+  let(:expected_value) { 4 }
+
+  it_behaves_like 'a correct instrumented metric value', { options: { event: 'pushes', counter_class: 'SourceCodeCounter' } }
+
+  it 'raises an exception if event option is not present' do
+    expect { described_class.new(counter_class: 'SourceCodeCounter') }.to raise_error(ArgumentError)
+  end
+
+  it 'raises an exception if counter_class option is not present' do
+    expect { described_class.new(event: 'pushes') }.to raise_error(ArgumentError)
+  end
+end

spec/lib/sidebars/projects/menus/packages_registries_menu_spec.rb

@@ -51,8 +51,8 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
       context 'when Container Registry is not visible' do
         let(:registry_enabled) { false }
 
-        it 'menu link points to Infrastructure Registry page' do
-          expect(subject.link).to eq described_class.new(context).renderable_items.find { |i| i.item_id == :infrastructure_registry }.link
+        it 'does not display menu link' do
+          expect(subject.render?).to eq false
         end
       end
     end
@@ -130,10 +130,26 @@ RSpec.describe Sidebars::Projects::Menus::PackagesRegistriesMenu do
 
           is_expected.not_to be_nil
         end
+
+        context 'when config package setting is disabled' do
+          it 'does not add the menu item to the list' do
+            stub_config(packages: { enabled: false })
+
+            is_expected.to be_nil
+          end
+        end
+
+        context 'when user cannot read packages' do
+          let(:user) { nil }
+
+          it 'does not add the menu item to the list' do
+            is_expected.to be_nil
+          end
+        end
       end
 
       context 'when feature flag :infrastructure_registry_page is disabled' do
-        it 'the menu item is not added to list of menu items' do
+        it 'does not add the menu item to the list' do
           stub_feature_flags(infrastructure_registry_page: false)
 
           is_expected.to be_nil