Support _FILE based configuration values for all config keys

[m90]

To configure this tool, a lot of potentially sensitive and powerful credentials and tokens need to be passed. Right now, these have to be passed as env vars. Docker Swarm supports "secrets" which allows putting credentials into files made available to the container at runtime.

This was added for S3 credentials in #161, but not for anything else.

Unfortunately package envconfig which is used here does not (and will not) support this out of the box: kelseyhightower/envconfig#130

Supporting this pattern for all values would be a very nice addition though, so I can see a few ways forward:

• find out if there is a fork of package envconfig that supports this (it's mostly dormant anyways)
• fork package envconfig and add the functionality ourselves
• find a different package for handling configuration (this would need to be non-breaking)

[m90]

There seems to be https://github.com/johnstairs/pathenvconfig

[MaxJa4]

There seems to be https://github.com/johnstairs/pathenvconfig

Just had a look at that code. It doesn't allow to specify a custom name like envconfig:"EMAIL_SMTP_PASSWORD", which is needed to process names with multiple capitalized letters next to each other. There also seems to be a bug in the used regex code. Wouldn't use it.

I found this though: https://github.com/caarlos0/env
Looks promising, will try it out.

[m90]

The nice thing about https://github.com/johnstairs/pathenvconfig compared to https://github.com/caarlos0/env is that you don't need to duplicate configuration keys. It just automatically picks up _FILE values when they're there, falling back to plain env vars when they're not. Ideally, we'd just support that for everything and hiding the entire logic in the configuration library.

[MaxJa4]

That would be ideal, yes. But that particular package doesn't work with capitalized variable names like SMTP and while trying out the regex (to verify that the cap. variables do/don't work) I found a regex error, which should make it useless (unless the regex tool and I are wrong).
Anyway, maybe we can fork it ourselves and just fix those two issues?

[m90]

Question is whether it's easier to make env or envconfig do what we want. Right now, I'd lean towards envconfig as we'd only have to teach it a single trick. Then again, I still cannot believe someone else has done this yet.

[MaxJa4]

env is more actively maintained, but envconfig has worked well for a while in this project.
I already implemented env and it ran through the CI successfully here: cc4492d
Now I'm also checking out pathenvconfig just to be sure that it doesn't work for us.

Edit: pathenvconfig cannot parse AwsS3BucketName properly it seems...

[MaxJa4]

@m90 Would you prefer to fork it yourself or in the offen organization? Otherwise I will do that later today, as I would volunteer to add that to envconfig.

[m90]

Depends on if you wanna maintain it in case someone else starts using it :) Maybe it'd make sense if I forked it to the Offen org and made you a repo admin? Let me know if that works for you. We also use the package in offen/offen where this feature has also been requested, so that'd be nice as well.

As for the implementation itself, I think it would make sense to keep this as low-profile as possible (as not to create maintenance burden) and use an approach similar to the one described in this comment: kelseyhightower/envconfig#130 (comment)

I think you wouldn't even need the new constructor and instead allow overriding a package global lookup function (with the same signature as os.LookupEnv so it could stay the default) that could then check whether _FILE exists:

        envconfig.Lookup = func(key string) (bool, string) { /* custom logic */ }
	if err := envconfig.Process("", s.c); err != nil {
		return nil, fmt.Errorf("newScript: failed to process configuration values: %w", err)
	}

What do you think?

[MaxJa4]

Depends on if you wanna maintain it in case someone else starts using it :) Maybe it'd make sense if I forked it to the Offen org and made you a repo admin? Let me know if that works for you. We also use the package in offen/offen where this feature has also been requested, so that'd be nice as well.

Works for me! Makes sense if offen/offen uses it too.

What do you think?

You mean the logic for reading the file would be inside /* custom logic */ instead of inside the package? Meaning, that only lookupEnv can be changed by .Lookup from e.g. os.LookupEnv to that custom lookup function?

On a general note: Just had a look at the envconfig code. There is a lot of really old code, like specific hacks for go-1.5 or go-1.8... also a seemingly unused usage.go (need to verify that locally first though). I'd opt to remove all of this unused legacy stuff.

Also, there is an issue of just reading anything as file and using its contents which ends with _FILE... if you need to pass on a variable like our SSH_IDENTITY_FILE, it would read it instead of passing the path to the file to the container. So it would need to be possible to actively enable or disable (default behavior?) that automatic process of reading _FILE vars as files.

[m90]

You mean the logic for reading the file would be inside /* custom logic */ instead of inside the package? Meaning, that only lookupEnv can be changed by .Lookup from e.g. os.LookupEnv to that custom lookup function?

Yes, I think that should work and would create the least invasive change in the original package. Also, reading the comments (kelseyhightower/envconfig#130 (comment)), it should be the only change that at least has minimal changes of getting merged back upstream.

I'd opt to remove all of this unused legacy stuff.

Depends on if the goal is to get the change merged, or to keep the fork separate. Not sure right now. Another option would be to just copy over the parts of the package we need, change them and keep them in this repo.

Also, there is an issue of just reading anything as file and using its contents which ends with _FILE

Not sure if I understand every detail, but if you'd implement it using a custom lookup function like:

func fileAwareLookup(key string) (string, bool) {
	location, ok := os.LookupEnv(key + "_FILE")
	if ok {
		contents, err := os.ReadFile(location)
		if err != nil {
			return "", false
		}
		return string(contents), true
	}
	return os.LookupEnv(key)
}

SSH_IDENTITY_FILE would just become SSH_IDENTITY_FILE_FILE. That's maybe a bit confusing, but works and can be cushioned by some docs.

[MaxJa4]

Yes, I think that should work and would create the least invasive change in the original package. Also, reading the comments (kelseyhightower/envconfig#130 (comment)), it should be the only change that at least has minimal changes of getting merged back upstream.

I think the statement of the repo owner in this issue is very clear:

Custom lookup functions make sense to me and I think it's the right path forward.

So only adding the Lookup function would probably have good chances of being merged to the original repo which removes a "burden" on our side again (maintaining another project).

Depends on if the goal is to get the change merged, or to keep the fork separate. Not sure right now. Another option would be to just copy over the parts of the package we need, change them and keep them in this repo.

Good point. If we want it to get merged, then definitely don't change anything else than the Lookup function. If we just want to integrate it in the codebase here, we'd only need envconfig.go it seems... this way we could integrate the file lookup directly into the package, so the script.go doesn't get even larger and more bloated (although not by much anyway). Also, we could fix the SSH_IDENTITY_FILE issue (_FILE_FILE looks quite bad to me and feels like a workaround) by just introducing a simple handle like file (similar to required or default) if we want an env var to look for files and read then if present.

[m90]

Also, we could fix the SSH_IDENTITY_FILE issue

How'd you fix it other than not supporting passing this value in a secret/file? If this tool was to support the _FILE convention, I think not applying it to everything would be pretty confusing and even worse than _FILE_FILE.