From 96b099cf352ad2d29a04bab71fce8910d78f033d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 4 Nov 2020 11:03:12 -0500 Subject: [PATCH 1/2] allow nil config attributes in ignore_changes The validation for ignore_changes was too broad, and makes it difficult to ignore changes in an attribute that the user does not want to set. While the goal of ignore_changes is to prevent changes in the configuration alone, we don't intend to break the use-case of ignoring drift from the provider. Since we cannot easily narrow the validation to only detect computed attributes at the moment, we can drop this error altogether for now. --- terraform/context_validate_test.go | 44 ------------------------------ terraform/eval_refresh.go | 2 ++ terraform/eval_validate.go | 35 ++++-------------------- 3 files changed, 7 insertions(+), 74 deletions(-) diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index e35803464..f9c22d921 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -1752,50 +1752,6 @@ output "out" { } } -func TestContext2Validate_invalidIgnoreChanges(t *testing.T) { - // validate module and output depends_on - m := testModuleInline(t, map[string]string{ - "main.tf": ` -resource "test_instance" "a" { - lifecycle { - ignore_changes = [foo] - } -} - -`, - }) - - p := testProvider("test") - p.GetSchemaReturn = &ProviderSchema{ - ResourceTypes: map[string]*configschema.Block{ - "test_instance": { - Attributes: map[string]*configschema.Attribute{ - "id": {Type: cty.String, Computed: true}, - "foo": {Type: cty.String, Computed: true, Optional: true}, - }, - }, - }, - } - - ctx := testContext2(t, &ContextOpts{ - Config: m, - Providers: map[addrs.Provider]providers.Factory{ - addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), - }, - }) - diags := ctx.Validate() - if !diags.HasErrors() { - t.Fatal("succeeded; want errors") - } - - for _, d := range diags { - des := d.Description().Summary - if !strings.Contains(des, "Cannot ignore") { - t.Fatalf(`expected "Invalid depends_on reference", got %q`, des) - } - } -} - func TestContext2Validate_rpcDiagnostics(t *testing.T) { // validate module and output depends_on m := testModuleInline(t, map[string]string{ diff --git a/terraform/eval_refresh.go b/terraform/eval_refresh.go index d69bedd64..0b5481031 100644 --- a/terraform/eval_refresh.go +++ b/terraform/eval_refresh.go @@ -127,6 +127,8 @@ func (n *EvalRefresh) Eval(ctx EvalContext) tfdiags.Diagnostics { // We have no way to exempt provider using the legacy SDK from this check, // so we can only log inconsistencies with the updated state values. + // In most cases these are not errors anyway, and represent "drift" from + // external changes which will be handled by the subsequent plan. if errs := objchange.AssertObjectCompatible(schema, priorVal, resp.NewState); len(errs) > 0 { var buf strings.Builder fmt.Fprintf(&buf, "[WARN] Provider %q produced an unexpected new value for %s during refresh.", n.ProviderAddr.Provider.String(), absAddr) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 288cc62a2..182f4d1f9 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -362,39 +362,14 @@ func (n *EvalValidateResource) Validate(ctx EvalContext) error { if cfg.Managed != nil { // can be nil only in tests with poorly-configured mocks for _, traversal := range cfg.Managed.IgnoreChanges { - // This will error out if the traversal contains an invalid - // index step. That is OK if we want users to be able to ignore - // a key that is no longer specified in the config. + // validate the ignore_changes traversals apply. moreDiags := schema.StaticValidateTraversal(traversal) diags = diags.Append(moreDiags) - if diags.HasErrors() { - continue - } - // first check to see if this assigned in the config - v, _ := traversal.TraverseRel(configVal) - if !v.IsNull() { - // it's assigned, so we can also assume it's not computed-only - continue - } - - // We can't ignore changes that don't exist in the configuration. - // We're not checking specifically if the traversal resolves to - // a computed-only value, but we can hint to the user that it - // might also be the case. - sourceRange := traversal.SourceRange() - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Cannot ignore argument not set in the configuration", - Detail: fmt.Sprintf("The ignore_changes argument is not set in the configuration.\n" + - "The ignore_changes mechanism only applies to changes " + - "within the configuration, and must be used with " + - "arguments set in the configuration and not computed by " + - "the provider.", - ), - Subject: &sourceRange, - }) - return diags.Err() + // TODO: we want to notify users that they can't use + // ignore_changes for computed attributes, but we don't have an + // easy way to correlate the config value, schema and + // traversal together. } } From 99db18388a21082bcc09d3a66ee973d001e68a41 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 4 Nov 2020 11:14:53 -0500 Subject: [PATCH 2/2] return diagnostics from resource validation Return diagnostics rather than an error, since the EvalNodes have been removed and the caller has been updated. IgnoreWarnings was no longer used. --- terraform/eval_validate.go | 34 +++++++------------- terraform/eval_validate_test.go | 57 ++++++--------------------------- 2 files changed, 20 insertions(+), 71 deletions(-) diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index 182f4d1f9..a84aa3ac9 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -252,10 +252,6 @@ type EvalValidateResource struct { Config *configs.Resource ProviderMetas map[addrs.Provider]*configs.ProviderMeta - // IgnoreWarnings means that warnings will not be passed through. This allows - // "just-in-time" passes of validation to continue execution through warnings. - IgnoreWarnings bool - // ConfigVal, if non-nil, will be updated with the value resulting from // evaluating the given configuration body. Since validation is performed // very early, this value is likely to contain lots of unknown values, @@ -264,12 +260,14 @@ type EvalValidateResource struct { ConfigVal *cty.Value } -func (n *EvalValidateResource) Validate(ctx EvalContext) error { - if n.ProviderSchema == nil || *n.ProviderSchema == nil { - return fmt.Errorf("EvalValidateResource has nil schema for %s", n.Addr) +func (n *EvalValidateResource) Validate(ctx EvalContext) tfdiags.Diagnostics { + var diags tfdiags.Diagnostics + + if *n.ProviderSchema == nil { + diags = diags.Append(fmt.Errorf("EvalValidateResource has nil schema for %s", n.Addr)) + return diags } - var diags tfdiags.Diagnostics provider := *n.Provider cfg := *n.Config schema := *n.ProviderSchema @@ -351,13 +349,13 @@ func (n *EvalValidateResource) Validate(ctx EvalContext) error { Detail: fmt.Sprintf("The provider %s does not support resource type %q.", cfg.ProviderConfigAddr(), cfg.Type), Subject: &cfg.TypeRange, }) - return diags.Err() + return diags } configVal, _, valDiags := ctx.EvaluateBlock(cfg.Config, schema, nil, keyData) diags = diags.Append(valDiags) if valDiags.HasErrors() { - return diags.Err() + return diags } if cfg.Managed != nil { // can be nil only in tests with poorly-configured mocks @@ -394,13 +392,13 @@ func (n *EvalValidateResource) Validate(ctx EvalContext) error { Detail: fmt.Sprintf("The provider %s does not support data source %q.", cfg.ProviderConfigAddr(), cfg.Type), Subject: &cfg.TypeRange, }) - return diags.Err() + return diags } configVal, _, valDiags := ctx.EvaluateBlock(cfg.Config, schema, nil, keyData) diags = diags.Append(valDiags) if valDiags.HasErrors() { - return diags.Err() + return diags } req := providers.ValidateDataSourceConfigRequest{ @@ -412,17 +410,7 @@ func (n *EvalValidateResource) Validate(ctx EvalContext) error { diags = diags.Append(resp.Diagnostics.InConfigBody(cfg.Config)) } - if n.IgnoreWarnings { - // If we _only_ have warnings then we'll return nil. - if diags.HasErrors() { - return diags.NonFatalErr() - } - return nil - } else { - // We'll return an error if there are any diagnostics at all, even if - // some of them are warnings. - return diags.NonFatalErr() - } + return diags } func (n *EvalValidateResource) validateCount(ctx EvalContext, expr hcl.Expression) tfdiags.Diagnostics { diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go index ce23b3596..5ff84fee1 100644 --- a/terraform/eval_validate_test.go +++ b/terraform/eval_validate_test.go @@ -238,45 +238,6 @@ func TestEvalValidateResource_warningsAndErrorsPassedThrough(t *testing.T) { } } -func TestEvalValidateResource_ignoreWarnings(t *testing.T) { - mp := simpleMockProvider() - mp.ValidateResourceTypeConfigFn = func(req providers.ValidateResourceTypeConfigRequest) providers.ValidateResourceTypeConfigResponse { - var diags tfdiags.Diagnostics - diags = diags.Append(tfdiags.SimpleWarning("warn")) - return providers.ValidateResourceTypeConfigResponse{ - Diagnostics: diags, - } - } - - p := providers.Interface(mp) - rc := &configs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "test_object", - Name: "foo", - Config: configs.SynthBody("", map[string]cty.Value{}), - } - node := &EvalValidateResource{ - Addr: addrs.Resource{ - Mode: addrs.ManagedResourceMode, - Type: "test-object", - Name: "foo", - }, - Provider: &p, - Config: rc, - ProviderSchema: &mp.GetSchemaReturn, - - IgnoreWarnings: true, - } - - ctx := &MockEvalContext{} - ctx.installSimpleEval() - - err := node.Validate(ctx) - if err != nil { - t.Fatalf("Expected no error, got: %s", err) - } -} - func TestEvalValidateResource_invalidDependsOn(t *testing.T) { mp := simpleMockProvider() mp.ValidateResourceTypeConfigFn = func(req providers.ValidateResourceTypeConfigRequest) providers.ValidateResourceTypeConfigResponse { @@ -323,9 +284,9 @@ func TestEvalValidateResource_invalidDependsOn(t *testing.T) { ctx := &MockEvalContext{} ctx.installSimpleEval() - err := node.Validate(ctx) - if err != nil { - t.Fatalf("error for supposedly-valid config: %s", err) + diags := node.Validate(ctx) + if diags.HasErrors() { + t.Fatalf("error for supposedly-valid config: %s", diags.ErrWithWarnings()) } // Now we'll make it invalid by adding additional traversal steps at @@ -344,11 +305,11 @@ func TestEvalValidateResource_invalidDependsOn(t *testing.T) { }, }) - err = node.Validate(ctx) - if err == nil { + diags = node.Validate(ctx) + if !diags.HasErrors() { t.Fatal("no error for invalid depends_on") } - if got, want := err.Error(), "Invalid depends_on reference"; !strings.Contains(got, want) { + if got, want := diags.Err().Error(), "Invalid depends_on reference"; !strings.Contains(got, want) { t.Fatalf("wrong error\ngot: %s\nwant: Message containing %q", got, want) } @@ -360,11 +321,11 @@ func TestEvalValidateResource_invalidDependsOn(t *testing.T) { }, }) - err = node.Validate(ctx) - if err == nil { + diags = node.Validate(ctx) + if !diags.HasErrors() { t.Fatal("no error for invalid depends_on") } - if got, want := err.Error(), "Invalid depends_on reference"; !strings.Contains(got, want) { + if got, want := diags.Err().Error(), "Invalid depends_on reference"; !strings.Contains(got, want) { t.Fatalf("wrong error\ngot: %s\nwant: Message containing %q", got, want) } }