Detect and reject unknown attributes in "connection" blocks #13400
Conversation
Since the validation of connection blocks is delegated to the communicator selected by "type", we were not previously doing any validation of the attribute names in these blocks until running provisioners during apply. Proper validation here requires us to already have the instance state, since the final connection info is a merge of values provided in config with values assigned automatically by the resource. However, we can do some basic name validation to catch typos during the validation pass, even though semantic validation and checking for missing attributes will still wait until the provisioner is instantiated. This fixes #6582 as much as we reasonably can.
| @@ -101,6 +121,64 @@ func (n *EvalValidateProvisioner) Eval(ctx EvalContext) (interface{}, error) { | |||
| } | |||
| } | |||
|
|
|||
| func (n *EvalValidateProvisioner) validateConnConfig(connConfig *ResourceConfig) (warns []string, errs []error) { | |||
There was a problem hiding this comment.
Can we move this functionality into the communicator package, or at least somehow get the connConfigSuperset definition in there? It wold be nice to have some logical locality next time we're trying to figure out why a new communicator fails validation ;)
There was a problem hiding this comment.
A fair point... I was a little unsure where to place this because the ownership of the "schema" of this block is rather unclear anyway, with the documentation implying that there are some "global attributes" which are in fact just supported by all the communicators by convention.
However, we do have the global communicator package that has the function for instantiating the specific communicators, so assuming that's what you were referring to here then I agree, I'll move it over there.
There was a problem hiding this comment.
Ugh... it turns out that this is more problematic than I initially thought because the communicator package already depends on the terraform package and so we can't call into it from terraform without creating a dependency cycle.
We've got away with this so far because it's the provisioners that call into communicator and the plugin indirection prevents Go from seeing it as a circular dependency.
I think there is a path here where the communicator interface can be adjusted so that it receives directly the connection info as a map[string]interface{} rather than getting a terraform.InstanceState and pulling it out of there itself, but that's a bit beyond the scope of what I wanted to do for this bug.
Do you think it's reasonable to move ahead with what I had here and address this later, or should I just shelve this for the moment and come back to it when there's more time for such refactoring?
There was a problem hiding this comment.
Hmm, that's what I was afraid of.
connConfig.Config is already just a map[string]interface{}; couldn't this just be a function like
communicator.ValidateConnConfig(cfg map[string]interface{}) error
and pass it ResourceConfig.Config?
There was a problem hiding this comment.
That's exactly what I tried, but then that adds a dependency from terraform to communicator, which is problematic. In order for us to call from terraform to communicator we need to eliminate all of the terraform dependencies from communicator itself, which I think is possible (it seems to use it only because currently communicators get passed a full terraform.InstanceState on init) but a pretty disruptive interface change for a relatively-minor UX improvement.
There was a problem hiding this comment.
🤦♂️ sorry, of course, I was worrying about the inverse.
Yeah, lets have this live here for now.
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Since the validation of connection blocks is delegated to the communicator selected by
type, we were not previously doing any validation of the attribute names in these blocks until running provisioners during apply.Proper validation here requires us to already have the instance state, since the final connection info is a merge of values provided in config with values assigned automatically by the resource. However, we can do some basic name validation to catch typos during the validation pass, even though semantic validation and checking for missing attributes will still wait until the provisioner is instantiated.
This fixes #6582 as much as we reasonably can.