Commit Graph

830 Commits

Author SHA1 Message Date
Martin Atkins 82c518209d experiments: New "preconditions_postconditions" experiment 2022-01-28 11:00:29 -05:00
Martin Atkins 4f41a0a1fe configs: Generalize "VariableValidation" as "CheckRule"
This construct of a block containing a condition and an error message will
be useful for other sorts of blocks defining expectations or contracts, so
we'll give it a more generic name in anticipation of it being used in
other situations.
2022-01-28 11:00:29 -05:00
Alisdair McDiarmid e21d5d36f4 core: Fix plan write/state write ordering bug 2022-01-26 15:33:18 -05:00
Alisdair McDiarmid 5a9bc76d1a
Merge pull request #30316 from superkooks/main
Fix autocomplete for workspace subcommands
2022-01-21 12:01:41 -05:00
Brian Flad a477d10bd1
Introduce Terraform Plugin Protocol 6.2 with legacy_type_system fields from Protocol 5 (#30375)
Reference: https://github.com/hashicorp/terraform/issues/30373

This change forward ports the `legacy_type_system` boolean fields in the `ApplyResourceChange.Response` and `PlanResourceChange.Response` messages that existed in protocol version 5, so that existing terraform-plugin-sdk/v2 providers can be muxed with protocol version 6 providers (e.g. terraform-plugin-framework) while also taking advantage of the newer protocol features. This functionality should not be used by any providers or SDKs except those built with terraform-plugin-sdk.

Updated via:

```shell
cp docs/plugin-protocol/tfplugin6.1.proto docs/plugin-protocol/tfplugin6.2.proto
# Copy legacy_type_system fields from tfplugin5.2.proto into ApplyResourceChange.Response and PlanResourceChange
rm internal/tfplugin6/tfplugin6.proto
ln -s ../../docs/plugin-protocol/tfplugin6.2.proto internal/tfplugin6/tfplugin6.proto
go run tools/protobuf-compile/protobuf-compile.go `pwd`
# Updates to internal/plugin6/grpc_provider.go
```
2022-01-20 09:57:42 -05:00
Krista LaFentres (she/her) 6dcf00aefc
Merge pull request #30344 from hashicorp/lafentres/refactor-show-command
cli: Refactor show command & migrate to command arguments and views
2022-01-13 13:58:53 -06:00
Krista LaFentres 64e1241ae3 backend/local: Remove unused DisablePlanFileStateLineageChecks flag
Now that show command has been refactored to remove its dependence
on a local backend and local run, this flag is no longer needed to
fix #30195.
2022-01-13 11:00:10 -06:00
Krista LaFentres fea8f6cfa2 cli: Migrate show command to use command arguments and views 2022-01-13 11:00:03 -06:00
Krista LaFentres 8d1bced812 cli: Refactor show command to remove dependence on local run and only load the backend when we need it
See https://github.com/hashicorp/terraform/pull/30205#issuecomment-997113175 for more context
2022-01-12 13:47:59 -06:00
Martin Atkins e95f29bf9d lang/funcs: fileexists slightly better "not a file" error message
Previously we were just returning a string representation of the file mode,
which spends more characters on the irrelevant permission bits that it
does on the directory entry type, and is presented in a Unix-centric
format that likely won't be familiar to the user of a Windows system.

Instead, we'll recognize a few specific directory entry types that seem
worth mentioning by name, and then use a generic message for the rest.

The original motivation here was actually to deal with the fact that our
tests for this function were previously not portable due to the error
message leaking system-specific permission detail that are not relevant
to the test. Rather than just directly addressing that portability
problem, I took the opportunity to improve the error messages at the same
time.

However, because of that initial focus there are only actually tests here
for the directory case. A test that tries to test any of these other file
modes would not be portable and in some cases would require superuser
access, so we'll just leave those cases untested for the moment since they
weren't tested before anyway, and so we've not _lost_ any test coverage
here.
2022-01-11 08:46:29 -08:00
Martin Atkins bdc5f152d7 refactoring: Implied move statements can be cross-package
Terraform uses "implied" move statements to represent the situation where
it automatically handles a switch from count to no-count on a resource.
Because that situation requires targeting only a specific resource
instance inside a specific module instance, implied move statements are
always presented as if they had been declared in the root module and then
traversed through the exact module instance path to reach the target
resource.

However, that means they can potentially cross a module package boundary,
if the changed resource belongs to an external module. Normally we
prohibit that to avoid the root module depending on implementation details
of the called module, but Terraform generates these implied statements
based only on information in the called module and so there's no need to
apply that same restriction to implied move statements, which will always
have source and destination addresses belonging to the same module
instance.

This change therefore fixes a misbehavior where Terraform would reject
an attempt to switch from no-count to count in a called module, where
previously the author of the calling configuration had no recourse to fix
it because the change has actually happened upstream.
2022-01-11 08:43:57 -08:00
James Bardin 684ed7505d remove synthetic default expression for variables
Now that variable evaluation checks for a nil expression the graph
transformer does not need to generate a synthetic expression for
variable defaults. This means that all default handling is now located
in one place, and we are not surprised by a configuration expression
showing up which doesn't actually exist in the configuration.

Rename nodeModuleVariable.evalModuleCallArgument to evalModuleVariable.
This method is no longer handling only the module call argument, it is
also dealing with the variable declaration defaults and validation
statements.

Add an additional tests for validation with a non-nullable variable.
2022-01-10 16:22:33 -05:00
Martin Atkins 9ebc3e1cd2 core: More accurate error message for invalid variable values
In earlier Terraform versions we had an extra validation step prior to
the graph walk which tried to partially validate root module input
variable values (just checking their type constraints) and then return
error messages which specified as accurately as possible where the value
had originally come from.

We're now handling that sort of validation exclusively during the graph
walk so that we can share the main logic between both root module and
child module variable values, but previously that shared code wasn't
able to generate such specific information about where the values had
originated, because it was adapted from code originally written to only
deal with child module variables.

Here then we restore a similar level of detail as before, when we're
processing root module variables. For child module variables, we use
synthetic InputValue objects which state that the value was declared
in the configuration, thus causing us to produce a similar sort of error
message as we would've before which includes a source range covering
the argument expression in the calling module block.
2022-01-10 12:26:54 -08:00
Martin Atkins 36c4d4c241 core and backend: remove redundant handling of default variable values
Previously we had three different layers all thinking they were
responsible for substituting a default value for an unset root module
variable:
 - the local backend, via logic in backend.ParseVariableValues
 - the context.Plan function (and other similar functions) trying to
   preprocess the input variables using
   terraform.mergeDefaultInputVariableValues .
 - the newer prepareFinalInputVariableValue, which aims to centralize all
   of the variable preparation logic so it can be common to both root and
   child module variables.

The second of these was also trying to handle type constraint checking,
which is also the responsibility of the central function and not something
we need to handle so early.

Only the last of these consistently handles both root and child module
variables, and so is the one we ought to keep. The others are now
redundant and are causing prepareFinalInputVariableValue to get a slightly
corrupted view of the caller's chosen variable values.

To rectify that, here we remove the two redundant layers altogether and
have unset root variables pass through as cty.NilVal all the way to the
central prepareFinalInputVariableValue function, which will then handle
them in a suitable way which properly respects the "nullable" setting.

This commit includes some test changes in the terraform package to make
those tests no longer rely on the mergeDefaultInputVariableValues logic
we've removed, and to instead explicitly set cty.NilVal for all unset
variables to comply with our intended contract for PlanOpts.SetVariables,
and similar. (This is so that we can more easily catch bugs in callers
where they _don't_ correctly handle input variables; it allows us to
distinguish between the caller explicitly marking a variable as unset vs.
not describing it at all, where the latter is a bug in the caller.)
2022-01-10 12:26:54 -08:00
Martin Atkins 37b1413ab3 core: Handle root and child module input variables consistently
Previously we had a significant discrepancy between these two situations:
we wrote the raw root module variables directly into the EvalContext and
then applied type conversions only at expression evaluation time, while
for child modules we converted and validated the values while visiting
the variable graph node and wrote only the _final_ value into the
EvalContext.

This confusion seems to have been the root cause for #29899, where
validation rules for root module variables were being applied at the wrong
point in the process, prior to type conversion.

To fix that bug and also make similar mistakes less likely in the future,
I've made the root module variable handling more like the child module
variable handling in the following ways:
 - The "raw value" (exactly as given by the user) lives only in the graph
   node representing the variable, which mirrors how the _expression_
   for a child module variable lives in its graph node. This means that
   the flow for the two is the same except that there's no expression
   evaluation step for root module variables, because they arrive as
   constant values from the caller.
 - The set of variable values in the EvalContext is always only "final"
   values, after type conversion is complete. That in turn means we no
   longer need to do "just in time" conversion in
   evaluationStateData.GetInputVariable, and can just return the value
   exactly as stored, which is consistent with how we handle all other
   references between objects.

This diff is noisier than I'd like because of how much it takes to wire
a new argument (the raw variable values) through to the plan graph builder,
but those changes are pretty mechanical and the interesting logic lives
inside the plan graph builder itself, in NodeRootVariable, and
the shared helper functions in eval_variable.go.

While here I also took the opportunity to fix a historical API wart in
EvalContext, where SetModuleCallArguments was built to take a set of
variable values all at once but our current caller always calls with only
one at a time. That is now just SetModuleCallArgument singular, to match
with the new SetRootModuleArgument to deal with root module variables.
2022-01-10 12:26:54 -08:00
Martin Atkins 483c38aca1 core: Remove TestContext2Validate_PlanGraphBuilder
This test seems to be a holdover from the many-moons-ago switch from one
graph for all operations to separate graphs for plan and apply. It is
effectively just a copy of a subset of the content of the Context.Validate
function and is a maintainability hazard because it tends to lag behind
updates to that function unless changes there happen to make it fail.

This test doesn't cover anything that the other validate context tests
don't exercise as an implementation detail of calling Context.Validate,
so I've just removed it with no replacement.
2022-01-10 12:26:54 -08:00
Martin Atkins 171e7ef6d9 core: Invalid for_each argument messaging improvements
Our original messaging here was largely just lifted from the equivalent
message for unknown values in "count", and it didn't really include any
specific advice on how to update a configuration to make for_each valid,
instead focusing only on the workaround of using the -target planning
option.

It's tough to pack in a fully-actionable suggestion here since unknown
values in for_each keys tends to be a gnarly architectural problem rather
than a local quirk -- when data flows between modules it can sometimes be
unclear whether it'll end up being used in a context which allows unknown
values.

I did my best to summarize the advice we've been giving in community forum
though, in the hope that more people will be able to address this for
themselves without asking for help, until we're one day able to smooth
this out better with a mechanism such as "partial apply".
2022-01-10 12:23:13 -08:00
SuperKooks 5171c89e1e Fix autocomplete for workspace subcommands 2022-01-08 14:41:20 +11:00
Nick Fagerlund 1e9075b4fa
Merge pull request #30226 from hashicorp/nf/dec21-derandomize-dependencies
Sort dependencies when encoding `ResourceInstanceObject`
2022-01-07 15:26:07 -08:00
Nick Fagerlund 05d0febf7f Relax test to focus on the behavior we care about (encoded == encoded)
The specific output order is meaningless, but it should always be the same after
two Encode() calls with identical (ignoring in-memory order) dependency sets.
2022-01-05 14:38:53 -08:00
Martin Atkins b802db75d7 build: Build and run e2etest as part of the release build pipeline
This uses the decoupled build and run strategy to run the e2etests so that
we can arrange to run the tests against the real release packages produced
elsewhere in this workflow, rather than ones generated just in time by
the test harness.

The modifications to make-archive.sh here make it more consistent with its
originally-intended purpose of producing a harness for testing "real"
release executables. Our earlier compromise of making it include its own
terraform executable came from a desire to use that script as part of
manual cross-platform testing when we weren't yet set up to support
automation of those tests as we're doing here. That does mean, however,
that the terraform-e2etest package content must be combined with content
from a terraform release package in order to produce a valid contest for
running the tests.

We use a single job to cross-compile the test harness for all of the
supported platforms, because that build is relatively fast and so not
worth the overhead of matrix build, but then use a matrix build to
actually run the tests so that we can run them in a worker matching the
target platform.

We currently have access only to amd64 (x64) runners in GitHub Actions
and so for the moment this process is limited only to the subset of our
supported platforms which use that architecture.
2022-01-05 14:31:04 -08:00
Alisdair McDiarmid df36a03be1 states: Add failing test for ordered dependencies 2022-01-05 14:24:03 -08:00
Alisdair McDiarmid 535da4ebc7
Merge pull request #30205 from hashicorp/alisdair/fix-show-plan-against-non-default-state
command/show: Disable plan state lineage checks
2022-01-05 12:03:28 -05:00
Katy Moe f8fdb6de3f
do not use pointer addr strings as map keys in set
When creating a Set of BasicEdges, the Hashcode function is used to determine
map keys for the underlying set data structure.

The string hex representation of the two vertices' pointers is unsafe to use
as a map key, since these addresses may change between the time they are added
to the set and the time the set is operated on.

Instead we modify the Hashcode function to maintain the references to the
underlying vertices so they cannot be garbage collected during the lifetime
of the Set.
2022-01-05 11:28:47 +00:00
James Bardin 9272ff2c29
Merge pull request #30286 from hashicorp/jbardin/dag
dag: minor cleanup
2022-01-04 12:51:21 -05:00
James Bardin 8bbba22f8c
Merge pull request #30253 from hashicorp/jbardin/move-graph
cleanup some move graph handling
2022-01-04 12:51:12 -05:00
Alisdair McDiarmid ef01d5d134
Merge pull request #30067 from hashicorp/alisdair/redact-sensitive-values-from-function-errors
lang/funcs: Redact sensitive values from function errors
2022-01-04 11:34:04 -05:00
James Bardin 344adb6c50 clarify dag comments
TransitiveReduction does not rely on having a single root, and only
must be free of cycles.

DepthFirstWalk and ReverseDepthFirstWalk do not do a topological sort,
so if order matters TransitiveReduction must be run first.
2022-01-04 10:07:31 -05:00
James Bardin fae68f166f Remove sorted walk functions
These two functions were left during a refactor to ensure the old
behavior of a sorted walk was still accessible in some manner. The
package has since been removed from any public API, and the sorted
versions are no longer called, so we can remove them.
2022-01-04 09:37:53 -05:00
James Bardin f46cf7b8bc cleanup some move graph handling
Create a separate `validateMoveStatementGraph` function so that
`ValidateMoves` and `ApplyMoves` both check the same conditions. Since
we're not using the builtin `graph.Validate` method, because we may have
multiple roots and want better cycle diagnostics, we need to add checks
for self references too. While multiple roots are an error enforced by
`Validate` for the concurrent walk, they are OK when using
`TransitiveReduction` and `ReverseDepthFirstWalk`, so we can skip that
check.

Apply moves must first use `TransitiveReduction` to reduce the graph,
otherwise nodes may be skipped if they are passed over by a transitive
edge.
2022-01-04 09:21:36 -05:00
James Bardin 22dc685052 check for nested module index changes
Changing only the index on a nested module will cause all nested moves
to create cycles, since their full addresses will match both the From
and To addresses. When building the dependency graph, check if the
parent is only changing the index of the containing module, and prevent
the backwards edge for the move.
2022-01-04 09:20:47 -05:00
James Bardin deb82daf2b find implied moves in nested modules
Implied moves in nested modules were being skipped
2022-01-04 09:20:47 -05:00
James Bardin 3d769b7282 IsModuleMoveReIndex
Add a method for checking if the From and To addresses in a move
statement are only changing the indexes of modules relative to the
statement module.

This is needed because move statement nested within the module will be
able to match against both the From and To addresses, causing cycles in
the order of move operations.
2022-01-04 09:20:47 -05:00
Martin Atkins 74761b2f8b getmodules: Use go-getter v1.5.10 and return to upstream GitGetter
There was an unintended regression in go-getter v1.5.9's GitGetter which
caused us to temporarily fork that particular getter into Terraform to
expedite a fix. However, upstream v1.5.10 now includes a
functionally-equivalent fix and so we can heal that fork by upgrading.

We'd also neglected to update the Module Sources docs when upgrading to
go-getter v1.5.9 originally and so we were missing documentation about the
new "depth" argument to enable shadow cloning, which I've added
retroactively here along with documenting its restriction of only
supporting named refs.

This new go-getter release also introduces a new credentials-passing
method for the Google Cloud Storage getter, and so we must incorporate
that into the Terraform-level documentation about module sources.
2022-01-03 11:44:16 -08:00
James Bardin 66b4d155b1
Merge pull request #30232 from hashicorp/jbardin/module-move-re-index
Handle move blocks within a module which is changing the index
2021-12-22 16:27:30 -05:00
James Bardin 75ef61c783 check for nested module index changes
Changing only the index on a nested module will cause all nested moves
to create cycles, since their full addresses will match both the From
and To addresses. When building the dependency graph, check if the
parent is only changing the index of the containing module, and prevent
the backwards edge for the move.
2021-12-22 16:15:04 -05:00
Barrett Clark 296acdd961
Merge pull request #30020 from hashicorp/barrettclark/cloud-e2e-parallel
Cloud: Add parallelism back into the tests
2021-12-22 12:14:06 -06:00
Alisdair McDiarmid f772cb085e
Merge pull request #30233 from hashicorp/alisdair/move-nested-modules
refactoring: Move nested modules
2021-12-22 10:22:21 -05:00
James Bardin a72d2d408d
Merge pull request #30095 from hashicorp/jbardin/invalid-provider-name
skip provider resolution when there are errors
2021-12-21 16:58:48 -05:00
James Bardin e761117562 find implied moves in nested modules
Implied moves in nested modules were being skipped
2021-12-21 16:49:25 -05:00
James Bardin 346418e31f IsModuleMoveReIndex
Add a method for checking if the From and To addresses in a move
statement are only changing the indexes of modules relative to the
statement module.

This is needed because move statement nested within the module will be
able to match against both the From and To addresses, causing cycles in
the order of move operations.
2021-12-21 16:49:25 -05:00
Alisdair McDiarmid d7ef123c12 refactoring: Move nested modules
When applying module `moved` statements by iterating through modules in
state, we previously required an exact match from the `moved`
statement's `from` field and the module address. This permitted moving
resources directly inside a module, but did not recur into module calls
within those moved modules.

This commit moves that exact match requirement so that it only applies
to `moved` statements targeting resources. In turn this allows nested
modules to be moved.
2021-12-21 16:25:06 -05:00
Nick Fagerlund 9b449bec99 Sort dependencies when encoding `ResourceInstanceObject`
Resource dependencies are by nature an unordered collection, but they're
persisted to state as a JSON array (in random order). This makes a mess for
`terraform apply -refresh-only`, which sees the new random order as a change
that requires the user to approve a state update.

(As an additional problem on top of that, the user interface for refresh-only
runs doesn't expect to see that as a type of change, so it says "no changes!
would you like to update to reflect these detected changes?")

This commit changes `ResourceInstanceObject.Encode()` to sort the in-memory
slice of dependencies (lexically, by address) before passing it on to be
compared and persisted. This appears to fix the observed UI issues with a
minimum of logic changes.
2021-12-20 21:46:39 -08:00
Barrett Clark d196d2870a Refactor cloud table test runs
As the cloud e2e tests evolved some common patters became apparent. This
standardizes and consolidates the patterns into a common test runner
that takes the table tests and runs them in parallel. Some tests also
needed to be converted to utilize table tests.
2021-12-20 16:36:06 -06:00
Martin Atkins 23395a1022 providercache: Discard lock entries for unused providers
Previously we would only ever add new lock entries or update existing
ones. However, it's possible that over time a module may _cease_ using
a particular provider, at which point we ought to remove it from the lock
file so that operations won't fail when seeing that the provider cache
directory is inconsistent with the lock file.

Now the provider installer (EnsureProviderVersions) will remove any lock
file entries that relate to providers not included in the given
requirements, which therefore makes the resulting lock file properly match
the set of packages the installer wrote into the cache.

This does potentially mean that someone could inadvertently defeat the
lock by removing a provider dependency, running "terraform init", then
undoing that removal, and finally running "terraform init" again. However,
that seems relatively unlikely compared to the likelihood of removing
a provider and keeping it removed, and in the event it _did_ happen the
changes to the lock entry for that provider would be visible in the diff
of the provider lock file as usual, and so could be noticed in code
review just as for any other change to dependencies.
2021-12-17 15:30:21 -08:00
Alisdair McDiarmid 768741c0f7 command/show: Disable plan state lineage checks
When showing a saved plan, we do not need to check the state lineage
against current state, because the plan cannot be applied. This is
relevant when plan and apply specify a `-state` argument to choose a
non-default state file. In this case, the stored prior state in the plan
will not match the default state file, so a lineage check will always
error.
2021-12-17 17:46:42 -05:00
James Bardin 8c4031ef15 don't persist a nil state from Apply
Apply should not return a nil state to be persisted.
2021-12-17 14:00:59 -05:00
James Bardin 58e001c22a return graph errors from Context.Apply
errors from building during apply were lost
2021-12-17 14:00:57 -05:00
James Bardin f83ed441bb
Merge pull request #30189 from hashicorp/jbardin/validate-moves
Ignore unexpanded paths when validating move statements.
2021-12-17 13:57:24 -05:00
James Bardin b213386a73 InstancesForModule should not panic
instances.Set is only used after all instances have been processes, so
it should therefor only handle known instances and not panic when given
an address that traverses an unexpanded module.
2021-12-17 13:31:41 -05:00
James Bardin 371660ab8f cleanup panic output 2021-12-17 11:57:52 -05:00
James Bardin a5017bff2f failing test moved with target 2021-12-16 18:20:49 -05:00
Barrett Clark c647b41d65 Add parallelism back into the tests
Running tests in parallel can help speed up overall test execution. Go
blocks parent tests while child tests run, so it does not fully fan out
as you might expect. It is noticably faster, though. Running 4 or more
concurrent processes knocks over a minute off the total execution time.
2021-12-15 11:37:49 -06:00
James Bardin e22ab70e03
Merge pull request #30171 from hashicorp/jbardin/revert-validate-for-each
use `cty.DynamicVal` for expanded resources during validation
2021-12-15 08:57:43 -05:00
James Bardin 645fcc5f12 test for lookup regression during validation 2021-12-15 08:43:37 -05:00
James Bardin 71b9682e8c ensure there is always a valid return value 2021-12-14 18:02:57 -05:00
James Bardin d469e86331 revert 6b8b0617
Revert the evaluation change from #29862.
While returning a dynamic value for all expanded resources during
validation is not optimal, trying to work around this using unknown maps
and lists is causing other undesirable behaviors during evaluation.
2021-12-14 17:58:10 -05:00
Martin Atkins c4d46e7c6b getmodules: Re-allow git:: source with ref=COMMIT_ID
Earlier versions of this code allowed "ref" to take any value that would
be accepted by "git checkout" as a valid target of a symbolic ref. We
inadvertently accepted a breaking change to upstream go-getter that broke
that as part of introducing a shallow clone optimization, because shallow
clone requires selecting a single branch.

To restore the previous capabilities while retaining the "depth" argument,
here we accept a compromise where "ref" has the stronger requirement of
being a valid named ref in the remote repository if and only if "depth"
is set to a value greater than zero. If depth isn't set or is less than
one, we will do the old behavior of just cloning all of the refs in the
remote repository in full and then switching to refer to the selected
branch, tag, or naked commit ID as a separate step.

This includes a heuristic to generate an additional error message hint if
we get an error from "git clone" and it looks like the user might've been
trying to use "depth" and "ref=COMMIT" together. We can't recognize that
error accurately because it's only reported as human-oriented git command
output, but this heuristic should hopefully minimize situations where we
show it inappropriately.

For now this is a change in the Terraform repository directly, so that we
can expedite the fix to an already-reported regression. After this is
released I tend to also submit a similar set of changes to upstream
go-getter, at which point we can revert Terraform to using the upstream
getter.GitGetter instead of our own local fork.
2021-12-14 11:24:23 -08:00
Martin Atkins b0ff17ef2a getmodules: Inline our own fork of getter.GitGetter
This is a pragmatic temporary solution to allow us to more quickly resolve
an upstream regression in go-getter locally within Terraform, so that the
work to upstream it for other callers can happen asynchronously and with
less time pressure.

This commit doesn't yet include any changes to address the bug, and
instead aims to be functionally equivalent to getter.GitGetter. A
subsequent commit will then address the regression, so that the diff of
that commit will be easier to apply later to the upstream to get the same
effect there.
2021-12-14 11:24:23 -08:00
Chris Arcand 8b8fe2771f
Merge pull request #30142 from hashicorp/chrisarcand/remote-backend-no-workspaces-regression
command/meta_backend: Allow the remote backend to have no workspaces [again]
2021-12-14 09:58:50 -06:00
Chris Arcand 98978b3853 command/meta_backend: Allow the remote backend to have no workspaces [again]
A regression introduced in d72a413ef8

The comment explains, but TLDR: The remote backend actually *depended*
on being able to write it's backend state even though an 'error'
occurred (no workspaces).
2021-12-14 09:50:42 -06:00
Martin Atkins 096cddb4b7 command/format: Limitation of plans.ResourceInstanceDeleteBecauseNoModule
This is an explicit technical debt note that our plan renderer isn't able
to give a fully-specific hint in this particular case of deletion reason.

This reason code means that at least one of the module instance keys in
the resource's module path doesn't match an instance declared in the
configuration, but the plan data structure doesn't retain enough
information to know which is the first step in the path which refers to
a missing instance, and so we just always return the whole thing.

This would be confusing if we return module.foo[0].module.bar not being
in the configuration as a result of module.foo not using "count"; it would
be better to say "module.foo[0] is not in the configuration" instead.

It would be most ideal to handle all of the different situations that
ResourceInstanceDeleteBecauseWrongRepetition's rendering does, so that we
can go further and explain exactly _why_ that module instance isn't
declared anymore.

We can do neither of those things today because only the Terraform Core
"expander" component knows that information, and we've discarded that
by the time we get to rendering a plan. To fix this one day would require
preserving in the plan information about which module instances are
declared, as a separate sidecar data structure from which resource
instances we're taking actions on, and then using that to identify which
step in addr.Module here first selects an invalid instance.
2021-12-13 10:04:15 -05:00
Martin Atkins ec6fe93fa8 instances: Non-existing module instance has no resource instances
Previously we were treating it as a programming error to ask for the
instances of a resource inside an instance of a module that is declared
but whose declaration doesn't include the given instance key.

However, that's actually a valid situation which can arise if, for
example, the user has changed the repetition/expansion mode for an
existing module call and so now all of the resource instances addresses it
previously contained are "orphaned".

To represent that, we'll instead say that an invalid instance key of a
declared module behaves as if it contains no resource instances at all,
regardless of the configurations of any resources nested inside. This
then gives the result needed to successfully detect all of the former
resource instances as "orphaned" and plan to destroy them.

However, this then introduces a new case for
NodePlannableResourceInstanceOrphan.deleteActionReason to deal with: the
resource configuration still exists (because configuration isn't aware of
individual module/resource instances) but the module instance does not.
This actually allows us to resolve, at least partially, a previous missing
piece of explaining to the user why the resource instances are planned
for deletion in that case, finally allowing us to be explicit to the user
that it's because of the module instance being removed, which
internally we call plans.ResourceInstanceDeleteBecauseNoModule.

Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com>
2021-12-13 10:03:50 -05:00
Chris Arcand f521ba6cd7 Remove 'enhanced' backend type distinction
As explained in the changes: The 'enhanced' backend terminology, which
only truly pertains to the 'remote' backend with a single API (Terraform
Cloud/Enterprise's), has been found to be a confusing vestige which need
only be explained in the context of the 'remote' backend.

These changes reorient the explanation(s) of backends to pertain more
directly to their primary purpose, which is storage of state snapshots
(and not implementing operations).

That Terraform operations are still _implemented_ by the literal
`Backend` and `Enhanced` interfaces is inconsequential a user of
Terraform, an internal detail.
2021-12-07 16:29:51 -06:00
James Bardin 0771a214d8 skip provider resolution when there are errors
If there are errors loading the configuration, don't try to resolve the
provider names, as those names may not even be valid.
2021-12-06 14:28:34 -05:00
Alisdair McDiarmid 5d7cb81c0c lang: Redact sensitive values from function errors
Some function errors include values derived from arguments. This commit
is the result of a manual audit of these errors, which resulted in:

- Adding a helper function to redact sensitive values;
- Applying that helper function where errors include values derived from
  possibly-sensitive arguments;
- Cleaning up other errors which need not include those values, or were
  otherwise incorrect.
2021-12-03 09:46:57 -05:00
Chris Arcand f4f5b7c968
Merge pull request #30059 from hashicorp/barrettclark/explicit-local-empty-migrate-messaging
Cloud: Do not ask to migrate empty default workspace
2021-12-01 11:53:36 -06:00
Barrett Clark c706c8f92b Do not ask to migrate empty default workspace
When migrating from an explicit local backend to Terraform Cloud, we ask
if you want to migrate the state. If there is no state to migrate we
should not ask if they want to migrate the emptiness.
2021-12-01 11:43:41 -06:00
Barrett Clark e08a02e7bf Fixes Issue #29959, Apply w/o init error message
When going from a local backend to Terraform Cloud, if you skip the
`terraform init` and run `terraform apply` this will give the user more
clear instructions.
2021-12-01 11:28:35 -06:00
Luces Huayhuaca d72a413ef8
command/meta_backend: Prompt to select workspace before saving backend config (#29945)
When terraform detects that a user has no workspaces that map to their current configuration, it will prompt the user to create a new workspace and enter a value name. If the user ignores the prompt and exits it, the legacy backend (terraform.tfstate) will be left in a awkward state:

1. This saved backend config will show a diff for the JSON attributes "serial", "tags" and "hash"
2. "Terraform workspace list" will show an empty list
3. "Terraform apply" will run successfully using the previous workspace, from the previous config, not the one from the current saved backend config
4. The cloud config is not reflective of the current working directory

Solution: If the user exits the prompt, the saved backend config should not be updated because they did not select a new workspace. They are back at the beginning where they are force to re run the init cmd again before proceeding with new changes.
2021-12-01 08:53:47 -08:00
Martin Atkins affe2c3295 addrs: Expose the registry address parser's error messages
Previously we ended up losing all of the error message detail produced by
the registry address parser, because we treated any registry address
failure as cause to parse the address as a go-getter-style remote address
instead.

That led to terrible feedback in the situation where the user _was_
trying to write a module address but it was invalid in some way.

Although we can't really tighten this up in the default case due to our
compatibility promises, it's never been valid to use the "version"
argument with anything other than a registry address and so as a
compromise here we'll use the presence of "version" as a heuristic for
user intent to parse the source address as a registry address, and thus
we can return a registry-address-specific error message in that case and
thus give more direct feedback about what was wrong.

This unfortunately won't help someone trying to install from the registry
_without_ a version constraint, but I didn't want to let perfect be the
enemy of the good here, particularly since we recommend using version
constraints with registry modules anyway; indeed, that's one of the main
benefits of using a registry rather than a remote source directly.
2021-11-30 15:46:16 -08:00
kmoe 8f923cea08
Merge pull request #30048 from hashicorp/kmoe/http-backend-debug-log
backend/http: supply logging.LogOutput() to retryable client
2021-11-30 14:25:28 +00:00
Alisdair McDiarmid 8ec9ad0407
Merge pull request #30029 from hashicorp/alisdair/add-sensitive-marks-for-nested-attributes
configs: Add sensitive marks for nested attributes
2021-11-30 09:20:43 -05:00
Alisdair McDiarmid d24403799e
Merge pull request #30019 from hashicorp/alisdair/fix-provider-schema-for-deleted-resource-instance
core: Fix schema loading for deleted resources
2021-11-30 09:20:27 -05:00
Katy Moe b1c071bb57
pass hclog logger to retryable client 2021-11-30 00:09:01 +00:00
Chris Arcand b5af7b6c92
Merge pull request #29987 from hashicorp/chrisarcand/backend-flag-with-tfc
command/init: Add -cloud alias to -backend, adjust `init` help output
2021-11-29 08:06:08 -06:00
Alisdair McDiarmid fbed52a353 configs: Add sensitive marks for nested attributes
Object values returned from providers have their attributes marked as
sensitive based on the provider schema. This was not fully implemented
for nested attribute types, which is corrected in this commit.
2021-11-25 11:55:17 -05:00
Alisdair McDiarmid 5ddf5e1f7d core: Fix schema loading for deleted resources
Resource instances removed from the configuration would previously use
the implied provider address. This is correct for default providers, but
incorrect for those from other namespaces or hosts. The fix here is to
use the stored provider config if it is present.
2021-11-24 15:23:20 -05:00
Barrett Clark a6b56ad76f
Merge pull request #29997 from hashicorp/barrettclark/cloud-input-false
Cloud integration requires input for migrations
2021-11-23 16:22:27 -06:00
Barrett Clark 419676cb69 Cloud integration requires input for migrations
We cannot programmatically migrate workspaces to Terraform Cloud without
prompts, so `-input=false` should not be allowed in those cases.

There are 4 scenarios where we need input from a user to complete
migrating workspaces to Terraform Cloud.

1.) Migrate from a single local workspace to Terraform Cloud

* Terraform config for a local backend. Implicit local (no backend
  specified) is fine.
* `terraform init` and `terraform apply`
* Change the Terraform config to use the cloud block
* `terraform init -input=false`
* You should now see an error message

2.) Migrate from a remote backend with a prefix to Terraform Cloud with
  tags

* Create a workspace in Terraform Cloud manually. The name should
  include a prefix, like "app-one"
* Have the terraform config use `backend "remote"` with a prefix set to
  "app-"
* `terraform init` and `terraform apply`
* Update the Terraform config to use a cloud block with `tags
  = ["app"]`. There should not be a prefix defined in the config now.
* `terraform init -input=false`
* You should now see an error message

3.) Migrate from multiple local workspaces to a single Terraform Cloud
  workspace
* Create one or many local workspaces
* `terraform init` and `terraform apply` in each
* Change the Terraform config to use the cloud block
* `terraform init -input=false`
* You should now see an error message

4.) Migrate to Terraform Cloud and ask for a workspace name
* Create several local workspaces
* `terraform init` and `terraform apply` in each
* Change the Terraform config to use the cloud block with tags
* `terraform init -input=false`
* You should now see an error message
2021-11-23 10:39:42 -06:00
Luces Huayhuaca f63b6198ca
Create a function for logic that assigns value to initReason var after changing backend configuration (#29967)
* Create a function for logic that assigns value to initReason var after changing backend configuration

Create func determineInitReason() for logic block that assigns value to initReason var after changing backend/cloud configuration block or migrating to a different type of backend configuration. Also clarify 'cloud' configuration block message to say 'Terraform Cloud configuration block has changed' instead of 'Terraform Cloud configuration has changed'.
2021-11-22 13:32:34 -08:00
Alisdair McDiarmid a8b9d086b2 cli: Fix nested single and map diff rendering
Indentation was out by two spaces. This is now consistent with the other
displays of object/map values: four spaces for each nesting level.
2021-11-19 15:48:10 -05:00
Chris Arcand a0c7c1dc8d command/init: Adjust init help output
Some of the wording here needed adjusting with the change that backends
largely reflect state snapshot storage (removing 'enhanced'
designation), and that a 'backend' is not necessarily always present.
2021-11-19 14:40:12 -06:00
Chris Arcand 33d7c1e6d6 command/init: Add -cloud alias (of -backend)
This fixes an issue where a user could not disable initialization of the
'cloud' configuration block (As is possible with -backend=false), as
well as add some syntactic sugar around -backend by adding a mutually
exclusive -cloud alias.
2021-11-19 14:40:12 -06:00
Alisdair McDiarmid b562c4fb84 cli: Fix nested set diff rendering indentation 2021-11-19 13:06:34 -05:00
Alisdair McDiarmid 2d0349d9a4 cli: Fix diff for nested set unchanged elements
Unchanged elements in nested attributes backed by sets were previously
misrendered as empty objects. This commit removes the additional
brackets and adds a count of unchanged elements.
2021-11-19 11:53:36 -05:00
Krista LaFentres 2fc4e06890 Fix failing lint check for deprecated x/crypto/openpgp library 2021-11-18 14:59:29 -06:00
Barrett Clark 7aeaec9b48 Update tests for new prompts
* Remove double question to migrate
* Remove parallelism (it has been flaky)
* Add subtests to apply_auto_approve_test (it was overlooked before)
2021-11-17 14:20:44 -08:00
Martin Atkins 7d6d31eff8 command/init: Skip redundant state migration prompt in Cloud mode
The specialized Terraform Cloud migration process asks right up top
whether the user wants to migrate state, because there are various other
questions contingent on that answer.

Therefore we ought to just honor their earlier answer when we get to the
point of actually doing the state migration, rather than prompting again.

This is tricky because we're otherwise just reusing a codepath that's
common to both modes. Hopefully we can find a better way to do this in
a later commit, but for the moment our main motivation is minimizing risk
to the very next release.
2021-11-17 14:20:44 -08:00
Barrett Clark 27aa51687e Fix cloud e2e tests for updated migration flow 2021-11-17 14:20:44 -08:00
Martin Atkins bac59d2480 command/init: Be explicit that some options are not relevant for Cloud
There are a few command line options for "terraform init" which are only
relevant when working with traditional backends, with the Cloud
integration previously just mostly ignoring them, or sometimes misbehaving
slightly due to them creating an unreasonable situation.

Now we'll catch these and return explicit errors, in order to be clear
that these options are not needed nor supported in Cloud mode.
2021-11-17 14:20:44 -08:00
Martin Atkins c28b57b4d6 cloud/e2e: Additional context for failing to find expected output
This just gives a little extra information to work with when trying to
understand why a test failed. It doesn't change what any of the tests are
actually trying to test.
2021-11-17 14:20:44 -08:00
Martin Atkins e5172fea95 cloud: DetectConfigChangeType helper
This aims to encapsulate the somewhat-weird logic we currently use to
distinguish between the various "terraform init" situations involving
Terraform Cloud mode, in the hope of making codepaths that branch based
on this slightly easier to read.

This isn't yet used, but uses of it will follow in subsequent commits.
2021-11-17 14:20:44 -08:00
Tom Harvey a1c7c0dfa4
Update internal/backend/remote-state/azure/backend.go
Co-authored-by: Tom Bamford <tbamford@hashicorp.com>
2021-11-17 20:24:22 +01:00
tombuildsstuff ffe9a41468 internal/backend/remote-state/azure: adding tests covering MSAL 2021-11-17 18:45:44 +01:00
tombuildsstuff a2cad7d579 internal/backend/remote-state/azure: prefixing the existing tests with ADAL 2021-11-17 18:43:39 +01:00
tombuildsstuff 9f710558ce internal/backend/remote-state/azure: adding a feature-toggle to use MSAL 2021-11-17 18:42:01 +01:00
tombuildsstuff 27a34d6c2c internal/backend/remote-state/azure: updating the link to the client secret
This redirects to the registry so we should point there directly
2021-11-17 18:28:47 +01:00
tombuildsstuff 5133d30b71 internal/backend/remote-state/azure: code changes for go-azure-helpers v0.18.0 2021-11-17 18:26:23 +01:00
Jared Ledvina 61a5808680 k8s-backend - Initial context support for library update
Signed-off-by: Jared Ledvina <jared.ledvina@datadoghq.com>
2021-11-16 14:43:50 -05:00