-
Notifications
You must be signed in to change notification settings - Fork 3k
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 support for custom SSLContext when using FastHttpUser #2113
add support for custom SSLContext when using FastHttpUser #2113
Conversation
locust/contrib/fasthttp.py
Outdated
@@ -107,6 +110,13 @@ def __init__( | |||
# store authentication header (we construct this by using _basic_auth_str() function from requests.auth) | |||
self.auth_header = _construct_basic_auth_str(parsed_url.username, parsed_url.password) | |||
|
|||
def _check_ssl_context(ssl_context): |
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.
Not sure this deserves its own method :)
Lgtm, apart from the style issues & my comment |
Thanks for reviewing it @cyberw . Pushed some fixes. |
locust/contrib/fasthttp.py
Outdated
if ssl_context: | ||
if not isinstance(ssl_context, SSLContext): | ||
raise TypeError("You must provide a valid SSLContext.") | ||
ssl_context_factory = lambda: ssl_context |
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.
Doesnt it make more sense to just pass a function instead of an instantiated SSLContext that we make a fake function for? (I made a comment and then deleted it, but after some more thought I think my initial comment was correct :)
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.
Surely it does. I was trying to make use of ssl_context
as an actual context then the caller could set it accordingly, but making it a factory keeps the way it does in the background. I will rename it to ssl_context_factory
and type-hint it as Callable. Thanks for reviewing it.
Nice! But the test doesnt really check anything, does it? I guess it could be hard to validate that the context was actually used in the request, but we could at least check that the function was called (perhaps by setting a value there that we assert on) |
Indeed! Now I realised what it was doing. Although it would fail using a wrong context that wasn't the real intention of that test. Added two new tests on the right place (I hope). They are |
As per #2066, currently there's no option for setting up certificates when using
FastHttpUser
. This PR intends to add a newssl_context_factory
param to enabling this functionality by defining a customCallable
returning a SSLContext when implementing the user.As the callable will be called within the class context if it is defined outside the
ApiLocust
class we need to fake an arg there. The two example on how implementing can be found below.Option 1
Option 2