Skip to content

Commit 83781c2

Browse files
coadlercoadler
committed
fix: properly apply metadata when multiple resources share the same id
Fixes: #5176
1 parent 012a9e7 commit 83781c2

18 files changed

+3747
-90
lines changed

‎coderd/templateversions.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1069,12 +1069,12 @@ func (api *API) postTemplateVersionsByOrganization(rw http.ResponseWriter, r *ht
10691069

10701070
// templateVersionResources returns the workspace agent resources associated
10711071
// with a template version. A template can specify more than one resource to be
1072-
// provisioned, each resource can have an agent that dials back to coderd.
1073-
// The agents returned are informative of the template version, and do not
1074-
// return agents associated with any particular workspace.
1072+
// provisioned, each resource can have an agent that dials back to coderd. The
1073+
// agents returned are informative of the template version, and do not return
1074+
// agents associated with any particular workspace.
10751075
func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request) {
1076-
ctx := r.Context()
10771076
var (
1077+
ctx = r.Context()
10781078
templateVersion = httpmw.TemplateVersionParam(r)
10791079
template = httpmw.TemplateParam(r)
10801080
)
@@ -1100,8 +1100,8 @@ func (api *API) templateVersionResources(rw http.ResponseWriter, r *http.Request
11001100
// and not any build logs for a workspace.
11011101
// Eg: Logs returned from 'terraform plan' when uploading a new terraform file.
11021102
func (api *API) templateVersionLogs(rw http.ResponseWriter, r *http.Request) {
1103-
ctx := r.Context()
11041103
var (
1104+
ctx = r.Context()
11051105
templateVersion = httpmw.TemplateVersionParam(r)
11061106
template = httpmw.TemplateParam(r)
11071107
)

‎provisioner/terraform/executor.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"encoding/json"
88
"fmt"
99
"io"
10-
"io/ioutil"
1110
"os"
1211
"os/exec"
1312
"path/filepath"
@@ -270,7 +269,7 @@ func (e *executor) graph(ctx, killCtx context.Context) (string, error) {
270269
return "", ctx.Err()
271270
}
272271

273-
var out bytes.Buffer
272+
var out strings.Builder
274273
cmd := exec.CommandContext(killCtx, e.binaryPath, "graph") // #nosec
275274
cmd.Stdout = &out
276275
cmd.Dir = e.workdir
@@ -289,14 +288,13 @@ func (e *executor) graph(ctx, killCtx context.Context) (string, error) {
289288
return out.String(), nil
290289
}
291290

292-
// revive:disable-next-line:flag-parameter
293291
func (e *executor) apply(
294292
ctx, killCtx context.Context, plan []byte, env []string, logr logSink,
295293
) (*proto.Provision_Response, error) {
296294
e.mut.Lock()
297295
defer e.mut.Unlock()
298296

299-
planFile, err := ioutil.TempFile("", "coder-terrafrom-plan")
297+
planFile, err := os.CreateTemp("", "coder-terrafrom-plan")
300298
if err != nil {
301299
return nil, xerrors.Errorf("create plan file: %w", err)
302300
}

‎provisioner/terraform/resources.go

Lines changed: 34 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,8 @@ type metadataItem struct {
6767
IsNull bool `mapstructure:"is_null"`
6868
}
6969

70-
// ConvertResources consumes Terraform state and a GraphViz representation produced by
71-
// `terraform graph` to produce resources consumable by Coder.
70+
// ConvertResources consumes Terraform state and a GraphViz representation
71+
// produced by `terraform graph` to produce resources consumable by Coder.
7272
// nolint:gocyclo
7373
func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Resource, error) {
7474
parsedGraph, err := gographviz.ParseString(rawGraph)
@@ -83,13 +83,9 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
8383
resources := make([]*proto.Resource, 0)
8484
resourceAgents := map[string][]*proto.Agent{}
8585

86-
// Indexes Terraform resources by their label and ID.
87-
// The label is what "terraform graph" uses to reference nodes, and the ID
88-
// is used by "coder_metadata" resources to refer to their targets. (The ID
89-
// field is only available when reading a state file, and not when reading a
90-
// plan file.)
86+
// Indexes Terraform resources by their label.
87+
// The label is what "terraform graph" uses to reference nodes.
9188
tfResourceByLabel := map[string]*tfjson.StateResource{}
92-
resourceLabelByID := map[string]string{}
9389
var findTerraformResources func(mod *tfjson.StateModule)
9490
findTerraformResources = func(mod *tfjson.StateModule) {
9591
for _, module := range mod.ChildModules {
@@ -99,14 +95,6 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
9995
label := convertAddressToLabel(resource.Address)
10096
// index by label
10197
tfResourceByLabel[label] = resource
102-
// index by ID, if it exists
103-
id, ok := resource.AttributeValues["id"]
104-
if ok {
105-
idString, ok := id.(string)
106-
if ok {
107-
resourceLabelByID[idString] = label
108-
}
109-
}
11098
}
11199
}
112100
findTerraformResources(module)
@@ -310,58 +298,48 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
310298
if resource.Type != "coder_metadata" {
311299
continue
312300
}
301+
313302
var attrs metadataAttributes
314303
err = mapstructure.Decode(resource.AttributeValues, &attrs)
315304
if err != nil {
316305
return nil, xerrors.Errorf("decode metadata attributes: %w", err)
317306
}
318307

319-
var targetLabel string
320-
// This occurs in a plan, because there is no resource ID.
321-
// We attempt to find the closest node, just so we can hide it from the UI.
322-
if attrs.ResourceID == "" {
323-
resourceLabel := convertAddressToLabel(resource.Address)
308+
resourceLabel := convertAddressToLabel(resource.Address)
324309

325-
var attachedNode *gographviz.Node
326-
for _, node := range graph.Nodes.Lookup {
327-
// The node attributes surround the label with quotes.
328-
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
329-
continue
330-
}
331-
attachedNode = node
332-
break
310+
var attachedNode *gographviz.Node
311+
for _, node := range graph.Nodes.Lookup {
312+
// The node attributes surround the label with quotes.
313+
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
314+
continue
333315
}
334-
if attachedNode == nil {
316+
attachedNode = node
317+
break
318+
}
319+
if attachedNode == nil {
320+
continue
321+
}
322+
var attachedResource *graphResource
323+
for _, resource := range findResourcesInGraph(graph, tfResourceByLabel, attachedNode.Name, 0, false) {
324+
if attachedResource == nil {
325+
// Default to the first resource because we have nothing to compare!
326+
attachedResource = resource
335327
continue
336328
}
337-
var attachedResource *graphResource
338-
for _, resource := range findResourcesInGraph(graph, tfResourceByLabel, attachedNode.Name, 0, false) {
339-
if attachedResource == nil {
340-
// Default to the first resource because we have nothing to compare!
341-
attachedResource = resource
342-
continue
343-
}
344-
if resource.Depth < attachedResource.Depth {
345-
// There's a closer resource!
346-
attachedResource = resource
347-
continue
348-
}
349-
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
350-
attachedResource = resource
351-
continue
352-
}
329+
if resource.Depth < attachedResource.Depth {
330+
// There's a closer resource!
331+
attachedResource = resource
332+
continue
353333
}
354-
if attachedResource == nil {
334+
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
335+
attachedResource = resource
355336
continue
356337
}
357-
targetLabel = attachedResource.Label
358-
}
359-
if targetLabel == "" {
360-
targetLabel = resourceLabelByID[attrs.ResourceID]
361338
}
362-
if targetLabel == "" {
339+
if attachedResource == nil {
363340
continue
364341
}
342+
targetLabel := attachedResource.Label
365343

366344
resourceHidden[targetLabel] = attrs.Hide
367345
resourceIcon[targetLabel] = attrs.Icon
@@ -407,9 +385,11 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
407385
}
408386

409387
// convertAddressToLabel returns the Terraform address without the count
410-
// specifier. eg. "module.ec2_dev.ec2_instance.dev[0]" becomes "module.ec2_dev.ec2_instance.dev"
388+
// specifier.
389+
// eg. "module.ec2_dev.ec2_instance.dev[0]" becomes "module.ec2_dev.ec2_instance.dev"
411390
func convertAddressToLabel(address string) string {
412-
return strings.Split(address, "[")[0]
391+
cut, _, _ := strings.Cut(address, "[")
392+
return cut
413393
}
414394

415395
type graphResource struct {

‎provisioner/terraform/resources_test.go

Lines changed: 71 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,14 @@ import (
88
"sort"
99
"testing"
1010

11+
protobuf "github.com/golang/protobuf/proto"
1112
tfjson "github.com/hashicorp/terraform-json"
1213
"github.com/stretchr/testify/require"
14+
"golang.org/x/exp/slices"
1315

1416
"github.com/coder/coder/cryptorand"
1517
"github.com/coder/coder/provisioner/terraform"
1618
"github.com/coder/coder/provisionersdk/proto"
17-
18-
protobuf "github.com/golang/protobuf/proto"
1919
)
2020

2121
func TestConvertResources(t *testing.T) {
@@ -165,6 +165,53 @@ func TestConvertResources(t *testing.T) {
165165
Sensitive: true,
166166
}},
167167
}},
168+
// Tests that resources with the same id correctly get metadata applied
169+
// to them.
170+
"kubernetes-metadata": {{
171+
Name: "coder_workspace",
172+
Type: "kubernetes_service_account",
173+
}, {
174+
Name: "coder_workspace",
175+
Type: "kubernetes_config_map",
176+
}, {
177+
Name: "coder_workspace",
178+
Type: "kubernetes_role",
179+
}, {
180+
Name: "coder_workspace",
181+
Type: "kubernetes_role_binding",
182+
}, {
183+
Name: "coder_workspace",
184+
Type: "kubernetes_secret",
185+
}, {
186+
Name: "main",
187+
Type: "kubernetes_pod",
188+
Metadata: []*proto.Resource_Metadata{{
189+
Key: "cpu",
190+
Value: "1",
191+
}, {
192+
Key: "memory",
193+
Value: "1Gi",
194+
}, {
195+
Key: "gpu",
196+
Value: "1",
197+
}},
198+
Agents: []*proto.Agent{{
199+
Name: "main",
200+
OperatingSystem: "linux",
201+
Architecture: "amd64",
202+
StartupScript: " #!/bin/bash\n # home folder can be empty, so copying default bash settings\n if [ ! -f ~/.profile ]; then\n cp /etc/skel/.profile $HOME\n fi\n if [ ! -f ~/.bashrc ]; then\n cp /etc/skel/.bashrc $HOME\n fi\n # install and start code-server\n curl -fsSL https://code-server.dev/install.sh | sh | tee code-server-install.log\n code-server --auth none --port 13337 | tee code-server-install.log &\n",
203+
Apps: []*proto.App{
204+
{
205+
Icon: "/icon/code.svg",
206+
Slug: "code-server",
207+
DisplayName: "code-server",
208+
Url: "http://localhost:13337?folder=/home/coder",
209+
},
210+
},
211+
Auth: &proto.Agent_Token{},
212+
ConnectionTimeoutSeconds: 120,
213+
}},
214+
}},
168215
} {
169216
folderName := folderName
170217
expected := expected
@@ -210,6 +257,17 @@ func TestConvertResources(t *testing.T) {
210257
err = json.Unmarshal(data, &resourcesMap)
211258
require.NoError(t, err)
212259

260+
slices.SortFunc(expectedNoMetadataMap, func(a, b map[string]interface{}) bool {
261+
//nolint:forcetypeassert
262+
return a["name"].(string)+a["type"].(string) <
263+
b["name"].(string)+b["type"].(string)
264+
})
265+
slices.SortFunc(resourcesMap, func(a, b map[string]interface{}) bool {
266+
//nolint:forcetypeassert
267+
return a["name"].(string)+a["type"].(string) <
268+
b["name"].(string)+b["type"].(string)
269+
})
270+
213271
require.Equal(t, expectedNoMetadataMap, resourcesMap)
214272
})
215273

@@ -251,6 +309,17 @@ func TestConvertResources(t *testing.T) {
251309
err = json.Unmarshal(data, &resourcesMap)
252310
require.NoError(t, err)
253311

312+
slices.SortFunc(expectedMap, func(a, b map[string]interface{}) bool {
313+
//nolint:forcetypeassert
314+
return a["name"].(string)+a["type"].(string) <
315+
b["name"].(string)+b["type"].(string)
316+
})
317+
slices.SortFunc(resourcesMap, func(a, b map[string]interface{}) bool {
318+
//nolint:forcetypeassert
319+
return a["name"].(string)+a["type"].(string) <
320+
b["name"].(string)+b["type"].(string)
321+
})
322+
254323
require.Equal(t, expectedMap, resourcesMap)
255324
})
256325
})

‎provisioner/terraform/testdata/calling-module/calling-module.tfplan.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎provisioner/terraform/testdata/calling-module/calling-module.tfstate.json

Lines changed: 5 additions & 5 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎provisioner/terraform/testdata/chaining-resources/chaining-resources.tfplan.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)