14 Commits (94de72c63834acea811c27a585c19eae29374a4b)

Author	SHA1	Message	Date
Alisdair McDiarmid	d2087fa98b	states/remote: Check for LockError error type ... When attempting to lock a remote state backend, failure due to an existing lock should return an instance of LockError. This allows the wrapping code to retry until the specified timeout, instead of immediately exiting. This commit adds a test for this in the TestRemoteLocks test helper, which is used in many of the remote state backend test suites.	4 years ago
James Bardin	34a01c92d6	copy Dependencies before sorting in state ... Instances of the same AbsResource may share the same Dependencies, which could point to the same backing array of values. Since address values are not pointers, and not meant to be shared, we must copy the value before sorting the slice in-place. Because individual instances of the same resource may be encoded to state concurrently, failure to copy the slice first can result in a data race.	4 years ago
chavacava	5f58daaf57	fix typo in struct tag (#30884) ... typo found by [`revive`](https://github.com/mgechev/revive)	4 years ago
Eng Zer Jun	fedd315275	test: use `T.TempDir` to create temporary test directory (#30803) ... This commit replaces `ioutil.TempDir` with `t.TempDir` in tests. The directory created by `t.TempDir` is automatically removed when the test and all its subtests complete. Prior to this commit, temporary directory created using `ioutil.TempDir` needs to be removed manually by calling `os.RemoveAll`, which is omitted in some tests. The error handling boilerplate e.g. defer func() { if err := os.RemoveAll(dir); err != nil { t.Fatal(err) } } is also tedious, but `t.TempDir` handles this for us nicely. Reference: https://pkg.go.dev/testing#T.TempDir Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>	4 years ago
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.	4 years ago
Alisdair McDiarmid	df36a03be1	states: Add failing test for ordered dependencies	4 years ago
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.	4 years ago
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.	5 years ago
Martin Atkins	f0034beb33	core: refactoring.ImpliedMoveStatements replaces NodeCountBoundary ... Going back a long time we've had a special magic behavior which tries to recognize a situation where a module author either added or removed the "count" argument from a resource that already has instances, and to silently rename the zeroth or no-key instance so that we don't plan to destroy and recreate the associated object. Now we have a more general idea of "move statements", and specifically the idea of "implied" move statements which replicates the same heuristic we used to use for this behavior, we can treat this magic renaming rule as just another "move statement", special only in that Terraform generates it automatically rather than it being written out explicitly in the configuration. In return for wiring that in, we can now remove altogether the NodeCountBoundary graph node type and its associated graph transformer, CountBoundaryTransformer. We handle moves as a preprocessing step before building the plan graph, so we no longer need to include any special nodes in the graph to deal with that situation. The test updates here are mainly for the graph builders themselves, to acknowledge that indeed we're no longer inserting the NodeCountBoundary vertices. The vertices that NodeCountBoundary previously depended on now become dependencies of the special "root" vertex, although in many cases here we don't see that explicitly because of the transitive reduction algorithm, which notices when there's already an equivalent indirect dependency chain and removes the redundant edge. We already have plenty of test coverage for these "count boundary" cases in the context tests whose names start with TestContext2Plan_count and TestContext2Apply_resourceCount, all of which continued to pass here without any modification and so are not visible in the diff. The test functions particularly relevant to this situation are: - TestContext2Plan_countIncreaseFromNotSet - TestContext2Plan_countDecreaseToOne - TestContext2Plan_countOneIndex - TestContext2Apply_countDecreaseToOneCorrupted The last of those in particular deals with the situation where we have both a no-key instance _and_ a zero-key instance in the prior state, which is interesting here because to exercises an intentional interaction between refactoring.ImpliedMoveStatements and refactoring.ApplyMoves, where we intentionally generate an implied move statement that produces a collision and then expect ApplyMoves to deal with it in the same way as it would deal with all other collisions, and thus ensure we handle both the explicit and implied collisions in the same way. This does affect some UI-level tests, because a nice side-effect of this new treatment of this old feature is that we can now report explicitly in the UI that we're assigning new addresses to these objects, whereas before we just said nothing and hoped the user would just guess what had happened and why they therefore weren't seeing a diff. The backend/local plan tests actually had a pre-existing bug where they were using a state with a different instance key than the config called for but getting away with it because we'd previously silently fix it up. That's still fixed up, but now done with an explicit mention in the UI and so I made the state consistent with the configuration here so that the tests would be able to recognize _real_ differences where present, as opposed to the errant difference caused by that inconsistency.	5 years ago
Martin Atkins	89b05050ec	core: Functional-style API for terraform.Context ... Previously terraform.Context was built in an unfortunate way where all of the data was provided up front in terraform.NewContext and then mutated directly by subsequent operations. That made the data flow hard to follow, commonly leading to bugs, and also meant that we were forced to take various actions too early in terraform.NewContext, rather than waiting until a more appropriate time during an operation. This (enormous) commit changes terraform.Context so that its fields are broadly just unchanging data about the execution context (current workspace name, available plugins, etc) whereas the main data Terraform works with arrives via individual method arguments and is returned in return values. Specifically, this means that terraform.Context no longer "has-a" config, state, and "planned changes", instead holding on to those only temporarily during an operation. The caller is responsible for propagating the outcome of one step into the next step so that the data flow between operations is actually visible. However, since that's a change to the main entry points in the "terraform" package, this commit also touches every file in the codebase which interacted with those APIs. Most of the noise here is in updating tests to take the same actions using the new API style, but this also affects the main-code callers in the backends and in the command package. My goal here was to refactor without changing observable behavior, but in practice there are a couple externally-visible behavior variations here that seemed okay in service of the broader goal: - The "terraform graph" command is no longer hooked directly into the core graph builders, because that's no longer part of the public API. However, I did include a couple new Context functions whose contract is to produce a UI-oriented graph, and _for now_ those continue to return the physical graph we use for those operations. There's no exported API for generating the "validate" and "eval" graphs, because neither is particularly interesting in its own right, and so "terraform graph" no longer supports those graph types. - terraform.NewContext no longer has the responsibility for collecting all of the provider schemas up front. Instead, we wait until we need them. However, that means that some of our error messages now have a slightly different shape due to unwinding through a differently-shaped call stack. As of this commit we also end up reloading the schemas multiple times in some cases, which is functionally acceptable but likely represents a performance regression. I intend to rework this to use caching, but I'm saving that for a later commit because this one is big enough already. The proximal reason for this change is to resolve the chicken/egg problem whereby there was previously no single point where we could apply "moved" statements to the previous run state before creating a plan. With this change in place, we can now do that as part of Context.Plan, prior to forking the input state into the three separate state artifacts we use during planning. However, this is at least the third project in a row where the previous API design led to piling more functionality into terraform.NewContext and then working around the incorrect order of operations that produces, so I intend that by paying the cost/risk of this large diff now we can in turn reduce the cost/risk of future projects that relate to our main workflow actions.	5 years ago
Martin Atkins	383bbdeebc	Upgrade to Go 1.17 ... This includes the addition of the new "//go:build" comment form in addition to the legacy "// +build" notation, as produced by gofmt to ensure consistent behavior between Go versions. The new directives are all equivalent to what was present before, so there's no change in behavior. Go 1.17 continues to use the Unicode 13 tables as in Go 1.16, so this upgrade does not require also upgrading our Unicode-related dependencies. This upgrade includes the following breaking changes which will also appear as breaking changes for Terraform users, but that are consistent with the Terraform v1.0 compatibility promises. - On MacOS, Terraform now requires macOS 10.13 High Sierra or later. This upgrade also includes the following breaking changes which will appear as breaking changes for Terraform users that are inconsistent with our compatibility promises, but have justified exceptions as follows: - cidrsubnet, cidrhost, and cidrnetmask will now reject IPv4 CIDR addresses whose decimal components have leading zeros, where previously they would just silently ignore those leading zeros. This is a security-motivated exception to our compatibility promises, because some external systems interpret zero-prefixed octets as octal numbers rather than decimal, and thus the previous lenient parsing could lead to a different interpretation of the address between systems, and thus potentially allow bypassing policy when configuring firewall rules etc. This upgrade also includes the following breaking changes which could _potentially_ appear as breaking changes for Terraform users, but that do not in practice for the reasons given: - The Go net/url package no longer allows query strings with pairs separated by semicolons instead of ampersands. This primarily affects HTTP servers written in Go, and Terraform includes a special temporary HTTP server as part of its implementation of OAuth for "terraform login", but that server only needs to accept URLs created by Terraform itself and Terraform does not generate any URLs that would be rejected.	5 years ago
Kristin Laemmert	43f698dc9d	states: new `Move` funcs for Resource, ResourceInstance, and ModuleInstances (#29068) ... * states: add MoveAbsResource and MoveAbsResourceInstance state functions and corresponding syncState wrapper functions. * states: add MoveModuleInstance and MaybeMoveModuleInstance * addrs: adding a new function, ModuleInstance.IsDeclaredByCall, which returns true if the receiver is an instance of the given AbsModuleCall.	5 years ago
James Bardin	d9dfd451ea	update to use typed sensitive marks	5 years ago
Martin Atkins	f40800b3a4	Move states/ to internal/states/ ... This is part of a general effort to move all of Terraform's non-library package surface under internal in order to reinforce that these are for internal use within Terraform only. If you were previously importing packages under this prefix into an external codebase, you could pin to an earlier release tag as an interim solution until you've make a plan to achieve the same functionality some other way.	5 years ago