Skip to content
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.

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

Conflict when adding coder_metadata with duplicate resouce_ids #305

Open
matifali opened this issue Nov 19, 2024 · 7 comments
Open

Conflict when adding coder_metadata with duplicate resouce_ids #305

matifali opened this issue Nov 19, 2024 · 7 comments

Comments

@matifali
Copy link
Member

matifali commented Nov 19, 2024

Problem Description

  1. While adding coder_metadata blocks inside a module we need to pass the resource_id to link the metadata to. The issue arises when the resource_id of the instance is linked to an existing coder_metadata block in the main template, which causes a conflict and the template fails to build.

  2. Attempting to use an alternative identifier, such as agent_id, doesn't show metadata on the workspace page.

Steps to Reproduce

  1. Add a coder_metadata block to a module and link it to the resource_id of an instance.
  2. Add another coder_metadata block in the main template referencing the same resource_id.
  3. Observe the conflict in the failed build.
  4. Attempt to use agent_id instead in the module and note the absence of display metadata.

Impact

  • Limits the flexibility of the coder modules because of possible conflicts.

Next Steps

  • Update the handling of coder_metadata to avoid conflicts between module-level and main template declarations when sharing the same resource_id. One possible solution is to merge and append the new item entries when we detect a conflict in resource_id.
  • Investigate why the agent_id does not work with coder_metadata.

Note

We did allow adding multiple coder_metadata with the same resource_id. See coder/coder#6517.

@matifali matifali changed the title Conflict When Adding coder_metadata in Modules Conflict when adding coder_metadata with duplicate resouce_ids Nov 19, 2024
@matifali
Copy link
Member Author

Related: #306

@ethanndickson
Copy link
Member

ethanndickson commented Nov 20, 2024

The provisioner doesn't currently handle associating coder_metadata with coder_* resources, such as the agent. (1).

All other Terraform resources are returned by the provisioner as containing a list of Resource_Metadata, which is populated using attached coder_metadata resources. Once that list is populated, those coder_metadata resources are discarded.
The agent is not returned by the provisioner as it's own resource, similarly to the metadata, it's just a field on the attached resource.

You can see this if you GET a workspace and view the raw json, this is the docker_container resource on my dogfood workspace, for example:
Image

So to resolve this issue, I'd propose just two changes:

  • If a coder_metadata resource would be attached to a coder_agent resource (via the output of terraform graph), then we should instead attempt to convert that metadata to static values that would be returned by https://dev.coder.com/api/v2/workspaceagents/{id}/watch-metadata. This would require minimal changes to the provisioner, and (presumably) no changes to the frontend. The static coder_metadata items would show up alongside the dynamic metadata on the agent (which is defined within metadata blocks within coder_agent).

  • As you said, If multiple coder_metadata resources would be attached to a single non-coder_* resource, then I agree that we should merge/append the items, instead of simply returning an error. Again, this would only require provisioner changes, and should be a pretty trivial implementation.

@matifali
Copy link
Member Author

matifali commented Nov 20, 2024

The static coder_metadata items would show up alongside the dynamic metadata on the agent (which is defined within metadata blocks within coder_agent).

I don't think we should merge the dynamic metadata of the coder_agent and the staticcoder_metadata linked to the same coder_agent as It would cause confusion for the user. If we want to do this, we should either,

  1. display them on a separate row/section (requires changes in the frontend and also would be different from general coder_metadata behavior attached to other non coder_* resources).
  2. Display them on top of the agent similar to how current coder_metadata gets displayed.
    Image

@matifali matifali transferred this issue from coder/coder Nov 20, 2024
@ethanndickson
Copy link
Member

ethanndickson commented Nov 20, 2024

1: Sounds like a good solution, just a fair bit more work.
2: This would effectively be merging the agent's static metadata with the first resource of the workspace, which would be equally confusing as merging it with the dynamic agent metadata.

EDIT: I don't think 1 is the right move, it would mean we have agent metadata in the UI in two different places, depending on whether it's set in the template, or retrieved dynamically via a command.

@matifali
Copy link
Member Author

matifali commented Nov 20, 2024

  1. This would effectively be merging the agent's static metadata with the first resource of the workspace, which would be equally confusing as merging it with the dynamic agent metadata.

I don't mean we merge the coder_metadata assigned to different resources but merge if multiple coder_metadata are linked to a single resource. Consider the example below,

resource "coder_metadata" "module" {
  count       = data.coder_workspace.me.start_count
  resource_id = resource.aws_instnace.id

  item {
    key   = "username"
    value = "var.admin_username"
  }
  item {
    key       = "password"
    value     = var.admin_password
    sensitive = true
  }
}

resource "coder_metadata" "template" {
  count       = data.coder_workspace.me.start_count
  resource_id = resource.aws_instnace.id # note that both use the same reosurce_id

  item {
    key   = "region"
    value = data.coder_parameter.region.value
  }
  item {
    key   = "instance type"
    value = aws_instance.dev.instance_type
  }
}

We should display a single coder_metadata section and merge the item from both coder_metadata resources as they are both linked with the same resource.aws_instance.dev resource. We may still get conflicts if the item keys are not unique.

What do you think about it @bpmct?

@ethanndickson
Copy link
Member

ethanndickson commented Nov 20, 2024

Oh I see what you mean, yeah. That's what you suggested originally, I definitely agree that's a good solution. but that doesn't address where coder_metadata attached to a coder_agent should go.

@matifali
Copy link
Member Author

matifali commented Nov 20, 2024

Oh I see what you mean, yeah. That's what you suggested originally, I definitely agree that's a good solution. but that doesn't address where coder_metadata attached to a coder_agent should go.

Yes, maybe we can narrow the scope so as not to support the coder_agent for now, just update our modules to pass another resource_id as input, and attach all metadata to that resource from within the modules. This would only need the ability to link multiple coder_metadata to a single resource and merge the items.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants