Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
05e81dd
Migrate Candlepin HTTP layer from RestClient to Faraday 2.x
pablomh May 11, 2026
7356ed9
Fix RestClient exception handling, UpstreamPool params, and controlle…
pablomh May 12, 2026
8dc6ebf
Collapse Proxy into a single proxy_request helper
pablomh May 12, 2026
2d5ad0f
Rename RestClientException to HttpError
pablomh May 12, 2026
6fd5610
Enrich HttpError with response_body, use default_headers in Proxy
pablomh May 12, 2026
9a432d4
Add params: keyword to issue_request, unify query_string helper
pablomh May 12, 2026
10771ab
Update tests and fix error handling for Faraday migration
pablomh May 12, 2026
4f4b889
Unify HTTP stacks: route Proxy and UpstreamPool through issue_request
pablomh May 12, 2026
3dedbd4
Fix error handler status code, unify upstream_owner_id, add comments
pablomh May 12, 2026
d73b700
DRY Proxy methods into issue_proxy helper
pablomh May 12, 2026
fe02026
Replace positional args with keyword args on HTTP verb methods
pablomh May 12, 2026
09d98e8
Update http_resource_test for keyword args, add params: test
pablomh May 12, 2026
09fcd5d
Guard code.to_i against empty string, document Owner#import multipart
pablomh May 12, 2026
59c7dd7
Route remaining raw connection calls through issue_request
pablomh May 12, 2026
3b4408a
Route all HTTP traffic through issue_request via connection: keyword
pablomh May 12, 2026
5080aaf
Log full URL for custom connection requests in issue_request
pablomh May 12, 2026
311d5b3
Fix ping dead rescue, derive OAuth URL from connection
pablomh May 12, 2026
e20dd3b
Add tests for connection:, process: false, and ping status mapping
pablomh May 12, 2026
3924a65
Lazy debug logging and memoize upstream_owner_id
pablomh May 12, 2026
cf799c9
Add reset_connection! for upstream identity cert changes
pablomh May 12, 2026
0a7c1c4
Reset upstream connection on manifest import, refresh, and delete
pablomh May 12, 2026
b3b1325
Key upstream memoization by Organization.current for multi-org safety
pablomh May 12, 2026
3eb1f95
Clear all upstream caches in reset_connection!, not per-org
pablomh May 12, 2026
929729f
Fix response body regressions and URL construction from Faraday migra…
pablomh May 16, 2026
c8ab788
Extract shared helpers: build_path, parse_json, update_content_overri…
pablomh May 16, 2026
988d2cf
Fix RuboCop violations
pablomh May 16, 2026
bd3f605
Fix RuboCop Style/TrailingCommaInHashLiteral violation
pablomh May 16, 2026
2c6b705
Address CodeRabbit review findings
pablomh May 16, 2026
77c8da7
Fix RuboCop violations in Faraday migration follow-up
pablomh May 19, 2026
07171f9
Tighten Faraday error handling follow-up
pablomh May 19, 2026
0aa4c2b
Add regression coverage for Faraday follow-up fixes
pablomh May 19, 2026
49b7e86
Fix test failures from Faraday migration
pablomh May 19, 2026
7493d4a
Revert cleanup_python_publications_test to RestClient::NotFound
pablomh May 19, 2026
8a86f4c
Fix static compliance test coverage.
pablomh May 20, 2026
f603894
Fix test failures from Faraday migration (round 2)
pablomh May 21, 2026
7de46e8
Align Faraday gem constraint with smart-proxy
pablomh May 26, 2026
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,25 @@ def repackage_message
end
end

rescue_from HttpResource::HttpError do |e|
if e.code.to_i == 404
parsed = JSON.parse(e.response_body.to_s) rescue {}
if parsed["errors"]&.any? { |error| error["code"] == "BLOB_UNKNOWN" }
Rails.logger.info(pp_exception(e, with_backtrace: false))
else
Rails.logger.error(pp_exception(e, with_backtrace: false))
end
else
Rails.logger.error(pp_exception(e, with_backtrace: false))
end
body = e.response_body.presence || e.message
if request_from_katello_cli?

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.

I'm surprised by this mechanism. I know it was already there, but Rails normally uses respond_to. I think this would be the Rails native mechanism, assuming the CLI sets Accept: application/json as a header:

