From 56606ed5417b06984e11a7a65396b5123eacacc1 Mon Sep 17 00:00:00 2001 From: Sandor Nemes Date: Fri, 10 Nov 2023 00:52:10 +0100 Subject: [PATCH 1/2] Set enable_cleanup_closed=True to drop TLS connections without a shutdown (#468) * Set enable_cleanup_closed=True to drop TLS connections without a shutdown AsyncOpenSearch seems to leak TLS connections due to a missing parameter in `aiohttp.TCPConnector`. This causes #172 and was also fixed "upstream" in this issue https://github.com/elastic/elasticsearch-py/issues/1910. Signed-off-by: Sandor Nemes * Update CHANGELOG.md Signed-off-by: Sandor Nemes --------- Signed-off-by: Sandor Nemes Signed-off-by: Daniel (dB.) Doubrovkine Co-authored-by: Daniel (dB.) Doubrovkine --- CHANGELOG.md | 3 ++- opensearchpy/_async/http_aiohttp.py | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bd0ff04..c32a0d17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Use API generator for all APIs ([#551](https://github.com/opensearch-project/opensearch-py/pull/551)) - Merge `.pyi` type stubs inline ([#563](https://github.com/opensearch-project/opensearch-py/pull/563)) - Expanded type coverage to benchmarks, samples and tests ([#566](https://github.com/opensearch-project/opensearch-py/pull/566)) +- Defaulted `enable_cleanup_closed=True` in `aiohttp.TCPConnector` to prevent TLS connection leaks ([#468](https://github.com/opensearch-project/opensearch-py/pull/468)) ### Deprecated - Deprecated point-in-time APIs (list_all_point_in_time, create_point_in_time, delete_point_in_time) and Security Client APIs (health_check and update_audit_config) ([#502](https://github.com/opensearch-project/opensearch-py/pull/502)) ### Removed @@ -162,4 +163,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) [2.2.0]: https://github.com/opensearch-project/opensearch-py/compare/v2.1.1...v2.2.0 [2.3.0]: https://github.com/opensearch-project/opensearch-py/compare/v2.2.0...v2.3.0 [2.3.1]: https://github.com/opensearch-project/opensearch-py/compare/v2.3.0...v2.3.1 -[2.3.2]: https://github.com/opensearch-project/opensearch-py/compare/v2.3.1...v2.3.2 \ No newline at end of file +[2.3.2]: https://github.com/opensearch-project/opensearch-py/compare/v2.3.1...v2.3.2 diff --git a/opensearchpy/_async/http_aiohttp.py b/opensearchpy/_async/http_aiohttp.py index 3c7010ed..f14d5384 100644 --- a/opensearchpy/_async/http_aiohttp.py +++ b/opensearchpy/_async/http_aiohttp.py @@ -376,7 +376,10 @@ async def _create_aiohttp_session(self) -> Any: cookie_jar=aiohttp.DummyCookieJar(), response_class=OpenSearchClientResponse, connector=aiohttp.TCPConnector( - limit=self._limit, use_dns_cache=True, ssl=self._ssl_context + limit=self._limit, + use_dns_cache=True, + enable_cleanup_closed=True, + ssl=self._ssl_context, ), trust_env=self._trust_env, ) From 58b83d8c060e123df682b8e0a6072c60c9bb4fa0 Mon Sep 17 00:00:00 2001 From: "Daniel (dB.) Doubrovkine" Date: Thu, 9 Nov 2023 20:41:37 -0500 Subject: [PATCH 2/2] Added Windows CI. (#569) Signed-off-by: dblock --- .github/workflows/test.yml | 1 + CHANGELOG.md | 1 + opensearchpy/_async/http_aiohttp.py | 4 ++- opensearchpy/connection/base.py | 5 ++++ .../test_async/test_connection.py | 27 ++++++++++--------- .../test_async/test_transport.py | 14 +++++++--- test_opensearchpy/test_transport.py | 14 +++++++--- 7 files changed, 45 insertions(+), 21 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index bd0ac738..f79929bc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -14,6 +14,7 @@ jobs: - { os: 'ubuntu-latest', python-version: "3.10" } - { os: 'ubuntu-latest', python-version: "3.11" } - { os: 'macos-latest', python-version: "3.11" } + - { os: 'windows-latest', python-version: "3.11" } name: test (os=${{ matrix.entry.os }}, python=${{ matrix.entry.python-version }}) continue-on-error: ${{ matrix.entry.experimental || false }} diff --git a/CHANGELOG.md b/CHANGELOG.md index c32a0d17..eeb5dd45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - Added a utf-8 header to all .py files ([#557](https://github.com/opensearch-project/opensearch-py/pull/557)) - Added `samples`, `benchmarks` and `docs` to `nox -rs format` ([#556](https://github.com/opensearch-project/opensearch-py/pull/556)) - Added guide on the document lifecycle API(s) ([#559](https://github.com/opensearch-project/opensearch-py/pull/559)) +- Added Windows CI ([#569](https://github.com/opensearch-project/opensearch-py/pull/569)) ### Changed - Generate `tasks` client from API specs ([#508](https://github.com/opensearch-project/opensearch-py/pull/508)) - Generate `ingest` client from API specs ([#513](https://github.com/opensearch-project/opensearch-py/pull/513)) diff --git a/opensearchpy/_async/http_aiohttp.py b/opensearchpy/_async/http_aiohttp.py index f14d5384..6ed1e884 100644 --- a/opensearchpy/_async/http_aiohttp.py +++ b/opensearchpy/_async/http_aiohttp.py @@ -183,7 +183,9 @@ def __init__( ssl_context.check_hostname = False ssl_context.verify_mode = ssl.CERT_NONE - ca_certs = self.default_ca_certs() if ca_certs is None else ca_certs + if ca_certs is None: + ca_certs = self.default_ca_certs() + if verify_certs: if not ca_certs: raise ImproperlyConfigured( diff --git a/opensearchpy/connection/base.py b/opensearchpy/connection/base.py index 59418bfa..54308c72 100644 --- a/opensearchpy/connection/base.py +++ b/opensearchpy/connection/base.py @@ -138,6 +138,11 @@ def __eq__(self, other: object) -> bool: raise TypeError("Unsupported equality check for %s and %s" % (self, other)) return self.__hash__() == other.__hash__() + def __lt__(self, other: object) -> bool: + if not isinstance(other, Connection): + raise TypeError("Unsupported lt check for %s and %s" % (self, other)) + return self.__hash__() < other.__hash__() + def __hash__(self) -> int: return id(self) diff --git a/test_opensearchpy/test_async/test_connection.py b/test_opensearchpy/test_async/test_connection.py index 9413d0e8..7969e987 100644 --- a/test_opensearchpy/test_async/test_connection.py +++ b/test_opensearchpy/test_async/test_connection.py @@ -37,7 +37,7 @@ import aiohttp import pytest from _pytest.mark.structures import MarkDecorator -from mock import patch +from mock import MagicMock, patch from multidict import CIMultiDict from pytest import raises @@ -254,26 +254,29 @@ async def test_warns_if_using_non_default_ssl_kwargs_with_ssl_context(self) -> N == str(w[0].message) ) - @patch("ssl.SSLContext.load_verify_locations") - async def test_uses_given_ca_certs( - self, load_verify_locations: Any, tmp_path: Any - ) -> None: + @patch("ssl.SSLContext", return_value=MagicMock()) + async def test_uses_given_ca_certs(self, ssl_context: Any, tmp_path: Any) -> None: path = tmp_path / "ca_certs.pem" path.touch() + ssl_context.return_value.load_verify_locations.return_value = None AIOHttpConnection(use_ssl=True, ca_certs=str(path)) - load_verify_locations.assert_called_once_with(cafile=str(path)) + ssl_context.return_value.load_verify_locations.assert_called_once_with( + cafile=str(path) + ) - @patch("ssl.SSLContext.load_verify_locations") - async def test_uses_default_ca_certs(self, load_verify_locations: Any) -> None: + @patch("ssl.SSLContext", return_value=MagicMock()) + async def test_uses_default_ca_certs(self, ssl_context: Any) -> None: + ssl_context.return_value.load_verify_locations.return_value = None AIOHttpConnection(use_ssl=True) - load_verify_locations.assert_called_once_with( + ssl_context.return_value.load_verify_locations.assert_called_once_with( cafile=Connection.default_ca_certs() ) - @patch("ssl.SSLContext.load_verify_locations") - async def test_uses_no_ca_certs(self, load_verify_locations: Any) -> None: + @patch("ssl.SSLContext", return_value=MagicMock()) + async def test_uses_no_ca_certs(self, ssl_context: Any) -> None: + ssl_context.return_value.load_verify_locations.return_value = None AIOHttpConnection(use_ssl=True, verify_certs=False) - load_verify_locations.assert_not_called() + ssl_context.return_value.load_verify_locations.assert_not_called() async def test_trust_env(self) -> None: con: Any = AIOHttpConnection(trust_env=True) diff --git a/test_opensearchpy/test_async/test_transport.py b/test_opensearchpy/test_async/test_transport.py index 4ef80707..b494f83f 100644 --- a/test_opensearchpy/test_async/test_transport.py +++ b/test_opensearchpy/test_async/test_transport.py @@ -272,7 +272,7 @@ async def test_add_connection(self) -> None: async def test_request_will_fail_after_X_retries(self) -> None: t: Any = AsyncTransport( - [{"exception": ConnectionError("abandon ship")}], + [{"exception": ConnectionError(None, "abandon ship", Exception())}], connection_class=DummyConnection, ) @@ -287,7 +287,7 @@ async def test_request_will_fail_after_X_retries(self) -> None: async def test_failed_connection_will_be_marked_as_dead(self) -> None: t: Any = AsyncTransport( - [{"exception": ConnectionError("abandon ship")}] * 2, + [{"exception": ConnectionError(None, "abandon ship", Exception())}] * 2, connection_class=DummyConnection, ) @@ -381,7 +381,10 @@ async def test_sniff_reuses_connection_instances_if_possible(self) -> None: async def test_sniff_on_fail_triggers_sniffing_on_fail(self) -> None: t: Any = AsyncTransport( - [{"exception": ConnectionError("abandon ship")}, {"data": CLUSTER_NODES}], + [ + {"exception": ConnectionError(None, "abandon ship", Exception())}, + {"data": CLUSTER_NODES}, + ], connection_class=DummyConnection, sniff_on_connection_fail=True, max_retries=0, @@ -407,7 +410,10 @@ async def test_sniff_on_fail_failing_does_not_prevent_retires( ) -> None: sniff_hosts.side_effect = [TransportError("sniff failed")] t: Any = AsyncTransport( - [{"exception": ConnectionError("abandon ship")}, {"data": CLUSTER_NODES}], + [ + {"exception": ConnectionError(None, "abandon ship", Exception())}, + {"data": CLUSTER_NODES}, + ], connection_class=DummyConnection, sniff_on_connection_fail=True, max_retries=3, diff --git a/test_opensearchpy/test_transport.py b/test_opensearchpy/test_transport.py index dc1a8f9e..4b37e3ac 100644 --- a/test_opensearchpy/test_transport.py +++ b/test_opensearchpy/test_transport.py @@ -266,7 +266,7 @@ def test_add_connection(self) -> None: def test_request_will_fail_after_X_retries(self) -> None: t: Any = Transport( - [{"exception": ConnectionError("abandon ship")}], + [{"exception": ConnectionError(None, "abandon ship", Exception())}], connection_class=DummyConnection, ) @@ -275,7 +275,7 @@ def test_request_will_fail_after_X_retries(self) -> None: def test_failed_connection_will_be_marked_as_dead(self) -> None: t: Any = Transport( - [{"exception": ConnectionError("abandon ship")}] * 2, + [{"exception": ConnectionError(None, "abandon ship", Exception())}] * 2, connection_class=DummyConnection, ) @@ -349,7 +349,10 @@ def test_sniff_reuses_connection_instances_if_possible(self) -> None: def test_sniff_on_fail_triggers_sniffing_on_fail(self) -> None: t: Any = Transport( - [{"exception": ConnectionError("abandon ship")}, {"data": CLUSTER_NODES}], + [ + {"exception": ConnectionError(None, "abandon ship", Exception())}, + {"data": CLUSTER_NODES}, + ], connection_class=DummyConnection, sniff_on_connection_fail=True, max_retries=0, @@ -366,7 +369,10 @@ def test_sniff_on_fail_failing_does_not_prevent_retires( ) -> None: sniff_hosts.side_effect = [TransportError("sniff failed")] t: Any = Transport( - [{"exception": ConnectionError("abandon ship")}, {"data": CLUSTER_NODES}], + [ + {"exception": ConnectionError(None, "abandon ship", Exception())}, + {"data": CLUSTER_NODES}, + ], connection_class=DummyConnection, sniff_on_connection_fail=True, max_retries=3,