From bafa44b958d4a123fac354624fde8ad4b74aec58 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 19 May 2021 11:10:05 -0400 Subject: [PATCH] return diagnostics from provisioners Do not convert provisioner diagnostics to errors so that users can get context from provisioner failures. Return diagnostics from the builtin provisioners that can be annotated with configuration context and instance addresses. --- .../provisioners/file/resource_provisioner.go | 25 ++++++++++-- .../file/resource_provisioner_test.go | 4 +- .../local-exec/resource_provisioner.go | 26 ++++++++++--- .../remote-exec/resource_provisioner.go | 38 +++++++++++++++---- .../remote-exec/resource_provisioner_test.go | 4 +- .../node_resource_abstract_instance.go | 32 +++++----------- 6 files changed, 87 insertions(+), 42 deletions(-) diff --git a/internal/builtin/provisioners/file/resource_provisioner.go b/internal/builtin/provisioners/file/resource_provisioner.go index 0ccb836ec..54c2e4a2b 100644 --- a/internal/builtin/provisioners/file/resource_provisioner.go +++ b/internal/builtin/provisioners/file/resource_provisioner.go @@ -10,6 +10,7 @@ import ( "github.com/hashicorp/terraform/internal/communicator" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/mitchellh/go-homedir" "github.com/zclconf/go-cty/cty" ) @@ -75,20 +76,32 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { if req.Connection.IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing connection configuration for provisioner")) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "file provisioner error", + "Missing connection configuration for provisioner.", + )) return resp } comm, err := communicator.New(req.Connection) if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "file provisioner error", + err.Error(), + )) return resp } // Get the source src, deleteSource, err := getSrc(req.Config) if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "file provisioner error", + err.Error(), + )) return resp } if deleteSource { @@ -98,7 +111,11 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques // Begin the file copy dst := req.Config.GetAttr("destination").AsString() if err := copyFiles(p.ctx, comm, src, dst); err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "file provisioner error", + err.Error(), + )) return resp } diff --git a/internal/builtin/provisioners/file/resource_provisioner_test.go b/internal/builtin/provisioners/file/resource_provisioner_test.go index 8dc869a75..c470743b3 100644 --- a/internal/builtin/provisioners/file/resource_provisioner_test.go +++ b/internal/builtin/provisioners/file/resource_provisioner_test.go @@ -112,7 +112,7 @@ func TestResourceProvisioner_connectionRequired(t *testing.T) { } got := resp.Diagnostics.Err().Error() - if !strings.Contains(got, "missing connection") { - t.Fatalf("expected 'missing connection' error: got %q", got) + if !strings.Contains(got, "Missing connection") { + t.Fatalf("expected 'Missing connection' error: got %q", got) } } diff --git a/internal/builtin/provisioners/local-exec/resource_provisioner.go b/internal/builtin/provisioners/local-exec/resource_provisioner.go index c4ac3e563..5a1dc04f2 100644 --- a/internal/builtin/provisioners/local-exec/resource_provisioner.go +++ b/internal/builtin/provisioners/local-exec/resource_provisioner.go @@ -11,6 +11,7 @@ import ( "github.com/armon/circbuf" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/mitchellh/go-linereader" "github.com/zclconf/go-cty/cty" ) @@ -65,7 +66,11 @@ func (p *provisioner) GetSchema() (resp provisioners.GetSchemaResponse) { func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisionerConfigRequest) (resp provisioners.ValidateProvisionerConfigResponse) { if _, err := p.GetSchema().Provisioner.CoerceValue(req.Config); err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "Invalid local-exec provisioner configuration", + err.Error(), + )) } return resp } @@ -73,7 +78,11 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { command := req.Config.GetAttr("command").AsString() if command == "" { - resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("local-exec provisioner command must be a non-empty string")) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "Invalid local-exec provisioner command", + "The command must be a non-empty string.", + )) return resp } @@ -120,7 +129,11 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques // See golang.org/issue/18874 pr, pw, err := os.Pipe() if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("failed to initialize pipe for output: %s", err)) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "local-exec provisioner error", + fmt.Sprintf("Failed to initialize pipe for output: %s", err), + )) return resp } @@ -171,8 +184,11 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques } if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(fmt.Errorf("Error running command '%s': %v. Output: %s", - command, err, output.Bytes())) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "local-exec provisioner error", + fmt.Sprintf("Error running command '%s': %v. Output: %s", command, err, output.Bytes()), + )) return resp } diff --git a/internal/builtin/provisioners/remote-exec/resource_provisioner.go b/internal/builtin/provisioners/remote-exec/resource_provisioner.go index a499add82..f8edfb785 100644 --- a/internal/builtin/provisioners/remote-exec/resource_provisioner.go +++ b/internal/builtin/provisioners/remote-exec/resource_provisioner.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform/internal/communicator/remote" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/provisioners" + "github.com/hashicorp/terraform/internal/tfdiags" "github.com/mitchellh/go-linereader" "github.com/zclconf/go-cty/cty" ) @@ -59,7 +60,11 @@ func (p *provisioner) GetSchema() (resp provisioners.GetSchemaResponse) { func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisionerConfigRequest) (resp provisioners.ValidateProvisionerConfigResponse) { cfg, err := p.GetSchema().Provisioner.CoerceValue(req.Config) if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "Invalid remote-exec provisioner configuration", + err.Error(), + )) return resp } @@ -78,28 +83,43 @@ func (p *provisioner) ValidateProvisionerConfig(req provisioners.ValidateProvisi set++ } if set != 1 { - resp.Diagnostics = resp.Diagnostics.Append(errors.New( - `only one of "inline", "script", or "scripts" must be set`)) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "Invalid remote-exec provisioner configuration", + `Only one of "inline", "script", or "scripts" must be set`, + )) } return resp } func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceRequest) (resp provisioners.ProvisionResourceResponse) { if req.Connection.IsNull() { - resp.Diagnostics = resp.Diagnostics.Append(errors.New("missing connection configuration for provisioner")) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "remote-exec provisioner error", + "Missing connection configuration for provisioner.", + )) return resp } comm, err := communicator.New(req.Connection) if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "remote-exec provisioner error", + err.Error(), + )) return resp } // Collect the scripts scripts, err := collectScripts(req.Config) if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "remote-exec provisioner error", + err.Error(), + )) return resp } for _, s := range scripts { @@ -108,7 +128,11 @@ func (p *provisioner) ProvisionResource(req provisioners.ProvisionResourceReques // Copy and execute each script if err := runScripts(p.ctx, req.UIOutput, comm, scripts); err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + resp.Diagnostics = resp.Diagnostics.Append(tfdiags.WholeContainingBody( + tfdiags.Error, + "remote-exec provisioner error", + err.Error(), + )) return resp } diff --git a/internal/builtin/provisioners/remote-exec/resource_provisioner_test.go b/internal/builtin/provisioners/remote-exec/resource_provisioner_test.go index fed3fe369..549dbf301 100644 --- a/internal/builtin/provisioners/remote-exec/resource_provisioner_test.go +++ b/internal/builtin/provisioners/remote-exec/resource_provisioner_test.go @@ -271,8 +271,8 @@ func TestResourceProvisioner_connectionRequired(t *testing.T) { } got := resp.Diagnostics.Err().Error() - if !strings.Contains(got, "missing connection") { - t.Fatalf("expected 'missing connection' error: got %q", got) + if !strings.Contains(got, "Missing connection") { + t.Fatalf("expected 'Missing connection' error: got %q", got) } } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index df3990001..45469e5a2 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1701,9 +1701,8 @@ func (n *NodeAbstractResourceInstance) evalApplyProvisioners(ctx EvalContext, st // If there are no errors, then we append it to our output error // if we have one, otherwise we just output it. - err := n.applyProvisioners(ctx, state, when, provs) - if err != nil { - diags = diags.Append(err) + diags = diags.Append(n.applyProvisioners(ctx, state, when, provs)) + if diags.HasErrors() { log.Printf("[TRACE] evalApplyProvisioners: %s provisioning failed, but we will continue anyway at the caller's request", n.Addr) return diags } @@ -1737,7 +1736,7 @@ func filterProvisioners(config *configs.Resource, when configs.ProvisionerWhen) } // applyProvisioners executes the provisioners for a resource. -func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state *states.ResourceInstanceObject, when configs.ProvisionerWhen, provs []*configs.Provisioner) error { +func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state *states.ResourceInstanceObject, when configs.ProvisionerWhen, provs []*configs.Provisioner) tfdiags.Diagnostics { var diags tfdiags.Diagnostics // this self is only used for destroy provisioner evaluation, and must @@ -1766,8 +1765,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state // Get the provisioner provisioner, err := ctx.Provisioner(prov.Type) if err != nil { - diags = diags.Append(err) - return diags.Err() + return diags.Append(err) } schema := ctx.ProvisionerSchema(prov.Type) @@ -1775,7 +1773,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state config, configDiags := evalScope(ctx, prov.Config, self, schema) diags = diags.Append(configDiags) if diags.HasErrors() { - return diags.Err() + return diags } // If the provisioner block contains a connection block of its own then @@ -1807,7 +1805,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state connInfo, connInfoDiags = evalScope(ctx, connBody, self, connectionBlockSupersetSchema) diags = diags.Append(connInfoDiags) if diags.HasErrors() { - return diags.Err() + return diags } } @@ -1817,7 +1815,7 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state return h.PreProvisionInstanceStep(n.Addr, prov.Type) }) if err != nil { - return err + return diags.Append(err) } } @@ -1874,27 +1872,17 @@ func (n *NodeAbstractResourceInstance) applyProvisioners(ctx EvalContext, state diags = diags.Append(applyDiags) if applyDiags.HasErrors() { log.Printf("[WARN] Errors while provisioning %s with %q, so aborting", n.Addr, prov.Type) - return diags.Err() + return diags } } // Deal with the hook if hookErr != nil { - return hookErr + return diags.Append(hookErr) } } - // we have to drop warning-only diagnostics for now - if diags.HasErrors() { - return diags.ErrWithWarnings() - } - - // log any warnings since we can't return them - if e := diags.ErrWithWarnings(); e != nil { - log.Printf("[WARN] applyProvisioners %s: %v", n.Addr, e) - } - - return nil + return diags } func (n *NodeAbstractResourceInstance) evalProvisionerConfig(ctx EvalContext, body hcl.Body, self cty.Value, schema *configschema.Block) (cty.Value, tfdiags.Diagnostics) {