Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BaseSettings fails to populate nested module with loaded secrets #154

Open
bkis opened this issue Aug 29, 2023 · 13 comments
Open

BaseSettings fails to populate nested module with loaded secrets #154

bkis opened this issue Aug 29, 2023 · 13 comments
Assignees

Comments

@bkis
Copy link

bkis commented Aug 29, 2023

The documentation explains how nested settings can be populated via env vars and also how secrets can be loaded from secrets files.

I'd expect BaseSettings to be able to pass a loaded secret down to nested settings if the name of the secret matches the configured notation.

I have a scenario for easy reproduction here:

app.py:

import os

from pydantic import BaseModel, SecretStr
from pydantic_settings import BaseSettings, SettingsConfigDict

os.environ["APPNAME_NESTED__SOMETHING"] = "nested-env-value"

class SubSettings(BaseModel):
    secret: SecretStr = SecretStr("default")
    something: str = "default"

class Settings(BaseSettings):
    model_config = SettingsConfigDict(
        env_prefix="APPNAME_",
        env_nested_delimiter="__",
        case_sensitive=False,
        secrets_dir="./secrets"
    )

    nested: SubSettings = SubSettings()
    secret: SecretStr = SecretStr("default")

settings = Settings()
print(f"Settings.secret: {settings.secret.get_secret_value()}")
print(f"Settings.nested.something: {settings.nested.something}")
print(f"Settings.nested.secret: {settings.nested.secret.get_secret_value()}")

./secrets/APPNAME_SECRET:

root-level-secret

./secrets/APPNAME_NESTED__SECRET:

nested-secret

SubSettings.something is only to demonstrate that populating a nested field via env var is working as expected with this setup!
Running app.py generates the following output:

Settings.secret: root-level-secret
Settings.nested.something: nested-env-value
Settings.nested.secret: default

...so the secret is not passed down to Settings.nested.secret.
I am using Pydantic 2.3.0 and Pydantic-Settings 2.0.3

Selected Assignee: @dmontagu

@hramezani
Copy link
Member

Thanks @bkis for this 🙏

Secret source only looks for the secret file based on the field name in the directories.

So, if you want to load nested model values from the secret you need to change the filename to APPNAME_NESTED and then include the submodel field values as json string in the file content. for example:

{"secret": "a secret", "something": "something else"}

@bkis
Copy link
Author

bkis commented Aug 30, 2023

Oh, I see. Thank you for your reply!

That's a bit unfortunate, though. Just to make sure I got this right: It takes all the attributes of the settings model that has a secrets_dir configured and checks if this directory contains any files that are named like this list of attributes?

Wouldn't it be more efficient (and universal) to do it the other way around? Grab a list of all the files in the secrets_dir, apply the same parsing to those file names that's applied to the vars loaded from environment and then populate the settings structure just in exactly the same way it's done with those env vars. Why do it all differently for secrets?

I think that my use case (the one I've shown above) isn't very... niche. And the way it is now leaves me with two rather bad possibilities:

  1. Completely refactor my settings structure and flatten it into one settings model to be able to use secrets the way I need.
  2. Force the users of my application to write JSON into a secret file instead of just a plain password. This would make the documentation of the deployment unnecessarily complicated, too.

@hramezani
Copy link
Member

Currently, we can't change it because it will introduce a breaking change. but you have some options:

  • Introducing this behavior with a flag to pydantic-settings by making a PR.
  • Creating your own custom settings resource. you can take a look customise settings sources.

@bkis
Copy link
Author

bkis commented Aug 30, 2023

Thanks, @hramezani , I'll look into that and see if I find the time to contribute. I'd love to do so, but I am also not sure if I'm up to the task ;)

@bkis
Copy link
Author

bkis commented Aug 30, 2023

Before I jump into this missing some obvious gotchas of my conceptual idea: What do you think of it? You have more experience with the inner workings of this library - is there anything problematic about my idea that I am not seeing?

@hramezani
Copy link
Member

complexity I would say. If you take a look at the code for env settings source in sources.py, you will see there is a lots of code to handle nested model complexity. we did it on that side because there were requests for that.

If you check the nested model tests in our tests you will find different cases that your suggested secret source has to support.

BTW, it might be complex but not impossible :)

@criccomini
Copy link

Just chiming in with a +1 for this feature. I'd like to be able to merge nested structures as well. :)

@bkis
Copy link
Author

bkis commented Aug 31, 2023

