aws_route53_record: More consistent unquoting of TXT/SPF records. (#14170)

* aws_route53_record: More consistent unquoting of TXT/SPF records.

Before, 'flatten' would remove the first two quotes.  This results in
confusing behavior if the value contained two quoted strings.

Now, 'flatten' we only remove the surrounding qutoes, which is more
consistent with 'expand'.

Should improve hashicorp/terraform#8423, but more could still be done.

* aws/route53: Cover new bugfix with an acceptance test

builtin/providers/aws/resource_aws_route53_record_test.go

@@ -406,6 +406,24 @@ func TestAccAWSRoute53Record_empty(t *testing.T) {
 	})
 }
 
+// Regression test for https://github.com/hashicorp/terraform/issues/8423
+func TestAccAWSRoute53Record_longTXTrecord(t *testing.T) {
+	resource.Test(t, resource.TestCase{
+		PreCheck:      func() { testAccPreCheck(t) },
+		IDRefreshName: "aws_route53_record.long_txt",
+		Providers:     testAccProviders,
+		CheckDestroy:  testAccCheckRoute53RecordDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccRoute53RecordConfigLongTxtRecord,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckRoute53RecordExists("aws_route53_record.long_txt"),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckRoute53RecordDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).r53conn
 	for _, rs := range s.RootModule().Resources {
@@ -1161,3 +1179,19 @@ resource "aws_route53_record" "empty" {
 	records = ["127.0.0.1"]
 }
 `
+
+const testAccRoute53RecordConfigLongTxtRecord = `
+resource "aws_route53_zone" "main" {
+	name = "notexample.com"
+}
+
+resource "aws_route53_record" "long_txt" {
+    zone_id = "${aws_route53_zone.main.zone_id}"
+    name = "google.notexample.com"
+    type = "TXT"
+    ttl = "30"
+    records = [
+        "v=DKIM1; k=rsa; p=MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAiajKNMp\" \"/A12roF4p3MBm9QxQu6GDsBlWUWFx8EaS8TCo3Qe8Cj0kTag1JMjzCC1s6oM0a43JhO6mp6z/"
+    ]
+}
+`

builtin/providers/aws/structure.go

@@ -820,7 +820,7 @@ func flattenResourceRecords(recs []*route53.ResourceRecord, typeStr string) []st
 		if r.Value != nil {
 			s := *r.Value
 			if typeStr == "TXT" || typeStr == "SPF" {
-				s = strings.Replace(s, "\"", "", 2)
+				s = expandTxtEntry(s)
 			}
 			strs = append(strs, s)
 		}
@@ -833,14 +833,71 @@ func expandResourceRecords(recs []interface{}, typeStr string) []*route53.Resour
 	for _, r := range recs {
 		s := r.(string)
 		if typeStr == "TXT" || typeStr == "SPF" {
-			// `flattenResourceRecords` removes quotes.  Add them back.
-			s = fmt.Sprintf("\"%s\"", s)
+			s = flattenTxtEntry(s)
 		}
 		records = append(records, &route53.ResourceRecord{Value: aws.String(s)})
 	}
 	return records
 }
 
+// How 'flattenTxtEntry' and 'expandTxtEntry' work.
+//
+// In the Route 53, TXT entries are written using quoted strings, one per line.
+// Example:
+//     "x=foo"
+//     "bar=12"
+//
+// In Terraform, there are two differences:
+// - We use a list of strings instead of separating strings with newlines.
+// - Within each string, we dont' include the surrounding quotes.
+// Example:
+//     records = ["x=foo", "bar=12"]    # Instead of ["\"x=foo\", \"bar=12\""]
+//
+// When we pull from Route 53, `expandTxtEntry` removes the surrounding quotes;
+// when we push to Route 53, `flattenTxtEntry` adds them back.
+//
+// One complication is that a single TXT entry can have multiple quoted strings.
+// For example, here are two TXT entries, one with two quoted strings and the
+// other with three.
+//     "x=" "foo"
+//     "ba" "r" "=12"
+//
+// DNS clients are expected to merge the quoted strings before interpreting the
+// value.  Since `expandTxtEntry` only removes the quotes at the end we can still
+// (hackily) represent the above configuration in Terraform:
+//      records = ["x=\" \"foo", "ba\" \"r\" \"=12"]
+//
+// The primary reason to use multiple strings for an entry is that DNS (and Route
+// 53) doesn't allow a quoted string to be more than 255 characters long.  If you
+// want a longer TXT entry, you must use multiple quoted strings.
+//
+// It would be nice if this Terraform automatically split strings longer than 255
+// characters.  For example, imagine "xxx..xxx" has 256 "x" characters.
+//      records = ["xxx..xxx"]
+// When pushing to Route 53, this could be converted to:
+//      "xxx..xx" "x"
+//
+// This could also work when the user is already using multiple quoted strings:
+//      records = ["xxx.xxx\" \"yyy..yyy"]
+// When pushing to Route 53, this could be converted to:
+//       "xxx..xx" "xyyy...y" "yy"
+//
+// If you want to add this feature, make sure to follow all the quoting rules in
+// <https://tools.ietf.org/html/rfc1464#section-2>.  If you make a mistake, people
+// might end up relying on that mistake so fixing it would be a breaking change.
+
+func flattenTxtEntry(s string) string {
+	return fmt.Sprintf(`"%s"`, s)
+}
+
+func expandTxtEntry(s string) string {
+	last := len(s) - 1
+	if last != 0 && s[0] == '"' && s[last] == '"' {
+		s = s[1:last]
+	}
+	return s
+}
+
 func expandESClusterConfig(m map[string]interface{}) *elasticsearch.ElasticsearchClusterConfig {
 	config := elasticsearch.ElasticsearchClusterConfig{}
 

builtin/providers/aws/structure_test.go

@@ -822,11 +822,15 @@ func TestFlattenResourceRecords(t *testing.T) {
 	original := []string{
 		`127.0.0.1`,
 		`"abc def"`,
+		`"abc" "def"`,
+		`"abc" ""`,
 	}
 
 	dequoted := []string{
 		`127.0.0.1`,
 		`abc def`,
+		`abc" "def`,
+		`abc" "`,
 	}
 
 	var wrapped []*route53.ResourceRecord = nil