Skip to content

fix: use resource_id directly for coder_metadata association #18300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
+388 −42
Open

fix: use resource_id directly for coder_metadata association #18300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9d60f47
fix: respect resource_id for coder_metadata
sreya Jun 10, 2025
1232eaa
cache id
sreya Jun 10, 2025
6182af4
update tests
sreya Jun 12, 2025
49a3a2b
fix tests
sreya Jun 12, 2025
f1dabf7
make fmt
sreya Jun 12, 2025
cfb02c7
remove comment
sreya Jun 12, 2025
3d92225
handle duplicate TF IDs
sreya Jun 12, 2025
a2afc35
undo deleted file
sreya Jun 12, 2025
7e0c0a2
ci: fix linter errors
ThomasK33 Aug 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli/vpndaemon_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
"github.com/coder/serpent"
)

func (r *RootCmd) vpnDaemonRun() *serpent.Command {
func (*RootCmd) vpnDaemonRun() *serpent.Command {
var (
rpcReadFD int64
rpcWriteFD int64
Expand Down
91 changes: 65 additions & 26 deletions provisioner/terraform/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
// The label is what "terraform graph" uses to reference nodes.
tfResourcesByLabel := map[string]map[string]*tfjson.StateResource{}

// Map resource IDs to labels for efficient lookup when processing metadata
// Multiple resources can have the same ID, so we store a slice of labels
labelsByResourceID := map[string][]string{}

// Extra array to preserve the order of rich parameters.
tfResourcesRichParameters := make([]*tfjson.StateResource, 0)
tfResourcesPresets := make([]*tfjson.StateResource, 0)
Expand All @@ -233,6 +237,13 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
tfResourcesByLabel[label] = map[string]*tfjson.StateResource{}
}
tfResourcesByLabel[label][resource.Address] = resource

// Build the ID to labels map - multiple resources can have the same ID
if idAttr, hasID := resource.AttributeValues["id"]; hasID {
if idStr, ok := idAttr.(string); ok && idStr != "" {
labelsByResourceID[idStr] = append(labelsByResourceID[idStr], label)
}
}
}
}
for _, module := range modules {
Expand Down Expand Up @@ -684,41 +695,69 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
if err != nil {
return nil, xerrors.Errorf("decode metadata attributes: %w", err)
}
resourceLabel := convertAddressToLabel(resource.Address)

var attachedNode *gographviz.Node
for _, node := range graph.Nodes.Lookup {
// The node attributes surround the label with quotes.
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
continue
var targetLabel string

// First, check if ResourceID is provided and try to find the resource by ID
if attrs.ResourceID != "" {
// Look for a resource with matching ID
foundLabels := labelsByResourceID[attrs.ResourceID]
switch len(foundLabels) {
case 0:
// If we couldn't find by ID, fall back to graph traversal
logger.Warn(ctx, "coder_metadata resource_id not found, falling back to graph traversal",
slog.F("resource_id", attrs.ResourceID),
slog.F("metadata_address", resource.Address))
Comment on lines +706 to +710
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can tell, this warn log will just end up in the server log and won't be especially visible to template authors.

I can see an argument for erroring out here, but the larger argument against is that it would most likely break existing templates. How do we feel about this?

case 1:
// Single match - use it
targetLabel = foundLabels[0]
default:
// Multiple resources with same ID - this creates ambiguity
logger.Warn(ctx, "multiple resources found with same resource_id, falling back to graph traversal",
slog.F("resource_id", attrs.ResourceID),
slog.F("metadata_address", resource.Address),
slog.F("matching_labels", foundLabels))
Comment on lines +714 to +719
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, I can see an argument for erroring here to surface the misconfiguration but worry about the potential for breaking existing templates.

}
attachedNode = node
break
}
if attachedNode == nil {
continue
}
var attachedResource *graphResource
for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, attachedNode.Name, 0, false) {
if attachedResource == nil {
// Default to the first resource because we have nothing to compare!
attachedResource = resource
continue

// If ResourceID wasn't provided or wasn't found, use graph traversal
if targetLabel == "" {
resourceLabel := convertAddressToLabel(resource.Address)

var attachedNode *gographviz.Node
for _, node := range graph.Nodes.Lookup {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage, it would make sense to extract the "metadata association" piece out and test it separately.

// The node attributes surround the label with quotes.
if strings.Trim(node.Attrs["label"], `"`) != resourceLabel {
continue
}
attachedNode = node
break
}
if resource.Depth < attachedResource.Depth {
// There's a closer resource!
attachedResource = resource
if attachedNode == nil {
continue
}
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
attachedResource = resource
var attachedResource *graphResource
for _, resource := range findResourcesInGraph(graph, tfResourcesByLabel, attachedNode.Name, 0, false) {
if attachedResource == nil {
// Default to the first resource because we have nothing to compare!
attachedResource = resource
continue
}
if resource.Depth < attachedResource.Depth {
// There's a closer resource!
attachedResource = resource
continue
}
if resource.Depth == attachedResource.Depth && resource.Label < attachedResource.Label {
attachedResource = resource
continue
}
}
if attachedResource == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what situation would we encounter this? It's not covered in existing or new tests.

continue
}
targetLabel = attachedResource.Label
}
if attachedResource == nil {
continue
}
targetLabel := attachedResource.Label

if metadataTargetLabels[targetLabel] {
return nil, xerrors.Errorf("duplicate metadata resource: %s", targetLabel)
Expand Down
99 changes: 84 additions & 15 deletions provisioner/terraform/resources_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1249,24 +1249,93 @@ func TestAgentNameDuplicate(t *testing.T) {
require.ErrorContains(t, err, "duplicate agent name")
}

func TestMetadataResourceDuplicate(t *testing.T) {
func TestMetadata(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)

// Load the multiple-apps state file and edit it.
dir := filepath.Join("testdata", "resources", "resource-metadata-duplicate")
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "resource-metadata-duplicate.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, "resource-metadata-duplicate.tfplan.dot"))
require.NoError(t, err)
t.Run("Duplicate", func(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)
// Load the multiple-apps state file and edit it.
dir := filepath.Join("testdata", "resources", "resource-metadata-duplicate")
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "resource-metadata-duplicate.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, "resource-metadata-duplicate.tfplan.dot"))
require.NoError(t, err)

state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
require.Nil(t, state)
require.Error(t, err)
require.ErrorContains(t, err, "duplicate metadata resource: null_resource.about")
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
require.Nil(t, state)
require.Error(t, err)
require.ErrorContains(t, err, "duplicate metadata resource: null_resource.about")
})

t.Run("ResourceID", func(t *testing.T) {
t.Parallel()

t.Run("OK", func(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)

dir := filepath.Join("testdata", "resources", "resource-id-provided")
tfStateRaw, err := os.ReadFile(filepath.Join(dir, "resource-id-provided.tfstate.json"))
require.NoError(t, err)
var tfState tfjson.State
err = json.Unmarshal(tfStateRaw, &tfState)
require.NoError(t, err)
tfStateGraph, err := os.ReadFile(filepath.Join(dir, "resource-id-provided.tfstate.dot"))
require.NoError(t, err)

state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfState.Values.RootModule}, string(tfStateGraph), logger)
require.NoError(t, err)
require.Len(t, state.Resources, 2)

// Find the resources
var firstResource, secondResource *proto.Resource
for _, res := range state.Resources {
if res.Name == "first" && res.Type == "null_resource" {
firstResource = res
} else if res.Name == "second" && res.Type == "null_resource" {
secondResource = res
}
}

require.NotNil(t, firstResource)
require.NotNil(t, secondResource)

// The metadata should be on the second resource (as specified by resource_id),
// not the first one (which is the closest in the graph)
require.Len(t, firstResource.Metadata, 0, "first resource should have no metadata")
require.Len(t, secondResource.Metadata, 1, "second resource should have metadata")
require.Equal(t, "test", secondResource.Metadata[0].Key)
require.Equal(t, "value", secondResource.Metadata[0].Value)
})

t.Run("NotFound", func(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)

dir := filepath.Join("testdata", "resources", "resource-id-not-found")
tfStateRaw, err := os.ReadFile(filepath.Join(dir, "resource-id-not-found.tfstate.json"))
require.NoError(t, err)
var tfState tfjson.State
err = json.Unmarshal(tfStateRaw, &tfState)
require.NoError(t, err)
tfStateGraph, err := os.ReadFile(filepath.Join(dir, "resource-id-not-found.tfstate.dot"))
require.NoError(t, err)

state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfState.Values.RootModule}, string(tfStateGraph), logger)
require.NoError(t, err)
require.Len(t, state.Resources, 1)

// The metadata should still be applied via graph traversal
require.Equal(t, "example", state.Resources[0].Name)
require.Len(t, state.Resources[0].Metadata, 1)
require.Equal(t, "test", state.Resources[0].Metadata[0].Key)
require.Equal(t, "value", state.Resources[0].Metadata[0].Value)
})
})
}

