Added better error when the user gives us a binary plan file (#2845)

GitOrigin-RevId: e4fe4c5010c5d4708f4d98a2659bc078710907a7

Author: dylanratcliffe

tfutils/plan_mapper.go

@@ -107,14 +107,18 @@ func (r *PlanMappingResult) numStatus(status MapStatus) int {
 }
 
 func MappedItemDiffsFromPlanFile(ctx context.Context, fileName string, scope string, lf log.Fields) (*PlanMappingResult, error) {
-	// read results from `terraform show -json ${tfplan file}`
-	planJSON, err := os.ReadFile(fileName)
+	planData, err := os.ReadFile(fileName)
 	if err != nil {
 		log.WithContext(ctx).WithError(err).WithFields(lf).Error("Failed to read terraform plan")
 		return nil, err
 	}
 
-	return MappedItemDiffsFromPlan(ctx, planJSON, fileName, scope, lf)
+	// Check if this is a JSON plan file
+	if !isJSONPlanFile(planData) {
+		return nil, fmt.Errorf("plan file '%s' appears to be in binary format, but Overmind only supports JSON plan files.\n\nTo fix this, convert your binary plan to JSON format:\n  1. Using OpenTofu: tofu show -json %s > plan.json\n  2. Using Terraform: terraform show -json %s > plan.json\n  3. Then run: overmind changes submit-plan plan.json", fileName, fileName, fileName)
+	}
+
+	return MappedItemDiffsFromPlan(ctx, planData, fileName, scope, lf)
 }
 
 type TfMapData struct {
@@ -343,6 +347,20 @@ func mapResourceToQuery(itemDiff *sdp.ItemDiff, terraformResource *Resource, map
 	}
 }
 
+// isJSONPlanFile checks if the supplied bytes are valid JSON that could be a plan file.
+// This is used to determine if we need to convert a binary plan or if it's already JSON.
+func isJSONPlanFile(bytes []byte) bool {
+	var jsonValue interface{}
+
+	err := json.Unmarshal(bytes, &jsonValue)
+	if err != nil {
+		return false
+	}
+
+	// If it's valid JSON, we can try to parse it as a plan
+	return true
+}
+
 // Checks if the supplied JSON bytes are a state file. It's a common  mistake to
 // pass a state file to Overmind rather than a plan file since the commands to
 // create them are similar

tfutils/plan_mapper_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 	"strconv"
 	"strings"
 	"testing"
@@ -748,3 +749,120 @@ func terraformDig(srcMapPtr interface{}, path string) interface{} {
 		return terraformDig(&valueMap, strings.Join(parts[1:], "."))
 	}
 }
+
+func TestIsJSONPlanFile(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    []byte
+		expected bool
+	}{
+		{
+			name:     "valid JSON object",
+			input:    []byte(`{"format_version": "1.0", "terraform_version": "1.0.0"}`),
+			expected: true,
+		},
+		{
+			name:     "valid JSON array",
+			input:    []byte(`[{"key": "value"}]`),
+			expected: true,
+		},
+		{
+			name:     "valid JSON string",
+			input:    []byte(`"hello world"`),
+			expected: true,
+		},
+		{
+			name:     "valid JSON number",
+			input:    []byte(`42`),
+			expected: true,
+		},
+		{
+			name:     "valid JSON boolean",
+			input:    []byte(`true`),
+			expected: true,
+		},
+		{
+			name:     "valid JSON null",
+			input:    []byte(`null`),
+			expected: true,
+		},
+		{
+			name:     "invalid JSON - binary data",
+			input:    []byte{0x50, 0x4B, 0x03, 0x04}, // ZIP file header
+			expected: false,
+		},
+		{
+			name:     "invalid JSON - incomplete",
+			input:    []byte(`{"incomplete":`),
+			expected: false,
+		},
+		{
+			name:     "empty input",
+			input:    []byte(``),
+			expected: false,
+		},
+		{
+			name:     "non-JSON text",
+			input:    []byte(`this is not json`),
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := isJSONPlanFile(tt.input)
+			require.Equal(t, tt.expected, result, "isJSONPlanFile(%q) = %v, want %v", string(tt.input), result, tt.expected)
+		})
+	}
+}
+
+func TestMappedItemDiffsFromPlanFileWithJSON(t *testing.T) {
+	// Test that existing JSON plan files still work
+	result, err := MappedItemDiffsFromPlanFile(context.Background(), "testdata/plan.json", "test-scope", log.Fields{})
+
+	// This should work if the test data exists and is valid JSON
+	if err != nil {
+		// If the test data doesn't exist or is invalid, that's okay for this test
+		// We're mainly testing that the JSON path is taken
+		t.Logf("Expected error for test data: %v", err)
+	} else {
+		require.NotNil(t, result)
+	}
+}
+
+func TestMappedItemDiffsFromPlanFileWithRealBinaryPlan(t *testing.T) {
+	// Test with the real binary plan file we created
+	binaryPlanPath := "testdata/binary-plan.tfplan"
+
+	// Check if the test file exists
+	if _, err := os.Stat(binaryPlanPath); os.IsNotExist(err) {
+		t.Skip("Skipping test: real binary plan file not found. Run 'make test-binary-plan' to generate it.")
+	}
+
+	// Test that the binary version is detected correctly and returns a clear error
+	t.Run("Binary_plan_detection", func(t *testing.T) {
+		// Read the binary plan file
+		binaryData, err := os.ReadFile(binaryPlanPath)
+		require.NoError(t, err)
+
+		// Verify it's detected as binary (not JSON)
+		isJSON := isJSONPlanFile(binaryData)
+		require.False(t, isJSON, "Binary plan should not be detected as JSON")
+
+		t.Logf("Binary plan correctly detected as non-JSON (size: %d bytes)", len(binaryData))
+	})
+
+	// Test that the binary plan returns a clear error message
+	t.Run("Binary_plan_error", func(t *testing.T) {
+		_, err := MappedItemDiffsFromPlanFile(context.Background(), binaryPlanPath, "test-scope", log.Fields{})
+
+		// We expect this to fail with a clear error message
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "appears to be in binary format, but Overmind only supports JSON plan files")
+		require.Contains(t, err.Error(), "tofu show -json")
+		require.Contains(t, err.Error(), "terraform show -json")
+		require.Contains(t, err.Error(), "overmind changes submit-plan plan.json")
+
+		t.Logf("Binary plan correctly rejected with clear error message")
+	})
+}