-
Notifications
You must be signed in to change notification settings - Fork 233
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
plugin: Serve via tf5server/tf6server rather than go-plugin directly #857
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
```release-note:note | ||
plugin: The `Debug` function has been deprecated in preference of setting the `Debug` field in the `ServeOpts` passed into the `Serve` function. | ||
``` | ||
|
||
```release-note:enhancement | ||
plugin: Increased maximum gRPC send and receive message size limit to 256MB | ||
``` | ||
|
||
```release-note:enhancement | ||
plugin: Added support for writing protocol data to disk by setting `TF_LOG_SDK_PROTO_DATA_DIR` environment variable | ||
``` |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,12 @@ | ||
package plugin | ||
|
||
import ( | ||
"errors" | ||
"log" | ||
|
||
hclog "github.com/hashicorp/go-hclog" | ||
"github.com/hashicorp/go-plugin" | ||
testing "github.com/mitchellh/go-testing-interface" | ||
"google.golang.org/grpc" | ||
|
||
"github.com/hashicorp/terraform-plugin-go/tfprotov5" | ||
"github.com/hashicorp/terraform-plugin-go/tfprotov5/tf5server" | ||
|
@@ -18,10 +18,16 @@ import ( | |
const ( | ||
// The constants below are the names of the plugins that can be dispensed | ||
// from the plugin server. | ||
// | ||
// Deprecated: This is no longer used, but left for backwards compatibility | ||
// since it is exported. It will be removed in the next major version. | ||
ProviderPluginName = "provider" | ||
) | ||
|
||
// Handshake is the HandshakeConfig used to configure clients and servers. | ||
// | ||
// Deprecated: This is no longer used, but left for backwards compatibility | ||
// since it is exported. It will be removed in the next major version. | ||
var Handshake = plugin.HandshakeConfig{ | ||
// The magic cookie values should NEVER be changed. | ||
MagicCookieKey: "TF_PLUGIN_MAGIC_COOKIE", | ||
|
@@ -45,11 +51,20 @@ type ServeOpts struct { | |
// Logger is the logger that go-plugin will use. | ||
Logger hclog.Logger | ||
|
||
// Debug starts a debug server and controls its lifecycle, printing the | ||
// information needed for Terraform to connect to the provider to stdout. | ||
// os.Interrupt will be captured and used to stop the server. | ||
// | ||
// This option cannot be combined with TestConfig. | ||
Debug bool | ||
|
||
// TestConfig should only be set when the provider is being tested; it | ||
// will opt out of go-plugin's lifecycle management and other features, | ||
// and will use the supplied configuration options to control the | ||
// plugin's lifecycle and communicate connection information. See the | ||
// go-plugin GoDoc for more information. | ||
// | ||
// This option cannot be combined with Debug. | ||
TestConfig *plugin.ServeTestConfig | ||
|
||
// Set NoLogOutputOverride to not override the log output with an hclog | ||
|
@@ -69,6 +84,11 @@ type ServeOpts struct { | |
// Serve serves a plugin. This function never returns and should be the final | ||
// function called in the main function of the plugin. | ||
func Serve(opts *ServeOpts) { | ||
if opts.Debug && opts.TestConfig != nil { | ||
log.Printf("[ERROR] Error starting provider: cannot set both Debug and TestConfig") | ||
return | ||
} | ||
|
||
if !opts.NoLogOutputOverride { | ||
// In order to allow go-plugin to correctly pass log-levels through to | ||
// terraform, we need to use an hclog.Logger with JSON output. We can | ||
|
@@ -84,65 +104,126 @@ func Serve(opts *ServeOpts) { | |
log.SetOutput(logger.StandardWriter(&hclog.StandardLoggerOptions{InferLevels: true})) | ||
} | ||
|
||
// since the plugins may not yet be aware of the new protocol, we | ||
// automatically wrap the plugins in the grpc shims. | ||
if opts.GRPCProviderFunc == nil && opts.ProviderFunc != nil { | ||
if opts.ProviderAddr == "" { | ||
opts.ProviderAddr = "provider" | ||
} | ||
|
||
var err error | ||
|
||
switch { | ||
case opts.ProviderFunc != nil && opts.GRPCProviderFunc == nil: | ||
opts.GRPCProviderFunc = func() tfprotov5.ProviderServer { | ||
return schema.NewGRPCProviderServer(opts.ProviderFunc()) | ||
} | ||
err = tf5serverServe(opts) | ||
case opts.GRPCProviderFunc != nil: | ||
err = tf5serverServe(opts) | ||
case opts.GRPCProviderV6Func != nil: | ||
err = tf6serverServe(opts) | ||
default: | ||
err = errors.New("no provider server defined in ServeOpts") | ||
} | ||
|
||
serveConfig := plugin.ServeConfig{ | ||
HandshakeConfig: Handshake, | ||
GRPCServer: func(opts []grpc.ServerOption) *grpc.Server { | ||
return grpc.NewServer(opts...) | ||
}, | ||
Logger: opts.Logger, | ||
Test: opts.TestConfig, | ||
if err != nil { | ||
log.Printf("[ERROR] Error starting provider: %s", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment about the |
||
} | ||
} | ||
|
||
// assume we have either a v5 or a v6 provider | ||
if opts.GRPCProviderFunc != nil { | ||
provider := opts.GRPCProviderFunc() | ||
addr := opts.ProviderAddr | ||
if addr == "" { | ||
addr = "provider" | ||
} | ||
serveConfig.VersionedPlugins = map[int]plugin.PluginSet{ | ||
5: { | ||
ProviderPluginName: &tf5server.GRPCProviderPlugin{ | ||
GRPCProvider: func() tfprotov5.ProviderServer { | ||
return provider | ||
}, | ||
Name: addr, | ||
}, | ||
}, | ||
} | ||
if opts.UseTFLogSink != nil { | ||
serveConfig.VersionedPlugins[5][ProviderPluginName].(*tf5server.GRPCProviderPlugin).Opts = append(serveConfig.VersionedPlugins[5][ProviderPluginName].(*tf5server.GRPCProviderPlugin).Opts, tf5server.WithLoggingSink(opts.UseTFLogSink)) | ||
} | ||
func tf5serverServe(opts *ServeOpts) error { | ||
var tf5serveOpts []tf5server.ServeOpt | ||
|
||
} else if opts.GRPCProviderV6Func != nil { | ||
provider := opts.GRPCProviderV6Func() | ||
addr := opts.ProviderAddr | ||
if addr == "" { | ||
addr = "provider" | ||
} | ||
serveConfig.VersionedPlugins = map[int]plugin.PluginSet{ | ||
6: { | ||
ProviderPluginName: &tf6server.GRPCProviderPlugin{ | ||
GRPCProvider: func() tfprotov6.ProviderServer { | ||
return provider | ||
}, | ||
Name: addr, | ||
}, | ||
}, | ||
} | ||
if opts.UseTFLogSink != nil { | ||
serveConfig.VersionedPlugins[6][ProviderPluginName].(*tf6server.GRPCProviderPlugin).Opts = append(serveConfig.VersionedPlugins[6][ProviderPluginName].(*tf6server.GRPCProviderPlugin).Opts, tf6server.WithLoggingSink(opts.UseTFLogSink)) | ||
} | ||
if opts.Debug { | ||
tf5serveOpts = append(tf5serveOpts, tf5server.WithManagedDebug()) | ||
} | ||
|
||
if opts.Logger != nil { | ||
tf5serveOpts = append(tf5serveOpts, tf5server.WithGoPluginLogger(opts.Logger)) | ||
} | ||
|
||
if opts.TestConfig != nil { | ||
// Convert send-only channels to bi-directional channels to appease | ||
// the compiler. WithDebug is errantly defined to require | ||
// bi-directional when send-only is actually needed, which may be | ||
// fixed in the future so the opts.TestConfig channels can be passed | ||
// through directly. | ||
closeCh := make(chan struct{}) | ||
reattachConfigCh := make(chan *plugin.ReattachConfig) | ||
|
||
go func() { | ||
// Always forward close channel receive, since its signaling that | ||
// the channel is closed. | ||
val := <-closeCh | ||
opts.TestConfig.CloseCh <- val | ||
}() | ||
|
||
go func() { | ||
val, ok := <-reattachConfigCh | ||
|
||
if ok { | ||
opts.TestConfig.ReattachConfigCh <- val | ||
} | ||
}() | ||
|
||
tf5serveOpts = append(tf5serveOpts, tf5server.WithDebug( | ||
opts.TestConfig.Context, | ||
reattachConfigCh, | ||
closeCh), | ||
) | ||
} | ||
|
||
if opts.UseTFLogSink != nil { | ||
tf5serveOpts = append(tf5serveOpts, tf5server.WithLoggingSink(opts.UseTFLogSink)) | ||
} | ||
|
||
return tf5server.Serve(opts.ProviderAddr, opts.GRPCProviderFunc, tf5serveOpts...) | ||
} | ||
|
||
func tf6serverServe(opts *ServeOpts) error { | ||
var tf6serveOpts []tf6server.ServeOpt | ||
|
||
if opts.Debug { | ||
tf6serveOpts = append(tf6serveOpts, tf6server.WithManagedDebug()) | ||
} | ||
|
||
if opts.Logger != nil { | ||
tf6serveOpts = append(tf6serveOpts, tf6server.WithGoPluginLogger(opts.Logger)) | ||
} | ||
|
||
if opts.TestConfig != nil { | ||
// Convert send-only channels to bi-directional channels to appease | ||
// the compiler. WithDebug is errantly defined to require | ||
// bi-directional when send-only is actually needed, which may be | ||
// fixed in the future so the opts.TestConfig channels can be passed | ||
// through directly. | ||
closeCh := make(chan struct{}) | ||
reattachConfigCh := make(chan *plugin.ReattachConfig) | ||
|
||
go func() { | ||
val, ok := <-closeCh | ||
|
||
if ok { | ||
opts.TestConfig.CloseCh <- val | ||
} | ||
}() | ||
|
||
go func() { | ||
val, ok := <-reattachConfigCh | ||
|
||
if ok { | ||
opts.TestConfig.ReattachConfigCh <- val | ||
} | ||
}() | ||
|
||
tf6serveOpts = append(tf6serveOpts, tf6server.WithDebug( | ||
opts.TestConfig.Context, | ||
reattachConfigCh, | ||
closeCh), | ||
) | ||
} | ||
|
||
if opts.UseTFLogSink != nil { | ||
tf6serveOpts = append(tf6serveOpts, tf6server.WithLoggingSink(opts.UseTFLogSink)) | ||
} | ||
|
||
plugin.Serve(&serveConfig) | ||
return tf6server.Serve(opts.ProviderAddr, opts.GRPCProviderV6Func, tf6serveOpts...) | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I assume that changing the interface of the
plugin
to return an error viaServe
would be madness (right?). But I do see that you are checking for a mutually exclusive "eitherDebug
orTestConfig
".Would it make sense for the
log.
call to belog.Fatal
orlog.Panic
? I'd vote forFatal
as it also callsexit(1)
, and that is what I'd presume we want (the provider process exiting with a POSIX error).Am I making any sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 newerrcheck
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.