Improve feature flag usage

This change improves feature flag checks for the `default_branch_image`
feature. We now check that the feature flag is enabled in the report
parser so that we can use `@report.pipeline.project` in order to enable
or disable the feature on a per-project basis. The feature flag status
is passed to the location parser as an attribute.

config/feature_flags/development/improved_container_scan_matching.yml

@@ -2,7 +2,7 @@
 name: improved_container_scan_matching
 introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/73486
 rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/344534
-milestone: '14.5'
+milestone: '14.6'
 type: development
 group: group::container security
 default_enabled: false

ee/lib/gitlab/ci/parsers/security/container_scanning.rb

@@ -13,15 +13,22 @@ module Gitlab
               operating_system: location_data['operating_system'],
               package_name: location_data.dig('dependency', 'package', 'name'),
               package_version: location_data.dig('dependency', 'version'),
-              default_branch_image: default_branch_image(location_data)
+              default_branch_image: default_branch_image(location_data),
+              improved_container_scan_matching_enabled: improved_container_scan_matching_enabled?
             )
           end
 
           def default_branch_image(location_data)
+            return unless improved_container_scan_matching_enabled?
+
             return if @report.pipeline.default_branch?
 
             location_data['default_branch_image']
           end
+
+          def improved_container_scan_matching_enabled?
+            Feature.enabled?(:improved_container_scan_matching, @report.pipeline.project, default_enabled: :yaml)
+          end
         end
       end
     end

ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb

@@ -6,34 +6,49 @@ module Gitlab
       module Security
         module Locations
           class ContainerScanning < Base
+            attr_reader :default_branch_image
             attr_reader :image
             attr_reader :operating_system
             attr_reader :package_name
             attr_reader :package_version
 
-            def initialize(image:, operating_system:, package_name: nil, package_version: nil, default_branch_image: nil)
+            def initialize(
+              image:,
+              operating_system:,
+              package_name: nil,
+              package_version: nil,
+              default_branch_image: nil,
+              improved_container_scan_matching_enabled: false
+            )
               @image = image
               @operating_system = operating_system
               @package_name = package_name
               @package_version = package_version
               @default_branch_image = default_branch_image
+              @improved_container_scan_matching_enabled = improved_container_scan_matching_enabled
             end
 
             def fingerprint_data
               "#{docker_image_name_without_tag}:#{package_name}"
             end
 
-            def default_branch_image
-              return @default_branch_image if Feature.enabled?(:improved_container_scan_matching)
+            def improved_container_scan_matching_enabled?
+              @improved_container_scan_matching_enabled
             end
 
             private
 
             def docker_image_name_without_tag
+              if improved_container_scan_matching_enabled?
                 image_name = default_branch_image.presence || image
                 base_name, _, version = image_name.rpartition(':')
 
+                return image_name if version_semver_like?(version)
+              else
+                base_name, version = image.split(':')
+
                 return image if version_semver_like?(version)
+              end
 
               base_name
             end

ee/spec/lib/gitlab/ci/parsers/security/container_scanning_spec.rb

@@ -7,18 +7,12 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
   let(:current_branch) { project.default_branch }
   let(:pipeline) { create(:ci_pipeline, ref: current_branch, project: project) }
   let(:job) { create(:ci_build, pipeline: pipeline)}
-  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
-
-  before do
-    artifact.each_blob { |blob| described_class.parse!(blob, report) }
-    stub_feature_flags(improved_container_scan_matching: false)
-  end
-
-  describe '#parse!' do
   let(:artifact) { create(:ee_ci_job_artifact, :container_scanning, job: job) }
+  let(:report) { Gitlab::Ci::Reports::Security::Report.new(artifact.file_type, pipeline, 2.weeks.ago) }
   let(:image) { 'registry.gitlab.com/gitlab-org/security-products/dast/webgoat-8.0@sha256:bc09fe2e0721dfaeee79364115aeedf2174cce0947b9ae5fe7c33312ee019a4e' }
   let(:default_branch_image) { 'registry.gitlab.com/gitlab-org/security-products/dast/webgoat-8.0:latest' }
 
+  shared_examples 'report' do
     it "parses all identifiers and findings for unapproved vulnerabilities" do
       expect(report.findings.length).to eq(8)
       expect(report.identifiers.length).to eq(8)
@@ -34,8 +28,7 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
         image: image,
         operating_system: 'debian:9',
         package_name: 'glibc',
-        package_version: '2.24-11+deb9u3',
-        default_branch_image: nil
+        package_version: '2.24-11+deb9u3'
       )
     end
 
@@ -46,12 +39,40 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
     it "adds report image's name to raw_metadata" do
       expect(Gitlab::Json.parse(report.findings.first.raw_metadata).dig('location', 'image')).to eq(image)
     end
+  end
 
-    context 'with improved_container_scan_matching' do
+  describe '#parse!' do
+    context 'when improved_container_scan_matching is disabled' do
+      before do
+        stub_feature_flags(improved_container_scan_matching: false)
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
+      end
+
+      it_behaves_like 'report'
+
+      context 'when not on default branch' do
+        let(:current_branch) { 'not-default' }
+
+        it 'does not include default_branch_image' do
+          location = report.findings.first.location
+
+          expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::ContainerScanning)
+          expect(location).to have_attributes(
+            default_branch_image: nil,
+            improved_container_scan_matching_enabled?: false
+          )
+        end
+      end
+    end
+
+    context 'when improved_container_scan_matching is enabled' do
       before do
         stub_feature_flags(improved_container_scan_matching: true)
+        artifact.each_blob { |blob| described_class.parse!(blob, report) }
       end
 
+      it_behaves_like 'report'
+
       context 'when on default branch' do
         let(:current_branch) { project.default_branch }
 
@@ -60,11 +81,8 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
 
           expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::ContainerScanning)
           expect(location).to have_attributes(
-            image: image,
-            operating_system: 'debian:9',
-            package_name: 'glibc',
-            package_version: '2.24-11+deb9u3',
-            default_branch_image: nil
+            default_branch_image: nil,
+            improved_container_scan_matching_enabled?: true
           )
         end
       end
@@ -77,11 +95,8 @@ RSpec.describe Gitlab::Ci::Parsers::Security::ContainerScanning do
 
           expect(location).to be_a(::Gitlab::Ci::Reports::Security::Locations::ContainerScanning)
           expect(location).to have_attributes(
-            image: image,
-            operating_system: 'debian:9',
-            package_name: 'glibc',
-            package_version: '2.24-11+deb9u3',
-            default_branch_image: default_branch_image
+            default_branch_image: default_branch_image,
+            improved_container_scan_matching_enabled?: true
           )
         end
       end

ee/spec/lib/gitlab/ci/reports/security/locations/container_scanning_spec.rb

@@ -24,10 +24,6 @@ RSpec.describe Gitlab::Ci::Reports::Security::Locations::ContainerScanning do
     sha1_of = -> (input) { Digest::SHA1.hexdigest(input) }
 
     context 'with feature enabled' do
-      before do
-        stub_feature_flags(improved_container_scan_matching: true)
-      end
-
       where(:image, :default_branch_image, :expected_fingerprint_input) do
         [
           ['alpine:3.7.3', nil, 'alpine:3.7.3:glibc'],
@@ -58,6 +54,11 @@ RSpec.describe Gitlab::Ci::Reports::Security::Locations::ContainerScanning do
             'registry.gitlab.com/group/project@sha256:a418bbb80b9411f9a08025baa4681e192aaafd16505039bdcb113ccdb90a88fd',
             'registry.gitlab.com/group/project:latest',
             'registry.gitlab.com/group/project:glibc'
+          ],
+          [
+            'registry.gitlab.com/group/project/feature:latest',
+            'registry.gitlab.com/group/project:1.0.0',
+            'registry.gitlab.com/group/project:1.0.0:glibc'
           ]
         ]
       end
@@ -69,7 +70,8 @@ RSpec.describe Gitlab::Ci::Reports::Security::Locations::ContainerScanning do
             default_branch_image: default_branch_image,
             operating_system: 'debian:9',
             package_name: 'glibc',
-            package_version: '1.2.3'
+            package_version: '1.2.3',
+            improved_container_scan_matching_enabled: true
           }
         end
 
@@ -78,10 +80,6 @@ RSpec.describe Gitlab::Ci::Reports::Security::Locations::ContainerScanning do
     end
 
     context 'with feature disabled' do
-      before do
-        stub_feature_flags(improved_container_scan_matching: false)
-      end
-
       let(:params) do
         {
           image: 'registry.gitlab.com/group/project/feature:ec301f43f14a2b477806875e49cfc4d3fa0d22c3',
@@ -95,6 +93,32 @@ RSpec.describe Gitlab::Ci::Reports::Security::Locations::ContainerScanning do
       it 'ignores default_branch_image' do
         expect(subject.fingerprint).to eq(sha1_of.call('registry.gitlab.com/group/project/feature:glibc'))
       end
+
+      where(:image, :expected_fingerprint_input) do
+        [
+          ['alpine:3.7.3', 'alpine:3.7.3:glibc'],
+          ['alpine:3.7', 'alpine:3.7:glibc'],
+          ['alpine:8101518288111119448185914762536722131810', 'alpine:glibc'],
+          ['alpine:1.0.0-beta', 'alpine:1.0.0-beta:glibc'],
+          [
+            'registry.gitlab.com/group/project/tmp:af864bd61230d3d694eb01d6205b268b4ad63ac0',
+            'registry.gitlab.com/group/project/tmp:glibc'
+          ]
+        ]
+      end
+
+      with_them do
+        let(:params) do
+          {
+            image: image,
+            operating_system: 'debian:9',
+            package_name: 'glibc',
+            package_version: '1.2.3'
+          }
+        end
+
+        specify { expect(subject.fingerprint).to eq(sha1_of.call(expected_fingerprint_input)) }
+      end
     end
   end
 end