Skip to content

Add support for benchmarking TLS endpoints#319

Open
rgerganov wants to merge 1 commit into
mlcommons:mainfrom
rgerganov:ssl-support
Open

Add support for benchmarking TLS endpoints#319
rgerganov wants to merge 1 commit into
mlcommons:mainfrom
rgerganov:ssl-support

Conversation

@rgerganov
Copy link
Copy Markdown

Enable the benchmark and probe commands to target HTTPS endpoints, including those serving self-signed certificates via a new --insecure flag that skips TLS certificate verification on the SSL context used by worker connections.

Also fix HttpResponseProtocol.eof_received() to return False so asyncio closes the transport itself; returning True is a no-op on SSL transports (which don't support TCP half-close) and was leaving pending body futures unresolved on connection teardown.

What does this PR do?

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor/cleanup

Related issues

Testing

  • Tests added/updated
  • All tests pass locally
  • Manual testing completed

Checklist

  • Code follows project style
  • Pre-commit hooks pass
  • Documentation updated (if needed)

Enable the benchmark and probe commands to target HTTPS endpoints, including
those serving self-signed certificates via a new --insecure flag that skips
TLS certificate verification on the SSL context used by worker connections.

Also fix HttpResponseProtocol.eof_received() to return False so asyncio
closes the transport itself; returning True is a no-op on SSL transports
(which don't support TCP half-close) and was leaving pending body futures
unresolved on connection teardown.
@rgerganov rgerganov requested review from a team and Copilot May 20, 2026 12:16
@github-actions
Copy link
Copy Markdown

MLCommons CLA bot:
Thank you very much for your submission; we really appreciate it. Before we can accept your contribution,
we ask that you sign the MLCommons CLA (Apache 2). Please submit your GitHub ID to our onboarding form to initiate
authorization. If you are from a MLCommons member organization, we will request that you be added to the CLA.
If you are not from a member organization, we will email you a CLA to sign. For any questions, please contact
support@mlcommons.org.
0 out of 1 committers have signed the MLCommons CLA.
@rgerganov
You can retrigger this bot by commenting recheck in this Pull Request

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enables HTTPS/TLS targeting for the endpoint client used by the benchmark and probe commands, including an opt-in --insecure flag for connecting to endpoints with self-signed/untrusted certificates. It also fixes HttpResponseProtocol.eof_received() to return False so asyncio reliably closes the transport (including SSL transports) and resolves pending body futures on teardown.

Changes:

  • Add insecure (--insecure) to HTTPClientConfig and propagate it into worker TLS ssl.SSLContext configuration (skip hostname/cert verification when enabled).
  • Ensure HTTPS connections are actually established by passing an SSL context into the worker connection pool when the endpoint scheme is https.
  • Fix HttpResponseProtocol.eof_received() to return False and update the unit test expectation accordingly; update benchmark config templates and probe CLI to expose --insecure.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/unit/endpoint_client/test_http.py Updates unit test to assert eof_received() returns False for correct asyncio transport closure behavior.
src/inference_endpoint/endpoint_client/worker.py Creates and configures an SSL context for https:// endpoints; applies insecure by disabling verification.
src/inference_endpoint/endpoint_client/http.py Changes eof_received() to return False and documents why this is required (especially for SSL transports).
src/inference_endpoint/endpoint_client/config.py Adds insecure config/CLI flag to control TLS certificate verification.
src/inference_endpoint/config/templates/online_template_full.yaml Adds settings.client.insecure to the full online benchmark template.
src/inference_endpoint/config/templates/offline_template_full.yaml Adds settings.client.insecure to the full offline benchmark template.
src/inference_endpoint/config/templates/concurrency_template_full.yaml Adds settings.client.insecure to the full concurrency benchmark template.
src/inference_endpoint/commands/probe.py Adds --insecure to probe CLI and forwards it into HTTPClientConfig.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces an "insecure" configuration option across the inference endpoint client, CLI, and configuration templates to allow skipping TLS certificate verification. Additionally, it updates the eof_received method in the HTTP protocol implementation to return False, ensuring proper transport closure for SSL connections that do not support TCP half-close. I have no feedback to provide as there were no review comments to evaluate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants