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

provider block ignored for hashicorp provider aliased to another one #26556

Closed
radeksimko opened this issue Oct 12, 2020 · 2 comments · Fixed by #26566
Closed

provider block ignored for hashicorp provider aliased to another one #26556

radeksimko opened this issue Oct 12, 2020 · 2 comments · Fixed by #26566
Assignees
Labels
bug new new issue not yet triaged

Comments

@radeksimko
Copy link
Member

Terraform Version

v0.13.3

Terraform Configuration Files

terraform {
  required_providers {
    google = {
      source  = "hashicorp/aws"
      version = "~> 3.6.0"
    }
  }
}

provider "google" {
  region = "us-east-1"
}

resource "aws_vpc" "example" {
  provider = google
  cidr_block = "10.0.0.0/16"
}

Expected Behavior

$ terraform validate
Success! The configuration is valid.

Actual Behavior

$ terraform validate

Error: Missing required argument

The argument "region" is required, but was not set.

Steps to Reproduce

  1. terraform init
  2. terraform validate

cc @mildwonkey

@radeksimko radeksimko added bug new new issue not yet triaged labels Oct 12, 2020
@mildwonkey mildwonkey self-assigned this Oct 13, 2020
mildwonkey added a commit that referenced this issue Oct 13, 2020
The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes #26556

This will also be backported to the v0.13 branch.
mildwonkey added a commit that referenced this issue Oct 13, 2020
The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes #26556

This will also be backported to the v0.13 branch.
mildwonkey added a commit that referenced this issue Oct 13, 2020
The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes #26556

This will also be backported to the v0.13 branch.
mildwonkey added a commit that referenced this issue Oct 13, 2020
The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes #26556

This will also be backported to the v0.13 branch.
@mildwonkey
Copy link
Contributor

All better! There was bug in the provider transformer - the configs package had all the correct information (which is why I was initially confused: the tests in configs all prove that we were properly mapping a given FQN to the local config), but terraform wasn't using that information.

I back ported the fix into v0.13 so it will be in the next v0.13 release as well as v0.14 beta.

Using the same config:

terraform validate
Success! The configuration is valid.

@ghost
Copy link

ghost commented Nov 13, 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 Nov 13, 2020
jgiannuzzi pushed a commit to G-Research/terraform that referenced this issue Nov 16, 2020
* Cleanup after v0.13.2 release

* Correct typo

* Correct annotation in expressions.html

`false` section was missing a finishing "`"

* improvement github token

Signed-off-by: shaowenchen <[email protected]>

* don't connect module closers to destroy nodes

One of the tenants of the graph transformations is that resource destroy
nodes can only be ordered relative to other resources, and can't be
referenced directly. This was broken by the module close node which
naively connected to all module nodes, creating cycles in some cases
when edges are reversed from CreateBeforeDestroy.

* update CHANGELOG.md

* Fix bug for force push for backends besides the remote backend

In refactoring the force push code when implementing force push
for the Terraform remote backend, a bug was introduced that
meant that backends that don't implement the EnableForcePush
method would still have their state validated. This commit
fixes that, and adds test coverage such that there is a separate
mockRemoteClient that has this method implemented.

* Update CHANGELOG.md

* Cherry-picking data-source updates

* Small typo in the internals link to the provider network mirror protocol page

* Update locals.html.md

I've just wasted an hour to two hours trying to find the problem to finally realize that although I declare a "locals" block, it's referred to as "local".  This is pretty weird! So let's be be clear about this.

* website: Clarify `locals` vs. `local.thing` distinction

The subtle difference in keywords when creating vs. accessing locals trips
people up, even more than the "variable" vs. "var" distinction. It deserves its
own subheader on the page, plus a nice noisy callout.

* website: Fix copy-paste error

* Minor typo

* website: Document jsonencode entity escaping

* Added Google and Microsoft products to the summary for supported remote backends

* Backends ordering change

* not all plan action changes are provider bugs

A provider cannot influence CreateThenDelete vs DeleteThenCreate, so we
shouldn't attribute this to the provider in the error.

* add test for forced cbd with no other changes

If a resource is forced CreateBeforeDestroy from a dependent resource,
and that dependent has no changes, the plan is changed from
CreateThenDelete to DeleteThenCreate causing an apply error.

* apply the stored plan CreateThenDelete action

When applying a plan, a forced CreateBeforeDestroy may not be set during
the apply walk when downstream resources are no longer present in the
graph. We still need to stick to that plan, and both the
NodeApplyableResourceInstance EvalTree and the individual Eval nodes
need to operate on that planned value.

Ensure that we always check for an existing plan when determining
CreateBeforeDestroy status. This must happen in 2 different code paths
due to the eval node pattern currently in-use. Future refactoring may be
able to unify these code-paths to make this less fragile.

* update CHANGELOG.md

* vendor: upgrade go-userdirs dependency to fix crash [v0.13 backport] (hashicorp#26250)

* go mod tidy
* vendor: upgrade go-userdirs library

* Update CHANGELOG.md

* destroy nodes must rely on the state cbd status

When there is only a destroy node, there is no descendent to check for
"forced CBD", so we can only rely on the state to verify.

* improve failing test

Correct the initial test state, and expand the test to cause a cycle
without the previous fix.

* allow evaluation of 0 instances during apply

While this was easier to spot during plan, it is also possible to
evaluate resources with 0 instances during apply as well.

This doesn't effect the failure when scaling CBD instances, it only
changes the fact that the inconsistent value is no longer unknown.

* update CHANGELOG.md

* Release v0.13.3

* Cleanup after v0.13.3 release

* configs: More interpolation-only expr deprecations

Extend the deprecation for interpolation-only expressions to include
module calls, data sources, outputs, and locals.

* Update CHANGELOG.md

* command: Providers schema shows required_providers

The providers schema command is using the Config.ProviderTypes method,
which had not been kept up to date with the changes to provider
requirements detection made in Config.ProviderRequirements. This
resulted in any currently-unused providers being omitted from the
output.

This commit changes the ProviderTypes method to use the same underlying
logic as ProviderRequirements, which ensures that `required_providers`
blocks are taken into account.

Includes an integration test case to verify that this fixes the provider
schemas command bug.

* Update CHANGELOG.md

* Add deprecation warning for vendor provisioners

Adds a warning for chef, habitat, puppet, and salt-masterless
provisioners, and a corresponding test file to test for the warning

* Add docs notes for deprecation

* configs: Deprecate nested redundant interpolations

Previous deprecations only included direct assignment of template-only
expressions to arguments. That is, this was not deprecated:

locals {
  foo = ["${var.foo}"]
}

This commit uses hclsyntax.VisitAll to detect and show deprecations for
all template-only expressions, no matter how deep they are in a given
expression.

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* refresh cbd test

* update create_before_destroy when refreshing

In order to save any changes to lifecycle options, we need to record
those changes during refresh, otherwise they would only be updated when
there is a change in the resource to be applied.

* update CHANGELOG.md

* data source depends_on

A data source referencing another data source through depends_on should
not be forced to defer until apply. Data sources have no side effects,
so nothing should need to be applied. If the dependency has a
planned change due to a managed resource, the original data source will
also encounter that further down the list of dependencies.

This prevents a data source being read during plan for any reason from
causing other data sources to be deferred until apply. It does not
change the behavior noticeably in 0.14, but because 0.13 still had
separate refresh and plan phases which could read the data source, the
deferral could cause many things downstream to become unexpectedly
unknown until apply.

* update CHANGELOG.md

* Fix passing wrong file info & Add test coverage (hashicorp#26400)

Co-authored-by: Chanh Hua <[email protected]>

* Update CHANGELOG.md

* Updating codeowners with our deprecated status for tool-specific provisioners

* Release v0.13.4

* Cleanup after v0.13.4 release

* terraform: fix ProviderConfigTransformer

The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes hashicorp#26556

This will also be backported to the v0.13 branch.

* Update CHANGELOG.md

* cache DecoderSpec calls

DecoderSpec may be called many times, and deeply recursive calls are
expensive. Since we cannot synchronize the Blocks themselves due to them
being copied in parts of the code, we use a separate cache to store the
generated Specs.

* add benchmark used for DecoderSpec

Not a great benchmark, but record it here for posterity. Practical
testing with the AWS provider gives us a 98% speedup for simple plans.

* update CHANGELOG.md

* AzureRM backend: correctly lookup environment from metadata host

* add missing Context argument

* Update CHANGELOG.md

Adding the AzureRM backend fix

* Update CHANGELOG.md

Spelling mistake. For shame.

* Release v0.13.5

Co-authored-by: hashicorp-ci <[email protected]>
Co-authored-by: Matthias Bartelmeß <[email protected]>
Co-authored-by: Alex Novak <[email protected]>
Co-authored-by: shaowenchen <[email protected]>
Co-authored-by: James Bardin <[email protected]>
Co-authored-by: Pam Selle <[email protected]>
Co-authored-by: Pam Selle <[email protected]>
Co-authored-by: Petros Kolyvas <[email protected]>
Co-authored-by: Lance Kind <[email protected]>
Co-authored-by: Nick Fagerlund <[email protected]>
Co-authored-by: Kirill Zaborsky <[email protected]>
Co-authored-by: Alisdair McDiarmid <[email protected]>
Co-authored-by: Kristin Laemmert <[email protected]>
Co-authored-by: Chanh Hua <[email protected]>
Co-authored-by: kt <[email protected]>
Co-authored-by: Petros Kolyvas <[email protected]>
msherid2 added a commit to G-Research/terraform that referenced this issue Feb 12, 2021
* Cleanup after v0.13.2 release

* Correct typo

* Correct annotation in expressions.html

`false` section was missing a finishing "`"

* improvement github token

Signed-off-by: shaowenchen <[email protected]>

* don't connect module closers to destroy nodes

One of the tenants of the graph transformations is that resource destroy
nodes can only be ordered relative to other resources, and can't be
referenced directly. This was broken by the module close node which
naively connected to all module nodes, creating cycles in some cases
when edges are reversed from CreateBeforeDestroy.

* update CHANGELOG.md

* Fix bug for force push for backends besides the remote backend

In refactoring the force push code when implementing force push
for the Terraform remote backend, a bug was introduced that
meant that backends that don't implement the EnableForcePush
method would still have their state validated. This commit
fixes that, and adds test coverage such that there is a separate
mockRemoteClient that has this method implemented.

* Update CHANGELOG.md

* Cherry-picking data-source updates

* Small typo in the internals link to the provider network mirror protocol page

* Update locals.html.md

I've just wasted an hour to two hours trying to find the problem to finally realize that although I declare a "locals" block, it's referred to as "local".  This is pretty weird! So let's be be clear about this.

* website: Clarify `locals` vs. `local.thing` distinction

The subtle difference in keywords when creating vs. accessing locals trips
people up, even more than the "variable" vs. "var" distinction. It deserves its
own subheader on the page, plus a nice noisy callout.

* website: Fix copy-paste error

* Minor typo

* website: Document jsonencode entity escaping

* Added Google and Microsoft products to the summary for supported remote backends

* Backends ordering change

* not all plan action changes are provider bugs

A provider cannot influence CreateThenDelete vs DeleteThenCreate, so we
shouldn't attribute this to the provider in the error.

* add test for forced cbd with no other changes

If a resource is forced CreateBeforeDestroy from a dependent resource,
and that dependent has no changes, the plan is changed from
CreateThenDelete to DeleteThenCreate causing an apply error.

* apply the stored plan CreateThenDelete action

When applying a plan, a forced CreateBeforeDestroy may not be set during
the apply walk when downstream resources are no longer present in the
graph. We still need to stick to that plan, and both the
NodeApplyableResourceInstance EvalTree and the individual Eval nodes
need to operate on that planned value.

Ensure that we always check for an existing plan when determining
CreateBeforeDestroy status. This must happen in 2 different code paths
due to the eval node pattern currently in-use. Future refactoring may be
able to unify these code-paths to make this less fragile.

* update CHANGELOG.md

* vendor: upgrade go-userdirs dependency to fix crash [v0.13 backport] (hashicorp#26250)

* go mod tidy
* vendor: upgrade go-userdirs library

* Update CHANGELOG.md

* destroy nodes must rely on the state cbd status

When there is only a destroy node, there is no descendent to check for
"forced CBD", so we can only rely on the state to verify.

* improve failing test

Correct the initial test state, and expand the test to cause a cycle
without the previous fix.

* allow evaluation of 0 instances during apply

While this was easier to spot during plan, it is also possible to
evaluate resources with 0 instances during apply as well.

This doesn't effect the failure when scaling CBD instances, it only
changes the fact that the inconsistent value is no longer unknown.

* update CHANGELOG.md

* Release v0.13.3

* Cleanup after v0.13.3 release

* configs: More interpolation-only expr deprecations

Extend the deprecation for interpolation-only expressions to include
module calls, data sources, outputs, and locals.

* Update CHANGELOG.md

* command: Providers schema shows required_providers

The providers schema command is using the Config.ProviderTypes method,
which had not been kept up to date with the changes to provider
requirements detection made in Config.ProviderRequirements. This
resulted in any currently-unused providers being omitted from the
output.

This commit changes the ProviderTypes method to use the same underlying
logic as ProviderRequirements, which ensures that `required_providers`
blocks are taken into account.

Includes an integration test case to verify that this fixes the provider
schemas command bug.

* Update CHANGELOG.md

* Add deprecation warning for vendor provisioners

Adds a warning for chef, habitat, puppet, and salt-masterless
provisioners, and a corresponding test file to test for the warning

* Add docs notes for deprecation

* configs: Deprecate nested redundant interpolations

Previous deprecations only included direct assignment of template-only
expressions to arguments. That is, this was not deprecated:

locals {
  foo = ["${var.foo}"]
}

This commit uses hclsyntax.VisitAll to detect and show deprecations for
all template-only expressions, no matter how deep they are in a given
expression.

* Update CHANGELOG.md

* Update CHANGELOG.md

* Update CHANGELOG.md

* refresh cbd test

* update create_before_destroy when refreshing

In order to save any changes to lifecycle options, we need to record
those changes during refresh, otherwise they would only be updated when
there is a change in the resource to be applied.

* update CHANGELOG.md

* data source depends_on

A data source referencing another data source through depends_on should
not be forced to defer until apply. Data sources have no side effects,
so nothing should need to be applied. If the dependency has a
planned change due to a managed resource, the original data source will
also encounter that further down the list of dependencies.

This prevents a data source being read during plan for any reason from
causing other data sources to be deferred until apply. It does not
change the behavior noticeably in 0.14, but because 0.13 still had
separate refresh and plan phases which could read the data source, the
deferral could cause many things downstream to become unexpectedly
unknown until apply.

* update CHANGELOG.md

* Fix passing wrong file info & Add test coverage (hashicorp#26400)

Co-authored-by: Chanh Hua <[email protected]>

* Update CHANGELOG.md

* Updating codeowners with our deprecated status for tool-specific provisioners

* Release v0.13.4

* Cleanup after v0.13.4 release

* terraform: fix ProviderConfigTransformer

The ProviderConfigTransformer was using only the provider FQN to attach
a provider configuration to the provider, but what it needs to do is
find the local name for the given provider FQN (which may not match the
type name) and use that when searching for matching provider
configuration.

Fixes hashicorp#26556

This will also be backported to the v0.13 branch.

* Update CHANGELOG.md

* cache DecoderSpec calls

DecoderSpec may be called many times, and deeply recursive calls are
expensive. Since we cannot synchronize the Blocks themselves due to them
being copied in parts of the code, we use a separate cache to store the
generated Specs.

* add benchmark used for DecoderSpec

Not a great benchmark, but record it here for posterity. Practical
testing with the AWS provider gives us a 98% speedup for simple plans.

* update CHANGELOG.md

* AzureRM backend: correctly lookup environment from metadata host

* add missing Context argument

* Update CHANGELOG.md

Adding the AzureRM backend fix

* Update CHANGELOG.md

Spelling mistake. For shame.

* Release v0.13.5

* Cleanup after v0.13.5 release

* Tool-specific provisioner docs change to red notice box (hashicorp#26681)

* states: Extract version check logic from read

Instead of always checking the Terraform version associated with a state
file when reading it, we add a CheckTerraformVersion method and call it
from locations where we care about enforcing this check.

For this commit, the check has been retained at all call sites for
states/statefile.Read, with these exceptions:

- Unit tests, which shouldn't care about the state file version;
- E2E test runner which should always be using valid state files;
- terraform.ShimLegacyState, where the check is pointless as the state
  file was just created by the current Terraform version.

* states: RefreshStateWithoutCheckVersion

Add RefreshStateWithoutCheckVersion method to the statemgr.Persistent
interface, allowing callers to refresh state from the backend without
raising errors if the state's Terraform version is thought to be
not fully compatible.

This enables use cases where we can be extremely confident that any
state file we can read is suitable, such as the Terraform provider's
remote state data source, which only reads outputs.

* backend: StateMgrWithoutCheckVersion

Allow users of backends to initialize a state manager instance without
checking the Terraform version of any state files which are retrieved
during this process. Many backends call RefreshState as part of
initialization, and this new method instead calls the new
RefreshStateWithoutCheckVersion method to prevent version checking.

* builtin: Disable Terraform provider version checks

The builtin Terraform provider's remote state data source uses a
configured backend to fetch a given state, in order to allow access to
its root module outputs. Until this commit, this was only possible
with remote states which are from the current Terraform version or
older, forcing multi-state users to carefully orchestrate Terraform
upgrades.

Building on previous commits in this branch, we now disable this version
check, and allow any Terraform state file that the current Terraform
version can parse. Since we are only ever accessing root module outputs,
this is very likely to be safe for the foreseeable future.

* website: Add Registry email to publishing doc (hashicorp#26327)

* website: Update Registry key management docs (hashicorp#26299)

* Update CHANGELOG.md

* Update link to new varibles tutorial

* website: Add Registry docs for webhooks (hashicorp#26870)

* backport of commit da7c488

* backend: Validate remote backend Terraform version

When using the enhanced remote backend, a subset of all Terraform
operations are supported. Of these, only plan and apply can be executed
on the remote infrastructure (e.g. Terraform Cloud). Other operations
run locally and use the remote backend for state storage.

This causes problems when the local version of Terraform does not match
the configured version from the remote workspace. If the two versions
are incompatible, an `import` or `state mv` operation can cause the
remote workspace to be unusable until a manual fix is applied.

To prevent this from happening accidentally, this commit introduces a
check that the local Terraform version and the configured remote
workspace Terraform version are compatible. This check is skipped for
commands which do not write state, and can also be disabled by the use
of a new command-line flag, `-ignore-remote-version`.

Terraform version compatibility is defined as:

- For all releases before 0.14.0, local must exactly equal remote, as
  two different versions cannot share state;
- 0.14.0 to 1.0.x are compatible, as we will not change the state
  version number until at least Terraform 1.1.0;
- Versions after 1.1.0 must have the same major and minor versions, as
  we will not change the state version number in a patch release.

If the two versions are incompatible, a diagnostic is displayed,
advising that the error can be suppressed with `-ignore-remote-version`.
When this flag is used, the diagnostic is still displayed, but as a
warning instead of an error.

Commands which will not write state can assert this fact by calling the
helper `meta.ignoreRemoteBackendVersionConflict`, which will disable the
checks. Those which can write state should instead call the helper
`meta.remoteBackendVersionCheck`, which will return diagnostics for
display.

In addition to these explicit paths for managing the version check, we
have an implicit check in the remote backend's state manager
initialization method. Both of the above helpers will disable this
check. This fallback is in place to ensure that future code paths which
access state cannot accidentally skip the remote version check.

* Update CHANGELOG.md

* backport of commit 107dc0c

* backport of commit 6e0d8cd

* Update docs and add warning for -get-plugins

As of Terraform 0.13+, the get-plugins command has been
superceded by new provider installation mechanisms, and
general philosophy (providers are always installed, but
the sources may be customized). Updat the init command
to give users a warning if they are setting this flag,
to encourage them to remove it from their workflow, and
update relevant docs and docstrings as well

* Update CHANGELOG.md

* Revert "backport of commit 6e0d8cd"

This reverts commit ea394d7.

* Revert "backport of commit 107dc0c"

This reverts commit 718a678.

* Revert "Update CHANGELOG.md"

This reverts commit c843bb9.

* Revert "backend: Validate remote backend Terraform version"

This reverts commit 804a667.

* Release v0.13.6

Co-authored-by: hashicorp-ci <[email protected]>
Co-authored-by: Matthias Bartelmeß <[email protected]>
Co-authored-by: Alex Novak <[email protected]>
Co-authored-by: shaowenchen <[email protected]>
Co-authored-by: James Bardin <[email protected]>
Co-authored-by: Pam Selle <[email protected]>
Co-authored-by: Pam Selle <[email protected]>
Co-authored-by: Petros Kolyvas <[email protected]>
Co-authored-by: Lance Kind <[email protected]>
Co-authored-by: Nick Fagerlund <[email protected]>
Co-authored-by: Kirill Zaborsky <[email protected]>
Co-authored-by: Alisdair McDiarmid <[email protected]>
Co-authored-by: Kristin Laemmert <[email protected]>
Co-authored-by: Chanh Hua <[email protected]>
Co-authored-by: kt <[email protected]>
Co-authored-by: Petros Kolyvas <[email protected]>
Co-authored-by: Justin Campbell <[email protected]>
Co-authored-by: Robin Norwood <[email protected]>
Co-authored-by: Martin Atkins <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug new new issue not yet triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants