From 9c87006c346b47d7825b91dbd449671e465eba34 Mon Sep 17 00:00:00 2001 From: Liam Cervante Date: Tue, 11 Apr 2023 10:54:30 +0200 Subject: [PATCH] checks: hide check diagnostics during plans that will not wait for approval (#32938) * checks: filter out check diagnostics during certain plans * wrap diagnostics produced by check blocks in a dedicated check block diagnostic * address comments --- internal/backend/local/backend_apply.go | 26 +++++++ internal/backend/local/backend_apply_test.go | 44 +++++++++++ .../local/testdata/apply-check/main.tf | 10 +++ internal/terraform/eval_conditions.go | 14 +++- .../node_resource_abstract_instance.go | 36 +++++---- internal/tfdiags/checks.go | 73 +++++++++++++++++++ internal/tfdiags/consolidate_warnings.go | 7 ++ internal/tfdiags/errors_as_warnings.go | 41 ----------- 8 files changed, 191 insertions(+), 60 deletions(-) create mode 100644 internal/backend/local/testdata/apply-check/main.tf create mode 100644 internal/tfdiags/checks.go delete mode 100644 internal/tfdiags/errors_as_warnings.go diff --git a/internal/backend/local/backend_apply.go b/internal/backend/local/backend_apply.go index f28b506019..c00f6ba960 100644 --- a/internal/backend/local/backend_apply.go +++ b/internal/backend/local/backend_apply.go @@ -180,6 +180,32 @@ func (b *Local) opApply( runningOp.Result = backend.OperationFailure return } + } else { + // If we didn't ask for confirmation from the user, and they have + // included any failing checks in their configuration, then they + // will see a very confusing output after the apply operation + // completes. This is because all the diagnostics from the plan + // operation will now be shown alongside the diagnostics from the + // apply operation. For check diagnostics, the plan output is + // irrelevant and simple noise after the same set of checks have + // been executed again during the apply stage. As such, we are going + // to remove all diagnostics marked as check diagnostics at this + // stage, so we will only show the user the check results from the + // apply operation. + // + // Note, if we did ask for approval then we would have displayed the + // plan check results at that point which is useful as the user can + // use them to make a decision about whether to apply the changes. + // It's just that if we didn't ask for approval then showing the + // user the checks from the plan alongside the checks from the apply + // is needlessly confusing. + var filteredDiags tfdiags.Diagnostics + for _, diag := range diags { + if !tfdiags.IsFromCheckBlock(diag) { + filteredDiags = filteredDiags.Append(diag) + } + } + diags = filteredDiags } } else { plan = lr.Plan diff --git a/internal/backend/local/backend_apply_test.go b/internal/backend/local/backend_apply_test.go index 493000a802..36ddf3c08c 100644 --- a/internal/backend/local/backend_apply_test.go +++ b/internal/backend/local/backend_apply_test.go @@ -72,6 +72,50 @@ test_instance.foo: t.Fatalf("unexpected error output:\n%s", errOutput) } } +func TestLocal_applyCheck(t *testing.T) { + b := TestLocal(t) + + p := TestLocalProvider(t, b, "test", applyFixtureSchema()) + p.ApplyResourceChangeResponse = &providers.ApplyResourceChangeResponse{NewState: cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("yes"), + "ami": cty.StringVal("bar"), + })} + + op, configCleanup, done := testOperationApply(t, "./testdata/apply-check") + defer configCleanup() + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Result != backend.OperationSuccess { + t.Fatal("operation failed") + } + + if p.ReadResourceCalled { + t.Fatal("ReadResource should not be called") + } + + if !p.PlanResourceChangeCalled { + t.Fatal("diff should be called") + } + + if !p.ApplyResourceChangeCalled { + t.Fatal("apply should be called") + } + + d := done(t) + if errOutput := d.Stderr(); errOutput != "" { + t.Fatalf("unexpected error output:\n%s", errOutput) + } + + if stdOutput := d.Stdout(); strings.Contains(stdOutput, "Check block assertion known after apply") { + // As we are running an auto approved plan the warning that was + // generated during the plan should have been hidden. + t.Fatalf("std output contained unexpected check output:\n%s", stdOutput) + } +} func TestLocal_applyEmptyDir(t *testing.T) { b := TestLocal(t) diff --git a/internal/backend/local/testdata/apply-check/main.tf b/internal/backend/local/testdata/apply-check/main.tf new file mode 100644 index 0000000000..5782be8f47 --- /dev/null +++ b/internal/backend/local/testdata/apply-check/main.tf @@ -0,0 +1,10 @@ +resource "test_instance" "foo" { + ami = "bar" +} + +check "test_instance_exists" { + assert { + condition = test_instance.foo.id != null + error_message = "value should have been computed" + } +} diff --git a/internal/terraform/eval_conditions.go b/internal/terraform/eval_conditions.go index 881f5dd595..b80460707b 100644 --- a/internal/terraform/eval_conditions.go +++ b/internal/terraform/eval_conditions.go @@ -127,14 +127,14 @@ func evalCheckRule(typ addrs.CheckRuleType, rule *configs.CheckRule, ctx EvalCon // Check assertions warn if a status is unknown. if typ == addrs.CheckAssertion { - diags = diags.Append(&hcl.Diagnostic{ + diags = diags.Append(tfdiags.AsCheckBlockDiagnostic(&hcl.Diagnostic{ Severity: hcl.DiagWarning, Summary: fmt.Sprintf("%s known after apply", typ.Description()), Detail: "The condition could not be evaluated at this time, a result will be known when this plan is applied.", Subject: rule.Condition.Range().Ptr(), Expression: rule.Condition, EvalContext: hclCtx, - }) + })) } // We'll wait until we've learned more, then. @@ -186,7 +186,7 @@ func evalCheckRule(typ addrs.CheckRuleType, rule *configs.CheckRule, ctx EvalCon if errorMessageForDiags == "" { errorMessageForDiags = "This check failed, but has an invalid error message as described in the other accompanying messages." } - diags = diags.Append(&hcl.Diagnostic{ + diag := &hcl.Diagnostic{ // The caller gets to choose the severity of this one, because we // treat condition failures as warnings in the presence of // certain special planning options. @@ -196,7 +196,13 @@ func evalCheckRule(typ addrs.CheckRuleType, rule *configs.CheckRule, ctx EvalCon Subject: rule.Condition.Range().Ptr(), Expression: rule.Condition, EvalContext: hclCtx, - }) + } + + if typ == addrs.CheckAssertion { + diags = diags.Append(tfdiags.AsCheckBlockDiagnostic(diag)) + } else { + diags = diags.Append(diag) + } return checkResult{ Status: status, diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 8b5ed87f11..73bc00c7c1 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1685,7 +1685,6 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule // We have a complete configuration with no dependencies to wait on, so we // can read the data source into the state. newVal, readDiags := n.readDataSource(ctx, configVal) - diags = diags.Append(readDiags) // Now we've loaded the data, and diags tells us whether we were successful // or not, we are going to create our plannedChange and our @@ -1709,7 +1708,7 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule // and handle the error. We'll also create the plannedChange if // appropriate. - if diags.HasErrors() { + if readDiags.HasErrors() { // If we had errors, then we can cover that up by marking the new // state as unknown. unmarkedConfigVal, configMarkPaths := configVal.UnmarkDeepWithPaths() @@ -1718,13 +1717,14 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule // We still want to report the check as failed even if we are still // letting it run again during the apply stage. - ctx.Checks().ReportCheckFailure(check.Addr().Absolute(n.Addr.Module), addrs.CheckDataResource, 0, diags.Err().Error()) - - // Also, let's hide the errors so that execution can continue as - // normal. - diags = tfdiags.WithErrorsAsWarnings(diags) + ctx.Checks().ReportCheckFailure(check.Addr().Absolute(n.Addr.Module), addrs.CheckDataResource, 0, readDiags.Err().Error()) } + // Any warning or error diagnostics we'll wrap with some special checks + // diagnostics. This is so we can identify them later, and so they'll + // only report as warnings. + readDiags = tfdiags.AsCheckBlockDiagnostics(readDiags) + if !skipPlanChanges { // refreshOnly plans cannot produce planned changes, so we only do // this if skipPlanChanges is false. @@ -1740,9 +1740,9 @@ func (n *NodeAbstractResourceInstance) planDataSource(ctx EvalContext, checkRule ActionReason: plans.ResourceInstanceReadBecauseCheckNested, } } - } + diags = diags.Append(readDiags) if !diags.HasErrors() { // Finally, let's make our new state. plannedNewState = &states.ResourceInstanceObject{ @@ -1883,20 +1883,26 @@ func (n *NodeAbstractResourceInstance) applyDataSource(ctx EvalContext, planned } newVal, readDiags := n.readDataSource(ctx, configVal) - diags = diags.Append(readDiags) - if check, nested := n.nestedInCheckBlock(); nested { - // We're just going to jump in here and hide away any erros for nested + // We're just going to jump in here and hide away any errors for nested // data blocks. - if diags.HasErrors() { - ctx.Checks().ReportCheckFailure(check.Addr().Absolute(n.Addr.Module), addrs.CheckDataResource, 0, diags.Err().Error()) - return nil, keyData, tfdiags.WithErrorsAsWarnings(diags) + if readDiags.HasErrors() { + ctx.Checks().ReportCheckFailure(check.Addr().Absolute(n.Addr.Module), addrs.CheckDataResource, 0, readDiags.Err().Error()) + diags = diags.Append(tfdiags.AsCheckBlockDiagnostics(readDiags)) + return nil, keyData, diags } + // Even though we know there are no errors here, we still want to + // identify these diags has having been generated from a check block. + readDiags = tfdiags.AsCheckBlockDiagnostics(readDiags) + // If no errors, just remember to report this as a success and continue // as normal. ctx.Checks().ReportCheckResult(check.Addr().Absolute(n.Addr.Module), addrs.CheckDataResource, 0, checks.StatusPass) - } else if diags.HasErrors() { + } + + diags = diags.Append(readDiags) + if readDiags.HasErrors() { return nil, keyData, diags } diff --git a/internal/tfdiags/checks.go b/internal/tfdiags/checks.go new file mode 100644 index 0000000000..1a31eb74a8 --- /dev/null +++ b/internal/tfdiags/checks.go @@ -0,0 +1,73 @@ +package tfdiags + +import ( + "fmt" + + "github.com/hashicorp/hcl/v2" +) + +var _ Diagnostic = CheckBlockDiagnostic{} + +// CheckBlockDiagnostic is a diagnostic produced by a Terraform config Check block. +// +// It only ever returns warnings, and will not be consolidated as part of the +// Diagnostics.ConsolidateWarnings function. +type CheckBlockDiagnostic struct { + diag Diagnostic +} + +// AsCheckBlockDiagnostics will wrap every diagnostic in diags in a +// CheckBlockDiagnostic. +func AsCheckBlockDiagnostics(diags Diagnostics) Diagnostics { + if len(diags) == 0 { + return nil + } + + ret := make(Diagnostics, len(diags)) + for i, diag := range diags { + ret[i] = CheckBlockDiagnostic{diag} + } + return ret +} + +// AsCheckBlockDiagnostic will wrap a Diagnostic or a hcl.Diagnostic in a +// CheckBlockDiagnostic. +func AsCheckBlockDiagnostic(diag interface{}) Diagnostic { + switch d := diag.(type) { + case Diagnostic: + return CheckBlockDiagnostic{d} + case *hcl.Diagnostic: + return CheckBlockDiagnostic{hclDiagnostic{d}} + default: + panic(fmt.Errorf("can't construct diagnostic from %T", diag)) + } +} + +// IsFromCheckBlock returns true if the specified Diagnostic is a +// CheckBlockDiagnostic. +func IsFromCheckBlock(diag Diagnostic) bool { + _, ok := diag.(CheckBlockDiagnostic) + return ok +} + +func (c CheckBlockDiagnostic) Severity() Severity { + // Regardless of the severity of the underlying diagnostic, check blocks + // only ever report Warning severity. + return Warning +} + +func (c CheckBlockDiagnostic) Description() Description { + return c.diag.Description() +} + +func (c CheckBlockDiagnostic) Source() Source { + return c.diag.Source() +} + +func (c CheckBlockDiagnostic) FromExpr() *FromExpr { + return c.diag.FromExpr() +} + +func (c CheckBlockDiagnostic) ExtraInfo() interface{} { + return c.diag.ExtraInfo() +} diff --git a/internal/tfdiags/consolidate_warnings.go b/internal/tfdiags/consolidate_warnings.go index 08d36d60b6..0de00bd1f5 100644 --- a/internal/tfdiags/consolidate_warnings.go +++ b/internal/tfdiags/consolidate_warnings.go @@ -43,6 +43,13 @@ func (diags Diagnostics) ConsolidateWarnings(threshold int) Diagnostics { continue } + if _, ok := diag.(CheckBlockDiagnostic); ok { + // Check diagnostics are never consolidated, the users have asked + // to be informed about each of these. + newDiags = newDiags.Append(diag) + continue + } + desc := diag.Description() summary := desc.Summary if g, ok := warningGroups[summary]; ok { diff --git a/internal/tfdiags/errors_as_warnings.go b/internal/tfdiags/errors_as_warnings.go deleted file mode 100644 index c208a36014..0000000000 --- a/internal/tfdiags/errors_as_warnings.go +++ /dev/null @@ -1,41 +0,0 @@ -package tfdiags - -type diagForceWarningSeverity struct { - wrapped Diagnostic -} - -func WithErrorsAsWarnings(diags Diagnostics) Diagnostics { - if len(diags) == 0 { - return nil - } - - ret := make(Diagnostics, len(diags)) - for i, diag := range diags { - if diag.Severity() == Error { - ret[i] = diagForceWarningSeverity{diag} - } else { - ret[i] = diag - } - } - return ret -} - -func (diag diagForceWarningSeverity) Severity() Severity { - return Warning -} - -func (diag diagForceWarningSeverity) Description() Description { - return diag.wrapped.Description() -} - -func (diag diagForceWarningSeverity) Source() Source { - return diag.wrapped.Source() -} - -func (diag diagForceWarningSeverity) FromExpr() *FromExpr { - return diag.wrapped.FromExpr() -} - -func (diag diagForceWarningSeverity) ExtraInfo() any { - return diag.wrapped.ExtraInfo() -}