From cf296a055e2bbf8ef304b644b1b7b260692a6d3f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 13 Feb 2026 16:53:02 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20enable=20best-effort=20SS?= =?UTF-8?q?A=20create-on-update?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow aggregated storage Update() to fall back to Create() when forceAllowCreate=true and the target coder template/workspace does not exist. This unblocks SSA create-on-update requests while keeping existing update validation and defensive assertions for normal update flows. Added a prominent FIXME in both storage implementations documenting that this is a compatibility hack and outlining future options, with the preferred long-term fix being first-class metadata support in Coder/codersdk. Also added storage tests for both codertemplates and coderworkspaces to validate create-on-update behavior, and documented the SSA limitation and roadmap in the aggregated API deployment guide. --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `openai:gpt-5.3-codex` • Thinking: `xhigh` • Cost: `$2.58`_ --- docs/how-to/deploy-aggregated-apiserver.md | 19 ++++ internal/aggregated/storage/storage_test.go | 100 ++++++++++++++++++++ internal/aggregated/storage/template.go | 57 +++++++++-- internal/aggregated/storage/workspace.go | 58 ++++++++++-- 4 files changed, 218 insertions(+), 16 deletions(-) diff --git a/docs/how-to/deploy-aggregated-apiserver.md b/docs/how-to/deploy-aggregated-apiserver.md index 9eda72f5..9b2b84e4 100644 --- a/docs/how-to/deploy-aggregated-apiserver.md +++ b/docs/how-to/deploy-aggregated-apiserver.md @@ -90,6 +90,25 @@ kubectl get codertemplates.aggregation.coder.com -A kubectl logs -n coder-system deploy/coder-k8s ``` +## Server-Side Apply (SSA) behavior + +`coder-k8s` now includes a compatibility fallback for SSA create-on-update requests +(for example, `kubectl apply --server-side` when the target resource does not exist yet). + +- For missing `coderworkspaces` / `codertemplates`, the aggregated API server's `Update` + path can delegate to `Create` when `forceAllowCreate=true`. +- This is intentionally **best-effort**: Coder resources do not currently provide a + first-class metadata store for Kubernetes `metadata.managedFields`, so SSA field-owner + conflict semantics are not durable. + +Planned follow-up options (in order of preference): + +1. **Preferred:** add first-class metadata support on template/workspace resources in + Coder + `codersdk`, and round-trip Kubernetes-managed metadata there. +2. Persist Kubernetes-only metadata in a shadow Kubernetes resource (ConfigMap/CRD) + managed by the aggregated API server. +3. Keep the compatibility fallback and continue documenting the limitations. + ## Template build wait tuning When updating `CoderTemplate.spec.files`, the aggregated API server now waits for diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index d5bfef3a..5ce6eb77 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -1477,6 +1477,58 @@ func TestTemplateStorageUpdateAllowsMetadataSpecChanges(t *testing.T) { } } +func TestTemplateStorageUpdateForceAllowCreateCreatesWhenMissing(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + desiredTemplate := &aggregationv1alpha1.CoderTemplate{ + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + DisplayName: "SSA-created template", + Description: "Created via forceAllowCreate update hack", + Icon: "/icons/ssa-template.png", + Files: map[string]string{ + "main.tf": "resource \"null_resource\" \"ssa_create\" {}", + }, + }, + } + + updatedObj, created, err := templateStorage.Update( + ctx, + "acme.ssa-template", + testUpdatedObjectInfo{obj: desiredTemplate}, + rest.ValidateAllObjectFunc, + rest.ValidateAllObjectUpdateFunc, + true, + nil, + ) + if err != nil { + t.Fatalf("expected create-on-update to succeed for missing template: %v", err) + } + if !created { + t.Fatal("expected update created=true when forceAllowCreate creates a template") + } + + createdTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj) + } + if createdTemplate.Name != "acme.ssa-template" { + t.Fatalf("expected created template name acme.ssa-template, got %q", createdTemplate.Name) + } + if createdTemplate.Spec.Organization != "acme" { + t.Fatalf("expected created template organization acme, got %q", createdTemplate.Spec.Organization) + } + if !state.hasTemplate("acme", "ssa-template") { + t.Fatal("expected create-on-update template to be persisted in mock server state") + } +} + func TestTemplateStorageUpdateRejectsMissingResourceVersion(t *testing.T) { t.Parallel() @@ -1846,6 +1898,54 @@ func TestWorkspaceStorageCreateAllowsMatchingTemplateVersionID(t *testing.T) { } } +func TestWorkspaceStorageUpdateForceAllowCreateCreatesWhenMissing(t *testing.T) { + t.Parallel() + + server, state := newMockCoderServer(t) + defer server.Close() + + workspaceStorage := NewWorkspaceStorage(newTestClientProvider(t, server.URL)) + ctx := namespacedContext("control-plane") + + desiredWorkspace := &aggregationv1alpha1.CoderWorkspace{ + Spec: aggregationv1alpha1.CoderWorkspaceSpec{ + Organization: "acme", + TemplateName: "starter-template", + Running: false, + }, + } + + updatedObj, created, err := workspaceStorage.Update( + ctx, + "acme.alice.ssa-workspace", + testUpdatedObjectInfo{obj: desiredWorkspace}, + rest.ValidateAllObjectFunc, + rest.ValidateAllObjectUpdateFunc, + true, + nil, + ) + if err != nil { + t.Fatalf("expected create-on-update to succeed for missing workspace: %v", err) + } + if !created { + t.Fatal("expected update created=true when forceAllowCreate creates a workspace") + } + + createdWorkspace, ok := updatedObj.(*aggregationv1alpha1.CoderWorkspace) + if !ok { + t.Fatalf("expected *CoderWorkspace from update, got %T", updatedObj) + } + if createdWorkspace.Name != "acme.alice.ssa-workspace" { + t.Fatalf("expected created workspace name acme.alice.ssa-workspace, got %q", createdWorkspace.Name) + } + if !state.hasWorkspace("alice", "ssa-workspace") { + t.Fatal("expected create-on-update workspace to be persisted in mock server state") + } + if !containsTransition(state.buildTransitionsSnapshot(), codersdk.WorkspaceTransitionStop) { + t.Fatal("expected create-on-update workspace with spec.running=false to queue stop transition") + } +} + func TestWorkspaceStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) { t.Parallel() diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index ee5d9ad7..55e25049 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -415,7 +415,7 @@ func (s *TemplateStorage) Update( ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, - _ rest.ValidateObjectFunc, + createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, _ *metav1.UpdateOptions, @@ -435,16 +435,57 @@ func (s *TemplateStorage) Update( if s.broadcaster == nil { return nil, false, fmt.Errorf("assertion failed: template broadcaster must not be nil") } - if forceAllowCreate { - return nil, false, apierrors.NewMethodNotSupported( - aggregationv1alpha1.Resource("codertemplates"), - "create on update", - ) - } currentObj, err := s.Get(ctx, name, nil) if err != nil { - return nil, false, err + if !forceAllowCreate || !apierrors.IsNotFound(err) { + return nil, false, err + } + + // FIXME: This branch intentionally enables best-effort SSA create-on-update + // semantics for codertemplates. + // + // The aggregated API is a stateless proxy to Coder and currently does not + // persist Kubernetes metadata.managedFields (or other Kubernetes-only metadata), + // so SSA field ownership/conflict semantics are not durable. + // + // Future options to remove this hack: + // 1. Preferred: add first-class metadata support on templates/workspaces in + // Coder + codersdk and round-trip Kubernetes-managed metadata there. + // 2. Persist Kubernetes-only metadata in a shadow Kubernetes resource + // (ConfigMap/CRD) managed by this aggregated API server. + // 3. Keep this fallback and continue documenting SSA limitations. + createObj, objInfoErr := objInfo.UpdatedObject(ctx, s.New()) + if objInfoErr != nil { + return nil, false, objInfoErr + } + if createObj == nil { + return nil, false, fmt.Errorf("assertion failed: create-on-update template object must not be nil") + } + + createTemplate, ok := createObj.(*aggregationv1alpha1.CoderTemplate) + if !ok { + return nil, false, fmt.Errorf("assertion failed: expected *CoderTemplate, got %T", createObj) + } + createTemplate = createTemplate.DeepCopy() + if createTemplate == nil { + return nil, false, fmt.Errorf("assertion failed: create-on-update template deep copy must not be nil") + } + if createTemplate.Name == "" { + createTemplate.Name = name + } + if createTemplate.Name != name { + return nil, false, apierrors.NewBadRequest( + fmt.Sprintf("updated object metadata.name %q must match request name %q", createTemplate.Name, name), + ) + } + + createdObj, createErr := s.Create(ctx, createTemplate, createValidation, nil) + if createErr != nil { + return nil, false, createErr + } + + return createdObj, true, nil } currentObjForUpdate := currentObj.DeepCopyObject() diff --git a/internal/aggregated/storage/workspace.go b/internal/aggregated/storage/workspace.go index 900b7d7a..0c168523 100644 --- a/internal/aggregated/storage/workspace.go +++ b/internal/aggregated/storage/workspace.go @@ -397,7 +397,7 @@ func (s *WorkspaceStorage) Update( ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, - _ rest.ValidateObjectFunc, + createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, _ *metav1.UpdateOptions, @@ -417,12 +417,6 @@ func (s *WorkspaceStorage) Update( if s.broadcaster == nil { return nil, false, fmt.Errorf("assertion failed: workspace broadcaster must not be nil") } - if forceAllowCreate { - return nil, false, apierrors.NewMethodNotSupported( - aggregationv1alpha1.Resource("coderworkspaces"), - "create on update", - ) - } namespace, badNamespaceErr := requiredNamespaceFromRequestContext(ctx) if badNamespaceErr != nil { @@ -441,7 +435,55 @@ func (s *WorkspaceStorage) Update( currentWorkspace, err := sdk.WorkspaceByOwnerAndName(ctx, userName, workspaceName, codersdk.WorkspaceOptions{}) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("coderworkspaces"), name) + mappedErr := coder.MapCoderError(err, aggregationv1alpha1.Resource("coderworkspaces"), name) + if !forceAllowCreate || !apierrors.IsNotFound(mappedErr) { + return nil, false, mappedErr + } + + // FIXME: This branch intentionally enables best-effort SSA create-on-update + // semantics for coderworkspaces. + // + // The aggregated API is a stateless proxy to Coder and currently does not + // persist Kubernetes metadata.managedFields (or other Kubernetes-only metadata), + // so SSA field ownership/conflict semantics are not durable. + // + // Future options to remove this hack: + // 1. Preferred: add first-class metadata support on templates/workspaces in + // Coder + codersdk and round-trip Kubernetes-managed metadata there. + // 2. Persist Kubernetes-only metadata in a shadow Kubernetes resource + // (ConfigMap/CRD) managed by this aggregated API server. + // 3. Keep this fallback and continue documenting SSA limitations. + createObj, objInfoErr := objInfo.UpdatedObject(ctx, s.New()) + if objInfoErr != nil { + return nil, false, objInfoErr + } + if createObj == nil { + return nil, false, fmt.Errorf("assertion failed: create-on-update workspace object must not be nil") + } + + createWorkspace, ok := createObj.(*aggregationv1alpha1.CoderWorkspace) + if !ok { + return nil, false, fmt.Errorf("assertion failed: expected *CoderWorkspace, got %T", createObj) + } + createWorkspace = createWorkspace.DeepCopy() + if createWorkspace == nil { + return nil, false, fmt.Errorf("assertion failed: create-on-update workspace deep copy must not be nil") + } + if createWorkspace.Name == "" { + createWorkspace.Name = name + } + if createWorkspace.Name != name { + return nil, false, apierrors.NewBadRequest( + fmt.Sprintf("updated object metadata.name %q must match request name %q", createWorkspace.Name, name), + ) + } + + createdObj, createErr := s.Create(ctx, createWorkspace, createValidation, nil) + if createErr != nil { + return nil, false, createErr + } + + return createdObj, true, nil } if currentWorkspace.OrganizationName != orgName { return nil, false, apierrors.NewNotFound(aggregationv1alpha1.Resource("coderworkspaces"), name)