Allow dynamic configuration as a default for input variables #2889
Conversation
| // For dynamic types we don't do conversion because we don't yet | ||
| // have the value. For now we require v.Type to be string so that | ||
| // the user isn't surprised by anything. Users can use explicit | ||
| // type conversion such as `tonumber`. |
There was a problem hiding this comment.
I was wondering how you'd handle types - requiring an hcl conversion is a good idea for now I think, and we can do fancier typing in the future if we want to. I was kind of dreading handling the object type.
There was a problem hiding this comment.
I think strings are the way to go.
For numerics, HCL is arbitrary precision numbers too so any floats will work fine and easy to convert.
For boolean, easy to use tobool (uses strconv.parseBool)
For objects, easy to use yamldecode jsondecode jsonnetdecode etc. depending on format, and easy to add new format support if they have it.
| @@ -18,6 +18,7 @@ import ( | |||
| "github.com/hashicorp/waypoint-plugin-sdk/component" | |||
| "github.com/hashicorp/waypoint-plugin-sdk/datadir" | |||
| "github.com/hashicorp/waypoint-plugin-sdk/terminal" | |||
| "github.com/hashicorp/waypoint/internal/appconfig" | |||
There was a problem hiding this comment.
Puritanical point: Now that the configsourcers are called in the variable system, it feels a little weird for their core logic to still live inside the appconfig package. It feels better for me for that logic to live somewhere else, and be invoked both inside variables and appconfig. Not a big deal though.
There was a problem hiding this comment.
Is this just a package naming thing? I think the role of appconfig package is still totally right, we can maybe just rename the package to something like externalconfig or something.
There was a problem hiding this comment.
Maybe a better example of the weirdness is that the variable system has to pretend that it's receiving config files (here and here), when really it's just getting string values back from the configsourcer plugins.
I think we might eventually want a new externalconfig or dynamicvalues package that invokes the plugins, and both the variables and appconfig could use it.
There was a problem hiding this comment.
I agree the files thing is weird, @evanphx noted that too. I think if we just change it to have a way to accept strings directly I still think this package is the right structure. It is requesting a "config var" in my mind.
|
|
||
| // Build our watcher | ||
| ch := make(chan *appconfig.UpdatedConfig) | ||
| w, err := appconfig.NewWatcher(append( |
There was a problem hiding this comment.
For posterity: If we wind up having to do this again down the road, it might make sense to make a "one-shot" configsourcer invoke mode, so you don't need to spool up a whole watcher and associated channels to get one value from the plugins. The watcher might even be able to call that one-shot logic internally.
There was a problem hiding this comment.
Yeah, on my mind too.
| // If we're a local runner, we can't support this. We don't | ||
| // support dynamic config sourcing on local runners since it | ||
| // requires config sourcing plugins, auth, etc. | ||
| if r.local { |
There was a problem hiding this comment.
I think this ok for now. I'd be surprised if there are many users sophisticated enough to be using these dynamic variables, but haven't set up the remote server/runner infra yet.
There was a problem hiding this comment.
Exactly, so I think this is good forever. This just gives people a nice error message.
| // | ||
| // We also do this if the runner is NOT local, in case it is processing | ||
| // jobs that are using config variables with dynamic defaults. | ||
| _, isLocal := record.Kind.(*pb.Runner_Local_) |
There was a problem hiding this comment.
A little spooky that this security check depends on the runner not lying to us about what type it is. I think we'll address this in the future with a more formal runner registration/approval process though?
| // Do not allow dynamic values as default values since they aren't valid. | ||
| // Dynamic values should be evaluated and overridden by LoadDynamicDefaults | ||
| // and provided via pbvars. If not, then an unset error will be created. | ||
| if def.Default != nil && def.Default.Value.Type() == dynamic.Type { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
Yeah not a big deal, just makes UX nicer. Since the dynamic type is an encapsulated value it can't be used for anything so its basically a poison pill. If anyone tried to use it for any values it'd just give an obscure HCL type error message. This just helps prevent that.
|
I seem to understand that this is a fix for #1553, is that correct? |
| // This is set to true on purpose because it forces the appconfig | ||
| // watcher to give us an easier to consume format (struct vs | ||
| // array of key=value strings for env vars). | ||
| NameIsPath: true, |
There was a problem hiding this comment.
At some point, we should make that flatten logic that NameIsPath implies selectable more obviously.
There was a problem hiding this comment.
Yes, agreed, a bit of a hack.
This allows the
defaultvalue for avariablestanza to use theconfigdynamic()function to source default values from any supported config source. You can now, for example, source data from Vault to use within your waypoint.hcl file. Previously, configdynamic could only be used to set application configuration (env vars and files that appear only to your app).One limitation (by design): This only works with remote runners. This is the only way we can guarantee consistency in plugin versions and protects against some secrets spilling onto local machines. However, you may use a local runner if you manually override the default value with something like
-varor awpvarsfile. You only cannot use local mode if the dynamic value must be fetched.nullConfig SourceThis PR also introduces a new plugin
nullwith a single component for now: a config sourcer. This is used for testing but will be generally available for people to use. I think this is useful for experimentation and learning how config sourcing works without having to deal with the complexities of a remote system.I mixed it with this PR because I used this to manually test (in addition to the unit tests) that this works.
Known Issues or Improvements
It's becoming harder and harder to know what variable values are used for a job. This exacerbates that but doesn't introduce this problem. In the future, we want to send back information about where variable values are coming from (and perhaps what they are).