From 179aa62ad416731407ed77a9e9dece7edd6d5e9b Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Wed, 15 Feb 2017 21:52:04 -0500 Subject: [PATCH 1/2] provider/cloudflare: Fix record validation Previously we only validated that the cloudflare record provided was a valid record type. However, a record can be of a valid type, and still not be proxied, making it an invalid record type. The main downside to having to check for whether or not the record type is proxied or not during validation, is that it relies on having two schema keys populated. This means that we can only catch the improper record type during `apply` time, instead of `plan` time. ``` $ go test -v -run "TestValidateRecordType" ./builtin/providers/cloudflare === RUN TestValidateRecordType --- PASS: TestValidateRecordType (0.00s) PASS ok github.com/hashicorp/terraform/builtin/providers/cloudflare 0.004s ``` --- .../cloudflare/resource_cloudflare_record.go | 30 ++++++----- builtin/providers/cloudflare/validators.go | 54 ++++++++++++------- .../providers/cloudflare/validators_test.go | 52 +++++++++--------- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/builtin/providers/cloudflare/resource_cloudflare_record.go b/builtin/providers/cloudflare/resource_cloudflare_record.go index bf9e95fcf..b0bdf65a5 100644 --- a/builtin/providers/cloudflare/resource_cloudflare_record.go +++ b/builtin/providers/cloudflare/resource_cloudflare_record.go @@ -18,51 +18,50 @@ func resourceCloudFlareRecord() *schema.Resource { SchemaVersion: 1, MigrateState: resourceCloudFlareRecordMigrateState, Schema: map[string]*schema.Schema{ - "domain": &schema.Schema{ + "domain": { Type: schema.TypeString, Required: true, }, - "name": &schema.Schema{ + "name": { Type: schema.TypeString, Required: true, }, - "hostname": &schema.Schema{ + "hostname": { Type: schema.TypeString, Computed: true, }, - "type": &schema.Schema{ - Type: schema.TypeString, - Required: true, - ForceNew: true, - ValidateFunc: validateRecordType, + "type": { + Type: schema.TypeString, + Required: true, + ForceNew: true, }, - "value": &schema.Schema{ + "value": { Type: schema.TypeString, Required: true, }, - "ttl": &schema.Schema{ + "ttl": { Type: schema.TypeInt, Optional: true, Computed: true, }, - "priority": &schema.Schema{ + "priority": { Type: schema.TypeInt, Optional: true, }, - "proxied": &schema.Schema{ + "proxied": { Default: false, Optional: true, Type: schema.TypeBool, }, - "zone_id": &schema.Schema{ + "zone_id": { Type: schema.TypeString, Computed: true, }, @@ -94,6 +93,11 @@ func resourceCloudFlareRecordCreate(d *schema.ResourceData, meta interface{}) er return fmt.Errorf("Error validating record name %q: %s", newRecord.Name, err) } + // Validate type + if err := validateRecordType(newRecord.Type, newRecord.Proxied); err != nil { + fmt.Errorf("Error validating record type %q: %s", newRecord.Type, err) + } + zoneId, err := client.ZoneIDByName(newRecord.ZoneName) if err != nil { return fmt.Errorf("Error finding zone %q: %s", newRecord.ZoneName, err) diff --git a/builtin/providers/cloudflare/validators.go b/builtin/providers/cloudflare/validators.go index 15dc51c6d..696a45f4f 100644 --- a/builtin/providers/cloudflare/validators.go +++ b/builtin/providers/cloudflare/validators.go @@ -7,26 +7,44 @@ import ( ) // validateRecordType ensures that the cloudflare record type is valid -func validateRecordType(v interface{}, k string) (ws []string, errors []error) { - value := v.(string) - - validTypes := map[string]struct{}{ - "A": {}, - "AAAA": {}, - "CNAME": {}, - "TXT": {}, - "SRV": {}, - "LOC": {}, - "MX": {}, - "NS": {}, - "SPF": {}, +func validateRecordType(t string, proxied bool) error { + switch t { + case "A": + return nil + case "AAAA": + return nil + case "CNAME": + return nil + case "TXT": + if !proxied { + return nil + } + case "SRV": + if !proxied { + return nil + } + case "LOC": + if !proxied { + return nil + } + case "MX": + if !proxied { + return nil + } + case "NS": + if !proxied { + return nil + } + case "SPF": + if !proxied { + return nil + } + default: + return fmt.Errorf( + `Invalid type %q. Valid types are "A", "AAAA", "CNAME", "TXT", "SRV", "LOC", "MX", "NS" or "SPF"`, t) } - if _, ok := validTypes[value]; !ok { - errors = append(errors, fmt.Errorf( - `%q contains an invalid type %q. Valid types are "A", "AAAA", "CNAME", "TXT", "SRV", "LOC", "MX", "NS" or "SPF"`, k, value)) - } - return + return fmt.Errorf("Type %q cannot be proxied", t) } // validateRecordName ensures that based on supplied record type, the name content matches diff --git a/builtin/providers/cloudflare/validators_test.go b/builtin/providers/cloudflare/validators_test.go index 0c97b18dc..6e3976099 100644 --- a/builtin/providers/cloudflare/validators_test.go +++ b/builtin/providers/cloudflare/validators_test.go @@ -3,36 +3,38 @@ package cloudflare import "testing" func TestValidateRecordType(t *testing.T) { - validTypes := []string{ - "A", - "AAAA", - "CNAME", - "TXT", - "SRV", - "LOC", - "MX", - "NS", - "SPF", + validTypes := map[string]bool{ + "A": true, + "AAAA": true, + "CNAME": true, + "TXT": false, + "SRV": false, + "LOC": false, + "MX": false, + "NS": false, + "SPF": false, } - for _, v := range validTypes { - _, errors := validateRecordType(v, "type") - if len(errors) != 0 { - t.Fatalf("%q should be a valid record type: %q", v, errors) + for k, v := range validTypes { + err := validateRecordType(k, v) + if err != nil { + t.Fatalf("%s should be a valid record type: %s", k, err) } } - invalidTypes := []string{ - "a", - "cName", - "txt", - "SRv", - "foo", - "bar", + invalidTypes := map[string]bool{ + "a": false, + "cName": false, + "txt": false, + "SRv": false, + "foo": false, + "bar": false, + "TXT": true, + "SRV": true, + "SPF": true, } - for _, v := range invalidTypes { - _, errors := validateRecordType(v, "type") - if len(errors) == 0 { - t.Fatalf("%q should be an invalid record type", v) + for k, v := range invalidTypes { + if err := validateRecordType(k, v); err == nil { + t.Fatalf("%s should be an invalid record type", k) } } } From e052ded26790b0e55c3b973f72eb1456d34b0dbe Mon Sep 17 00:00:00 2001 From: Jake Champlin Date: Thu, 16 Feb 2017 07:57:34 -0500 Subject: [PATCH 2/2] add missing return --- builtin/providers/cloudflare/resource_cloudflare_record.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/providers/cloudflare/resource_cloudflare_record.go b/builtin/providers/cloudflare/resource_cloudflare_record.go index b0bdf65a5..b0000fe4a 100644 --- a/builtin/providers/cloudflare/resource_cloudflare_record.go +++ b/builtin/providers/cloudflare/resource_cloudflare_record.go @@ -95,7 +95,7 @@ func resourceCloudFlareRecordCreate(d *schema.ResourceData, meta interface{}) er // Validate type if err := validateRecordType(newRecord.Type, newRecord.Proxied); err != nil { - fmt.Errorf("Error validating record type %q: %s", newRecord.Type, err) + return fmt.Errorf("Error validating record type %q: %s", newRecord.Type, err) } zoneId, err := client.ZoneIDByName(newRecord.ZoneName)