fix: use resource_id directly for coder_metadata association #18300
Conversation
439f812 to
558c1eb
Compare
Change-Id: Ibda8f39393b6df90b98bc82e2a005a506830ce00 Signed-off-by: Thomas Kosiewski <tk@coder.com>
ae3960b to
7e0c0a2
Compare
| 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)) |
There was a problem hiding this comment.
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?
| 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)) |
There was a problem hiding this comment.
Again, I can see an argument for erroring here to surface the misconfiguration but worry about the potential for breaking existing templates.
| continue | ||
| } | ||
| } | ||
| if attachedResource == nil { |
There was a problem hiding this comment.
In what situation would we encounter this? It's not covered in existing or new tests.
| resourceLabel := convertAddressToLabel(resource.Address) | ||
|
|
||
| var attachedNode *gographviz.Node | ||
| for _, node := range graph.Nodes.Lookup { |
There was a problem hiding this comment.
At this stage, it would make sense to extract the "metadata association" piece out and test it separately.
| resource "null_resource" "example" {} | ||
|
|
||
| resource "coder_metadata" "example" { | ||
| resource_id = "non-existent-id" |
There was a problem hiding this comment.
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.
Previously, coder_metadata resources would associate with the wrong Terraform resource because the implementation relied on graph traversal instead of using the explicit resource_id attribute.
This fix:
This ensures that metadata is correctly associated with the intended resource, even when there are complex dependencies in the Terraform configuration.
Fixes the issue where coder_metadata would incorrectly associate with resources it references in its values rather than the resource specified in resource_id.