Add env var defaults and processing to server #145
Conversation
cmd/setec/setec.go
Outdated
| if serverArgs.KMSKeyName != "" { | ||
| if data, err := os.ReadFile(serverArgs.KMSKeyName); err == nil { | ||
| serverArgs.KMSKeyName = strings.TrimSpace(string(data)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I wasn't very consistent on this handling. There are several ideas here and I didn't see native support for it with the flax dependency
cmd/setec/setec.go
Outdated
|
|
||
| func runServer(env *command.Env) error { | ||
| // If KMSKeyName is a filepath, read the file contents and use that as the key name. This allows users to specify the KMS key name via a file, which can be useful in some deployment scenarios. | ||
| if serverArgs.KMSKeyName != "" { |
There was a problem hiding this comment.
My main concern with this approach is it introduces "magic" to the argument strings. I'd rather setec not get into the business of guessing whether the operator wants this literal text or some file that may or may not happen to be present in the filesystem (especially not based on its presence/absence).
Environment variables seem fine, though if we are going to use them, I'd like to give them distinctive names, e.g., SETEC_STATE_DIR, SETEC_HOSTNAME, etc., and document them in help text (e.g., as here).
But if we want to read configuration values from files, I'd be more comfortable adding a new opt-in flag to specify a config file that the operator can generate and explicitly set. (Format subject to discussion, but JSON or INI would be reasonable-ish choices).
I'd also prefer to require that either config comes from a config file, or from flags/environments, but never both, and report an error if any of the other flags are explicitly set when a config file is provided.
\cc @danderson in case you have other thoughts.
There was a problem hiding this comment.
I'm a fan of the config file idea. Would that include the env vars that are needed but are not currently taken as flags (TS_AUTHKEY, AWS env vars)?
There was a problem hiding this comment.
I was thinking primarily of the configurations for setec itself. The boundary is not sharp, but at some point I wonder if it's simpler to let the operator provide a startup script instead, which can read files, populate environment variables, etc.
The marginal utility of baking it into the CLI seems to drop off pretty quickly. Maybe TS_AUTHKEY make sense, but if we add support for GCP key management or other stores, I'd really rather not start carrying around a bunch of ad-hoc API-specific bits. A config file could have a ball of unstructured key-value pairs to stuff into the environment, but at that point maybe it's better to just have that be a script instead.
Having environments for the flags seems unambiguously OK to me, though.
There was a problem hiding this comment.
That makes sense. For my use case, I'm not sure I've quite got the hang of adding additional binaries to a distroless container to (cleanly) do what you suggested in a containerized environment. I think for now I'll modify the PR to just add the env vars and keep the bespoke handling on my fork. I'll see about revisiting this with a cleaner pass later
creachadair
left a comment
creachadair
left a comment
There was a problem hiding this comment.
Apologies for the delay! I apparently missed the notification for this PR getting updated. A couple small things only.
Co-authored-by: M. J. Fromberger <[email protected]>
No worries, I appreciate the review! |
2 participants
Getting
setecinto a distroless container was a tricky process of injecting the secrets it needs to start up. Adding the following env var handling simplifies it, at least as far as the settings I was using.tailscale/tailscale'supcommand has some of its own handling of files-as-env-vars, so I'm not in love with what I did here. But it does work.https://github.com/tailscale/tailscale/blob/main/cmd/tailscale/cli/up.go#L205