From a43f613b1abb154c058197e55e62c32f4a83d326 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 17:29:05 +0000 Subject: [PATCH 01/12] feat(api): add spec.files field to CoderTemplate for template source --- api/aggregation/v1alpha1/types.go | 9 +++++++++ api/aggregation/v1alpha1/zz_generated.deepcopy.go | 9 ++++++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/api/aggregation/v1alpha1/types.go b/api/aggregation/v1alpha1/types.go index 5f4204e8..aa83dcc7 100644 --- a/api/aggregation/v1alpha1/types.go +++ b/api/aggregation/v1alpha1/types.go @@ -70,6 +70,15 @@ type CoderTemplateSpec struct { Description string `json:"description,omitempty"` Icon string `json:"icon,omitempty"` + // Files is the template source tree for the active template version. + // + // Keys are slash-delimited relative paths (e.g. "main.tf"). + // Values are UTF-8 file contents. + // + // Populated on GET; intentionally omitted from LIST to keep responses small. + // On CREATE/UPDATE with files, the server uploads source and creates a new template version. + Files map[string]string `json:"files,omitempty"` + // Running is a legacy flag retained temporarily for in-repo callers that still read template run-state directly. Running bool `json:"running,omitempty"` } diff --git a/api/aggregation/v1alpha1/zz_generated.deepcopy.go b/api/aggregation/v1alpha1/zz_generated.deepcopy.go index 7e7de573..4b18e0f7 100644 --- a/api/aggregation/v1alpha1/zz_generated.deepcopy.go +++ b/api/aggregation/v1alpha1/zz_generated.deepcopy.go @@ -14,7 +14,7 @@ func (in *CoderTemplate) DeepCopyInto(out *CoderTemplate) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) in.Status.DeepCopyInto(&out.Status) return } @@ -73,6 +73,13 @@ func (in *CoderTemplateList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *CoderTemplateSpec) DeepCopyInto(out *CoderTemplateSpec) { *out = *in + if in.Files != nil { + in, out := &in.Files, &out.Files + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } return } From a203cc9ebfb6ebea23f551c80d12f4ba5dfb3461 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 17:46:14 +0000 Subject: [PATCH 02/12] feat: reconcile template spec.files in storage --- internal/aggregated/convert/template.go | 17 + internal/aggregated/storage/storage_test.go | 676 +++++++++++++++++- internal/aggregated/storage/template.go | 146 +++- internal/aggregated/storage/template_files.go | 211 ++++++ 4 files changed, 1016 insertions(+), 34 deletions(-) create mode 100644 internal/aggregated/storage/template_files.go diff --git a/internal/aggregated/convert/template.go b/internal/aggregated/convert/template.go index be93f4e4..97227cd3 100644 --- a/internal/aggregated/convert/template.go +++ b/internal/aggregated/convert/template.go @@ -72,3 +72,20 @@ func TemplateCreateRequestFromK8s(obj *aggregationv1alpha1.CoderTemplate, templa Icon: obj.Spec.Icon, }, nil } + +// TemplateUpdateMetaRequestFromK8s builds a codersdk.UpdateTemplateMeta request. +func TemplateUpdateMetaRequestFromK8s(obj *aggregationv1alpha1.CoderTemplate) codersdk.UpdateTemplateMeta { + if obj == nil { + panic("assertion failed: template object must not be nil") + } + + displayName := obj.Spec.DisplayName + description := obj.Spec.Description + icon := obj.Spec.Icon + + return codersdk.UpdateTemplateMeta{ + DisplayName: &displayName, + Description: &description, + Icon: &icon, + } +} diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 47b1fd29..9830558d 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -27,6 +28,8 @@ import ( "github.com/coder/coder/v2/codersdk" ) +const seededTemplateMainTF = `resource "null_resource" "example" {}` + func TestTemplateStorageCRUDWithCoderSDK(t *testing.T) { t.Parallel() @@ -111,6 +114,360 @@ func TestTemplateStorageCRUDWithCoderSDK(t *testing.T) { } } +func TestTemplateStorageGetPopulatesSpecFiles(t *testing.T) { + t.Parallel() + + server, _ := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + obj, err := templateStorage.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("expected template get to succeed: %v", err) + } + + template, ok := obj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", obj) + } + + expectedFiles := map[string]string{"main.tf": seededTemplateMainTF} + if !reflect.DeepEqual(template.Spec.Files, expectedFiles) { + t.Fatalf("expected get to populate spec.files %v, got %v", expectedFiles, template.Spec.Files) + } +} + +func TestTemplateStorageListOmitsSpecFiles(t *testing.T) { + t.Parallel() + + server, _ := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + listObj, err := templateStorage.List(ctx, nil) + if err != nil { + t.Fatalf("expected template list to succeed: %v", err) + } + + list, ok := listObj.(*aggregationv1alpha1.CoderTemplateList) + if !ok { + t.Fatalf("expected *CoderTemplateList, got %T", listObj) + } + if len(list.Items) == 0 { + t.Fatal("expected at least one template from list") + } + + for _, template := range list.Items { + if len(template.Spec.Files) != 0 { + t.Fatalf("expected list to omit spec.files for %q, got %v", template.Name, template.Spec.Files) + } + } +} + +func TestTemplateStorageCreateWithFiles(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + createFiles := map[string]string{"main.tf": "resource \"null_resource\" \"created\" {}"} + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + + createObj := &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.files-template"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + DisplayName: "Files Template", + Description: "Template created from spec.files", + Icon: "/icons/files.png", + Files: cloneStringMap(createFiles), + }, + } + + createdObj, err := templateStorage.Create(ctx, createObj, rest.ValidateAllObjectFunc, nil) + if err != nil { + t.Fatalf("expected template create with files to succeed: %v", err) + } + + createdTemplate, ok := createdObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from create, got %T", createdObj) + } + + if state.fileCount() != fileCountBefore+1 { + t.Fatalf("expected one uploaded file, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore+1 { + t.Fatalf("expected one new template version, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionID, ok := state.templateActiveVersionID("acme", "files-template") + if !ok { + t.Fatal("expected created template active version in mock state") + } + if activeVersionID == uuid.Nil { + t.Fatal("expected created template active version to be non-nil") + } + if createdTemplate.Status.ActiveVersionID != activeVersionID.String() { + t.Fatalf("expected created status.activeVersionID %q, got %q", activeVersionID.String(), createdTemplate.Status.ActiveVersionID) + } + + fetchedObj, err := templateStorage.Get(ctx, "acme.files-template", nil) + if err != nil { + t.Fatalf("expected get for created template to succeed: %v", err) + } + fetchedTemplate, ok := fetchedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", fetchedObj) + } + if !reflect.DeepEqual(fetchedTemplate.Spec.Files, createFiles) { + t.Fatalf("expected created template files %v, got %v", createFiles, fetchedTemplate.Spec.Files) + } +} + +func TestTemplateStorageUpdateWithChangedFiles(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + initialFiles := map[string]string{"main.tf": "resource \"null_resource\" \"initial\" {}"} + createObj := &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.update-files-template"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + DisplayName: "Update Files Template", + Files: cloneStringMap(initialFiles), + }, + } + + createdObj, err := templateStorage.Create(ctx, createObj, rest.ValidateAllObjectFunc, nil) + if err != nil { + t.Fatalf("expected template create with files to succeed: %v", err) + } + createdTemplate, ok := createdObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from create, got %T", createdObj) + } + + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + activeVersionBefore, ok := state.templateActiveVersionID("acme", "update-files-template") + if !ok { + t.Fatal("expected active version for created template") + } + + updatedFiles := map[string]string{"main.tf": "resource \"null_resource\" \"updated\" {}"} + desiredTemplate := createdTemplate.DeepCopy() + desiredTemplate.Spec.Files = cloneStringMap(updatedFiles) + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected update with changed files to succeed: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + + if state.fileCount() != fileCountBefore+1 { + t.Fatalf("expected file upload during changed-files update, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore+1 { + t.Fatalf("expected template version creation during changed-files update, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionAfter, ok := state.templateActiveVersionID("acme", "update-files-template") + if !ok { + t.Fatal("expected active version for updated template") + } + if activeVersionAfter == activeVersionBefore { + t.Fatalf("expected active version to change, both were %q", activeVersionAfter) + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if !reflect.DeepEqual(updatedTemplate.Spec.Files, updatedFiles) { + t.Fatalf("expected updated files %v, got %v", updatedFiles, updatedTemplate.Spec.Files) + } + + fetchedObj, err := templateStorage.Get(ctx, "acme.update-files-template", nil) + if err != nil { + t.Fatalf("expected get after update to succeed: %v", err) + } + fetchedTemplate, ok := fetchedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", fetchedObj) + } + if !reflect.DeepEqual(fetchedTemplate.Spec.Files, updatedFiles) { + t.Fatalf("expected get to return updated files %v, got %v", updatedFiles, fetchedTemplate.Spec.Files) + } +} + +func TestTemplateStorageUpdateWithIdenticalFilesIsNoOp(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + initialFiles := map[string]string{"main.tf": "resource \"null_resource\" \"stable\" {}"} + createObj := &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.noop-files-template"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + Files: cloneStringMap(initialFiles), + }, + } + + createdObj, err := templateStorage.Create(ctx, createObj, rest.ValidateAllObjectFunc, nil) + if err != nil { + t.Fatalf("expected template create with files to succeed: %v", err) + } + createdTemplate, ok := createdObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from create, got %T", createdObj) + } + + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + activeVersionBefore, ok := state.templateActiveVersionID("acme", "noop-files-template") + if !ok { + t.Fatal("expected active version for created template") + } + + desiredTemplate := createdTemplate.DeepCopy() + desiredTemplate.Spec.Files = cloneStringMap(initialFiles) + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected update with identical files to succeed: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + + if state.fileCount() != fileCountBefore { + t.Fatalf("expected no new upload for identical files, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore { + t.Fatalf("expected no new template version for identical files, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionAfter, ok := state.templateActiveVersionID("acme", "noop-files-template") + if !ok { + t.Fatal("expected active version for template after no-op update") + } + if activeVersionAfter != activeVersionBefore { + t.Fatalf("expected active version to remain %q on no-op update, got %q", activeVersionBefore, activeVersionAfter) + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if !reflect.DeepEqual(updatedTemplate.Spec.Files, initialFiles) { + t.Fatalf("expected updated files to stay %v, got %v", initialFiles, updatedTemplate.Spec.Files) + } +} + +func TestTemplateStorageUpdateMetadata(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("expected template get to succeed: %v", err) + } + currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", currentObj) + } + + metaUpdateCountBefore := state.templateMetaUpdateCount() + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + + desiredTemplate := currentTemplate.DeepCopy() + desiredTemplate.Spec.DisplayName = "Renamed Starter Template" + desiredTemplate.Spec.Description = "Updated description" + desiredTemplate.Spec.Icon = "/icons/renamed.png" + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected metadata update to succeed: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + if state.templateMetaUpdateCount() != metaUpdateCountBefore+1 { + t.Fatalf("expected one metadata update call, before=%d after=%d", metaUpdateCountBefore, state.templateMetaUpdateCount()) + } + if state.fileCount() != fileCountBefore { + t.Fatalf("expected metadata update to avoid file uploads, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore { + t.Fatalf("expected metadata update to avoid template version creation, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if updatedTemplate.Spec.DisplayName != desiredTemplate.Spec.DisplayName { + t.Fatalf("expected updated displayName %q, got %q", desiredTemplate.Spec.DisplayName, updatedTemplate.Spec.DisplayName) + } + if updatedTemplate.Spec.Description != desiredTemplate.Spec.Description { + t.Fatalf("expected updated description %q, got %q", desiredTemplate.Spec.Description, updatedTemplate.Spec.Description) + } + if updatedTemplate.Spec.Icon != desiredTemplate.Spec.Icon { + t.Fatalf("expected updated icon %q, got %q", desiredTemplate.Spec.Icon, updatedTemplate.Spec.Icon) + } +} + func TestTemplateStorageListAllowsAllNamespacesRequest(t *testing.T) { t.Parallel() @@ -337,18 +694,18 @@ func TestTemplateStorageUpdateAllowsEmptyOptionalFieldsWhenTogglingRunning(t *te if updatedTemplate.Spec.Running != currentTemplate.Spec.Running { t.Fatalf("expected update response running=%t from current backend object, got %t", currentTemplate.Spec.Running, updatedTemplate.Spec.Running) } - if updatedTemplate.Spec.DisplayName != currentTemplate.Spec.DisplayName { - t.Fatalf("expected update response spec.displayName %q from current backend object, got %q", currentTemplate.Spec.DisplayName, updatedTemplate.Spec.DisplayName) + if updatedTemplate.Spec.DisplayName != desiredTemplate.Spec.DisplayName { + t.Fatalf("expected updated spec.displayName %q, got %q", desiredTemplate.Spec.DisplayName, updatedTemplate.Spec.DisplayName) } - if updatedTemplate.Spec.Description != currentTemplate.Spec.Description { - t.Fatalf("expected update response spec.description %q from current backend object, got %q", currentTemplate.Spec.Description, updatedTemplate.Spec.Description) + if updatedTemplate.Spec.Description != desiredTemplate.Spec.Description { + t.Fatalf("expected updated spec.description %q, got %q", desiredTemplate.Spec.Description, updatedTemplate.Spec.Description) } - if updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon { - t.Fatalf("expected update response spec.icon %q from current backend object, got %q", currentTemplate.Spec.Icon, updatedTemplate.Spec.Icon) + if updatedTemplate.Spec.Icon != desiredTemplate.Spec.Icon { + t.Fatalf("expected updated spec.icon %q, got %q", desiredTemplate.Spec.Icon, updatedTemplate.Spec.Icon) } } -func TestTemplateStorageUpdateRejectsDifferentVersionID(t *testing.T) { +func TestTemplateStorageUpdateIgnoresDifferentVersionID(t *testing.T) { t.Parallel() server, _ := newMockCoderServer(t) @@ -374,7 +731,7 @@ func TestTemplateStorageUpdateRejectsDifferentVersionID(t *testing.T) { t.Fatal("expected test fixture to use a different spec.versionID") } - _, _, err = templateStorage.Update( + updatedObj, created, err := templateStorage.Update( ctx, desiredTemplate.Name, testUpdatedObjectInfo{obj: desiredTemplate}, @@ -383,15 +740,23 @@ func TestTemplateStorageUpdateRejectsDifferentVersionID(t *testing.T) { false, nil, ) - if !apierrors.IsBadRequest(err) { - t.Fatalf("expected BadRequest when changing spec.versionID, got %v", err) + if err != nil { + t.Fatalf("expected template update to succeed when changing spec.versionID: %v", err) } - if err == nil || !strings.Contains(err.Error(), "spec.running") { - t.Fatalf("expected immutable-field error mentioning spec.running, got %v", err) + if created { + t.Fatal("expected update created=false") + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { + t.Fatalf("expected returned spec.versionID %q from backend, got %q", currentTemplate.Spec.VersionID, updatedTemplate.Spec.VersionID) } } -func TestTemplateStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) { +func TestTemplateStorageUpdateAllowsMetadataSpecChanges(t *testing.T) { t.Parallel() server, _ := newMockCoderServer(t) @@ -413,7 +778,7 @@ func TestTemplateStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) { desiredTemplate := currentTemplate.DeepCopy() desiredTemplate.Spec.DisplayName = "Renamed Template" - _, _, err = templateStorage.Update( + updatedObj, created, err := templateStorage.Update( ctx, desiredTemplate.Name, testUpdatedObjectInfo{obj: desiredTemplate}, @@ -422,11 +787,19 @@ func TestTemplateStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) { false, nil, ) - if !apierrors.IsBadRequest(err) { - t.Fatalf("expected BadRequest when changing immutable template spec fields, got %v", err) + if err != nil { + t.Fatalf("expected update to allow metadata spec changes: %v", err) } - if err == nil || !strings.Contains(err.Error(), "spec.running") { - t.Fatalf("expected immutable-field error mentioning spec.running, got %v", err) + if created { + t.Fatal("expected update created=false") + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if updatedTemplate.Spec.DisplayName != desiredTemplate.Spec.DisplayName { + t.Fatalf("expected updated displayName %q, got %q", desiredTemplate.Spec.DisplayName, updatedTemplate.Spec.DisplayName) } } @@ -1415,11 +1788,13 @@ type mockCoderServerState struct { templatesByID map[uuid.UUID]codersdk.Template templateIDsByOrg map[string]map[string]uuid.UUID templateVersionsByID map[uuid.UUID]codersdk.TemplateVersion + filesByID map[uuid.UUID][]byte workspacesByID map[uuid.UUID]codersdk.Workspace workspaceIDsByUser map[string]map[string]uuid.UUID - buildTransitions []codersdk.WorkspaceTransition - failBuildTransitions map[codersdk.WorkspaceTransition]int + buildTransitions []codersdk.WorkspaceTransition + failBuildTransitions map[codersdk.WorkspaceTransition]int + templateMetaPatchCall int } func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) { @@ -1429,11 +1804,17 @@ func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) orgID := uuid.New() templateID := uuid.New() activeVersionID := uuid.New() + fileID := uuid.New() workspaceID := uuid.New() workspaceBuildID := uuid.New() ttlMillis := int64(3600000) autostartSchedule := "CRON_TZ=UTC 0 9 * * 1-5" + seededTemplateSourceZip, seedErr := buildSourceZip(map[string]string{"main.tf": seededTemplateMainTF}) + if seedErr != nil { + t.Fatalf("build seeded template source zip: %v", seedErr) + } + organization := codersdk.Organization{ MinimalOrganization: codersdk.MinimalOrganization{ ID: orgID, @@ -1466,6 +1847,9 @@ func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) UpdatedAt: now.Add(-2 * time.Hour), Name: "starter-template-v1", Message: "initial version", + Job: codersdk.ProvisionerJob{ + FileID: fileID, + }, } workspace := codersdk.Workspace{ @@ -1507,6 +1891,9 @@ func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) templateVersionsByID: map[uuid.UUID]codersdk.TemplateVersion{ templateVersion.ID: templateVersion, }, + filesByID: map[uuid.UUID][]byte{ + fileID: seededTemplateSourceZip, + }, workspacesByID: map[uuid.UUID]codersdk.Workspace{ workspace.ID: workspace, }, @@ -1544,12 +1931,27 @@ func (s *mockCoderServerState) handleRequest(t *testing.T, w http.ResponseWriter case r.Method == http.MethodPost && hasSegments(segments, "api", "v2", "organizations") && len(segments) == 5 && segments[4] == "templates": s.handleCreateTemplate(w, r, segments[3]) return + case r.Method == http.MethodPost && hasSegments(segments, "api", "v2", "organizations") && len(segments) == 5 && segments[4] == "templateversions": + s.handleCreateTemplateVersion(w, r, segments[3]) + return + case r.Method == http.MethodPatch && hasSegments(segments, "api", "v2", "templates") && len(segments) == 4: + s.handleUpdateTemplateMeta(w, r, segments[3]) + return + case r.Method == http.MethodPatch && hasSegments(segments, "api", "v2", "templates") && len(segments) == 5 && segments[4] == "versions": + s.handleUpdateActiveTemplateVersion(w, r, segments[3]) + return case r.Method == http.MethodDelete && hasSegments(segments, "api", "v2", "templates") && len(segments) == 4: s.handleDeleteTemplate(w, segments[3]) return case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "templateversions") && len(segments) == 4: s.handleGetTemplateVersion(w, segments[3]) return + case r.Method == http.MethodPost && hasSegments(segments, "api", "v2", "files") && len(segments) == 3: + s.handleUploadFile(w, r) + return + case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "files") && len(segments) == 4: + s.handleGetFile(w, r, segments[3]) + return case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "workspaces") && len(segments) == 3: s.handleListWorkspaces(w) return @@ -1662,6 +2064,186 @@ func (s *mockCoderServerState) handleCreateTemplate(w http.ResponseWriter, r *ht writeJSON(w, http.StatusCreated, template) } +func (s *mockCoderServerState) handleUploadFile(w http.ResponseWriter, r *http.Request) { + s.mu.Lock() + defer s.mu.Unlock() + + fileData, err := io.ReadAll(r.Body) + if err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("read upload body: %v", err)) + return + } + + fileID := uuid.New() + s.filesByID[fileID] = fileData + + writeJSON(w, http.StatusCreated, codersdk.UploadResponse{ID: fileID}) +} + +func (s *mockCoderServerState) handleGetFile(w http.ResponseWriter, r *http.Request, fileIDSegment string) { + s.mu.Lock() + defer s.mu.Unlock() + + fileID, err := uuid.Parse(fileIDSegment) + if err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("invalid file id %q", fileIDSegment)) + return + } + + format := r.URL.Query().Get("format") + if format != "" && format != codersdk.FormatZip { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("unsupported format %q", format)) + return + } + + fileData, ok := s.filesByID[fileID] + if !ok { + writeCoderError(w, http.StatusNotFound, "file not found") + return + } + + w.Header().Set("Content-Type", codersdk.ContentTypeZip) + w.WriteHeader(http.StatusOK) + _, _ = w.Write(fileData) +} + +func (s *mockCoderServerState) handleCreateTemplateVersion(w http.ResponseWriter, r *http.Request, orgSegment string) { + s.mu.Lock() + defer s.mu.Unlock() + + if orgSegment != s.organization.Name && orgSegment != s.organization.ID.String() { + writeCoderError(w, http.StatusNotFound, "organization not found") + return + } + + var request codersdk.CreateTemplateVersionRequest + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("decode create template version request: %v", err)) + return + } + if request.FileID == uuid.Nil { + writeCoderError(w, http.StatusBadRequest, "file_id is required") + return + } + if _, ok := s.filesByID[request.FileID]; !ok { + writeCoderError(w, http.StatusNotFound, "file not found") + return + } + + now := time.Now().UTC() + templateVersion := codersdk.TemplateVersion{ + ID: uuid.New(), + OrganizationID: s.organization.ID, + CreatedAt: now, + UpdatedAt: now, + Name: fmt.Sprintf("template-version-%d", len(s.templateVersionsByID)+1), + Message: request.Message, + Job: codersdk.ProvisionerJob{ + FileID: request.FileID, + }, + } + if request.TemplateID != uuid.Nil { + if _, ok := s.templatesByID[request.TemplateID]; !ok { + writeCoderError(w, http.StatusNotFound, "template not found") + return + } + templateID := request.TemplateID + templateVersion.TemplateID = &templateID + } + + s.templateVersionsByID[templateVersion.ID] = templateVersion + + writeJSON(w, http.StatusCreated, templateVersion) +} + +func (s *mockCoderServerState) handleUpdateTemplateMeta(w http.ResponseWriter, r *http.Request, templateIDSegment string) { + s.mu.Lock() + defer s.mu.Unlock() + + templateID, err := uuid.Parse(templateIDSegment) + if err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("invalid template id %q", templateIDSegment)) + return + } + + template, ok := s.templatesByID[templateID] + if !ok { + writeCoderError(w, http.StatusNotFound, "template not found") + return + } + + var request codersdk.UpdateTemplateMeta + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("decode update template metadata request: %v", err)) + return + } + + if request.DisplayName != nil { + template.DisplayName = *request.DisplayName + } + if request.Description != nil { + template.Description = *request.Description + } + if request.Icon != nil { + template.Icon = *request.Icon + } + template.UpdatedAt = time.Now().UTC() + + s.templatesByID[templateID] = template + s.templateMetaPatchCall++ + + writeJSON(w, http.StatusOK, template) +} + +func (s *mockCoderServerState) handleUpdateActiveTemplateVersion(w http.ResponseWriter, r *http.Request, templateIDSegment string) { + s.mu.Lock() + defer s.mu.Unlock() + + templateID, err := uuid.Parse(templateIDSegment) + if err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("invalid template id %q", templateIDSegment)) + return + } + + template, ok := s.templatesByID[templateID] + if !ok { + writeCoderError(w, http.StatusNotFound, "template not found") + return + } + + var request codersdk.UpdateActiveTemplateVersion + if err := json.NewDecoder(r.Body).Decode(&request); err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("decode update active template version request: %v", err)) + return + } + if request.ID == uuid.Nil { + writeCoderError(w, http.StatusBadRequest, "active version id is required") + return + } + + templateVersion, ok := s.templateVersionsByID[request.ID] + if !ok { + writeCoderError(w, http.StatusNotFound, "template version not found") + return + } + if templateVersion.TemplateID != nil && *templateVersion.TemplateID != templateID { + writeCoderError(w, http.StatusBadRequest, "template version does not belong to template") + return + } + if templateVersion.TemplateID == nil { + templateIDCopy := templateID + templateVersion.TemplateID = &templateIDCopy + templateVersion.UpdatedAt = time.Now().UTC() + s.templateVersionsByID[templateVersion.ID] = templateVersion + } + + template.ActiveVersionID = request.ID + template.UpdatedAt = time.Now().UTC() + s.templatesByID[templateID] = template + + writeJSON(w, http.StatusOK, map[string]string{"message": "template active version updated"}) +} + func (s *mockCoderServerState) handleDeleteTemplate(w http.ResponseWriter, templateIDSegment string) { s.mu.Lock() defer s.mu.Unlock() @@ -1900,6 +2482,47 @@ func (s *mockCoderServerState) hasTemplate(organization, templateName string) bo return ok } +func (s *mockCoderServerState) fileCount() int { + s.mu.Lock() + defer s.mu.Unlock() + + return len(s.filesByID) +} + +func (s *mockCoderServerState) templateVersionCount() int { + s.mu.Lock() + defer s.mu.Unlock() + + return len(s.templateVersionsByID) +} + +func (s *mockCoderServerState) templateActiveVersionID(organization, templateName string) (uuid.UUID, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + organizationTemplates, ok := s.templateIDsByOrg[organization] + if !ok { + return uuid.Nil, false + } + templateID, ok := organizationTemplates[templateName] + if !ok { + return uuid.Nil, false + } + template, ok := s.templatesByID[templateID] + if !ok { + return uuid.Nil, false + } + + return template.ActiveVersionID, true +} + +func (s *mockCoderServerState) templateMetaUpdateCount() int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.templateMetaPatchCall +} + func (s *mockCoderServerState) setTemplateVersionTemplateID(templateVersionID, templateID uuid.UUID) { if templateVersionID == uuid.Nil { panic("assertion failed: template version ID must not be nil") @@ -2002,6 +2625,19 @@ func namespacedContext(namespace string) context.Context { return genericapirequest.WithNamespace(context.Background(), namespace) } +func cloneStringMap(source map[string]string) map[string]string { + if source == nil { + return nil + } + + cloned := make(map[string]string, len(source)) + for key, value := range source { + cloned[key] = value + } + + return cloned +} + func containsTransition(transitions []codersdk.WorkspaceTransition, transition codersdk.WorkspaceTransition) bool { for _, got := range transitions { if got == transition { diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index cab6baf8..476ef492 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -1,9 +1,12 @@ package storage import ( + "bytes" "context" "fmt" + "reflect" + "github.com/google/uuid" apierrors "k8s.io/apimachinery/pkg/api/errors" metainternalversion "k8s.io/apimachinery/pkg/apis/meta/internalversion" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -105,7 +108,15 @@ func (s *TemplateStorage) Get(ctx context.Context, name string, _ *metav1.GetOpt return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) } - return convert.TemplateToK8s(namespace, template), nil + obj := convert.TemplateToK8s(namespace, template) + + files, err := fetchTemplateSourceFiles(ctx, sdk, template.ActiveVersionID) + if err != nil { + return nil, fmt.Errorf("fetch template source files: %w", err) + } + obj.Spec.Files = files + + return obj, nil } // List fetches CoderTemplate objects from codersdk. @@ -216,6 +227,40 @@ func (s *TemplateStorage) Create( return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } + if len(templateObj.Spec.Files) > 0 { + zipBytes, err := buildSourceZip(templateObj.Spec.Files) + if err != nil { + return nil, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) + } + + uploadResponse, err := sdk.Upload(ctx, codersdk.ContentTypeZip, bytes.NewReader(zipBytes)) + if err != nil { + return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + } + + templateVersion, err := sdk.CreateTemplateVersion(ctx, org.ID, codersdk.CreateTemplateVersionRequest{ + StorageMethod: codersdk.ProvisionerStorageMethodFile, + FileID: uploadResponse.ID, + Provisioner: codersdk.ProvisionerTypeTerraform, + }) + if err != nil { + return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + } + + createdTemplate, err := sdk.CreateTemplate(ctx, org.ID, codersdk.CreateTemplateRequest{ + Name: templateName, + VersionID: templateVersion.ID, + DisplayName: templateObj.Spec.DisplayName, + Description: templateObj.Spec.Description, + Icon: templateObj.Spec.Icon, + }) + if err != nil { + return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + } + + return convert.TemplateToK8s(namespace, createdTemplate), nil + } + request, err := convert.TemplateCreateRequestFromK8s(templateObj, templateName) if err != nil { return nil, apierrors.NewBadRequest(err.Error()) @@ -229,7 +274,7 @@ func (s *TemplateStorage) Create( return convert.TemplateToK8s(namespace, createdTemplate), nil } -// Update applies a legacy-compatible template update. +// Update applies a template metadata/source reconcile. func (s *TemplateStorage) Update( ctx context.Context, name string, @@ -317,22 +362,95 @@ func (s *TemplateStorage) Update( return nil, false, err } } - - // Template updates via codersdk are currently limited. The legacy spec.running - // field remains for compatibility with in-repo callers and is a no-op in the - // Coder backend. Reject updates to all other spec fields to avoid drift between - // accepted update payloads and persisted backend state. - if updatedTemplate.Spec.Organization != currentTemplate.Spec.Organization || - (updatedTemplate.Spec.VersionID != "" && updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID) || - (updatedTemplate.Spec.DisplayName != "" && updatedTemplate.Spec.DisplayName != currentTemplate.Spec.DisplayName) || - (updatedTemplate.Spec.Description != "" && updatedTemplate.Spec.Description != currentTemplate.Spec.Description) || - (updatedTemplate.Spec.Icon != "" && updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon) { + if updatedTemplate.Spec.Organization != currentTemplate.Spec.Organization { return nil, false, apierrors.NewBadRequest( - "template update only supports changing spec.running; other spec fields are immutable", + fmt.Sprintf( + "spec.organization %q must match existing organization %q", + updatedTemplate.Spec.Organization, + currentTemplate.Spec.Organization, + ), ) } - return currentTemplate, false, nil + templateID, err := uuid.Parse(currentTemplate.Status.ID) + if err != nil { + return nil, false, fmt.Errorf("parse current template status.id %q: %w", currentTemplate.Status.ID, err) + } + + sdk, err := s.clientForNamespace(ctx, namespace) + if err != nil { + return nil, false, wrapClientError(err) + } + + metadataChanged := updatedTemplate.Spec.DisplayName != currentTemplate.Spec.DisplayName || + updatedTemplate.Spec.Description != currentTemplate.Spec.Description || + updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon + if metadataChanged { + _, err := sdk.UpdateTemplateMeta(ctx, templateID, convert.TemplateUpdateMetaRequestFromK8s(updatedTemplate)) + if err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + } + + if updatedTemplate.Spec.Files != nil { + currentActiveVersionID, err := uuid.Parse(currentTemplate.Status.ActiveVersionID) + if err != nil { + return nil, false, fmt.Errorf( + "parse current template status.activeVersionID %q: %w", + currentTemplate.Status.ActiveVersionID, + err, + ) + } + + currentFiles, err := fetchTemplateSourceFiles(ctx, sdk, currentActiveVersionID) + if err != nil { + return nil, false, fmt.Errorf("fetch current template source files: %w", err) + } + + if !reflect.DeepEqual(updatedTemplate.Spec.Files, currentFiles) { + zipBytes, err := buildSourceZip(updatedTemplate.Spec.Files) + if err != nil { + return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) + } + + uploadResponse, err := sdk.Upload(ctx, codersdk.ContentTypeZip, bytes.NewReader(zipBytes)) + if err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + if uploadResponse.ID == uuid.Nil { + return nil, false, fmt.Errorf("assertion failed: uploaded file ID must not be nil") + } + + org, err := sdk.OrganizationByName(ctx, currentTemplate.Spec.Organization) + if err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + + newVersion, err := sdk.CreateTemplateVersion(ctx, org.ID, codersdk.CreateTemplateVersionRequest{ + TemplateID: templateID, + StorageMethod: codersdk.ProvisionerStorageMethodFile, + FileID: uploadResponse.ID, + Provisioner: codersdk.ProvisionerTypeTerraform, + }) + if err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + if newVersion.ID == uuid.Nil { + return nil, false, fmt.Errorf("assertion failed: new template version ID must not be nil") + } + + if err := sdk.UpdateActiveTemplateVersion(ctx, templateID, codersdk.UpdateActiveTemplateVersion{ID: newVersion.ID}); err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + } + } + + refreshedObj, err := s.Get(ctx, name, nil) + if err != nil { + return nil, false, err + } + + return refreshedObj, false, nil } // Delete deletes a CoderTemplate through codersdk. diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go new file mode 100644 index 00000000..6c76cc46 --- /dev/null +++ b/internal/aggregated/storage/template_files.go @@ -0,0 +1,211 @@ +package storage + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "io" + "path" + "sort" + "strings" + "unicode/utf8" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" +) + +const ( + maxTemplateSourceZipBytes = 20 << 20 // 20 MiB compressed + maxTemplateSourceTotalUncompressedBytes = 40 << 20 // 40 MiB total extracted + maxTemplateSourceFiles = 2000 + maxTemplateSourceFileBytes = 2 << 20 // 2 MiB per file +) + +// fetchTemplateSourceFiles downloads the source archive for a template version and +// returns a map of relative file paths to UTF-8 file contents. +func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, versionID uuid.UUID) (map[string]string, error) { + if ctx == nil { + return nil, fmt.Errorf("assertion failed: context must not be nil") + } + if sdk == nil { + return nil, fmt.Errorf("assertion failed: codersdk client must not be nil") + } + if versionID == uuid.Nil { + return nil, fmt.Errorf("assertion failed: template version ID must not be nil") + } + + version, err := sdk.TemplateVersion(ctx, versionID) + if err != nil { + return nil, fmt.Errorf("fetch template version %q: %w", versionID, err) + } + if version.Job.FileID == uuid.Nil { + return nil, fmt.Errorf("assertion failed: template version %q job.fileID must not be nil", versionID) + } + + sourceZip, _, err := sdk.DownloadWithFormat(ctx, version.Job.FileID, codersdk.FormatZip) + if err != nil { + return nil, fmt.Errorf("download template source zip for file %q: %w", version.Job.FileID, err) + } + if len(sourceZip) > maxTemplateSourceZipBytes { + return nil, fmt.Errorf("template source zip exceeds max size: %d > %d", len(sourceZip), maxTemplateSourceZipBytes) + } + + archiveReader, err := zip.NewReader(bytes.NewReader(sourceZip), int64(len(sourceZip))) + if err != nil { + return nil, fmt.Errorf("open template source zip: %w", err) + } + if len(archiveReader.File) > maxTemplateSourceFiles { + return nil, fmt.Errorf("template source zip contains too many entries: %d > %d", len(archiveReader.File), maxTemplateSourceFiles) + } + + files := make(map[string]string, len(archiveReader.File)) + totalUncompressedBytes := int64(0) + + for _, archiveFile := range archiveReader.File { + if archiveFile == nil { + return nil, fmt.Errorf("assertion failed: template source zip entry must not be nil") + } + if archiveFile.FileInfo().IsDir() { + continue + } + + relativePath, err := validateTemplateSourcePath(archiveFile.Name) + if err != nil { + return nil, fmt.Errorf("validate template source path %q: %w", archiveFile.Name, err) + } + if archiveFile.UncompressedSize64 > uint64(maxTemplateSourceFileBytes) { + return nil, fmt.Errorf( + "template source file %q exceeds max file size: %d > %d", + relativePath, + archiveFile.UncompressedSize64, + maxTemplateSourceFileBytes, + ) + } + + entryReader, err := archiveFile.Open() + if err != nil { + return nil, fmt.Errorf("open template source file %q: %w", relativePath, err) + } + + contents, readErr := io.ReadAll(io.LimitReader(entryReader, maxTemplateSourceFileBytes+1)) + closeErr := entryReader.Close() + if readErr != nil { + return nil, fmt.Errorf("read template source file %q: %w", relativePath, readErr) + } + if closeErr != nil { + return nil, fmt.Errorf("close template source file %q: %w", relativePath, closeErr) + } + if len(contents) > maxTemplateSourceFileBytes { + return nil, fmt.Errorf("template source file %q exceeds max file size: %d > %d", relativePath, len(contents), maxTemplateSourceFileBytes) + } + + totalUncompressedBytes += int64(len(contents)) + if totalUncompressedBytes > maxTemplateSourceTotalUncompressedBytes { + return nil, fmt.Errorf( + "template source files exceed max total size: %d > %d", + totalUncompressedBytes, + maxTemplateSourceTotalUncompressedBytes, + ) + } + + if !utf8.Valid(contents) { + continue + } + files[relativePath] = string(contents) + } + + return files, nil +} + +// buildSourceZip creates an in-memory zip archive from a file map. +func buildSourceZip(files map[string]string) ([]byte, error) { + if files == nil { + return nil, fmt.Errorf("assertion failed: files map must not be nil") + } + if len(files) > maxTemplateSourceFiles { + return nil, fmt.Errorf("template source file count exceeds limit: %d > %d", len(files), maxTemplateSourceFiles) + } + + normalizedFiles := make(map[string]string, len(files)) + paths := make([]string, 0, len(files)) + totalUncompressedBytes := int64(0) + + for requestedPath, content := range files { + normalizedPath, err := validateTemplateSourcePath(requestedPath) + if err != nil { + return nil, fmt.Errorf("validate template source path %q: %w", requestedPath, err) + } + if _, exists := normalizedFiles[normalizedPath]; exists { + return nil, fmt.Errorf("duplicate normalized template source path %q", normalizedPath) + } + if !utf8.ValidString(content) { + return nil, fmt.Errorf("template source file %q contains invalid UTF-8", normalizedPath) + } + if len(content) > maxTemplateSourceFileBytes { + return nil, fmt.Errorf("template source file %q exceeds max file size: %d > %d", normalizedPath, len(content), maxTemplateSourceFileBytes) + } + + totalUncompressedBytes += int64(len(content)) + if totalUncompressedBytes > maxTemplateSourceTotalUncompressedBytes { + return nil, fmt.Errorf( + "template source files exceed max total size: %d > %d", + totalUncompressedBytes, + maxTemplateSourceTotalUncompressedBytes, + ) + } + + normalizedFiles[normalizedPath] = content + paths = append(paths, normalizedPath) + } + + sort.Strings(paths) + + var buffer bytes.Buffer + zipWriter := zip.NewWriter(&buffer) + + for _, sourcePath := range paths { + fileWriter, err := zipWriter.Create(sourcePath) + if err != nil { + return nil, fmt.Errorf("create zip entry %q: %w", sourcePath, err) + } + if _, err := fileWriter.Write([]byte(normalizedFiles[sourcePath])); err != nil { + return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) + } + } + + if err := zipWriter.Close(); err != nil { + return nil, fmt.Errorf("close source zip writer: %w", err) + } + + result := buffer.Bytes() + if len(result) > maxTemplateSourceZipBytes { + return nil, fmt.Errorf("template source zip exceeds max size: %d > %d", len(result), maxTemplateSourceZipBytes) + } + + return result, nil +} + +func validateTemplateSourcePath(templatePath string) (string, error) { + if templatePath == "" { + return "", fmt.Errorf("path must not be empty") + } + + cleanedPath := path.Clean(templatePath) + if cleanedPath == "." || cleanedPath == "/" { + return "", fmt.Errorf("path must not resolve to root") + } + if strings.HasPrefix(cleanedPath, "/") { + return "", fmt.Errorf("path must be relative") + } + if cleanedPath == ".." || strings.HasPrefix(cleanedPath, "../") { + return "", fmt.Errorf("path must not escape root") + } + for _, component := range strings.Split(cleanedPath, "/") { + if component == ".." { + return "", fmt.Errorf("path must not contain parent directory components") + } + } + + return cleanedPath, nil +} From dfc03f7520b4b480aa9fb3866e0ac4a01d0c566e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 17:48:42 +0000 Subject: [PATCH 03/12] docs: regenerate API reference for spec.files field --- docs/reference/api/codertemplate.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/reference/api/codertemplate.md b/docs/reference/api/codertemplate.md index c1ed0af6..78f071b1 100644 --- a/docs/reference/api/codertemplate.md +++ b/docs/reference/api/codertemplate.md @@ -18,6 +18,7 @@ | `displayName` | string | | | `description` | string | | | `icon` | string | | +| `files` | object (keys:string, values:string) | Files is the template source tree for the active template version. Keys are slash-delimited relative paths (e.g. "main.tf"). Values are UTF-8 file contents. Populated on GET; intentionally omitted from LIST to keep responses small. On CREATE/UPDATE with files, the server uploads source and creates a new template version. | | `running` | boolean | Running is a legacy flag retained temporarily for in-repo callers that still read template run-state directly. | ## Status From a8b29e0d608e552a23c2de5366458ff28d12752e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 18:07:36 +0000 Subject: [PATCH 04/12] fix template file updates for binary preservation and path normalization --- internal/aggregated/storage/storage_test.go | 280 +++++++++++++++++- internal/aggregated/storage/template.go | 14 +- internal/aggregated/storage/template_files.go | 216 +++++++++++++- 3 files changed, 504 insertions(+), 6 deletions(-) diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 9830558d..953fa729 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -1,6 +1,8 @@ package storage import ( + "archive/zip" + "bytes" "context" "encoding/json" "errors" @@ -401,6 +403,176 @@ func TestTemplateStorageUpdateWithIdenticalFilesIsNoOp(t *testing.T) { } } +func TestTemplateStorageUpdatePreservesNonUTF8Files(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("expected template get to succeed: %v", err) + } + currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", currentObj) + } + if _, hasBinary := currentTemplate.Spec.Files["binary.dat"]; hasBinary { + t.Fatal("expected non-UTF8 binary.dat to be omitted from spec.files") + } + + updatedMainTF := "resource \"null_resource\" \"updated_binary_preserve\" {}" + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + activeVersionBefore, ok := state.templateActiveVersionID("acme", "starter-template") + if !ok { + t.Fatal("expected active version for starter-template") + } + + desiredTemplate := currentTemplate.DeepCopy() + desiredTemplate.Spec.Files = map[string]string{"main.tf": updatedMainTF} + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected update with changed files to succeed: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + + if state.fileCount() != fileCountBefore+1 { + t.Fatalf("expected file upload during changed-files update, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore+1 { + t.Fatalf("expected template version creation during changed-files update, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionAfter, ok := state.templateActiveVersionID("acme", "starter-template") + if !ok { + t.Fatal("expected active version for starter-template after update") + } + if activeVersionAfter == activeVersionBefore { + t.Fatalf("expected active version to change, both were %q", activeVersionAfter) + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + expectedFiles := map[string]string{"main.tf": updatedMainTF} + if !reflect.DeepEqual(updatedTemplate.Spec.Files, expectedFiles) { + t.Fatalf("expected updated files %v, got %v", expectedFiles, updatedTemplate.Spec.Files) + } + + activeSourceZip, ok := state.templateActiveSourceZip("acme", "starter-template") + if !ok { + t.Fatal("expected active source zip for starter-template") + } + zipEntries := unzipEntries(t, activeSourceZip) + + updatedMainBytes, ok := zipEntries["main.tf"] + if !ok { + t.Fatal("expected merged source zip to include main.tf") + } + if string(updatedMainBytes) != updatedMainTF { + t.Fatalf("expected merged main.tf %q, got %q", updatedMainTF, string(updatedMainBytes)) + } + + expectedBinary := []byte{0x80, 0x81, 0x82} + binaryBytes, ok := zipEntries["binary.dat"] + if !ok { + t.Fatal("expected merged source zip to preserve binary.dat") + } + if !bytes.Equal(binaryBytes, expectedBinary) { + t.Fatalf("expected preserved binary.dat bytes %v, got %v", expectedBinary, binaryBytes) + } +} + +func TestTemplateStorageUpdateNormalizesPathsForNoOp(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("expected template get to succeed: %v", err) + } + currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", currentObj) + } + + mainTF, ok := currentTemplate.Spec.Files["main.tf"] + if !ok { + t.Fatal("expected starter-template spec.files to contain main.tf") + } + + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + activeVersionBefore, ok := state.templateActiveVersionID("acme", "starter-template") + if !ok { + t.Fatal("expected active version for starter-template") + } + + desiredTemplate := currentTemplate.DeepCopy() + desiredTemplate.Spec.Files = map[string]string{"./main.tf": mainTF} + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected update with normalized-identical files to succeed: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + + if state.fileCount() != fileCountBefore { + t.Fatalf("expected no new upload for normalized-identical files, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore { + t.Fatalf("expected no new template version for normalized-identical files, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionAfter, ok := state.templateActiveVersionID("acme", "starter-template") + if !ok { + t.Fatal("expected active version for starter-template after update") + } + if activeVersionAfter != activeVersionBefore { + t.Fatalf("expected active version to remain %q on no-op update, got %q", activeVersionBefore, activeVersionAfter) + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + expectedFiles := map[string]string{"main.tf": mainTF} + if !reflect.DeepEqual(updatedTemplate.Spec.Files, expectedFiles) { + t.Fatalf("expected files %v, got %v", expectedFiles, updatedTemplate.Spec.Files) + } +} + func TestTemplateStorageUpdateMetadata(t *testing.T) { t.Parallel() @@ -1810,7 +1982,7 @@ func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) ttlMillis := int64(3600000) autostartSchedule := "CRON_TZ=UTC 0 9 * * 1-5" - seededTemplateSourceZip, seedErr := buildSourceZip(map[string]string{"main.tf": seededTemplateMainTF}) + seededTemplateSourceZip, seedErr := buildSeededTemplateSourceZip() if seedErr != nil { t.Fatalf("build seeded template source zip: %v", seedErr) } @@ -2516,6 +2688,37 @@ func (s *mockCoderServerState) templateActiveVersionID(organization, templateNam return template.ActiveVersionID, true } +func (s *mockCoderServerState) templateActiveSourceZip(organization, templateName string) ([]byte, bool) { + s.mu.Lock() + defer s.mu.Unlock() + + organizationTemplates, ok := s.templateIDsByOrg[organization] + if !ok { + return nil, false + } + templateID, ok := organizationTemplates[templateName] + if !ok { + return nil, false + } + template, ok := s.templatesByID[templateID] + if !ok { + return nil, false + } + version, ok := s.templateVersionsByID[template.ActiveVersionID] + if !ok { + return nil, false + } + if version.Job.FileID == uuid.Nil { + panic("assertion failed: template version file ID must not be nil") + } + fileData, ok := s.filesByID[version.Job.FileID] + if !ok { + return nil, false + } + + return append([]byte(nil), fileData...), true +} + func (s *mockCoderServerState) templateMetaUpdateCount() int { s.mu.Lock() defer s.mu.Unlock() @@ -2607,6 +2810,81 @@ func (s *mockCoderServerState) setBuildTransitionFailure(transition codersdk.Wor s.failBuildTransitions[transition] = statusCode } +func buildSeededTemplateSourceZip() ([]byte, error) { + var sourceZip bytes.Buffer + zipWriter := zip.NewWriter(&sourceZip) + + mainTFWriter, err := zipWriter.Create("main.tf") + if err != nil { + return nil, fmt.Errorf("create seeded main.tf zip entry: %w", err) + } + if _, err := mainTFWriter.Write([]byte(seededTemplateMainTF)); err != nil { + return nil, fmt.Errorf("write seeded main.tf zip entry: %w", err) + } + + binaryWriter, err := zipWriter.Create("binary.dat") + if err != nil { + return nil, fmt.Errorf("create seeded binary.dat zip entry: %w", err) + } + if _, err := binaryWriter.Write([]byte{0x80, 0x81, 0x82}); err != nil { + return nil, fmt.Errorf("write seeded binary.dat zip entry: %w", err) + } + + if err := zipWriter.Close(); err != nil { + return nil, fmt.Errorf("close seeded source zip writer: %w", err) + } + + result := sourceZip.Bytes() + if len(result) > maxTemplateSourceZipBytes { + return nil, fmt.Errorf("seeded template source zip exceeds max size: %d > %d", len(result), maxTemplateSourceZipBytes) + } + + return result, nil +} + +func unzipEntries(t *testing.T, sourceZip []byte) map[string][]byte { + t.Helper() + + if sourceZip == nil { + t.Fatal("assertion failed: source zip must not be nil") + } + + archiveReader, err := zip.NewReader(bytes.NewReader(sourceZip), int64(len(sourceZip))) + if err != nil { + t.Fatalf("open source zip: %v", err) + } + + entries := make(map[string][]byte, len(archiveReader.File)) + for _, archiveFile := range archiveReader.File { + if archiveFile == nil { + t.Fatal("assertion failed: source zip entry must not be nil") + } + if archiveFile.FileInfo().IsDir() { + continue + } + if _, exists := entries[archiveFile.Name]; exists { + t.Fatalf("duplicate source zip entry %q", archiveFile.Name) + } + + entryReader, err := archiveFile.Open() + if err != nil { + t.Fatalf("open source zip entry %q: %v", archiveFile.Name, err) + } + contents, readErr := io.ReadAll(entryReader) + closeErr := entryReader.Close() + if readErr != nil { + t.Fatalf("read source zip entry %q: %v", archiveFile.Name, readErr) + } + if closeErr != nil { + t.Fatalf("close source zip entry %q: %v", archiveFile.Name, closeErr) + } + + entries[archiveFile.Name] = contents + } + + return entries +} + func newTestClientProvider(t *testing.T, serverURL string) coder.ClientProvider { t.Helper() diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 476ef492..bba97a2e 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -393,6 +393,11 @@ func (s *TemplateStorage) Update( } if updatedTemplate.Spec.Files != nil { + normalizedDesiredFiles, err := normalizeFileKeys(updatedTemplate.Spec.Files) + if err != nil { + return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) + } + currentActiveVersionID, err := uuid.Parse(currentTemplate.Status.ActiveVersionID) if err != nil { return nil, false, fmt.Errorf( @@ -407,8 +412,13 @@ func (s *TemplateStorage) Update( return nil, false, fmt.Errorf("fetch current template source files: %w", err) } - if !reflect.DeepEqual(updatedTemplate.Spec.Files, currentFiles) { - zipBytes, err := buildSourceZip(updatedTemplate.Spec.Files) + if !reflect.DeepEqual(normalizedDesiredFiles, currentFiles) { + currentRawSourceZip, err := fetchRawTemplateSourceZip(ctx, sdk, currentActiveVersionID) + if err != nil { + return nil, false, fmt.Errorf("fetch current template source zip: %w", err) + } + + zipBytes, err := buildMergedSourceZip(currentRawSourceZip, normalizedDesiredFiles) if err != nil { return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) } diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index 6c76cc46..be579efe 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -22,9 +22,8 @@ const ( maxTemplateSourceFileBytes = 2 << 20 // 2 MiB per file ) -// fetchTemplateSourceFiles downloads the source archive for a template version and -// returns a map of relative file paths to UTF-8 file contents. -func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, versionID uuid.UUID) (map[string]string, error) { +// fetchRawTemplateSourceZip downloads the raw source archive for a template version. +func fetchRawTemplateSourceZip(ctx context.Context, sdk *codersdk.Client, versionID uuid.UUID) ([]byte, error) { if ctx == nil { return nil, fmt.Errorf("assertion failed: context must not be nil") } @@ -51,6 +50,27 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version return nil, fmt.Errorf("template source zip exceeds max size: %d > %d", len(sourceZip), maxTemplateSourceZipBytes) } + return sourceZip, nil +} + +// fetchTemplateSourceFiles downloads the source archive for a template version and +// returns a map of relative file paths to UTF-8 file contents. +func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, versionID uuid.UUID) (map[string]string, error) { + if ctx == nil { + return nil, fmt.Errorf("assertion failed: context must not be nil") + } + if sdk == nil { + return nil, fmt.Errorf("assertion failed: codersdk client must not be nil") + } + if versionID == uuid.Nil { + return nil, fmt.Errorf("assertion failed: template version ID must not be nil") + } + + sourceZip, err := fetchRawTemplateSourceZip(ctx, sdk, versionID) + if err != nil { + return nil, err + } + archiveReader, err := zip.NewReader(bytes.NewReader(sourceZip), int64(len(sourceZip))) if err != nil { return nil, fmt.Errorf("open template source zip: %w", err) @@ -118,6 +138,196 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version return files, nil } +// normalizeFileKeys validates and normalizes source file paths. +func normalizeFileKeys(files map[string]string) (map[string]string, error) { + if files == nil { + return nil, fmt.Errorf("assertion failed: files map must not be nil") + } + + normalizedFiles := make(map[string]string, len(files)) + for requestedPath, content := range files { + normalizedPath, err := validateTemplateSourcePath(requestedPath) + if err != nil { + return nil, fmt.Errorf("validate template source path %q: %w", requestedPath, err) + } + if _, exists := normalizedFiles[normalizedPath]; exists { + return nil, fmt.Errorf("duplicate normalized template source path %q", normalizedPath) + } + normalizedFiles[normalizedPath] = content + } + + return normalizedFiles, nil +} + +// buildMergedSourceZip merges desired UTF-8 files with binary files from an existing source zip. +func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]string) ([]byte, error) { + if originalZipBytes == nil { + return nil, fmt.Errorf("assertion failed: original source zip bytes must not be nil") + } + if desiredFiles == nil { + return nil, fmt.Errorf("assertion failed: desired files map must not be nil") + } + + normalizedDesiredFiles, err := normalizeFileKeys(desiredFiles) + if err != nil { + return nil, err + } + if len(normalizedDesiredFiles) > maxTemplateSourceFiles { + return nil, fmt.Errorf("template source file count exceeds limit: %d > %d", len(normalizedDesiredFiles), maxTemplateSourceFiles) + } + + archiveReader, err := zip.NewReader(bytes.NewReader(originalZipBytes), int64(len(originalZipBytes))) + if err != nil { + return nil, fmt.Errorf("open template source zip: %w", err) + } + if len(archiveReader.File) > maxTemplateSourceFiles { + return nil, fmt.Errorf("template source zip contains too many entries: %d > %d", len(archiveReader.File), maxTemplateSourceFiles) + } + + preservedBinaryFiles := make(map[string][]byte) + totalUncompressedBytes := int64(0) + + for _, archiveFile := range archiveReader.File { + if archiveFile == nil { + return nil, fmt.Errorf("assertion failed: template source zip entry must not be nil") + } + if archiveFile.FileInfo().IsDir() { + continue + } + + relativePath, err := validateTemplateSourcePath(archiveFile.Name) + if err != nil { + return nil, fmt.Errorf("validate template source path %q: %w", archiveFile.Name, err) + } + if _, exists := normalizedDesiredFiles[relativePath]; exists { + continue + } + if archiveFile.UncompressedSize64 > uint64(maxTemplateSourceFileBytes) { + return nil, fmt.Errorf( + "template source file %q exceeds max file size: %d > %d", + relativePath, + archiveFile.UncompressedSize64, + maxTemplateSourceFileBytes, + ) + } + + entryReader, err := archiveFile.Open() + if err != nil { + return nil, fmt.Errorf("open template source file %q: %w", relativePath, err) + } + + contents, readErr := io.ReadAll(io.LimitReader(entryReader, maxTemplateSourceFileBytes+1)) + closeErr := entryReader.Close() + if readErr != nil { + return nil, fmt.Errorf("read template source file %q: %w", relativePath, readErr) + } + if closeErr != nil { + return nil, fmt.Errorf("close template source file %q: %w", relativePath, closeErr) + } + if len(contents) > maxTemplateSourceFileBytes { + return nil, fmt.Errorf("template source file %q exceeds max file size: %d > %d", relativePath, len(contents), maxTemplateSourceFileBytes) + } + + totalUncompressedBytes += int64(len(contents)) + if totalUncompressedBytes > maxTemplateSourceTotalUncompressedBytes { + return nil, fmt.Errorf( + "template source files exceed max total size: %d > %d", + totalUncompressedBytes, + maxTemplateSourceTotalUncompressedBytes, + ) + } + + if utf8.Valid(contents) { + continue + } + if _, exists := preservedBinaryFiles[relativePath]; exists { + return nil, fmt.Errorf("duplicate normalized template source path %q", relativePath) + } + preservedBinaryFiles[relativePath] = contents + } + + for sourcePath, content := range normalizedDesiredFiles { + if !utf8.ValidString(content) { + return nil, fmt.Errorf("template source file %q contains invalid UTF-8", sourcePath) + } + if len(content) > maxTemplateSourceFileBytes { + return nil, fmt.Errorf("template source file %q exceeds max file size: %d > %d", sourcePath, len(content), maxTemplateSourceFileBytes) + } + + totalUncompressedBytes += int64(len(content)) + if totalUncompressedBytes > maxTemplateSourceTotalUncompressedBytes { + return nil, fmt.Errorf( + "template source files exceed max total size: %d > %d", + totalUncompressedBytes, + maxTemplateSourceTotalUncompressedBytes, + ) + } + } + + if len(normalizedDesiredFiles)+len(preservedBinaryFiles) > maxTemplateSourceFiles { + return nil, fmt.Errorf( + "template source file count exceeds limit: %d > %d", + len(normalizedDesiredFiles)+len(preservedBinaryFiles), + maxTemplateSourceFiles, + ) + } + + paths := make([]string, 0, len(normalizedDesiredFiles)+len(preservedBinaryFiles)) + for sourcePath := range normalizedDesiredFiles { + paths = append(paths, sourcePath) + } + for sourcePath := range preservedBinaryFiles { + if _, exists := normalizedDesiredFiles[sourcePath]; exists { + return nil, fmt.Errorf("assertion failed: source path %q exists in both desired and preserved maps", sourcePath) + } + paths = append(paths, sourcePath) + } + sort.Strings(paths) + + var buffer bytes.Buffer + zipWriter := zip.NewWriter(&buffer) + + for _, sourcePath := range paths { + fileWriter, err := zipWriter.Create(sourcePath) + if err != nil { + return nil, fmt.Errorf("create zip entry %q: %w", sourcePath, err) + } + + desiredContent, hasDesired := normalizedDesiredFiles[sourcePath] + preservedBytes, hasPreservedBinary := preservedBinaryFiles[sourcePath] + if hasDesired == hasPreservedBinary { + return nil, fmt.Errorf( + "assertion failed: expected exactly one source entry for %q (desired=%t preservedBinary=%t)", + sourcePath, + hasDesired, + hasPreservedBinary, + ) + } + + if hasDesired { + if _, err := fileWriter.Write([]byte(desiredContent)); err != nil { + return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) + } + continue + } + + if _, err := fileWriter.Write(preservedBytes); err != nil { + return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) + } + } + + if err := zipWriter.Close(); err != nil { + return nil, fmt.Errorf("close source zip writer: %w", err) + } + + result := buffer.Bytes() + if len(result) > maxTemplateSourceZipBytes { + return nil, fmt.Errorf("template source zip exceeds max size: %d > %d", len(result), maxTemplateSourceZipBytes) + } + + return result, nil +} + // buildSourceZip creates an in-memory zip archive from a file map. func buildSourceZip(files map[string]string) ([]byte, error) { if files == nil { From 98564f6cde3ea48a6777ed423cfb9d1d29134c34 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 18:22:45 +0000 Subject: [PATCH 05/12] fix: verify template active version promotion on update --- internal/aggregated/storage/storage_test.go | 118 +++++++++++++++++++- internal/aggregated/storage/template.go | 15 +++ 2 files changed, 129 insertions(+), 4 deletions(-) diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 953fa729..10644582 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -326,6 +326,84 @@ func TestTemplateStorageUpdateWithChangedFiles(t *testing.T) { } } +func TestTemplateStorageUpdateVerifiesActiveVersionPromotion(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + initialFiles := map[string]string{"main.tf": "resource \"null_resource\" \"initial\" {}"} + createObj := &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.verify-promotion-template"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + DisplayName: "Verify Promotion Template", + Files: cloneStringMap(initialFiles), + }, + } + + createdObj, err := templateStorage.Create(ctx, createObj, rest.ValidateAllObjectFunc, nil) + if err != nil { + t.Fatalf("expected template create with files to succeed: %v", err) + } + createdTemplate, ok := createdObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from create, got %T", createdObj) + } + + activeVersionBefore, ok := state.templateActiveVersionID("acme", "verify-promotion-template") + if !ok { + t.Fatal("expected active version for created template") + } + fileCountBefore := state.fileCount() + templateVersionCountBefore := state.templateVersionCount() + state.setFailActiveVersionPromotion(true) + + updatedFiles := map[string]string{"main.tf": "resource \"null_resource\" \"updated\" {}"} + desiredTemplate := createdTemplate.DeepCopy() + desiredTemplate.Spec.Files = cloneStringMap(updatedFiles) + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err == nil { + t.Fatal("expected update to fail when active version promotion is silently ignored") + } + if updatedObj != nil { + t.Fatalf("expected update object to be nil on failure, got %T", updatedObj) + } + if created { + t.Fatal("expected update created=false") + } + if !strings.Contains(err.Error(), "active version promotion did not take effect") { + t.Fatalf("expected promotion verification error, got %v", err) + } + + if state.fileCount() != fileCountBefore+1 { + t.Fatalf("expected file upload before promotion verification failure, before=%d after=%d", fileCountBefore, state.fileCount()) + } + if state.templateVersionCount() != templateVersionCountBefore+1 { + t.Fatalf("expected template version creation before promotion verification failure, before=%d after=%d", templateVersionCountBefore, state.templateVersionCount()) + } + + activeVersionAfter, ok := state.templateActiveVersionID("acme", "verify-promotion-template") + if !ok { + t.Fatal("expected active version for template after failed update") + } + if activeVersionAfter != activeVersionBefore { + t.Fatalf("expected active version to remain %q after failed promotion, got %q", activeVersionBefore, activeVersionAfter) + } +} + func TestTemplateStorageUpdateWithIdenticalFilesIsNoOp(t *testing.T) { t.Parallel() @@ -1964,9 +2042,10 @@ type mockCoderServerState struct { workspacesByID map[uuid.UUID]codersdk.Workspace workspaceIDsByUser map[string]map[string]uuid.UUID - buildTransitions []codersdk.WorkspaceTransition - failBuildTransitions map[codersdk.WorkspaceTransition]int - templateMetaPatchCall int + buildTransitions []codersdk.WorkspaceTransition + failBuildTransitions map[codersdk.WorkspaceTransition]int + templateMetaPatchCall int + failActiveVersionPromotion bool } func newMockCoderServer(t *testing.T) (*httptest.Server, *mockCoderServerState) { @@ -2097,6 +2176,9 @@ func (s *mockCoderServerState) handleRequest(t *testing.T, w http.ResponseWriter case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "templates") && len(segments) == 3: s.handleListTemplates(w) return + case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "templates") && len(segments) == 4: + s.handleGetTemplate(w, segments[3]) + return case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "organizations") && len(segments) == 6 && segments[4] == "templates": s.handleGetTemplateByName(w, segments[3], segments[5]) return @@ -2172,6 +2254,25 @@ func (s *mockCoderServerState) handleListTemplates(w http.ResponseWriter) { writeJSON(w, http.StatusOK, templates) } +func (s *mockCoderServerState) handleGetTemplate(w http.ResponseWriter, templateIDSegment string) { + s.mu.Lock() + defer s.mu.Unlock() + + templateID, err := uuid.Parse(templateIDSegment) + if err != nil { + writeCoderError(w, http.StatusBadRequest, fmt.Sprintf("invalid template id %q", templateIDSegment)) + return + } + + template, ok := s.templatesByID[templateID] + if !ok { + writeCoderError(w, http.StatusNotFound, "template not found") + return + } + + writeJSON(w, http.StatusOK, template) +} + func (s *mockCoderServerState) handleGetTemplateByName(w http.ResponseWriter, orgSegment, templateName string) { s.mu.Lock() defer s.mu.Unlock() @@ -2409,7 +2510,9 @@ func (s *mockCoderServerState) handleUpdateActiveTemplateVersion(w http.Response s.templateVersionsByID[templateVersion.ID] = templateVersion } - template.ActiveVersionID = request.ID + if !s.failActiveVersionPromotion { + template.ActiveVersionID = request.ID + } template.UpdatedAt = time.Now().UTC() s.templatesByID[templateID] = template @@ -2726,6 +2829,13 @@ func (s *mockCoderServerState) templateMetaUpdateCount() int { return s.templateMetaPatchCall } +func (s *mockCoderServerState) setFailActiveVersionPromotion(fail bool) { + s.mu.Lock() + defer s.mu.Unlock() + + s.failActiveVersionPromotion = fail +} + func (s *mockCoderServerState) setTemplateVersionTemplateID(templateVersionID, templateID uuid.UUID) { if templateVersionID == uuid.Nil { panic("assertion failed: template version ID must not be nil") diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index bba97a2e..868f5a97 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -452,6 +452,21 @@ func (s *TemplateStorage) Update( if err := sdk.UpdateActiveTemplateVersion(ctx, templateID, codersdk.UpdateActiveTemplateVersion{ID: newVersion.ID}); err != nil { return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) } + + // Post-condition: verify promotion succeeded. The vendored SDK silently + // swallows transport errors in UpdateActiveTemplateVersion, so we must + // confirm the active version actually changed. + verifyTemplate, err := sdk.Template(ctx, templateID) + if err != nil { + return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + } + if verifyTemplate.ActiveVersionID != newVersion.ID { + return nil, false, fmt.Errorf( + "assertion failed: active version promotion did not take effect: expected %q, got %q", + newVersion.ID.String(), + verifyTemplate.ActiveVersionID.String(), + ) + } } } From 6e890c4ec50c8e06279fa72b3c361a7384ac92c0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 18:34:22 +0000 Subject: [PATCH 06/12] fix: cap template source zip download reads --- internal/aggregated/storage/template_files.go | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index be579efe..bf2e74d2 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -6,6 +6,7 @@ import ( "context" "fmt" "io" + "net/http" "path" "sort" "strings" @@ -42,10 +43,32 @@ func fetchRawTemplateSourceZip(ctx context.Context, sdk *codersdk.Client, versio return nil, fmt.Errorf("assertion failed: template version %q job.fileID must not be nil", versionID) } - sourceZip, _, err := sdk.DownloadWithFormat(ctx, version.Job.FileID, codersdk.FormatZip) + resp, err := sdk.Request( + ctx, + http.MethodGet, + fmt.Sprintf("/api/v2/files/%s", version.Job.FileID), + nil, + codersdk.WithQueryParam("format", codersdk.FormatZip), + ) if err != nil { return nil, fmt.Errorf("download template source zip for file %q: %w", version.Job.FileID, err) } + if resp.StatusCode != http.StatusOK { + responseErr := codersdk.ReadBodyAsError(resp) + if closeErr := resp.Body.Close(); closeErr != nil { + return nil, fmt.Errorf("close template source zip response body for file %q: %w", version.Job.FileID, closeErr) + } + return nil, fmt.Errorf("download template source zip for file %q: %w", version.Job.FileID, responseErr) + } + + sourceZip, readErr := io.ReadAll(io.LimitReader(resp.Body, int64(maxTemplateSourceZipBytes)+1)) + closeErr := resp.Body.Close() + if readErr != nil { + return nil, fmt.Errorf("read template source zip for file %q: %w", version.Job.FileID, readErr) + } + if closeErr != nil { + return nil, fmt.Errorf("close template source zip response body for file %q: %w", version.Job.FileID, closeErr) + } if len(sourceZip) > maxTemplateSourceZipBytes { return nil, fmt.Errorf("template source zip exceeds max size: %d > %d", len(sourceZip), maxTemplateSourceZipBytes) } From 5b118a4b9d49d18e90dce8a67ed00d947ed574e5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 18:50:35 +0000 Subject: [PATCH 07/12] fix: preserve template zip metadata and reject versionID updates --- internal/aggregated/storage/storage_test.go | 63 ++++++++++++++----- internal/aggregated/storage/template.go | 12 ++++ internal/aggregated/storage/template_files.go | 33 +++++++--- 3 files changed, 83 insertions(+), 25 deletions(-) diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 10644582..e1506840 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "io" + "io/fs" "net/http" "net/http/httptest" "net/url" @@ -575,6 +576,13 @@ func TestTemplateStorageUpdatePreservesNonUTF8Files(t *testing.T) { if !bytes.Equal(binaryBytes, expectedBinary) { t.Fatalf("expected preserved binary.dat bytes %v, got %v", expectedBinary, binaryBytes) } + binaryMode, ok := zipEntryMode(t, activeSourceZip, "binary.dat") + if !ok { + t.Fatal("expected merged source zip mode metadata for binary.dat") + } + if binaryMode.Perm() != fs.FileMode(0o755) { + t.Fatalf("expected preserved binary.dat mode 0755, got %v", binaryMode) + } } func TestTemplateStorageUpdateNormalizesPathsForNoOp(t *testing.T) { @@ -839,7 +847,7 @@ func TestTemplateStorageUpdateReturnsCurrentBackendObjectForLegacyRunningField(t } } -func TestTemplateStorageUpdateAllowsEmptyVersionIDWhenTogglingRunning(t *testing.T) { +func TestTemplateStorageUpdateAllowsUnchangedVersionIDWhenTogglingRunning(t *testing.T) { t.Parallel() server, _ := newMockCoderServer(t) @@ -863,7 +871,6 @@ func TestTemplateStorageUpdateAllowsEmptyVersionIDWhenTogglingRunning(t *testing desiredTemplate := currentTemplate.DeepCopy() desiredTemplate.Spec.Running = !currentTemplate.Spec.Running - desiredTemplate.Spec.VersionID = "" updatedObj, created, err := templateStorage.Update( ctx, @@ -875,7 +882,7 @@ func TestTemplateStorageUpdateAllowsEmptyVersionIDWhenTogglingRunning(t *testing nil, ) if err != nil { - t.Fatalf("expected template update to succeed when desired spec.versionID is empty: %v", err) + t.Fatalf("expected template update to succeed when spec.versionID is unchanged: %v", err) } if created { t.Fatal("expected update created=false") @@ -955,7 +962,7 @@ func TestTemplateStorageUpdateAllowsEmptyOptionalFieldsWhenTogglingRunning(t *te } } -func TestTemplateStorageUpdateIgnoresDifferentVersionID(t *testing.T) { +func TestTemplateStorageUpdateRejectsVersionIDChange(t *testing.T) { t.Parallel() server, _ := newMockCoderServer(t) @@ -981,7 +988,7 @@ func TestTemplateStorageUpdateIgnoresDifferentVersionID(t *testing.T) { t.Fatal("expected test fixture to use a different spec.versionID") } - updatedObj, created, err := templateStorage.Update( + _, created, err := templateStorage.Update( ctx, desiredTemplate.Name, testUpdatedObjectInfo{obj: desiredTemplate}, @@ -990,20 +997,15 @@ func TestTemplateStorageUpdateIgnoresDifferentVersionID(t *testing.T) { false, nil, ) - if err != nil { - t.Fatalf("expected template update to succeed when changing spec.versionID: %v", err) + if !apierrors.IsBadRequest(err) { + t.Fatalf("expected BadRequest when changing spec.versionID, got %v", err) + } + if err == nil || !strings.Contains(err.Error(), "spec.versionID is read-only") { + t.Fatalf("expected read-only spec.versionID error, got %v", err) } if created { t.Fatal("expected update created=false") } - - updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) - if !ok { - t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) - } - if updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { - t.Fatalf("expected returned spec.versionID %q from backend, got %q", currentTemplate.Spec.VersionID, updatedTemplate.Spec.VersionID) - } } func TestTemplateStorageUpdateAllowsMetadataSpecChanges(t *testing.T) { @@ -2932,7 +2934,9 @@ func buildSeededTemplateSourceZip() ([]byte, error) { return nil, fmt.Errorf("write seeded main.tf zip entry: %w", err) } - binaryWriter, err := zipWriter.Create("binary.dat") + binaryHeader := zip.FileHeader{Name: "binary.dat", Method: zip.Deflate} + binaryHeader.SetMode(fs.FileMode(0o755)) + binaryWriter, err := zipWriter.CreateHeader(&binaryHeader) if err != nil { return nil, fmt.Errorf("create seeded binary.dat zip entry: %w", err) } @@ -2995,6 +2999,33 @@ func unzipEntries(t *testing.T, sourceZip []byte) map[string][]byte { return entries } +func zipEntryMode(t *testing.T, sourceZip []byte, path string) (fs.FileMode, bool) { + t.Helper() + + if sourceZip == nil { + t.Fatal("assertion failed: source zip must not be nil") + } + if path == "" { + t.Fatal("assertion failed: path must not be empty") + } + + archiveReader, err := zip.NewReader(bytes.NewReader(sourceZip), int64(len(sourceZip))) + if err != nil { + t.Fatalf("open source zip: %v", err) + } + + for _, archiveFile := range archiveReader.File { + if archiveFile == nil { + t.Fatal("assertion failed: source zip entry must not be nil") + } + if archiveFile.Name == path { + return archiveFile.Mode(), true + } + } + + return 0, false +} + func newTestClientProvider(t *testing.T, serverURL string) coder.ClientProvider { t.Helper() diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 868f5a97..15d13115 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -372,6 +372,18 @@ func (s *TemplateStorage) Update( ) } + // spec.versionID is informational (populated from the backend active version). + // Reject explicit mutations to avoid silent drift when the active version remains unchanged. + if updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { + return nil, false, apierrors.NewBadRequest( + fmt.Sprintf( + "spec.versionID is read-only; to change the active version, update spec.files instead (current: %q, requested: %q)", + currentTemplate.Spec.VersionID, + updatedTemplate.Spec.VersionID, + ), + ) + } + templateID, err := uuid.Parse(currentTemplate.Status.ID) if err != nil { return nil, false, fmt.Errorf("parse current template status.id %q: %w", currentTemplate.Status.ID, err) diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index bf2e74d2..77efdb77 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -207,7 +207,11 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin return nil, fmt.Errorf("template source zip contains too many entries: %d > %d", len(archiveReader.File), maxTemplateSourceFiles) } - preservedBinaryFiles := make(map[string][]byte) + type preservedBinaryEntry struct { + contents []byte + header zip.FileHeader + } + preservedBinaryFiles := make(map[string]preservedBinaryEntry) totalUncompressedBytes := int64(0) for _, archiveFile := range archiveReader.File { @@ -266,7 +270,13 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin if _, exists := preservedBinaryFiles[relativePath]; exists { return nil, fmt.Errorf("duplicate normalized template source path %q", relativePath) } - preservedBinaryFiles[relativePath] = contents + + header := archiveFile.FileHeader + header.Name = relativePath + preservedBinaryFiles[relativePath] = preservedBinaryEntry{ + contents: contents, + header: header, + } } for sourcePath, content := range normalizedDesiredFiles { @@ -311,13 +321,8 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin zipWriter := zip.NewWriter(&buffer) for _, sourcePath := range paths { - fileWriter, err := zipWriter.Create(sourcePath) - if err != nil { - return nil, fmt.Errorf("create zip entry %q: %w", sourcePath, err) - } - desiredContent, hasDesired := normalizedDesiredFiles[sourcePath] - preservedBytes, hasPreservedBinary := preservedBinaryFiles[sourcePath] + preservedEntry, hasPreservedBinary := preservedBinaryFiles[sourcePath] if hasDesired == hasPreservedBinary { return nil, fmt.Errorf( "assertion failed: expected exactly one source entry for %q (desired=%t preservedBinary=%t)", @@ -328,13 +333,23 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin } if hasDesired { + fileWriter, err := zipWriter.Create(sourcePath) + if err != nil { + return nil, fmt.Errorf("create zip entry %q: %w", sourcePath, err) + } if _, err := fileWriter.Write([]byte(desiredContent)); err != nil { return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) } continue } - if _, err := fileWriter.Write(preservedBytes); err != nil { + header := preservedEntry.header + header.Name = sourcePath + fileWriter, err := zipWriter.CreateHeader(&header) + if err != nil { + return nil, fmt.Errorf("create zip entry %q from original header: %w", sourcePath, err) + } + if _, err := fileWriter.Write(preservedEntry.contents); err != nil { return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) } } From 84df43ce9b914c2a33ad1fa860a134431c1bb7b7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 19:03:39 +0000 Subject: [PATCH 08/12] fix: preserve zip headers for updated UTF-8 files --- internal/aggregated/storage/storage_test.go | 24 +++++++++++++- internal/aggregated/storage/template_files.go | 31 +++++++++++++++++-- 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index e1506840..62dc70cc 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -503,6 +503,18 @@ func TestTemplateStorageUpdatePreservesNonUTF8Files(t *testing.T) { t.Fatal("expected non-UTF8 binary.dat to be omitted from spec.files") } + originalSourceZip, ok := state.templateActiveSourceZip("acme", "starter-template") + if !ok { + t.Fatal("expected active source zip for starter-template before update") + } + mainModeBefore, ok := zipEntryMode(t, originalSourceZip, "main.tf") + if !ok { + t.Fatal("expected seeded source zip mode metadata for main.tf") + } + if mainModeBefore.Perm() != fs.FileMode(0o755) { + t.Fatalf("expected seeded main.tf mode 0755, got %v", mainModeBefore) + } + updatedMainTF := "resource \"null_resource\" \"updated_binary_preserve\" {}" fileCountBefore := state.fileCount() templateVersionCountBefore := state.templateVersionCount() @@ -568,6 +580,14 @@ func TestTemplateStorageUpdatePreservesNonUTF8Files(t *testing.T) { t.Fatalf("expected merged main.tf %q, got %q", updatedMainTF, string(updatedMainBytes)) } + updatedMainMode, ok := zipEntryMode(t, activeSourceZip, "main.tf") + if !ok { + t.Fatal("expected merged source zip mode metadata for main.tf") + } + if updatedMainMode.Perm() != fs.FileMode(0o755) { + t.Fatalf("expected preserved main.tf mode 0755, got %v", updatedMainMode) + } + expectedBinary := []byte{0x80, 0x81, 0x82} binaryBytes, ok := zipEntries["binary.dat"] if !ok { @@ -2926,7 +2946,9 @@ func buildSeededTemplateSourceZip() ([]byte, error) { var sourceZip bytes.Buffer zipWriter := zip.NewWriter(&sourceZip) - mainTFWriter, err := zipWriter.Create("main.tf") + mainTFHeader := zip.FileHeader{Name: "main.tf", Method: zip.Deflate} + mainTFHeader.SetMode(fs.FileMode(0o755)) + mainTFWriter, err := zipWriter.CreateHeader(&mainTFHeader) if err != nil { return nil, fmt.Errorf("create seeded main.tf zip entry: %w", err) } diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index 77efdb77..bd80bc89 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -212,6 +212,7 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin header zip.FileHeader } preservedBinaryFiles := make(map[string]preservedBinaryEntry) + originalHeaders := make(map[string]zip.FileHeader, len(archiveReader.File)) totalUncompressedBytes := int64(0) for _, archiveFile := range archiveReader.File { @@ -226,6 +227,12 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin if err != nil { return nil, fmt.Errorf("validate template source path %q: %w", archiveFile.Name, err) } + if _, exists := originalHeaders[relativePath]; exists { + return nil, fmt.Errorf("duplicate normalized template source path %q", relativePath) + } + header := archiveFile.FileHeader + header.Name = relativePath + originalHeaders[relativePath] = header if _, exists := normalizedDesiredFiles[relativePath]; exists { continue } @@ -271,11 +278,9 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin return nil, fmt.Errorf("duplicate normalized template source path %q", relativePath) } - header := archiveFile.FileHeader - header.Name = relativePath preservedBinaryFiles[relativePath] = preservedBinaryEntry{ contents: contents, - header: header, + header: originalHeaders[relativePath], } } @@ -333,6 +338,26 @@ func buildMergedSourceZip(originalZipBytes []byte, desiredFiles map[string]strin } if hasDesired { + if originalHeader, exists := originalHeaders[sourcePath]; exists { + header := originalHeader + header.Name = sourcePath + // Header sizes are specific to the original contents. Reset them so + // the zip writer recomputes values for the updated file bytes. + header.CompressedSize64 = 0 + header.UncompressedSize64 = 0 + //nolint:staticcheck // archive/zip still consumes legacy 32-bit size fields. + header.CompressedSize = 0 + //nolint:staticcheck // archive/zip still consumes legacy 32-bit size fields. + header.UncompressedSize = 0 + fileWriter, err := zipWriter.CreateHeader(&header) + if err != nil { + return nil, fmt.Errorf("create zip entry %q from original header: %w", sourcePath, err) + } + if _, err := fileWriter.Write([]byte(desiredContent)); err != nil { + return nil, fmt.Errorf("write zip entry %q: %w", sourcePath, err) + } + continue + } fileWriter, err := zipWriter.Create(sourcePath) if err != nil { return nil, fmt.Errorf("create zip entry %q: %w", sourcePath, err) From fd380c9aab12011f4a01862414082b3160805aca Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 19:14:47 +0000 Subject: [PATCH 09/12] fix: validate template files before metadata update --- internal/aggregated/storage/template.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 15d13115..0b3f1e46 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -394,6 +394,20 @@ func (s *TemplateStorage) Update( return nil, false, wrapClientError(err) } + // Pre-validate spec.files before any mutations to avoid partial updates. + var normalizedDesiredFiles map[string]string + if updatedTemplate.Spec.Files != nil { + var normalizeErr error + normalizedDesiredFiles, normalizeErr = normalizeFileKeys(updatedTemplate.Spec.Files) + if normalizeErr != nil { + return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", normalizeErr)) + } + // Validate that files can be built into a zip (path/size/UTF-8 checks). + if _, buildErr := buildSourceZip(normalizedDesiredFiles); buildErr != nil { + return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", buildErr)) + } + } + metadataChanged := updatedTemplate.Spec.DisplayName != currentTemplate.Spec.DisplayName || updatedTemplate.Spec.Description != currentTemplate.Spec.Description || updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon @@ -405,9 +419,8 @@ func (s *TemplateStorage) Update( } if updatedTemplate.Spec.Files != nil { - normalizedDesiredFiles, err := normalizeFileKeys(updatedTemplate.Spec.Files) - if err != nil { - return nil, false, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) + if normalizedDesiredFiles == nil { + return nil, false, fmt.Errorf("assertion failed: normalized desired template files must not be nil when spec.files is provided") } currentActiveVersionID, err := uuid.Parse(currentTemplate.Status.ActiveVersionID) From c9b42a34d8ff96b62979172349193f509b2b5567 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 19:24:59 +0000 Subject: [PATCH 10/12] fix: allow empty template versionID updates --- internal/aggregated/storage/storage_test.go | 54 +++++++++++++++++++ internal/aggregated/storage/template.go | 5 +- internal/aggregated/storage/template_files.go | 3 ++ 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 62dc70cc..31cf4ac9 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -920,6 +920,60 @@ func TestTemplateStorageUpdateAllowsUnchangedVersionIDWhenTogglingRunning(t *tes } } +func TestTemplateStorageUpdateAllowsEmptyVersionIDWhenTogglingRunning(t *testing.T) { + t.Parallel() + + server, _ := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("expected template get to succeed: %v", err) + } + + currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from get, got %T", currentObj) + } + if currentTemplate.Spec.VersionID == "" { + t.Fatal("expected current template spec.versionID to be populated") + } + + desiredTemplate := currentTemplate.DeepCopy() + desiredTemplate.Spec.VersionID = "" + desiredTemplate.Spec.Running = !currentTemplate.Spec.Running + + updatedObj, created, err := templateStorage.Update( + ctx, + desiredTemplate.Name, + testUpdatedObjectInfo{obj: desiredTemplate}, + nil, + rest.ValidateAllObjectUpdateFunc, + false, + nil, + ) + if err != nil { + t.Fatalf("expected template update to succeed when desired spec.versionID is empty: %v", err) + } + if created { + t.Fatal("expected update created=false") + } + + updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if updatedTemplate.Spec.Running != currentTemplate.Spec.Running { + t.Fatalf("expected update response running=%t from current backend object, got %t", currentTemplate.Spec.Running, updatedTemplate.Spec.Running) + } + if updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { + t.Fatalf("expected update response spec.versionID %q from current backend object, got %q", currentTemplate.Spec.VersionID, updatedTemplate.Spec.VersionID) + } +} + func TestTemplateStorageUpdateAllowsEmptyOptionalFieldsWhenTogglingRunning(t *testing.T) { t.Parallel() diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 0b3f1e46..8f2a1c2f 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -373,8 +373,9 @@ func (s *TemplateStorage) Update( } // spec.versionID is informational (populated from the backend active version). - // Reject explicit mutations to avoid silent drift when the active version remains unchanged. - if updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { + // Reject explicit non-empty mutations to avoid silent drift when the active version remains unchanged. + // Allow empty desired values so GitOps clients can omit this informational field on updates. + if updatedTemplate.Spec.VersionID != "" && updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID { return nil, false, apierrors.NewBadRequest( fmt.Sprintf( "spec.versionID is read-only; to change the active version, update spec.files instead (current: %q, requested: %q)", diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index bd80bc89..fc5e4505 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -155,6 +155,9 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version if !utf8.Valid(contents) { continue } + if _, exists := files[relativePath]; exists { + return nil, fmt.Errorf("duplicate normalized path %q in template source zip", relativePath) + } files[relativePath] = string(contents) } From aa23172add9fc88ae98e9052945c3a051feca821 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 19:36:33 +0000 Subject: [PATCH 11/12] fix: detect duplicate normalized paths before UTF-8 filtering --- internal/aggregated/storage/template_files.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index fc5e4505..6a1948c1 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -103,6 +103,7 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version } files := make(map[string]string, len(archiveReader.File)) + seenPaths := make(map[string]struct{}, len(archiveReader.File)) totalUncompressedBytes := int64(0) for _, archiveFile := range archiveReader.File { @@ -117,6 +118,10 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version if err != nil { return nil, fmt.Errorf("validate template source path %q: %w", archiveFile.Name, err) } + if _, seen := seenPaths[relativePath]; seen { + return nil, fmt.Errorf("duplicate normalized path %q in template source zip", relativePath) + } + seenPaths[relativePath] = struct{}{} if archiveFile.UncompressedSize64 > uint64(maxTemplateSourceFileBytes) { return nil, fmt.Errorf( "template source file %q exceeds max file size: %d > %d", @@ -155,9 +160,6 @@ func fetchTemplateSourceFiles(ctx context.Context, sdk *codersdk.Client, version if !utf8.Valid(contents) { continue } - if _, exists := files[relativePath]; exists { - return nil, fmt.Errorf("duplicate normalized path %q in template source zip", relativePath) - } files[relativePath] = string(contents) } From e9828aa3d25a0fc084d9accfeb44aba9b122a5de Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 12 Feb 2026 19:46:42 +0000 Subject: [PATCH 12/12] fix: handle empty template files map and reject backslashes --- internal/aggregated/storage/template.go | 2 +- internal/aggregated/storage/template_files.go | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 8f2a1c2f..b9c66e6d 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -227,7 +227,7 @@ func (s *TemplateStorage) Create( return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } - if len(templateObj.Spec.Files) > 0 { + if templateObj.Spec.Files != nil { zipBytes, err := buildSourceZip(templateObj.Spec.Files) if err != nil { return nil, apierrors.NewBadRequest(fmt.Sprintf("invalid template spec.files: %v", err)) diff --git a/internal/aggregated/storage/template_files.go b/internal/aggregated/storage/template_files.go index 6a1948c1..58746ec3 100644 --- a/internal/aggregated/storage/template_files.go +++ b/internal/aggregated/storage/template_files.go @@ -469,6 +469,10 @@ func validateTemplateSourcePath(templatePath string) (string, error) { return "", fmt.Errorf("path must not be empty") } + if strings.ContainsRune(templatePath, '\\') { + return "", fmt.Errorf("path must not contain backslashes") + } + cleanedPath := path.Clean(templatePath) if cleanedPath == "." || cleanedPath == "/" { return "", fmt.Errorf("path must not resolve to root")