func TestParameterValidation(t *testing.T) {
Expand Down
19 changes: 19 additions & 0 deletions provisioner/terraform/testdata/resources/resource-id-not-found/resource-id-not-found.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
terraform {
required_providers {
coder = {
source = "coder/coder"
version = ">=2.0.0"
}
}
}

resource "null_resource" "example" {}

resource "coder_metadata" "example" {
resource_id = "non-existent-id"
Copy link
Member

@ethanndickson ethanndickson Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have another test to check that it falls back to graph traversal when the resource_id is set to coder_agent.whatever.id. I've seen that pattern in many customer templates in the past.

depends_on = [null_resource.example]
item {
key = "test"
value = "value"
}
}
17 changes: 17 additions & 0 deletions ...ioner/terraform/testdata/resources/resource-id-not-found/resource-id-not-found.tfplan.dot
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file added ...oner/terraform/testdata/resources/resource-id-not-found/resource-id-not-found.tfplan.json
View file
Open in desktop
Empty file.
17 changes: 17 additions & 0 deletions ...oner/terraform/testdata/resources/resource-id-not-found/resource-id-not-found.tfstate.dot
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

54 changes: 54 additions & 0 deletions ...ner/terraform/testdata/resources/resource-id-not-found/resource-id-not-found.tfstate.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions provisioner/terraform/testdata/resources/resource-id-provided/resource-id-provided.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
terraform {
required_providers {
coder = {
source = "coder/coder"
version = ">=2.0.0"
}
}
}

resource "null_resource" "first" {}

resource "null_resource" "second" {}

resource "coder_metadata" "example" {
resource_id = null_resource.second.id
depends_on = [null_resource.first]
item {
key = "test"
value = "value"
}
}
21 changes: 21 additions & 0 deletions ...isioner/terraform/testdata/resources/resource-id-provided/resource-id-provided.tfplan.dot
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file added ...sioner/terraform/testdata/resources/resource-id-provided/resource-id-provided.tfplan.json
View file
Open in desktop
Empty file.
Loading
Loading