Skip to content

Initial commit for proxy support #1936

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.

Sign up for GitHub

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

Open
gareth-ellis wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Initial commit for proxy support #1936

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c8f398
Initial commit for proxy support
gareth-ellis Mar 31, 2025
ba50092
Fix mypy issue
gareth-ellis Apr 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions esrally/client/asynchronous.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ def _create_aiohttp_session(self):
cookie_jar=aiohttp.DummyCookieJar(),
request_class=self._request_class,
response_class=self._response_class,
trust_env=True,
connector=connector,
trace_configs=self.trace_configs,
)
Expand Down
13 changes: 12 additions & 1 deletion esrally/client/synchronous.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.

import logging
import os
import warnings
from collections.abc import Iterable, Mapping
from typing import Any, Optional
Expand All @@ -25,6 +27,7 @@
HeadApiResponse,
ListApiResponse,
ObjectApiResponse,
RequestsHttpNode,
TextApiResponse,
)
from elastic_transport.client_utils import DEFAULT
Expand Down Expand Up @@ -123,9 +126,17 @@ def check_product(cls, headers, response):

class RallySyncElasticsearch(Elasticsearch):
def __init__(self, *args, **kwargs):
self.logging = logging.getLogger(__name__)
distribution_version = kwargs.pop("distribution_version", None)
distribution_flavor = kwargs.pop("distribution_flavor", None)
super().__init__(*args, **kwargs)
proxy_env_vars = ["http_proxy", "https_proxy", "HTTP_PROXY", "HTTPS_PROXY", "all_proxy", "ALL_PROXY"]
if any(x in os.environ for x in proxy_env_vars):
# If we have proxy env vars, we need to use the requests transport
# to ensure that we use the correct proxy settings.
Comment on lines +134 to +135
Copy link
Member

@pquentin pquentin Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered using requests inconditionally here? Without any proxy tests, requests support could be subtly broken.

Copy link
Member Author

@gareth-ellis gareth-ellis Mar 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it - my aim here though was to try and retain as much current behaviour when not using proxies as possible. I was aiming to get in a set of proxy tests to cover the proxy use case.

Copy link
Contributor

@fressi-elastic fressi-elastic Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen with any of these variables set to empty value? Example:

http_proxy=

Copy link
Member Author

@gareth-ellis gareth-ellis Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requests would be used, but requests likely would not use the proxy. Requests actually looks at any env var ending with _proxy - we can check if there is a value if you feel that is necessary, though i don't want to start doing any other validation on the input, since that will automatically be up to the client that we are using (requests for synchronus, aiohttp for async).

self.logging.warning("Proxy settings detected. Using requests transport.")
super().__init__(*args, node_class=RequestsHttpNode, **kwargs)
Copy link
Contributor

@fressi-elastic fressi-elastic Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If kwargs already contains "node_class" key, then this would raise an exception. I think you should add node_class as an expecific parameter at the beginning of this method so that in case is None then we can replace its value. Example:

def __init__(self, node_class = None, **kwargs):
   
   # ...
  if node_class is None and any(x in os.environ for x in proxy_env_vars):
      node_class = RequestsHttpNode
  super().__init__(*args, node_class=node_class, **kwargs)

else:
super().__init__(*args, **kwargs)
self._verified_elasticsearch = None
self.distribution_version = distribution_version
self.distribution_flavor = distribution_flavor
Expand Down