Skip to content
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

Only use HostHeaderSSLAdapter for SSL/HTTPS connections #4498

Merged
merged 2 commits into from Aug 9, 2018

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Aug 8, 2018

We made a mistake using HostHeaderSSLAdapter even when the connection wasn't HTTPS (in dev, that is). This causes problems connecting to the API.

Fixes #4494

@davidfischer davidfischer requested review from humitos and agjohnson Aug 8, 2018
humitos
humitos approved these changes Aug 8, 2018
Copy link
Member

@humitos humitos left a comment

I'm 👍 with these changes.

I'm not sure if it's not good to expand a bit more the situation. Maybe not in the comment, but in the commit/PR description mentioning the assert_hostname kw and the difference that you found around HTTPSConnectionPool and HTTPConnectionPool.

host_header_ssl.HostHeaderSSLAdapter(
max_retries=3,
),
)
Copy link
Contributor

@agjohnson agjohnson Aug 8, 2018

max_retries was from the original adapter, we should add this with the standard http adapter if the connection is not ssl.

Copy link
Contributor Author

@davidfischer davidfischer Aug 8, 2018

That was not there prior to the PR that added the HostHeaderSSLAdapter. Do you think it is necessary to support retries in dev?

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Aug 9, 2018

I re-added the default adapter class.

Copy link
Contributor

@agjohnson agjohnson left a comment

LGTM!

@agjohnson agjohnson merged commit 2d44811 into master Aug 9, 2018
1 check passed
@agjohnson agjohnson deleted the davidfischer/hostheaderssladapter-fix branch Aug 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants