This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
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.
support federation queries through http connect proxy #10475
support federation queries through http connect proxy #10475
Changes from 19 commits
85081fa
7115567
ef8792a
781da36
196af98
86b3623
3c7c3b6
cee7f65
e8cb087
73c5ff5
4cd4a48
1595da4
5acd7ce
3342bda
0bf5ac8
b81eaaf
08b19a6
70a8b11
b68a14d
b766a0f
eab6c74
73a7380
007ea8a
258fab8
08823be
67522c3
512166b
ab2067d
e8fd305
d0933e5
c953242
2720c88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I am not sure if
BrowserLikePolicyForHTTPS
is the best default policy.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.
I don't think we need a default policy here.
tls_client_options_factory=None
is supposed to disable TLS, not fall back to a default. I would make thetls_options_factory
parameter to_http_proxy_endpoint
Optional, and raise an Exception if the scheme ishttps
but there is no tls factory.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.
Im not sure what is the best error to raise.
ValueError
,ConfigError
,RuntimeError
or anything else?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.
should we just move this into
HTTPConnectProxyEndpoint
, to save doing it each time we construct one?(in other words: make
HTTPConnectProxyEndpoint
take anOptional[ProxyCredentials]
parameter instead of a customheaders
parameter)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.
This change was larger.
I had to move
ProxyCredentials
fromproxyagent
toconnectproxyclient
. The reason was a circular import.I have replaced
headers
parameter because it was introduced in #9657 only for proxy connections and is not needed anymore.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.
per #9306 (comment):
I suggest you move the code at lines 330-334 which builds a
BlacklistingReactorWrapper
intoMatrixFederationAgent
. There is no need forMatrixFederationHttpClient.reactor
to be aBlacklistingReactorWrapper
.