Merge branch 'wrap-update-canary-in-sidekiq' into 'master'

Execute HTTP calls to Kubernetes in Sidekiq Worker Job

See merge request gitlab-org/gitlab!47065

config/sidekiq_queues.yml

@@ -112,6 +112,8 @@
   - 2
 - - emails_on_push
   - 2
+- - environments_canary_ingress_update
+  - 1
 - - epics
   - 2
 - - error_tracking_issue_link

ee/app/graphql/mutations/environments/canary_ingress/update.rb

@@ -23,7 +23,7 @@ module Mutations
 
           result = ::Environments::CanaryIngress::UpdateService
             .new(environment.project, current_user, kwargs)
-            .execute(environment)
+            .execute_async(environment)
 
           { errors: Array.wrap(result[:message]) }
         end

ee/app/services/environments/canary_ingress/update_service.rb

@@ -3,11 +3,20 @@
 module Environments
   module CanaryIngress
     class UpdateService < ::BaseService
-      def execute(environment)
+      def execute_async(environment)
         result = validate(environment)
 
         return result unless result[:status] == :success
 
+        Environments::CanaryIngress::UpdateWorker.perform_async(environment.id, params)
+
+        success
+      end
+
+      # This method actually executes the PATCH request to Kubernetes,
+      # that is used by internal processes i.e. sidekiq worker.
+      # You should always use `execute_async` to properly validate user's requests.
+      def execute(environment)
         canary_ingress = environment.ingresses&.find(&:canary?)
 
         unless canary_ingress.present?

ee/app/workers/all_queues.yml

@@ -661,6 +661,14 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: environments_canary_ingress_update
+  :feature_category: :continuous_delivery
+  :has_external_dependencies: true
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent: true
+  :tags: []
 - :name: group_saml_group_sync
   :feature_category: :authentication_and_authorization
   :has_external_dependencies:

ee/app/workers/environments/canary_ingress/update_worker.rb

+# frozen_string_literal: true
+
+module Environments
+  module CanaryIngress
+    class UpdateWorker
+      include ApplicationWorker
+
+      sidekiq_options retry: false
+      idempotent!
+      worker_has_external_dependencies!
+      feature_category :continuous_delivery
+
+      def perform(environment_id, params)
+        Environment.find_by_id(environment_id).try do |environment|
+          Environments::CanaryIngress::UpdateService
+            .new(environment.project, nil, params.with_indifferent_access)
+            .execute(environment)
+        end
+      end
+    end
+  end
+end

ee/spec/graphql/mutations/environments/canary_ingress/update_spec.rb

@@ -29,7 +29,7 @@ RSpec.describe Mutations::Environments::CanaryIngress::Update do
 
     context 'when service execution succeeded' do
       before do
-        allow(update_service).to receive(:execute) { { status: :success } }
+        allow(update_service).to receive(:execute_async) { { status: :success } }
       end
 
       it 'returns no errors' do
@@ -39,7 +39,7 @@ RSpec.describe Mutations::Environments::CanaryIngress::Update do
 
     context 'when service encounters a problem' do
       before do
-        allow(update_service).to receive(:execute) { { status: :error, message: 'something went wrong' } }
+        allow(update_service).to receive(:execute_async) { { status: :error, message: 'something went wrong' } }
       end
 
       it 'returns an error' do

ee/spec/services/environments/canary_ingress/update_service_spec.rb

@@ -21,63 +21,20 @@ RSpec.describe Environments::CanaryIngress::UpdateService, :clean_gitlab_redis_c
     stub_licensed_features(deploy_board: true)
   end
 
-  describe '#execute' do
-    subject { service.execute(environment) }
+  shared_examples_for 'failed request' do
+    it 'returns an error' do
+      expect(subject[:status]).to eq(:error)
+      expect(subject[:message]).to eq(message)
+    end
+  end
+
+  describe '#execute_async' do
+    subject { service.execute_async(environment) }
 
     let(:environment) { create(:environment, project: project) }
     let(:params) { { weight: 50 } }
     let(:canary_ingress) { ::Gitlab::Kubernetes::Ingress.new(kube_ingress(track: :canary)) }
 
-    shared_examples_for 'failed request' do
-      it 'returns an error' do
-        expect(subject[:status]).to eq(:error)
-        expect(subject[:message]).to eq(message)
-      end
-    end
-
-    context 'when canary ingress is present in the environment' do
-      before do
-        allow(environment).to receive(:ingresses) { [canary_ingress] }
-      end
-
-      context 'when patch request succeeds' do
-        let(:patch_data) do
-          {
-            metadata: {
-              annotations: {
-                Gitlab::Kubernetes::Ingress::ANNOTATION_KEY_CANARY_WEIGHT => params[:weight].to_s
-              }
-            }
-          }
-        end
-
-        before do
-          allow(environment).to receive(:patch_ingress).with(canary_ingress, patch_data) { true }
-        end
-
-        it 'returns success' do
-          expect(subject[:status]).to eq(:success)
-          expect(subject[:message]).to be_nil
-        end
-      end
-
-      context 'when patch request does not succeed' do
-        before do
-          allow(environment).to receive(:patch_ingress) { false }
-        end
-
-        it_behaves_like 'failed request' do
-          let(:message) { 'Failed to update the Canary Ingress.' }
-        end
-      end
-    end
-
-    context 'when canary ingress is not present in the environment' do
-      it_behaves_like 'failed request' do
-        let(:message) { 'Canary Ingress does not exist in the environment.' }
-      end
-    end
-
     context 'when canary_ingress_weight_control feature flag is disabled' do
       before do
         stub_feature_flags(canary_ingress_weight_control: false)
@@ -142,4 +99,55 @@ RSpec.describe Environments::CanaryIngress::UpdateService, :clean_gitlab_redis_c
       end
     end
   end
+
+  describe '#execute' do
+    subject { service.execute(environment) }
+
+    let(:environment) { create(:environment, project: project) }
+    let(:params) { { weight: 50 } }
+    let(:canary_ingress) { ::Gitlab::Kubernetes::Ingress.new(kube_ingress(track: :canary)) }
+
+    context 'when canary ingress is present in the environment' do
+      before do
+        allow(environment).to receive(:ingresses) { [canary_ingress] }
+      end
+
+      context 'when patch request succeeds' do
+        let(:patch_data) do
+          {
+            metadata: {
+              annotations: {
+                Gitlab::Kubernetes::Ingress::ANNOTATION_KEY_CANARY_WEIGHT => params[:weight].to_s
+              }
+            }
+          }
+        end
+
+        before do
+          allow(environment).to receive(:patch_ingress).with(canary_ingress, patch_data) { true }
+        end
+
+        it 'returns success' do
+          expect(subject[:status]).to eq(:success)
+          expect(subject[:message]).to be_nil
+        end
+      end
+
+      context 'when patch request does not succeed' do
+        before do
+          allow(environment).to receive(:patch_ingress) { false }
+        end
+
+        it_behaves_like 'failed request' do
+          let(:message) { 'Failed to update the Canary Ingress.' }
+        end
+      end
+    end
+
+    context 'when canary ingress is not present in the environment' do
+      it_behaves_like 'failed request' do
+        let(:message) { 'Canary Ingress does not exist in the environment.' }
+      end
+    end
+  end
 end

ee/spec/workers/environments/canary_ingress/update_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Environments::CanaryIngress::UpdateWorker do
+  let_it_be(:environment) { create(:environment) }
+  let(:worker) { described_class.new }
+
+  describe '#perform' do
+    subject { worker.perform(environment_id, params) }
+
+    let(:environment_id) { environment.id }
+    let(:params) { { 'weight' => 50 } }
+
+    it 'executes the update service' do
+      expect_next_instance_of(Environments::CanaryIngress::UpdateService, environment.project, nil, params) do |service|
+        expect(service).to receive(:execute).with(environment)
+      end
+
+      subject
+    end
+
+    context 'when an environment does not exist' do
+      let(:environment_id) { non_existing_record_id }
+
+      it 'does not execute the update service' do
+        expect(Environments::CanaryIngress::UpdateService).not_to receive(:new)
+
+        subject
+      end
+    end
+  end
+end