Revert "Internal client config refactor"

[aramprice]

Reverts #2710

[coderabbitai]

Walkthrough

This pull request refactors SSL/TLS certificate handling across multiple BOSH components. The changes shift from file metadata-based validation (using File.file? and File.zero?) to content-oriented validation (using File.exist? and File.read with strip-emptiness checks). When CA certificate files are missing or empty, the code now initializes OpenSSL::X509::Store with default paths instead of omitting SSL configuration. The bosh-monitor component's SSLHelpers module is removed, and its functionality is inlined. Multiple method signatures in the HTTP request helper are updated to remove ca_cert parameters. Some endpoints now use unconditional VERIFY_NONE SSL verification mode rather than conditional peer verification based on certificate availability.

Suggested reviewers

• mkocher

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is largely incomplete. It only contains a reference to the reverted PR without addressing most required template sections.	Add explanations for: what changed, contextual links, tests run, release notes description, breaking changes, and team tags per the template.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main purpose of the PR: reverting a previous refactor. It is concise and clearly communicates the primary change.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch revert-2710-client-tls-refactor

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb (1)

3-18: ⚠️ Potential issue | 🟡 Minor

describe descriptor does not match the class under test.

The top-level describe is 'Bosh::Monitor::Plugins::Resurrector', but plugin on Line 18 is instantiated as Bosh::Monitor::Plugins::EventLogger.new(options) (and the file is event_logger_spec.rb). This produces misleading RSpec documentation output and conflicts with the AI summary for this file. Given the filename and the EventLogger.new subject, the descriptor should remain EventLogger — please revert this string alongside the rest of the revert.

Proposed fix

