Merge pull request #614 from KennaSecurity/SUP-1886-update-veracode-code

SUP-1886-update veracode retry on 504

Author: effany

lib/http.rb

@@ -11,25 +11,46 @@ module Http
           Faraday::ConnectionFailed, Faraday::ClientError, Net::OpenTimeout, Errno::ECONNREFUSED, EOFError, Faraday::ServerError
         ]
 
-        def connection(verify_ssl = true, max_retries = 5, hmac_client: nil)
+        def connection(verify_ssl = true, max_retries = 5, hmac_client: nil, retry_options: nil)
+          default_config = {
+            max: max_retries,
+            interval: 0.1,
+            max_interval: 30,
+            backoff_factor: 5,
+            methods: %i[get post],
+            exceptions: RETRY_EXCEPTIONS,
+            retry_statuses: [429, 500, 502, 503, 504],
+            retry_block: method(:log_retry),
+            exhausted_retries_block: method(:log_retries_exhausted)
+          }
+
+          priority_config = nil
+          if retry_options
+            # `retry_options` configures a dedicated retry policy for a specific set of
+            # statuses (e.g. 504). It is applied as a separate, outer retry middleware so
+            # those statuses get their own max/interval, while every other error keeps the
+            # default behaviour below. The prioritised statuses are removed from the default
+            # middleware so they are never retried twice.
+            priority_statuses = Array(retry_options[:retry_statuses])
+            priority_config = default_config.merge(retry_options).merge(
+              methods: [], # rely solely on retry_if so only the prioritised statuses retry
+              retry_if: ->(env, exception) { priority_statuses.include?(retry_status(env, exception)) }
+            )
+
+            default_config[:retry_statuses] -= priority_statuses
+            default_config[:methods] = []
+            default_config[:retry_if] = ->(env, exception) { !priority_statuses.include?(retry_status(env, exception)) }
+          end
+
           Faraday.new do |faraday|
             faraday.request :multipart
             faraday.request :json
             faraday.ssl.verify = verify_ssl
             if @options && @options[:debug] == true
               faraday.response :logger # This logs to STDOUT by default
             end
-            faraday.request :retry, {
-              max: max_retries,
-              interval: 0.1,
-              max_interval: 30,
-              backoff_factor: 5,
-              methods: %i[get post],
-              exceptions: RETRY_EXCEPTIONS,
-              retry_statuses: [429, 500, 502, 503, 504],
-              retry_block: method(:log_retry),
-              exhausted_retries_block: method(:log_retries_exhausted)
-            }
+            faraday.request :retry, priority_config if priority_config
+            faraday.request :retry, default_config
             if hmac_client
               require_relative './faraday_middlewares/faraday_hmac_middleware'
               faraday.use FaradayHmac, hmac_client
@@ -40,20 +61,31 @@ def connection(verify_ssl = true, max_retries = 5, hmac_client: nil)
           end
         end
 
-        def http_get(url, headers, max_retries = 5, verify_ssl = true, hmac_client: nil)
-          connection(verify_ssl, max_retries, hmac_client:).run_request(:get, url, nil, headers)
+        # Resolves the HTTP status for a retry decision. `raise_error` runs before the retry
+        # middleware, so error responses arrive here as exceptions; we read the status from
+        # the exception when present and fall back to the response env otherwise.
+        def retry_status(env, exception)
+          if exception.respond_to?(:response_status) && exception.response_status
+            exception.response_status
+          else
+            env&.status
+          end
+        end
+
+        def http_get(url, headers, max_retries = 5, verify_ssl = true, hmac_client: nil, retry_options: nil)
+          connection(verify_ssl, max_retries, hmac_client:, retry_options:).run_request(:get, url, nil, headers)
         end
 
-        def http_post(url, headers, payload, max_retries = 5, verify_ssl = true, hmac_client: nil)
-          connection(verify_ssl, max_retries, hmac_client:).run_request(:post, url, payload, headers)
+        def http_post(url, headers, payload, max_retries = 5, verify_ssl = true, hmac_client: nil, retry_options: nil)
+          connection(verify_ssl, max_retries, hmac_client:, retry_options:).run_request(:post, url, payload, headers)
         end
 
-        def http_put(url, headers, payload, max_retries = 5, verify_ssl = true)
-          connection(verify_ssl, max_retries).run_request(:put, url, payload, headers)
+        def http_put(url, headers, payload, max_retries = 5, verify_ssl = true, retry_options: nil)
+          connection(verify_ssl, max_retries, retry_options:).run_request(:put, url, payload, headers)
         end
 
-        def http_delete(url, headers, max_retries = 5, verify_ssl = true)
-          connection(verify_ssl, max_retries).run_request(:delete, url, nil, headers)
+        def http_delete(url, headers, max_retries = 5, verify_ssl = true, retry_options: nil)
+          connection(verify_ssl, max_retries, retry_options:).run_request(:delete, url, nil, headers)
         end
 
         def log_retry(retry_count:, exception:, will_retry_in:, **_kwargs)

spec/lib/http_spec.rb

@@ -0,0 +1,127 @@
+# frozen_string_literal: true
+
+require "rspec_helper"
+require_relative "../../lib/http"
+
+class TestHelper
+  include Kenna::Toolkit::Helpers::Http
+
+  attr_reader :options
+
+  def initialize(options = {})
+    @options = options
+  end
+end
+
+RSpec.describe Kenna::Toolkit::Helpers::Http do
+  subject(:helper) { TestHelper.new }
+
+  describe "#connection" do
+    context "without retry_options" do
+      it "uses default retry configuration" do
+        conn = helper.connection
+        # Verify connection is created without errors
+        expect(conn).to be_a(Faraday::Connection)
+      end
+    end
+
+    context "with custom retry_options" do
+      it "merges retry_statuses instead of replacing them" do
+        custom_options = {
+          retry_statuses: [504]
+        }
+        conn = helper.connection(true, 5, retry_options: custom_options)
+        expect(conn).to be_a(Faraday::Connection)
+      end
+
+      it "preserves default exceptions while adding custom ones" do
+        custom_options = {
+          exceptions: [Faraday::TooManyRequestsError]
+        }
+        conn = helper.connection(true, 5, retry_options: custom_options)
+        expect(conn).to be_a(Faraday::Connection)
+      end
+
+      it "allows overriding other retry options" do
+        custom_options = {
+          interval: 60,
+          max_interval: 60,
+          backoff_factor: 2
+        }
+        conn = helper.connection(true, 5, retry_options: custom_options)
+        expect(conn).to be_a(Faraday::Connection)
+      end
+    end
+
+    context "HMAC client support" do
+      it "creates connection with HMAC middleware when hmac_client is provided" do
+        mock_client = double("hmac_client")
+        conn = helper.connection(true, 5, hmac_client: mock_client)
+        expect(conn).to be_a(Faraday::Connection)
+      end
+    end
+
+    context "SSL verification" do
+      it "disables SSL verification when verify_ssl is false" do
+        conn = helper.connection(false)
+        expect(conn).to be_a(Faraday::Connection)
+      end
+    end
+  end
+
+  describe "#http_get" do
+    it "makes GET requests" do
+      stub_request(:get, "https://example.com/test")
+        .to_return(body: "success", status: 200)
+
+      response = helper.http_get("https://example.com/test", {})
+      expect(response.status).to eq(200)
+      expect(response.body).to eq("success")
+    end
+
+    it "accepts retry_options parameter" do
+      stub_request(:get, "https://example.com/test")
+        .to_return(body: "success", status: 200)
+
+      retry_opts = { retry_statuses: [504] }
+      response = helper.http_get("https://example.com/test", {}, 5, true, retry_options: retry_opts)
+      expect(response.status).to eq(200)
+    end
+  end
+
+  describe "#http_post" do
+    it "makes POST requests with retry options" do
+      stub_request(:post, "https://example.com/test")
+        .to_return(body: "created", status: 201)
+
+      retry_opts = { retry_statuses: [504] }
+      response = helper.http_post("https://example.com/test", {}, { foo: "bar" }, 5, true, retry_options: retry_opts)
+      expect(response.status).to eq(201)
+      expect(response.body).to eq("created")
+    end
+  end
+
+  describe "#http_put" do
+    it "makes PUT requests with retry options" do
+      stub_request(:put, "https://example.com/test")
+        .to_return(body: "updated", status: 200)
+
+      retry_opts = { retry_statuses: [504] }
+      response = helper.http_put("https://example.com/test", {}, { foo: "bar" }, 5, true, retry_options: retry_opts)
+      expect(response.status).to eq(200)
+      expect(response.body).to eq("updated")
+    end
+  end
+
+  describe "#http_delete" do
+    it "makes DELETE requests with retry options" do
+      stub_request(:delete, "https://example.com/test")
+        .to_return(body: "deleted", status: 200)
+
+      retry_opts = { retry_statuses: [504] }
+      response = helper.http_delete("https://example.com/test", {}, 5, true, retry_options: retry_opts)
+      expect(response.status).to eq(200)
+      expect(response.body).to eq("deleted")
+    end
+  end
+end