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

Add support for force pushing with the remote backend #24696

Merged
merged 2 commits into from May 6, 2020

Conversation

leetrout
Copy link

This PR adds support for the -force flag for state push with the remote backend. Per the documentation around manual state push / pull https://www.terraform.io/docs/backends/state.html#manual-state-pull-push both differing serials and lineage protections can be bypassed with the -force flag.

  • The first change you'll notice is the addition of a forcePush property on the remoteClient struct. This allows the client to know if it is operating in the context of a force push.

  • The second change is an optional interface ClientForcePusher for clients (like the remoteClient) that would need to know they are operating in the context of a force push. There was a force flag added to the Go TFE client that needs to be specified at persistence time.

  • The last change is tracking the serial and lineage in addition to the state. As noted above, lineage and serial can be overwritten with -force so we must look at them to determine if we can exit early from writing state.

To prevent changing every method signature of PersistState to accept force bool I added the optional interface. That provides a hook to flag the Client as operating in a force push context. Changing the method signature would be more explicit at the cost of not being used anywhere else currently or the optional interface pattern could be applied to the state itself so it could be upgraded to support PersistState(force bool) only when needed. Let me know if either of these seem preferable instead!

Background

The Go TFE client received support for a Force attribute in March 2019 hashicorp/go-tfe#63 however that was not brought over into Terraform via the remote backend. It is possible it worked until changes were made to prevent sending snapshots unless the resources had changed (otherwise the function exits early) 85eda8a

Existing remote state behaviors

I’ve compiled examples of the behavior of two backends, Postgres and S3 and how they currently work with and without the force flag. Lineages and serials are both flagged when trying to push but are correctly overridden when using the force flag as evidence by the diff when pulling after the force push.

https://gist.github.com/leetrout/51e605000fa1f59d1f6902023d685c8c

Manual testing

Configure Terraform Cloud

  1. Setup an organization
  2. Setup a workspace

Add Terraform config

terraform {
  backend "remote" {
    hostname = "<Terraform Cloud/Enterprise>"
    organization = "terraform-testing"

    workspaces {
      name = "testing"
    }
  }
}

resource "random_integer" "rando" {
  min     = 1
  max     = 50000
}

Create resource

  1. init
  2. apply
Terraform will perform the following actions:

  # random_integer.rando will be created
  + resource "random_integer" "rando" {
      + id     = (known after apply)
      + max    = 50000
      + min    = 1
      + result = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions in workspace "testing"?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_integer.rando: Creating...
random_integer.rando: Creation complete after 0s [id=23721]

Mutate lineage

In this case, change all 1's to 2's in the lineage:

terraform state pull > remote.tfstate
jq ".lineage=\"$(jq -r '.lineage' remote.tfstate | sed 's/1/2/g')\"" remote.tfstate > edited.tfstate

Confirm what changed with diff remote.tfstate edited.tfstate

5c5
<   "lineage": "dd577646-0d5c-d9ba-93b2-d2e04881ebb7",
---
>   "lineage": "dd577646-0d5c-d9ba-93b2-d2e04882ebb7",
30d29
<

Pushing

Try pushing (without -force) to ensure it doesn't work.

terraform state push edited.tfstate

Failed to write state: cannot import state with lineage "dd577646-0d5c-d9ba-93b2-d2e04882ebb7" over unrelated state with lineage "dd577646-0d5c-d9ba-93b2-d2e04881ebb7"

Now add the force flag:

terraform state push -force edited.tfstate

There should be no output but exit status of 0 and you should see the new state in TFC.

new-state

new-state-details

@codecov
Copy link

codecov bot commented Apr 17, 2020

Codecov Report

Merging #24696 into master will increase coverage by 0.03%.
The diff coverage is 88.23%.

Impacted Files Coverage Δ
state/remote/remote.go 0.00% <ø> (ø)
backend/remote/backend_state.go 57.31% <33.33%> (-0.92%) ⬇️
state/remote/state.go 67.30% <100.00%> (+19.43%) ⬆️
dag/walk.go 92.54% <0.00%> (-0.63%) ⬇️
terraform/evaluate.go 53.15% <0.00%> (+0.45%) ⬆️

@leetrout leetrout requested review from a team and removed request for apparentlymart and pselle April 17, 2020 16:15
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

Some questions. As well, could you look into adding tests for this change? Both for the enhanced remote backend, and some of the changes you've made (that I think are great) that ensure a more granular checking of state when deciding whether/not to push the state.

backend/remote/backend_state.go Outdated Show resolved Hide resolved
if err := statemgr.CheckValidImport(f, checkFile); err != nil {
return err
}
} else if isForcePusher {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused why the additional interface is needed, since there already is this bool related to force. Wouldn't a general else be what is needed?

Copy link
Author

Choose a reason for hiding this comment

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

Open to all suggestions on how to make the comment in L68-L71 make more sense 🙏

The CLI snags the flag and drops it in this function where the s.Client is just the Client interface so L72 checks to see if the optional ClientForcePusher is implemented.

Would you rather change it to inline the check and remove the prior check and descriptive isForcePusher for the if-ok pattern?

else if c, ok := s.Client.(ClientForcePusher); ok {
	c.EnableForcePush()
}

lineageUnchanged := s.readLineage != "" && s.lineage == s.readLineage
serialUnchanged := s.readSerial != 0 && s.serial == s.readSerial
stateUnchanged := statefile.StatesMarshalEqual(s.state, s.readState)
if stateUnchanged && lineageUnchanged && serialUnchanged {
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate the addition of these more granular checks -- we'll probably want to note this in the changelog as part of the changes included in this changeset (more specific checks for whether state needs to be updated)

Copy link
Author

Choose a reason for hiding this comment

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

👍 will do

state/remote/state.go Outdated Show resolved Hide resolved
state/remote/state.go Show resolved Hide resolved
We only persist a new state if the actual state contents have
changed. This test demonstrates that behavior by calling write
and persist methods when either the lineage or serial have changed.
@leetrout leetrout force-pushed the leetrout/remote-state-force-push branch 2 times, most recently from 5f06a69 to 7fa1016 Compare May 5, 2020 00:30
Copy link
Contributor

@pselle pselle left a comment

Choose a reason for hiding this comment

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

This looks great! One comment about a comment (that "Implements.." pattern we talked about, but this looks really good.

state/remote/remote_test.go Show resolved Hide resolved
state/remote/state.go Show resolved Hide resolved
state/remote/state.go Show resolved Hide resolved
Both differing serials and lineage protections should be bypassed
with the -force flag (in addition to resources).

Compared to other backends we aren’t just shipping over the state
bytes in a simple payload during the persistence phase of the push
command and the force flag added to the Go TFE client needs to be
specified at that time.

To prevent changing every method signature of PersistState of the
remote client I added an optional interface that provides a hook
to flag the Client as operating in a force push context. Changing
the method signature would be more explicit at the cost of not
being used anywhere else currently or the optional interface pattern
could be applied to the state itself so it could be upgraded to
support PersistState(force bool) only when needed.

Prior to this only the resources of the state were checked for
changes not the lineage or the serial. To bring this in line with
documented behavior noted above those attributes also have a “read”
counterpart just like state has. These are now checked along with
state to determine if the state as a whole is unchanged.

Tests were altered to table driven test format and testing was
expanded to include WriteStateForMigration and its interaction
with a ClientForcePusher type.
@leetrout leetrout force-pushed the leetrout/remote-state-force-push branch from 19d2088 to cb0e20c Compare May 6, 2020 16:11
@pselle pselle merged commit 60b3815 into master May 6, 2020
@pselle pselle deleted the leetrout/remote-state-force-push branch May 6, 2020 19:23
@ghost
Copy link

ghost commented Jun 6, 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.

@hashicorp hashicorp locked and limited conversation to collaborators Jun 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants