From a53ab52acd13d9568f54a579731d068b79bd3bba Mon Sep 17 00:00:00 2001 From: jeanluc Date: Fri, 17 May 2024 13:18:34 +0200 Subject: [PATCH] Support retries and connection settings --- changelog/65.added.md | 1 + docs/ref/configuration.md | 147 ++++++- src/saltext/vault/runners/vault.py | 1 + src/saltext/vault/utils/vault/__init__.py | 10 +- src/saltext/vault/utils/vault/client.py | 265 ++++++++++-- src/saltext/vault/utils/vault/exceptions.py | 6 + src/saltext/vault/utils/vault/factory.py | 55 ++- tests/integration/runners/test_vault.py | 59 ++- tests/unit/runners/vault/test_vault.py | 14 + tests/unit/utils/vault/conftest.py | 24 ++ tests/unit/utils/vault/test_client.py | 428 +++++++++++++++++++- tests/unit/utils/vault/test_factory.py | 41 +- 12 files changed, 980 insertions(+), 71 deletions(-) create mode 100644 changelog/65.added.md diff --git a/changelog/65.added.md b/changelog/65.added.md new file mode 100644 index 00000000..8d092856 --- /dev/null +++ b/changelog/65.added.md @@ -0,0 +1 @@ +Added support for retry logic and specific connection settings in `vault:client` diff --git a/docs/ref/configuration.md b/docs/ref/configuration.md index 1fa0ad00..2fbe9d3e 100644 --- a/docs/ref/configuration.md +++ b/docs/ref/configuration.md @@ -184,6 +184,133 @@ The time in seconds to cache tokens/SecretIDs for. Defaults to `ttl`, which caches the secret for as long as it is valid, unless a new configuration is requested from the master. +:::{vconf} client +::: +### `client` +:::{versionadded} 1.1.0 +::: + +Configures Vault API client behavior. By default, +the client retries requests with a backoff strategy, +unless the response includes a `Retry-After` header, which is respected. +Connection errors as well as responses with the status codes +`412`, `429`, `500`, `502`, `503`, `504` are retried. + +:::{vconf} client:connect_timeout +::: +#### connect_timeout +:::{versionadded} 1.1.0 +::: +The number of seconds to wait for a connection to be established. +Defaults to `9.2`. + +:::{vconf} client:read_timeout +::: +#### read_timeout +:::{versionadded} 1.1.0 +::: +The number of seconds to wait between packets sent by the server. +Defaults to `30`. + +:::{vconf} client:max_retries +::: +#### max_retries +:::{versionadded} 1.1.0 +::: +The maximum number of retries (not including the initial request) before +raising an exception. Set this to `0` to disable retry behavior. +Defaults to `5`. Maximum: `10`. + +:::{vconf} client:backoff_factor +::: +#### backoff_factor +:::{versionadded} 1.1.0 +::: +A backoff factor (in seconds) to use between retry attempts when applying +the backoff strategy (based on the Fibonacci sequence). +Defaults to `0.1`. Maximum: `3.0` + +:::{hint} +The effective sleep time before the nth retry is given by: + + > {backoff_factor} * {Fibonacci(n+3)} + +The default values thus result in the following sleep times (in seconds), +without accounting for {vconf}`backoff_jitter ` +and only if the response did not include the `Retry-After` header: + + > [initial request] 0.2 [1st] 0.3 [2nd] 0.5 [3rd] 0.8 [4th] 1.3 [5th] + +If we did not receive a response (connection/read error), the first retry +is executed immediately, thus the following sleep times are in effect by default: + + > [initial request] 0 [1st] 0.2 [2nd] 0.3 [3rd] 0.5 [4th] 0.8 [5th] + +::: + +:::{vconf} client:backoff_max +::: +#### backoff_max +:::{versionadded} 1.1.0 +::: +A cap for the effective sleep time between retries. +Defaults to `10.0`. Maximum: `60.0`. + +:::{vconf} client:backoff_jitter +::: +#### backoff_jitter +:::{versionadded} 1.1.0 +::: +A maximum number of seconds to randomize the effective sleep time +between retries by. Defaults to `0.2`. Maximum: `5.0` + +:::{vconf} client:retry_post +::: +#### retry_post +:::{versionadded} 1.1.0 +::: +Whether to retry requests that are potentially non-idempotent (`POST`, `PATCH`). Defaults to `False`. + +:::{note} +HTTP 429 responses are always retried, regardless of HTTP verb. +::: + +:::{vconf} client:retry_status +::: +#### retry_status +:::{versionadded} 1.1.0 +::: +A list of HTTP status codes which should be retried. +Defaults to `[412, 500, 502, 503, 504]`. + +:::{note} +HTTP 429 is always retried, regardless of HTTP verb and whether it is present +in this list. It is recommended to ensure the `Retry-After` header is sent by Vault to optimize the spent resources. +See {vconf}`respect_retry_after ` for details. +::: + +:::{vconf} client:respect_retry_after +::: +#### respect_retry_after +:::{versionadded} 1.1.0 +::: +Whether to respect the `Retry-After` header sent by Vault, usually when a +rate limit has been hit. Defaults to `True`. + +:::{hint} +This header is not sent by default and must be enabled explicitly +via [enable_rate_limit_response_headers](https://developer.hashicorp.com/vault/api-docs/system/quotas-config#enable_rate_limit_response_headers). +::: + +:::{vconf} client:retry_after_max +::: +#### retry_after_max +:::{versionadded} 1.1.0 +::: +When {vconf}`respect_retry_after ` is True, limit +the maximum amount of seconds the client will sleep before retrying. Set this to `null` (YAML/JSON)/`None` (Python) +to disable this behavior. Defaults to `60`. + :::{vconf} server ::: ### `server` @@ -390,8 +517,8 @@ all Vault modules will be broken to prevent an infinite loop. ## Minion-only configuration :::{note} -In addition to the following minion-only values, {vconf}`auth:token_lifecycle` and {vconf}`server:verify` -can be set on the minion as well, even if it pulls its configuration from a master. +In addition to the following minion-only values, {vconf}`auth:token_lifecycle`, {vconf}`server:verify` +and {vconf}`client` can be set on the minion as well, even if it pulls its configuration from a master. ::: :::{vconf} config_location @@ -439,6 +566,22 @@ vault: config: 3600 kv_metadata: connection secret: ttl + client: + max_retries: 5 + connect_timeout: 9.2 + read_timeout: 30 + backoff_factor: 0.1 + backoff_max: 10 + backoff_jitter: 0.2 + retry_post: false + retry_status: + - 412 + - 500 + - 502 + - 503 + - 504 + respect_retry_after: true + retry_after_max: 60 config_location: issue: allow_minion_override_params: false diff --git a/src/saltext/vault/runners/vault.py b/src/saltext/vault/runners/vault.py index 17de2b88..0d1148f7 100644 --- a/src/saltext/vault/runners/vault.py +++ b/src/saltext/vault/runners/vault.py @@ -324,6 +324,7 @@ def get_config( "token_lifecycle": _config("auth:token_lifecycle"), }, "cache": _config("cache"), + "client": _config("client"), "server": _config("server"), "wrap_info_nested": [], } diff --git a/src/saltext/vault/utils/vault/__init__.py b/src/saltext/vault/utils/vault/__init__.py index 3eecc60f..b5f779b2 100644 --- a/src/saltext/vault/utils/vault/__init__.py +++ b/src/saltext/vault/utils/vault/__init__.py @@ -4,15 +4,6 @@ import logging -import salt.cache -import salt.crypt -import salt.exceptions -import salt.utils.data -import salt.utils.dictupdate -import salt.utils.json -import salt.utils.versions - -import saltext.vault.utils.vault.helpers as hlp from saltext.vault.utils.vault.auth import InvalidVaultSecretId from saltext.vault.utils.vault.auth import InvalidVaultToken from saltext.vault.utils.vault.auth import LocalVaultSecretId @@ -24,6 +15,7 @@ from saltext.vault.utils.vault.exceptions import VaultNotFoundError from saltext.vault.utils.vault.exceptions import VaultPermissionDeniedError from saltext.vault.utils.vault.exceptions import VaultPreconditionFailedError +from saltext.vault.utils.vault.exceptions import VaultRateLimitExceededError from saltext.vault.utils.vault.exceptions import VaultServerError from saltext.vault.utils.vault.exceptions import VaultUnavailableError from saltext.vault.utils.vault.exceptions import VaultUnsupportedOperationError diff --git a/src/saltext/vault/utils/vault/client.py b/src/saltext/vault/utils/vault/client.py index 5219f7fe..d0f89a02 100644 --- a/src/saltext/vault/utils/vault/client.py +++ b/src/saltext/vault/utils/vault/client.py @@ -3,11 +3,14 @@ """ import logging +import random import re +from itertools import takewhile import requests import salt.exceptions -from requests.packages.urllib3.util.ssl_ import create_urllib3_context +from requests.adapters import HTTPAdapter +from requests.adapters import Retry import saltext.vault.utils.vault.leases as leases from saltext.vault.utils.vault.exceptions import VaultAuthExpired @@ -15,11 +18,23 @@ from saltext.vault.utils.vault.exceptions import VaultNotFoundError from saltext.vault.utils.vault.exceptions import VaultPermissionDeniedError from saltext.vault.utils.vault.exceptions import VaultPreconditionFailedError +from saltext.vault.utils.vault.exceptions import VaultRateLimitExceededError from saltext.vault.utils.vault.exceptions import VaultServerError from saltext.vault.utils.vault.exceptions import VaultUnavailableError from saltext.vault.utils.vault.exceptions import VaultUnsupportedOperationError from saltext.vault.utils.vault.exceptions import VaultUnwrapException +try: + from urllib3.util import create_urllib3_context + + URLLIB3V1 = False +except ImportError: + # urllib <2 + from urllib3.util.ssl_ import create_urllib3_context + + URLLIB3V1 = True + + log = logging.getLogger(__name__) logging.getLogger("requests").setLevel(logging.WARNING) @@ -33,6 +48,30 @@ "sys/health", ) +HTTP_TOO_MANY_REQUESTS = 429 + +# Default timeout configuration +DEFAULT_CONNECT_TIMEOUT = 9.2 +DEFAULT_READ_TIMEOUT = 30 + +# Default retry configuration +DEFAULT_MAX_RETRIES = 5 +DEFAULT_BACKOFF_FACTOR = 0.1 +DEFAULT_BACKOFF_MAX = 10.0 +DEFAULT_BACKOFF_JITTER = 0.2 +DEFAULT_RETRY_POST = False +DEFAULT_RESPECT_RETRY_AFTER = True +DEFAULT_RETRY_AFTER_MAX = 60 +# https://developer.hashicorp.com/vault/api-docs#http-status-codes +# 412: eventually consistent data is still missing (Enterprise) +DEFAULT_RETRY_STATUS = (412, 500, 502, 503, 504) + +# Caps for retry configuration +MAX_MAX_RETRIES = 10 +MAX_BACKOFF_FACTOR = 3.0 +MAX_BACKOFF_MAX = 60.0 +MAX_BACKOFF_JITTER = 5.0 + def _get_expected_creation_path(secret_type, config=None): if secret_type == "token": @@ -65,27 +104,71 @@ class VaultClient: Base class for authenticated client. """ - def __init__(self, url, namespace=None, verify=None, session=None): + def __init__( + self, + url, + namespace=None, + verify=None, + session=None, + connect_timeout=DEFAULT_CONNECT_TIMEOUT, + read_timeout=DEFAULT_READ_TIMEOUT, + max_retries=DEFAULT_MAX_RETRIES, + backoff_factor=DEFAULT_BACKOFF_FACTOR, + backoff_max=DEFAULT_BACKOFF_MAX, + backoff_jitter=DEFAULT_BACKOFF_JITTER, + retry_post=DEFAULT_RETRY_POST, + respect_retry_after=DEFAULT_RESPECT_RETRY_AFTER, + retry_status=DEFAULT_RETRY_STATUS, + retry_after_max=DEFAULT_RETRY_AFTER_MAX, + ): self.url = url self.namespace = namespace self.verify = verify - ca_cert = None - try: - if verify.startswith("-----BEGIN CERTIFICATE"): - ca_cert = verify - verify = None - except AttributeError: - pass + self.connect_timeout = connect_timeout + self.read_timeout = read_timeout + + # Cap the retry-backoff values somewhat + self.max_retries = max(0, min(max_retries, MAX_MAX_RETRIES)) + self.backoff_factor = max(0, min(backoff_factor, MAX_BACKOFF_FACTOR)) + self.backoff_max = max(0, min(backoff_max, MAX_BACKOFF_MAX)) + self.backoff_jitter = max(0, min(backoff_jitter, MAX_BACKOFF_JITTER)) + self.retry_post = bool(retry_post) + self.respect_retry_after = bool(respect_retry_after) + self.retry_after_max = max(0, retry_after_max) if retry_after_max is not None else None + self.retry_status = tuple(retry_status) if retry_status is not None else None + + retry = VaultRetry( + total=self.max_retries, + backoff_factor=self.backoff_factor, + backoff_max=self.backoff_max, + backoff_jitter=self.backoff_jitter, + respect_retry_after_header=self.respect_retry_after, + retry_after_max=self.retry_after_max, + allowed_methods=None if retry_post else Retry.DEFAULT_ALLOWED_METHODS, + raise_on_status=False, + status_forcelist=self.retry_status, + ) - # Keep the actual requests parameter separate from the client config - # to reduce complexity in config validation. - self._requests_verify = verify if session is None: session = requests.Session() - if ca_cert: - adapter = CACertHTTPSAdapter(ca_cert) - session.mount(url, adapter) + adapter = VaultAPIAdapter( + max_retries=retry, + verify=verify, + connect_timeout=self.connect_timeout, + read_timeout=self.read_timeout, + ) + session.mount(url, adapter) + else: + # Sessions should only be inherited from other instances + # of this class. A changed ``verify`` setting causes a fresh + # client to be instantiated. + # We want to keep the TCP connection alive, so we'll modify + # the adapter in place. + adapter = session.get_adapter(url) + adapter.max_retries = retry + adapter.connect_timeout = self.connect_timeout + adapter.read_timeout = self.read_timeout self.session = session def delete(self, endpoint, wrap=False, raise_error=True, add_headers=None): @@ -197,7 +280,6 @@ def request_raw(self, method, endpoint, payload=None, wrap=False, add_headers=No url, headers=headers, json=payload, - verify=self._requests_verify, **kwargs, ) return res @@ -242,9 +324,7 @@ def unwrap(self, wrapped, expected_creation_path=None): headers["X-Vault-Token"] = str(wrapped) else: payload["token"] = str(wrapped) - res = self.session.request( - "POST", url, headers=headers, json=payload, verify=self._requests_verify - ) + res = self.session.request("POST", url, headers=headers, json=payload) if not res.ok: self._raise_status(res) return res.json() @@ -328,6 +408,8 @@ def _raise_status(self, res): raise VaultUnsupportedOperationError(errors) if res.status_code == 412: raise VaultPreconditionFailedError(errors) + if res.status_code == HTTP_TOO_MANY_REQUESTS: + raise VaultRateLimitExceededError(errors) if res.status_code in (500, 502): raise VaultServerError(errors) if res.status_code == 503: @@ -499,14 +581,28 @@ def _get_headers(self, wrap=False): return headers -class CACertHTTPSAdapter(requests.sessions.HTTPAdapter): +class VaultAPIAdapter(HTTPAdapter): """ - Allows to restrict requests CA chain validation - to a single root certificate without writing it to disk. + An adapter that + + * allows to restrict requests CA chain validation to a single + root certificate without writing it to disk. + * sets default values for timeout settings without having to + specify it in every request. """ - def __init__(self, ca_cert_data, *args, **kwargs): + def __init__(self, *args, verify=None, connect_timeout=None, read_timeout=None, **kwargs): + ca_cert_data = None + try: + if verify.strip().startswith("-----BEGIN CERTIFICATE"): + ca_cert_data = verify + verify = None + except AttributeError: + pass self.ca_cert_data = ca_cert_data + self.verify = verify + self.connect_timeout = connect_timeout or DEFAULT_CONNECT_TIMEOUT + self.read_timeout = read_timeout or DEFAULT_READ_TIMEOUT super().__init__(*args, **kwargs) def init_poolmanager( @@ -516,7 +612,124 @@ def init_poolmanager( block=requests.adapters.DEFAULT_POOLBLOCK, **pool_kwargs, ): - ssl_context = create_urllib3_context() - ssl_context.load_verify_locations(cadata=self.ca_cert_data) - pool_kwargs["ssl_context"] = ssl_context + if self.ca_cert_data is not None: + ssl_context = create_urllib3_context() + ssl_context.load_verify_locations(cadata=self.ca_cert_data) + pool_kwargs["ssl_context"] = ssl_context return super().init_poolmanager(connections, maxsize, block=block, **pool_kwargs) + + def send(self, request, stream=False, timeout=None, verify=True, cert=None, proxies=None): + """ + Wrap sending the request to ensure ``verify`` and ``timeout`` is set + as specified on every request. ``timeout`` can be overridden per request. + """ + if self.verify is not None: + verify = self.verify + if timeout is None: + timeout = (self.connect_timeout, self.read_timeout) + return super().send( + request, stream=stream, timeout=timeout, verify=verify, cert=cert, proxies=proxies + ) + + +class VaultRetry(Retry): + """ + The Vault API responds with HTTP 429 when rate limits have been hit. + We want to always retry 429, regardless of the HTTP verb and the presence + of the ``Retry-After`` header, thus we need to subclass the retry configuration class. + For HTTP error responses, we do not want to retry immediately if the header was not set. + + We override the default exponential power-of-2 algorithm for calculating + the backoff time with a Fibonacci one because we expect a relatively + quick turnaround. + """ + + PHI = 1.618 + SQRT5 = 2.236 + + def __init__( + self, + *args, + backoff_jitter=0.0, + backoff_max=Retry.DEFAULT_BACKOFF_MAX, + retry_after_max=DEFAULT_RETRY_AFTER_MAX, + **kwargs, + ): + """ + For ``urllib3<2``, backport ``backoff_max`` and ``backoff_jitter``. + Also, allow limiting the value returned by ``Retry-After`` by + specifying ``retry_after_max``. + """ + if URLLIB3V1: + self.backoff_max = backoff_max + self.backoff_jitter = backoff_jitter + else: + kwargs["backoff_max"] = backoff_max + kwargs["backoff_jitter"] = backoff_jitter + self.retry_after_max = retry_after_max + super().__init__(*args, **kwargs) + + def is_retry(self, method, status_code, has_retry_after=False): + """ + HTTP 429 is always retryable (even for POST/PATCH), otherwise fall back + to the configuration. + """ + if status_code == HTTP_TOO_MANY_REQUESTS: + return True + return super().is_retry(method, status_code, has_retry_after=has_retry_after) + + def get_backoff_time(self): + """ + When we're retrying HTTP error responses, ensure we don't execute the + first retry immediately. + Also overrides the default 2**n algorithm with one based on the Fibonacci sequence. + On ``urllib3<2``, this also backports ``backoff_jitter`` and ``backoff_max``. + """ + # We want to consider only the last consecutive errors sequence (Ignore redirects). + consecutive_errors = list( + takewhile(lambda x: x.redirect_location is None, reversed(self.history)) + ) + consecutive_errors_len = len(consecutive_errors) + if consecutive_errors_len and consecutive_errors[0].status is not None: + # Ensure we only immediately retry for local (connection/read) errors, + # not when we got an HTTP response. + consecutive_errors_len += 1 + if consecutive_errors_len <= 1: + return 0 + # Approximate the nth Fibonacci number. + # We want to begin with the 4th one (2). + backoff_value = round( + self.backoff_factor * round(self.PHI ** (consecutive_errors_len + 1) / self.SQRT5), + 1, + ) + if self.backoff_jitter != 0.0: + backoff_value += random.random() * self.backoff_jitter + return float(max(0, min(self.backoff_max, backoff_value))) + + def get_retry_after(self, response): + """ + The default implementation sleeps for as long as requested + by the ``Retry-After`` header. We want to limit that somewhat + to avoid sleeping until the end of the universe. + """ + retry_after = response.headers.get("Retry-After") + + if retry_after is None: + return None + + res = self.parse_retry_after(retry_after) + if self.retry_after_max is None: + return res + return min(res, self.retry_after_max) + + def new(self, **kw): + """ + Since we backport some params and introduce a new one, + ensure all requests use the defined parameters, not the default ones. + """ + ret = super().new(**kw) + if URLLIB3V1: + ret.backoff_jitter = self.backoff_jitter + ret.backoff_max = self.backoff_max + ret.retry_after_max = self.retry_after_max + return ret diff --git a/src/saltext/vault/utils/vault/exceptions.py b/src/saltext/vault/utils/vault/exceptions.py index 93953527..38eed853 100644 --- a/src/saltext/vault/utils/vault/exceptions.py +++ b/src/saltext/vault/utils/vault/exceptions.py @@ -95,6 +95,12 @@ class VaultPreconditionFailedError(VaultException): """ +class VaultRateLimitExceededError(VaultException): + """ + HTTP 429 + """ + + class VaultServerError(VaultException): """ HTTP 500 diff --git a/src/saltext/vault/utils/vault/factory.py b/src/saltext/vault/utils/vault/factory.py index 146c5bbb..9978efb0 100644 --- a/src/saltext/vault/utils/vault/factory.py +++ b/src/saltext/vault/utils/vault/factory.py @@ -14,8 +14,6 @@ import salt.utils.context import salt.utils.data import salt.utils.dictupdate -import salt.utils.json -import salt.utils.versions from requests.exceptions import ConnectionError from salt.defaults import NOT_SET @@ -345,7 +343,7 @@ def _build_authd_client(opts, context, force_local=False): token_store=token_auth, ) client = vclient.AuthenticatedVaultClient( - auth, session=unauthd_client.session, **config["server"] + auth, session=unauthd_client.session, **config["server"], **config["client"] ) elif config["auth"]["method"] in ("token", "wrapped_token"): token = _fetch_token( @@ -358,7 +356,7 @@ def _build_authd_client(opts, context, force_local=False): ) auth = vauth.VaultTokenAuth(token=token, cache=token_cache) client = vclient.AuthenticatedVaultClient( - auth, session=unauthd_client.session, **config["server"] + auth, session=unauthd_client.session, **config["server"], **config["client"] ) if client is not None: @@ -397,11 +395,28 @@ def _build_revocation_client(opts, context, force_local=False): return None, None auth = vauth.VaultTokenAuth(token=token, cache=token_cache) client = vclient.AuthenticatedVaultClient( - auth, session=unauthd_client.session, **config["server"] + auth, session=unauthd_client.session, **config["server"], **config["client"] ) return client, config +def _check_upgrade(config, pre_flush=False): + """ + Check if cached configuration contains all expected keys. + Relevant when new keys are introduced to not break immediately after + an update since the cached config is assumed to have been parsed already. + + pre_flush needs to be passed since we don't want to cause an update + immediately before flushing the cache anyways. + """ + # introduced in v1.1.0 + if "client" not in config: + if not pre_flush: + return True + config["client"] = {} + return False + + def _get_connection_config(cbank, opts, context, force_local=False, pre_flush=False, update=False): if ( hlp._get_salt_run_type(opts) in (hlp.SALT_RUNTYPE_MASTER, hlp.SALT_RUNTYPE_MINION_LOCAL) @@ -422,9 +437,12 @@ def _get_connection_config(cbank, opts, context, force_local=False, pre_flush=Fa # an exception indicating all connection-scoped data should be flushed # if the config is outdated. config = config_cache.get() - if config is not None and not update: - log.debug("Using cached Vault server connection configuration.") - return config, None, vclient.VaultClient(**config["server"]) + if config is not None: + # Check if the cached config is compatible with the current version. + update = update or _check_upgrade(config, pre_flush) + if not update: + log.debug("Using cached Vault server connection configuration.") + return config, None, vclient.VaultClient(**config["server"], **config["client"]) if pre_flush: # used when building a client that revokes leases before clearing cache @@ -460,6 +478,7 @@ def _get_connection_config(cbank, opts, context, force_local=False, pre_flush=Fa new_config = { "auth": new_config["auth"], "cache": new_config["cache"], + "client": new_config["client"], "server": new_config["server"], } if update and config: @@ -479,6 +498,8 @@ def _get_connection_config(cbank, opts, context, force_local=False, pre_flush=Fa config_cache.flush(cbank=False) config_cache.store(new_config) + if unwrap_client is None: + unwrap_client = vclient.VaultClient(**new_config["server"], **new_config["client"]) return new_config, embedded_token, unwrap_client @@ -490,10 +511,11 @@ def _use_local_config(opts): { "auth": config["auth"], "cache": config["cache"], + "client": config["client"], "server": config["server"], }, embedded_token, - vclient.VaultClient(**config["server"]), + vclient.VaultClient(**config["server"], **config["client"]), ) @@ -882,6 +904,18 @@ def parse_config(config, validate=True, opts=None, require_token=True): "kv_metadata": "connection", "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": list(vclient.DEFAULT_RETRY_STATUS), + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "issue": { "allow_minion_override_params": False, "type": "token", @@ -970,6 +1004,9 @@ def parse_config(config, validate=True, opts=None, require_token=True): # same for token_lifecycle if local_config.get("auth", {}).get("token_lifecycle"): merged["auth"]["token_lifecycle"] = local_config["auth"]["token_lifecycle"] + # and client config + if local_config.get("client"): + merged["client"] = {**merged["client"], **local_config["client"]} if not validate: return merged diff --git a/tests/integration/runners/test_vault.py b/tests/integration/runners/test_vault.py index 50d7bb55..dc205068 100644 --- a/tests/integration/runners/test_vault.py +++ b/tests/integration/runners/test_vault.py @@ -8,7 +8,9 @@ from pathlib import Path import pytest +import salt.utils.data import salt.utils.files +import salt.utils.msgpack from saltfactories.utils import random_string from tests.support.vault import vault_delete_secret @@ -693,7 +695,7 @@ def cache_auth_outdated( self, missing_auth_cache, minion_conn_cachedir, vault_port ): # pylint: disable=unused-argument vault_url = f"http://127.0.0.1:{vault_port}" - config_data = b"\xdf\x00\x00\x00\x03\xa4auth\xdf\x00\x00\x00\x04\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa5token\xa9secret_id\xc0\xa5cache\xdf\x00\x00\x00\x03\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6server\xdf\x00\x00\x00\x03\xa9namespace\xc0\xa6verify\xc0\xa3url" + config_data = b"\x84\xa4auth\x84\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa5token\xa9secret_id\xc0\xa5cache\x83\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6client\x86\xabmax_retries\x05\xaebackoff_factor\xcb?\xd3333333\xabbackoff_max\x08\xaebackoff_jitter\xcb?\xf0\x00\x00\x00\x00\x00\x00\xaaretry_post\xc3\xb3respect_retry_after\xc3\xa6server\x83\xa9namespace\xc0\xa6verify\xc0\xa3url" config_data += (len(vault_url) + 160).to_bytes(1, "big") + vault_url.encode() config_cachefile = minion_conn_cachedir / "config.p" with salt.utils.files.fopen(config_cachefile, "wb") as f: @@ -708,7 +710,7 @@ def cache_auth_outdated( def cache_server_outdated( self, missing_auth_cache, minion_conn_cachedir ): # pylint: disable=unused-argument - config_data = b"\xdf\x00\x00\x00\x03\xa4auth\xdf\x00\x00\x00\x05\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa7approle\xa7role_id\xactest-role-id\xa9secret_id\xc3\xa5cache\xdf\x00\x00\x00\x03\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6server\xdf\x00\x00\x00\x03\xa9namespace\xc0\xa6verify\xc0\xa3url\xb2http://127.0.0.1:8" + config_data = b"\x84\xa4auth\x85\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa7approle\xa7role_id\xactest-role-id\xa9secret_id\xc3\xa5cache\x83\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6client\x86\xabmax_retries\x05\xaebackoff_factor\xcb?\xd3333333\xabbackoff_max\x08\xaebackoff_jitter\xcb?\xf0\x00\x00\x00\x00\x00\x00\xaaretry_post\xc3\xb3respect_retry_after\xc3\xa6server\x83\xa9namespace\xc0\xa6verify\xc0\xa3url\xb2http://127.0.0.1:8" config_cachefile = minion_conn_cachedir / "config.p" with salt.utils.files.fopen(config_cachefile, "wb") as f: f.write(config_data) @@ -929,7 +931,7 @@ def cache_auth_outdated( self, missing_auth_cache, minion_conn_cachedir, vault_port ): # pylint: disable=unused-argument vault_url = f"http://127.0.0.1:{vault_port}" - config_data = b"\xdf\x00\x00\x00\x03\xa4auth\xdf\x00\x00\x00\x05\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa7approle\xa7role_id\xactest-role-id\xa9secret_id\xc3\xa5cache\xdf\x00\x00\x00\x03\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6server\xdf\x00\x00\x00\x03\xa9namespace\xc0\xa6verify\xc0\xa3url" + config_data = b"\x84\xa4auth\x85\xadapprole_mount\xa7approle\xacapprole_name\xbavault-approle-int-minion-1\xa6method\xa7approle\xa7role_id\xactest-role-id\xa9secret_id\xc3\xa5cache\x83\xa7backend\xa4disk\xa6config\xcd\x0e\x10\xa6secret\xa3ttl\xa6client\x86\xabmax_retries\x05\xaebackoff_factor\xcb?\xd3333333\xabbackoff_max\x08\xaebackoff_jitter\xcb?\xf0\x00\x00\x00\x00\x00\x00\xaaretry_post\xc3\xb3respect_retry_after\xc3\xa6server\x83\xa9namespace\xc0\xa6verify\xc0\xa3url" config_data += (len(vault_url) + 160).to_bytes(1, "big") + vault_url.encode() config_cachefile = minion_conn_cachedir / "config.p" with salt.utils.files.fopen(config_cachefile, "wb") as f: @@ -940,6 +942,32 @@ def cache_auth_outdated( if config_cachefile.exists(): config_cachefile.unlink() + @pytest.fixture + def cache_from_old_version(self, vault_salt_call_cli, minion_conn_cachedir): + """ + Removes any top-level keys from cached config except + for auth, cache and server. + Added when ``client`` was introduced to the config to + simulate upgrades from old versions. + """ + ret = vault_salt_call_cli.run("vault.read_secret", "secret/path/foo") + assert ret.returncode == 0 + config_cachefile = minion_conn_cachedir / "config.p" + assert config_cachefile.exists() + cached_config = salt.utils.data.decode( + salt.utils.msgpack.loads(config_cachefile.read_bytes()) + ) + old_params = {} + for param in ("auth", "cache", "server"): + old_params[param] = cached_config.pop(param) + config_cachefile.write_bytes(salt.utils.msgpack.dumps(old_params)) + cached_config.update(old_params) + try: + yield cached_config + finally: + if config_cachefile.exists(): + config_cachefile.unlink() + @pytest.fixture(scope="class") def issue_overrides(self): # only explicit_max_ttl and num_uses are respected, the rest is for testing purposes @@ -994,6 +1022,31 @@ def test_auth_method_switch_does_not_break_minion_auth(self, vault_salt_call_cli assert ret.data.get("success") == "yeehaaw" assert "Master returned error and requested cache expiration" in caplog.text + @pytest.mark.parametrize("vault_container_version", ("latest",), indirect=True) + @pytest.mark.usefixtures("cache_from_old_version") + def test_upgrade_does_not_break_auth( + self, vault_salt_call_cli, minion_conn_cachedir, cache_from_old_version + ): + """ + Test that after this saltext has been upgraded, an old cached configuration + is updated without breaking anything. + """ + token_cachefile = minion_conn_cachedir / "session" / "__token.p" + token_data = token_cachefile.read_bytes() + ret = vault_salt_call_cli.run("vault.read_secret", "secret/path/foo") + assert ret.returncode == 0 + assert ret.data + assert ret.data.get("success") == "yeehaaw" + config_cachefile = minion_conn_cachedir / "config.p" + cached_config = salt.utils.data.decode( + salt.utils.msgpack.loads(config_cachefile.read_bytes()) + ) + # cache_from_old_version gives us the expected config, a bit misleading + # It should be updated to the new format. + assert cached_config == cache_from_old_version + # The token should be the same. + assert token_cachefile.read_bytes() == token_data + @pytest.mark.parametrize("vault_container_version", ["latest"], indirect=True) @pytest.mark.parametrize("ckey", ["config", "__token"]) def test_cache_is_used_on_the_minion(self, ckey, vault_salt_call_cli, minion_conn_cachedir): diff --git a/tests/unit/runners/vault/test_vault.py b/tests/unit/runners/vault/test_vault.py index 0609e63d..5f75960c 100644 --- a/tests/unit/runners/vault/test_vault.py +++ b/tests/unit/runners/vault/test_vault.py @@ -42,6 +42,18 @@ def default_config(): "kv_metadata": "connection", "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": vclient.DEFAULT_RETRY_STATUS, + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "issue": { "allow_minion_override_params": False, "type": "token", @@ -501,6 +513,7 @@ def test_get_config_token( }, }, "cache": config("cache"), + "client": config("client"), "server": config("server"), "wrap_info_nested": [], } @@ -574,6 +587,7 @@ def test_get_config_approle(config, validate_signature, wrapped_serialized, issu }, }, "cache": config("cache"), + "client": config("client"), "server": config("server"), "wrap_info_nested": [], } diff --git a/tests/unit/utils/vault/conftest.py b/tests/unit/utils/vault/conftest.py index 7e861967..aa120267 100644 --- a/tests/unit/utils/vault/conftest.py +++ b/tests/unit/utils/vault/conftest.py @@ -70,6 +70,18 @@ def test_config(server_config, request): "expire_events": False, "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": vclient.DEFAULT_RETRY_STATUS, + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "issue": { "allow_minion_override_params": False, "type": "token", @@ -154,6 +166,18 @@ def test_remote_config(server_config, request): "kv_metadata": "connection", "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": vclient.DEFAULT_RETRY_STATUS, + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "server": server_config, } diff --git a/tests/unit/utils/vault/test_client.py b/tests/unit/utils/vault/test_client.py index 167fcb03..88830c5f 100644 --- a/tests/unit/utils/vault/test_client.py +++ b/tests/unit/utils/vault/test_client.py @@ -1,10 +1,14 @@ +import contextlib from unittest.mock import ANY +from unittest.mock import MagicMock from unittest.mock import Mock +from unittest.mock import call from unittest.mock import patch import pytest import requests import salt.exceptions +import urllib3.exceptions from saltext.vault.utils import vault from saltext.vault.utils.vault import client as vclient @@ -31,7 +35,6 @@ def test_vault_client_request_raw_url(endpoint, client, req): expected_url, headers=ANY, json=None, - verify=client.get_config()["verify"], ) @@ -47,7 +50,6 @@ def test_vault_client_request_raw_kwargs_passthrough(client, req): ANY, headers=ANY, json=ANY, - verify=ANY, allow_redirects=False, cert="/etc/certs/client.pem", ) @@ -126,6 +128,7 @@ def test_vault_client_request_raw_does_not_raise_http_exception(client): (404, vault.VaultNotFoundError), (405, vault.VaultUnsupportedOperationError), (412, vault.VaultPreconditionFailedError), + (429, vault.VaultRateLimitExceededError), (500, vault.VaultServerError), (502, vault.VaultServerError), (503, vault.VaultUnavailableError), @@ -606,6 +609,13 @@ def test_get_expected_creation_path_fails_for_unknown_type(): vclient._get_expected_creation_path("nonexistent") # pylint: disable=protected-access +@pytest.fixture +def _send_mock(): + with patch("saltext.vault.utils.vault.client.HTTPAdapter.send", autospec=True) as send: + send.return_value.is_redirect = False + yield send + + @pytest.mark.parametrize( "server_config", [ @@ -616,25 +626,401 @@ def test_get_expected_creation_path_fails_for_unknown_type(): ], indirect=True, ) -def test_vault_client_verify_pem(server_config): +def test_vault_api_adapter_pem(server_config, _send_mock): """ Test that the ``verify`` parameter to the client can contain a PEM-encoded certificate which will be used as the sole trust anchor for the Vault URL. - The ``verify`` parameter to ``Session.request`` should be None in that case since - it requires a local file path. - """ - with patch("saltext.vault.utils.vault.client.CACertHTTPSAdapter", autospec=True) as adapter: - with patch("saltext.vault.utils.vault.client.requests.Session", autospec=True) as session: - client = vclient.VaultClient(**server_config) - adapter.assert_called_once_with(server_config["verify"]) - session.return_value.mount.assert_called_once_with( - server_config["url"], adapter.return_value - ) - client.request_raw("GET", "test") - session.return_value.request.assert_called_once_with( - "GET", - f"{server_config['url']}/v1/test", - headers=ANY, - json=ANY, - verify=None, - ) + The ``verify`` parameter to ``HTTPAdapter.send`` should be the default (True) + in that case since it requires a local file path. + """ + with patch("saltext.vault.utils.vault.client.create_urllib3_context", autospec=True) as tls: + client = vclient.VaultClient(**server_config, connect_timeout=42, read_timeout=1337) + tls.return_value.load_verify_locations.assert_called_once_with( + cadata=server_config["verify"] + ) + client.request_raw("GET", "test") + _send_mock.assert_called_once_with( + ANY, ANY, stream=ANY, timeout=ANY, verify=True, cert=None, proxies=ANY + ) + + +def test_vault_api_adapter_timeout_default(server_config, _send_mock): + """ + Ensure the timeout defaults are set by the adapter. + """ + client = vclient.VaultClient(**server_config, connect_timeout=42, read_timeout=1337) + client.request_raw("GET", "test") + _send_mock.assert_called_once_with( + ANY, ANY, stream=ANY, timeout=(42, 1337), verify=ANY, cert=ANY, proxies=ANY + ) + + +def test_vault_api_adapter_timeout_override(server_config, _send_mock): + """ + Ensure the timeout defaults can be overridden. + """ + client = vclient.VaultClient(**server_config, connect_timeout=42, read_timeout=1337) + client.request_raw("GET", "test", timeout=1234) + _send_mock.assert_called_once_with( + ANY, ANY, stream=ANY, timeout=1234, verify=ANY, cert=ANY, proxies=ANY + ) + + +@pytest.mark.parametrize( + "server_config", + [ + { + "url": "https://127.0.0.1:8200", + "verify": False, + }, + { + "url": "https://127.0.0.1:8200", + "verify": "/some/path", + }, + ], + indirect=True, +) +def test_vault_api_adapter_verify_set(server_config, _send_mock): + """ + Ensure the ``verify`` parameter is always set as specified. + """ + with patch("saltext.vault.utils.vault.client.create_urllib3_context", autospec=True) as tls: + client = vclient.VaultClient(**server_config) + client.request_raw("GET", "test") + _send_mock.assert_called_once_with( + ANY, + ANY, + stream=ANY, + timeout=ANY, + verify=server_config["verify"], + cert=ANY, + proxies=ANY, + ) + tls.assert_not_called() + + +@pytest.fixture +def sleep_mock(): + with patch("time.sleep") as sleep: + yield sleep + + +@pytest.fixture(params=({},)) +def req_mock(request): + headers = {} + msg_mock = MagicMock() + msg_mock.values.side_effect = headers.values + msg_mock.keys.side_effect = headers.keys + msg_mock.items.side_effect = headers.items + msg_mock.get.side_effect = headers.get + msg_mock.get_all.return_value = {} + params = getattr(request, "param", {}) + status = params.get("status", 500) + retry_after = params.get("retry_after") + error = params.get("err") + if error is not None: + conn_ctx = patch( + "urllib3.connectionpool.HTTPConnectionPool._new_conn", autospec=True, side_effect=error + ) + exp_ctx = pytest.raises(requests.exceptions.ConnectionError) + else: + conn_ctx = patch("urllib3.connectionpool.HTTPConnectionPool._make_request", autospec=True) + exp_ctx = contextlib.nullcontext() + if retry_after is not None: + headers["Retry-After"] = retry_after + with conn_ctx as req: + if error is not None: + req.side_effect = error + else: + req.return_value.get_redirect_location.return_value = None + req.return_value.cookies = None + req.return_value.msg = msg_mock # urllib <2 + req.return_value.headers = headers # urllib >=2 + req.return_value.status = status + yield req, exp_ctx + + +@pytest.mark.parametrize( + "req_mock,cnt,slept,respect_retry_after", + ( + ({"status": 200}, 1, 0, True), + ({"status": 204}, 1, 0, True), + ({"status": 412}, 6, 5, True), + ({"status": 429}, 6, 5, True), + ({"status": 500}, 6, 5, True), + ({"status": 502}, 6, 5, True), + ({"status": 503}, 6, 5, True), + ({"status": 504}, 6, 5, True), + ({"err": urllib3.exceptions.ConnectTimeoutError}, 6, 4, True), + ({"err": urllib3.exceptions.ProtocolError}, 6, 4, True), + ( + { + "err": urllib3.exceptions.ReadTimeoutError( + MagicMock(), "http://127.0.0.1/v1/test", "foo" + ) + }, + 6, + 4, + True, + ), + ({"status": 200, "retry_after": "1234"}, 1, 0, True), + ({"status": 204, "retry_after": "1234"}, 1, 0, True), + ({"status": 412, "retry_after": "1234"}, 6, 5, False), + ({"status": 429, "retry_after": "1234"}, 6, 5, False), + ({"status": 500, "retry_after": "1234"}, 6, 5, False), + ({"status": 502, "retry_after": "1234"}, 6, 5, False), + ({"status": 503, "retry_after": "1234"}, 6, 5, False), + ({"status": 504, "retry_after": "1234"}, 6, 5, False), + ({"err": urllib3.exceptions.ConnectTimeoutError}, 6, 4, False), + ({"err": urllib3.exceptions.ProtocolError}, 6, 4, False), + ( + { + "err": urllib3.exceptions.ReadTimeoutError( + MagicMock(), "http://127.0.0.1/v1/test", "foo" + ) + }, + 6, + 4, + False, + ), + ), + indirect=("req_mock",), +) +def test_vault_retry(server_config, cnt, slept, respect_retry_after, req_mock, sleep_mock): + """ + Ensure the client retries specific response codes only. + HTTP error responses should not retry immediately, only connection/read errors. + """ + client = vclient.VaultClient( + **server_config, respect_retry_after=respect_retry_after, backoff_jitter=0.0 + ) + with req_mock[1]: + client.request_raw("GET", "test") + assert req_mock[0].call_count == cnt + default_sleep = [0.2, 0.3, 0.5, 0.8, 1.3, 2.1, 3.4] + calls = [call(default_sleep[x]) for x in range(slept)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.parametrize( + "req_mock", + ( + {"status": 412, "retry_after": "42"}, + {"status": 429, "retry_after": "42"}, + {"status": 500, "retry_after": "42"}, + {"status": 502, "retry_after": "42"}, + {"status": 503, "retry_after": "42"}, + {"status": 504, "retry_after": "42"}, + ), + indirect=True, +) +def test_vault_retry_retry_after(server_config, req_mock, sleep_mock): + """ + Ensure the Retry-After header is respected by default and overrides + the backoff algorithm. + """ + client = vclient.VaultClient(**server_config, backoff_jitter=0.0) + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + calls = [call(42) for x in range(5)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.parametrize( + "req_mock", + ( + {"status": 412, "retry_after": "9999999999"}, + {"status": 429, "retry_after": "9999999999"}, + {"status": 500, "retry_after": "9999999999"}, + {"status": 502, "retry_after": "9999999999"}, + {"status": 503, "retry_after": "9999999999"}, + {"status": 504, "retry_after": "9999999999"}, + ), + indirect=True, +) +@pytest.mark.parametrize("disabled", (False, True)) +def test_vault_retry_retry_after_max(server_config, disabled, req_mock, sleep_mock): + """ + Ensure the Retry-After header is respected by default and overrides + the backoff algorithm. + """ + kwargs = {} + if disabled: + kwargs["retry_after_max"] = None + client = vclient.VaultClient(**server_config, backoff_jitter=0.0, **kwargs) + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + calls = [call(60 if not disabled else 9999999999) for x in range(5)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.parametrize( + "req_mock,slept", + ( + ({"err": urllib3.exceptions.ConnectTimeoutError}, 4), + ({"err": urllib3.exceptions.ProtocolError}, 4), + ( + { + "err": urllib3.exceptions.ReadTimeoutError( + MagicMock(), "http://127.0.0.1/v1/test", "foo" + ) + }, + 4, + ), + ({"status": 412}, 5), + ({"status": 429}, 5), + ({"status": 500}, 5), + ({"status": 502}, 5), + ({"status": 503}, 5), + ({"status": 504}, 5), + ), + indirect=("req_mock",), +) +def test_vault_retry_backoff_factor(server_config, slept, req_mock, sleep_mock): + """ + Ensure the backoff_factor has an effect. + """ + client = vclient.VaultClient(**server_config, backoff_factor=0.2, backoff_jitter=0.0) + with req_mock[1]: + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + default_sleep = [0.4, 0.6, 1.0, 1.6, 2.6, 4.2, 6.8] + calls = [call(default_sleep[x]) for x in range(slept)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.parametrize( + "req_mock,slept", + ( + ({"err": urllib3.exceptions.ConnectTimeoutError}, 4), + ({"err": urllib3.exceptions.ProtocolError}, 4), + ( + { + "err": urllib3.exceptions.ReadTimeoutError( + MagicMock(), "http://127.0.0.1/v1/test", "foo" + ) + }, + 4, + ), + ({"status": 412}, 5), + ({"status": 429}, 5), + ({"status": 500}, 5), + ({"status": 502}, 5), + ({"status": 503}, 5), + ({"status": 504}, 5), + ), + indirect=("req_mock",), +) +def test_vault_retry_backoff_max(server_config, slept, req_mock, sleep_mock): + """ + Ensure backoff_max has an effect. + """ + client = vclient.VaultClient( + **server_config, backoff_factor=3, backoff_max=10, backoff_jitter=0.0 + ) + with req_mock[1]: + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + default_sleep = [6.0, 9.0, 10.0, 10.0, 10.0, 10.0, 10.0] + calls = [call(default_sleep[x]) for x in range(slept)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.parametrize("max_retries", (0, 8)) +@pytest.mark.parametrize( + "req_mock,slept", + ( + ({"err": urllib3.exceptions.ConnectTimeoutError}, -1), + ({"err": urllib3.exceptions.ProtocolError}, -1), + ( + { + "err": urllib3.exceptions.ReadTimeoutError( + MagicMock(), "http://127.0.0.1/v1/test", "foo" + ) + }, + -1, + ), + ({"status": 412}, 0), + ({"status": 429}, 0), + ({"status": 500}, 0), + ({"status": 502}, 0), + ({"status": 503}, 0), + ({"status": 504}, 0), + ), + indirect=("req_mock",), +) +def test_vault_retry_max_retries(server_config, max_retries, slept, req_mock, sleep_mock): + """ + Ensure max_retries has an effect. + """ + client = vclient.VaultClient(**server_config, max_retries=max_retries, backoff_jitter=0.0) + with req_mock[1]: + client.request_raw("GET", "test") + assert req_mock[0].call_count == max_retries + 1 + default_sleep = [0.2, 0.3, 0.5, 0.8, 1.3, 2.1, 3.4, 5.5] + calls = [call(default_sleep[x]) for x in range(max_retries + slept)] + assert sleep_mock.call_args_list == calls + + +@pytest.mark.usefixtures("sleep_mock") +@pytest.mark.parametrize("method", ("POST", "PATCH")) +def test_vault_retry_post_not_default(server_config, method, req_mock): + """ + Ensure that by default, we don't retry potentially non-idempotent actions. + """ + client = vclient.VaultClient(**server_config) + client.request_raw(method, "test") + assert req_mock[0].call_count == 1 + + +@pytest.mark.usefixtures("sleep_mock") +@pytest.mark.parametrize("req_mock", ({"status": 429},), indirect=True) +@pytest.mark.parametrize("method", ("POST", "PATCH")) +def test_vault_retry_post_with_too_many_requests(server_config, method, req_mock): + """ + Ensure 429 is always retried, even with POST/PATCH. + """ + client = vclient.VaultClient(**server_config) + client.request_raw(method, "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + + +@pytest.mark.usefixtures("sleep_mock") +@pytest.mark.parametrize("method", ("POST", "PATCH")) +def test_vault_retry_post_enabled(server_config, method, req_mock): + """ + Ensure POST/PATCH requests are retried if enabled. + """ + client = vclient.VaultClient(**server_config, retry_post=True) + client.request_raw(method, "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + + +@pytest.mark.usefixtures("sleep_mock") +@pytest.mark.parametrize("statuses", ((412,), None)) +def test_vault_retry_status(server_config, statuses, req_mock): + """ + Ensure retry_status can be set and HTTP 429 is retried + regardless. + """ + client = vclient.VaultClient(**server_config, retry_status=statuses) + client.request_raw("GET", "test") + assert req_mock[0].call_count == 1 + req_mock[0].return_value.status = 429 + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 2 + + +def test_vault_retry_backoff_jitter(server_config, req_mock, sleep_mock): + """ + Ensure that by default, we introduce some jitter to retry intervals. + """ + client = vclient.VaultClient(**server_config, backoff_factor=1, backoff_max=100) + client.request_raw("GET", "test") + assert req_mock[0].call_count == vclient.DEFAULT_MAX_RETRIES + 1 + default_sleep = [2.0, 3.0, 5.0, 8.0, 13.0, 21.0, 43.0] + assert len(sleep_mock.call_args_list) == vclient.DEFAULT_MAX_RETRIES + for exp, cll in zip(default_sleep, sleep_mock.call_args_list): + # random.random returning 0 exactly should be very seldom, + # so let's accept a tiny bit of flakiness + assert exp + vclient.DEFAULT_BACKOFF_JITTER >= cll.args[0] > exp diff --git a/tests/unit/utils/vault/test_factory.py b/tests/unit/utils/vault/test_factory.py index 2345db6e..2a2b9c95 100644 --- a/tests/unit/utils/vault/test_factory.py +++ b/tests/unit/utils/vault/test_factory.py @@ -1479,6 +1479,18 @@ def test_clear_cache_clears_client_from_context( "expire_events": False, "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": vclient.DEFAULT_RETRY_STATUS, + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "server": { "url": "http://127.0.0.1:8200", "namespace": None, @@ -1509,6 +1521,18 @@ def test_clear_cache_clears_client_from_context( "expire_events": False, "secret": "ttl", }, + "client": { + "connect_timeout": vclient.DEFAULT_CONNECT_TIMEOUT, + "read_timeout": vclient.DEFAULT_READ_TIMEOUT, + "max_retries": vclient.DEFAULT_MAX_RETRIES, + "backoff_factor": vclient.DEFAULT_BACKOFF_FACTOR, + "backoff_max": vclient.DEFAULT_BACKOFF_MAX, + "backoff_jitter": vclient.DEFAULT_BACKOFF_JITTER, + "retry_post": vclient.DEFAULT_RETRY_POST, + "retry_status": vclient.DEFAULT_RETRY_STATUS, + "respect_retry_after": vclient.DEFAULT_RESPECT_RETRY_AFTER, + "retry_after_max": vclient.DEFAULT_RETRY_AFTER_MAX, + }, "server": { "url": "http://127.0.0.1:8200", "namespace": None, @@ -1522,7 +1546,7 @@ def test_clear_cache_clears_client_from_context( ) def test_use_local_config(test_config, expected_config, expected_token): """ - Ensure that _use_local_config only returns auth, cache, server scopes + Ensure that _use_local_config only returns auth, cache, client and server scopes and pops an embedded token, if present """ with patch("saltext.vault.utils.vault.factory.parse_config", Mock(return_value=test_config)): @@ -1570,6 +1594,21 @@ def test_parse_config_respects_local_verify(opts): assert ret["server"]["verify"] == testval +def test_parse_config_respects_local_client(): + """ + Ensure locally configured verify values are respected. + """ + opts = {"vault": {"client": {"connect_timeout": 123, "backoff_jitter": 1}}} + ret = vfactory.parse_config( + {"client": {"read_timeout": 30, "connect_timeout": 1, "backoff_jitter": 0.1}}, + validate=False, + opts=opts, + ) + assert ret["client"]["read_timeout"] == 30 + assert ret["client"]["connect_timeout"] == 123 + assert ret["client"]["backoff_jitter"] == 1 + + ############################################ # Deprecation tests ############################################