Skip to content

Fixes #39359 - Add shared Faraday transport for Foreman requests#944

Open
pablomh wants to merge 2 commits into
theforeman:developfrom
pablomh:faraday-transport-refactor
Open

Fixes #39359 - Add shared Faraday transport for Foreman requests#944
pablomh wants to merge 2 commits into
theforeman:developfrom
pablomh:faraday-transport-refactor

Conversation

@pablomh
Copy link
Copy Markdown

@pablomh pablomh commented May 26, 2026

Summary

  • replace the per-request outbound Foreman client path with a shared Faraday transport using faraday-net_http_persistent
  • reuse persistent upstream connections on the smart-proxy to Foreman or Satellite leg while keeping the existing request-building API stable
  • add layered connect and read timeout support plus a cleaner seam for future TLS and PQC transport changes

Test plan

  • Run ruby -c lib/proxy/foreman_transport.rb
  • Run ruby -c lib/proxy/request.rb
  • Run ruby -c test/request_test.rb
  • Run RuboCop on changed files with Ruby 3.2
  • Run focused request tests in a full project Ruby and Bundler environment
  • Run broader smart-proxy test coverage in CI

@pablomh pablomh force-pushed the faraday-transport-refactor branch from 8f6a8ef to 927efcc Compare May 26, 2026 21:31
Copy link
Copy Markdown
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of using all of this custom code over plain Faraday? IMHO this makes it complicated to review. Is it an idea to first rewrite it to Faraday and only then add connection pooling?

I see a lot of custom classes and I'd prefer to pass options to Faraday like in https://lostisland.github.io/faraday/#/customization/connection-options.

Then I also know that we have various HTTP "implementations". Perhaps it's also good to do some research on how to unify those. We have those because we didn't include an external dependency, but if you're adding that then it should be considered.

Comment thread lib/proxy/foreman_transport.rb Outdated
options[:client_key] = OpenSSL::PKey.read(File.read(ssl_paths[:private_key]), nil)
end

apply_future_tls_profile!(options)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO this shouldn't be included. We've been steering at using system wide crypto-policies and avoid using application config within Foreman. Smart Proxy isn't quite there today, but application code shouldn't be aware of it if possible.

Replace the outbound Net::HTTP client path with Faraday while preserving the existing request-building API, CA and client certificate wiring, and configurable read timeout behavior. Add a new foreman_open_timeout setting that defaults to the existing Net::HTTP open timeout when unset.

Co-authored-by: Cursor <cursoragent@cursor.com>
@pablomh
Copy link
Copy Markdown
Author

pablomh commented May 27, 2026

Quick cleanup/update for review:

  • I reshaped the branch into two commits so the Faraday introduction and the shared connection pooling are easier to review independently.
  • The first commit now covers the Faraday migration plus the explicit foreman_open_timeout handling.
  • The second commit contains the shared transport/pooling change.

The goal here was to keep the diff easier to reason about and make it simpler to split the pooling part later if that is requested.

@pablomh pablomh force-pushed the faraday-transport-refactor branch from 2ec9012 to 3340965 Compare May 27, 2026 15:47
Cache Faraday connections per upstream transport profile so RHSM forwarding can reuse persistent smart-proxy to Foreman connections across request instances and subclasses. Invalidate and rebuild cached connections after transport-level failures, and switch the shared client to the net_http_persistent adapter.

Co-authored-by: Cursor <cursoragent@cursor.com>
@pablomh pablomh force-pushed the faraday-transport-refactor branch from 3340965 to 13b7729 Compare May 27, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants