-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Conversation
Codecov Report
|
There was a problem hiding this 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.
state/remote/state.go
Outdated
if err := statemgr.CheckValidImport(f, checkFile); err != nil { | ||
return err | ||
} | ||
} else if isForcePusher { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 will do
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.
5f06a69
to
7fa1016
Compare
There was a problem hiding this 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.
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.
19d2088
to
cb0e20c
Compare
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. |
This PR adds support for the
-force
flag forstate 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 theremoteClient
) 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 acceptforce bool
I added the optional interface. That provides a hook to flag theClient
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 supportPersistState(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) 85eda8aExisting 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
Add Terraform config
Create resource
Mutate lineage
In this case, change all 1's to 2's in the lineage:
Confirm what changed with
diff remote.tfstate edited.tfstate
Pushing
Try pushing (without
-force
) to ensure it doesn't work.terraform state push edited.tfstate
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.