Add Advisor and Vulnerabilities to Foreman ping #1042
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduce a ForemanRhCloud::Ping model to perform health checks against advisor and vulnerability services and wire it into the plugin via ping and status extensions. Sequence diagram for registering ping and status extensionssequenceDiagram
participant Plugin as ForemanRhCloud::Plugin
participant Ping as ForemanRhCloud::Ping
Plugin->>Ping: register_ping_extension { Ping.ping }
Plugin->>Ping: register_status_extension { Ping.status }
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
jeremylenz
force-pushed
the
rh_cloud_ping
branch
2 times, most recently
from
62616cb to
faca077
Compare
jeremylenz
force-pushed
the
rh_cloud_ping
branch
from
36e4273 to
00cb424
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jeremylenz - I've reviewed your changes - here's some feedback:
- SERVICE_URLS currently defines the vulnerability endpoint as "/api//api/…" – please fix the duplicate path segment.
- Consider extracting the SSL-enabled RestClient logic in ping_url into a reusable HTTP client helper to reduce duplication and improve testability.
- The ping_services and ping! methods mix status computation and control flow – you might simplify by returning a single structured result object or raising specific exceptions for clearer error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SERVICE_URLS currently defines the vulnerability endpoint as "/api//api/…" – please fix the duplicate path segment.
- Consider extracting the SSL-enabled RestClient logic in ping_url into a reusable HTTP client helper to reduce duplication and improve testability.
- The ping_services and ping! methods mix status computation and control flow – you might simplify by returning a single structured result object or raising specific exceptions for clearer error handling.
## Individual Comments
### Comment 1
<location> `app/models/foreman_rh_cloud/ping.rb:8` </location>
<code_context>
+
+ SERVICE_URLS = {
+ :advisor => "https://localhost:24443/api/insights/v1/status/live/",
+ :vulnerability => "https://localhost:24443/api//api/vulnerability/v1/apistatus"
+ }
+
</code_context>
<issue_to_address>
Double slash in vulnerability service URL may cause issues.
The vulnerability service URL includes a double slash, which may cause unexpected server behavior. Please normalize the URL.
</issue_to_address>
### Comment 2
<location> `app/models/foreman_rh_cloud/ping.rb:63` </location>
<code_context>
+
+ options = {}
+ options[:ssl_ca_file] = ca_file unless ca_file.nil?
+ options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(File.read(Setting[:ssl_certificate]))
+ options[:ssl_client_key] = OpenSSL::PKey.read(File.read(Setting[:ssl_priv_key]))
+
+ client = RestClient::Resource.new(url, options)
</code_context>
<issue_to_address>
No error handling for missing or invalid SSL certificate/key files.
Add error handling for missing or invalid certificate/key files to prevent unhandled exceptions and improve error reporting.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
options = {}
options[:ssl_ca_file] = ca_file unless ca_file.nil?
options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(File.read(Setting[:ssl_certificate]))
options[:ssl_client_key] = OpenSSL::PKey.read(File.read(Setting[:ssl_priv_key]))
client = RestClient::Resource.new(url, options)
=======
options = {}
options[:ssl_ca_file] = ca_file unless ca_file.nil?
begin
cert_content = File.read(Setting[:ssl_certificate])
key_content = File.read(Setting[:ssl_priv_key])
options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(cert_content)
options[:ssl_client_key] = OpenSSL::PKey.read(key_content)
rescue Errno::ENOENT => e
Rails.logger.error("SSL certificate or key file not found: #{e.message}")
raise "SSL certificate or key file not found: #{e.message}"
rescue OpenSSL::OpenSSLError => e
Rails.logger.error("Invalid SSL certificate or key: #{e.message}")
raise "Invalid SSL certificate or key: #{e.message}"
end
client = RestClient::Resource.new(url, options)
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `app/models/foreman_rh_cloud/ping.rb:68` </location>
<code_context>
+
+ client = RestClient::Resource.new(url, options)
+
+ response = client.get
+ return {} if response.empty?
+ begin
+ result = JSON.parse(response).with_indifferent_access
</code_context>
<issue_to_address>
Empty response check may not be robust for all RestClient responses.
Since not all RestClient responses implement `empty?`, check the response body or status code directly to prevent potential errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
response = client.get
return {} if response.empty?
begin
result = JSON.parse(response).with_indifferent_access
rescue JSON::ParserError, NoMethodError
result = { :response => response.body&.strip }
end
result
=======
response = client.get
# Robustly check for empty response body or non-200 status
if response.nil? || (response.respond_to?(:body) && response.body.to_s.strip.empty?) || (response.respond_to?(:code) && response.code != 200)
return {}
end
begin
result = JSON.parse(response).with_indifferent_access
rescue JSON::ParserError, NoMethodError
# Fallback: handle both String and RestClient::Response
response_body = response.respond_to?(:body) ? response.body : response.to_s
result = { :response => response_body&.strip }
end
result
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `app/models/foreman_rh_cloud/ping.rb:70` </location>
<code_context>
+
+ response = client.get
+ return {} if response.empty?
+ begin
+ result = JSON.parse(response).with_indifferent_access
+ rescue JSON::ParserError, NoMethodError
+ result = { :response => response.body&.strip }
+ end
</code_context>
<issue_to_address>
Catching NoMethodError may mask unrelated issues.
Rescue only the specific errors expected from JSON parsing to avoid hiding unrelated bugs.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+61
to
+66
| options = {} | ||
| options[:ssl_ca_file] = ca_file unless ca_file.nil? | ||
| options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(File.read(Setting[:ssl_certificate])) | ||
| options[:ssl_client_key] = OpenSSL::PKey.read(File.read(Setting[:ssl_priv_key])) | ||
|
|
||
| client = RestClient::Resource.new(url, options) |
There was a problem hiding this comment.
suggestion (bug_risk): No error handling for missing or invalid SSL certificate/key files.
Add error handling for missing or invalid certificate/key files to prevent unhandled exceptions and improve error reporting.
Suggested change
| options = {} | |
| options[:ssl_ca_file] = ca_file unless ca_file.nil? | |
| options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(File.read(Setting[:ssl_certificate])) | |
| options[:ssl_client_key] = OpenSSL::PKey.read(File.read(Setting[:ssl_priv_key])) | |
| client = RestClient::Resource.new(url, options) | |
| options = {} | |
| options[:ssl_ca_file] = ca_file unless ca_file.nil? | |
| begin | |
| cert_content = File.read(Setting[:ssl_certificate]) | |
| key_content = File.read(Setting[:ssl_priv_key]) | |
| options[:ssl_client_cert] = OpenSSL::X509::Certificate.new(cert_content) | |
| options[:ssl_client_key] = OpenSSL::PKey.read(key_content) | |
| rescue Errno::ENOENT => e | |
| Rails.logger.error("SSL certificate or key file not found: #{e.message}") | |
| raise "SSL certificate or key file not found: #{e.message}" | |
| rescue OpenSSL::OpenSSLError => e | |
| Rails.logger.error("Invalid SSL certificate or key: #{e.message}") | |
| raise "Invalid SSL certificate or key: #{e.message}" | |
| end | |
| client = RestClient::Resource.new(url, options) |
jeremylenz
force-pushed
the
rh_cloud_ping
branch
from
1c2029f to
8ae86cf
Compare
ehelms
reviewed
Jul 31, 2025
|
|
||
| SERVICE_URLS = { | ||
| :advisor => "https://localhost:24443/api/insights/v1/status/live/", | ||
| :vulnerability => "https://localhost:24443/api/vulnerability/v1/apistatus" |
There was a problem hiding this comment.
You should be able to derive the base URL from the smart-proxy that represents the gateway allowing the https://localhost:24443 to be informed by the smart-proxy and not hard-coded.
There was a problem hiding this comment.
Further more, there are patterns for using the SmartProxy object we an lean on. This PR adds a method to fetch the iop smart-proxy object.
You could then use the Smart Proxy instance for the iop feature to pass into a ProxyAPI class for the iop smart-proxy where the properties, such as endpoints and specific requests are stored in a single classs creating a nice interface. An example from the Foreman code base:
https://github.com/theforeman/foreman/blob/develop/app/services/proxy_api/puppet_ca.rb
Which can be initialized like:
@puppetca = ProxyAPI::PuppetCA.new :url => puppet_ca_proxy.url
There is likely other re-factorings in the code that could take advantage of this pattern. I think this PR and @nofaralfasi PR can synergize nicely with our Smart Proxy patterns.
There was a problem hiding this comment.
Sounds good. Will wait for Nofar's PR to be in and then refactor.
ehelms
reviewed
Jul 31, 2025
|
|
||
| def status | ||
| { | ||
| version: ForemanRhCloud::VERSION, |
There was a problem hiding this comment.
Is status the unauthenticated one? If so, I think we should not return the version of the plugin. We've seen requests not to display version information in unauthenticated requests.
There was a problem hiding this comment.
This was copied from Katello ping. Should I also remove the version there?
vkrizan
reviewed
Aug 1, 2025
|
|
||
| SERVICE_URLS = { | ||
| :advisor => "https://localhost:24443/api/insights/v1/status/live/", | ||
| :vulnerability => "https://localhost:24443/api/vulnerability/v1/apistatus" |
There was a problem hiding this comment.
Could the hardcoded values be replaced by the URL from the Smart Proxy. This assumes binding on port 24443 on localhost.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Add a
ForemanRhCloud::Pingmodel to::Foreman::Ping.Considerations taken when implementing this change?
I am currently pinging only two URLs (see the code). Please let me know if there are any I should add, or change.
What are the testing steps for this pull request?
First you need a dev environment with with IoP services running.
You can see the list of services with
You should see all the IoP services:
Until you have the IoP services, don't continue.
In
bundle exec rails console:You should see the
:foreman_rh_cloudservices. also tryYou can test the FAIL state by stopping a service, such as
Summary by Sourcery
Add a new Ping model and integrate ping/status endpoints for advisor and vulnerabilities services
New Features: