Skip to content

Commit 7bc4c6e

Browse files
bfladbflad
authored
helper/resource: Various small fixes (#1091)
Reference: #1063 Reference: hashicorp/go-plugin#220 This includes the following small bug fixes: - Clarifies/fixes the trace logging during `terraform show` commands - Improves performance slightly for `terraform show` raw plan usage (terraform-exec already captures and returns stdout for raw plans) - Prevents a goroutine leak specific to the detached stop context in `schema.GRPCProvider` Previously, leak detection on testing would contain an entry per Terraform command such as: ``` [Goroutine 276 in state select, with github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.mergeStop on top of the stack: goroutine 276 [select]: github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.mergeStop({0x103a5f298?, 0x14000390c00?}, 0x1400033e5f0, 0x140004bb560) /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider.go:46 +0x64 created by github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema.(*GRPCProviderServer).StopContext /Users/bflad/src/github.com/hashicorp/terraform-plugin-sdk/helper/schema/grpc_provider.go:63 +0x178 ] ``` Leaked goroutines such as: ``` [Goroutine 53 in state select, with github.com/hashicorp/go-plugin.(*gRPCBrokerServer).Recv on top of the stack: goroutine 53 [select]: github.com/hashicorp/go-plugin.(*gRPCBrokerServer).Recv(0x29?) /Users/bflad/go/pkg/mod/github.com/hashicorp/go-plugin@v1.4.4/grpc_broker.go:121 +0x58 github.com/hashicorp/go-plugin.(*GRPCBroker).Run(0x140003ca190) /Users/bflad/go/pkg/mod/github.com/hashicorp/go-plugin@v1.4.4/grpc_broker.go:411 +0x40 created by github.com/hashicorp/go-plugin.(*GRPCServer).Init /Users/bflad/go/pkg/mod/github.com/hashicorp/go-plugin@v1.4.4/grpc_server.go:85 +0x424 ] ``` Will likely require a fix upstream in go-plugin.
1 parent 6275669 commit 7bc4c6e

4 files changed

Lines changed: 94 additions & 16 deletions

File tree

‎.changelog/1091.txt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
```release-note:bug
2+
helper/resource: Prevented goroutine leak per Terraform command when testing terraform-plugin-sdk based providers via `Providers` or `ProviderFactories`
3+
```

‎helper/resource/plugin.go‎

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,13 @@ func runProviderCommand(ctx context.Context, t testing.T, f func() error, wd *pl
157157
host = v
158158
}
159159

160+
// schema.Provider have a global stop context that is created outside
161+
// the server context and have their own associated goroutine. Since
162+
// Terraform does not call the StopProvider RPC to stop the server in
163+
// reattach mode, ensure that we save these servers to later call that
164+
// RPC and end those goroutines.
165+
legacyProviderServers := make([]*schema.GRPCProviderServer, 0, len(factories.legacy))
166+
160167
// Spin up gRPC servers for every provider factory, start a
161168
// WaitGroup to listen for all of the close channels.
162169
var wg sync.WaitGroup
@@ -180,13 +187,19 @@ func runProviderCommand(ctx context.Context, t testing.T, f func() error, wd *pl
180187
// shut down.
181188
wg.Add(1)
182189

190+
grpcProviderServer := schema.NewGRPCProviderServer(provider)
191+
legacyProviderServers = append(legacyProviderServers, grpcProviderServer)
192+
193+
// Ensure StopProvider is always called when returning early.
194+
defer grpcProviderServer.StopProvider(ctx, nil) //nolint:errcheck // does not return errors
195+
183196
// configure the settings our plugin will be served with
184197
// the GRPCProviderFunc wraps a non-gRPC provider server
185198
// into a gRPC interface, and the logger just discards logs
186199
// from go-plugin.
187200
opts := &plugin.ServeOpts{
188201
GRPCProviderFunc: func() tfprotov5.ProviderServer {
189-
return schema.NewGRPCProviderServer(provider)
202+
return grpcProviderServer
190203
},
191204
Logger: hclog.New(&hclog.LoggerOptions{
192205
Name: "plugintest",
@@ -430,6 +443,12 @@ func runProviderCommand(ctx context.Context, t testing.T, f func() error, wd *pl
430443
// get closed, and we'll hang here.
431444
cancel()
432445

446+
// For legacy providers, call the StopProvider RPC so the StopContext
447+
// goroutine is cleaned up properly.
448+
for _, legacyProviderServer := range legacyProviderServers {
449+
legacyProviderServer.StopProvider(ctx, nil) //nolint:errcheck // does not return errors
450+
}
451+
433452
logging.HelperResourceTrace(ctx, "Waiting for providers to stop")
434453

435454
// wait for the servers to actually shut down; it may take a moment for

‎helper/resource/plugin_test.go‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,17 @@
11
package resource
22

33
import (
4+
"context"
45
"fmt"
6+
"os"
57
"testing"
68

79
"github.com/google/go-cmp/cmp"
810
"github.com/hashicorp/terraform-plugin-go/tfprotov5"
911
"github.com/hashicorp/terraform-plugin-go/tfprotov6"
12+
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
1013
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
14+
"github.com/hashicorp/terraform-plugin-sdk/v2/internal/plugintest"
1115
)
1216

1317
func TestProtoV5ProviderFactoriesMerge(t *testing.T) {
@@ -234,3 +238,62 @@ func TestSdkProviderFactoriesMerge(t *testing.T) {
234238
})
235239
}
236240
}
241+
242+
func TestRunProviderCommand(t *testing.T) {
243+
currentDir, err := os.Getwd()
244+
245+
if err != nil {
246+
t.Fatalf("unable to get working directory: %s", err)
247+
}
248+
249+
ctx := context.Background()
250+
funcCalled := false
251+
helper := plugintest.AutoInitProviderHelper(ctx, currentDir)
252+
253+
err = runProviderCommand(
254+
ctx,
255+
t,
256+
func() error {
257+
funcCalled = true
258+
return nil
259+
},
260+
helper.RequireNewWorkingDir(ctx, t),
261+
&providerFactories{
262+
legacy: map[string]func() (*schema.Provider, error){
263+
"examplecloud": func() (*schema.Provider, error) { //nolint:unparam // required signature
264+
return &schema.Provider{
265+
ResourcesMap: map[string]*schema.Resource{
266+
"examplecloud_thing": {
267+
CreateContext: func(ctx context.Context, d *schema.ResourceData, i interface{}) diag.Diagnostics {
268+
d.SetId("id")
269+
270+
return nil
271+
},
272+
DeleteContext: func(_ context.Context, _ *schema.ResourceData, _ interface{}) diag.Diagnostics {
273+
return nil
274+
},
275+
ReadContext: func(_ context.Context, d *schema.ResourceData, _ interface{}) diag.Diagnostics {
276+
return nil
277+
},
278+
Schema: map[string]*schema.Schema{
279+
"id": {
280+
Computed: true,
281+
Type: schema.TypeString,
282+
},
283+
},
284+
},
285+
},
286+
}, nil
287+
},
288+
},
289+
},
290+
)
291+
292+
if err != nil {
293+
t.Fatal(err)
294+
}
295+
296+
if !funcCalled {
297+
t.Error("expected func to be called")
298+
}
299+
}

‎internal/plugintest/working_dir.go‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
package plugintest
22

33
import (
4-
"bytes"
54
"context"
65
"encoding/json"
76
"fmt"
8-
"io"
97
"os"
108
"path/filepath"
119

@@ -281,11 +279,11 @@ func (wd *WorkingDir) SavedPlan(ctx context.Context) (*tfjson.Plan, error) {
281279
return nil, fmt.Errorf("there is no current saved plan")
282280
}
283281

284-
logging.HelperResourceTrace(ctx, "Calling Terraform CLI apply command")
282+
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command for JSON plan")
285283

286284
plan, err := wd.tf.ShowPlanFile(context.Background(), wd.planFilename(), tfexec.Reattach(wd.reattachInfo))
287285

288-
logging.HelperResourceTrace(ctx, "Calling Terraform CLI apply command")
286+
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command for JSON plan")
289287

290288
return plan, err
291289
}
@@ -299,34 +297,29 @@ func (wd *WorkingDir) SavedPlanRawStdout(ctx context.Context) (string, error) {
299297
return "", fmt.Errorf("there is no current saved plan")
300298
}
301299

302-
var ret bytes.Buffer
303-
304-
wd.tf.SetStdout(&ret)
305-
defer wd.tf.SetStdout(io.Discard)
306-
307-
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command")
300+
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command for stdout plan")
308301

309-
_, err := wd.tf.ShowPlanFileRaw(context.Background(), wd.planFilename(), tfexec.Reattach(wd.reattachInfo))
302+
stdout, err := wd.tf.ShowPlanFileRaw(context.Background(), wd.planFilename(), tfexec.Reattach(wd.reattachInfo))
310303

311-
logging.HelperResourceTrace(ctx, "Called Terraform CLI show command")
304+
logging.HelperResourceTrace(ctx, "Called Terraform CLI show command for stdout plan")
312305

313306
if err != nil {
314307
return "", err
315308
}
316309

317-
return ret.String(), nil
310+
return stdout, nil
318311
}
319312

320313
// State returns an object describing the current state.
321314
//
322315

323316
// If the state cannot be read, State returns an error.
324317
func (wd *WorkingDir) State(ctx context.Context) (*tfjson.State, error) {
325-
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command")
318+
logging.HelperResourceTrace(ctx, "Calling Terraform CLI show command for JSON state")
326319

327320
state, err := wd.tf.Show(context.Background(), tfexec.Reattach(wd.reattachInfo))
328321

329-
logging.HelperResourceTrace(ctx, "Called Terraform CLI show command")
322+
logging.HelperResourceTrace(ctx, "Called Terraform CLI show command for JSON state")
330323

331324
return state, err
332325
}

0 commit comments

Comments
 (0)