Refactor internal client config machinery

[aramprice]

No description provided.

[coderabbitai]

Walkthrough

This pull request updates TLS certificate verification across the BOSH monitor and NATS sync components. SSL connections now use VERIFY_PEER instead of VERIFY_NONE, with optional support for custom director CA certificates. HTTP helper methods in HttpRequestHelper are refactored to use keyword arguments instead of positional parameters, and the ca_cert_path parameter is threaded through async and synchronous request handlers. The director module and plugins (EventLogger, Resurrector, Pagerduty) are updated to integrate with the new TLS configuration and method signatures. Comprehensive unit tests validate TLS peer verification behavior with both system defaults and custom CA files.

Suggested reviewers

• mkocher
• ragaskar
• ystros

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author, leaving all required template sections empty: change rationale, context, tests, release notes, breaking changes, and team tags.	Add a comprehensive pull request description following the template: explain the TLS verification changes, link relevant issues, list tests run, provide release notes, and tag the team.
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.70% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title is vague and generic, describing 'internal client config machinery' without specifying the actual change—enabling TLS peer certificate verification and CA certificate support across multiple HTTP clients.	Revise the title to explicitly mention the main change: e.g., 'Enable TLS peer certificate verification for HTTP clients' or 'Add CA certificate support to HTTP clients'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ 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 cleanup-http-options

---

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

_{Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.}

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

[ragaskar]

Aram and I looked through this changeset together on zoom.

We agreed that there could be some consul plugin breakage, and this likely should be solved by folks relying on this behavior adding any needed certs to the director trust store.

[coderabbitai]

Actionable comments posted: 1

🤖 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-nats-sync/lib/nats_sync/users_sync.rb`:
- Around line 118-120: The TLS peer verification can raise
OpenSSL::SSL::SSLError which isn't included in the current retry/fallback set
and isn't rescued by execute_users_sync; update the failure handling so SSL
errors trigger the degraded/retry path: add OpenSSL::SSL::SSLError to
DIRECTOR_CONNECTION_ERRORS (or the equivalent error list/constant) so
wait_for_director_connection and query_all_running_vms treat it like other
transient director connection errors, and expand the rescue in
execute_users_sync to also catch OpenSSL::SSL::SSLError (alongside RuntimeError)
to ensure TLS handshake failures fall back to degraded mode rather than bubbling
up.

🪄 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: 47479891-201e-4498-b744-578b6d6f681d

📥 Commits

Reviewing files that changed from the base of the PR and between 4e1be33 and 868b400.

📒 Files selected for processing (13)

• src/bosh-monitor/lib/bosh/monitor/director.rb
• src/bosh-monitor/lib/bosh/monitor/plugins/consul_event_forwarder.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/pagerduty.rb
• src/bosh-monitor/lib/bosh/monitor/plugins/resurrector.rb
• src/bosh-monitor/spec/unit/bosh/monitor/director_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/plugins/consul_event_forwarder_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/pagerduty_spec.rb
• src/bosh-monitor/spec/unit/bosh/monitor/plugins/resurrector_spec.rb
• src/bosh-nats-sync/lib/nats_sync/users_sync.rb

[aramprice]

One-off integration build: https://bosh.ci.cloudfoundry.org/builds/349753046