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: fix keys for default providers #30525

Merged
merged 3 commits into from Feb 22, 2022
Merged

jsonconfig: fix keys for default providers #30525

merged 3 commits into from Feb 22, 2022

Conversation

nozaq
Copy link
Contributor

@nozaq nozaq commented Feb 15, 2022

Fixes provider config keys to reflect implicit provider inheritance/creation.

Issue 1. Implicit provider inheritance in child modules

The issue occurs when a parent module calls a child module without passing any providers. A child module implictly inherit default providers from the parent. From the implementation perspective, there could be three cases.

  1. Child module has a required_providers block for the implicitly inherited provider.
  2. Child module has no required_providers and provider blocks for the implicitly inherited provider.
  3. Child module has a required_providers block and it contains a provider with a local name which exists in the parent module, but those providers have different fully-qualified provider addresses. Child module won't inherit this mis-matching provider, then create a separate default provider.

Issue 2. Implicit provider creation in the root module

The issue occurs when a resource is defined without corresponding provider or required_providers block. Terraform implicitly creates a default provider in this case if no provider inheritance occurs, but currently terraform show -json does not produce a provider_config entry for these implicitly created providers.

Changes in this PR

This PR contains following changes.

  • Add a test case which contains the above cases.
  • Add a logic to find the source provider key for implicitly inherited providers (Issue 1).
  • Add a logic to generate provider_config entries for implicitly created providers (Issue 2).
  • Extract findSourceProviderKey() to reuse the same logic.

Fixes #30513.

Fixes provider config keys to reflect implicit provider inheritance.
Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks very much for submitting this!

The general shape of this makes sense to me, but I think there's a problem which prevents it from being correct. In the findSourceProviderKey function, we traverse up the module hierarchy looking for a provider configuration, matching by local name. Terraform's logic for doing this (in ProviderTransformer and in (AbsProviderConfig) Inherited) uses the fully-qualified provider address.

In practice, this means that this branch does not map configuration correctly in the (obscure) case where two modules use the same local name to refer to different providers.

I verified this locally with a root module using hashicorp/consul with an explicit provider config, calling a child module which is using a requirements block with alisdair/consul. Terraform behaviour was such that the child module's resources had no provider configuration, using only defaults, but the terraform show -json output had the provider config key pointing at the root module's provider configuration for hashicorp/consul.

At the moment I don't have any direct suggestions for how to fix this. I had hoped to be able to unify the provider resolution logic, such that we can determine exactly which provider configuration is used for a given resource using only the configuration (not the graph), but that seems like a much larger task than I'd hoped.

If you have any idea for an isolated change to the jsonconfig package which would allow it to operate correctly in this case, that would be excellent. I'll continue to think about this as well and update you if I have any ideas. Please let me know if anything I've said here is unclear!

@nozaq
Copy link
Contributor Author

nozaq commented Feb 18, 2022

@alisdair Thank you for the review and detailed exaplanation!

I was able to reproduce the case you mentioned, and added an example project here. This project uses hashicorp/null and nozaq/null, and I've confirmed that the state file and the JSON output point to different providers.

I'll take some time to think about this issue as well. I guess we would need to somehow...

  • avoid mapping providers with different fully-qualified addresses.
  • avoid inheriting the source provider when a requirement with the conflicting name exists.

@nozaq
Copy link
Contributor Author

nozaq commented Feb 18, 2022

@alisdair I've just pushed two additional commits. Please let me know if you have any comments/feedback!
(Also the PR description was updated to reflect the whole changes)

Fix for local name conflicts

The basic idea is to check if the fully-qualified provider address matches with the parent one during traversing up the module call tree. This commit consists of the following changes.

  • Add a test case for the local name conflict case.
  • Update findSourceProviderKey() to receive the fully-qualified provider address and return the parent key only when both the local name and the fully-qualified address match.
  • Fix the bug that findSourceProviderKey() could return the parent key which does not exist.

Fix for existing test cases

Apart from the case above, we needed to catch default providers implicitly created in the root module. this commit fixes the logic and affected test cases as well.

  • Generate a provider key for providers implicitly created in the root module.
  • Fix expected values of existing test cases defined in testdata/show-json/ directory to be consistent with this change. This affects most of existing test cases in that directory. I think this is sensible given the goal of jsonconfig: Improve provider configuration output #30138, but would like to hear your opinion :)

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

This is really great, thank you! 🙌 I really appreciate you breaking out the latter changes into two logical commits, that made it much easier to understand.

Thanks again for working on this!

@alisdair alisdair merged commit 70db495 into hashicorp:main Feb 22, 2022
@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.

@crw crw added the bug label Feb 22, 2022
@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 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implicitly passed default providers are not resolved to parent provider keys in JSON output
3 participants