feat(stacks): add CLI config in RPC API handshake

[mjyocca]

This PR adds support for including fields typically found in the Terraform CLI configuration in the RPC API handshake. This allows us to include global configuration arguments that impact the RPC API session without requiring instrumentation for each RPC service. The new Config struct currently supports only service discovery credentials, but it can be expanded in the future.

[apparentlymart]

Calling this "CLI Configuration" in the rpcapi context (where we've presented this as a way to "bypass" Terraform CLI and get to Terraform Core directly) seems a little idiosyncratic, but I must admit I'm not sure what else I'd call the general container for "CLI-configuration-like things".

The only non-weird alternative that comes to my mind is to just put this new credentials field directly in ClientCapabilities, without an additional level of nesting, and just assume that any future things we add in this vein will also be direct fields in ClientCapabilities. That seems okay to me, but I don't feel strongly about it.

I also don't really feel strongly about the overall oddity in the first place. I mention it just in case you want to consider it. I've no objection to accepting the idiosyncratic name if that feels like the best of the available options. 🤷‍♂️

[mjyocca]

Calling this "CLI Configuration" in the rpcapi context (where we've presented this as a way to "bypass" Terraform CLI and get to Terraform Core directly) seems a little idiosyncratic, but I must admit I'm not sure what else I'd call the general container for "CLI-configuration-like things".

The only non-weird alternative that comes to my mind is to just put this new credentials field directly in ClientCapabilities, without an additional level of nesting, and just assume that any future things we add in this vein will also be direct fields in ClientCapabilities. That seems okay to me, but I don't feel strongly about it.

I also don't really feel strongly about the overall oddity in the first place. I mention it just in case you want to consider it. I've no objection to accepting the idiosyncratic name if that feels like the best of the available options. 🤷‍♂️

I'm okay with either deviating away from "CLI" naming or hoisting those fields to ClientCapabilities.

Some ideas that come to mind: rename the message struct to a generic Config or something along the lines of ClientConfig.

[apparentlymart]

Looking again at the full protobuf schema (rather than just the parts visible in the diff! 😅) I see one more possibility:

We could add credentials as another field in Handshake.Request, sibling to the ClientCapabilities object. The credentials aren't really a "client capability" anyway, after all, and we can freely add new optional arguments to Handshake.Request in future without breaking backward compatibility. (That's the main reason for using these extra Request/Response message types on all of the RPCs; we know from experience with earlier versions of the provider plugin protocol that this is the most effective way to allow for unplanned evolution of the protocol in future.)

Of all of these options I think the one I've described in this comment seems to get us what we need in as simple a way as possible and so I favor this one, but again I'm not wedded to it.

[mjyocca]

Looking again at the full protobuf schema (rather than just the parts visible in the diff! 😅) I see one more possibility:

We could add credentials as another field in Handshake.Request, sibling to the ClientCapabilities object. The credentials aren't really a "client capability" anyway, after all, and we can freely add new optional arguments to Handshake.Request in future without breaking backward compatibility. (That's the main reason for using these extra Request/Response message types on all of the RPCs; we know from experience with earlier versions of the provider plugin protocol that this is the most effective way to allow for unplanned evolution of the protocol in future.)

Of all of these options I think the one I've described in this comment seems to get us what we need in as simple a way as possible and so I favor this one, but again I'm not wedded to it.

Yeah adding this new field to the HandShake Request makes a lot of sense to me. I previously headed down that direction until I came across this comment which made me reconsider using "Capabilities" instead.

terraform/internal/rpcapi/plugin.go

Lines 59 to 68 in 7012371

	// NOTE: This is intentionally not the same disco that "package main"
	// instantiates for Terraform CLI, because the RPC API is
	// architecturally independent from CLI despite being launched through
	// it, and so it is not subject to any ambient CLI configuration files
	// that might be in scope. If we later discover requirements for
	// callers to customize the service discovery settings, consider
	// adding new fields to terraform1.ClientCapabilities (even though
	// this isn't strictly a "capability") so that the RPC caller has
	// full control without needing to also tinker with the current user's
	// CLI configuration.

Update: Nevertheless, I'm on board with either solution that would better position the protocol for future revisions 😄.

[apparentlymart]

I expect I probably wrote that comment, and if so I imagine I wrote it with the same partial mental context I had when I was initially commenting on this PR, before I read the full schema and remembered that we also have the Handshake.Request message type. 🙃

Sorry for inadvertently leading you down a strange path!

I note that this comment is in a function that only has access to the ClientCapabilities and not to the request it was wrapped in -- which is probably why I forgot that there's a wrapping message type -- but I don't see any big problem with changing that function to take the whole Handshake.Request message instead, so that it can access all of the other properties of the handshake request.

[mjyocca]

I expect I probably wrote that comment, and if so I imagine I wrote it with the same partial mental context I had when I was initially commenting on this PR, before I read the full schema and remembered that we also have the Handshake.Request message type. 🙃

Sorry for inadvertently leading you down a strange path!

I note that this comment is in a function that only has access to the ClientCapabilities and not to the request it was wrapped in -- which is probably why I forgot that there's a wrapping message type -- but I don't see any big problem with changing that function to take the whole Handshake.Request message instead, so that it can access all of the other properties of the handshake request.

All paths lead to the desired destination given enough time (Cheesy I know) 😄. But made the revisions as discussed 🎊.

[apparentlymart]

This looks good to me! Thanks for the patience while we moved all the little bits around. 😀

I have one little note inline. If you agree with me and change it, please just go ahead and merge the updated version without another round of review, since it's a very minor thing and so not worth another round-trip.

[mjyocca]

This looks good to me! Thanks for the patience while we moved all the little bits around. 😀

I have one little note inline. If you agree with me and change it, please just go ahead and merge the updated version without another round of review, since it's a very minor thing and so not worth another round-trip.

Sounds good and no worries at all, been a fun iteration cycle 😄.

[github-actions]

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