Skip to content

Commit ac27cf8

Browse files
coadlercoadler
authored
fix: properly apply metadata when multiple resources share the same id (coder#5443)
1 parent 308a060 commit ac27cf8

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
@@ -68,8 +68,8 @@ type metadataItem struct {
6868
IsNull bool `mapstructure:"is_null"`
6969
}
7070

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

87-
// Indexes Terraform resources by their label and ID.
88-
// The label is what "terraform graph" uses to reference nodes, and the ID
89-
// is used by "coder_metadata" resources to refer to their targets. (The ID
90-
// field is only available when reading a state file, and not when reading a
91-
// plan file.)
87+
// Indexes Terraform resources by their label.
88+
// The label is what "terraform graph" uses to reference nodes.
9289
tfResourceByLabel := map[string]*tfjson.StateResource{}
93-
resourceLabelByID := map[string]string{}
9490
var findTerraformResources func(mod *tfjson.StateModule)
9591
findTerraformResources = func(mod *tfjson.StateModule) {
9692
for _, module := range mod.ChildModules {
@@ -100,14 +96,6 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
10096
label := convertAddressToLabel(resource.Address)
10197
// index by label
10298
tfResourceByLabel[label] = resource
103-
// index by ID, if it exists
104-
id, ok := resource.AttributeValues["id"]
105-
if ok {
106-
idString, ok := id.(string)
107-
if ok {
108-
resourceLabelByID[idString] = label
109-
}
110-
}
11199
}
112100
}
113101
findTerraformResources(module)
@@ -319,58 +307,48 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
319307
if resource.Type != "coder_metadata" {
320308
continue
321309
}
310+
322311
var attrs metadataAttributes
323312
err = mapstructure.Decode(resource.AttributeValues, &attrs)
324313
if err != nil {
325314
return nil, xerrors.Errorf("decode metadata attributes: %w", err)
326315
}
327316

328-
var targetLabel string
329-
// This occurs in a plan, because there is no resource ID.
330-
// We attempt to find the closest node, just so we can hide it from the UI.
331-
if attrs.ResourceID == "" {
332-
resourceLabel := convertAddressToLabel(resource.Address)
317+
resourceLabel := convertAddressToLabel(resource.Address)
333318

334-
var attachedNode *gographviz.Node
335-
for _, node := range graph.Nodes.Lookup {
336-
// The node attributes surround the label with quotes.
337-
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
338-
continue
339-
}
340-
attachedNode = node
341-
break
319+
var attachedNode *gographviz.Node
320+
for _, node := range graph.Nodes.Lookup {
321+
// The node attributes surround the label with quotes.
322+
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
323+
continue
342324
}
343-
if attachedNode == nil {
325+
attachedNode = node
326+
break
327+
}
328+
if attachedNode == nil {
329+
continue
330+
}
331+
var attachedResource *graphResource
332+
for _, resource := range findResourcesInGraph(graph, tfResourceByLabel, attachedNode.Name, 0, false) {
333+
if attachedResource == nil {
334+
// Default to the first resource because we have nothing to compare!
335+
attachedResource = resource
344336
continue
345337
}
346-
var attachedResource *graphResource
347-
for _, resource := range findResourcesInGraph(graph, tfResourceByLabel, attachedNode.Name, 0, false) {
348-
if attachedResource == nil {
349-
// Default to the first resource because we have nothing to compare!
350-
attachedResource = resource
351-
continue
352-
}
353-
if resource.Depth < attachedResource.Depth {
354-
// There's a closer resource!
355-
attachedResource = resource
356-
continue
357-
}
358-
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
359-
attachedResource = resource
360-
continue
361-
}
338+
if resource.Depth < attachedResource.Depth {
339+
// There's a closer resource!
340+
attachedResource = resource
341+
continue
362342
}
363-
if attachedResource == nil {
343+
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
344+
attachedResource = resource
364345
continue
365346
}
366-
targetLabel = attachedResource.Label
367-
}
368-
if targetLabel == "" {
369-
targetLabel = resourceLabelByID[attrs.ResourceID]
370347
}
371-
if targetLabel == "" {
348+
if attachedResource == nil {
372349
continue
373350
}
351+
targetLabel := attachedResource.Label
374352

375353
resourceHidden[targetLabel] = attrs.Hide
376354
resourceIcon[targetLabel] = attrs.Icon
@@ -416,9 +394,11 @@ func ConvertResources(module *tfjson.StateModule, rawGraph string) ([]*proto.Res
416394
}
417395

418396
// convertAddressToLabel returns the Terraform address without the count
419-
// specifier. eg. "module.ec2_dev.ec2_instance.dev[0]" becomes "module.ec2_dev.ec2_instance.dev"
397+
// specifier.
398+
// eg. "module.ec2_dev.ec2_instance.dev[0]" becomes "module.ec2_dev.ec2_instance.dev"
420399
func convertAddressToLabel(address string) string {
421-
return strings.Split(address, "[")[0]
400+
cut, _, _ := strings.Cut(address, "[")
401+
return cut
422402
}
423403

424404
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)