respond_to do |format|
  format.json { render json: { errors: [body] }, status: e.code }
  format.any { render plain: body, status: e.code }
end

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I checked the current Hammer stack and it does reliably send Accept: application/json;version=... via apipie-bindings, so respond_to should work for the maintained CLI path. You're also right that request_from_katello_cli? appears to be legacy behavior tied to the old Python-based Katello CLI rather than modern Hammer. My only hesitation is calling it fully dead code without scoping that claim — the registry endpoints are more likely hit by container tooling (podman, skopeo) and scripts than by Hammer itself. If we're comfortable aligning this with maintained clients rather than preserving the old User-Agent behavior, I think respond_to is reasonable — but I'd do it as a follow-up that updates all the sibling controllers consistently rather than just this one handler.

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.

Agreed it belongs in a separate PR.

render json: { errors: [body] }, status: e.code
else
render plain: body, status: e.code
end
end

def authenticate_cert_request
if cert_present?
client_cert = ::Cert::RhsmClient.new(cert_from_request)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,20 +44,20 @@ def find_host(uuid = nil)
uuid ||= params[:id]
facet = Katello::Host::SubscriptionFacet.where(:uuid => uuid).first
if facet.nil?
# check with candlepin if consumer is Gone, raises RestClient::Gone
# check with candlepin if consumer is Gone, raises HttpError
User.as_anonymous_admin { Resources::Candlepin::Consumer.get(uuid) }
fail HttpErrors::NotFound, _("Couldn't find consumer '%s'") % uuid
end
@host = facet.host
end

rescue_from RestClient::Exception do |e|
rescue_from HttpResource::HttpError do |e|
Rails.logger.error(pp_exception(e, with_backtrace: false))
Rails.logger.error(e.backtrace.detect { |line| line.match("katello.*controller") })
body = e.response_body.presence || e.message
if request_from_katello_cli?
render :json => { :errors => [e.http_body] }, :status => e.http_code
render :json => { :errors => [body] }, :status => e.code
else
render :plain => e.http_body, :status => e.http_code
render :plain => body, :status => e.code
end
end

Expand Down
29 changes: 19 additions & 10 deletions app/controllers/katello/api/rhsm/candlepin_proxies_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,23 @@ def error_response_json(display_message, errors)

rescue_from RestClient::Exception do |e|
Rails.logger.error(pp_exception(e, with_backtrace: false))
Rails.logger.error(e.backtrace.detect { |line| line.match("katello.*controller") })
if request_from_katello_cli?
render :json => { :errors => [e.http_body] }, :status => e.http_code
else
render :plain => e.http_body, :status => e.http_code
end
end

rescue_from HttpResource::HttpError do |e|
Rails.logger.error(pp_exception(e, with_backtrace: false))
body = e.response_body.presence || e.message
if request_from_katello_cli?
render :json => { :errors => [body] }, :status => e.code
else
render :plain => body, :status => e.code
end
end

def proxy_request_path
@request_path = drop_api_namespace(@_request.fullpath)
end
Expand All @@ -85,26 +94,26 @@ def get
end

r = Resources::Candlepin::Proxy.get(@request_path, extra_headers)
logger.debug filter_sensitive_data(r)
render :json => r, :status => r.code
logger.debug filter_sensitive_data(r.body)
render :json => r.body, :status => r.status
end

def delete
r = Resources::Candlepin::Proxy.delete(@request_path, @request_body.read)
logger.debug filter_sensitive_data(r)
render :json => r
logger.debug filter_sensitive_data(r.body)
render :json => r.body, :status => r.status
end

def post
r = Resources::Candlepin::Proxy.post(@request_path, @request_body.read)
logger.debug filter_sensitive_data(r)
render :json => r
logger.debug filter_sensitive_data(r.body)
render :json => r.body, :status => r.status
end

def put
r = Resources::Candlepin::Proxy.put(@request_path, @request_body.read)
logger.debug filter_sensitive_data(r)
render :json => r
logger.debug filter_sensitive_data(r.body)
render :json => r.body, :status => r.status
end

#api :GET, "/consumers/:id", N_("Show a system")
Expand Down Expand Up @@ -321,7 +330,7 @@ def find_host(uuid = nil)
uuid ||= params[:id]
facet = Katello::Host::SubscriptionFacet.where(:uuid => uuid).first
if facet.nil?
# check with candlepin if consumer is Gone, raises RestClient::Gone
# check with candlepin if consumer is Gone, raises HttpError
User.as_anonymous_admin { Resources::Candlepin::Consumer.get(uuid) }
end
@host = ::Host::Managed.unscoped.find(facet.host_id)
Expand Down
35 changes: 20 additions & 15 deletions app/lib/actions/candlepin/consumer/clean_backend_objects.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@ def run
# Set bulk load size for Candlepin operations
original_candlepin_page_size = SETTINGS[:katello][:candlepin][:bulk_load_size]
SETTINGS[:katello][:candlepin][:bulk_load_size] = 125

# Gather data from Candlepin and Katello
candlepin_uuids = fetch_candlepin_uuids
katello_candlepin_uuids = fetch_katello_candlepin_uuids

# Find hosts with issues
cleanup_hosts_with_nil_facets
cleanup_hosts_with_no_subscriptions(candlepin_uuids)

# Clean up orphaned Candlepin consumers
cleanup_candlepin_orphans(candlepin_uuids, katello_candlepin_uuids)
SETTINGS[:katello][:candlepin][:bulk_load_size] = original_candlepin_page_size
begin
# Gather data from Candlepin and Katello
candlepin_uuids = fetch_candlepin_uuids
katello_candlepin_uuids = fetch_katello_candlepin_uuids

# Find hosts with issues
cleanup_hosts_with_nil_facets
cleanup_hosts_with_no_subscriptions(candlepin_uuids)

# Clean up orphaned Candlepin consumers
cleanup_candlepin_orphans(candlepin_uuids, katello_candlepin_uuids)
ensure
SETTINGS[:katello][:candlepin][:bulk_load_size] = original_candlepin_page_size
end
end
end

Expand Down Expand Up @@ -66,8 +68,9 @@ def cleanup_hosts_with_nil_facets

