sts-sponsors team mailing list archive

[Alberto Donato <mp+433305@xxxxxxxxxxxxxxxxxx>]

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)