Skip to content

Commit b736502

Browse files
ThomasK33ThomasK33
committed
fix: tolerate empty immutable fields on workspace and template updates
1 parent 2ae2bbb commit b736502

5 files changed

Lines changed: 215 additions & 2 deletions

File tree

‎internal/aggregated/convert/template.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ func TemplateToK8s(namespace string, t codersdk.Template) *aggregationv1alpha1.C
3535
},
3636
Spec: aggregationv1alpha1.CoderTemplateSpec{
3737
Organization: t.OrganizationName,
38+
VersionID: t.ActiveVersionID.String(),
3839
DisplayName: t.DisplayName,
3940
Description: t.Description,
4041
Icon: t.Icon,

‎internal/aggregated/convert/template_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ func TestTemplateToK8s(t *testing.T) {
5353
if converted.Spec.Organization != "acme" {
5454
t.Fatalf("expected spec organization acme, got %q", converted.Spec.Organization)
5555
}
56+
if converted.Spec.VersionID != activeVersionID.String() {
57+
t.Fatalf("expected spec version ID %q, got %q", activeVersionID.String(), converted.Spec.VersionID)
58+
}
5659
if converted.Spec.DisplayName != "Starter Template" {
5760
t.Fatalf("expected spec display name Starter Template, got %q", converted.Spec.DisplayName)
5861
}

‎internal/aggregated/storage/storage_test.go‎

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,103 @@ func TestTemplateStorageUpdateReturnsDesiredObjectForLegacyRunningField(t *testi
222222
}
223223
}
224224

225+
func TestTemplateStorageUpdateAllowsEmptyVersionIDWhenTogglingRunning(t *testing.T) {
226+
t.Parallel()
227+
228+
server, _ := newMockCoderServer(t)
229+
defer server.Close()
230+
231+
templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL))
232+
ctx := namespacedContext("control-plane")
233+
234+
currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil)
235+
if err != nil {
236+
t.Fatalf("expected template get to succeed: %v", err)
237+
}
238+
239+
currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate)
240+
if !ok {
241+
t.Fatalf("expected *CoderTemplate from get, got %T", currentObj)
242+
}
243+
if currentTemplate.Spec.VersionID == "" {
244+
t.Fatal("expected current template spec.versionID to be populated")
245+
}
246+
247+
desiredTemplate := currentTemplate.DeepCopy()
248+
desiredTemplate.Spec.Running = !currentTemplate.Spec.Running
249+
desiredTemplate.Spec.VersionID = ""
250+
251+
updatedObj, created, err := templateStorage.Update(
252+
ctx,
253+
desiredTemplate.Name,
254+
testUpdatedObjectInfo{obj: desiredTemplate},
255+
nil,
256+
rest.ValidateAllObjectUpdateFunc,
257+
false,
258+
nil,
259+
)
260+
if err != nil {
261+
t.Fatalf("expected template update to succeed when desired spec.versionID is empty: %v", err)
262+
}
263+
if created {
264+
t.Fatal("expected update created=false")
265+
}
266+
267+
updatedTemplate, ok := updatedObj.(*aggregationv1alpha1.CoderTemplate)
268+
if !ok {
269+
t.Fatalf("expected *CoderTemplate from update, got %T", updatedObj)
270+
}
271+
if updatedTemplate.Spec.Running != desiredTemplate.Spec.Running {
272+
t.Fatalf("expected updated running=%t, got %t", desiredTemplate.Spec.Running, updatedTemplate.Spec.Running)
273+
}
274+
if updatedTemplate.Spec.VersionID != "" {
275+
t.Fatalf("expected returned desired spec.versionID to remain empty, got %q", updatedTemplate.Spec.VersionID)
276+
}
277+
}
278+
279+
func TestTemplateStorageUpdateRejectsDifferentVersionID(t *testing.T) {
280+
t.Parallel()
281+
282+
server, _ := newMockCoderServer(t)
283+
defer server.Close()
284+
285+
templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL))
286+
ctx := namespacedContext("control-plane")
287+
288+
currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil)
289+
if err != nil {
290+
t.Fatalf("expected template get to succeed: %v", err)
291+
}
292+
293+
currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate)
294+
if !ok {
295+
t.Fatalf("expected *CoderTemplate from get, got %T", currentObj)
296+
}
297+
298+
desiredTemplate := currentTemplate.DeepCopy()
299+
desiredTemplate.Spec.Running = !currentTemplate.Spec.Running
300+
desiredTemplate.Spec.VersionID = uuid.New().String()
301+
if desiredTemplate.Spec.VersionID == currentTemplate.Spec.VersionID {
302+
t.Fatal("expected test fixture to use a different spec.versionID")
303+
}
304+
305+
_, _, err = templateStorage.Update(
306+
ctx,
307+
desiredTemplate.Name,
308+
testUpdatedObjectInfo{obj: desiredTemplate},
309+
nil,
310+
rest.ValidateAllObjectUpdateFunc,
311+
false,
312+
nil,
313+
)
314+
if !apierrors.IsBadRequest(err) {
315+
t.Fatalf("expected BadRequest when changing spec.versionID, got %v", err)
316+
}
317+
if err == nil || !strings.Contains(err.Error(), "spec.running") {
318+
t.Fatalf("expected immutable-field error mentioning spec.running, got %v", err)
319+
}
320+
}
321+
225322
func TestTemplateStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) {
226323
t.Parallel()
227324

@@ -483,6 +580,118 @@ func TestWorkspaceStorageUpdateAllowsPinnedTemplateVersionIDWhenTogglingRunning(
483580
}
484581
}
485582

