Confusing priority of vault_url value sources

[kazauwa]

I encountered the problem that the vault_url value specified in class Config takes priority over the VAULT_ADDR environment variable. In my head, it should be the opposite: we define a default value in the code, but we can override it externally by setting an environment variable (see 12-factor app). Although, I found out that this can be achieved by setting vault_url to None, I still find this confusing.

Luckily, it can be easily fixed by rearranging the order of reading vault URL sources in _get_authenticated_vault_client():

_vault_url: Optional[str] = None
if getattr(settings.__config__, "vault_url", None) is not None:
    _vault_url = settings.__config__.vault_url  # type: ignore
    logger.debug(f"Found Vault Address '{_vault_url}' in Config")
if "VAULT_ADDR" in os.environ:
    _vault_url = os.environ["VAULT_ADDR"]
    logger.debug(f"Found Vault Address '{_vault_url}' in environment variables")
if _vault_url is None:
    raise VaultParameterError("No URL provided to connect to Vault")

I can make a pull request with the change and cover them with tests if that's okay with you.

[nymous]

Hi @kazauwa! That is indeed something that bugged me from the beginning, and one of the biggest reasons pydantic-vault is still in 0.X. I was not sure about the priority to give to environment variables over values declared in the code. If I remember correctly, I followed the same order as the "field value priority" in Pydantic (yes that's Pydantic 1 docs, but it's the same in 2.x), where values defined when initializing the class override values from the environment. However I can see it being different in this case as we are "configuring" the configuration, so following the 12-factor order could make more sense.

As you can see, I am still torn on this :sweat_smile: Maybe other users of pydantic-vault can weigh in on this? @ingvaldlorentzen @jonasKs @area42 @aleksey925 @vladimir-kotikov @raider444 @yanbin-pan (sorry for the ping ^^' but I know you use/have used the library) Could you vote on this message?

• :+1: if you want to change the priority (environment variables will override what is written in the Config class)
• :-1: if you want to keep the current priority (what is written in the Config class is what counts, environmnent variables do not override)

You can always comment below if you want to express a more nuanced opinion ^^

In any case, sorry for the delay @kazauwa :grimacing: If this goes through (or I decide that in the end I want the change) I would gladly accept a PR for this, if you are still motivated.

[vladimir-kotikov]

If we're going to refer to pydantic's list of priorities then I personally would say that defining vault_url in Config class is more like number 5 because it essentialy bakes the parameter into class definition and hence need to have the lowest priority, otherwise you won't be able to override it.

So I basically agree with Igor - that's going to be a good candidate to be changed.

[nymous]

I agree that it does make sense.

Also by giving the link to Pydantic docs I didn't want to say "this is how we should do things", I was just trying to remember where my decision was coming from :sweat_smile:

[ingvaldlorentzen]

I agree that environment variables should override what is written in the Config class. We've "implemented" this ourselves with: vault_address = os.environ.get('VAULT_ADDR', 'https://default-vault.example')

[kazauwa]

Hi everyone! Thanks for participating! I'm glad we're on the same page with this. I'm still up for the task if you're happy with the change.

[nymous]

Thanks everyone! I am now convinced that this should change, for all environment variables used in the Config class. @kazauwa, I would gladly accept your contribution ^^ I wanted to release #24 first because I knew it would add a new env variable, so I didn't reply earlier. But this would mark the "great 1.0 release"! :tada:

[nymous]

This is now released in 1.0.0, thank you again @kazauwa! :rocket: :fireworks: