From 503c413de290c440ba71fcadb9b8ea6d3c563f90 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 2 Apr 2021 16:49:25 -0400 Subject: [PATCH 1/5] add addresses to diagnostics Add an address argument to tfdiags.InConfigBody, and store the address string the diagnostics details. Since nearly every place where we want to annotate the diagnostics with the config context we also have some sort of address, we can use the same call to insert them both into the diagnostic. Perhaps we should rename InConfigBody and ElaborateFromConfigBody to reflect the additional address parameter, but for now we can verify this is a pattern that suits us. --- tfdiags/contextual.go | 27 ++++++++++++++++++++------- tfdiags/contextual_test.go | 16 +++++++++++++++- tfdiags/diagnostic.go | 1 + tfdiags/diagnostic_base.go | 2 ++ 4 files changed, 38 insertions(+), 8 deletions(-) diff --git a/tfdiags/contextual.go b/tfdiags/contextual.go index f66e5d913..c59280f34 100644 --- a/tfdiags/contextual.go +++ b/tfdiags/contextual.go @@ -19,17 +19,20 @@ import ( // contextualFromConfig is an interface type implemented by diagnostic types // that can elaborate themselves when given information about the configuration -// body they are embedded in. +// body they are embedded in, as well as the runtime address associated with +// that configuration. // // Usually this entails extracting source location information in order to // populate the "Subject" range. type contextualFromConfigBody interface { - ElaborateFromConfigBody(hcl.Body) Diagnostic + ElaborateFromConfigBody(hcl.Body, string) Diagnostic } // InConfigBody returns a copy of the receiver with any config-contextual -// diagnostics elaborated in the context of the given body. -func (diags Diagnostics) InConfigBody(body hcl.Body) Diagnostics { +// diagnostics elaborated in the context of the given body. An optional address +// argument may be added to indicate which instance of the configuration the +// error related to. +func (diags Diagnostics) InConfigBody(body hcl.Body, addr string) Diagnostics { if len(diags) == 0 { return nil } @@ -37,7 +40,7 @@ func (diags Diagnostics) InConfigBody(body hcl.Body) Diagnostics { ret := make(Diagnostics, len(diags)) for i, srcDiag := range diags { if cd, isCD := srcDiag.(contextualFromConfigBody); isCD { - ret[i] = cd.ElaborateFromConfigBody(body) + ret[i] = cd.ElaborateFromConfigBody(body, addr) } else { ret[i] = srcDiag } @@ -112,7 +115,12 @@ type attributeDiagnostic struct { // source location information is still available, for more accuracy. This // is not always possible due to system architecture, so this serves as a // "best effort" fallback behavior for such situations. -func (d *attributeDiagnostic) ElaborateFromConfigBody(body hcl.Body) Diagnostic { +func (d *attributeDiagnostic) ElaborateFromConfigBody(body hcl.Body, addr string) Diagnostic { + // don't change an existing address + if d.address == "" { + d.address = addr + } + if len(d.attrPath) < 1 { // Should never happen, but we'll allow it rather than crashing. return d @@ -353,7 +361,12 @@ type wholeBodyDiagnostic struct { subject *SourceRange // populated only after ElaborateFromConfigBody } -func (d *wholeBodyDiagnostic) ElaborateFromConfigBody(body hcl.Body) Diagnostic { +func (d *wholeBodyDiagnostic) ElaborateFromConfigBody(body hcl.Body, addr string) Diagnostic { + // don't change an existing address + if d.address == "" { + d.address = addr + } + if d.subject != nil { // Don't modify an already-elaborated diagnostic. return d diff --git a/tfdiags/contextual_test.go b/tfdiags/contextual_test.go index 806d0c3a9..e29712a7b 100644 --- a/tfdiags/contextual_test.go +++ b/tfdiags/contextual_test.go @@ -185,6 +185,7 @@ simple_attr = "val" diagnosticBase: diagnosticBase{ summary: "preexisting", detail: "detail", + address: "original", }, subject: &SourceRange{ Filename: "somewhere_else.tf", @@ -535,9 +536,22 @@ simple_attr = "val" for i, tc := range testCases { t.Run(fmt.Sprintf("%d:%s", i, tc.Diag.Description()), func(t *testing.T) { var diags Diagnostics + + origAddr := tc.Diag.Description().Address diags = diags.Append(tc.Diag) - gotDiags := diags.InConfigBody(f.Body) + + gotDiags := diags.InConfigBody(f.Body, "test.addr") gotRange := gotDiags[0].Source().Subject + gotAddr := gotDiags[0].Description().Address + + switch { + case origAddr != "": + if gotAddr != origAddr { + t.Errorf("original diagnostic address modified from %s to %s", origAddr, gotAddr) + } + case gotAddr != "test.addr": + t.Error("missing detail address") + } for _, problem := range deep.Equal(gotRange, tc.ExpectedRange) { t.Error(problem) diff --git a/tfdiags/diagnostic.go b/tfdiags/diagnostic.go index a7699cf01..d84fa666f 100644 --- a/tfdiags/diagnostic.go +++ b/tfdiags/diagnostic.go @@ -25,6 +25,7 @@ const ( ) type Description struct { + Address string Summary string Detail string } diff --git a/tfdiags/diagnostic_base.go b/tfdiags/diagnostic_base.go index 50bf9d8eb..04e56773d 100644 --- a/tfdiags/diagnostic_base.go +++ b/tfdiags/diagnostic_base.go @@ -9,6 +9,7 @@ type diagnosticBase struct { severity Severity summary string detail string + address string } func (d diagnosticBase) Severity() Severity { @@ -19,6 +20,7 @@ func (d diagnosticBase) Description() Description { return Description{ Summary: d.summary, Detail: d.detail, + Address: d.address, } } From 406ac9796500585132732b510454d52d11e0ca9c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 2 Apr 2021 17:10:31 -0400 Subject: [PATCH 2/5] add the address field to the view diagnostics --- command/format/diagnostic.go | 4 ++++ command/views/json/diagnostic.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/command/format/diagnostic.go b/command/format/diagnostic.go index 2b1a7f826..2d9d9c8ae 100644 --- a/command/format/diagnostic.go +++ b/command/format/diagnostic.go @@ -213,6 +213,10 @@ func DiagnosticWarningsCompact(diags tfdiags.Diagnostics, color *colorstring.Col } func appendSourceSnippets(buf *bytes.Buffer, diag *viewsjson.Diagnostic, color *colorstring.Colorize) { + if diag.Address != "" { + fmt.Fprintf(buf, " (%s)\n", diag.Address) + } + if diag.Range == nil { return } diff --git a/command/views/json/diagnostic.go b/command/views/json/diagnostic.go index 3ea69db23..8ff595594 100644 --- a/command/views/json/diagnostic.go +++ b/command/views/json/diagnostic.go @@ -30,6 +30,7 @@ type Diagnostic struct { Severity string `json:"severity"` Summary string `json:"summary"` Detail string `json:"detail"` + Address string `json:"address,omitempty"` Range *DiagnosticRange `json:"range,omitempty"` Snippet *DiagnosticSnippet `json:"snippet,omitempty"` } @@ -124,6 +125,7 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost Severity: sev, Summary: desc.Summary, Detail: desc.Detail, + Address: desc.Address, } sourceRefs := diag.Source() From 265b5106cac80c2375ef2049598dd4bd6614d8cf Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 2 Apr 2021 17:11:20 -0400 Subject: [PATCH 3/5] call the InConfigBody with addresses --- backend/testing.go | 4 ++-- command/meta_backend.go | 4 ++-- plugin/convert/diagnostics_test.go | 2 +- terraform/node_provider.go | 6 +++--- terraform/node_resource_abstract_instance.go | 15 ++++++++------- terraform/node_resource_validate.go | 4 ++-- 6 files changed, 18 insertions(+), 17 deletions(-) diff --git a/backend/testing.go b/backend/testing.go index 1e0fd3b11..6a4c134c7 100644 --- a/backend/testing.go +++ b/backend/testing.go @@ -39,7 +39,7 @@ func TestBackendConfig(t *testing.T, b Backend, c hcl.Body) Backend { diags = diags.Append(decDiags) newObj, valDiags := b.PrepareConfig(obj) - diags = diags.Append(valDiags.InConfigBody(c)) + diags = diags.Append(valDiags.InConfigBody(c, "")) if len(diags) != 0 { t.Fatal(diags.ErrWithWarnings()) @@ -49,7 +49,7 @@ func TestBackendConfig(t *testing.T, b Backend, c hcl.Body) Backend { confDiags := b.Configure(obj) if len(confDiags) != 0 { - confDiags = confDiags.InConfigBody(c) + confDiags = confDiags.InConfigBody(c, "") t.Fatal(confDiags.ErrWithWarnings()) } diff --git a/command/meta_backend.go b/command/meta_backend.go index 329e5f84e..3aed84326 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -1086,13 +1086,13 @@ func (m *Meta) backendInitFromConfig(c *configs.Backend) (backend.Backend, cty.V } newVal, validateDiags := b.PrepareConfig(configVal) - diags = diags.Append(validateDiags.InConfigBody(c.Config)) + diags = diags.Append(validateDiags.InConfigBody(c.Config, "")) if validateDiags.HasErrors() { return nil, cty.NilVal, diags } configureDiags := b.Configure(newVal) - diags = diags.Append(configureDiags.InConfigBody(c.Config)) + diags = diags.Append(configureDiags.InConfigBody(c.Config, "")) return b, configVal, diags } diff --git a/plugin/convert/diagnostics_test.go b/plugin/convert/diagnostics_test.go index 604c98d57..00c0bf05e 100644 --- a/plugin/convert/diagnostics_test.go +++ b/plugin/convert/diagnostics_test.go @@ -393,7 +393,7 @@ func TestProtoDiagnostics_emptyAttributePath(t *testing.T) { if parseDiags.HasErrors() { t.Fatal(parseDiags) } - diags := tfDiags.InConfigBody(f.Body) + diags := tfDiags.InConfigBody(f.Body, "") if len(tfDiags) != 1 { t.Fatalf("expected 1 diag, got %d", len(tfDiags)) diff --git a/terraform/node_provider.go b/terraform/node_provider.go index a72c1f6f2..3e70cee08 100644 --- a/terraform/node_provider.go +++ b/terraform/node_provider.go @@ -167,14 +167,14 @@ func (n *NodeApplyableProvider) ConfigureProvider(ctx EvalContext, provider prov diags = diags.Append(tfdiags.Sourceless( tfdiags.Error, "Invalid provider configuration", - fmt.Sprintf(providerConfigErr, configDiags.InConfigBody(configBody).Err(), n.Addr.Provider), + fmt.Sprintf(providerConfigErr, configDiags.InConfigBody(configBody, n.Addr.String()).Err(), n.Addr.Provider), )) return diags } else { - return diags.Append(configDiags.InConfigBody(configBody)) + return diags.Append(configDiags.InConfigBody(configBody, n.Addr.String())) } } - diags = diags.Append(configDiags.InConfigBody(configBody)) + diags = diags.Append(configDiags.InConfigBody(configBody, n.Addr.String())) return diags } diff --git a/terraform/node_resource_abstract_instance.go b/terraform/node_resource_abstract_instance.go index eb7a492fc..7197311f5 100644 --- a/terraform/node_resource_abstract_instance.go +++ b/terraform/node_resource_abstract_instance.go @@ -625,8 +625,9 @@ func (n *NodeAbstractResourceInstance) plan( Config: unmarkedConfigVal, }, ) + if validateResp.Diagnostics.HasErrors() { - diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config)) + diags = diags.Append(validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) return plan, state, diags } @@ -659,7 +660,7 @@ func (n *NodeAbstractResourceInstance) plan( PriorPrivate: priorPrivate, ProviderMeta: metaConfigVal, }) - diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) if diags.HasErrors() { return plan, state, diags } @@ -869,7 +870,7 @@ func (n *NodeAbstractResourceInstance) plan( // Consequently, we break from the usual pattern here and only // append these new diagnostics if there's at least one error inside. if resp.Diagnostics.HasErrors() { - diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) return plan, state, diags } plannedNewVal = resp.PlannedState @@ -1195,7 +1196,7 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal }, ) if validateResp.Diagnostics.HasErrors() { - return newVal, validateResp.Diagnostics.InConfigBody(config.Config) + return newVal, validateResp.Diagnostics.InConfigBody(config.Config, n.Addr.String()) } // If we get down here then our configuration is complete and we're read @@ -1207,7 +1208,7 @@ func (n *NodeAbstractResourceInstance) readDataSource(ctx EvalContext, configVal Config: configVal, ProviderMeta: metaConfigVal, }) - diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(config.Config, n.Addr.String())) if diags.HasErrors() { return newVal, diags } @@ -1741,7 +1742,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state Connection: unmarkedConnInfo, UIOutput: &output, }) - applyDiags := resp.Diagnostics.InConfigBody(prov.Config) + applyDiags := resp.Diagnostics.InConfigBody(prov.Config, n.Addr.String()) // Call post hook hookErr := ctx.Hook(func(h Hook) (HookAction, error) { @@ -1907,7 +1908,7 @@ func (n *NodeAbstractResourceInstance) apply( }) applyDiags := resp.Diagnostics if applyConfig != nil { - applyDiags = applyDiags.InConfigBody(applyConfig.Config) + applyDiags = applyDiags.InConfigBody(applyConfig.Config, n.Addr.String()) } diags = diags.Append(applyDiags) diff --git a/terraform/node_resource_validate.go b/terraform/node_resource_validate.go index a1e7bc7e4..0f6640849 100644 --- a/terraform/node_resource_validate.go +++ b/terraform/node_resource_validate.go @@ -381,7 +381,7 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag } resp := provider.ValidateResourceConfig(req) - diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config, n.Addr.String())) case addrs.DataResourceMode: schema, _ := providerSchema.SchemaForResourceType(n.Config.Mode, n.Config.Type) @@ -409,7 +409,7 @@ func (n *NodeValidatableResource) validateResource(ctx EvalContext) tfdiags.Diag } resp := provider.ValidateDataResourceConfig(req) - diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config)) + diags = diags.Append(resp.Diagnostics.InConfigBody(n.Config.Config, n.Addr.String())) } return diags From 9e5baf4662f4b920350446c6aff8473a05a81ff8 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 2 Apr 2021 17:12:47 -0400 Subject: [PATCH 4/5] use WholeContainingBody instead of Sourceless When returning generic grpc errors from a provider, use WholeContainingBody so that callers can annotate the error with all the available contextual information. This can help troubleshoot problems by narrowing down problems to a particular configuration or specific resource instance. --- plugin/grpc_error.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/plugin/grpc_error.go b/plugin/grpc_error.go index 4875b576c..99ce8c8b8 100644 --- a/plugin/grpc_error.go +++ b/plugin/grpc_error.go @@ -45,26 +45,26 @@ func grpcErr(err error) (diags tfdiags.Diagnostics) { case codes.Unavailable: // This case is when the plugin has stopped running for some reason, // and is usually the result of a crash. - diags = diags.Append(tfdiags.Sourceless( + diags = diags.Append(tfdiags.WholeContainingBody( tfdiags.Error, "Plugin did not respond", fmt.Sprintf("The plugin encountered an error, and failed to respond to the %s call. "+ "The plugin logs may contain more details.", requestName), )) case codes.Canceled: - diags = diags.Append(tfdiags.Sourceless( + diags = diags.Append(tfdiags.WholeContainingBody( tfdiags.Error, "Request cancelled", fmt.Sprintf("The %s request was cancelled.", requestName), )) case codes.Unimplemented: - diags = diags.Append(tfdiags.Sourceless( + diags = diags.Append(tfdiags.WholeContainingBody( tfdiags.Error, "Unsupported plugin method", fmt.Sprintf("The %s method is not supported by this plugin.", requestName), )) default: - diags = diags.Append(tfdiags.Sourceless( + diags = diags.Append(tfdiags.WholeContainingBody( tfdiags.Error, "Plugin error", fmt.Sprintf("The plugin returned an unexpected error from %s: %v", requestName, err), From d04999863ccde3d015dca5c687800317ede8de25 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 6 Apr 2021 15:50:30 -0400 Subject: [PATCH 5/5] "with" formatting --- command/format/diagnostic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/format/diagnostic.go b/command/format/diagnostic.go index 2d9d9c8ae..5156595a3 100644 --- a/command/format/diagnostic.go +++ b/command/format/diagnostic.go @@ -214,7 +214,7 @@ func DiagnosticWarningsCompact(diags tfdiags.Diagnostics, color *colorstring.Col func appendSourceSnippets(buf *bytes.Buffer, diag *viewsjson.Diagnostic, color *colorstring.Colorize) { if diag.Address != "" { - fmt.Fprintf(buf, " (%s)\n", diag.Address) + fmt.Fprintf(buf, " with %s,\n", diag.Address) } if diag.Range == nil {