fix(coderd/templatebuilder): fix archive bundling for nested static files and counted agents - #26901
Conversation
…le subdirectories BundleTar wrote static files with nested paths (e.g. cloud-init/cloud-config.yaml.tftpl) without emitting TypeDir entries for their parent directories. The provisioner's archive extractor requires explicit directory entries and failed with 'no such file or directory' when unpacking.
ExtractAgentResourceName now detects when the coder_agent resource uses count or for_each and appends [0] to the returned name. This produces correct references (e.g. coder_agent.dev[0].id) in module templates for bases like aws-linux where the agent is counted.
| var agentCountPattern = regexp.MustCompile( | ||
| `resource\s+"coder_agent"\s+"\w+"\s*\{[^}]*\b(?:count|for_each)\s*=`, | ||
| ) |
There was a problem hiding this comment.
don't tell dev random
| // Emit directory entries for any subdirectories so that | ||
| // extractors that do not implicitly create parents can | ||
| // unpack the archive. | ||
| dirs := make(map[string]bool) | ||
| for _, name := range names { | ||
| for dir := path.Dir(name); dir != "." && !dirs[dir]; dir = path.Dir(dir) { | ||
| dirs[dir] = true | ||
| } | ||
| } | ||
| sortedDirs := make([]string, 0, len(dirs)) | ||
| for d := range dirs { | ||
| sortedDirs = append(sortedDirs, d) | ||
| } | ||
| slices.Sort(sortedDirs) | ||
| for _, d := range sortedDirs { | ||
| if err := writeTarDir(tw, d); err != nil { | ||
| return nil, xerrors.Errorf("write dir %s to tar: %w", d, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we have to create them in a separate loop rather than make writeTarFile ensure the dir exists because we can only write it once or something? Multiple WriteHeader(dir) calls I am guessing is an issue.
I wonder if we should store the files in their original tree structure so we can traverse it and create the dirs and files as we go.
Or even have ExtraFiles be an FS implementation so we could just do tw.AddFS(result.ExtraFiles).
Not a blocker though, this seems reasonable too, mostly just thinking out loud.
There was a problem hiding this comment.
Yeah, duplicate WriteHeader for the same dir path would produce a malformed archive, so we collect unique dirs first. The separate loop also keeps the dir entries sorted before the file entries, which is conventional for tar.
tw.AddFS is a great call; that would handle dir creation automatically. ExtraFiles as an fs.FS (or even just keeping the embedded fs.FS through to bundling) would be the cleanest path. Happy to take that on as a follow-up if you want.
Generated by Coder Agents (on behalf of @jeremyruppel)
There was a problem hiding this comment.
keeps the dir entries sorted before the file entries, which is conventional for tar.
Wait really?? But tar supports piping, that would mean it has to wait for the full input to put all the dirs first.
I tested a code-server release tar (generated by GNU tar) and it does not seem to do that.
nbd either way though.
There was a problem hiding this comment.
lol you caught the robot!!!
Fair point. GNU tar interleaves directory entries right before their children as it traverses the tree; it doesn't hoist all dirs to the top. My "conventional" claim was wrong. Streaming tars can't buffer the full input to sort dirs first.
| case 1: | ||
| return string(matches[0][1]), nil | ||
| name := string(matches[0][1]) | ||
| if agentCountPattern.Match(hcl) { |
There was a problem hiding this comment.
Seems chill since the input is well-known and we are not rendering arbitrary HCL.
But (my understanding around this is weak), is there no way to "properly" parse the HCL? Whether we already have something like that (I think dynamic parameters does some parsing) or using some library.
Or is more that it just is not worth the overhead.
There was a problem hiding this comment.
Agreed, the input is well-known so regex is fine here. We could use hclwrite.ParseConfig to walk the AST properly; the hclwrite package is already a dependency. The main reason I stuck with regex is that ExtractAgentResourceName was already regex-based and this is a small incremental check on the same pattern. Not worth the overhead for curated input, but if this function grows more complex it would be worth switching.
Generated by Coder Agents (on behalf of @jeremyruppel)
2 participants
Fixes two template builder bugs that caused AWS EC2 (Linux) template imports to fail:
Missing directory entries in tar archive:
BundleTarwrote static files with nested paths (e.g.cloud-init/cloud-config.yaml.tftpl) without emittingTypeDirentries for parent directories. The provisioner's archive extractor requires explicit directory entries and failed with "no such file or directory".Incorrect agent reference for counted resources:
ExtractAgentResourceNamereturneddevfor the AWS Linux base template, but the agent usescount = data.coder_workspace.me.start_count, so module templates needcoder_agent.dev[0].id. The function now detectscount/for_eachand appends[0].Note
Generated by Coder Agents (on behalf of @jeremyruppel)