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

Add httpx.SSLContext configuration. #3022

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

karpetrosyan
Copy link
Member

@karpetrosyan karpetrosyan commented Dec 25, 2023

Discussed in #3007

TODO:

  • Add changelog
  • Add more migration examples

Migration examples

Enabling and disabling verification

import httpx

# instead of 
httpx.Client(verify=False)

# do
context = httpx.SSLContext(verify=False)
httpx.Client(ssl_context=context)

Use a different set of certificates

import httpx

# instead of
httpx.Client(verify="path/to/certs.pem")

# do
context = httpx.SSLContext(verify="path/to/certs.pem")
httpx.Client(ssl_context=context)

Client side certificates

import httpx

# instead of
httpx.Client(cert="path/to/client.pem")

# do
context = httpx.SSLContext(cert="path/to/client.pem")
httpx.Client(ssl_context=context)

Using alternate SSL contexts

import ssl
import httpx

context = ssl.create_default_context()

# instead of
client = httpx.Client(verify=context)

# do
client = httpx.Client(ssl_context=context)

@karpetrosyan karpetrosyan added the api change PRs that contain breaking public API changes label Dec 25, 2023
@T-256
Copy link
Contributor

T-256 commented Dec 26, 2023

Removing verify and cert is too aggressive, I'd recommend deprecate them first.

@karpetrosyan
Copy link
Member Author

Yes, I agree that removing them completely is not what the user expects.
However, I believe migration is simple enough to skip that stage in order to avoid making that part of the code overly complicated.

Or should we deprecate them anyway? Not obvious to me.

@T-256
Copy link
Contributor

T-256 commented Dec 26, 2023

There are dropping from top-level API, I think deprecation policy should be applied here.

Also, we have other deprecated drops those pending to remove in v1.0.0 (IIUC):

  • proxies=
  • data= (non-Mapping objects)
  • cookies= (per-request)

httpx/_config.py Outdated Show resolved Hide resolved
httpx/_client.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member

Looking good.

Update documentation

I'd suggest you don't spin time on that until we've had a clear discussion around the "API finessing" and "Documentation refresh" areas mentioned in #947 (comment). I like this change, but we'll want to make it as part of a coherent & consistent 1.0 refresh.

@tomchristie tomchristie mentioned this pull request Jan 8, 2024
@tomchristie tomchristie added the 1.0 proposal Pull requests proposing 1.0 API changes label Jan 11, 2024
@tomchristie tomchristie changed the title Replace the verify and cert arguments with ssl_context. Add httpx.SSLContext configuration. Jan 12, 2024
@tomchristie
Copy link
Member

Okay, shall we take the docs from this comment... #3007 (comment) and include them in the PR?

@karpetrosyan
Copy link
Member Author

Okay, shall we take the docs from this comment... #3007 (comment) and include them in the PR?

I like it

@karpetrosyan
Copy link
Member Author

Have we decided to remove or deprecate the old arguments?

@T-256
Copy link
Contributor

T-256 commented Feb 2, 2024

Have we decided to remove or deprecate the old arguments?

deprecate warning on 0.27
raise error on 1.0
remove at some point later on 1.x

This is what I can be happy with.

@T-256 T-256 mentioned this pull request Feb 2, 2024
3 tasks
@karpetrosyan
Copy link
Member Author

karpetrosyan commented Feb 4, 2024

Is there a reason we should continue to support the old API?

As far as I can tell, the primary reason for deprecating is that we are adding new functionality and fixing bugs in applications that use the old API.
Do we want to deprecate them only for 1-2 releases before the 1.0 version?

@T-256
Copy link
Contributor

T-256 commented Feb 4, 2024

My contextual reason got from https://astral.sh/blog/ruff-v0.2.0

Do we want to deprecate them only for 1-2 releases before the 1.0 version?

Actually, those releases are for users using version pinning (httpx < 1.0.0) before major migrating. they might be decided that they aren't ready to stop using old api. so, they can cope with warnings and use latest versions of which deprecated apis allowed to use.

@karpetrosyan
Copy link
Member Author

they might be decided that they aren't ready to stop using old api. so, they can cope with warnings and use latest versions of which deprecated apis allowed to use.

Actually, we are very close to HTTPX 1.0, so I believe there will be just 1-2 releases before then, in which we would primarily introduce the new API, so there will be no features that we would want to ship to our users who are still using the old API.

Also, this migration is rather easy, and I don't believe we need to complicate this PR. Client configurations are frequently found in 1-2 places across the source code, therefore let's make this PR concise and avoid dealing with both new and old APIs.

@T-256
Copy link
Contributor

T-256 commented Feb 6, 2024

Then, we can merge this PR for v1.0 but not 0.27. (breaking api)

raise error on 1.0
remove at some point later on 1.x

The error could describe the reason for the drop. This could be removed sometimes later.
I think it's better than confusing TypeError: 'verify' is an invalid keyword argument for httpx.Client() message.

@tomchristie
Copy link
Member

Then, we can merge this PR for v1.0 but not 0.27.

Yep, that's what we'll do.

The error could describe the reason for the drop

We might choose not to handle the error cases for this: it requires leaving dead code across a lot of function calls.
I'm okay with us just making this change outright with a 1.0 release, so long as it's clearly documented in the release.

@tomchristie
Copy link
Member

Related... #3126

@tomchristie
Copy link
Member

Looks wonderful... some conflicts that need resolving, other than that I think we ought to get this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 proposal Pull requests proposing 1.0 API changes api change PRs that contain breaking public API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants