-
Notifications
You must be signed in to change notification settings - Fork 1
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
pyatls: urllib3 and requests support with cleanup #10
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports You could have the same capabilities but better runtime performances if you use a MegaLinter flavor:
|
17218a4
to
30d8b70
Compare
d65f47e
to
2c9d6ad
Compare
d576517
to
d701564
Compare
d701564
to
4130e15
Compare
python-package/README.md
Outdated
@@ -72,12 +72,12 @@ running on a confidential ACI instance with the corresponding attestation | |||
document issuer, and submit an HTTP request: | |||
|
|||
```python | |||
from atls import AttestedHTTPSConnection, AttestedTLSContext | |||
from atls import HTTPAConnection, ATLSContext |
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.
Would be great to add the httpa
example here.
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.
Added.
|
||
|
||
def use_requests() -> None: | ||
session = requests.Session() |
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.
nit: possibly low priority but better to add this as pytest.
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'd like to add unit tests that run automatically but we need an ACI-capable server instance. I do want to add tests for things that can be tested without a server-side component, though; that's on my to-do list.
# Mount the HTTP/aTLS adapter such that any URL whose scheme is httpa:// | ||
# results in an HTTPAConnection object that in turn establishes an aTLS | ||
# connection with the server. | ||
session.mount("httpa://", HTTPAAdapter([validator])) |
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.
How to set timeout for the connection in this case?
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.
session.request(..., timeout=)
if _orig_urllib3_connection_cls is None: | ||
return | ||
|
||
import urllib3 |
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.
Nit: can we just do this once at the top of the file?
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.
Changed.
from urllib3.util.retry import Retry as Retry | ||
|
||
|
||
class _HTTPAConnectionShim(HTTPAConnection): |
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.
Does this class need to be defined twice?
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.
If you look closely, they're slightly different.
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.
Why can't we use this definition for the inner class in the other spot? The only difference I can see is where validators is gotten from
from urllib3.util.retry import Retry as Retry | ||
|
||
|
||
class _HTTPAConnectionShim(HTTPAConnection): |
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.
Why can't we use this definition for the inner class in the other spot? The only difference I can see is where validators is gotten from
HTTPAConnection class. | ||
""" | ||
|
||
Validators: List[Validator] |
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.
Nit
Validators: List[Validator] | |
validators: List[Validator] |
Overview
This PR contains various improvements and features. Namely, this PR:
atls.validators.AzAasAciValidtor
becomesatls.validators.azure.aas.aci.AciValidator
;urllib3
such that, with the patch in place, all HTTPS requests automatically become HTTP/aTLS requests, enabling users ofurllib3
to leverage aTLS (this is not required for our usage, I left this here because it was the first idea I had to makerequests
work with aTLS, but I had a better one afterward and figured I could leave the directurllib3
support if someone wants to use it);requests
library via an adapter that captures URLs with thehttpa://
scheme and routes them into HTTP/aTLS connections, enabling users ofrequests
to transparently adopt aTLS.Note: Commits are structured to be reviewed independently.
Requests Adapter
The primary addition of this PR is item no. 3.
Since the
requests
adapter ties cleanly into the library, consumers of it can seamlessly leverage the library's support for connection pooling. With connection pooling, it is no longer necessary to create a new aTLS connection for every request. Instead, a readily established connection can be recycled for subsequent requests. This in turn dramatically reduces request latency.The
HTTPAAdapter
captures requests made against URLs with thehttpa://
scheme and uses this package'sHTTPAConnection
class. This automatically upgrades any requests made with that scheme to HTTP/aTLS.Sample Usage: