Alberto Donato has proposed merging ~ack/maas:vault-client-error-wrap into maas:master.
Commit message: wrap hvac.exceptions with local exceptions (for better error messages) Requested reviews: MAAS Maintainers (maas-maintainers) For more details, see: https://code.launchpad.net/~ack/maas/+git/maas/+merge/433305 -- Your team MAAS Maintainers is requested to review the proposed merge of ~ack/maas:vault-client-error-wrap into maas:master.
diff --git a/src/maascli/snap.py b/src/maascli/snap.py index 0459598..a8b8068 100644 --- a/src/maascli/snap.py +++ b/src/maascli/snap.py @@ -16,11 +16,9 @@ from textwrap import dedent import threading import time -from hvac.exceptions import VaultError import netifaces import psycopg2 from psycopg2.extensions import parse_dsn -from requests.exceptions import ConnectionError import tempita from maascli.command import Command, CommandError @@ -33,7 +31,7 @@ from maascli.init import ( prompt_for_choices, read_input, ) -from maasserver.vault import prepare_wrapped_approle +from maasserver.vault import prepare_wrapped_approle, VaultError ARGUMENTS = OrderedDict( [ @@ -592,7 +590,7 @@ def get_vault_settings(options) -> dict: secrets_path=options.vault_secrets_path, secrets_mount=options.vault_secrets_mount, ) - except (VaultError, ConnectionError) as e: + except VaultError as e: raise CommandError(e) return { diff --git a/src/maascli/tests/test_snap.py b/src/maascli/tests/test_snap.py index dca2c72..00f5262 100644 --- a/src/maascli/tests/test_snap.py +++ b/src/maascli/tests/test_snap.py @@ -13,13 +13,13 @@ import time from unittest.mock import MagicMock from fixtures import EnvironmentVariableFixture -from hvac.exceptions import VaultError import netifaces import pytest from maascli import snap from maascli.command import CommandError from maascli.parser import ArgumentParser +from maasserver.vault import VaultError from maastesting.factory import factory from maastesting.testcase import MAASTestCase diff --git a/src/maasserver/djangosettings/pytest_tests/test_settings.py b/src/maasserver/djangosettings/pytest_tests/test_settings.py index 72ec38d..d655bff 100644 --- a/src/maasserver/djangosettings/pytest_tests/test_settings.py +++ b/src/maasserver/djangosettings/pytest_tests/test_settings.py @@ -10,7 +10,6 @@ from unittest.mock import MagicMock from django.core.exceptions import ImproperlyConfigured from django.db import connections -from hvac.exceptions import InvalidPath, VaultError from psycopg2.extensions import ISOLATION_LEVEL_REPEATABLE_READ import pytest @@ -21,6 +20,7 @@ from maasserver.djangosettings.settings import ( _get_local_timezone, _read_timezone, ) +from maasserver.vault import UnknownSecretPath, VaultError from maastesting.factory import factory @@ -125,7 +125,7 @@ class TestGetDefaultDbConfig: def side_effect(given_path): if given_path == db_creds_vault_path: return expected - raise InvalidPath() + raise UnknownSecretPath(given_path) vault_client.get.side_effect = side_effect get_vault_mock = mocker.patch.object( @@ -158,7 +158,7 @@ class TestGetDefaultDbConfig: "database_pass": factory.make_name("uuid"), } vault_client = MagicMock() - vault_client.get.side_effect = [InvalidPath()] + vault_client.get.side_effect = [UnknownSecretPath("some-path")] mocker.patch.object( settings, "get_region_vault_client" ).return_value = vault_client diff --git a/src/maasserver/djangosettings/settings.py b/src/maasserver/djangosettings/settings.py index ba47b2f..4767bde 100644 --- a/src/maasserver/djangosettings/settings.py +++ b/src/maasserver/djangosettings/settings.py @@ -6,12 +6,15 @@ import logging import os from django.core.exceptions import ImproperlyConfigured -from hvac.exceptions import InvalidPath, VaultError from maasserver.config import get_db_creds_vault_path, RegionConfiguration from maasserver.djangosettings import fix_up_databases from maasserver.djangosettings.monkey import patch_get_script_prefix -from maasserver.vault import get_region_vault_client +from maasserver.vault import ( + get_region_vault_client, + UnknownSecretPath, + VaultError, +) from provisioningserver.utils.env import MAAS_ID @@ -64,7 +67,7 @@ def _get_default_db_config(config: RegionConfiguration) -> dict: raise ImproperlyConfigured( f"Incomplete Vault-stored DB credentials, missing key {e}" ) - except InvalidPath: + except UnknownSecretPath: # Vault does not have DB credentials, but is available. No need to report anything, use local credentials. pass except VaultError as e: diff --git a/src/maasserver/management/commands/config_vault.py b/src/maasserver/management/commands/config_vault.py index a9ad295..ca9f551 100644 --- a/src/maasserver/management/commands/config_vault.py +++ b/src/maasserver/management/commands/config_vault.py @@ -9,8 +9,6 @@ import time from typing import Optional from django.core.management.base import BaseCommand, CommandError -from hvac.exceptions import VaultError -from requests.exceptions import ConnectionError import yaml from maascli.init import prompt_yes_no @@ -29,6 +27,7 @@ from maasserver.utils.orm import transactional, with_connection from maasserver.vault import ( configure_region_with_vault, get_region_vault_client, + VaultError, WrappedSecretError, ) from provisioningserver.utils.env import MAAS_ID @@ -95,7 +94,7 @@ class Command(BaseCommand): """ ) - except (ConnectionError, VaultError, WrappedSecretError) as e: + except (VaultError, WrappedSecretError) as e: raise CommandError(e) def _get_online_regions(self) -> list[str]: diff --git a/src/maasserver/management/commands/pytest_tests/test_config_vault.py b/src/maasserver/management/commands/pytest_tests/test_config_vault.py index 7f4bd11..f79881c 100644 --- a/src/maasserver/management/commands/pytest_tests/test_config_vault.py +++ b/src/maasserver/management/commands/pytest_tests/test_config_vault.py @@ -1,9 +1,7 @@ from unittest.mock import MagicMock from django.core.management import CommandError -from hvac.exceptions import VaultError import pytest -from requests.exceptions import ConnectionError import yaml from maasserver.management.commands import config_vault @@ -15,7 +13,7 @@ from maasserver.models import ( Secret, ) from maasserver.testing.factory import factory -from maasserver.vault import WrappedSecretError +from maasserver.vault import VaultError, WrappedSecretError from provisioningserver.utils.env import MAAS_ID @@ -58,7 +56,7 @@ class TestConfigVaultConfigurateCommand: def test_wraps_specific_exceptions_only(self, configure_mock): handler = Command() - side_effects = [ConnectionError(), VaultError(), WrappedSecretError()] + side_effects = [VaultError("error"), WrappedSecretError()] configure_mock.side_effect = side_effects kwargs = self._configure_kwargs() diff --git a/src/maasserver/pytest_tests/test_vault.py b/src/maasserver/pytest_tests/test_vault.py index cc2ff6d..d050999 100644 --- a/src/maasserver/pytest_tests/test_vault.py +++ b/src/maasserver/pytest_tests/test_vault.py @@ -2,7 +2,7 @@ from datetime import datetime, timedelta, timezone from unittest.mock import ANY, MagicMock from django.core.exceptions import ImproperlyConfigured -from hvac.exceptions import VaultError +from hvac.exceptions import VaultError as HVACVaultError import pytest from maasserver import vault @@ -12,8 +12,9 @@ from maasserver.vault import ( clear_vault_client_caches, get_region_vault_client, get_region_vault_client_if_enabled, - hvac, + UnknownSecretPath, VaultClient, + VaultError, WrappedSecretError, ) @@ -62,7 +63,7 @@ class TestVaultClient: assert vault_client.get("mysecret") == value def test_get_not_found(self, vault_client): - with pytest.raises(hvac.exceptions.InvalidPath): + with pytest.raises(UnknownSecretPath): vault_client.get("mysecret") def test_delete(self, mock_hvac_client): @@ -134,7 +135,7 @@ class TestVaultClient: client=mock_hvac_client, ) ensure_auth = mocker.patch.object(client, "_ensure_auth") - ensure_auth.side_effect = [VaultError()] + ensure_auth.side_effect = [HVACVaultError()] with pytest.raises(VaultError): client.check_authentication() ensure_auth.assert_called_once() @@ -250,7 +251,7 @@ class TestUnwrapSecret: ): mocker.patch.object( mock_hvac_client.sys, "unwrap" - ).side_effect = VaultError("Test") + ).side_effect = HVACVaultError("Test") with pytest.raises(VaultError): vault.unwrap_secret("http://vault:8200", "token") diff --git a/src/maasserver/regiondservices/tests/test_vault_secrets_cleanup.py b/src/maasserver/regiondservices/tests/test_vault_secrets_cleanup.py index 000ae35..e77215f 100644 --- a/src/maasserver/regiondservices/tests/test_vault_secrets_cleanup.py +++ b/src/maasserver/regiondservices/tests/test_vault_secrets_cleanup.py @@ -4,7 +4,6 @@ from unittest.mock import call from django.db import transaction -from hvac.exceptions import VaultError from twisted.internet import reactor from twisted.internet.defer import inlineCallbacks @@ -17,6 +16,7 @@ from maasserver.secrets import SecretManager from maasserver.testing.testcase import MAASTransactionServerTestCase from maasserver.testing.vault import FakeVaultClient from maasserver.utils.threads import deferToDatabase +from maasserver.vault import VaultError from maastesting.crochet import wait_for wait_for_reactor = wait_for() diff --git a/src/maasserver/regiondservices/vault_secrets_cleanup.py b/src/maasserver/regiondservices/vault_secrets_cleanup.py index 2bd693f..9a5b43f 100644 --- a/src/maasserver/regiondservices/vault_secrets_cleanup.py +++ b/src/maasserver/regiondservices/vault_secrets_cleanup.py @@ -3,13 +3,16 @@ from datetime import timedelta import logging from django.db import transaction -from hvac.exceptions import VaultError from twisted.internet.defer import inlineCallbacks from maasserver.models import VaultSecret from maasserver.utils.orm import transactional from maasserver.utils.threads import deferToDatabase -from maasserver.vault import get_region_vault_client_if_enabled, VaultClient +from maasserver.vault import ( + get_region_vault_client_if_enabled, + VaultClient, + VaultError, +) from provisioningserver.utils.services import SingleInstanceService from provisioningserver.utils.twisted import synchronous diff --git a/src/maasserver/secrets.py b/src/maasserver/secrets.py index 0607581..300413f 100644 --- a/src/maasserver/secrets.py +++ b/src/maasserver/secrets.py @@ -1,10 +1,13 @@ from typing import Any, Literal, NamedTuple, Optional from django.db.models import Model -from hvac.exceptions import InvalidPath from maasserver.models import Node, RootKey, Secret, VaultSecret -from maasserver.vault import get_region_vault_client_if_enabled, VaultClient +from maasserver.vault import ( + get_region_vault_client_if_enabled, + UnknownSecretPath, + VaultClient, +) SIMPLE_SECRET_KEY = "secret" @@ -182,5 +185,5 @@ class SecretManager: def _get_secret_from_vault(self, path: str): try: return self._vault_client.get(path) - except InvalidPath: + except UnknownSecretPath: raise SecretNotFound(path) diff --git a/src/maasserver/tests/test_start_up.py b/src/maasserver/tests/test_start_up.py index 0031760..7391da7 100644 --- a/src/maasserver/tests/test_start_up.py +++ b/src/maasserver/tests/test_start_up.py @@ -5,7 +5,6 @@ from pathlib import Path import random from unittest.mock import call, MagicMock -from hvac.exceptions import InvalidPath, VaultError from testtools.matchers import HasLength from maasserver import deprecations, eventloop, locks, start_up @@ -24,6 +23,7 @@ from maasserver.testing.testcase import ( MAASTransactionServerTestCase, ) from maasserver.utils.orm import post_commit_hooks +from maasserver.vault import UnknownSecretPath, VaultError from maastesting import get_testing_timeout from maastesting.matchers import ( MockCalledOnceWith, @@ -39,7 +39,7 @@ from provisioningserver.utils.env import ( MAAS_SECRET, MAAS_SHARED_SECRET, ) -from provisioningserver.utils.testing import MAASIDFixture +from provisioningserver.utils.testing import MAASIDFixture, MAASUUIDFixture class TestStartUp(MAASTransactionServerTestCase): @@ -101,6 +101,7 @@ class TestInnerStartUp(MAASServerTestCase): def setUp(self): super().setUp() self.useFixture(MAASIDFixture(None)) + self.useFixture(MAASUUIDFixture("uuid")) self.patch_autospec(start_up, "dns_kms_setting_changed") self.patch(ipaddr, "get_ip_addr").return_value = {} self.versions_info = DebVersionsInfo( @@ -388,7 +389,7 @@ class TestVaultMigrateDbCredentials(MAASServerTestCase): def get_side_effect(path): if path == self.creds_path: return {"name": db_name, "user": db_user, "pass": db_pass} - raise InvalidPath(path) + raise UnknownSecretPath(path) client = MagicMock() client.get.side_effect = get_side_effect diff --git a/src/maasserver/vault.py b/src/maasserver/vault.py index 5b4e80b..269c983 100644 --- a/src/maasserver/vault.py +++ b/src/maasserver/vault.py @@ -1,10 +1,11 @@ from datetime import datetime, timedelta, timezone -from functools import cache -from typing import Any, NamedTuple, Optional +from functools import cache, wraps +from typing import Any, Callable, NamedTuple, Optional from dateutil.parser import isoparse from django.core.exceptions import ImproperlyConfigured import hvac +import requests from maasserver.config import RegionConfiguration @@ -15,10 +16,34 @@ TOKEN_BEFORE_EXPIRY_LIMIT = timedelta(seconds=10) SecretValue = dict[str, Any] +class VaultError(Exception): + """Raised to wrap the hvac.exception.VaultError one.""" + + +class UnknownSecretPath(Exception): + """Raised when the path for a secret is unknown.""" + + class WrappedSecretError(Exception): """Raised when the provided token could not be used to obtain secret_id by unwrapping""" +def wrap_errors(func: Callable) -> Callable: + """Wrap hvac exceptions with local ones.""" + + @wraps(func) + def wrapper(*args, **kwargs): + try: + return func(*args, **kwargs) + except requests.exceptions.ConnectionError as e: + raise VaultError("Vault connection failed") from e + except hvac.exceptions.VaultError as e: + raise VaultError("Vault request failed") from e + + return wrapper + + +@wrap_errors def unwrap_secret(url: str, wrapped_token: str) -> str: """Helper function to unwrap approle secret id from wrapped token""" client = hvac.Client(url=url, token=wrapped_token) @@ -50,6 +75,7 @@ class VaultClient: # ensure a token is created at the first request self._token_expire = self._utcnow() + @wrap_errors def set(self, path: str, value: SecretValue): self._ensure_auth() self._kv.create_or_update_secret( @@ -58,13 +84,18 @@ class VaultClient: mount_point=self._secrets_mount, ) + @wrap_errors def get(self, path: str) -> SecretValue: self._ensure_auth() - return self._kv.read_secret( - self._secret_path(path), - mount_point=self._secrets_mount, - )["data"]["data"] - + try: + return self._kv.read_secret( + self._secret_path(path), + mount_point=self._secrets_mount, + )["data"]["data"] + except hvac.exceptions.InvalidPath: + raise UnknownSecretPath(path) + + @wrap_errors def delete(self, path: str): self._ensure_auth() return self._kv.delete_metadata_and_all_versions( @@ -72,6 +103,7 @@ class VaultClient: mount_point=self._secrets_mount, ) + @wrap_errors def check_authentication(self): """Checks if vault is available, throws otherwise""" self._ensure_auth(force=True)
-- Mailing list: https://launchpad.net/~sts-sponsors Post to : sts-sponsors@lists.launchpad.net Unsubscribe : https://launchpad.net/~sts-sponsors More help : https://help.launchpad.net/ListHelp