Commit Graph

513 Commits

Author SHA1 Message Date
Martin Atkins 5b266dd5ca command: Remove the experimental "terraform add" command
We introduced this experiment to gather feedback, and the feedback we saw
led to us deciding to do another round of design work before we move
forward with something to meet this use-case.

In addition to being experimental, this has only been included in alpha
releases so far, and so on both counts it is not protected by the
Terraform v1.0 Compatibility Promises.
2021-10-20 06:42:47 -07:00
Alisdair McDiarmid cdd5ee6fb3
Merge pull request #29773 from hashicorp/alisdair/init-lock-flags
cli: Restore -lock and -lock-timeout init flags
2021-10-19 09:45:15 -04:00
Alisdair McDiarmid c587384dff cli: Restore -lock and -lock-timeout init flags
The -lock and -lock-timeout flags were removed prior to the release of
1.0 as they were thought to have no effect. This is not true in the case
of state migrations when changing backends. This commit restores these
flags, and adds test coverage for locking during backend state
migration.

Also update the help output describing other boolean flags, showing the
argument as the user would type it rather than the default behavior.
2021-10-19 09:32:30 -04:00
Alisdair McDiarmid fb58f9e6d2 cli: Fix flaky init cancel test
There is a race between the MockSource and ShutdownCh which sometimes
causes this test to fail. Add a HangingSource implementation of Source
which hangs until the context is cancelled, so that there is always time
for a user-initiated shutdown to trigger the cancellation code path
under test.
2021-10-19 09:10:49 -04:00
Martin Atkins 39779e7021 backend/remote-state/cos: Don't use github.com/likexian/gokit
We don't use this library anywhere else in Terraform, and this backend was
using it only for trivial helpers that are easy to express inline anyway.
The new direct code is also type-checkable, whereas these helper functions
seem to be written using reflection.

This gives us one fewer dependency to worry about and makes the test code
for this backend follow a similar assertions style as the rest of this
codebase.
2021-10-14 10:50:05 -07:00
James Bardin ef3c98466d
Merge pull request #29755 from hashicorp/jbardin/first-plan-lineage
Check for stale plan with no state metadata
2021-10-14 13:31:30 -04:00
James Bardin 9c80574417 test planfile may need to have a specific lineage
In order to test applying a plan from an existing state, we need to be
able to inject the state meta into the planfile.
2021-10-13 17:28:14 -04:00
James Bardin 5ffa0839f9 only check serial when applying the first plan
Ensure that we still check for a stale plan even when it was created
with no previous state.

Create separate errors for incorrect lineage vs incorrect serial.

To prevent confusion when applying a first plan multiple times, only
report it as a stale plan rather than different lineage.
2021-10-13 17:17:51 -04:00
Martin Atkins bee7403f3e command/workspace_delete: Allow deleting a workspace with empty husks
Previously we would reject attempts to delete a workspace if its state
contained any resources at all, even if none of the resources had any
resource instance objects associated with it.

Nowadays there isn't any situation where the normal Terraform workflow
will leave behind resource husks, and so this isn't as problematic as it
might've been in the v0.12 era, but nonetheless what we actually care
about for this check is whether there might be any remote objects that
this state is tracking, and for that it's more precise to look for
non-nil resource instance objects, rather than whole resources.

This also includes some adjustments to our error messaging to give more
information about the problem and to use terminology more consistent with
how we currently talk about this situation in our documentation and
elsewhere in the UI.

We were also using the old State.HasResources method as part of some of
our tests. I considered preserving it to avoid changing the behavior of
those tests, but the new check seemed close enough to the intent of those
tests that it wasn't worth maintaining this method that wouldn't be used
in any main code anymore. I've therefore updated those tests to use
the new HasResourceInstanceObjects method instead.
2021-10-13 13:54:11 -07:00
Megan Bang 9b9b26a3cd update error message for invalid json 2021-10-13 13:51:07 -05:00
James Bardin 87e852c832
Merge pull request #29741 from hashicorp/jbardin/test-fix
fix test fixture with multiple provider instances
2021-10-13 09:53:02 -04:00
James Bardin 73b1263a86 wrap multiple provider creations into a factory fn
When a test uses multiple instances of the same provider, we may need to
have separate objects to prevent overwriting of the MockProvider state.
Create a completely new MockProvider in each factory function call
rather than re-using the original provider value.
2021-10-12 17:47:50 -04:00
Martin Atkins 965c0f3f91 build: Run staticcheck with "go run"
Running the tool this way ensures that we'll always run the version
selected by our go.mod file, rather than whatever happened to be available
in $GOPATH/bin on the system where we're running this.

This change caused some contexts to now be using a newer version of
staticcheck with additional checks, and so this commit also includes some
changes to quiet the new warnings without any change in overall behavior.
2021-10-12 11:42:17 -07:00
Martin Atkins d76759a6a9 configs/configload: snapshotDir must be used via pointer
A snapshotDir tracks its current position as part of its state, so we need
to use it via pointer rather than value so that Readdirnames can actually
update that position, or else we'll just get stuck at position zero.

In practice this wasn't hurting anything because we only call Readdir once
on our snapshots, to read the whole directory at once. Still nice to fix
to avoid a gotcha for future maintenence, though.
2021-10-12 11:42:17 -07:00
James Bardin 903ae5edfd fix test fixtures with multiple providers
Allow these to share the same backing MockProvider.
2021-10-12 14:34:34 -04:00
James Bardin 656f03b250 fix test fixture had the instance in the wrong mod
Make the state match the fixture config. The old test was not
technically invalid, but because it caused multiple instances of the
provider to be created, they were backed by the same MockProvider value
resulting in the `*Called` fields interfering.
2021-10-12 13:53:02 -04:00
James Bardin ef8f1b3e38
Merge pull request #29734 from hashicorp/jbardin/missing-change
Check for nil change during apply
2021-10-12 12:22:51 -04:00
Martin Atkins 2e6b6e9a6b go.mod: go get google.golang.org/protobuf@v1.27.1 2021-10-11 16:37:39 -07:00
James Bardin 3f4b680f1c Check for nil change during apply
Because NodeAbstractResourceInstance.readDiff can return a nil change,
we must check for that in all callers.
2021-10-08 16:46:29 -04:00
James Bardin 3c942ee3ac
Merge pull request #29720 from hashicorp/jbardin/destroy-plan-deposed
don't refresh deposed instances during the destroy plan
2021-10-08 16:23:59 -04:00
James Bardin a036109bc1 add comment about when we call ConfigureProvider 2021-10-08 15:23:36 -04:00
megan07 7dda3366a6
Update internal/backend/remote-state/gcs/backend.go
Co-authored-by: appilon <apilon@hashicorp.com>
2021-10-08 10:02:05 -05:00
James Bardin 03f71c2f06 fixup tests for MockProvider changes
Resetting the *Called fields and enforcing configuration broke a few
tests.
2021-10-08 08:42:06 -04:00
Megan Bang 81201d69a3 check valid json 2021-10-07 16:33:21 -05:00
Megan Bang 348c761bea add better error if credentials are invalid 2021-10-07 16:28:47 -05:00
James Bardin 22b400b8de skip refreshing deposed during destroy plan
The destroy plan should not require a configured provider (the complete
configuration is not evaluated, so they cannot be configured).

Deposed instances were being refreshed during the destroy plan, because
this instance type is only ever destroyed and shares the same
implementation between plan and walkPlanDestroy. Skip refreshing during
walkPlanDestroy.
2021-10-07 16:51:48 -04:00
James Bardin ad9944e523 test that providers are configured for calls
Have the MockProvider ensure that Configure is always called before any
methods that may require a configured provider.

Ensure the MockProvider *Called fields are zeroed out when re-using the
provider instance.
2021-10-07 16:48:56 -04:00
Alisdair McDiarmid b9f3dab035 json-output: Release format version 1.0 2021-10-06 11:13:06 -04:00
Omar Ismail bea7e3ebce
Backend State Migration: change variable names from one/two to source/destination (#29699) 2021-10-05 16:36:50 -04:00
Martin Atkins 01b22f4b76 command/e2etest: TestProviderTampering
We have various mechanisms that aim to ensure that the installed provider
plugins are consistent with the lock file and that the lock file is
consistent with the provider requirements, and we do have existing unit
tests for them, but all of those cases mock our fake out at least part of
the process and in the past that's caused us to miss usability
regressions, where we still catch the error but do so at the wrong layer
and thus generate error message lacking useful additional context.

Here we'll add some new end-to-end tests to supplement the existing unit
tests, making sure things work as expected when we assemble the system
together as we would in a release. These tests cover a number of different
ways in which the plugin selections can grow inconsistent.

These new tests all run only when we're in a context where we're allowed
to access the network, because they exercise the real plugin installer
codepath. We could technically build this to use a local filesystem mirror
or other such override to avoid that, but the point here is to make sure
we see the expected behavior in the main case, and so it's worth the
small additional cost of downloading the null provider from the real
registry.
2021-10-05 10:59:59 -07:00
Martin Atkins d09510a8fb command: Early error message for missing cache entries of locked providers
In the original incarnation of Meta.providerFactories we were returning
into a Meta.contextOpts whose signature didn't allow it to return an
error directly, and so we had compromised by making the provider factory
functions themselves return errors once called.

Subsequent work made Meta.contextOpts need to return an error anyway, but
at the time we neglected to update our handling of the providerFactories
result, having it still defer the error handling until we finally
instantiate a provider.

Although that did ultimately get the expected result anyway, the error
ended up being reported from deep in the guts of a Terraform Core graph
walk, in whichever concurrently-visited graph node happened to try to
instantiate the plugin first. This meant that the exact phrasing of the
error message would vary between runs and the reporting codepath didn't
have enough context to given an actionable suggestion on how to proceed.

In this commit we make Meta.contextOpts pass through directly any error
that Meta.providerFactories produces, and then make Meta.providerFactories
produce a special error type so that Meta.Backend can ultimately return
a user-friendly diagnostic message containing a specific suggestion to
run "terraform init", along with a short explanation of what a provider
plugin is.

The reliance here on an implied contract between two functions that are
not directly connected in the callstack is non-ideal, and so hopefully
we'll revisit this further in future work on the overall architecture of
the CLI layer. To try to make this robust in the meantime though, I wrote
it to use the errors.As function to potentially unwrap a wrapped version
of our special error type, in case one of the intervening layers is
changed at some point to wrap the downstream error before returning it.
2021-10-05 10:59:59 -07:00
James Bardin aece887a85
Merge pull request #29701 from hashicorp/jbardin/proposed-new-null-objs
objchange: fix ProposedNew from null objects
2021-10-05 13:09:49 -04:00
James Bardin 58d85fcc2e add test for planing unknown data source values 2021-10-05 12:31:23 -04:00
James Bardin 18d354223e objchange: fix ProposedNew from null objects
The codepath for AllAttributesNull was not correct for any nested object
types with collections, and should create single null values for the
correct NestingMode rather than a single object with null attributes.

Since there is no reason to descend into nested object types to create
nullv alues, we can drop the AllAttributesNull function altogether and
create null values as needed during ProposedNew.

The corresponding AllBlockAttributesNull was only called internally in 1
location, and simply delegated to schema.EmptyValue. We can reduce the
package surface area by dropping that function too and calling
EmptyValue directly.
2021-10-04 15:03:25 -04:00
James Bardin c68422d125
Merge pull request #29682 from hashicorp/jbardin/less-data-depends_on
Refine data depends_on dependency checks
2021-10-04 11:39:21 -04:00
Martin Atkins df578afd7e backend/local: Check dependency lock consistency before any operations
In historical versions of Terraform the responsibility to check this was
inside the terraform.NewContext function, along with various other
assorted concerns that made that function particularly complicated.

More recently, we reduced the responsibility of the "terraform" package
only to instantiating particular named plugins, assuming that its caller
is responsible for selecting appropriate versions of any providers that
_are_ external. However, until this commit we were just assuming that
"terraform init" had correctly selected appropriate plugins and recorded
them in the lock file, and so nothing was dealing with the problem of
ensuring that there haven't been any changes to the lock file or config
since the most recent "terraform init" which would cause us to need to
re-evaluate those decisions.

Part of the game here is to slightly extend the role of the dependency
locks object to also carry information about a subset of provider
addresses whose lock entries we're intentionally disregarding as part of
the various little edge-case features we have for overridding providers:
dev_overrides, "unmanaged providers", and the testing overrides in our
own unit tests. This is an in-memory-only annotation, never included in
the serialized plan files on disk.

I had originally intended to create a new package to encapsulate all of
this plugin-selection logic, including both the version constraint
checking here and also the handling of the provider factory functions, but
as an interim step I've just made version constraint consistency checks
the responsibility of the backend/local package, which means that we'll
always catch problems as part of preparing for local operations, while
not imposing these additional checks on commands that _don't_ run local
operations, such as "terraform apply" when in remote operations mode.
2021-10-01 14:43:58 -07:00
Martin Atkins 702413702c plans/planfile: Include dependency locks in saved plan files
We recently removed the legacy way we used to track the SHA256 hashes of
individual provider executables as part of a plans.Plan, because these
days we want to track the checksums of entire provider packages rather
than just the executable.

In order to achieve that new goal, we can save a copy of the dependency
lock information inside the plan file. This follows our existing precedent
of using exactly the same serialization formats we'd normally use for
this information, and thus we can reuse the existing models and
serializers and be confident we won't lose any detail in the round-trip.

As of this commit there's not yet anything actually making use of this
mechanism. In a subsequent commit we'll teach the main callers that write
and read plan files to include and expect (respectively) dependency
information, verifying that the available providers still match by the
time we're applying the plan.
2021-10-01 14:43:58 -07:00
Martin Atkins 3f85591998 depsfile: SaveLocksToBytes function
In a future commit we'll use this to include dependency lock information
in full fidelity inside a saved plan file.
2021-10-01 14:43:58 -07:00
Martin Atkins 6a98e4720c plans/planfile: Create takes most arguments via a struct type
Previously the planfile.Create function had accumulated probably already
too many positional arguments, and I'm intending to add another one in
a subsequent commit and so this is preparation to make the callsites more
readable (subjectively) and make it clearer how we can extend this
function's arguments to include further components in a plan file.

There's no difference in observable functionality here. This is just
passing the same set of arguments in a slightly different way.
2021-10-01 14:43:58 -07:00
Martin Atkins 8d193ad268 core: Simplify and centralize plugin availability checks
Historically the responsibility for making sure that all of the available
providers are of suitable versions and match the appropriate checksums has
been split rather inexplicably over multiple different layers, with some
of the checks happening as late as creating a terraform.Context.

We're gradually iterating towards making that all be handled in one place,
but in this step we're just cleaning up some old remnants from the
main "terraform" package, which is now no longer responsible for any
version or checksum verification and instead just assumes it's been
provided with suitable factory functions by its caller.

We do still have a pre-check here to make sure that we at least have a
factory function for each plugin the configuration seems to depend on,
because if we don't do that up front then it ends up getting caught
instead deep inside the Terraform runtime, often inside a concurrent
graph walk and thus it's not deterministic which codepath will happen to
catch it on a particular run.

As of this commit, this actually does leave some holes in our checks: the
command package is using the dependency lock file to make sure we have
exactly the provider packages we expect (exact versions and checksums),
which is the most crucial part, but we don't yet have any spot where
we make sure that the lock file is consistent with the current
configuration, and we are no longer preserving the provider checksums as
part of a saved plan.

Both of those will come in subsequent commits. While it's unusual to have
a series of commits that briefly subtracts functionality and then adds
back in equivalent functionality later, the lock file checking is the only
part that's crucial for security reasons, with everything else mainly just
being to give better feedback when folks seem to be using Terraform
incorrectly. The other bits are therefore mostly cosmetic and okay to be
absent briefly as we work towards a better design that is clearer about
where that responsibility belongs.
2021-10-01 14:43:58 -07:00
James Bardin 618e9cf8ec test for unexpected data reads 2021-09-30 17:13:33 -04:00
James Bardin 016463ea9c don't check all ancestors for data depends_on
Only depends_on ancestors for transitive dependencies when we're not
pointed directly at a resource. We can't be much more precise here,
since in order to maintain our guarantee that data sources will wait for
explicit dependencies, if those dependencies happen to be a module,
output, or variable, we have to find some upstream managed resource in
order to check for a planned change.
2021-09-30 16:43:09 -04:00
Zach Whaley c9a5fdb366
cliconfig: Fix error message about invalid credentials helper type 2021-09-29 13:36:59 -07:00
James Bardin f93f16824c
Merge pull request #29665 from hashicorp/jbardin/required_version
Check required_version as early as possible
2021-09-29 08:18:20 -04:00
James Bardin ab0322e406 remove debugging println 2021-09-28 17:58:40 -04:00
James Bardin c2e0d265cf LoadModule now always returns the module
We don't need to load the configuration twice, since configload can
return the module for us.
2021-09-28 17:58:40 -04:00
James Bardin a53faf43f6 return partial config from LoadConfig with errors
LoadConfig should return any parsed configuration in order for the
caller to verify `required_version`.
2021-09-28 13:30:03 -04:00
James Bardin 625e768678 make sure required_version is checked before diags
We must ensure that the terraform required_version is checked as early
as possible, so that new configuration constructs don't cause init to
fail without indicating the version is incompatible.

The loadConfig call before the earlyconfig parsing seems to be unneeded,
and we can delay that to de-tangle it from installing the modules which
may have their own constraints.

TODO: it seems that loadConfig should be able to handle returning the
version constraints in the same manner as loadSingleModule.
2021-09-28 13:30:03 -04:00
Martin Atkins f60d55d6ad core: Emit only one warning for move collisions in destroy-plan mode
Our current implementation of destroy planning includes secretly running a
normal plan first, in order to get its effect of refreshing the state.

Previously our warning about colliding moves would betray that
implementation detail because we'd return it from both of our planning
operations here and thus show the message twice. That would also have
happened in theory for any other warnings emitted by both plan operations,
but it's the move collision warning that made it immediately visible.

We'll now only return warnings from the initial plan if we're also
returning errors from that plan, and thus the warnings of both plans can
never mix together into the same diags and thus we'll avoid duplicating
any warnings.

This does mean that we'd lose any warnings which might hypothetically
emerge only from the hidden normal plan and not from the subsequent
destroy plan, but we'll accept that as an okay tradeoff here because those
warnings are likely to not be super relevant to the destroy case anyway,
or else we'd emit them from the destroy-plan walk too.
2021-09-27 15:46:36 -07:00
James Bardin 372814e49a
Merge pull request #29659 from hashicorp/jbardin/really-nested-within
refactoring: exhaustive NestedWithin checks
2021-09-27 12:55:04 -04:00