diff --git a/.gitlab/ci/qa.gitlab-ci.yml b/.gitlab/ci/qa.gitlab-ci.yml index 676e895035b..1ebc408e0d4 100644 --- a/.gitlab/ci/qa.gitlab-ci.yml +++ b/.gitlab/ci/qa.gitlab-ci.yml @@ -115,13 +115,13 @@ update-qa-cache: .package-and-qa-ff-base: script: - | - feature_flags=$(scripts/changed-feature-flags --files $(cat $CHANGES_FILE | tr ' ' ',') --state $QA_FF_STATE) + feature_flags=$(scripts/changed-feature-flags --files $CHANGES_DIFFS_DIR --state $QA_FF_STATE) if [[ $feature_flags ]]; then export GITLAB_QA_OPTIONS="--set-feature-flags $feature_flags" echo $GITLAB_QA_OPTIONS ./scripts/trigger-build.rb omnibus else - echo "No changed feature flag found to test. The tests are skipped if the flag was removed." + echo "No changed feature flag found to test as $QA_FF_STATE." fi package-and-qa: @@ -135,7 +135,7 @@ package-and-qa-ff-enabled: - .package-and-qa-ff-base - .qa:rules:package-and-qa:feature-flags variables: - QA_FF_STATE: "enable" + QA_FF_STATE: "enabled" package-and-qa-ff-disabled: extends: @@ -143,4 +143,12 @@ package-and-qa-ff-disabled: - .package-and-qa-ff-base - .qa:rules:package-and-qa:feature-flags variables: - QA_FF_STATE: "disable" + QA_FF_STATE: "disabled" + +package-and-qa-ff-deleted: + extends: + - .package-and-qa-base + - .package-and-qa-ff-base + - .qa:rules:package-and-qa:feature-flags + variables: + QA_FF_STATE: "deleted" diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index d7deaa90b13..7fd7a02873d 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -900,9 +900,6 @@ rules: - <<: *if-not-ee when: never - - <<: *if-dot-com-gitlab-org-and-security-merge-request - changes: *feature-flag-development-config-patterns - when: never - <<: *if-merge-request-targeting-stable-branch allow_failure: true - <<: *if-dot-com-gitlab-org-and-security-merge-request diff --git a/doc/user/application_security/sast/analyzers.md b/doc/user/application_security/sast/analyzers.md index 2eba892e75e..888b91641f8 100644 --- a/doc/user/application_security/sast/analyzers.md +++ b/doc/user/application_security/sast/analyzers.md @@ -105,18 +105,21 @@ Prerequisites: - The custom Docker registry must provide images for all the official analyzers. +NOTE: +This variable affects all Secure analyzers, not just the analyzers for SAST. + To have GitLab download the analyzers' images from a custom Docker registry, define the prefix with the `SECURE_ANALYZERS_PREFIX` CI/CD variable. -For example, the following instructs SAST to pull `my-docker-registry/gl-images/sast/bandit` instead -of `registry.gitlab.com/security-products/sast/bandit`: +For example, the following instructs SAST to pull `my-docker-registry/gitlab-images/bandit` instead +of `registry.gitlab.com/security-products/bandit`: ```yaml include: - template: Security/SAST.gitlab-ci.yml variables: - SECURE_ANALYZERS_PREFIX: my-docker-registry/gl-images + SECURE_ANALYZERS_PREFIX: my-docker-registry/gitlab-images ``` ### Disable all default analyzers diff --git a/doc/user/application_security/sast/index.md b/doc/user/application_security/sast/index.md index 4a3d0ed006d..38f26b7578d 100644 --- a/doc/user/application_security/sast/index.md +++ b/doc/user/application_security/sast/index.md @@ -982,19 +982,19 @@ import the following default SAST analyzer images from `registry.gitlab.com` int [local Docker container registry](../../packages/container_registry/index.md): ```plaintext -registry.gitlab.com/security-products/sast/bandit:2 -registry.gitlab.com/security-products/sast/brakeman:2 -registry.gitlab.com/security-products/sast/eslint:2 -registry.gitlab.com/security-products/sast/flawfinder:2 -registry.gitlab.com/security-products/sast/gosec:3 -registry.gitlab.com/security-products/sast/kubesec:2 -registry.gitlab.com/security-products/sast/nodejs-scan:2 -registry.gitlab.com/security-products/sast/phpcs-security-audit:2 -registry.gitlab.com/security-products/sast/pmd-apex:2 -registry.gitlab.com/security-products/sast/security-code-scan:2 -registry.gitlab.com/security-products/sast/semgrep:2 -registry.gitlab.com/security-products/sast/sobelow:2 -registry.gitlab.com/security-products/sast/spotbugs:2 +registry.gitlab.com/security-products/bandit:2 +registry.gitlab.com/security-products/brakeman:2 +registry.gitlab.com/security-products/eslint:2 +registry.gitlab.com/security-products/flawfinder:2 +registry.gitlab.com/security-products/gosec:3 +registry.gitlab.com/security-products/kubesec:2 +registry.gitlab.com/security-products/nodejs-scan:2 +registry.gitlab.com/security-products/phpcs-security-audit:2 +registry.gitlab.com/security-products/pmd-apex:2 +registry.gitlab.com/security-products/security-code-scan:2 +registry.gitlab.com/security-products/semgrep:2 +registry.gitlab.com/security-products/sobelow:2 +registry.gitlab.com/security-products/spotbugs:2 ``` The process for importing Docker images into a local offline Docker registry depends on diff --git a/qa/qa/runtime/feature.rb b/qa/qa/runtime/feature.rb index ec28813c1f6..93e215e9635 100644 --- a/qa/qa/runtime/feature.rb +++ b/qa/qa/runtime/feature.rb @@ -42,6 +42,8 @@ module QA enable(flag, **scopes) when 'disabled', 'disable', 'false', 0, false disable(flag, **scopes) + when 'deleted' + QA::Runtime::Logger.info("Feature flag definition for '#{flag}' was deleted. The state of the feature flag has not been changed.") else raise UnknownStateError, "Unknown feature flag state: #{state}" end diff --git a/scripts/changed-feature-flags b/scripts/changed-feature-flags index 3a4f18bd78f..86214e86788 100755 --- a/scripts/changed-feature-flags +++ b/scripts/changed-feature-flags @@ -3,10 +3,12 @@ require 'yaml' require 'optparse' +require 'pathname' require_relative 'api/default_options' # This script returns the desired feature flag state as a comma-separated string for the feature flags in the specified files. -# Each desired feature flag state is specified as 'feature-flag=state'. +# Each desired feature flag state is specified as 'feature-flag=state'. This allows us to run package-and-qa with the +# feature flag set to the desired state. # # For example, if the specified files included `config/feature_flags/development/ci_yaml_limit_size.yml` and the desired # state as specified by the second argument was enabled, the value returned would be `ci_yaml_limit_size=enabled` @@ -15,31 +17,84 @@ class GetFeatureFlagsFromFiles def initialize(options) @files = options.delete(:files) @state = options.delete(:state) + + abort("ERROR: Please specify the directory containing MR diffs.") if @files.to_s.empty? end + # Gets feature flags from definition files or diffs of deleted defition files + # + # @return [String] a comma-separated list of feature flags and their desired state def extracted_flags - files.each_with_object([]) do |file_path, all| - next unless file_path =~ %r{/feature_flags/(development|ops)/.*\.yml} - next unless File.exist?(file_path) + flags_list = diffs_dir.glob('**/*').each_with_object([]) do |file_path, flags| + ff_yaml = ff_yaml_for_file(file_path) + next if ff_yaml.nil? + break [] if ff_yaml.empty? - ff_yaml = YAML.safe_load(File.read(file_path)) - ff_to_add = "#{ff_yaml['name']}" - ff_to_add += "=#{state}" unless state.to_s.empty? + flags << ff_yaml['name'] + end + flags_list = flags_list.map { |flag| "#{flag}=#{state}" } unless state.to_s.empty? + flags_list.join(',') + end - all << ff_to_add - end.join(',') + # Loads the YAML feature flag definition based on a diff of the definition file. The definition is loaded from the + # definition file itself, or from a diff of the deleted definition file. + # + # @param [Pathname] path the path to the diff + # @return [Hash] a hash containing the YAML data for the feature flag definition + def ff_yaml_for_file(path) + return unless File.expand_path(path).to_s =~ %r{/feature_flags/(development|ops)/.*\.yml} + + if path.to_s.end_with?('yml.deleted.diff') + # Ignore deleted feature flag definitions if we want to enable/disable existing flags. + return if state != 'deleted' + + yaml_from_deleted_diff(path) + else + # If we want deleted definition files but find one that wasn't deleted, we return immediately to + # because non-deleted flags are tested in separate jobs from deleted flags, so we don't need to run + # a job with just deleted flags. + return [] if state == 'deleted' + + yaml_from_file(path, diffs_dir) + end end private attr_reader :files, :state + + # The absolute path to the directory of diffs + # + # @return [String] + def diffs_dir + @diffs_dir ||= Pathname.new(files).expand_path + end + + # Loads the YAML feature flag definition from a file corresponding to a diff of the definition file. + # + # @param [Pathname] file_path the path to the diff + # @param [Pathname] diffs_dir the path to the diffs directory + # @return [Hash] a hash containing the YAML data from the feature flag definition file corresponding to the diff + def yaml_from_file(file_path, diffs_dir) + real_file_path = File.join(Dir.pwd, file_path.to_s.delete_prefix(diffs_dir.to_s)).delete_suffix('.diff') + YAML.safe_load(File.read(real_file_path)) + end + + # Loads the YAML feature flag definition from a diff of the deleted feature flag definition file. + # + # @param [Pathname] file_path the path of the diff + # @return [Hash] a hash containing the YAML data for the feature flag definition from the diff + def yaml_from_deleted_diff(file_path) + cleaned_diff = File.read(file_path).gsub(/^[^a-z]+/, '') + YAML.safe_load(cleaned_diff) + end end if $0 == __FILE__ options = API::DEFAULT_OPTIONS.dup OptionParser.new do |opts| - opts.on("-f", "--files FILES", Array, "Comma-separated list of feature flag config files") do |value| + opts.on("-f", "--files FILES", String, "A directory containing diffs including feature flag definition change diffs") do |value| options[:files] = value end diff --git a/spec/scripts/changed-feature-flags_spec.rb b/spec/scripts/changed-feature-flags_spec.rb index 5c858588c0c..bbae49a90e4 100644 --- a/spec/scripts/changed-feature-flags_spec.rb +++ b/spec/scripts/changed-feature-flags_spec.rb @@ -6,8 +6,8 @@ load File.expand_path('../../scripts/changed-feature-flags', __dir__) RSpec.describe 'scripts/changed-feature-flags' do describe GetFeatureFlagsFromFiles do - let(:feature_flag_definition1) do - file = Tempfile.new('foo.yml', ff_dir) + let!(:feature_flag_definition1) do + file = File.open(File.join(ff_dir, "#{file_name1}.yml"), 'w+') file.write(<<~YAML) --- name: foo_flag @@ -17,8 +17,8 @@ RSpec.describe 'scripts/changed-feature-flags' do file end - let(:feature_flag_definition2) do - file = Tempfile.new('bar.yml', ff_dir) + let!(:feature_flag_definition2) do + file = File.open(File.join(ff_dir, "#{file_name2}.yml"), 'w+') file.write(<<~YAML) --- name: bar_flag @@ -28,48 +28,136 @@ RSpec.describe 'scripts/changed-feature-flags' do file end + let!(:feature_flag_diff1) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "#{file_name1}.yml.diff"), 'w+') + file.write(<<~YAML) + @@ -5,4 +5,4 @@ + name: foo_flag + -default_enabled: false + +default_enabled: true + YAML + file.rewind + file + end + + let!(:feature_flag_diff2) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "#{file_name2}.yml.diff"), 'w+') + file.write(<<~YAML) + @@ -0,0 +0,0 @@ + name: bar_flag + -default_enabled: true + +default_enabled: false + YAML + file.rewind + file + end + + let!(:deleted_feature_flag_diff) do + FileUtils.mkdir_p(File.join(diffs_dir, ff_sub_dir)) + file = File.open(File.join(diffs_dir, ff_sub_dir, "foobar_ff_#{SecureRandom.hex(8)}.yml.deleted.diff"), 'w+') + file.write(<<~YAML) + @@ -0,0 +0,0 @@ + -name: foobar_flag + -default_enabled: true + YAML + file.rewind + file + end + + before do + allow(Dir).to receive(:pwd).and_return(Dir.tmpdir) + end + after do - FileUtils.remove_entry(ff_dir, true) + feature_flag_definition1.close + feature_flag_definition2.close + feature_flag_diff1.close + feature_flag_diff2.close + deleted_feature_flag_diff.close + FileUtils.rm_r(ff_dir) + FileUtils.rm_r(diffs_dir) end describe '.extracted_flags' do + let(:file_name1) { "foo_ff_#{SecureRandom.hex(8)}"} + let(:file_name2) { "bar_ff_#{SecureRandom.hex(8)}"} + let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, ff_sub_dir)) } + let(:diffs_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'diffs')).first } + shared_examples 'extract feature flags' do it 'returns feature flags on their own' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path] }) + subject = described_class.new({ files: diffs_dir }) - expect(subject.extracted_flags).to eq('foo_flag,bar_flag') + expect(subject.extracted_flags.split(',')).to include('foo_flag', 'bar_flag') end it 'returns feature flags and their state as enabled' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path], state: 'enabled' }) + subject = described_class.new({ files: diffs_dir, state: 'enabled' }) - expect(subject.extracted_flags).to eq('foo_flag=enabled,bar_flag=enabled') + expect(subject.extracted_flags.split(',')).to include('foo_flag=enabled', 'bar_flag=enabled') end it 'returns feature flags and their state as disabled' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path], state: 'disabled' }) + subject = described_class.new({ files: diffs_dir, state: 'disabled' }) - expect(subject.extracted_flags).to eq('foo_flag=disabled,bar_flag=disabled') + expect(subject.extracted_flags.split(',')).to include('foo_flag=disabled', 'bar_flag=disabled') + end + + it 'does not return feature flags when there are mixed deleted and non-deleted definition files' do + subject = described_class.new({ files: diffs_dir, state: 'deleted' }) + + expect(subject.extracted_flags).to eq('') end end context 'with definition files in the development directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'development')) } + let(:ff_sub_dir) { %w[feature_flags development] } it_behaves_like 'extract feature flags' end context 'with definition files in the ops directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'ops')) } + let(:ff_sub_dir) { %w[feature_flags ops] } it_behaves_like 'extract feature flags' end context 'with definition files in the experiment directory' do - let(:ff_dir) { FileUtils.mkdir_p(File.join(Dir.tmpdir, 'feature_flags', 'experiment')) } + let(:ff_sub_dir) { %w[feature_flags experiment] } it 'ignores the files' do - subject = described_class.new({ files: [feature_flag_definition1.path, feature_flag_definition2.path] }) + subject = described_class.new({ files: diffs_dir }) + + expect(subject.extracted_flags).to eq('') + end + end + + context 'with only deleted definition files' do + let(:ff_sub_dir) { %w[feature_flags development] } + + before do + feature_flag_diff1.close + feature_flag_diff2.close + FileUtils.rm_r(feature_flag_diff1) + FileUtils.rm_r(feature_flag_diff2) + end + + it 'returns feature flags and their state as deleted' do + subject = described_class.new({ files: diffs_dir, state: 'deleted' }) + + expect(subject.extracted_flags).to eq('foobar_flag=deleted') + end + + it 'does not return feature flags when the desired state is enabled' do + subject = described_class.new({ files: diffs_dir, state: 'enabled' }) + + expect(subject.extracted_flags).to eq('') + end + + it 'does not return feature flags when the desired state is disabled' do + subject = described_class.new({ files: diffs_dir, state: 'disabled' }) expect(subject.extracted_flags).to eq('') end diff --git a/tooling/bin/find_change_diffs b/tooling/bin/find_change_diffs index 7857945ea74..b28b20df0f4 100755 --- a/tooling/bin/find_change_diffs +++ b/tooling/bin/find_change_diffs @@ -33,6 +33,8 @@ end Gitlab.merge_request_changes(mr_project_path, mr_iid).changes.each do |change| next if change['diff'].empty? - output_diffs_dir.join(File.dirname(change['new_path'])).mkpath - output_diffs_dir.join("#{change['new_path']}.diff").write(change['diff']) + ext = change['deleted_file'] ? ".deleted.diff" : ".diff" + new_path = output_diffs_dir.join("#{change['new_path']}#{ext}") + new_path.dirname.mkpath + new_path.write(change['diff']) end