-describe 'Bosh::Monitor::Plugins::Resurrector' do
+describe 'Bosh::Monitor::Plugins::EventLogger' do

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb` around
lines 3 - 18, The RSpec top-level describe string is incorrect: change the
descriptor from 'Bosh::Monitor::Plugins::Resurrector' to
'Bosh::Monitor::Plugins::EventLogger' so it matches the instantiated subject
plugin (Bosh::Monitor::Plugins::EventLogger.new(options)) and the file
event_logger_spec.rb; update only the describe(...) string to reflect
EventLogger so RSpec output and test summaries align with the class under test.

src/bosh-director/spec/unit/bosh/director/config_server/auth_http_client_spec.rb (1)

26-44: 🧹 Nitpick | 🔵 Trivial

Prefer let(:store_double) over a shared closure variable.

Declaring store_double = nil at the shared-examples body level evaluates once at load time and then gets mutated by the before block via closure. It works today because before reassigns per example, but it's non-idiomatic, brittle, and easy to misuse if another example is added that reads the variable before the before runs. Idiomatic RSpec here is let.

♻️ Proposed refactor

       shared_examples 'cert_store' do
-        store_double = nil
+        let(:store_double) { instance_double(OpenSSL::X509::Store) }

         before do
           allow(http_client).to receive(:use_ssl=).with(true)
           allow(http_client).to receive(:verify_mode=).with(OpenSSL::SSL::VERIFY_PEER)

-          store_double = instance_double(OpenSSL::X509::Store)
           allow(store_double).to receive(:set_default_paths)
           allow(OpenSSL::X509::Store).to receive(:new).and_return(store_double)
         end

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@src/bosh-director/spec/unit/bosh/director/config_server/auth_http_client_spec.rb`
around lines 26 - 44, The shared_examples 'cert_store' uses a closure variable
store_double initialized at load time and reassigned in the before block;
replace this pattern with an RSpec let by defining let(:store_double) {
instance_double(OpenSSL::X509::Store) } and update the before block to stub
methods on store_double (e.g. allow(store_double).to
receive(:set_default_paths)) and the OpenSSL::X509::Store constructor stub
(allow(OpenSSL::X509::Store).to receive(:new).and_return(store_double)); keep
expectations referencing cert_store= and store_double as before but remove the
top-level store_double = nil declaration.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@src/bosh-director/spec/unit/bosh/director/config_server/auth_http_client_spec.rb`:
- Around line 57-58: Scope the File stubs to only the CA cert path used in this
example: replace the unscoped allow(File).to receive(:exist?).and_return(true)
and allow(File).to receive(:read).and_return('') with scoped stubs that match
the configured CA path (e.g. allow(File).to
receive(:exist?).with(ca_cert_path).and_return(true) and allow(File).to
receive(:read).with(ca_cert_path).and_return('')), and if other File calls must
remain real, add a fallback like allow(File).to
receive(:exist?).and_call_original / allow(File).to
receive(:read).and_call_original for other args so only the CA file is stubbed;
update the spec variables (ca_cert_path/configured path) used by AuthHttpClient
test accordingly.

In `@src/bosh-monitor/lib/bosh/monitor/director.rb`:
- Line 77: The error path in method info interpolates an undefined local
variable `http`, causing NameError instead of raising DirectorError; change the
interpolation to reference the correct endpoint variable used elsewhere (e.g.,
`parsed_endpoint` or the same `endpoint + '/info'` style used by
deployments/resurrection_config/get_deployment_instances) so the raise becomes:
raise DirectorError, "Cannot get status from director at
#{parsed_endpoint}/info: #{status} #{body}" (or equivalent using
async_endpoint/endpoint) and ensure the message includes status and body.

In `@src/bosh-monitor/spec/unit/bosh/monitor/plugins/http_request_helper_spec.rb`:
- Line 251: The example description is misleading: inside the describe
'#send_http_post_request_synchronous_with_tls_verify_peer' -> context 'making
the request' the it block currently reads "sends a get request" but calls
send_http_post_request_synchronous_with_tls_verify_peer; update the it
description to reflect a POST (e.g., "sends a post request" or "sends a POST
request with TLS peer verification") so the test description matches the
behavior and target method name.

---

Outside diff comments:
In
`@src/bosh-director/spec/unit/bosh/director/config_server/auth_http_client_spec.rb`:
- Around line 26-44: The shared_examples 'cert_store' uses a closure variable
store_double initialized at load time and reassigned in the before block;
replace this pattern with an RSpec let by defining let(:store_double) {
instance_double(OpenSSL::X509::Store) } and update the before block to stub
methods on store_double (e.g. allow(store_double).to
receive(:set_default_paths)) and the OpenSSL::X509::Store constructor stub
(allow(OpenSSL::X509::Store).to receive(:new).and_return(store_double)); keep
expectations referencing cert_store= and store_double as before but remove the
top-level store_double = nil declaration.

In `@src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb`:
- Around line 3-18: The RSpec top-level describe string is incorrect: change the
descriptor from 'Bosh::Monitor::Plugins::Resurrector' to
'Bosh::Monitor::Plugins::EventLogger' so it matches the instantiated subject
plugin (Bosh::Monitor::Plugins::EventLogger.new(options)) and the file
event_logger_spec.rb; update only the describe(...) string to reflect
EventLogger so RSpec output and test summaries align with the class under test.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: bc931557-08fa-4a82-9457-dfea34cd0aa6

📥 Commits

Reviewing files that changed from the base of the PR and between c70527f and 6b772db.

📒 Files selected for processing (20)

• src/bosh-director/lib/bosh/director/config_server/auth_http_client.rb
• src/bosh-director/lib/bosh/director/config_server/uaa_auth_provider.rb
• src/bosh-director/spec/unit/bosh/director/config_server/auth_http_client_spec.rb
• src/bosh-director/spec/unit/bosh/director/config_server/uaa_auth_provider_spec.rb
• src/bosh-monitor/lib/bosh/monitor.rb
• src/bosh-monitor/lib/bosh/monitor/auth_provider.rb
• src/bosh-monitor/lib/bosh/monitor/director.rb
• src/bosh-monitor/lib/bosh/monitor/plugins/event_logger.rb
• src/bosh-monitor/lib/bosh/monitor/plugins/http_request_helper.rb
• src/bosh-monitor/lib/bosh/monitor/plugins/resurrector.rb
• src/bosh-monitor/lib/bosh/monitor/ssl_helpers.rb
• src/bosh-monitor/spec/unit/bosh/monitor/auth_provider_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/director_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/plugins/event_logger_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/plugins/http_request_helper_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/plugins/resurrector_spec.rb
• src/bosh-nats-sync/lib/nats_sync/auth_provider.rb
• src/bosh-nats-sync/lib/nats_sync/users_sync.rb
• src/bosh-nats-sync/spec/nats_sync/auth_provider_spec.rb
• src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb

💤 Files with no reviewable changes (3)

• src/bosh-monitor/lib/bosh/monitor.rb
• src/bosh-nats-sync/spec/nats_sync/users_sync_spec.rb
• src/bosh-monitor/lib/bosh/monitor/ssl_helpers.rb