@criccomini You're very welcome to have a try on implementing it. The strategy I was thinking about goes like this:

  1. have an additional kwarg nested_secrets: bool (or similar) for SettingsConfigDict that triggers the use of the alternative implementation (obviously only if secret_dir is given, too)
  2. create an extra NestedSecretsSettingsSource source class in pydantic_settings/sources that
    • scans the secrets_dir for files and collects their names
    • parses those names the same way env vars are parsed (respecting case_sensitive, env_prefix and env_nested_delimiter)
    • created a nested dict resulting from this process that represents the structure of these nested secrets and contains their values
    • apply this to the nested settings

It's kinda straight forward in theory, but I had a look at the existing implementation and had to learn that it'll take some time to understand what's happening and how to integrate the functionality in a way that respects the existing design of the codebase.

@MrTrick
Copy link

MrTrick commented Feb 2, 2024

I can confirm that this occurs too - the SecretsSettingSource class doesn't receive any env_nested_delimiter argument.
https://github.com/pydantic/pydantic-settings/blob/main/pydantic_settings/main.py#L169-L171
(and looks like it doesn't support the concept?)

With env_nested_delimiter: '__' and secrets_dir: /run/secrets, I was unable to load my nested setting TEST_DATABASE__PASSWORD, even though it was confirmed to exist at /run/secrets/TEST_DATABASE__PASSWORD

Did anyone make progress on a fork already?

@MrTrick
Copy link

MrTrick commented Feb 2, 2024

Running this just before building the settings object is a workaround for me. (source priority not a big deal)

# Workaround: pydantic-settings doesn't yet support importing secrets with nesting,
# Load them into the environment variable space.
for child in Path("/run/secrets").iterdir():
    if child.is_file():
        environ[child.name] = child.read_text().strip()

@A-Telfer
Copy link

A-Telfer commented May 5, 2024

I just created my own secrets setting source as suggested, then defaulted to the EnvSettingsSource behaviour, except I read in the secrets_dir instead of using os.environ

from pydantic_settings.sources import EnvSettingsSource, parse_env_vars
from typing import Mapping
from pathlib import Path
from pydantic_settings import BaseSettings

class SecretsSettingsSource(EnvSettingsSource):
    """
    Source class for loading settings values from secret files.
    """

    def __init__(
        self,
        settings_cls: type[BaseSettings],
        secrets_dir: str | Path | None = None,
        case_sensitive: bool | None = None,
        env_prefix: str | None = None,
        env_nested_delimiter: str | None = None,
        env_ignore_empty: bool | None = None,
        env_parse_none_str: str | None = None
    ) -> None:
        self.secrets_dir = secrets_dir if secrets_dir is not None else self.config.get('secrets_dir')
        super().__init__(
            settings_cls,
            case_sensitive=case_sensitive,
            env_prefix=env_prefix,
            env_nested_delimiter=env_nested_delimiter,
            env_ignore_empty=env_ignore_empty,
            env_parse_none_str=env_parse_none_str
        )

    def _load_env_vars(self) -> Mapping[str, str | None]:
        if not Path(self.secrets_dir).is_dir():
            return {}
        
        file_dict = {
            str(f.stem): open(f).read() for f in Path(self.secrets_dir).iterdir() if f.is_file()
        }

        return parse_env_vars(
            file_dict,
            self.case_sensitive,
            self.env_ignore_empty,
            self.env_parse_none_str,
        )

    def __repr__(self) -> str:
        return f'SecretsSettingsSource(secrets_dir={self.secrets_dir!r})'

@bkis
Copy link
Author

bkis commented May 6, 2024

@A-Telfer Nice! I took your SecretsSettingsSource for a ride and integrated it into my original MRE from this issue (see further below). As in the original example, I set env_nested_delimiter to __ (dunder) and secrets_dir to ./secrets and created two secret files:

  1. .secrets/APPNAME_NESTED__SECRET containing nested-secret-value
  2. .secrets/APPNAME_SECRET containing secret-value

When running my example, I noticed three issues I had with your code. I am listing them here from most to least problematic (also see the comments in the code below):

  1. You are calling self.config.get('secrets_dir') in SecretsSettingsSource.__init__() to set the value of self.secrets_dir before calling super().__init__(). But self.config is only set after super().__init__() is called (in PydanticBaseSettingsSource.__init__() which is like 4 classes higher up in the hierarchy). I am not sure how this worked on your end. Maybe I misunderstood something here, but for me it looks like this wouldn't work.
  2. You are using open(f) inside the dict comprehension without a context manager. I think in this case it's better to just use the read_text() method of the Path object that's already there.
  3. I added .strip() to the read_text() call (see point no. 2) as trailing spaces/newlines would lead to rather tricky errors.

After making these changes my example looks as follows (and it seems to work just fine!). Please note that the project I use this in requires Python >=3.10 – hence the changes in the way I make type hints.

import os

from collections.abc import Mapping
from pathlib import Path

from pydantic import BaseModel, SecretStr
from pydantic_settings import (
    BaseSettings,
    PydanticBaseSettingsSource,
    SettingsConfigDict,
)
from pydantic_settings.sources import EnvSettingsSource, parse_env_vars


os.environ["APPNAME_NESTED__SOMETHING"] = "nested-env-value"


class SecretsSettingsSource(EnvSettingsSource):
    """
    Source class for loading settings values from secret files.
    """

    def __init__(
        self,
        settings_cls: type[BaseSettings],
        secrets_dir: str | Path | None = None,
        case_sensitive: bool | None = None,
        env_prefix: str | None = None,
        env_nested_delimiter: str | None = None,
        env_ignore_empty: bool | None = None,
        env_parse_none_str: str | None = None,
    ) -> None:
        # take secrets_dir from settings_cls.model_config instead of from self.config,
        # as self.config is only set AFTER super().__init__() is called later on
        # (it's in PydanticBaseSettingsSource.__init__()
        # which is like 4 classesc up in the hierarchy)
        self.secrets_dir = (
            secrets_dir
            if secrets_dir is not None
            else settings_cls.model_config.get("secrets_dir")
        )
        super().__init__(
            settings_cls,
            case_sensitive=case_sensitive,
            env_prefix=env_prefix,
            env_nested_delimiter=env_nested_delimiter,
            env_ignore_empty=env_ignore_empty,
            env_parse_none_str=env_parse_none_str,
        )

    def _load_env_vars(self) -> Mapping[str, str | None]:
        if not Path(self.secrets_dir).is_dir():
            return {}

        # use Path.read_text() and strip() it to avoid trailing newlines
        # (the original approach was to open() the file without using a context manager)
        file_dict = {
            str(f.stem): f.read_text(encoding="utf-8").strip()
            for f in Path(self.secrets_dir).iterdir()
            if f.is_file()
        }

        return parse_env_vars(
            file_dict,
            self.case_sensitive,
            self.env_ignore_empty,
            self.env_parse_none_str,
        )

    def __repr__(self) -> str:
        return f"SecretsSettingsSource(secrets_dir={self.secrets_dir!r})"


class SubSettings(BaseModel):
    secret: SecretStr = SecretStr("default")
    something: str = "default"


class Settings(BaseSettings):
    model_config = SettingsConfigDict(
        env_prefix="APPNAME_",
        env_nested_delimiter="__",
        case_sensitive=False,
        secrets_dir="./secrets",
    )

    nested: SubSettings = SubSettings()
    secret: SecretStr = SecretStr("default")

    @classmethod
    def settings_customise_sources(
        cls,
        settings_cls: type[BaseSettings],
        init_settings: PydanticBaseSettingsSource,
        env_settings: PydanticBaseSettingsSource,
        dotenv_settings: PydanticBaseSettingsSource,
        file_secret_settings: PydanticBaseSettingsSource,
    ) -> tuple[PydanticBaseSettingsSource, ...]:
        return (
            init_settings,
            SecretsSettingsSource(settings_cls),
            env_settings,
            file_secret_settings,
        )


settings = Settings()
print(f"Settings.secret: {settings.secret.get_secret_value()}")
print(f"Settings.nested.something: {settings.nested.something}")
print(f"Settings.nested.secret: {settings.nested.secret.get_secret_value()}")

...it prints the following output:

Settings.secret: secret-value
Settings.nested.something: nested-env-value
Settings.nested.secret: nested-secret-value

...which is exactly what I'd expect. Now I am not sure how robust this is, but it's looking good!

@makukha
Copy link
Contributor

makukha commented Aug 16, 2024

Just wrote a small package that solves this problem:
https://github.com/makukha/pydantic-file-secrets

Some features:

  • Use secret file source in nested settings models
  • Drop-in replacement of standard SecretsSettingsSource
  • Plain or nested directory layout: /run/secrets/dir__key or /run/secrets/dir/key
  • Respects env_prefix, env_nested_delimiter and other config options
  • Has secrets_prefix, secrets_nested_delimiter, etc. to configure secrets and env vars separately
  • Pure Python thin wrapper over standard EnvSettingsSource
  • No third party dependencies except pydantic-settings
  • 100% test coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants