Add support for the VAULT_NAMESPACE environment variable

[mporrato]

If the Client class is instantiated without the namespace argument, use the content of the VAULT_NAMESPACE environment variable instead, mimicking the behaviour of the official vault CLI tool.

[briantist]

This could be considered a breaking change, code that worked before could stop working if VAULT_NAMESPACE was set before erroneously.

I personally think that this being a library, it should be infer these kinds of things (I know that it already does for some of the other CLI env vars though), and it should be up to the consumer of the library to decide.

[mporrato]

Thanks for the feedback.
I don't think this can be considered a breaking change, because for an application to work correctly at this time with a namespace, it has to be specified when creating the Client object; with this patch the namespace passed to the Client constructor would override the content of the environment variable so it would not change the behaviour of a currently working application.

[briantist]

Thanks for the feedback. I don't think this can be considered a breaking change, because for an application to work correctly at this time with a namespace, it has to be specified when creating the Client object; with this patch the namespace passed to the Client constructor would override the content of the environment variable so it would not change the behaviour of a currently working application.

I was thinking of the case where a namespace is not in use at this time, but for some reason (unbeknownst) the VAULT_NAMESPACE environment variable is set to a value (perhaps it is old, or was used in testing). After the proposed change, any software using hvac would suddenly pick up this value and try to use it, changing the behavior.

As with all changes, the definition of what is breaking is not always clear, and varies based on opinion, so I am offering a scenario where that would be the case.

---

Other than the question of it being considered sufficiently breaking or not, I still stand by the view that having a library like this share environment variables with the CLI does more harm than good, and that that should be something the consuming software decides in most cases.

[Tylerlhess]

So why are we doing this for certificates then?

https://github.com/hvac/hvac/blob/develop/hvac/constants/client.py

I see the value in letting software decide. I have also fought developers to move hundreds of apps from wrapped token based auth to vault agent approle based auth for over a year and it still not being done. To me having a way to have software not have to explicitly decide has decidedly greater benefit, especially in this age of containerization.
It's one thing to worry about mainframes with decade long uptimes using software with what could be a breaking change. It is completely different when the software your api library is built to leverage is designed to be used for limited use passwords in containerized workloads.

[briantist]

So why are we doing this for certificates then?

https://github.com/hvac/hvac/blob/develop/hvac/constants/client.py

I can't answer for the maintainers, but it seems as though the idea was to try and get some re-use out of the environment variables used with the CLI, especially ones you wouldn't often change like VAULT_ADDR. The library also has code to read the default location of the token file that the CLI writes.

I see the value in letting software decide. I have also fought developers to move hundreds of apps from wrapped token based auth to vault agent approle based auth for over a year and it still not being done. To me having a way to have software not have to explicitly decide has decidedly greater benefit, especially in this age of containerization. It's one thing to worry about mainframes with decade long uptimes using software with what could be a breaking change. It is completely different when the software your api library is built to leverage is designed to be used for limited use passwords in containerized workloads.

The use case you describe is one possible use for Vault; it serves many other uses as well. And this library is also for many different kinds of software. I don't think it's much burden for a given project to decide to read an environment variable and pass it into a library parameter; on the other hand, if your software does not want that, and the library reads it, it's a lot harder to avoid, and it can come as a surprise, something you may not find until a bug report from a confused/frustrated end user or customer.

But to be clear, this is my opinionated stance after having used the library for a while. At first, using the env vars made sense and I wouldn't have opposed them. After some time and experience, my feelings have gone the other way.

I'm sure others will have different experiences and having the vars is better for them.

[briantist]

Hi @mporrato ,

The hvac project is simplifying its developer workflow and removing the develop branch. Please be aware for future PRs that the project's default branch is now main.

Since your PR was opened before this change, I have:

• updated your PR to merge into the project's main branch
• rebased your branch against main and force-pushed it (please pull before making any further changes)

Re: the PR, my previous comments were before I was a maintainer on the project, but my view has mostly remained the same. See also the discussion in #772 .

I believe we'll look to avoid adding new CLI environment variables, and may even remove some over time. But similar to that PR, I will keep this open until there is a definitive proposal/decision for the project.

Thanks for opening this and I'm sorry it's been open for so long.

[codecov]

Codecov Report

Merging #772 (068cfb4) into main (31aca14) will increase coverage by 2.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #772      +/-   ##
==========================================
+ Coverage   81.98%   84.00%   +2.02%     
==========================================
  Files          65       65              
  Lines        3019     3020       +1     
==========================================
+ Hits         2475     2537      +62     
+ Misses        544      483      -61     

Impacted Files	Coverage Δ
hvac/v1/__init__.py	88.60% <100.00%> (+0.07%)	⬆️

... and 4 files with indirect coverage changes