From 0b404f4a958103f8c3e70afa9c3d84fd7b55dbc6 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 3 Jun 2022 14:27:55 -0400 Subject: [PATCH] Return early on GetProviderSchema RPC responses with error diagnostics Reference: https://github.com/hashicorp/terraform/issues/31047 Prevent potential panics and immediately return provider-defined errors diagnostics. Previously: ``` --- FAIL: TestGRPCProvider_GetSchema_ResponseErrorDiagnostic (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x17fa752] goroutine 13 [running]: testing.tRunner.func1.2({0x191a100, 0x2236330}) /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1389 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1392 +0x39f panic({0x191a100, 0x2236330}) /usr/local/Cellar/go/1.18.2/libexec/src/runtime/panic.go:838 +0x207 github.com/hashicorp/terraform/internal/plugin6/convert.ProtoToConfigSchema(0x0) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin6/convert/schema.go:110 +0x52 github.com/hashicorp/terraform/internal/plugin6/convert.ProtoToProviderSchema(...) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin6/convert/schema.go:98 github.com/hashicorp/terraform/internal/plugin6.(*GRPCProvider).GetProviderSchema(0xc00004a200) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin6/grpc_provider.go:152 +0x29a github.com/hashicorp/terraform/internal/plugin6.TestGRPCProvider_GetSchema_ResponseErrorDiagnostic(0x0?) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin6/grpc_provider_test.go:158 +0x265 testing.tRunner(0xc0001031e0, 0x1a733d8) /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1439 +0x102 created by testing.(*T).Run /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1486 +0x35f ``` Previously: ``` --- FAIL: TestGRPCProvider_GetSchema_ResponseErrorDiagnostic (0.00s) panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x18a2732] goroutine 7 [running]: testing.tRunner.func1.2({0x1a5e720, 0x250be50}) /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1389 +0x24e testing.tRunner.func1() /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1392 +0x39f panic({0x1a5e720, 0x250be50}) /usr/local/Cellar/go/1.18.2/libexec/src/runtime/panic.go:838 +0x207 github.com/hashicorp/terraform/internal/plugin/convert.ProtoToConfigSchema(0x0) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin/convert/schema.go:104 +0x52 github.com/hashicorp/terraform/internal/plugin/convert.ProtoToProviderSchema(...) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin/convert/schema.go:92 github.com/hashicorp/terraform/internal/plugin.(*GRPCProvider).GetProviderSchema(0xc00004a600) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin/grpc_provider.go:149 +0x29a github.com/hashicorp/terraform/internal/plugin.TestGRPCProvider_GetSchema_ResponseErrorDiagnostic(0x0?) /Users/bflad/src/github.com/hashicorp/terraform/internal/plugin/grpc_provider_test.go:130 +0x265 testing.tRunner(0xc0001031e0, 0x1be9500) /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1439 +0x102 created by testing.(*T).Run /usr/local/Cellar/go/1.18.2/libexec/src/testing/testing.go:1486 +0x35f ``` --- internal/plugin/grpc_provider.go | 4 ++ internal/plugin/grpc_provider_test.go | 61 ++++++++++++++++++++++++++ internal/plugin6/grpc_provider.go | 4 ++ internal/plugin6/grpc_provider_test.go | 61 ++++++++++++++++++++++++++ 4 files changed, 130 insertions(+) diff --git a/internal/plugin/grpc_provider.go b/internal/plugin/grpc_provider.go index 9496607e1782..6f78c79b85ab 100644 --- a/internal/plugin/grpc_provider.go +++ b/internal/plugin/grpc_provider.go @@ -137,6 +137,10 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) + if resp.Diagnostics.HasErrors() { + return resp + } + if protoResp.Provider == nil { resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing provider schema")) return resp diff --git a/internal/plugin/grpc_provider_test.go b/internal/plugin/grpc_provider_test.go index fc6216cc0606..4e8d2f6c9207 100644 --- a/internal/plugin/grpc_provider_test.go +++ b/internal/plugin/grpc_provider_test.go @@ -39,6 +39,15 @@ func checkDiags(t *testing.T, d tfdiags.Diagnostics) { } } +// checkDiagsHasError ensures error diagnostics are present or fails the test. +func checkDiagsHasError(t *testing.T, d tfdiags.Diagnostics) { + t.Helper() + + if !d.HasErrors() { + t.Fatal("expected error diagnostics") + } +} + func providerProtoSchema() *proto.GetProviderSchema_Response { return &proto.GetProviderSchema_Response{ Provider: &proto.Schema{ @@ -92,6 +101,58 @@ func TestGRPCProvider_GetSchema(t *testing.T) { checkDiags(t, resp.Diagnostics) } +// Ensure that gRPC errors are returned early. +// Reference: https://github.com/hashicorp/terraform/issues/31047 +func TestGRPCProvider_GetSchema_GRPCError(t *testing.T) { + ctrl := gomock.NewController(t) + client := mockproto.NewMockProviderClient(ctrl) + + client.EXPECT().GetSchema( + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(&proto.GetProviderSchema_Response{}, fmt.Errorf("test error")) + + p := &GRPCProvider{ + client: client, + } + + resp := p.GetProviderSchema() + + checkDiagsHasError(t, resp.Diagnostics) +} + +// Ensure that provider error diagnostics are returned early. +// Reference: https://github.com/hashicorp/terraform/issues/31047 +func TestGRPCProvider_GetSchema_ResponseErrorDiagnostic(t *testing.T) { + ctrl := gomock.NewController(t) + client := mockproto.NewMockProviderClient(ctrl) + + client.EXPECT().GetSchema( + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(&proto.GetProviderSchema_Response{ + Diagnostics: []*proto.Diagnostic{ + { + Severity: proto.Diagnostic_ERROR, + Summary: "error summary", + Detail: "error detail", + }, + }, + // Trigger potential panics + Provider: &proto.Schema{}, + }, nil) + + p := &GRPCProvider{ + client: client, + } + + resp := p.GetProviderSchema() + + checkDiagsHasError(t, resp.Diagnostics) +} + func TestGRPCProvider_PrepareProviderConfig(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{ diff --git a/internal/plugin6/grpc_provider.go b/internal/plugin6/grpc_provider.go index 21c3a14e48ad..90533a5cff17 100644 --- a/internal/plugin6/grpc_provider.go +++ b/internal/plugin6/grpc_provider.go @@ -144,6 +144,10 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp resp.Diagnostics = resp.Diagnostics.Append(convert.ProtoToDiagnostics(protoResp.Diagnostics)) + if resp.Diagnostics.HasErrors() { + return resp + } + if protoResp.Provider == nil { resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing provider schema")) return resp diff --git a/internal/plugin6/grpc_provider_test.go b/internal/plugin6/grpc_provider_test.go index cca0b58208fa..300a09b4ade4 100644 --- a/internal/plugin6/grpc_provider_test.go +++ b/internal/plugin6/grpc_provider_test.go @@ -46,6 +46,15 @@ func checkDiags(t *testing.T, d tfdiags.Diagnostics) { } } +// checkDiagsHasError ensures error diagnostics are present or fails the test. +func checkDiagsHasError(t *testing.T, d tfdiags.Diagnostics) { + t.Helper() + + if !d.HasErrors() { + t.Fatal("expected error diagnostics") + } +} + func providerProtoSchema() *proto.GetProviderSchema_Response { return &proto.GetProviderSchema_Response{ Provider: &proto.Schema{ @@ -99,6 +108,58 @@ func TestGRPCProvider_GetSchema(t *testing.T) { checkDiags(t, resp.Diagnostics) } +// Ensure that gRPC errors are returned early. +// Reference: https://github.com/hashicorp/terraform/issues/31047 +func TestGRPCProvider_GetSchema_GRPCError(t *testing.T) { + ctrl := gomock.NewController(t) + client := mockproto.NewMockProviderClient(ctrl) + + client.EXPECT().GetProviderSchema( + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(&proto.GetProviderSchema_Response{}, fmt.Errorf("test error")) + + p := &GRPCProvider{ + client: client, + } + + resp := p.GetProviderSchema() + + checkDiagsHasError(t, resp.Diagnostics) +} + +// Ensure that provider error diagnostics are returned early. +// Reference: https://github.com/hashicorp/terraform/issues/31047 +func TestGRPCProvider_GetSchema_ResponseErrorDiagnostic(t *testing.T) { + ctrl := gomock.NewController(t) + client := mockproto.NewMockProviderClient(ctrl) + + client.EXPECT().GetProviderSchema( + gomock.Any(), + gomock.Any(), + gomock.Any(), + ).Return(&proto.GetProviderSchema_Response{ + Diagnostics: []*proto.Diagnostic{ + { + Severity: proto.Diagnostic_ERROR, + Summary: "error summary", + Detail: "error detail", + }, + }, + // Trigger potential panics + Provider: &proto.Schema{}, + }, nil) + + p := &GRPCProvider{ + client: client, + } + + resp := p.GetProviderSchema() + + checkDiagsHasError(t, resp.Diagnostics) +} + func TestGRPCProvider_PrepareProviderConfig(t *testing.T) { client := mockProviderClient(t) p := &GRPCProvider{