unregister_options = host_unregister_options(host)
::Katello::RegistrationManager.unregister_host(host, unregister_options)
rescue RestClient::ResourceNotFound
rescue HttpResource::HttpError => e
# Ignore if already gone
raise unless e.code == '404'
rescue => e
output[:results][:errors] << {
type: 'unregister_host',
Expand All @@ -94,8 +97,9 @@ def cleanup_hosts_with_no_subscriptions(candlepin_uuids)

unregister_options = host_unregister_options(host)
::Katello::RegistrationManager.unregister_host(host, unregister_options)
rescue RestClient::ResourceNotFound
rescue HttpResource::HttpError => e
# Ignore if already gone
raise unless e.code == '404'
rescue => e
output[:results][:errors] << {
type: 'unregister_host',
Expand All @@ -117,8 +121,9 @@ def cleanup_candlepin_orphans(candlepin_uuids, katello_candlepin_uuids)
}

::Katello::Resources::Candlepin::Consumer.destroy(consumer_uuid)
rescue RestClient::ResourceNotFound
rescue HttpResource::HttpError => e
# Ignore if already gone
raise unless e.code == '404'
rescue => e
output[:results][:errors] << {
type: 'destroy_consumer',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ class AddContentToEnvironment < Candlepin::Abstract

def run
output[:add_response] = ::Katello::Resources::Candlepin::Environment.add_content(input[:view_env_cp_id], [input[:content_id]])
rescue RestClient::Conflict
rescue HttpResource::HttpError => e
raise unless e.code == '409'
Rails.logger.info("attempted to add content ID #{input[:content_id]} to environment, but content ID already exists.")
end
end
Expand Down
37 changes: 20 additions & 17 deletions app/lib/actions/candlepin/environment/set_content.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,33 +30,36 @@ def finalize # rubocop:disable Metrics/AbcSize, Metrics/MethodLength
begin
output[:add_response] = ::Katello::Resources::Candlepin::Environment.add_content(input[:cp_environment_id], output[:add_ids])
break
rescue RestClient::Conflict => e
raise e if ((retries += 1) == max_retries)
# Candlepin raises a 409 in case it gets a duplicate content id add to an environment
# Since its a dup id refresh the existing ids list (which hopefully will not have the duplicate content)
# and try again.
output[:add_ids] = content_ids - existing_ids
rescue RestClient::ResourceNotFound, RestClient::NotFound => e
# Set a higher limit for retries just in case the missing content is not being parsed from the error body correctly.
# If the content is not found after the retries, assume it is gone and continue.
raise e if ((retries += 1) == 1_000)
# Parse the missing content from the Candlepin response and remove it from the add_ids list.
missing_content = JSON.parse(e.response.body)['displayMessage'].split(' ')[-1].gsub(/[".]/, '')
Rails.logger.debug "Content #{missing_content} not found in the environment. Removing it from the add_ids list."
output[:add_ids].delete(missing_content)
rescue HttpResource::HttpError => e
case e.code
when '409'
# Candlepin raises a 409 in case it gets a duplicate content id add to an environment.
# Refresh the existing ids list and try again.
raise e if ((retries += 1) == max_retries)
output[:add_ids] = content_ids - existing_ids
when '404'
# Set a higher limit for retries just in case the missing content is not being parsed correctly.
# If the content is not found after the retries, assume it is gone and continue.
raise e if ((retries += 1) == 1_000)
missing_content = e.message.split(' ')[-1].gsub(/[".]/, '')
Rails.logger.debug "Content #{missing_content} not found in the environment. Removing it from the add_ids list."
output[:add_ids].delete(missing_content)
else
raise
end
end
end
retries = 0
until output[:delete_ids].empty?
begin
output[:delete_response] = ::Katello::Resources::Candlepin::Environment.delete_content(input[:cp_environment_id], output[:delete_ids])
break
rescue RestClient::ResourceNotFound
# If the content is not found after the retries, assume it is gone and continue.
break if ((retries += 1) == max_retries)
rescue HttpResource::HttpError => e
raise unless e.code == '404'
# Candlepin raises a 404 in case a content id is not found in this environment
# If thats the case lets just refresh the existing ids list (which hopefully will not have the 404'd content)
# and try again.
break if ((retries += 1) == max_retries)
output[:delete_ids] = existing_ids - content_ids
end
end
Expand Down
1 change: 1 addition & 0 deletions app/lib/actions/katello/organization/manifest_delete.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ def humanized_name

def finalize
subject_organization.audit_manifest_action(_('Manifest deleted'))
::Katello::Resources::Candlepin::UpstreamCandlepinResource.reset_connection!
end
end
end
Expand Down
1 change: 1 addition & 0 deletions app/lib/actions/katello/organization/manifest_import.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ def humanized_input
def finalize
subject_organization.clear_manifest_expired_notifications
subject_organization.audit_manifest_action(_('Manifest imported'))
::Katello::Resources::Candlepin::UpstreamCandlepinResource.reset_connection!
end
end
end
Expand Down
1 change: 1 addition & 0 deletions app/lib/actions/katello/organization/manifest_refresh.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def finalize
org = ::Organization.find(input[:organization_id])
org.clear_manifest_expired_notifications
subject_organization.audit_manifest_action(_('Manifest refreshed'))
::Katello::Resources::Candlepin::UpstreamCandlepinResource.reset_connection!
end
end
end
Expand Down
16 changes: 9 additions & 7 deletions app/lib/actions/middleware/propagate_candlepin_errors.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@ def finalize(*args)

def propagate_candlepin_errors
yield
rescue RestClient::ExceptionWithResponse => e
error_class = if e.response.request.url.include?('/candlepin')
::Katello::Errors::CandlepinError
else
::Katello::Errors::UpstreamCandlepinError
end
raise(error_class.from_exception(e) || e)
rescue ::Katello::Errors::CandlepinError
raise
rescue HttpResource::HttpError => e
display_message = e.message
if e.response_body.present?
parsed = JSON.parse(e.response_body) rescue {}
display_message = parsed['displayMessage'] || display_message
end
raise ::Katello::Errors::CandlepinError, display_message
Comment on lines +22 to +28

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve upstream-vs-local error typing here.

This now rewrites every HttpResource::HttpError into Katello::Errors::CandlepinError, so upstream failures lose their specialized class and any downstream rescue logic for UpstreamCandlepinError will stop matching. Please keep the service-specific mapping when re-raising instead of collapsing everything into the local Candlepin type.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/lib/actions/middleware/propagate_candlepin_errors.rb` around lines 20 -
26, The rescue currently converts every HttpResource::HttpError into
Katello::Errors::CandlepinError, losing upstream-specific types; change the
re-raise to preserve upstream error classes: after extracting display_message
(using JSON.parse of e.response_body as done), check if
e.is_a?(Katello::Errors::UpstreamCandlepinError) (or whatever upstream subclass
lives under Katello::Errors) and if so re-raise an instance of e.class with the
display_message (e.g. raise e.class, display_message, e.backtrace) so downstream
rescues still match, otherwise raise ::Katello::Errors::CandlepinError with
display_message as today.

end
end
end
Expand Down
24 changes: 18 additions & 6 deletions app/lib/katello/api/v2/error_handling.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module ErrorHandling
rescue_from HttpErrors::WrappedError, :with => :rescue_from_wrapped_error

rescue_from RestClient::ExceptionWithResponse, :with => :rescue_from_exception_with_response
rescue_from HttpResource::HttpError, :with => :rescue_from_exception_with_response
rescue_from ActiveRecord::RecordInvalid, :with => :rescue_from_record_invalid
rescue_from ActiveRecord::RecordNotFound, :with => :rescue_from_record_not_found

Expand Down Expand Up @@ -57,11 +58,12 @@ def rescue_from_wrapped_error(exception)

def rescue_from_exception_with_response(exception)
logger.error "exception when talking to a remote client: #{exception.message} " << pp_exception(exception)
code = exception.respond_to?(:code) ? exception.code.to_i : 0
status = (code > 0) ? code : :bad_request
if request_from_katello_cli?
# TODO: why not use http_code from the exception???
render :json => format_subsys_exception_hash(exception), :status => :bad_request
render :json => format_subsys_exception_hash(exception), :status => status
else
respond_for_exception(exception, :status => :bad_request)
respond_for_exception(exception, :status => status)
end
end

Expand Down Expand Up @@ -149,16 +151,26 @@ def pp_exception(exception, options = {})
message = ""
message << "#{exception.class}: " if options[:with_class]
message << "#{exception.message}\n"
message << "Body: #{exception.http_body}\n" if options[:with_body] && exception.respond_to?(:http_body)
body = if exception.respond_to?(:http_body)
exception.http_body
elsif exception.respond_to?(:response_body)
exception.response_body
end
message << "Body: #{body}\n" if options[:with_body] && body
message << exception.backtrace.join("\n") if options[:with_backtrace]
message
end

def format_subsys_exception_hash(exception)
orig_hash = JSON.parse(exception.response).with_indifferent_access rescue {}
raw_body = if exception.respond_to?(:response)
exception.response
elsif exception.respond_to?(:response_body)
exception.response_body
end
orig_hash = JSON.parse(raw_body).with_indifferent_access rescue {}

orig_hash[:message] = orig_hash.delete(:displayMessage) || orig_hash[:message]
orig_hash[:message] = exception.response.to_s.gsub(/^"|"$/, "") if orig_hash[:message].nil? && exception.respond_to?(:response)
orig_hash[:message] = raw_body.to_s.gsub(/^"|"$/, "") if orig_hash[:message].nil? && raw_body
orig_hash[:message] = exception.message if orig_hash[:message].blank?
orig_hash[:errors] = [orig_hash[:message]] if orig_hash[:errors].nil?
orig_hash
Expand Down
43 changes: 43 additions & 0 deletions app/lib/katello/concerns/oauth_request_signing.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
require 'oauth'

module Katello
module Concerns
module OauthRequestSigning
extend ActiveSupport::Concern

HTTP_CLASSES = {
get: Net::HTTP::Get,
post: Net::HTTP::Post,
put: Net::HTTP::Put,
patch: Net::HTTP::Patch,
delete: Net::HTTP::Delete,
}.freeze

class_methods do
def sign_request(req, url, method)
fail "#{name}: OAuth consumer_key and consumer_secret required" if self.consumer_key.blank? || self.consumer_secret.blank?
req.headers['Authorization'] = build_oauth_header(url, method)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
end

private

def oauth_consumer
@oauth_consumer ||= OAuth::Consumer.new(
self.consumer_key, self.consumer_secret,
:site => self.site,
:request_token_path => "",
:authorize_path => "",
:access_token_path => "",
:ca_file => self.ssl_ca_file
)
end

def build_oauth_header(url, method)
request = HTTP_CLASSES.fetch(method).new(url)
oauth_consumer.sign!(request)
request['Authorization']
end
end
end
end
end
Loading
Loading