Relocate localterraform.com aliasing to backend configurators

[brandonc]

Previously, remote and cloud backends would automatically alias localterraform.com as the configured hostname during configuration. This turned out to be an issue with how backends could potentially be used within the builtin terraform_remote_state data source. Those data sources each configure the same service discovery with different targets for localterraform.com, and do so simultaneously, creating an occasional concurrent map read & write panic when multiple data sources are defined.

localterraform.com is obviously not useful for every backend configuration. Therefore, I relocated the alias configuration to the callers, so they may specify when to use it. The modified design adds a new method to backend.Enhanced to allow configurators to ask which aliases should be defined.

Fixes #33333

Target Release

1.5.2

BUG FIXES

• Fixed a panic when reading from terraform_remote_state data sources when multiple resources are defined

[apparentlymart]

This seems like a reasonable approach to me.

I left an inline comment about slightly tweaking the separation of concerns between command.Meta and the backend so that the backend is responsible only for providing the information and not for acting on it, but I'm not sure if that's better enough to be worth additional complexity; if doing that makes things significantly more complicated then we can probably live with the slightly strange separation of concerns this current changeset implies.

[apparentlymart]

This does seem overall reasonable but I worry a little about this implicit assumption that b.services here is always the same object as the service discovery object used by the module and provider installers in terraform init. I imagine that's true today but it seems relatively easy for future changes to break that assumption if e.g. we end up needing to give different parts of Terraform different views of the discovery information for some reason yet to be determined.

While I'm not sure how likely that actually is, what bothers me is that someone could potentially do it without realizing that they are breaking a non-obvious assumption. For everything else about the service discovery system we treat it as something that's set once inside package main and then not mutated again, and so it would be reasonable for a new contributor to not notice this special exception to that.

I'm honestly not really sure what to suggest instead, though. 😖 One idea that came to my mind was to make this backend-level function just ask the question "what hostname should localterraform.com be an alias for?" and have it return targetHost instead of directly write it into its own b.services, and then it could be the caller's responsibility to actually register it with the service discovery object. I think that would also make this function infallible (no error cases) because the only possible error here is that the given hostname is invalid, and under this new design this method would not actually need to know the alias hostname at all.

That is admittedly only a marginal improvement since it still assumes that everyone's sharing a single disco object, but I imagine the caller that would be doing it would be command.Meta and that's already a sort of "central controller" for everything else and so I have a hunch that it's more likely to be able to ensure that everyone's svchost.Disco gets this information, even if in future it ends up having to register this against more than one discovery object for some reason.

[brandonc]

@apparentlymart I tried my best to minimize the services modification being done by the backend, but it did turn out to be a marginal improvement because command.Meta is still modifying the service disco it doesn't own. But at least it's not happening one level further down in the backend. There was just no nice way I could see getting this alias information to the main package since the backend initialization is brokered by backendInit.

[apparentlymart]

Thanks for working through the compromise we discussed in the thread above! Although it's not quite the ideal of having the disco object be immutable once constructed, I do think this handling in Meta is more likely to stay correct under future maintenance because we're accustomed to that object being the shared state for everything in package command, at least.

I left another little thing inline; I'm hoping it's a relatively easy change and if I'm wrong about that (if it would be very time-consuming to do) I don't think it's crucial.

[github-actions]

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

[github-actions]

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.