-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass default SSLContext instances to Octoprint custom HTTP sessions #105351
Conversation
Hey there @rfleming71, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
No test case currently, as I could not think of a clean way to write one for this. Happy to add if anyone has a good idea how to go about it. |
Can you please explain this further? When are different requests using different SSL configurations? |
Sure. Since #90001, the Octoprint integration is creating its own Most other parts of Home Assistant, such as the REST integration, use the shared "default" HTTP sessions ( An Concretely, the way that Home Assistant configures its contexts is similar to what Thus, the Octoprint integration may fail to make a TLS connection while the REST integration succeeds for the same server, or vice versa. This happens when the user has set The best way to ensure consistent behavior is to try to use the same contexts everywhere. Re-using contexts also has performance benefits. The context is the container for various caches such as TLS session identifiers/tickets, which enable faster and more efficient reconnections. And of course, slightly less memory usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vexofp 👍
tagged for 2024.1.6 because this fixes the |
Proposed change
This PR restores the use of the default Home Assistant
SSLContext
instances in the Octoprint integration. In #90001, a switch was made to use custom HTTP sessions. These sessions are currently creating their own SSL contexts. Using different contexts can lead to inconsistent request failures, due to different SSL configurations being used for different requests (which is how I discovered this).Currently, we pass
ssl=None
/False
when creating the customaiohttp.TCPConnector
, which causes it to create its own context with or without verification, respectively. Instead, this PR passes in the appropriately configured default Home AssistantSSLContext
instance fromget_default_context()
orget_default_no_verify_context()
, respectively.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: