CHANGELOG.md

@@ -1,3 +1,11 @@
+## 1.0.4 (August 04, 2021)
+
+
+BUG FIXES:
+
+* backend/consul: Fix a bug where the state value may be too large for consul to accept ([#28838](https://github.com/hashicorp/terraform/issues/28838))
+* cli: Fixed a crashing bug with some edge-cases when reporting syntax errors that happen to be reported at the position of a newline. ([#29048](https://github.com/hashicorp/terraform/issues/29048))
+
 ## 1.0.3 (July 21, 2021)
 
 ENHANCEMENTS

internal/backend/remote-state/consul/client.go

@@ -198,72 +198,92 @@ func (c *RemoteClient) Put(data []byte) error {
 		verb = consulapi.KVSet
 	}
 
-	// If the payload is too large we first write the chunks and replace it
-	// 524288 is the default value, we just hope the user did not set a smaller
-	// one but there is really no reason for them to do so, if they changed it
-	// it is certainly to set a larger value.
-	limit := 524288
-	if len(payload) > limit {
-		md5 := md5.Sum(data)
-		chunks := split(payload, limit)
-		chunkPaths := make([]string, 0)
-
-		// First we write the new chunks
-		for i, p := range chunks {
-			path := strings.TrimRight(c.Path, "/") + fmt.Sprintf("/tfstate.%x/%d", md5, i)
-			chunkPaths = append(chunkPaths, path)
-			_, err := kv.Put(&consulapi.KVPair{
-				Key:   path,
-				Value: p,
-			}, nil)
-
-			if err != nil {
-				return err
-			}
+	// The payload may be too large to store in a single KV entry in Consul. We
+	// could try to determine whether it will fit or not before sending the
+	// request but since we are using the Transaction API and not the KV API,
+	// it grows by about a 1/3 when it is base64 encoded plus the overhead of
+	// the fields specific to the Transaction API.
+	// Rather than trying to calculate the overhead (which could change from
+	// one version of Consul to another, and between Consul Community Edition
+	// and Consul Enterprise), we try to send the whole state in one request, if
+	// it fails because it is too big we then split it in chunks and send each
+	// chunk separately.
+	// When splitting in chunks, we make each chunk 524288 bits, which is the
+	// default max size for raft. If the user changed it, we still may send
+	// chunks too big and fail but this is not a setting that should be fiddled
+	// with anyway.
+
+	store := func(payload []byte) error {
+		// KV.Put doesn't return the new index, so we use a single operation
+		// transaction to get the new index with a single request.
+		txOps := consulapi.KVTxnOps{
+			&consulapi.KVTxnOp{
+				Verb:  verb,
+				Key:   c.Path,
+				Value: payload,
+				Index: c.modifyIndex,
+			},
 		}
 
-		// We update the link to point to the new chunks
-		payload, err = json.Marshal(map[string]interface{}{
-			"current-hash": fmt.Sprintf("%x", md5),
-			"chunks":       chunkPaths,
-		})
+		ok, resp, _, err := kv.Txn(txOps, nil)
 		if err != nil {
 			return err
 		}
-	}
+		// transaction was rolled back
+		if !ok {
+			return fmt.Errorf("consul CAS failed with transaction errors: %v", resp.Errors)
+		}
 
-	var txOps consulapi.KVTxnOps
-	// KV.Put doesn't return the new index, so we use a single operation
-	// transaction to get the new index with a single request.
-	txOps = consulapi.KVTxnOps{
-		&consulapi.KVTxnOp{
-			Verb:  verb,
-			Key:   c.Path,
-			Value: payload,
-			Index: c.modifyIndex,
-		},
+		if len(resp.Results) != 1 {
+			// this probably shouldn't happen
+			return fmt.Errorf("expected on 1 response value, got: %d", len(resp.Results))
+		}
+
+		c.modifyIndex = resp.Results[0].ModifyIndex
+
+		// We remove all the old chunks
+		cleanupOldChunks()
+
+		return nil
 	}
 
-	ok, resp, _, err := kv.Txn(txOps, nil)
-	if err != nil {
+	if err = store(payload); err == nil {
+		// The payload was small enough to be stored
+		return nil
+	} else if !strings.Contains(err.Error(), "too large") {
+		// We failed for some other reason, report this to the user
 		return err
 	}
-	// transaction was rolled back
-	if !ok {
-		return fmt.Errorf("consul CAS failed with transaction errors: %v", resp.Errors)
-	}
 
-	if len(resp.Results) != 1 {
-		// this probably shouldn't happen
-		return fmt.Errorf("expected on 1 response value, got: %d", len(resp.Results))
-	}
+	// The payload was too large so we split it in multiple chunks
 
-	c.modifyIndex = resp.Results[0].ModifyIndex
+	md5 := md5.Sum(data)
+	chunks := split(payload, 524288)
+	chunkPaths := make([]string, 0)
 
-	// We remove all the old chunks
-	cleanupOldChunks()
+	// First we write the new chunks
+	for i, p := range chunks {
+		path := strings.TrimRight(c.Path, "/") + fmt.Sprintf("/tfstate.%x/%d", md5, i)
+		chunkPaths = append(chunkPaths, path)
+		_, err := kv.Put(&consulapi.KVPair{
+			Key:   path,
+			Value: p,
+		}, nil)
 
-	return nil
+		if err != nil {
+			return err
+		}
+	}
+
+	// Then we update the link to point to the new chunks
+	payload, err = json.Marshal(map[string]interface{}{
+		"current-hash": fmt.Sprintf("%x", md5),
+		"chunks":       chunkPaths,
+	})
+	if err != nil {
+		return err
+	}
+	return store(payload)
 }
 
 func (c *RemoteClient) Delete() error {

internal/backend/remote-state/consul/client_test.go

@@ -136,11 +136,6 @@ func TestConsul_largeState(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		// md5 := md5.Sum(payload)
-		// if !bytes.Equal(md5[:], remote.MD5) {
-		// 	t.Fatal("the md5 sums do not match")
-		// }
-
 		if !bytes.Equal(payload, remote.Data) {
 			t.Fatal("the data do not match")
 		}
@@ -161,6 +156,19 @@ func TestConsul_largeState(t *testing.T) {
 		},
 	)
 
+	// This payload is just short enough to be stored but will be bigger when
+	// going through the Transaction API as it will be base64 encoded
+	testPayload(
+		t,
+		map[string]string{
+			"foo": strings.Repeat("a", 524288-10),
+		},
+		[]string{
+			"tf-unit/test-large-state",
+			"tf-unit/test-large-state/tfstate.4f407ace136a86521fd0d366972fe5c7/0",
+		},
+	)
+
 	// We try to replace the payload with a small one, the old chunks should be removed
 	testPayload(
 		t,

internal/command/format/diagnostic.go

@@ -250,6 +250,21 @@ func appendSourceSnippets(buf *bytes.Buffer, diag *viewsjson.Diagnostic, color *
 			}
 		}
 
+		// If either start or end is out of range for the code buffer then
+		// we'll cap them at the bounds just to avoid a panic, although
+		// this would happen only if there's a bug in the code generating
+		// the snippet objects.
+		if start < 0 {
+			start = 0
+		} else if start > len(code) {
+			start = len(code)
+		}
+		if end < 0 {
+			end = 0
+		} else if end > len(code) {
+			end = len(code)
+		}
+
 		before, highlight, after := code[0:start], code[start:end], code[end:]
 		code = fmt.Sprintf(color.Color("%s[underline]%s[reset]%s"), before, highlight, after)
 

internal/command/views/json/diagnostic.go

@@ -220,12 +220,23 @@ func NewDiagnostic(diag tfdiags.Diagnostic, sources map[string][]byte) *Diagnost
 			// to the code snippet string.
 			start := highlightRange.Start.Byte - codeStartByte
 			end := start + (highlightRange.End.Byte - highlightRange.Start.Byte)
-			if start > len(codeStr) {
+
+			// We can end up with some quirky results here in edge cases like
+			// when a source range starts or ends at a newline character,
+			// so we'll cap the results at the bounds of the highlight range
+			// so that consumers of this data don't need to contend with
+			// out-of-bounds errors themselves.
+			if start < 0 {
+				start = 0
+			} else if start > len(codeStr) {
 				start = len(codeStr)
 			}
-			if end > len(codeStr) {
+			if end < 0 {
+				end = 0
+			} else if end > len(codeStr) {
 				end = len(codeStr)
 			}
+
 			diagnostic.Snippet.HighlightStartOffset = start
 			diagnostic.Snippet.HighlightEndOffset = end
 

internal/command/views/json/diagnostic_test.go

@@ -28,7 +28,8 @@ func TestNewDiagnostic(t *testing.T) {
   }
 }
 `),
-		"short.tf": []byte("bad source code"),
+		"short.tf":       []byte("bad source code"),
+		"odd-comment.tf": []byte("foo\n\n#\n"),
 		"values.tf": []byte(`[
   var.a,
   var.b,
@@ -284,6 +285,51 @@ func TestNewDiagnostic(t *testing.T) {
 				},
 			},
 		},
+		"error whose range starts at a newline": {
+			&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid newline",
+				Detail:   "How awkward!",
+				Subject: &hcl.Range{
+					Filename: "odd-comment.tf",
+					Start:    hcl.Pos{Line: 2, Column: 5, Byte: 4},
+					End:      hcl.Pos{Line: 3, Column: 1, Byte: 6},
+				},
+			},
+			&Diagnostic{
+				Severity: "error",
+				Summary:  "Invalid newline",
+				Detail:   "How awkward!",
+				Range: &DiagnosticRange{
+					Filename: "odd-comment.tf",
+					Start: Pos{
+						Line:   2,
+						Column: 5,
+						Byte:   4,
+					},
+					End: Pos{
+						Line:   3,
+						Column: 1,
+						Byte:   6,
+					},
+				},
+				Snippet: &DiagnosticSnippet{
+					Code:      `#`,
+					StartLine: 2,
+					Values:    []DiagnosticExpressionValue{},
+
+					// Due to the range starting at a newline on a blank
+					// line, we end up stripping off the initial newline
+					// to produce only a one-line snippet. That would
+					// therefore cause the start offset to naturally be
+					// -1, just before the Code we returned, but then we
+					// force it to zero so that the result will still be
+					// in range for a byte-oriented slice of Code.
+					HighlightStartOffset: 0,
+					HighlightEndOffset:   1,
+				},
+			},
+		},
 		"error with source code subject and known expression": {
 			&hcl.Diagnostic{
 				Severity: hcl.DiagError,

internal/command/views/json/testdata/diagnostic/error-whose-range-starts-at-a-newline.json

@@ -0,0 +1,26 @@
+{
+  "severity": "error",
+  "summary": "Invalid newline",
+  "detail": "How awkward!",
+  "range": {
+    "filename": "odd-comment.tf",
+    "start": {
+      "line": 2,
+      "column": 5,
+      "byte": 4
+    },
+    "end": {
+      "line": 3,
+      "column": 1,
+      "byte": 6
+    }
+  },
+  "snippet": {
+    "context": null,
+    "code": "#",
+    "start_line": 2,
+    "highlight_start_offset": 0,
+    "highlight_end_offset": 1,
+    "values": []
+  }
+}

internal/configs/configschema/path.go

@@ -7,13 +7,19 @@ import (
 // AttributeByPath looks up the Attribute schema which corresponds to the given
 // cty.Path. A nil value is returned if the given path does not correspond to a
 // specific attribute.
-// TODO: this will need to be updated for nested attributes
 func (b *Block) AttributeByPath(path cty.Path) *Attribute {
 	block := b
-	for _, step := range path {
+	for i, step := range path {
 		switch step := step.(type) {
 		case cty.GetAttrStep:
 			if attr := block.Attributes[step.Name]; attr != nil {
+				// If the Attribute is defined with a NestedType and there's
+				// more to the path, descend into the NestedType
+				if attr.NestedType != nil && i < len(path)-1 {
+					return attr.NestedType.AttributeByPath(path[i+1:])
+				} else if i < len(path)-1 { // There's more to the path, but not more to this Attribute.
+					return nil
+				}
 				return attr
 			}
 
@@ -27,3 +33,23 @@ func (b *Block) AttributeByPath(path cty.Path) *Attribute {
 	}
 	return nil
 }
+
+// AttributeByPath recurses through a NestedType to look up the Attribute scheme
+// which corresponds to the given cty.Path. A nil value is returned if the given
+// path does not correspond to a specific attribute.
+func (o *Object) AttributeByPath(path cty.Path) *Attribute {
+	for i, step := range path {
+		switch step := step.(type) {
+		case cty.GetAttrStep:
+			if attr := o.Attributes[step.Name]; attr != nil {
+				if attr.NestedType != nil && i < len(path)-1 {
+					return attr.NestedType.AttributeByPath(path[i+1:])
+				} else if i < len(path)-1 { // There's more to the path, but not more to this Attribute.
+					return nil
+				}
+				return attr
+			}
+		}
+	}
+	return nil
+}