From 85ec09111b5ea32dd3c18b07f4b2d9672d6c884a Mon Sep 17 00:00:00 2001 From: James Nugent Date: Wed, 31 Aug 2016 15:26:57 -0500 Subject: [PATCH] helper/schema: Add diff suppression callback This commit adds a new callback, DiffSuppressFunc, to the schema.Schema structure. If set for a given schema, a callback to the user-supplied function will be made for each attribute for which the default type-based diff mechanism produces an attribute diff. Returning `true` from the callback will suppress the diff (i.e. pretend there was no diff), and returning false will retain it as part of the plan. There are a number of motivating examples for this - one of which is included as an example: 1. On SSH public keys, trailing whitespace does not matter in many cases - and in some cases it is added by provider APIs. For digitalocean_ssh_key resources we previously had a StateFunc that trimmed the whitespace - we now have a DiffSuppressFunc which verifies whether the trimmed strings are equivalent. 2. IAM policy equivalence for AWS. A good proportion of AWS issues relate to IAM policies which have been "normalized" (used loosely) by the IAM API endpoints. This can make the JSON strings differ from those generated by iam_policy_document resources or template files, even though the semantics are the same (for example, reordering of `bucket-prefix/` and `bucket-prefix/*` in an S3 bucket policy. DiffSupressFunc can be used to test for semantic equivalence rather than pure text equivalence, but without having to deal with the complexity associated with a full "provider-land" diff implementation without helper/schema. --- .../resource_digitalocean_ssh_key.go | 22 ++--- helper/schema/schema.go | 36 +++++++- helper/schema/schema_test.go | 86 +++++++++++++++++++ 3 files changed, 130 insertions(+), 14 deletions(-) diff --git a/builtin/providers/digitalocean/resource_digitalocean_ssh_key.go b/builtin/providers/digitalocean/resource_digitalocean_ssh_key.go index db57085b6..ba09206de 100644 --- a/builtin/providers/digitalocean/resource_digitalocean_ssh_key.go +++ b/builtin/providers/digitalocean/resource_digitalocean_ssh_key.go @@ -21,26 +21,24 @@ func resourceDigitalOceanSSHKey() *schema.Resource { }, Schema: map[string]*schema.Schema{ - "id": &schema.Schema{ + "id": { Type: schema.TypeString, Computed: true, }, - "name": &schema.Schema{ + "name": { Type: schema.TypeString, Required: true, }, - "public_key": &schema.Schema{ - Type: schema.TypeString, - Required: true, - ForceNew: true, - StateFunc: func(val interface{}) string { - return strings.TrimSpace(val.(string)) - }, + "public_key": { + Type: schema.TypeString, + Required: true, + ForceNew: true, + DiffSuppressFunc: resourceDigitalOceanSSHKeyPublicKeyDiffSuppress, }, - "fingerprint": &schema.Schema{ + "fingerprint": { Type: schema.TypeString, Computed: true, }, @@ -48,6 +46,10 @@ func resourceDigitalOceanSSHKey() *schema.Resource { } } +func resourceDigitalOceanSSHKeyPublicKeyDiffSuppress(k, old, new string, d *schema.ResourceData) bool { + return strings.TrimSpace(old) == strings.TrimSpace(new) +} + func resourceDigitalOceanSSHKeyCreate(d *schema.ResourceData, meta interface{}) error { client := meta.(*godo.Client) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index b2934a763..ca241f4ae 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -52,6 +52,15 @@ type Schema struct { Optional bool Required bool + // If this is non-nil, the provided function will be used during diff + // of this field. If this is nil, a default diff for the type of the + // schema will be used. + // + // This allows comparison based on something other than primitive, list + // or map equality - for example SSH public keys may be considered + // equivalent regardless of trailing whitespace. + DiffSuppressFunc SchemaDiffSuppressFunc + // If this is non-nil, then this will be a default value that is used // when this item is not set in the configuration/state. // @@ -153,6 +162,13 @@ type Schema struct { Sensitive bool } +// SchemaDiffSuppresFunc is a function which can be used to determine +// whether a detected diff on a schema element is "valid" or not, and +// suppress it from the plan if necessary. +// +// Return true if the diff should be suppressed, false to retain it. +type SchemaDiffSuppressFunc func(k, old, new string, d *ResourceData) bool + // SchemaDefaultFunc is a function called to return a default value for // a field. type SchemaDefaultFunc func() (interface{}, error) @@ -603,20 +619,32 @@ func (m schemaMap) diff( diff *terraform.InstanceDiff, d *ResourceData, all bool) error { + + unsupressedDiff := new(terraform.InstanceDiff) + unsupressedDiff.Attributes = make(map[string]*terraform.ResourceAttrDiff) + var err error switch schema.Type { case TypeBool, TypeInt, TypeFloat, TypeString: - err = m.diffString(k, schema, diff, d, all) + err = m.diffString(k, schema, unsupressedDiff, d, all) case TypeList: - err = m.diffList(k, schema, diff, d, all) + err = m.diffList(k, schema, unsupressedDiff, d, all) case TypeMap: - err = m.diffMap(k, schema, diff, d, all) + err = m.diffMap(k, schema, unsupressedDiff, d, all) case TypeSet: - err = m.diffSet(k, schema, diff, d, all) + err = m.diffSet(k, schema, unsupressedDiff, d, all) default: err = fmt.Errorf("%s: unknown type %#v", k, schema.Type) } + for attrK, attrV := range unsupressedDiff.Attributes { + if schema.DiffSuppressFunc != nil && schema.DiffSuppressFunc(attrK, attrV.Old, attrV.New, d) { + continue + } + + diff.Attributes[attrK] = attrV + } + return err } diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 5700aba2a..b7d7673b6 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -2939,6 +2939,92 @@ func TestSchemaMap_InternalValidate(t *testing.T) { } +func TestSchemaMap_DiffSuppress(t *testing.T) { + cases := map[string]struct { + Schema map[string]*Schema + State *terraform.InstanceState + Config map[string]interface{} + ConfigVariables map[string]ast.Variable + ExpectedDiff *terraform.InstanceDiff + Err bool + }{ + "#0 - Suppress otherwise valid diff by returning true": { + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeString, + Optional: true, + DiffSuppressFunc: func(k, old, new string, d *ResourceData) bool { + // Always suppress any diff + return true + }, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "availability_zone": "foo", + }, + + ExpectedDiff: nil, + + Err: false, + }, + "#1 - Don't suppress diff by returning false": { + Schema: map[string]*Schema{ + "availability_zone": { + Type: TypeString, + Optional: true, + DiffSuppressFunc: func(k, old, new string, d *ResourceData) bool { + // Always suppress any diff + return false + }, + }, + }, + + State: nil, + + Config: map[string]interface{}{ + "availability_zone": "foo", + }, + + ExpectedDiff: &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "availability_zone": { + Old: "", + New: "foo", + }, + }, + }, + + Err: false, + }, + } + + for tn, tc := range cases { + c, err := config.NewRawConfig(tc.Config) + if err != nil { + t.Fatalf("#%q err: %s", tn, err) + } + + if len(tc.ConfigVariables) > 0 { + if err := c.Interpolate(tc.ConfigVariables); err != nil { + t.Fatalf("#%q err: %s", tn, err) + } + } + + d, err := schemaMap(tc.Schema).Diff( + tc.State, terraform.NewResourceConfig(c)) + if err != nil != tc.Err { + t.Fatalf("#%q err: %s", tn, err) + } + + if !reflect.DeepEqual(tc.ExpectedDiff, d) { + t.Fatalf("#%q:\n\nexpected:\n%#v\n\ngot:\n%#v", tn, tc.ExpectedDiff, d) + } + } +} + func TestSchemaMap_Validate(t *testing.T) { cases := map[string]struct { Schema map[string]*Schema