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

jsonconfig: Improve provider configuration output #30138

Merged
merged 3 commits into from Feb 10, 2022

Conversation

alisdair
Copy link
Member

@alisdair alisdair commented Dec 10, 2021

When rendering configuration as JSON, we have a single map of provider configurations at the top level, since these are globally applicable. Each resource has an opaque key into this map which points at the configuration data for the provider.

This commit fixes two bugs in this implementation:

  • Resources in non-root modules had an invalid provider config key, which meant that there was never a valid reference to the provider config block. These keys were prefixed with the local module name instead of the path to the module. This is now corrected.

  • Modules with passed provider configs would point to either an empty provider config block or one which is not present at all. This has been fixed so that these resources point to the provider config block from the calling module (or wherever up the module tree it was originally defined).

We also add a "full_name" key-value pair to the provider config block, with the entire fully-qualified provider name including hostname and namespace.

Fixes #30113. Will require a small corresponding update to terraform-json.

Comment on lines 448 to 451
"providers": {
"aws": "aws",
"aws.secondary": "aws.foo"
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be helpful for us to proactively expand this to be the underlying full provider address, so that a consumer doesn't need to reimplement the handling of the local provider name indirection?

Admittedly it does seem like that would make the data structure a bit more "complicated", so I'm not sure but I think we've typically leaned towards making these JSON outputs present the interpreted results of Terraform's work rather than just the raw input, at least where it's been practical to do so:

  "providers": {
    "registry.terraform.io/hashicorp/aws": {
      "": "",
      "secondary": "foo"
    }
  }

I'm not super happy with the above in particular because using "" to represent "default" makes the result look bizarre in the default-to-default case above. Might be better to just make it a flat array of objects with provider/caller_alias/callee_alias properties instead, so it's a bit more self-describing, but the details of how exactly to serialize it are less important than whether including the full provider source address would make the motivating use-case easier to achieve, which I'm not sure about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand the motivation for using the full provider address here.

In the use case which triggered this proposed change, the motivation was to be able to determine the provider config for a resource which is using a provider alias. That doesn't require knowing what the full address of the provider is, as at the moment it seems that nothing in jsonconfig deals with full provider addresses at all.

That said, your question did make me think more concretely about the proposed changes, and I don't think they're as useful as they ought to be. For the use cases I can imagine, we want the keys in the new map to match the resource's opaque provider_config_key, so that the consumer can look up the map directly without parsing the resource type to determine the provider name. I think we might similarly need the values in the new map to correspond to opaque keys, although I'm not yet sure how to make that useful for nested modules. Still investigating.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what initially got me thinking about this was noticing that in resource objects we have provider_name set to the fully-qualified provider name. However, on second look I realize I was misreading what I referred to and was accidentally looking at the planned changes rather than the configuration.

However, I do now see the indirection through the provider_config map you mentioned, and so I suppose if we were going to add the fully-qualified provider address somewhere it would probably be best recorded as an additional property of the objects inside that map. However, that's clearly outside of the scope of what you're trying to do here, so that's just a thought for the future, in case there's e.g. a use-case for knowing which provider's schema to use when interpreting the configuration expressions.

I agree with your assessment that ideally this new addition would also use the provider_config key format. If memory serves, we intentionally hoisted the provider configurations out like that to reflect the fact that provider configurations are a global thing which can span across modules, even though each module has a different "view" of them, and so I agree that it does seem kinda weird now to think about how to express the translation between those two "views" in the same terms, since it's the same provider configuration on both sides, and just a different name for it in each module's namespace.

I've not thought super deeply about this, and I'm not intending to impose any particular design on you, but as I was briefly considering this I did ponder if it might be better to make the modules themselves have a mapping from the local address as they see it to the key in the provider_config map, rather than presenting it as part of the call. That is, a sibling property of module_calls which is a map from local addresses to opaque provider_config keys covering all of the provider configurations that are visible in that module's namespace, regardless of whether they were inherited or passed explicitly:

{
  "module_calls": { (unchanged) },
  "provider_configs": {
    "aws": "opaque_aws_thing_1",
    "aws.foo": "opaque_aws_thing_2"
  }
}

I'm still not 100% sure I'm understanding the underlying use-case, but this seems like it would allow answering the question of "which configuration does aws.foo refer to inside this module". Perhaps inverting this mapping so that the opaque thing is the key and the local address is the value would suit it better, since in that issue the story seems to be about starting with a provider_config_key from a resource. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for continuing to engage on this!

I have thought about this further and ended up taking a completely different approach. The PR description & commit comment go into more detail, but the gist is:

  • Fix the incorrectly calculated opaque keys for resources in non-root modules—this seems to have been broken for a while
  • While building the output, keep track of passed provider configs and their parent values
  • For resources in modules with passed provider configs, flatten out this linked list so that they point directly at the original provider configuration block in the ancestor module

I believe this addresses the linked use case ("which region was this resource created in?") without any compatibility breaks. The change to the opaque provider config key values is a bug fix, and I believe that the previous values were useless unless the consumer disregarded the warning that they were to be considered opaque.

@alisdair alisdair force-pushed the alisdair/json-module-call-providers-mapping branch from 4bf7240 to c834461 Compare January 28, 2022 15:35
@alisdair alisdair changed the title jsonconfig: Add provider configs to module calls jsonconfig: Improve provider configuration output Jan 28, 2022
@@ -68,7 +68,7 @@
"mode": "managed",
"type": "test_instance",
"name": "test",
"provider_config_key": "more:test",
"provider_config_key": "module.my_module.module.more:test",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example of how the previous version of these opaque keys were incorrect. If two cousin modules were named "more", these values would clash, and either way there would never be an entry in the provider config map for "more:test".

Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new design makes a lot of sense to me... I like that it retains the original intent (bugs notwithstanding) of freeing consumers of this data from having to reimplement the complex resource-to-provider association logic, and so gives a direct mapping from resource to provider configuration without going via all of the indirection we allow in the configuration language.

Given that decision, I also agree that it makes sense for the information in the providers argument of a modules block to not be included explicitly, because the effect of those is now correctly captured at the resource level, regardless of how the module authors achieved that mapping.

When rendering configuration as JSON, we have a single map of provider
configurations at the top level, since these are globally applicable.
Each resource has an opaque key into this map which points at the
configuration data for the provider.

This commit fixes two bugs in this implementation:

- Resources in non-root modules had an invalid provider config key,
  which meant that there was never a valid reference to the provider
  config block. These keys were prefixed with the local module name
  instead of the path to the module. This is now corrected.

- Modules with passed provider configs would point to either an empty
  provider config block or one which is not present at all. This has
  been fixed so that these resources point to the provider config block
  from the calling module (or wherever up the module tree it was
  originally defined).

We also add a "full_name" key-value pair to the provider config block,
with the entire fully-qualified provider name including hostname and
namespace.
Instead of manually updating every JSON output test fixture when we
change the format version, disregard any differences when testing.
The JSON plan configuration data now includes a `full_name` field for
providers. This addition warrants a backwards compatible increment to
the version number.
@alisdair alisdair force-pushed the alisdair/json-module-call-providers-mapping branch from c834461 to fe8183c Compare February 7, 2022 20:06
@alisdair alisdair merged commit d801827 into main Feb 10, 2022
@alisdair alisdair deleted the alisdair/json-module-call-providers-mapping branch February 10, 2022 15:33
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

terraform show does not properly associate resource configuration with provider when proxied
2 participants