583+
func TestWorkspaceStorageUpdateAllowsEmptyTemplateVersionIDWhenTogglingRunning(t *testing.T) {
584+
t.Parallel()
585+
586+
server, state := newMockCoderServer(t)
587+
defer server.Close()
588+
589+
workspaceStorage := NewWorkspaceStorage(newTestClientProvider(t, server.URL))
590+
ctx := namespacedContext("control-plane")
591+
592+
currentObj, err := workspaceStorage.Get(ctx, "acme.alice.dev-workspace", nil)
593+
if err != nil {
594+
t.Fatalf("expected workspace get to succeed: %v", err)
595+
}
596+
597+
currentWorkspace, ok := currentObj.(*aggregationv1alpha1.CoderWorkspace)
598+
if !ok {
599+
t.Fatalf("expected *CoderWorkspace from get, got %T", currentObj)
600+
}
601+
if currentWorkspace.Spec.TemplateVersionID == "" {
602+
t.Fatal("expected current workspace spec.templateVersionID to be populated")
603+
}
604+
605+
desiredWorkspace := currentWorkspace.DeepCopy()
606+
desiredWorkspace.Spec.TemplateVersionID = ""
607+
desiredWorkspace.Spec.Running = !currentWorkspace.Spec.Running
608+
609+
updatedObj, created, err := workspaceStorage.Update(
610+
ctx,
611+
desiredWorkspace.Name,
612+
testUpdatedObjectInfo{obj: desiredWorkspace},
613+
nil,
614+
rest.ValidateAllObjectUpdateFunc,
615+
false,
616+
nil,
617+
)
618+
if err != nil {
619+
t.Fatalf("expected workspace update to succeed when desired spec.templateVersionID is empty: %v", err)
620+
}
621+
if created {
622+
t.Fatal("expected update created=false")
623+
}
624+
625+
updatedWorkspace, ok := updatedObj.(*aggregationv1alpha1.CoderWorkspace)
626+
if !ok {
627+
t.Fatalf("expected *CoderWorkspace from update, got %T", updatedObj)
628+
}
629+
if updatedWorkspace.Spec.Running != desiredWorkspace.Spec.Running {
630+
t.Fatalf("expected updated running=%t, got %t", desiredWorkspace.Spec.Running, updatedWorkspace.Spec.Running)
631+
}
632+
if updatedWorkspace.Spec.TemplateVersionID != currentWorkspace.Spec.TemplateVersionID {
633+
t.Fatalf(
634+
"expected updated templateVersionID %q, got %q",
635+
currentWorkspace.Spec.TemplateVersionID,
636+
updatedWorkspace.Spec.TemplateVersionID,
637+
)
638+
}
639+
640+
expectedTransition := codersdk.WorkspaceTransitionStop
641+
if desiredWorkspace.Spec.Running {
642+
expectedTransition = codersdk.WorkspaceTransitionStart
643+
}
644+
if !containsTransition(state.buildTransitionsSnapshot(), expectedTransition) {
645+
t.Fatalf("expected update to queue %q transition", expectedTransition)
646+
}
647+
}
648+
649+
func TestWorkspaceStorageUpdateRejectsDifferentTemplateVersionID(t *testing.T) {
650+
t.Parallel()
651+
652+
server, state := newMockCoderServer(t)
653+
defer server.Close()
654+
655+
workspaceStorage := NewWorkspaceStorage(newTestClientProvider(t, server.URL))
656+
ctx := namespacedContext("control-plane")
657+
658+
currentObj, err := workspaceStorage.Get(ctx, "acme.alice.dev-workspace", nil)
659+
if err != nil {
660+
t.Fatalf("expected workspace get to succeed: %v", err)
661+
}
662+
663+
currentWorkspace, ok := currentObj.(*aggregationv1alpha1.CoderWorkspace)
664+
if !ok {
665+
t.Fatalf("expected *CoderWorkspace from get, got %T", currentObj)
666+
}
667+
668+
desiredWorkspace := currentWorkspace.DeepCopy()
669+
desiredWorkspace.Spec.Running = !currentWorkspace.Spec.Running
670+
desiredWorkspace.Spec.TemplateVersionID = uuid.New().String()
671+
if desiredWorkspace.Spec.TemplateVersionID == currentWorkspace.Spec.TemplateVersionID {
672+
t.Fatal("expected test fixture to use a different spec.templateVersionID")
673+
}
674+
675+
_, _, err = workspaceStorage.Update(
676+
ctx,
677+
desiredWorkspace.Name,
678+
testUpdatedObjectInfo{obj: desiredWorkspace},
679+
nil,
680+
rest.ValidateAllObjectUpdateFunc,
681+
false,
682+
nil,
683+
)
684+
if !apierrors.IsBadRequest(err) {
685+
t.Fatalf("expected BadRequest when changing spec.templateVersionID, got %v", err)
686+
}
687+
if err == nil || !strings.Contains(err.Error(), "spec.running") {
688+
t.Fatalf("expected immutable-field error mentioning spec.running, got %v", err)
689+
}
690+
if transitions := state.buildTransitionsSnapshot(); len(transitions) != 0 {
691+
t.Fatalf("expected no workspace build transitions on immutable-field error, got %v", transitions)
692+
}
693+
}
694+
486695
func TestWorkspaceStorageUpdateRejectsStaleResourceVersion(t *testing.T) {
487696
t.Parallel()
488697

‎internal/aggregated/storage/template.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ func (s *TemplateStorage) Update(
290290
// Coder backend. Reject updates to all other spec fields to avoid drift between
291291
// accepted update payloads and persisted backend state.
292292
if updatedTemplate.Spec.Organization != currentTemplate.Spec.Organization ||
293-
updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID ||
293+
(updatedTemplate.Spec.VersionID != "" && updatedTemplate.Spec.VersionID != currentTemplate.Spec.VersionID) ||
294294
updatedTemplate.Spec.DisplayName != currentTemplate.Spec.DisplayName ||
295295
updatedTemplate.Spec.Description != currentTemplate.Spec.Description ||
296296
updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon {

‎internal/aggregated/storage/workspace.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,7 +355,7 @@ func (s *WorkspaceStorage) Update(
355355
// transitions, which map only to spec.running toggles in this API.
356356
if desiredObj.Spec.Organization != currentK8sObj.Spec.Organization ||
357357
desiredObj.Spec.TemplateName != currentK8sObj.Spec.TemplateName ||
358-
desiredObj.Spec.TemplateVersionID != currentK8sObj.Spec.TemplateVersionID ||
358+
(desiredObj.Spec.TemplateVersionID != "" && desiredObj.Spec.TemplateVersionID != currentK8sObj.Spec.TemplateVersionID) ||
359359
!equalInt64Ptr(desiredObj.Spec.TTLMillis, currentK8sObj.Spec.TTLMillis) ||
360360
!equalStringPtr(desiredObj.Spec.AutostartSchedule, currentK8sObj.Spec.AutostartSchedule) {
361361
return nil, false, apierrors.NewBadRequest(

0 commit comments

Comments
 (0)