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

configs: Fix provider lookup local name mismatch #26871

Merged
merged 1 commit into from
Nov 11, 2020

Conversation

alisdair
Copy link
Contributor

When a resource has no provider argument specified, its provider is derived from the implied provider type based on the resource type. For example, a boop_instance resource has an implied provider local name of boop. Correspondingly, its provider configuration is specified with a provider "boop" block.

However, users can use the required_providers configuration to give a different local name to a given provider than its defined type. For example, a provider may be published at foobar/beep, but provide resources such as boop_instance. The most convenient way to use this provider is with a required_providers map:

terraform {
  required_providers {
    boop = {
      source = "foobar/beep"
    }
  }
}

Once that local name is defined, it is used for provider configuration (a provider "boop" block, not provider "beep"). It should also be used when looking up a resource's provider configuration or provider.

This commit fixes a bug with this edge case, where previously we were looking up the local provider configuration block using the resource's assigned provider type. Instead, if no provider argument is specified, we should be using the implied provider type, as that is what binds the resource to the local provider configuration.

Specifically fixes #26868, where a onepassword provider offering resources like onepassword_item_login has been published at milosbackonja/1password. With this commit, the provider mapping in the original report works as expected (and documented). More details and diagnosis over in the issue.


Assuming this passes review, I think this may be a candidate for an 0.14 backport, despite the very close RC. Any concerns with backporting?

When a resource has no `provider` argument specified, its provider is
derived from the implied provider type based on the resource type. For
example, a `boop_instance` resource has an implied provider local name
of `boop`. Correspondingly, its provider configuration is specified with
a `provider "boop"` block.

However, users can use the `required_providers` configuration to give a
different local name to a given provider than its defined type. For
example, a provider may be published at `foobar/beep`, but provide
resources such as `boop_instance`. The most convenient way to use this
provider is with a `required_providers` map:

terraform {
  required_providers {
    boop = {
      source = "foobar/beep"
    }
  }
}

Once that local name is defined, it is used for provider configuration
(a `provider "boop"` block, not `provider "beep"`). It should also be
used when looking up a resource's provider configuration or provider.

This commit fixes a bug with this edge case, where previously we were
looking up the local provider configuration block using the resource's
assigned provider type. Instead, if no provider argument is specified,
we should be using the implied provider type, as that is what binds the
resource to the local provider configuration.
@alisdair alisdair added config 0.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged labels Nov 10, 2020
@alisdair alisdair requested a review from a team November 10, 2020 20:29
@alisdair alisdair self-assigned this Nov 10, 2020
@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #26871 (45671a3) into master (7c98be9) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted Files Coverage Δ
configs/resource.go 80.46% <0.00%> (ø)
terraform/eval_apply.go 73.27% <0.00%> (-0.61%) ⬇️
terraform/evaluate.go 52.89% <0.00%> (-0.42%) ⬇️
terraform/evaluate_valid.go 79.57% <0.00%> (+5.63%) ⬆️

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Oof! Good find! Every time I think we found all of those old references to r.Provider.Type in place of the ImpliedProvider() func, another one pops up.

@alisdair alisdair merged commit fce77f2 into master Nov 11, 2020
@alisdair alisdair deleted the alisdair/fix-provider-lookup-local-name-mismatch branch November 11, 2020 15:16
@ghost
Copy link

ghost commented Dec 12, 2020

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 ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
0.14-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged config
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1Password Data Source Implicit Provider Inference Doesn't Work
2 participants