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

support emerging _FILE convention #130

Open
bradrydzewski opened this issue Aug 20, 2018 · 13 comments · May be fixed by #210
Open

support emerging _FILE convention #130

bradrydzewski opened this issue Aug 20, 2018 · 13 comments · May be fixed by #210

Comments

@bradrydzewski
Copy link

I was wondering if there was interest in supporting an emerging pattern in the container world:

As an alternative to passing sensitive information via environment variables, _FILE may be appended to the previously listed environment variables, causing the initialization script to load the values for those variables from files present in the container.

For example, the POSTGRES_PASSWORD environment variable can also be sourced from a file, where POSTGRES_PASSWORD_FILE provides the path to the file. This pattern is very useful when you are targeting kubernetes and swarm environments, that provide the ability to source secrets from a mounted file.

This could be supported with an optional tag:

type Specification struct {
    Password string `envconfig:"POSTGRES_PASSWORD", file:"true"`
}

I would be open to sending a pull request, if there was interest in this capability.

@starkers
Copy link

starkers commented Dec 9, 2018

Just wanted to say that this would be great!

@teepark
Copy link
Collaborator

teepark commented May 24, 2019

I can totally see the usefulness. For the time being you could definitely implement this on individual fields with a custom decoder that goes and reads the file itself.

That's been my answer to a lot of feature requests, and it's a nice way to keep envconfig itself simple -- get opinionated on the basics, and also offer a total swiss army knife. Not sure where I stand on this though, seems useful enough that first-class support makes a sort of sense too.

@TonyPythoneer
Copy link

I think @teepark had answered @bradrydzewski 's question.
This issue should be closed.

@bradrydzewski
Copy link
Author

bradrydzewski commented Aug 19, 2019

the answer also indicates first class support could make sense. If that is the case I would be happy to send a pull request. If not, I completely understand and this issue can be closed.

@TonyPythoneer
Copy link

@bradrydzewski
Copy link
Author

I understand this can be achieved with custom decoders. I am interested in contributing a patch to support this natively. If this is out-of-scope for the project I completely understand and this can be closed. We need this feature and we use this library for 100+ repositories ,and are therefore comfortable maintaining our own fork if needed.

@TonyPythoneer
Copy link

TonyPythoneer commented Aug 20, 2019

@bradrydzewski

I can feel your enthusiasm from the comment.

Could you talk about the advantage and disadvantage if you want to send the PR.

I expect I can hear about the feature brings value except for the technical layer, how about it effect this community? If it encounters bottleneck, how does it maintain and optimize in the future?

@TonyPythoneer
Copy link

Personally, I am sorry to talk about that.
We are engineers. We should understand a lib of responsibility.

If there is a core goal or focused solution goal, we think we can define the behavior and functionality to complete the target.

If it does thing is not on the track, we should stop that and introspect, maybe it's over-engineering.

I am not sure, it's correct or wrong. But at least, we have Github this platform provides discussion chance to make engineers communicate.

Please use here and express your idea. Before taking actions, I want to know how you think.

@decentral1se
Copy link

Just to chime in, I am very much in favour of this, it would be fantastic! Docker swarm is not going to be dying anytime soon it seems, so this request will probably only be coming more and more ;)

@czeigler
Copy link

This is a great idea and I'd love to see @bradrydzewski's PR get merged.

@bradrydzewski
Copy link
Author

bradrydzewski commented Jun 18, 2020

@teepark what about an option to provide a custom lookup function or interface? It would require some minor refactoring but people could add their own custom lookup and their own custom tags, thus giving a path to customization without having to expand the scope of this repository (or maintain a fork).

p := envconfig.New()
p = envconfig.NewPrefix("FOO_") // alternate constructor
p.Lookup = func(info VarInfo) (bool, string) { /* custom logic */ }
p.Process(spec)

Note that this would not replace or remove the existing global Process and MustProcess functions. These global functions would invoke NewPrefix under the hood. This is similar to how the json package has global functions, that create encoders and decoders under the hood.

@kelseyhightower
Copy link
Owner

I think it's important to limit the scope of this library. If we don't stop at files, then soon people will want us to load data from secrets managers and key/value stores. Custom lookup functions make sense to me and I think it's the right path forward.

@m90
Copy link

m90 commented Sep 25, 2020

I'll ask a very basic question: How would the scenario described in the original issue (i.e. picking either POSTGRES_PASSWORD_FILE or POSTGRES_PASSWORD) look like when implemented using a custom decoder function?

Maybe I am missing something, but from what I understand a custom decoder would only allow me to check whether the provided env value might be a file and then try to read it (which means I'd have to define POSTGRES_PASSWORD=/run/secrets/postgrespassword). But it wouldn't let me retrieve the configuration value from either the one or the other source? The custom decoder doesn't even receive the key that has been used for looking up the passed value.

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

Successfully merging a pull request may close this issue.

8 participants