plugin: Serve via tf5server/tf6server rather than go-plugin directly

[bflad]

Closes #650

Providers will automatically receive upstream enhancements to provider servers, including protocol logging and the increased gRPC message limit. Lower level dependencies in this SDK, such as gRPC and go-plugin versions, can instead be managed upstream, which will ensure consistency across the SDKs.

[bflad]

Will rebase now due to [email protected] merging in.

[detro]

I can't pretend to understand fully this, but I'm giving it a go.

So, here you are making the call to Serve use tf5server and tf6server, instead of creating a new grcp.Server. I have to assume this was a "transitional state", where a GRPC Server was added to TF5 and TF6 but not used yet?

Anyway, I left some comments on the logging, thinking that it might useful if the "unrecoverable errors" that you are logging can be .Fatal or .Panic.

Given this is not just a style thing like the PR from yesterday, I think I need to hold the Approval for now: I'm not confident enough in my understanding :)

[detro]

Question: I assume that changing the interface of the plugin to return an error via Serve would be madness (right?). But I do see that you are checking for a mutually exclusive "either Debug or TestConfig".

Would it make sense for the log. call to be log.Fatal or log.Panic? I'd vote for Fatal as it also calls exit(1), and that is what I'd presume we want (the provider process exiting with a POSIX error).

Am I making any sense?

[bflad]

Given Serve is exported and effectively in use by every provider developed on top of this SDK (or any wrapping logic developed externally), we need to be very careful about breaking changes, both in Go terms and potential usage.

Since there are no returns now, any calling code wouldn't necessarily be checking for the error return upon upgrading. Calling codebases would need to be using something like the errcheck linter to catch the issue, which we cannot assume, and introducing the new errcheck issue (for those using it) is probably "breaking" enough. Any external Go types that alias the function would also receive the breaking change.

We also cannot assume how callers were interacting with this function. For example, there's no requirement it is only called by main function in provider binaries where it would generally be appropriate to exit(1). There are potential use cases where providers are served as part of other logic and the unilateral decision to exit the Go runtime would be very inappropriate for us to introduce.

Whether we should separately introduce a "Serve" function that has an error return to be more correct in this situation is likely debatable -- no one has asked for it yet and errors (if they didn't already panic or exit themselves as part of the called logic) were previously swallowed. I agree in principle that its probably something that should exist, but I'd love to hear the demand and use cases before extending the package surface area, especially since going forward we are encouraging folks to use the new framework, rather than this one.

[detro]

There are potential use cases where providers are served as part of other logic and the unilateral decision to exit the Go runtime would be very inappropriate for us to introduce.
Absolutely!

[detro]

Same comment about the .Fatal here. If this fails, definitely the provider isn't working (and will exit(0)).

[bflad]

I have to assume this was a "transitional state", where a GRPC Server was added to TF5 and TF6 but not used yet?

It indeed is a transitional state, but maybe not in the way you're alluding. 😄 The brief historical context here:

• Plugin/Provider netRPC servers have existed in the merged Terraform CLI+SDK codebase since the very early Terraform days (0.1 or 0.2?)
• netRPC was eventually superseded by gRPC
• The terraform-plugin-sdk Go module was split from the Terraform CLI codebase
• There was a decision to reimagine the provider development experience because this SDK was built around Terraform 0.11 and earlier type system amongst other things, which wound up with terraform-plugin-go (low level gRPC handling and Go bindings), terraform-plugin-mux (to combine providers), and eventually terraform-plugin-framework (a new higher level SDK) -- you can see tf6server in use in terraform-plugin-framework
• As part of updating the helper/resource package (the "acceptance testing framework" of this SDK) to support provider testing outside of the types defined by this SDK, the plugin package was migrated over to using the terraform-plugin-go tfprotov5 and tfprotov6 types

However, the server handling hasn't been migrated yet and here we are! This will standardize tf5server and tf6server being the lowest level handlers in both the higher level SDKs, so terraform-plugin-go can consistently manage gRPC/go-plugin dependencies, manage gRPC settings, and have a consistent logger setup (such as generated RPC request IDs, protocol level logging, etc).

[detro]

Thanks for the clarification.
It makes perfect sense: the amount of ways that a change like that can go wrong is just too much.

LGTM

[bflad]

Since there's #650, I'm going to add an enhancement changelog entry for this to match the upstream one: https://github.com/hashicorp/terraform-plugin-go/pull/139/files

[bflad]

We should hold off merging or releasing this until hashicorp/terraform-plugin-go#153 is reviewed, merged, released, and the dependency updated. Refer to hashicorp/terraform-plugin-go#152 for additional context.

[bflad]

#884 was merged with the necessary tf5server ServeOpt fixes, rebased this branch and re-verified expected acceptance testing output via terraform-provider-corner, merging!

[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.