From 5a6ebadc14a3c9a5510827833d73d9449a5a38c8 Mon Sep 17 00:00:00 2001 From: Sergey Sannikov Date: Sat, 31 Oct 2026 20:38:04 +0300 Subject: [PATCH 1/2] aws: create every boto3 client with retries and a connect timeout The clients used the botocore defaults. Use the standard retry mode with 5 attempts and a connect timeout of 10 seconds, the way the client in #603 did, and add a test that fails when a client is created without it. Co-authored-by: Eric Krichbaum <11653069+ekrichbaum@users.noreply.github.com> --- module/sources/aws/client.py | 24 ++++++++++++++------- tests/test_aws_client_config.py | 38 +++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) create mode 100644 tests/test_aws_client_config.py diff --git a/module/sources/aws/client.py b/module/sources/aws/client.py index 0332c901..fdcc8096 100644 --- a/module/sources/aws/client.py +++ b/module/sources/aws/client.py @@ -23,9 +23,11 @@ try: import boto3 + from botocore.config import Config from botocore.exceptions import ClientError except ImportError: # pragma: no cover - exercised via monkeypatching boto3 = None boto3 = None + Config = None ClientError = Exception from module.common.logging import get_logger @@ -43,6 +45,12 @@ MAX_ITEMS_PER_PAGE = 20 +# Every client retries throttled and transient failures with backoff and gives up on an +# unreachable endpoint after 10 seconds instead of the botocore default of 60. +BOTO_CONFIG = Config( + retries={"mode": "standard", "max_attempts": 5}, connect_timeout=10, read_timeout=60 +) if Config is not None else None + class AWSDiscoveryError(Exception): """A fatal AWS discovery failure. The caller must not write to NetBox.""" @@ -168,7 +176,7 @@ def _bootstrap_region(self, regions, discovery_region): return regions[0] def _get_identity(self, bootstrap_region, account, sts=None): - sts = sts or self.session.client("sts", region_name=bootstrap_region) + sts = sts or self.session.client("sts", region_name=bootstrap_region, config=BOTO_CONFIG) identity_account = None with self.tolerate_access_denied(None, None, "account"): response = sts.get_caller_identity() @@ -183,7 +191,7 @@ def _get_identity(self, bootstrap_region, account, sts=None): return identity_account def _enabled_regions(self, bootstrap_region, account): - ec2 = self.session.client("ec2", region_name=bootstrap_region) + ec2 = self.session.client("ec2", region_name=bootstrap_region, config=BOTO_CONFIG) enabled = set() denied_scope = (account, None, "ec2") with self.tolerate_access_denied(*denied_scope): @@ -221,7 +229,7 @@ def discover(self, regions, account=None, discovery_region=None, services=()): return identity_account, snapshot def _discover_region(self, region, account, services): - ec2 = self.session.client("ec2", region_name=region) + ec2 = self.session.client("ec2", region_name=region, config=BOTO_CONFIG) data = { "instances": [], "InstanceTypes": {}, "NetworkInterfaces": {}, "Volumes": {}, "Subnets": {}, "Vpcs": {}, @@ -248,7 +256,7 @@ def _discover_region(self, region, account, services): if "s3" in services: buckets = self._tolerated_list(self.discover_s3, account, region, "s3", ec2.meta.region_name if False else None, - args=(self.session.client("s3", region_name=region), region, account)) + args=(self.session.client("s3", region_name=region, config=BOTO_CONFIG), region, account)) if buckets is None: inaccessible.append("s3") data["Buckets"] = [] @@ -256,7 +264,7 @@ def _discover_region(self, region, account, services): data["Buckets"] = buckets if "lambda" in services: functions = self._tolerated_list(self.discover_lambda, account, region, "lambda", None, - args=(self.session.client("lambda", region_name=region), region, account)) + args=(self.session.client("lambda", region_name=region, config=BOTO_CONFIG), region, account)) if functions is None: inaccessible.append("lambda") data["Functions"] = [] @@ -264,7 +272,7 @@ def _discover_region(self, region, account, services): data["Functions"] = functions if "eks" in services: clusters = self._tolerated_list(self.discover_eks, account, region, "eks", None, - args=(self.session.client("eks", region_name=region), region, account)) + args=(self.session.client("eks", region_name=region, config=BOTO_CONFIG), region, account)) if clusters is None: inaccessible.append("eks") data["EKSClusters"] = [] @@ -479,12 +487,12 @@ def discover_organization(self, regions, role_name, expected_account=None, disco self.access_denied = [] self.access_checks = [] bootstrap_region = self._bootstrap_region(regions, discovery_region) - sts = self.session.client("sts", region_name=bootstrap_region) + sts = self.session.client("sts", region_name=bootstrap_region, config=BOTO_CONFIG) management_account = self._get_identity(bootstrap_region, expected_account, sts=sts) if management_account is None: return {} - organizations = self.session.client("organizations", region_name=bootstrap_region) + organizations = self.session.client("organizations", region_name=bootstrap_region, config=BOTO_CONFIG) org_info = None with self.tolerate_access_denied(management_account, None, "organizations"): org_info = organizations.describe_organization()["Organization"] diff --git a/tests/test_aws_client_config.py b/tests/test_aws_client_config.py new file mode 100644 index 00000000..bb3de846 --- /dev/null +++ b/tests/test_aws_client_config.py @@ -0,0 +1,38 @@ +# -*- coding: utf-8 -*- +# Copyright (c) 2020 - 2026 netbox-sync team. All rights reserved. +# +# netbox-sync.py +# +# This work is licensed under the terms of the MIT license. +# For a copy, see file LICENSE.txt included in this +# repository or visit: . + +"""Every boto3 client of the AWS source retries with backoff and has a short connect timeout.""" + +import ast +from pathlib import Path + +import module.sources.aws.client as aws_client + + +def test_boto_config_values(): + config = aws_client.BOTO_CONFIG + + assert config.retries == {"mode": "standard", "max_attempts": 5} + assert config.connect_timeout == 10 + assert config.read_timeout == 60 + + +def test_every_client_is_created_with_the_config(): + tree = ast.parse(Path(aws_client.__file__).read_text(encoding="utf-8")) + + calls = [ + node for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) and node.func.attr == "client" + and isinstance(node.func.value, ast.Attribute) and node.func.value.attr == "session" + ] + + assert calls, "no session.client() calls found, the check needs updating" + without_config = [call.lineno for call in calls if "config" not in {kw.arg for kw in call.keywords}] + assert not without_config, f"session.client() without config= at lines {without_config}" From d00bea63a72c6693d4512826c3ecc81cbc8bafb3 Mon Sep 17 00:00:00 2001 From: Sergey Sannikov Date: Sat, 31 Oct 2026 20:51:10 +0300 Subject: [PATCH 2/2] aws: turn SDK failures into AWSDiscoveryError A lost connection, a timeout, missing credentials or an unknown profile raised a bare botocore exception out of discover(), discover_organization() or the session setup, although the module promises AWSDiscoveryError for every failure that is not an authorization denial. Wrap them the way the client in #603 did and add tests for all three. Co-authored-by: Eric Krichbaum <11653069+ekrichbaum@users.noreply.github.com> --- module/sources/aws/client.py | 25 +++++++++++++++++++++++-- tests/test_aws_client_config.py | 29 ++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/module/sources/aws/client.py b/module/sources/aws/client.py index fdcc8096..49116d80 100644 --- a/module/sources/aws/client.py +++ b/module/sources/aws/client.py @@ -19,17 +19,21 @@ """ from contextlib import contextmanager from dataclasses import dataclass +from functools import wraps from typing import Optional try: import boto3 from botocore.config import Config - from botocore.exceptions import ClientError + from botocore.exceptions import BotoCoreError, ClientError except ImportError: # pragma: no cover - exercised via monkeypatching boto3 = None boto3 = None Config = None ClientError = Exception + class BotoCoreError(Exception): + pass + from module.common.logging import get_logger log = get_logger() @@ -56,6 +60,18 @@ class AWSDiscoveryError(Exception): """A fatal AWS discovery failure. The caller must not write to NetBox.""" +def fatal_on_sdk_error(func): + """Turn an SDK level failure (no route to the endpoint, timeout, missing credentials, ...) + into the AWSDiscoveryError the handler knows how to deal with.""" + @wraps(func) + def wrapper(*args, **kwargs): + try: + return func(*args, **kwargs) + except BotoCoreError as exc: + raise AWSDiscoveryError(f"Incomplete AWS discovery: {exc}") from exc + return wrapper + + @dataclass(frozen=True) class AWSAccessCheck: account: Optional[str] @@ -79,7 +95,10 @@ def __init__(self, profile=None): "the 'boto3' package is required for the AWS source; " "install it with 'pip install -r requirements-aws.txt'" ) - self.session = boto3.Session(profile_name=profile) if profile else boto3.Session() + try: + self.session = boto3.Session(profile_name=profile) if profile else boto3.Session() + except BotoCoreError as exc: + raise AWSDiscoveryError(f"Unable to initialize AWS credentials: {exc}") from exc self.access_denied = [] self.access_checks = [] self.account_names = {} @@ -207,6 +226,7 @@ def _enabled_regions(self, bootstrap_region, account): # -- single-account discovery -------------------------------------------- + @fatal_on_sdk_error def discover(self, regions, account=None, discovery_region=None, services=()): # Starts a fresh access_denied/access_checks run so a prior denial does # not linger forever once access is restored. Member clients created by @@ -482,6 +502,7 @@ def _paginate_token(self, func, key, token_key): # -- organization --------------------------------------------------------- + @fatal_on_sdk_error def discover_organization(self, regions, role_name, expected_account=None, discovery_region=None, include_management=False, external_id=None, account_ids=None, services=()): self.access_denied = [] diff --git a/tests/test_aws_client_config.py b/tests/test_aws_client_config.py index bb3de846..3408a8e3 100644 --- a/tests/test_aws_client_config.py +++ b/tests/test_aws_client_config.py @@ -7,12 +7,18 @@ # For a copy, see file LICENSE.txt included in this # repository or visit: . -"""Every boto3 client of the AWS source retries with backoff and has a short connect timeout.""" +"""Every boto3 client of the AWS source retries with backoff and has a short connect timeout, and +failures of the SDK itself end up as AWSDiscoveryError like every other discovery failure.""" import ast from pathlib import Path +from unittest.mock import Mock + +import pytest +from botocore.exceptions import EndpointConnectionError, ProfileNotFound import module.sources.aws.client as aws_client +from module.sources.aws.client import AWSClient, AWSDiscoveryError def test_boto_config_values(): @@ -36,3 +42,24 @@ def test_every_client_is_created_with_the_config(): assert calls, "no session.client() calls found, the check needs updating" without_config = [call.lineno for call in calls if "config" not in {kw.arg for kw in call.keywords}] assert not without_config, f"session.client() without config= at lines {without_config}" + + +@pytest.mark.parametrize("method", ["discover", "discover_organization"]) +def test_sdk_error_during_discovery_is_a_discovery_error(monkeypatch, method): + session = Mock(region_name="us-west-2") + session.client.side_effect = EndpointConnectionError(endpoint_url="https://sts.us-west-2.amazonaws.com") + monkeypatch.setattr(aws_client.boto3, "Session", Mock(return_value=session)) + client = AWSClient() + + with pytest.raises(AWSDiscoveryError, match="Incomplete AWS discovery"): + if method == "discover": + client.discover(["us-west-2"]) + else: + client.discover_organization(["us-west-2"], "OrganizationAccountAccessRole") + + +def test_unknown_profile_is_a_discovery_error(monkeypatch): + monkeypatch.setattr(aws_client.boto3, "Session", Mock(side_effect=ProfileNotFound(profile="nope"))) + + with pytest.raises(AWSDiscoveryError, match="Unable to initialize AWS credentials"): + AWSClient(profile="nope")