Merge branch...

Merge branch '335014-retry-proxied-jiraconnection-installation-deletion-if-the-remote-server-is-not-reachable' into 'master'

Resolve "Retry proxied JiraConnection installation deletion if the remote server is not reachable"

See merge request gitlab-org/gitlab!67987

app/workers/all_queues.yml

@@ -1118,6 +1118,15 @@
   :weight: 1
   :idempotent:
   :tags: []
+- :name: jira_connect:jira_connect_retry_request
+  :worker_name: JiraConnect::RetryRequestWorker
+  :feature_category: :integrations
+  :has_external_dependencies: true
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 1
+  :idempotent:
+  :tags: []
 - :name: jira_connect:jira_connect_sync_branch
   :worker_name: JiraConnect::SyncBranchWorker
   :feature_category: :integrations

app/workers/jira_connect/forward_event_worker.rb

@@ -13,16 +13,17 @@ module JiraConnect
 
     def perform(installation_id, base_path, event_path)
       installation = JiraConnectInstallation.find_by_id(installation_id)
+      instance_url = installation&.instance_url
 
-      return if installation&.instance_url.nil?
+      installation.destroy if installation
+
+      return if instance_url.nil?
 
-      proxy_url = installation.instance_url + event_path
-      qsh = Atlassian::Jwt.create_query_string_hash(proxy_url, 'POST', installation.instance_url + base_path)
+      proxy_url = instance_url + event_path
+      qsh = Atlassian::Jwt.create_query_string_hash(proxy_url, 'POST', instance_url + base_path)
       jwt = Atlassian::Jwt.encode({ iss: installation.client_key, qsh: qsh }, installation.shared_secret)
 
-      Gitlab::HTTP.post(proxy_url, headers: { 'Authorization' => "JWT #{jwt}" })
-    ensure
-      installation.destroy if installation
+      JiraConnect::RetryRequestWorker.perform_async(proxy_url, jwt)
     end
   end
 end

app/workers/jira_connect/retry_request_worker.rb

+# frozen_string_literal: true
+
+module JiraConnect
+  class RetryRequestWorker # rubocop:disable Scalability/IdempotentWorker
+    include ApplicationWorker
+
+    data_consistency :always
+    queue_namespace :jira_connect
+    feature_category :integrations
+    urgency :low
+
+    worker_has_external_dependencies!
+
+    def perform(proxy_url, jwt, attempts = 3)
+      r = Gitlab::HTTP.post(proxy_url, headers: { 'Authorization' => "JWT #{jwt}" })
+
+      self.class.perform_in(1.hour, proxy_url, jwt, attempts - 1) if r.code >= 400 && attempts > 0
+    rescue *Gitlab::HTTP::HTTP_ERRORS
+      self.class.perform_in(1.hour, proxy_url, jwt, attempts - 1) if attempts > 0
+    end
+  end
+end

spec/workers/jira_connect/forward_event_worker_spec.rb

@@ -15,23 +15,23 @@ RSpec.describe JiraConnect::ForwardEventWorker do
     let(:client_key) { '123' }
     let(:shared_secret) { '123' }
 
-    subject { described_class.new.perform(jira_connect_installation.id, base_path, event_path) }
+    subject(:perform) { described_class.new.perform(jira_connect_installation.id, base_path, event_path) }
 
-    it 'forwards the event including the auth header and deletes the installation' do
+    it 'forwards the event and deletes the installation' do
       stub_request(:post, event_url)
 
       expect(Atlassian::Jwt).to receive(:create_query_string_hash).with(event_url, 'POST', base_url).and_return('some_qsh')
       expect(Atlassian::Jwt).to receive(:encode).with({ iss: client_key, qsh: 'some_qsh' }, shared_secret).and_return('auth_token')
-      expect { subject }.to change(JiraConnectInstallation, :count).by(-1)
+      expect(JiraConnect::RetryRequestWorker).to receive(:perform_async).with(event_url, 'auth_token')
 
-      expect(WebMock).to have_requested(:post, event_url).with(headers: { 'Authorization' => 'JWT auth_token' })
+      expect { perform }.to change(JiraConnectInstallation, :count).by(-1)
     end
 
     context 'when installation does not exist' do
       let(:jira_connect_installation) { instance_double(JiraConnectInstallation, id: -1) }
 
       it 'does nothing' do
-        expect { subject }.not_to change(JiraConnectInstallation, :count)
+        expect { perform }.not_to change(JiraConnectInstallation, :count)
       end
     end
 
@@ -39,17 +39,9 @@ RSpec.describe JiraConnect::ForwardEventWorker do
       let!(:jira_connect_installation) { create(:jira_connect_installation) }
 
       it 'forwards the event including the auth header' do
-        expect { subject }.to change(JiraConnectInstallation, :count).by(-1)
+        expect { perform }.to change(JiraConnectInstallation, :count).by(-1)
 
-        expect(WebMock).not_to have_requested(:post, '*')
-      end
-    end
-
-    context 'when it fails to forward the event' do
-      it 'still deletes the installation' do
-        allow(Gitlab::HTTP).to receive(:post).and_raise(StandardError)
-
-        expect { subject }.to raise_error(StandardError).and change(JiraConnectInstallation, :count).by(-1)
+        expect(JiraConnect::RetryRequestWorker).not_to receive(:perform_async)
       end
     end
   end

spec/workers/jira_connect/retry_request_worker_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe JiraConnect::RetryRequestWorker do
+  describe '#perform' do
+    let(:jwt) { 'some-jwt' }
+    let(:event_url) { 'https://example.com/somewhere' }
+    let(:attempts) { 3 }
+
+    subject(:perform) { described_class.new.perform(event_url, jwt, attempts) }
+
+    it 'sends the request, with the appropriate headers' do
+      expect(JiraConnect::RetryRequestWorker).not_to receive(:perform_in)
+
+      stub_request(:post, event_url)
+
+      perform
+
+      expect(WebMock).to have_requested(:post, event_url).with(headers: { 'Authorization' => 'JWT some-jwt' })
+    end
+
+    context 'when the proxied request fails' do
+      before do
+        stub_request(:post, event_url).to_return(status: 500, body: '', headers: {})
+      end
+
+      it 'arranges to retry the request' do
+        expect(JiraConnect::RetryRequestWorker).to receive(:perform_in).with(1.hour, event_url, jwt, attempts - 1)
+
+        perform
+      end
+
+      context 'when there are no more attempts left' do
+        let(:attempts) { 0 }
+
+        it 'does not retry' do
+          expect(JiraConnect::RetryRequestWorker).not_to receive(:perform_in)
+
+          perform
+        end
+      end
+    end
+  end
+end