fix: validate that parameter names are unique (coder#7882)

code-asher · web-flow · commit 125e9ef00ec1 · 2023-06-07T09:44:50.000-08:00
diff --git a/coderd/util/strings/strings.go b/coderd/util/strings/strings.go
@@ -0,0 +1,19 @@
+package strings
+
+import (
+	"fmt"
+	"strings"
+)
+
+// JoinWithConjunction joins a slice of strings with commas except for the last
+// two which are joined with "and".
+func JoinWithConjunction(s []string) string {
+	last := len(s) - 1
+	if last == 0 {
+		return s[last]
+	}
+	return fmt.Sprintf("%s and %s",
+		strings.Join(s[:last], ", "),
+		s[last],
+	)
+}
diff --git a/coderd/util/strings/strings_test.go b/coderd/util/strings/strings_test.go
@@ -0,0 +1,16 @@
+package strings_test
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/coder/coder/coderd/util/strings"
+)
+
+func TestJoinWithConjunction(t *testing.T) {
+	t.Parallel()
+	require.Equal(t, "foo", strings.JoinWithConjunction([]string{"foo"}))
+	require.Equal(t, "foo and bar", strings.JoinWithConjunction([]string{"foo", "bar"}))
+	require.Equal(t, "foo, bar and baz", strings.JoinWithConjunction([]string{"foo", "bar", "baz"}))
+}
diff --git a/provisioner/terraform/resources.go b/provisioner/terraform/resources.go
@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"strings"
 
 	"github.com/awalterschulze/gographviz"
@@ -10,6 +11,8 @@ import (
 
 	"github.com/coder/terraform-provider-coder/provider"
 
+	"github.com/coder/coder/coderd/util/slice"
+	stringutil "github.com/coder/coder/coderd/util/strings"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/provisioner"
 	"github.com/coder/coder/provisionersdk/proto"
@@ -473,6 +476,7 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
 		}
 	}
 
+	var duplicatedParamNames []string
 	parameters := make([]*proto.RichParameter, 0)
 	for _, resource := range orderedRichParametersResources(tfResourcesRichParameters, rawParameterNames) {
 		var param provider.Parameter
@@ -536,9 +540,31 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
 				})
 			}
 		}
+
+		// Check if this parameter duplicates an existing parameter.
+		formattedName := fmt.Sprintf("%q", protoParam.Name)
+		if !slice.Contains(duplicatedParamNames, formattedName) &&
+			slice.ContainsCompare(parameters, protoParam, func(a, b *proto.RichParameter) bool {
+				return a.Name == b.Name
+			}) {
+			duplicatedParamNames = append(duplicatedParamNames, formattedName)
+		}
+
 		parameters = append(parameters, protoParam)
 	}
 
+	// Enforce that parameters be uniquely named.
+	if len(duplicatedParamNames) > 0 {
+		s := ""
+		if len(duplicatedParamNames) == 1 {
+			s = "s"
+		}
+		return nil, xerrors.Errorf(
+			"coder_parameter names must be unique but %s appear%s multiple times",
+			stringutil.JoinWithConjunction(duplicatedParamNames), s,
+		)
+	}
+
 	// A map is used to ensure we don't have duplicates!
 	gitAuthProvidersMap := map[string]struct{}{}
 	for _, tfResources := range tfResourcesByLabel {
diff --git a/provisioner/terraform/resources_test.go b/provisioner/terraform/resources_test.go
@@ -2,6 +2,7 @@ package terraform_test
 
 import (
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -584,6 +585,69 @@ func TestAppSlugValidation(t *testing.T) {
 	require.ErrorContains(t, err, "duplicate app slug")
 }
 
+func TestParameterValidation(t *testing.T) {
+	t.Parallel()
+
+	// nolint:dogsled
+	_, filename, _, _ := runtime.Caller(0)
+
+	// Load the rich-parameters state file and edit it.
+	dir := filepath.Join(filepath.Dir(filename), "testdata", "rich-parameters")
+	tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "rich-parameters.tfplan.json"))
+	require.NoError(t, err)
+	var tfPlan tfjson.Plan
+	err = json.Unmarshal(tfPlanRaw, &tfPlan)
+	require.NoError(t, err)
+	tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "rich-parameters.tfplan.dot"))
+	require.NoError(t, err)
+
+	// Change all names to be identical.
+	var names []string
+	for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
+		if resource.Type == "coder_parameter" {
+			resource.AttributeValues["name"] = "identical"
+			names = append(names, resource.Name)
+		}
+	}
+
+	state, err := terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
+	require.Nil(t, state)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical\" appears multiple times")
+
+	// Make two sets of identical names.
+	count := 0
+	names = nil
+	for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
+		if resource.Type == "coder_parameter" {
+			resource.AttributeValues["name"] = fmt.Sprintf("identical-%d", count%2)
+			names = append(names, resource.Name)
+			count++
+		}
+	}
+
+	state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
+	require.Nil(t, state)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\" and \"identical-1\" appear multiple times")
+
+	// Once more with three sets.
+	count = 0
+	names = nil
+	for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
+		if resource.Type == "coder_parameter" {
+			resource.AttributeValues["name"] = fmt.Sprintf("identical-%d", count%3)
+			names = append(names, resource.Name)
+			count++
+		}
+	}
+
+	state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
+	require.Nil(t, state)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\", \"identical-1\" and \"identical-2\" appear multiple times")
+}
+
 func TestInstanceTypeAssociation(t *testing.T) {
 	t.Parallel()
 	type tc struct {
