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.
|
Is there an issue that describes the problem this is intending to solve? It worries me that we're using resource IDs to identify the resource the metadata refers to when resource IDs are not unique. I think it would be better to deprecate the resource ID and instead use something unique like (module, resource_type, name). |
|
Yeah the problem is the metadata gets assigned to the wrong terraform resource. So on the workspace page, metadata you intend to be displayed alongside the agent is instead hidden on a random resource that you have to open the side bar to see. This one is working as intended but to give an idea of what i'm describing 
|
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.