Avoid configuring backend when validating terraform_remote_state

[bendrucker]

This PR ensures that the Configure method for a backend object powering a terraform_remote_state data source is not called during terraform validate. This ensures that only the supplied configuration will be validated but the state itself will not be loaded.

I followed @apparentlymart's ever-helpful advice and slightly refactored getBackend to accomplish this. Rather than return a configured backend object, getBackend now returns an un-configured object and the validated configuration (plus diags).

dataSourceRemoteStateValidate continues to ignore the backend, and now the config too, and just cares about diags. dataSourceRemoteStateRead can combine those two values into a b.Configure(cfg) call to actually configure the backend and invoke any remote API calls that may entail.

To test this, I built a binary and tested against a module that could not be validated without credentials that has an S3 remote state dependency:

$ terraform validate

Error: The role "arn:aws:iam::123456789:role/terraform" cannot be assumed.

  There are a number of possible causes of this - the most common are:
    * The credentials used in order to assume the role are invalid
    * The credentials do not have appropriate permission to assume the role
    * The role ARN is not valid

And after:

$ $GOBIN/terraform validate
Success! The configuration is valid.

Closes #21761, Closes #15811

[codecov]

Codecov Report

Merging #24887 into master will increase coverage by 0.02%.
The diff coverage is 70.58%.

Impacted Files	Coverage Δ
builtin/providers/terraform/data_source_state.go	85.61% <70.58%> (+9.39%)	⬆️
terraform/node_resource_plan.go	91.59% <0.00%> (-1.69%)	⬇️
helper/resource/state.go	85.47% <0.00%> (-0.86%)	⬇️
terraform/evaluate.go	53.60% <0.00%> (+0.45%)	⬆️
dag/marshal.go	53.40% <0.00%> (+1.13%)	⬆️

[apparentlymart]

Thanks for working on this, @bendrucker!

I ended up writing a little test case to help me review it and I figured afterwards I might as well clean it up and push it, so I added an extra commit here that just contains that extra test. I'm going to merge your commits along with my extra one now.

Thanks again!

[bendrucker]

Thank you for the test! Was imagining some kind of poison Backend implementation to actually cover the change (as opposed to just adding some coverage for CodeCov 😉) but didn't have a full sense of how to implement that and forgot to pursue it. The overrides factory pattern is clever!

[wyardley]

Thanks! Will be looking forward to testing this after the next release!

[aerickson]

Can we postmortem why this was solved now versus when #22163 was filed?

I'd like to be able to help like @bendrucker was able to... and kind of tried in the earlier bug, but we never got any attention. @bendrucker did you reach out to the team on IRC or what was your secret?

[bendrucker]

No secret, per se! I do have a personal interest in seeing this fixed so I was motivated to work on it.

I saw that Martin had left some hints on another issue where this was reported:

#15811 (comment)

Martin is a superhuman and leaves this kind of explanation of internals on a lot of issues so I was eager to dig in. Took less than an hour to write/test, and Martin graciously reviewed very quickly and added test coverage.

[danieldreier]

@aerickson I'm the engineering manager for the team, and so I feel like I should chime in and answer. I took a look at the #22163 discussion, and I see a lot of "I'm seeing this too" reports, and you contributed a good reproduction case, but I don't think anyone made PRs or volunteered to fix it. @bendrucker came along with a good PR just as Martin had time to review it, so things worked out great.

There's no secret IRC but feel free to email me directly at [email protected] if there's an issue you want to fix and you're running into any barriers to contributing.

[aerickson]

I didn't know this was an issue since #15811. Could the docs mention a bug number if a feature is significantly flawed (#15811 (comment) is pretty blunt)?

It would have saved the people on #22163 significant frustration and effort. The comment that lead to the fix (in addition to a temporary workaround) was on that original bug.

[wyardley]

Do we know when this will land? Looks like it wasn't in 0.12.25, right?

[parabolic]

Ditto that, 0.12.26 is released and still no trace from this PR.
Any hope we can see it in 0.12.27?
Thanks!

[ghost]

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.