Skip to content
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

Add support for plugin protocol v6 #27826

Merged
merged 2 commits into from
Feb 22, 2021
Merged

Add support for plugin protocol v6 #27826

merged 2 commits into from
Feb 22, 2021

Conversation

mildwonkey
Copy link
Contributor

@mildwonkey mildwonkey commented Feb 18, 2021

This PR turns on support for plugin protocol v6. A provider can
advertise itself as supporting protocol version 6 and terraform will
use the correct client.

I apologize for the noisy name changes in plugin6 which should have been caught in the previous PR; I'd inadvertently left out a test file with a load-bearing interface assertion.

A few notable TODOs which are outside of the scope of this PR:

  • The "unmanaged" providers functionality does not support protocol
    version, so at the moment terraform will continue to assume that
    "unmanaged" providers are on protocol v5. This will require some
    upstream work on go-plugin (I believe).

  • I would like to convert the builtin providers to use protocol v6 in a
    future PR; however it is not necessary until we remove protocol v6.

  • T E S T I N G!
    It's easy to prove that this is working for providers using protocol v5 (the e2e and other tests would blow up if I'd gotten that wrong!) but my next step will likely involve adding an internal test provider that uses protocol v6 and writing tests that use both in one config.

This PR turns on support for plugin protocol v6. A provider can
advertise itself as supporting protocol version 6 and terraform will
use the correct client.

Todo:

The "unmanaged" providers functionality does not support protocol
version, so at the moment terraform will continue to assume that
"unmanaged" providers are on protocol v5. This will require some
upstream work on go-plugin (I believe).

I would like to convert the builtin providers to use protocol v6 in a
future PR; however it is not necessary until we remove protocol v6.
@codecov
Copy link

codecov bot commented Feb 18, 2021

Codecov Report

Merging #27826 (d403e44) into master (f650587) will decrease coverage by 0.06%.
The diff coverage is 18.75%.

Impacted Files Coverage Δ
command/meta_providers.go 30.26% <0.00%> (-1.91%) ⬇️
plugin6/serve.go 0.00% <0.00%> (ø)
plugin6/grpc_provider.go 54.01% <100.00%> (ø)
terraform/node_resource_plan.go 96.11% <0.00%> (-1.95%) ⬇️
plugin6/grpc_error.go 0.00% <0.00%> (ø)
dag/marshal.go 63.49% <0.00%> (+1.58%) ⬆️
internal/providercache/dir.go 73.46% <0.00%> (+6.12%) ⬆️

@@ -57,7 +52,7 @@ func pluginSet(opts *ServeOpts) map[int]plugin.PluginSet {

// add the new protocol versions if they're configured
if opts.GRPCProviderFunc != nil {
plugins[5] = plugin.PluginSet{}
plugins[6] = plugin.PluginSet{}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈 nothing to see here this was correct all along

@mildwonkey mildwonkey requested review from a team February 18, 2021 21:18
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but you may want an additional 👍🏻 from the SDK team as it would impact them more than anyone else.

@radeksimko radeksimko requested a review from a team February 19, 2021 08:17
- copied grpcwrap and made a version that returns protocol v6 provider
- copied the test provider, provider-simple, and made a version that's
  using protocol v6 with the above fun
- added an e2etest
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems right to me! I have more investigation I want to do with @mildwonkey about protocol version selection, but that almost certainly exists outside the scope of this PR.

@ghost
Copy link

ghost commented Mar 25, 2021

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.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants