Skip to content

Commit 125e9ef

Browse files
code-ashercode-asher
authored
fix: validate that parameter names are unique (coder#7882)
1 parent fbdbc8a commit 125e9ef

4 files changed

Lines changed: 125 additions & 0 deletions

File tree

‎coderd/util/strings/strings.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
package strings
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
)
7+
8+
// JoinWithConjunction joins a slice of strings with commas except for the last
9+
// two which are joined with "and".
10+
func JoinWithConjunction(s []string) string {
11+
last := len(s) - 1
12+
if last == 0 {
13+
return s[last]
14+
}
15+
return fmt.Sprintf("%s and %s",
16+
strings.Join(s[:last], ", "),
17+
s[last],
18+
)
19+
}

‎coderd/util/strings/strings_test.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
package strings_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/require"
7+
8+
"github.com/coder/coder/coderd/util/strings"
9+
)
10+
11+
func TestJoinWithConjunction(t *testing.T) {
12+
t.Parallel()
13+
require.Equal(t, "foo", strings.JoinWithConjunction([]string{"foo"}))
14+
require.Equal(t, "foo and bar", strings.JoinWithConjunction([]string{"foo", "bar"}))
15+
require.Equal(t, "foo, bar and baz", strings.JoinWithConjunction([]string{"foo", "bar", "baz"}))
16+
}

‎provisioner/terraform/resources.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package terraform
22

33
import (
4+
"fmt"
45
"strings"
56

67
"github.com/awalterschulze/gographviz"
@@ -10,6 +11,8 @@ import (
1011

1112
"github.com/coder/terraform-provider-coder/provider"
1213

14+
"github.com/coder/coder/coderd/util/slice"
15+
stringutil "github.com/coder/coder/coderd/util/strings"
1316
"github.com/coder/coder/codersdk"
1417
"github.com/coder/coder/provisioner"
1518
"github.com/coder/coder/provisionersdk/proto"
@@ -473,6 +476,7 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
473476
}
474477
}
475478

479+
var duplicatedParamNames []string
476480
parameters := make([]*proto.RichParameter, 0)
477481
for _, resource := range orderedRichParametersResources(tfResourcesRichParameters, rawParameterNames) {
478482
var param provider.Parameter
@@ -536,9 +540,31 @@ func ConvertState(modules []*tfjson.StateModule, rawGraph string, rawParameterNa
536540
})
537541
}
538542
}
543+
544+
// Check if this parameter duplicates an existing parameter.
545+
formattedName := fmt.Sprintf("%q", protoParam.Name)
546+
if !slice.Contains(duplicatedParamNames, formattedName) &&
547+
slice.ContainsCompare(parameters, protoParam, func(a, b *proto.RichParameter) bool {
548+
return a.Name == b.Name
549+
}) {
550+
duplicatedParamNames = append(duplicatedParamNames, formattedName)
551+
}
552+
539553
parameters = append(parameters, protoParam)
540554
}
541555

556+
// Enforce that parameters be uniquely named.
557+
if len(duplicatedParamNames) > 0 {
558+
s := ""
559+
if len(duplicatedParamNames) == 1 {
560+
s = "s"
561+
}
562+
return nil, xerrors.Errorf(
563+
"coder_parameter names must be unique but %s appear%s multiple times",
564+
stringutil.JoinWithConjunction(duplicatedParamNames), s,
565+
)
566+
}
567+
542568
// A map is used to ensure we don't have duplicates!
543569
gitAuthProvidersMap := map[string]struct{}{}
544570
for _, tfResources := range tfResourcesByLabel {

‎provisioner/terraform/resources_test.go‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package terraform_test
22

33
import (
44
"encoding/json"
5+
"fmt"
56
"os"
67
"path/filepath"
78
"runtime"
@@ -584,6 +585,69 @@ func TestAppSlugValidation(t *testing.T) {
584585
require.ErrorContains(t, err, "duplicate app slug")
585586
}
586587

588+
func TestParameterValidation(t *testing.T) {
589+
t.Parallel()
590+
591+
// nolint:dogsled
592+
_, filename, _, _ := runtime.Caller(0)
593+
594+
// Load the rich-parameters state file and edit it.
595+
dir := filepath.Join(filepath.Dir(filename), "testdata", "rich-parameters")
596+
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "rich-parameters.tfplan.json"))
597+
require.NoError(t, err)
598+
var tfPlan tfjson.Plan
599+
err = json.Unmarshal(tfPlanRaw, &tfPlan)
600+
require.NoError(t, err)
601+
tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "rich-parameters.tfplan.dot"))
602+
require.NoError(t, err)
603+
604+
// Change all names to be identical.
605+
var names []string
606+
for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
607+
if resource.Type == "coder_parameter" {
608+
resource.AttributeValues["name"] = "identical"
609+
names = append(names, resource.Name)
610+
}
611+
}
612+
613+
state, err := terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
614+
require.Nil(t, state)
615+
require.Error(t, err)
616+
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical\" appears multiple times")
617+
618+
// Make two sets of identical names.
619+
count := 0
620+
names = nil
621+
for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
622+
if resource.Type == "coder_parameter" {
623+
resource.AttributeValues["name"] = fmt.Sprintf("identical-%d", count%2)
624+
names = append(names, resource.Name)
625+
count++
626+
}
627+
}
628+
629+
state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
630+
require.Nil(t, state)
631+
require.Error(t, err)
632+
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\" and \"identical-1\" appear multiple times")
633+
634+
// Once more with three sets.
635+
count = 0
636+
names = nil
637+
for _, resource := range tfPlan.PriorState.Values.RootModule.Resources {
638+
if resource.Type == "coder_parameter" {
639+
resource.AttributeValues["name"] = fmt.Sprintf("identical-%d", count%3)
640+
names = append(names, resource.Name)
641+
count++
642+
}
643+
}
644+
645+
state, err = terraform.ConvertState([]*tfjson.StateModule{tfPlan.PriorState.Values.RootModule}, string(tfPlanGraph), names)
646+
require.Nil(t, state)
647+
require.Error(t, err)
648+
require.ErrorContains(t, err, "coder_parameter names must be unique but \"identical-0\", \"identical-1\" and \"identical-2\" appear multiple times")
649+
}
650+
587651
func TestInstanceTypeAssociation(t *testing.T) {
588652
t.Parallel()
589653
type tc struct {

0 commit comments

Comments
 (0)