From b4cf6a423bdb69e159ff46f0e4ac29a37966cfd1 Mon Sep 17 00:00:00 2001 From: Carl Csaposs Date: Fri, 5 Apr 2024 11:37:46 +0000 Subject: [PATCH] Check response status in `OpenSearchDistribution.request()` (#210) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Issue By default, `requests` does not raise an exception if an HTTP request errors (i.e. response status code 400-599) Currently, `request()` is expecting an exception to be raised if request errors For example, this usage of `request()` expects an `OpenSearchHttpError` if the status code is 400—but an exception will not be raised if the status code is 400 https://github.com/canonical/opensearch-operator/blob/c0225203320a412c3492ac827ac47b3b5b763787/lib/charms/opensearch/v0/opensearch_locking.py#L53-L68 Also, currently, requests with status code 400-599 are never retried, regardless of the value of `retries` argument ## Solution Calling `raise_for_status()` will raise a `requests.exceptions.HTTPError` exception if the response status code is 400-599 --------- Co-authored-by: Mehdi-Bendriss --- lib/charms/opensearch/v0/helper_http.py | 50 ++++++++ .../opensearch/v0/opensearch_base_charm.py | 1 - lib/charms/opensearch/v0/opensearch_distro.py | 113 +++++++----------- .../opensearch/v0/opensearch_exceptions.py | 8 +- .../v0/opensearch_relation_provider.py | 2 +- 5 files changed, 100 insertions(+), 74 deletions(-) create mode 100644 lib/charms/opensearch/v0/helper_http.py diff --git a/lib/charms/opensearch/v0/helper_http.py b/lib/charms/opensearch/v0/helper_http.py new file mode 100644 index 000000000..62bc807fe --- /dev/null +++ b/lib/charms/opensearch/v0/helper_http.py @@ -0,0 +1,50 @@ +# Copyright 2023 Canonical Ltd. +# See LICENSE file for licensing details. + +"""File containing http related helpers.""" +from typing import Any, Dict, List, Optional + +from charms.opensearch.v0.helper_networking import reachable_hosts +from tenacity import RetryCallState + +# The unique Charmhub library identifier, never change it +LIBID = "f8bb15ca9ffc4e3f8d113421e03abe06" + +# Increment this major API version when introducing breaking changes +LIBAPI = 0 + +# Increment this PATCH version before using `charmcraft publish-lib` or reset +# to 0 if you are raising the major API version +LIBPATCH = 1 + + +def error_http_retry_log( + logger, retry_max: int, method: str, url: str, payload: Optional[Dict[str, Any]] +): + """Return a custom log function to run before a new Tenacity retry.""" + + def log_error(retry_state: RetryCallState): + logger.error( + f"Request {method} to {url} with payload: {payload} failed." + f"(Attempts left: {retry_max - retry_state.attempt_number})\n" + f"\tError: {retry_state.outcome.exception()}" + ) + + return log_error + + +def full_urls( + primary_host: str, port: int, path: str, alt_hosts: List[str], check_hosts_reach: bool = False +) -> List[str]: + """Returns a list of well formatted and potentially reachable hosts.""" + target_hosts = [primary_host] + if alt_hosts: + target_hosts.extend([alt_host for alt_host in alt_hosts if alt_host != primary_host]) + + if not check_hosts_reach: + return target_hosts + + return [ + f"https://{host_candidate}:{port}/{path}" + for host_candidate in reachable_hosts(target_hosts) + ] diff --git a/lib/charms/opensearch/v0/opensearch_base_charm.py b/lib/charms/opensearch/v0/opensearch_base_charm.py index 62e822b50..0df87328d 100644 --- a/lib/charms/opensearch/v0/opensearch_base_charm.py +++ b/lib/charms/opensearch/v0/opensearch_base_charm.py @@ -738,7 +738,6 @@ def _start_opensearch(self, event: EventBase) -> None: # noqa: C901 except OpenSearchHttpError: self.peers_data.delete(Scope.UNIT, "starting") event.defer() - self._post_start_init(event) return except OpenSearchProvidedRolesException as e: logger.exception(e) diff --git a/lib/charms/opensearch/v0/opensearch_distro.py b/lib/charms/opensearch/v0/opensearch_distro.py index 02310e150..b2f1607b9 100644 --- a/lib/charms/opensearch/v0/opensearch_distro.py +++ b/lib/charms/opensearch/v0/opensearch_distro.py @@ -20,11 +20,8 @@ from charms.opensearch.v0.constants_secrets import ADMIN_PW from charms.opensearch.v0.helper_cluster import Node from charms.opensearch.v0.helper_conf_setter import YamlConfigSetter -from charms.opensearch.v0.helper_networking import ( - get_host_ip, - is_reachable, - reachable_hosts, -) +from charms.opensearch.v0.helper_http import error_http_retry_log, full_urls +from charms.opensearch.v0.helper_networking import get_host_ip, is_reachable from charms.opensearch.v0.opensearch_exceptions import ( OpenSearchCmdError, OpenSearchError, @@ -32,7 +29,13 @@ OpenSearchStartTimeoutError, ) from charms.opensearch.v0.opensearch_internal_data import Scope -from tenacity import retry, stop_after_attempt, wait_fixed +from tenacity import ( + Retrying, + retry, + retry_if_exception_type, + stop_after_attempt, + wait_fixed, +) # The unique Charmhub library identifier, never change it LIBID = "7145c219467d43beb9c566ab4a72c454" @@ -210,53 +213,18 @@ def request( # noqa OpenSearchHttpError if hosts are unreachable """ - def full_urls() -> List[str]: - """Returns a list of reachable hosts.""" - primary_host = host or self.host - target_hosts = [primary_host] - if alt_hosts: - target_hosts.extend( - [alt_host for alt_host in alt_hosts if alt_host != primary_host] - ) - - if not check_hosts_reach: - return target_hosts - - return [ - f"https://{host_candidate}:{self.port}/{endpoint}" - for host_candidate in reachable_hosts(target_hosts) - ] - - def call( - remaining_retries: int, - return_failed_resp: bool, - error_response: Optional[requests.Response] = None, - ) -> requests.Response: + def call(url: str) -> requests.Response: """Performs an HTTP request.""" - if remaining_retries < 0: - if error_response is None: - raise OpenSearchHttpError() - - if return_failed_resp: - return error_response - - raise OpenSearchHttpError( - response_body=error_response.text, response_code=error_response.status_code - ) - - urls = full_urls() - if not urls: - logger.error( - f"Host {host or self.host}:{self.port} and alternative_hosts: {alt_hosts or []} not reachable." - ) - raise OpenSearchHttpError() - - try: - with requests.Session() as s: - s.auth = ( - "admin", - self._charm.secrets.get(Scope.APP, ADMIN_PW), - ) + for attempt in Retrying( + retry=retry_if_exception_type(requests.RequestException) + | retry_if_exception_type(urllib3.exceptions.HTTPError), + stop=stop_after_attempt(retries), + wait=wait_fixed(1), + before_sleep=error_http_retry_log(logger, retries, method, url, payload), + reraise=True, + ): + with attempt, requests.Session() as s: + s.auth = ("admin", self._charm.secrets.get(Scope.APP, ADMIN_PW)) request_kwargs = { "method": method.upper(), @@ -274,19 +242,8 @@ def call( ) response = s.request(**request_kwargs) - + response.raise_for_status() return response - except (requests.exceptions.RequestException, urllib3.exceptions.HTTPError) as e: - logger.error( - f"Request {method} to {urls[0]} with payload: {payload} failed. " - f"(Attempts left: {remaining_retries})\n{e}" - ) - time.sleep(1) - return call( - remaining_retries - 1, - return_failed_resp, - e.response if isinstance(e, requests.exceptions.HTTPError) else None, - ) if None in [endpoint, method]: raise ValueError("endpoint or method missing") @@ -294,15 +251,33 @@ def call( if endpoint.startswith("/"): endpoint = endpoint[1:] - resp = call(retries, resp_status_code) - - if resp_status_code: - return resp.status_code + urls = full_urls(host or self.host, self.port, endpoint, alt_hosts, check_hosts_reach) + if not urls: + raise OpenSearchHttpError( + f"Host {host or self.host}:{self.port} and alternative_hosts: {alt_hosts or []} not reachable." + ) + resp = None try: + resp = call(urls[0]) + if resp_status_code: + return resp.status_code + return resp.json() + except (requests.RequestException, urllib3.exceptions.HTTPError) as e: + if not isinstance(e, requests.RequestException) or e.response is None: + raise OpenSearchHttpError(response_text=str(e)) + + if resp_status_code: + return e.response.status_code + + raise OpenSearchHttpError( + response_text=e.response.text, response_code=e.response.status_code + ) except requests.JSONDecodeError: - raise OpenSearchHttpError(response_body=resp.text) + raise OpenSearchHttpError(response_text=resp.text) + except Exception as e: + raise OpenSearchHttpError(response_text=str(e)) def write_file(self, path: str, data: str, override: bool = True): """Persists data into file. Useful for files generated on the fly, such as certs etc.""" diff --git a/lib/charms/opensearch/v0/opensearch_exceptions.py b/lib/charms/opensearch/v0/opensearch_exceptions.py index ac52c7595..531126b36 100644 --- a/lib/charms/opensearch/v0/opensearch_exceptions.py +++ b/lib/charms/opensearch/v0/opensearch_exceptions.py @@ -74,12 +74,14 @@ class OpenSearchCmdError(OpenSearchError): class OpenSearchHttpError(OpenSearchError): """Exception thrown when an OpenSearch REST call fails.""" - def __init__(self, response_body: Optional[str] = None, response_code: Optional[int] = None): + def __init__(self, response_text: Optional[str] = None, response_code: Optional[int] = None): try: - self.response_body = json.loads(response_body) + self.response_text = response_text + self.response_body = json.loads(response_text) except (json.JSONDecodeError, TypeError): self.response_body = {} - self.response_code = response_code + finally: + self.response_code = response_code class OpenSearchHAError(OpenSearchError): diff --git a/lib/charms/opensearch/v0/opensearch_relation_provider.py b/lib/charms/opensearch/v0/opensearch_relation_provider.py index 6fbbbf068..873eeef75 100644 --- a/lib/charms/opensearch/v0/opensearch_relation_provider.py +++ b/lib/charms/opensearch/v0/opensearch_relation_provider.py @@ -374,7 +374,7 @@ def update_endpoints(self, relation: Relation, omit_endpoints: Optional[Set[str] """Updates endpoints in the databag for the given relation.""" # we can only set endpoints if we're the leader, and we can only get endpoints if the node # is running. - if not self.unit.is_leader() or not self.opensearch.is_node_up(): + if not self.unit.is_leader() or not self.opensearch.is_node_up() or not relation.app: return if not omit_endpoints: