-
Notifications
You must be signed in to change notification settings - Fork 10.1k
PSS: Update how commands access backends, so both backend and state_store configuration can be used
#37569
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
base: main
Are you sure you want to change the base?
Conversation
| func (c *ShowCommand) getDataFromCloudPlan(plan *cloudplan.SavedPlanBookmark, redacted bool) (*cloudplan.RemotePlanJSON, error) { | ||
| mod, diags := c.Meta.loadSingleModule(".") | ||
| if diags.HasErrors() { | ||
| return nil, diags.Err() | ||
| } | ||
|
|
||
| // Set up the backend | ||
| b, backendDiags := c.Backend(nil) | ||
| if backendDiags.HasErrors() { | ||
| return nil, errUnusable(backendDiags.Err(), "cloud plan") | ||
| b, diags := c.Meta.prepareBackend(mod) | ||
| if diags.HasErrors() { | ||
| return nil, errUnusable(diags.Err(), "cloud plan") | ||
| } |
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 currently doesn't have any test coverage
f8d4dcc to
57f1d85
Compare
57f1d85 to
3df8563
Compare
| } | ||
| // TODO: Update BackendForLocalPlan to use state storage, and plan to be able to contain State Store config details | ||
| be, beDiags = c.BackendForLocalPlan(plan.Backend) |
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 think this is scoped to this ticket: https://hashicorp.atlassian.net/browse/TF-28374
I've linked here from that ticket.
…nfig parsing needs to be explicitly added
… can use already parsed config
…eds to be refactored
…in use within operations backend
3df8563 to
823c5b4
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.
Some early thoughts after briefly glancing over the diffs
- (mostly stylistic) Can we drop all that
.Metastuff from the calling code?Metais embedded struct inside of every command and it seems unnecessarily verbose to use that notation? - If we now call
loadSingleModule()to load the module configuration from various places, doesn't it mean that any diagnostics which aren't directly related to the backend or state store would prevent it from being initialised? 🤔
I can dig deeper and think more the effects and side effects, especially in relation to (2) later.
UPDATE:
Re (2) Upon closer inspection I can now see that the pre-existing logic already calls loadSingleModule and so there is no change in terms of what diagnostics would be raised and when, I suppose. The change just makes this effect more obvious.
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 comments in-line.
Regarding the note about missing test - I think the error case of cloud backend decoding could be covered by a new test in internal/command/meta_backend_test.go?
One more meta-question (no pun intended):
Could we make similar change to the earlier one and drop the .Meta from all of the c.Meta.loadSingleModule?
internal/command/meta_backend.go
Outdated
| // This method should be used in NON-init operations only; it's incapable of processing new init command CLI flags used | ||
| // for partial configuration, however it will use the backend state file to use partial configuration from a previous | ||
| // init command. | ||
| func (m *Meta) prepareBackend(root *configs.Module) (backendrun.OperationsBackend, tfdiags.Diagnostics) { |
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.
Mostly readability related nitpick: I'd consider making this method's arguments a bit less "greedy", e.g. backend, cloudConfig, stateStore.
Then it makes it more obvious to the reader from all the calling places that this method cannot possibly do any business with the rest of the module.
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 could resolve this by making this method load the root module, instead of using data from the calling code.
That would address this concern about duplicated warnings and resembles the original backendConfig method used.
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.
Ah but now I take a proper look I realise that one of the things I was trying to do was avoid the root module being parsed multiple times (e.g. in internal/command/providers.go the diff uses some already-parsed config as the argument to this method).
I'll think on it more.
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.
See #37569 (comment)
|
One more note - sorry for the chaos! I'd say this note in the existing code is relatively important: terraform/internal/command/meta_config.go Lines 147 to 151 in 2e5b5de
I'm just unsure how to best preserve it in the wide-scale refactoring here. |
…to make relationship to (Meta).Backend more obvious.
… adhere to calling code's viewtype instructions
| // Only return error diagnostics at this point. Any warnings will be caught | ||
| // again later and duplicated in the output. | ||
| root, mDiags := m.loadSingleModule(configPath) | ||
| if mDiags.HasErrors() { | ||
| diags = diags.Append(mDiags) | ||
| return nil, diags | ||
| } |
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've made the new backend method resume being the code to parse the configuration for backend/state_store config. This means that some commands parse the configuration twice, as this code is no longer able to accept parsed config as input.
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.
Ack, I expect this will have some performance impact but also I hope/assume it will be negligible in the grand scheme of things.
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.
Yeah agreed - I think only a few commands would be negatively affected so it's not too bad
| b, backendDiags := c.Backend(&BackendOpts{ | ||
| BackendConfig: backendConfig, | ||
| }) | ||
| b, backendDiags := c.backend(".", arguments.ViewHuman) |
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 know this isn't related to your PR really but it's interesting to note the graph command output is always "machine-readable" in the sense that it's DOT language which a human would first pipe into something like graphviz before they can read it. 😅
Though it's not JSON, so neither of the two view types we have are a good match. 🤷🏻
And yes - mimicking the existing default behaviour is a very sensible approach!
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.
What I like about the new changes is how they highlight the differences between various commands and the output formats and maybe make it easier to spot some gaps/opportunities for adding machine-readable output.
internal/command/output.go
Outdated
|
|
||
| // Load the backend | ||
| b, backendDiags := c.Backend(nil) | ||
| b, backendDiags := c.backend(".", arguments.ViewHuman) |
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 command does support both JSON and Human views so I think we should pass the chosen type as an argument instead of hard-coding to ViewHuman here?
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.
Good catch! In that case we may be potentially fixing a bug here; the original code didn't pass a view type in.
…ggable state store
|
Thanks for that feedback @radeksimko ! When we spoke about testing we discussed using E2E tests on the happy path - I've got that work in draft separate to this PR, and I think the testing for this PR is just that we didn't break usage of |
Context & how code was structured before this PR
Terraform command implementations obtain an operations backends to use from the
(Meta).Backendmethod. This operations backend may be used for accessing state and/or for running operations.When using the (Meta).Backend method, calling code may include parsed
backendconfiguration in theBackendOptsargument passed into theBackendmethod:Or, the calling code could pass a
nilvalue into theBackendmethod and instead rely on downstream logic to parse anybackendblocks in the configuration (this happens if the BackendOpts.BackendConfig value is missing).Changes in this PR
This PR adds a new method: *(m Meta) prepareBackend
This method makes it easier to access an instance of an operations backend. This method parses any
backendorstate_storeconfiguration present in the root module, provides that input to the(Meta).Backendmethod, and returns the operations backend from there. When pluggable state storage is in use the method takes care of obtaining locks and provider factories necessary for using PSS.Overall, this PR makes Terraform commands compatible with state stored via
state_store. The exceptions are commands liketerraform cloud *which expect a specific type of state storage to be present in the configuration when the user executes the command.Target Release
N/A
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry