Automatic merge of gitlab-org/gitlab master

ee/app/assets/javascripts/threat_monitoring/components/policy_editor/policy_editor.vue

@@ -40,7 +40,7 @@ export default {
       return this.isEditing ? this.policyType : this.newPolicyType;
     },
     isEditing() {
-      return Boolean(this.existingPolicy);
+      return Boolean(this.existingPolicy?.creation_timestamp || this.existingPolicy?.updatedAt);
     },
     policyTypes() {
       return Object.values(POLICY_TYPE_COMPONENT_OPTIONS);

ee/app/services/network_policies/find_resource_service.rb

@@ -27,12 +27,21 @@ module NetworkPolicies
     def get_policy
       client = @platform.kubeclient
       if @kind == Gitlab::Kubernetes::CiliumNetworkPolicy::KIND
-        resource = client.get_cilium_network_policy(@resource_name, @kubernetes_namespace)
+        resource = find_cilium_policy_or_predefined(client)
         Gitlab::Kubernetes::CiliumNetworkPolicy.from_resource(resource)
       elsif @kind == Gitlab::Kubernetes::NetworkPolicy::KIND
         resource = client.get_network_policy(@resource_name, @kubernetes_namespace)
         Gitlab::Kubernetes::NetworkPolicy.from_resource(resource)
       end
     end
+
+    def find_cilium_policy_or_predefined(client)
+      client.get_cilium_network_policy(@resource_name, @kubernetes_namespace)
+    rescue Kubeclient::ResourceNotFoundError
+      policy_yaml = Gitlab::Kubernetes::CiliumNetworkPolicy::PREDEFINED_POLICIES[@resource_name]
+      raise if policy_yaml.nil?
+
+      Gitlab::Kubernetes::CiliumNetworkPolicy.from_yaml(policy_yaml).resource
+    end
   end
 end

ee/spec/frontend/threat_monitoring/components/policy_editor/policy_editor_spec.js

@@ -93,9 +93,9 @@ describe('PolicyEditor component', () => {
 
   describe('when an existing policy is present', () => {
     it.each`
-      policyType                 | option                                         | existingPolicy                  | findComponent
-      ${'container_policy'}      | ${POLICY_TYPE_COMPONENT_OPTIONS.container}     | ${{ manifest: mockL3Manifest }} | ${findNeworkPolicyEditor}
-      ${'scan_execution_policy'} | ${POLICY_TYPE_COMPONENT_OPTIONS.scanExecution} | ${mockDastScanExecutionObject}  | ${findScanExecutionPolicyEditor}
+      policyType                 | option                                         | existingPolicy                                                     | findComponent
+      ${'container_policy'}      | ${POLICY_TYPE_COMPONENT_OPTIONS.container}     | ${{ manifest: mockL3Manifest, updatedAt: '2020-04-14T00:08:30Z' }} | ${findNeworkPolicyEditor}
+      ${'scan_execution_policy'} | ${POLICY_TYPE_COMPONENT_OPTIONS.scanExecution} | ${mockDastScanExecutionObject}                                     | ${findScanExecutionPolicyEditor}
     `(
       'renders the disabled form select for existing policy of type $policyType',
       async ({ existingPolicy, findComponent, option, policyType }) => {

ee/spec/frontend/threat_monitoring/components/policy_editor/scan_execution_policy/lib/from_yaml_spec.js

@@ -6,6 +6,6 @@ import {
 
 describe('fromYaml', () => {
   it('returns policy object', () => {
-    expect(fromYaml(mockDastScanExecutionManifest)).toStrictEqual(mockDastScanExecutionObject);
+    expect(fromYaml(mockDastScanExecutionManifest)).toMatchObject(mockDastScanExecutionObject);
   });
 });

ee/spec/frontend/threat_monitoring/mocks/mock_data.js

@@ -23,6 +23,7 @@ rules:
   - type: pipeline
     branches:
       - main
+updatedAt: '2020-04-14T00:08:30Z'
 actions:
   - scan: dast
     site_profile: required_site_profile
@@ -35,6 +36,7 @@ export const mockDastScanExecutionObject = {
   description: 'This policy enforces pipeline configuration to have a job with DAST scan',
   enabled: false,
   rules: [{ type: 'pipeline', branches: ['main'] }],
+  updatedAt: '2020-04-14T00:08:30Z',
   actions: [
     {
       scan: 'dast',

ee/spec/services/network_policies/find_resource_service_spec.rb

@@ -3,7 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe NetworkPolicies::FindResourceService do
-  let(:service) { described_class.new(resource_name: 'policy', environment: environment, kind: kind) }
+  let(:resource_name) { 'policy' }
+  let(:service) { described_class.new(resource_name: resource_name, environment: environment, kind: kind) }
   let(:environment) { instance_double('Environment', deployment_platform: platform, deployment_namespace: 'namespace') }
   let(:platform) { instance_double('Clusters::Platforms::Kubernetes', kubeclient: kubeclient) }
   let(:kubeclient) { double('Kubeclient::Client') }
@@ -49,6 +50,42 @@ RSpec.describe NetworkPolicies::FindResourceService do
         expect(subject).to be_success
         expect(subject.payload.as_json).to eq(policy.as_json)
       end
+
+      context 'when it was not found in the cluster' do
+        before do
+          allow(kubeclient).to receive(:get_cilium_network_policy).with(resource_name, environment.deployment_namespace).and_raise(Kubeclient::ResourceNotFoundError.new(404, 'policy not found', {}))
+        end
+
+        let(:policy) do
+          {
+            creation_timestamp: nil,
+            environment_ids: [],
+            is_autodevops: false,
+            is_enabled: false,
+            name: "drop-outbound",
+            namespace: nil
+          }
+        end
+
+        context 'and has name reserved for predefined policy' do
+          let(:resource_name) { 'drop-outbound' }
+
+          it 'returns success response with predefined policy' do
+            expect(subject).to be_success
+            expect(subject.payload.as_json).to include(policy)
+          end
+        end
+
+        context 'and has name different from any predefined policy' do
+          let(:resource_name) { 'not-predefined-policy' }
+
+          it 'returns success response with predefined policy' do
+            expect(subject).to be_error
+            expect(subject.http_status).to eq(:bad_request)
+            expect(subject.message).to eq('Kubernetes error: policy not found')
+          end
+        end
+      end
     end
 
     context 'with invalid policy kind' do
@@ -57,7 +94,7 @@ RSpec.describe NetworkPolicies::FindResourceService do
       it 'returns error response' do
         expect(subject).to be_error
         expect(subject.http_status).to eq(:bad_request)
-        expect(subject.message).not_to be_nil
+        expect(subject.message).to eq('Invalid or unsupported policy kind')
       end
     end
 
@@ -67,7 +104,7 @@ RSpec.describe NetworkPolicies::FindResourceService do
       it 'returns error response' do
         expect(subject).to be_error
         expect(subject.http_status).to eq(:bad_request)
-        expect(subject.message).not_to be_nil
+        expect(subject.message).to eq('Environment does not have deployment platform')
       end
     end
 

lib/gitlab/kubernetes/cilium_network_policy.rb

@@ -9,6 +9,36 @@ module Gitlab
       API_VERSION = "cilium.io/v2"
       KIND = 'CiliumNetworkPolicy'
 
+      PREDEFINED_POLICIES = {
+        'allow-inbound-http' => <<~YAML.rstrip,
+          apiVersion: cilium.io/v2
+          kind: CiliumNetworkPolicy
+          metadata:
+            name: allow-inbound-http
+          spec:
+            endpointSelector:
+              matchLabels:
+                network-policy.gitlab.com/disabled_by: gitlab
+            ingress:
+            - toPorts:
+              - ports:
+                - port: '80'
+                - port: '443'
+        YAML
+        'drop-outbound' => <<~YAML.rstrip
+          apiVersion: cilium.io/v2
+          kind: CiliumNetworkPolicy
+          metadata:
+            name: drop-outbound
+          spec:
+            endpointSelector:
+              matchLabels:
+                network-policy.gitlab.com/disabled_by: gitlab
+            egress:
+            - {}
+        YAML
+      }.freeze
+
       # We are modeling existing kubernetes resource and don't have
       # control over amount of parameters.
       # rubocop:disable Metrics/ParameterLists

spec/services/ci/job_artifacts/destroy_all_expired_service_spec.rb

@@ -10,20 +10,16 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
   describe '.execute' do
     subject { service.execute }
 
-    let_it_be(:artifact, refind: true) do
-      create(:ci_job_artifact, expire_at: 1.day.ago)
-    end
-
-    before(:all) do
-      artifact.job.pipeline.unlocked!
-    end
+    let_it_be(:locked_pipeline) { create(:ci_pipeline, :artifacts_locked) }
+    let_it_be(:pipeline) { create(:ci_pipeline, :unlocked) }
+    let_it_be(:locked_job) { create(:ci_build, :success, pipeline: locked_pipeline) }
+    let_it_be(:job) { create(:ci_build, :success, pipeline: pipeline) }
 
     context 'when artifact is expired' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       context 'with preloaded relationships' do
         before do
-          job = create(:ci_build, pipeline: artifact.job.pipeline)
-          create(:ci_job_artifact, :archive, :expired, job: job)
-
           stub_const("#{described_class}::LOOP_LIMIT", 1)
         end
 
@@ -39,7 +35,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
           # COMMIT
           # SELECT next expired ci_job_artifacts
 
-          expect(log.count).to be_within(1).of(11)
+          expect(log.count).to be_within(1).of(10)
         end
       end
 
@@ -48,7 +44,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
           expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
         end
 
-        context 'when the artifact does not a file attached to it' do
+        context 'when the artifact does not have a file attached to it' do
           it 'does not create deleted objects' do
             expect(artifact.exists?).to be_falsy # sanity check
 
@@ -57,10 +53,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
         end
 
         context 'when the artifact has a file attached to it' do
-          before do
-            artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip')
-            artifact.save!
-          end
+          let!(:artifact) { create(:ci_job_artifact, :expired, :zip, job: job) }
 
           it 'creates a deleted object' do
             expect { subject }.to change { Ci::DeletedObject.count }.by(1)
@@ -81,9 +74,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
       end
 
       context 'when artifact is locked' do
-        before do
-          artifact.job.pipeline.artifacts_locked!
-        end
+        let!(:artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
         it 'does not destroy job artifact' do
           expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -92,9 +83,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when artifact is not expired' do
-      before do
-        artifact.update_column(:expire_at, 1.day.since)
-      end
+      let!(:artifact) { create(:ci_job_artifact, job: job) }
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -102,9 +91,7 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when artifact is permanent' do
-      before do
-        artifact.update_column(:expire_at, nil)
-      end
+      let!(:artifact) { create(:ci_job_artifact, expire_at: nil, job: job) }
 
       it 'does not destroy expired job artifacts' do
         expect { subject }.not_to change { Ci::JobArtifact.count }
@@ -112,6 +99,8 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when failed to destroy artifact' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       before do
         stub_const("#{described_class}::LOOP_LIMIT", 10)
       end
@@ -146,58 +135,67 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
     end
 
     context 'when exclusive lease has already been taken by the other instance' do
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+
       before do
         stub_exclusive_lease_taken(described_class::EXCLUSIVE_LOCK_KEY, timeout: described_class::LOCK_TIMEOUT)
       end
 
       it 'raises an error and does not start destroying' do
         expect { subject }.to raise_error(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError)
+                          .and not_change { Ci::JobArtifact.count }.from(1)
       end
     end
 
-    context 'when timeout happens' do
-      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+    context 'with a second artifact and batch size of 1' do
+      let(:second_job) { create(:ci_build, :success, pipeline: pipeline) }
+      let!(:second_artifact) { create(:ci_job_artifact, :archive, expire_at: 1.day.ago, job: second_job) }
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
 
       before do
-        stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds)
         stub_const("#{described_class}::BATCH_SIZE", 1)
-
-        second_artifact.job.pipeline.unlocked!
       end
 
-      it 'destroys one artifact' do
-        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
-      end
-
-      it 'reports the number of destroyed artifacts' do
-        is_expected.to eq(1)
-      end
-    end
+      context 'when timeout happens' do
+        before do
+          stub_const("#{described_class}::LOOP_TIMEOUT", 0.seconds)
+        end
 
-    context 'when loop reached loop limit' do
-      before do
-        stub_const("#{described_class}::LOOP_LIMIT", 1)
-        stub_const("#{described_class}::BATCH_SIZE", 1)
+        it 'destroys one artifact' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        end
 
-        second_artifact.job.pipeline.unlocked!
+        it 'reports the number of destroyed artifacts' do
+          is_expected.to eq(1)
+        end
       end
 
-      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
+      context 'when loop reached loop limit' do
+        before do
+          stub_const("#{described_class}::LOOP_LIMIT", 1)
+        end
 
-      it 'destroys one artifact' do
-        expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        it 'destroys one artifact' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        end
+
+        it 'reports the number of destroyed artifacts' do
+          is_expected.to eq(1)
+        end
       end
 
-      it 'reports the number of destroyed artifacts' do
-        is_expected.to eq(1)
+      context 'when the number of artifacts is greater than than batch size' do
+        it 'destroys all expired artifacts' do
+          expect { subject }.to change { Ci::JobArtifact.count }.by(-2)
+        end
+
+        it 'reports the number of destroyed artifacts' do
+          is_expected.to eq(2)
+        end
       end
     end
 
     context 'when there are no artifacts' do
-      before do
-        artifact.destroy!
-      end
-
       it 'does not raise error' do
         expect { subject }.not_to raise_error
       end
@@ -207,42 +205,18 @@ RSpec.describe Ci::JobArtifacts::DestroyAllExpiredService, :clean_gitlab_redis_s
       end
     end
 
-    context 'when there are artifacts more than batch sizes' do
-      before do
-        stub_const("#{described_class}::BATCH_SIZE", 1)
-
-        second_artifact.job.pipeline.unlocked!
-      end
-
-      let!(:second_artifact) { create(:ci_job_artifact, expire_at: 1.day.ago) }
-
-      it 'destroys all expired artifacts' do
-        expect { subject }.to change { Ci::JobArtifact.count }.by(-2)
-      end
-
-      it 'reports the number of destroyed artifacts' do
-        is_expected.to eq(2)
-      end
-    end
-
     context 'when some artifacts are locked' do
-      before do
-        pipeline = create(:ci_pipeline, locked: :artifacts_locked)
-        job = create(:ci_build, pipeline: pipeline)
-        create(:ci_job_artifact, expire_at: 1.day.ago, job: job)
-      end
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: job) }
+      let!(:locked_artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
       it 'destroys only unlocked artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(-1)
+        expect(locked_artifact).to be_persisted
       end
     end
 
     context 'when all artifacts are locked' do
-      before do
-        pipeline = create(:ci_pipeline, locked: :artifacts_locked)
-        job = create(:ci_build, pipeline: pipeline)
-        artifact.update!(job: job)
-      end
+      let!(:artifact) { create(:ci_job_artifact, :expired, job: locked_job) }
 
       it 'destroys no artifacts' do
         expect { subject }.to change { Ci::JobArtifact.count }.by(0)