From 1a799d66792bf07384fbd669918de42e009b8b6f Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 21 Jul 2026 11:52:47 +0100 Subject: [PATCH 1/4] TOPS-2500 - cap boto3 client timeouts & retries (env load hangs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit envars2's AWS clients were built with no botocore.Config, inheriting the defaults: connect_timeout=60s, read_timeout=60s, legacy retries (up to 5 attempts). A single stalled STS/KMS/SSM/CloudFormation endpoint could hang for up to ~300s, and a full resolve makes several such calls (STS for location auto-detect + one KMS decrypt per secret), which compounds. This is the intermittent 1-5 min env-loading hang reported in DSS-3428 — not the GCP-credential theory in that ticket (all GCP paths are gated on cloud_provider == "gcp"; an AWS-only resolve touches zero GCP endpoints). - Add shared bounded AWS_CLIENT_CONFIG (connect=3s, read=5s, standard retries, 3 total attempts) and apply it to every boto3 client (aws_kms, aws_ssm, aws_cloudformation, cloud_utils). Worst case on a dead endpoint drops from ~300s to ~17s, overridable via ENVARS_AWS_* env vars. - get_aws_account_id() now catches BotoCoreError/ClientError, so a stalled STS degrades to None (caller asks for --loc) instead of crashing `envars exec` with an uncaught ReadTimeoutError. - Tests: assert clients use the bounded config and that a stalled STS returns None rather than raising. --- src/envars/aws_cloudformation.py | 4 +- src/envars/aws_config.py | 36 +++++++++++++++++ src/envars/aws_kms.py | 4 +- src/envars/aws_ssm.py | 4 +- src/envars/cloud_utils.py | 13 ++++-- tests/test_aws_config.py | 68 ++++++++++++++++++++++++++++++++ 6 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 src/envars/aws_config.py create mode 100644 tests/test_aws_config.py diff --git a/src/envars/aws_cloudformation.py b/src/envars/aws_cloudformation.py index 775376b..95b979e 100644 --- a/src/envars/aws_cloudformation.py +++ b/src/envars/aws_cloudformation.py @@ -1,9 +1,11 @@ import boto3 +from .aws_config import AWS_CLIENT_CONFIG + class CloudFormationExports: def __init__(self, region_name: str | None = None): - self.client = boto3.client("cloudformation", region_name=region_name) + self.client = boto3.client("cloudformation", region_name=region_name, config=AWS_CLIENT_CONFIG) self._exports_cache: dict[str, str] | None = None def _populate_exports_cache(self): diff --git a/src/envars/aws_config.py b/src/envars/aws_config.py new file mode 100644 index 0000000..2ef4b4f --- /dev/null +++ b/src/envars/aws_config.py @@ -0,0 +1,36 @@ +"""Shared botocore client configuration for envars' AWS calls. + +Without an explicit ``Config``, ``boto3.client(...)`` inherits the botocore defaults: +``connect_timeout=60s``, ``read_timeout=60s`` and legacy retries (up to 5 attempts). +A single stalled STS/KMS/SSM/CloudFormation endpoint can therefore hang for up to +``5 x 60 = ~300s``, and a full resolve makes several such calls (STS for location +auto-detect, one KMS decrypt per secret, plus any ``parameter_store:`` / +``cloudformation_export:`` lookups). These bounds turn a silent multi-minute hang into +a fast, legible failure while still tolerating a transient blip. + +Override per-environment via the ``ENVARS_AWS_*`` variables if the defaults are too tight. +""" + +import os + +from botocore.config import Config + + +def _int_env(name: str, default: int) -> int: + """Reads a positive int from the environment, falling back to ``default``.""" + try: + value = int(os.environ[name]) + except (KeyError, ValueError): + return default + return value if value > 0 else default + + +# Bounded so a stalled endpoint fails in seconds, not minutes. Standard retry mode adds +# one backoff retry for transient errors while capping the worst case far below botocore's +# default of read_timeout(60) x legacy 5 attempts = ~300s. A fully-unreachable endpoint +# means the whole resolve will fail regardless, so we fail fast rather than wait it out. +AWS_CLIENT_CONFIG = Config( + connect_timeout=_int_env("ENVARS_AWS_CONNECT_TIMEOUT", 3), + read_timeout=_int_env("ENVARS_AWS_READ_TIMEOUT", 5), + retries={"max_attempts": _int_env("ENVARS_AWS_MAX_ATTEMPTS", 2), "mode": "standard"}, +) diff --git a/src/envars/aws_kms.py b/src/envars/aws_kms.py index b02d63d..7545d98 100644 --- a/src/envars/aws_kms.py +++ b/src/envars/aws_kms.py @@ -3,13 +3,15 @@ import boto3 from botocore.exceptions import ClientError +from .aws_config import AWS_CLIENT_CONFIG + class AWSKMSAgent: """A class to handle AWS KMS operations.""" def __init__(self, region_name: str | None = None): """Initializes the KMS client.""" - self.kms_client = boto3.client("kms", region_name=region_name) + self.kms_client = boto3.client("kms", region_name=region_name, config=AWS_CLIENT_CONFIG) def encrypt(self, data: str, key_id: str, encryption_context: dict[str, str]) -> str: """Encrypts data using the specified KMS key.""" diff --git a/src/envars/aws_ssm.py b/src/envars/aws_ssm.py index f78d7cc..f6e2ee8 100644 --- a/src/envars/aws_ssm.py +++ b/src/envars/aws_ssm.py @@ -1,9 +1,11 @@ import boto3 +from .aws_config import AWS_CLIENT_CONFIG + class SSMParameterStore: def __init__(self, region_name: str | None = None): - self.client = boto3.client("ssm", region_name=region_name) + self.client = boto3.client("ssm", region_name=region_name, config=AWS_CLIENT_CONFIG) def get_parameter(self, name: str, with_decryption: bool = True) -> str | None: try: diff --git a/src/envars/cloud_utils.py b/src/envars/cloud_utils.py index d1f3929..eb411d0 100644 --- a/src/envars/cloud_utils.py +++ b/src/envars/cloud_utils.py @@ -2,10 +2,12 @@ import sys import boto3 -from botocore.exceptions import NoCredentialsError +from botocore.exceptions import BotoCoreError, ClientError from google.auth import default as google_auth_default from google.auth.exceptions import DefaultCredentialsError +from .aws_config import AWS_CLIENT_CONFIG + def _debug(message): """Prints a debug message to stderr if ENVARS_DEBUG is set.""" @@ -16,11 +18,14 @@ def _debug(message): def get_aws_account_id() -> str | None: """Retrieves the AWS account ID from the current credentials.""" try: - account_id = boto3.client("sts").get_caller_identity().get("Account") + account_id = boto3.client("sts", config=AWS_CLIENT_CONFIG).get_caller_identity().get("Account") _debug(f"Found AWS Account ID: {account_id}") return account_id - except NoCredentialsError: - _debug("No AWS credentials found.") + except (BotoCoreError, ClientError) as e: + # NoCredentialsError, connect/read timeouts and API errors all land here. This is + # best-effort location auto-detection, so degrade to "unknown" (the caller then asks + # for --loc) instead of hanging/crashing when STS is slow or unreachable. + _debug(f"Could not determine AWS account ID: {type(e).__name__}: {e}") return None diff --git a/tests/test_aws_config.py b/tests/test_aws_config.py new file mode 100644 index 0000000..b943876 --- /dev/null +++ b/tests/test_aws_config.py @@ -0,0 +1,68 @@ +"""Tests for the shared bounded AWS client configuration (TOPS-2500).""" + +from unittest.mock import MagicMock, patch + +import boto3 +from botocore.exceptions import ReadTimeoutError + +from src.envars import aws_config, cloud_utils +from src.envars.aws_cloudformation import CloudFormationExports +from src.envars.aws_kms import AWSKMSAgent +from src.envars.aws_ssm import SSMParameterStore + + +def test_default_config_is_bounded(): + """The shared config caps timeouts and retries well below botocore's defaults (60s/60s/5).""" + cfg = aws_config.AWS_CLIENT_CONFIG + assert cfg.connect_timeout == 3 + assert cfg.read_timeout == 5 + assert cfg.retries == {"max_attempts": 2, "mode": "standard"} + + +def test_int_env_parses_positive_override(monkeypatch): + """_int_env returns a valid positive override from the environment.""" + monkeypatch.setenv("ENVARS_AWS_READ_TIMEOUT", "42") + assert aws_config._int_env("ENVARS_AWS_READ_TIMEOUT", 10) == 42 + + +def test_int_env_falls_back_when_unset_invalid_or_non_positive(monkeypatch): + """Unset, non-numeric, and zero/negative values all fall back to the default.""" + monkeypatch.delenv("ENVARS_AWS_READ_TIMEOUT", raising=False) + assert aws_config._int_env("ENVARS_AWS_READ_TIMEOUT", 10) == 10 # unset + + monkeypatch.setenv("ENVARS_AWS_READ_TIMEOUT", "not-an-int") + assert aws_config._int_env("ENVARS_AWS_READ_TIMEOUT", 10) == 10 # invalid + + monkeypatch.setenv("ENVARS_AWS_READ_TIMEOUT", "0") + assert aws_config._int_env("ENVARS_AWS_READ_TIMEOUT", 10) == 10 # non-positive + + +def test_kms_client_uses_bounded_config(): + """AWSKMSAgent constructs its client with the shared bounded config.""" + agent = AWSKMSAgent(region_name="eu-west-1") + assert agent.kms_client.meta.config.connect_timeout == 3 + assert agent.kms_client.meta.config.read_timeout == 5 + + +def test_ssm_client_uses_bounded_config(): + """SSMParameterStore constructs its client with the shared bounded config.""" + store = SSMParameterStore(region_name="eu-west-1") + assert store.client.meta.config.read_timeout == 5 + + +def test_cloudformation_client_uses_bounded_config(): + """CloudFormationExports constructs its client with the shared bounded config.""" + exports = CloudFormationExports(region_name="eu-west-1") + assert exports.client.meta.config.read_timeout == 5 + + +def test_get_aws_account_id_returns_none_on_timeout(): + """A stalled STS endpoint degrades to None instead of raising ReadTimeoutError. + + Regression for the DSS-3428 crash: get_aws_account_id only caught NoCredentialsError, + so a stalled endpoint surfaced as an uncaught traceback out of `envars exec`. + """ + stalled = MagicMock() + stalled.get_caller_identity.side_effect = ReadTimeoutError(endpoint_url="https://sts.eu-west-1.amazonaws.com/") + with patch.object(boto3, "client", return_value=stalled): + assert cloud_utils.get_aws_account_id() is None From 5add2fe623707d89d7a86bdc8cda5ed9eca6c122 Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 21 Jul 2026 12:02:15 +0100 Subject: [PATCH 2/4] TOPS-2500 - PR review: clarify retry count, pin behaviour, widen override tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round (both bot reviewers converged on the same false premise that max_attempts is inclusive of the initial request). Measured against botocore 1.39.4: max_attempts=2 -> 3 total HTTP attempts (total_max_attempts=N+1), so the value is already the intended "3 total attempts" — declined the bump. Real fixes from the signal: - Extract _build_config() with an accurate docstring: max_attempts=2 => 3 total attempts (1 initial + 2 retries). Corrects the previous "one backoff retry". - Add test asserting the resolved total_max_attempts == 3 (pins the ~17s worst case against a botocore mapping change; makes "3 total attempts" executable). - Widen override test to cover all three ENVARS_AWS_* vars, not just read_timeout. --- src/envars/aws_config.py | 27 ++++++++++++++++++--------- tests/test_aws_config.py | 23 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/src/envars/aws_config.py b/src/envars/aws_config.py index 2ef4b4f..8fc1378 100644 --- a/src/envars/aws_config.py +++ b/src/envars/aws_config.py @@ -25,12 +25,21 @@ def _int_env(name: str, default: int) -> int: return value if value > 0 else default -# Bounded so a stalled endpoint fails in seconds, not minutes. Standard retry mode adds -# one backoff retry for transient errors while capping the worst case far below botocore's -# default of read_timeout(60) x legacy 5 attempts = ~300s. A fully-unreachable endpoint -# means the whole resolve will fail regardless, so we fail fast rather than wait it out. -AWS_CLIENT_CONFIG = Config( - connect_timeout=_int_env("ENVARS_AWS_CONNECT_TIMEOUT", 3), - read_timeout=_int_env("ENVARS_AWS_READ_TIMEOUT", 5), - retries={"max_attempts": _int_env("ENVARS_AWS_MAX_ATTEMPTS", 2), "mode": "standard"}, -) +def _build_config() -> Config: + """Builds the bounded AWS client config, reading ``ENVARS_AWS_*`` overrides at call time. + + Bounded so a stalled endpoint fails in seconds, not minutes. botocore treats + ``retries.max_attempts`` as the RETRY count, so ``max_attempts=2`` resolves to 3 total + attempts (1 initial + 2 retries, i.e. ``total_max_attempts=3``); with ``read_timeout=5s`` + the worst case is ~17s, versus botocore's default of ``read_timeout(60) x legacy 5 + attempts = ~300s``. A fully-unreachable endpoint fails the whole resolve regardless, so we + fail fast rather than wait it out. + """ + return Config( + connect_timeout=_int_env("ENVARS_AWS_CONNECT_TIMEOUT", 3), + read_timeout=_int_env("ENVARS_AWS_READ_TIMEOUT", 5), + retries={"max_attempts": _int_env("ENVARS_AWS_MAX_ATTEMPTS", 2), "mode": "standard"}, + ) + + +AWS_CLIENT_CONFIG = _build_config() diff --git a/tests/test_aws_config.py b/tests/test_aws_config.py index b943876..9112ace 100644 --- a/tests/test_aws_config.py +++ b/tests/test_aws_config.py @@ -19,6 +19,29 @@ def test_default_config_is_bounded(): assert cfg.retries == {"max_attempts": 2, "mode": "standard"} +def test_resolved_client_makes_three_total_attempts(): + """max_attempts=2 resolves to 3 total HTTP attempts (1 initial + 2 retries). + + botocore treats retries.max_attempts as the retry count, so total_max_attempts = N + 1 + (verified against botocore 1.39.4). Pinning the resolved value keeps the ~300s -> ~17s + worst case from silently regressing if that mapping ever changes, and makes the PR's + "3 total attempts" claim executable. + """ + client = boto3.client("sts", region_name="eu-west-1", config=aws_config.AWS_CLIENT_CONFIG) + assert client.meta.config.retries["total_max_attempts"] == 3 + + +def test_env_overrides_flow_through(monkeypatch): + """All three ENVARS_AWS_* overrides wire through to the built config, not just read_timeout.""" + monkeypatch.setenv("ENVARS_AWS_CONNECT_TIMEOUT", "7") + monkeypatch.setenv("ENVARS_AWS_READ_TIMEOUT", "11") + monkeypatch.setenv("ENVARS_AWS_MAX_ATTEMPTS", "4") + client = boto3.client("sts", region_name="eu-west-1", config=aws_config._build_config()) + assert client.meta.config.connect_timeout == 7 + assert client.meta.config.read_timeout == 11 + assert client.meta.config.retries["total_max_attempts"] == 5 # input 4 -> 5 total (N+1) + + def test_int_env_parses_positive_override(monkeypatch): """_int_env returns a valid positive override from the environment.""" monkeypatch.setenv("ENVARS_AWS_READ_TIMEOUT", "42") From 46a6835e1c484cce017ea49279799b1674742454 Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 21 Jul 2026 12:13:30 +0100 Subject: [PATCH 3/4] TOPS-2500 - PR review: make retries assertions robust to botocore in-place rewrite botocore rewrites Config.retries in place when a client is built from the config (max_attempts -> total_max_attempts), so: - test_default_config_is_bounded used full-dict equality on the shared AWS_CLIENT_CONFIG, which only passed by test ordering (before any client build) and was version-fragile. Now asserts specific keys. - test_resolved_client built its client from the shared AWS_CLIENT_CONFIG, mutating it for later tests. Renamed to test_default_makes_three_total_ attempts and built from a fresh _build_config(). Production is unaffected: sequential clients from the one shared config all resolve to total_max_attempts=3 (the rewrite is idempotent). --- tests/test_aws_config.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/test_aws_config.py b/tests/test_aws_config.py index 9112ace..3b90bf0 100644 --- a/tests/test_aws_config.py +++ b/tests/test_aws_config.py @@ -12,22 +12,28 @@ def test_default_config_is_bounded(): - """The shared config caps timeouts and retries well below botocore's defaults (60s/60s/5).""" + """The shared config caps timeouts and retries well below botocore's defaults (60s/60s/5). + + Asserts individual keys rather than full-dict equality on ``retries``: botocore rewrites + that dict in place when a client is built from the config (``max_attempts`` becomes + ``total_max_attempts``), so an equality assertion would be order- and version-fragile. + """ cfg = aws_config.AWS_CLIENT_CONFIG assert cfg.connect_timeout == 3 assert cfg.read_timeout == 5 - assert cfg.retries == {"max_attempts": 2, "mode": "standard"} + assert cfg.retries["mode"] == "standard" -def test_resolved_client_makes_three_total_attempts(): +def test_default_makes_three_total_attempts(): """max_attempts=2 resolves to 3 total HTTP attempts (1 initial + 2 retries). botocore treats retries.max_attempts as the retry count, so total_max_attempts = N + 1 (verified against botocore 1.39.4). Pinning the resolved value keeps the ~300s -> ~17s worst case from silently regressing if that mapping ever changes, and makes the PR's - "3 total attempts" claim executable. + "3 total attempts" claim executable. Built from a fresh _build_config() so botocore's + in-place rewrite of retries doesn't leak into the shared AWS_CLIENT_CONFIG other tests read. """ - client = boto3.client("sts", region_name="eu-west-1", config=aws_config.AWS_CLIENT_CONFIG) + client = boto3.client("sts", region_name="eu-west-1", config=aws_config._build_config()) assert client.meta.config.retries["total_max_attempts"] == 3 From 64ad3bbda68686c5d50f8e68296b49a1d10ee54e Mon Sep 17 00:00:00 2001 From: Keith Harvey Date: Tue, 21 Jul 2026 12:23:18 +0100 Subject: [PATCH 4/4] TOPS-2500 - PR review: clarify AWS_CLIENT_CONFIG is built at import _build_config()'s "at call time" wording could mislead: production uses the module-level AWS_CLIENT_CONFIG, built once at import, so ENVARS_AWS_* must be set in the environment before invoking envars (the normal case for a short-lived CLI). Docstring + a comment on the constant now say so, and point at _build_config() for the dynamic-re-read case. Docs only. --- src/envars/aws_config.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/envars/aws_config.py b/src/envars/aws_config.py index 8fc1378..a7e9e52 100644 --- a/src/envars/aws_config.py +++ b/src/envars/aws_config.py @@ -26,7 +26,7 @@ def _int_env(name: str, default: int) -> int: def _build_config() -> Config: - """Builds the bounded AWS client config, reading ``ENVARS_AWS_*`` overrides at call time. + """Builds the bounded AWS client config from the current ``ENVARS_AWS_*`` environment values. Bounded so a stalled endpoint fails in seconds, not minutes. botocore treats ``retries.max_attempts`` as the RETRY count, so ``max_attempts=2`` resolves to 3 total @@ -42,4 +42,8 @@ def _build_config() -> Config: ) +# Built once, at import. envars is a short-lived CLI, so reading ENVARS_AWS_* here (from the +# already-populated process environment) is equivalent to reading them per client — set any +# overrides in the environment before invoking envars, not after import. Every client is built +# from this shared instance; call _build_config() directly only if you need a dynamic re-read. AWS_CLIENT_CONFIG = _build_config()