Apply a confirmable run when given a saved cloud plan #33270

nfagerlund · 2023-05-26T20:36:14Z

This PR is about half? a third? of the Terraform Core portion of the saved cloud plans project. It adds the ability to pass terraform apply a saved cloud plan file (actually a little JSON blob that refers to a run by its ID and its TFC instance hostname), and have Terraform confirm the referenced run and display the outcome of applying it.

Testing instructions

Interestingly, this ability does not depend on the TFC changes to add saved-plan runs as a new run type! It works today, sort of! To test it, you'll need to:

Configure the cloud backend in a local config directory.
Start a (applicable) run in the currently selected workspace, and allow the plan and checks to finish so that it's awaiting confirmation. (Easiest is probably to terraform apply on the CLI and then ctrl-C instead of approving.)
Note the run ID, and create a little json file with the following format:

{
    "remote_plan_format": 1,
    "run_id": "run-y3X9kafnB8hfwxxx",
    "hostname": "app.terraform.io"
}

Run terraform apply <PATH TO JSON FILE>
Try some more times, with runs that aren't in a good-to-go state. For example: a planned-and-finished run with no changes, a run that was already applied, a discarded run, an errored run, a run that's still planning, etc.

About the automated tests

I managed to get a nice concise test for the happy path of applying a run that's ready, but I wasn't able to craft a test for the "error if already applied" case or its siblings; something about crafting mock runs or duplicate Operation structs eluded me.
I also couldn't figure out how to test the apply command with this new behavior — up at that highest level of the internal packages, I couldn't see a sensible way to teach Terraform about mocking the TFC APIs! 😓 This might be worthy of an integration test against real TFC, but that won't really be practical until the rest of the work in this project is closer to done.

If you see some way around those issues, or if you spot other things I should be testing in more detail, please let me know!

Sidenotes

I originally had this idea of configuring the cloud backend from ONLY a run ID and hostname, because once you have a client you can just traverse through relationships and get the workspace and organization names from the server. However, it turned out to be more trouble than it was worth — since you HAVE to be running commands from the config working directory anyway, there's no real upside, so we might as well load the backend from the config like we always do.

Target Release

1.6.x

We're now in a decent position to switch behavior for cloud plans within the apply command.

… so in apply command Faking up the backend config cty object for this alternate path is kind of strange and awkward, and these cloud backend setup functions are asking for a bigger refactor... but this gets it working with a minimally intrusive diff.

- Skips confirmation, like applying a local saved plan. - Gives more detailed errors than a local plan can; with local, we just know the serial and lineage isn't right, but with cloud we can actually tell you what happened to that specific run.

…lobally

As with slices, Append consumes its argument instead of mutating in-place.

`Cloud.setOrgAndWorkspaceFromRun` relies on including the run's workspace, so it can get the organization name and the workspace name in a single request. This allows the mocks to cope with that. It doesn't yet add support for all the possible run includes, since that seems premature.

The extra code complexity doesn't seem worth it, since you need to be inside the config directory (and able to load the config) in order to apply the run regardless.

apparentlymart · 2023-06-13T23:15:37Z

internal/plans/planfile/wrapped.go

+	Local *Reader
+	Cloud *cloudplan.SavedPlanBookmark


I might consider making both of these fields unexported and then changing the two accessor methods to be like func (w *WrappedPlanFile) Local() (*Reader, bool) (and similar for Cloud) so that the caller can't so easily avoid checking which variant is selected, and thus it's hopefully more obvious how to use this API correctly. I don't feel super strongly about it, but just suggesting it in case it seems viable and worthwhile to you.

Ah, thanks! I'll wait to hear from the other reviewers, but it seems like probably a reasonable improvement to make.

this sounds good to me!

Additionally, it seems to me that it is currently possible to set both Local & Cloud plan even though the intention is to disallow it, in which case the func OpenWrapped... will pick the Local plan since it is the first block. I think it might be useful to check that both Local and Cloud are not set and return an error if they are, before proceeding to open the local or cloud plan file.

OK, I did that refactor! 👍🏼 And as for setting both fields — I made a pair of constructor functions for tests that need to fake up one of these structs, and those plus OpenWrapped are the only things allowed to actually build one of these; that should make it impossible to get ahold of a malformed one.

brandonc · 2023-06-15T20:38:48Z

internal/cloud/backend_apply.go

This was really exciting to smoke test! I think I wanted to see the familiar remote execution yellow header that you get with plan. Like, here was the output:

$ ./terraform apply plan.json null_resource.hello: Destroying... [id=6984397633657598306] null_resource.hello: Destruction complete after 0s null_resource.hello: Creating... null_resource.hello: Creation complete after 0s [id=4736096538695284461] Apply complete! Resources: 1 added, 0 changed, 1 destroyed. Outputs: name = "hello"

It think it would be a good idea to re-render the applyDefaultHeader so that I can be reasonably sure this output came from the remote run.

This is awesome! It looks like you found a lot of corner cases I hadn't previously considered.

Uk1288

Good work on this!

General suggestions:

While testing with speculative runs that have changes, the error message currently says: Error: Saved plan has no changes │ The given plan file contains no changes, so it cannot be applied…. Should the message in this case be something like, speculative plans cannot be applied...
When attempting to confirm a plan that is awaiting override approval for soft policy error, I wonder if the error message should be more descriptive, indicating that the run is waiting for override. The current message displays: Error: Saved plan cannot be applied │ Terraform Cloud cannot apply the given plan file. This may mean the plan and checks have not yet completed, or │ may indicate another problem
The link to the run only shows up if confirmation resulted in an error, adding the run link even for a successful case might be helpful.

Uk1288 · 2023-06-15T23:19:56Z

internal/cloud/tfe_client_mock.go

+			case tfe.RunWorkspace:
+				ws, ok := m.client.Workspaces.workspaceIDs[r.Workspace.ID]
+				if ok {
+					r.Workspace = ws


should probably break out of the loop once the expected workspace is found and set on the Run.

Hmm, should we though?? We're not looping through workspaces here, we're just looping through the list of ?include= parameters that got set on the request. There would only be like, five? total if you set them all... and theoretically we would handle all of them, I just didn't bother coding the others yet because YAGNI.

If we were looping through workspaces, that'd definitely want an early loop exit.

Uk1288 · 2023-06-15T23:34:56Z

internal/plans/planfile/planfile_test.go

+
+func TestWrappedError(t *testing.T) {
+	// Open something that isn't a cloud or local planfile: should error
+	wrongFile := "not a valid zip file"


while testing with a yaml file type, I was expecting the error message to describe the expected file format, json in this case: i.e not a valid json file. Is this a general error for wrong file types?

Following the current error message, i attempted to pass in a zip file version of the plan file and I got this error: Error: the given file is not a valid plan file.

I ended up addressing this by combining both errors, in the hope that at least one of them will hint at what the user did wrong. That said, because these are both meant as opaque formats, the exact details of the errors aren't really meant for normal users to think too hard about — they hopefully just point the way to remembering which file you meant to click.

Uk1288 · 2023-06-15T23:49:41Z

internal/plans/planfile/wrapped.go

+	Local *Reader
+	Cloud *cloudplan.SavedPlanBookmark


this sounds good to me!

Additionally, it seems to me that it is currently possible to set both Local & Cloud plan even though the intention is to disallow it, in which case the func OpenWrapped... will pick the Local plan since it is the first block. I think it might be useful to check that both Local and Cloud are not set and return an error if they are, before proceeding to open the local or cloud plan file.

nfagerlund · 2023-06-16T00:37:31Z

Good catches, all! I'll revise and follow up.

…public fields

In an earlier draft this wasn't necessary, since we were constructing a backend from the saved plan runID+host, but now it's a useful extra check to do.

This condition isn't really possible for classic decisive-apply runs, but it IS possible for saved-plan runs, which are currently feature-flagged.

nfagerlund · 2023-06-17T00:04:27Z

@Uk1288 @brandonc OK, this is ready for re-review!

New error for locked workspace (need to make a "save-plan": true run while another run is current, to be able to see that one. But don't worry about it too much, since that's feature flagged anyway.)
Shows a run header (with URL) when starting up, now!
Refactored access patterns for planfile.WrappedPlanFile.
Some lil stuff.

This reverts commit 83b2763. Unfortunately, this actually can't be checked safely! A locked workspace check has to also check whether the workspace is locked by the entity that's about to attempt locking it, and go-tfe does not support the `locked_by` relationship on workspaces (probably because it's polymorphic run/team/user).

Uk1288

LGTM 🎉

github-actions · 2023-06-21T02:06:32Z

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

It displays a run header with link to web UI, like starting a new plan does, then confirms the run and streams the apply logs. If you can't apply the run (it's from a different workspace, is in an unconfirmable state, etc. etc.), it displays an error instead. Notable points along the way: * Implement `WrappedPlanFile` sum type, and update planfile consumers to use it instead of a plain `planfile.Reader`. * Enable applying a saved cloud plan * Update TFC mocks — add org name to workspace, and minimal support for includes on MockRuns.ReadWithOptions.

github-actions · 2023-12-14T02:07:25Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

vercel bot deployed to Preview May 26, 2023 20:38 View deployment

vercel bot deployed to Preview May 26, 2023 21:12 View deployment

vercel bot deployed to Preview May 26, 2023 22:57 View deployment

vercel bot deployed to Preview May 26, 2023 23:28 View deployment

vercel bot deployed to Preview May 27, 2023 00:13 View deployment

nfagerlund force-pushed the nf/may23-apply-a-confirmable-run branch from f261f81 to 4ecd3bb Compare June 5, 2023 23:38

vercel bot deployed to Preview June 5, 2023 23:41 View deployment

vercel bot deployed to Preview June 6, 2023 00:40 View deployment

vercel bot deployed to Preview June 6, 2023 01:48 View deployment

vercel bot deployed to Preview June 6, 2023 03:20 View deployment

nfagerlund force-pushed the nf/may23-apply-a-confirmable-run branch from 7ba32a2 to 041753b Compare June 6, 2023 03:46

vercel bot deployed to Preview June 6, 2023 03:49 View deployment

nfagerlund force-pushed the nf/may23-apply-a-confirmable-run branch from 041753b to 765bb2f Compare June 7, 2023 01:06

vercel bot deployed to Preview June 7, 2023 01:09 View deployment

nfagerlund added 7 commits June 8, 2023 18:00

Implement WrappedPlanFile sum type

ead0a5e

Change (*Meta).PlanFile() (and callers) to use OpenWrapped

5ddd6b4

We're now in a decent position to switch behavior for cloud plans within the apply command.

Update backend.Operation struct to use planfile.WrappedPlanFile

7d3ecb0

Enable applying a saved cloud plan

0ec83cf

- Skips confirmation, like applying a local saved plan. - Gives more detailed errors than a local plan can; with local, we just know the serial and lineage isn't right, but with cloud we can actually tell you what happened to that specific run.

Enable correct behavior when token is stored in config, rather than g…

cb8649e

…lobally

Fix expected error in local planfile w/ cloud test

76e1a6e

nfagerlund force-pushed the nf/may23-apply-a-confirmable-run branch from 765bb2f to 76e1a6e Compare June 9, 2023 01:01

vercel bot deployed to Preview June 9, 2023 01:05 View deployment

nfagerlund added 7 commits June 8, 2023 18:18

Add tests for planfile.OpenWrapped

4b87bfc

Add test for applying a saved cloud plan

9e7b93c

Bugfix: Forgot to reassign diags in backend_local

0da5789

As with slices, Append consumes its argument instead of mutating in-place.

Add test for failing to create local run with cloud plan

7607c88

Add missing mock in testUnconfiguredBackend

1887da0

tfc mocks: add organization name to workspaces

34b75b4

nfagerlund added 2 commits June 12, 2023 14:55

Rip out backend-from-run-ID functionality

9a99bd6

The extra code complexity doesn't seem worth it, since you need to be inside the config directory (and able to load the config) in order to apply the run regardless.

Add check for runs from the wrong workspace

793417b

vercel bot deployed to Preview June 12, 2023 23:06 View deployment

apparentlymart reviewed Jun 13, 2023

View reviewed changes

brandonc reviewed Jun 15, 2023

View reviewed changes

Uk1288 reviewed Jun 16, 2023

View reviewed changes

nfagerlund added 6 commits June 15, 2023 18:04

Refactor planfile.WrappedPlanFile to use checking getters instead of …

5c2c24a

…public fields

Check hostname before reading run to apply cloud plan

fb8da19

In an earlier draft this wasn't necessary, since we were constructing a backend from the saved plan runID+host, but now it's a useful extra check to do.

Add error for policy override status

aae26d7

Add remote run header when applying saved plan

df58a56

Include both errors when failing to open a WrappedPlanFile

689b971

Add error handling for locked workspace

83b2763

This condition isn't really possible for classic decisive-apply runs, but it IS possible for saved-plan runs, which are currently feature-flagged.

vercel bot deployed to Preview June 16, 2023 23:55 View deployment

vercel bot deployed to Preview June 20, 2023 18:30 View deployment

Add unusable plan message for Errored plans

d30bc78

vercel bot deployed to Preview June 20, 2023 18:34 View deployment

Uk1288 approved these changes Jun 20, 2023

View reviewed changes

nfagerlund merged commit f30bbdb into cli-team/saved-cloud-plans Jun 21, 2023

nfagerlund deleted the nf/may23-apply-a-confirmable-run branch June 21, 2023 02:06

nfagerlund mentioned this pull request Jul 7, 2023

Implement saved cloud plans #33492

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply a confirmable run when given a saved cloud plan #33270

Apply a confirmable run when given a saved cloud plan #33270

nfagerlund commented May 26, 2023 •

edited

Loading

apparentlymart Jun 13, 2023

nfagerlund Jun 14, 2023

Uk1288 Jun 15, 2023

nfagerlund Jun 17, 2023

brandonc Jun 15, 2023

Uk1288 left a comment •

edited by nfagerlund

Loading

Uk1288 Jun 15, 2023

nfagerlund Jun 16, 2023

Uk1288 Jun 15, 2023

nfagerlund Jun 16, 2023

Uk1288 Jun 15, 2023

nfagerlund commented Jun 16, 2023

nfagerlund commented Jun 17, 2023

Uk1288 left a comment

github-actions bot commented Jun 21, 2023

github-actions bot commented Dec 14, 2023

Apply a confirmable run when given a saved cloud plan #33270

Apply a confirmable run when given a saved cloud plan #33270

Conversation

nfagerlund commented May 26, 2023 • edited Loading

Testing instructions

About the automated tests

Sidenotes

Target Release

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uk1288 left a comment • edited by nfagerlund Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfagerlund commented Jun 16, 2023

nfagerlund commented Jun 17, 2023

Uk1288 left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 21, 2023

github-actions bot commented Dec 14, 2023

nfagerlund commented May 26, 2023 •

edited

Loading

Uk1288 left a comment •

edited by nfagerlund

Loading