Check response status in OpenSearchDistribution.request() (#210)

## 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 <[email protected]>

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)
+    ]

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)

lib/charms/opensearch/v0/opensearch_distro.py

@@ -20,19 +20,22 @@
 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,
     OpenSearchHttpError,
     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,35 +242,42 @@ 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")
 
         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."""

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):

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: