Skip to content

Commit bcbab37

Browse files
ThomasK33ThomasK33
committed
fix: fail fast on missing coder config and enforce template resourceVersion
1 parent e441397 commit bcbab37

4 files changed

Lines changed: 123 additions & 41 deletions

File tree

‎internal/aggregated/storage/storage_test.go‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,9 @@ func TestTemplateStorageUpdateReturnsDesiredObjectForLegacyRunningField(t *testi
190190
if !ok {
191191
t.Fatalf("expected *CoderTemplate from get, got %T", currentObj)
192192
}
193+
if currentTemplate.ResourceVersion == "" {
194+
t.Fatal("expected current template resourceVersion to be populated")
195+
}
193196

194197
desiredTemplate := currentTemplate.DeepCopy()
195198
desiredTemplate.Spec.Running = !currentTemplate.Spec.Running
@@ -358,6 +361,89 @@ func TestTemplateStorageUpdateRejectsNonRunningSpecChanges(t *testing.T) {
358361
}
359362
}
360363

364+
func TestTemplateStorageUpdateRejectsMissingResourceVersion(t *testing.T) {
365+
t.Parallel()
366+
367+
server, _ := newMockCoderServer(t)
368+
defer server.Close()
369+
370+
templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL))
371+
ctx := namespacedContext("control-plane")
372+
373+
currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil)
374+
if err != nil {
375+
t.Fatalf("expected template get to succeed: %v", err)
376+
}
377+
378+
currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate)
379+
if !ok {
380+
t.Fatalf("expected *CoderTemplate from get, got %T", currentObj)
381+
}
382+
if currentTemplate.ResourceVersion == "" {
383+
t.Fatal("expected current template resourceVersion to be populated")
384+
}
385+
386+
desiredTemplate := currentTemplate.DeepCopy()
387+
desiredTemplate.Spec.Running = !currentTemplate.Spec.Running
388+
desiredTemplate.ResourceVersion = ""
389+
390+
_, _, err = templateStorage.Update(
391+
ctx,
392+
desiredTemplate.Name,
393+
testUpdatedObjectInfo{obj: desiredTemplate},
394+
nil,
395+
rest.ValidateAllObjectUpdateFunc,
396+
false,
397+
nil,
398+
)
399+
if !apierrors.IsBadRequest(err) {
400+
t.Fatalf("expected BadRequest for missing resourceVersion, got %v", err)
401+
}
402+
if err == nil || !strings.Contains(err.Error(), "metadata.resourceVersion is required for update") {
403+
t.Fatalf("expected missing resourceVersion error message, got %v", err)
404+
}
405+
}
406+
407+
func TestTemplateStorageUpdateRejectsStaleResourceVersion(t *testing.T) {
408+
t.Parallel()
409+
410+
server, _ := newMockCoderServer(t)
411+
defer server.Close()
412+
413+
templateStorage := NewTemplateStorage(newTestClientProvider(t, server.URL))
414+
ctx := namespacedContext("control-plane")
415+
416+
currentObj, err := templateStorage.Get(ctx, "acme.starter-template", nil)
417+
if err != nil {
418+
t.Fatalf("expected template get to succeed: %v", err)
419+
}
420+
421+
currentTemplate, ok := currentObj.(*aggregationv1alpha1.CoderTemplate)
422+
if !ok {
423+
t.Fatalf("expected *CoderTemplate from get, got %T", currentObj)
424+
}
425+
426+
desiredTemplate := currentTemplate.DeepCopy()
427+
desiredTemplate.Spec.Running = !currentTemplate.Spec.Running
428+
desiredTemplate.ResourceVersion = currentTemplate.ResourceVersion + "-stale"
429+
430+
_, _, err = templateStorage.Update(
431+
ctx,
432+
desiredTemplate.Name,
433+
testUpdatedObjectInfo{obj: desiredTemplate},
434+
nil,
435+
rest.ValidateAllObjectUpdateFunc,
436+
false,
437+
nil,
438+
)
439+
if !apierrors.IsConflict(err) {
440+
t.Fatalf("expected Conflict for stale resourceVersion, got %v", err)
441+
}
442+
if err == nil || !strings.Contains(err.Error(), "resource version mismatch") {
443+
t.Fatalf("expected stale resourceVersion error message, got %v", err)
444+
}
445+
}
446+
361447
func TestWorkspaceStorageCRUDWithCoderSDK(t *testing.T) {
362448
t.Parallel()
363449

‎internal/aggregated/storage/template.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,21 @@ func (s *TemplateStorage) Update(
285285
return nil, false, fmt.Errorf("assertion failed: expected *CoderTemplate, got %T", currentObj)
286286
}
287287

288+
if updatedTemplate.ResourceVersion == "" {
289+
return nil, false, apierrors.NewBadRequest("metadata.resourceVersion is required for update")
290+
}
291+
if updatedTemplate.ResourceVersion != currentTemplate.ResourceVersion {
292+
return nil, false, apierrors.NewConflict(
293+
aggregationv1alpha1.Resource("codertemplates"),
294+
name,
295+
fmt.Errorf(
296+
"resource version mismatch: got %q, current is %q",
297+
updatedTemplate.ResourceVersion,
298+
currentTemplate.ResourceVersion,
299+
),
300+
)
301+
}
302+
288303
// Template updates via codersdk are currently limited. The legacy spec.running
289304
// field remains for compatibility with in-repo callers and is a no-op in the
290305
// Coder backend. Reject updates to all other spec fields to avoid drift between

‎internal/app/apiserverapp/apiserverapp.go‎

Lines changed: 3 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1"
3030
"github.com/coder/coder-k8s/internal/aggregated/coder"
3131
"github.com/coder/coder-k8s/internal/aggregated/storage"
32-
"github.com/coder/coder/v2/codersdk"
3332
)
3433

3534
const (
@@ -55,29 +54,6 @@ type Options struct {
5554
CoderRequestTimeout time.Duration
5655
}
5756

58-
type errClientProvider struct {
59-
err error
60-
}
61-
62-
var _ coder.ClientProvider = (*errClientProvider)(nil)
63-
64-
func (p *errClientProvider) ClientForNamespace(ctx context.Context, namespace string) (*codersdk.Client, error) {
65-
if p == nil {
66-
return nil, fmt.Errorf("assertion failed: error client provider must not be nil")
67-
}
68-
if ctx == nil {
69-
return nil, fmt.Errorf("assertion failed: context must not be nil")
70-
}
71-
if namespace == "" {
72-
return nil, fmt.Errorf("assertion failed: namespace must not be empty")
73-
}
74-
if p.err == nil {
75-
return nil, fmt.Errorf("assertion failed: error client provider error must not be nil")
76-
}
77-
78-
return nil, p.err
79-
}
80-
8157
func buildClientProvider(opts Options, requestTimeout time.Duration) (coder.ClientProvider, error) {
8258
if requestTimeout <= 0 {
8359
return nil, fmt.Errorf("assertion failed: request timeout must be positive")
@@ -93,15 +69,10 @@ func buildClientProvider(opts Options, requestTimeout time.Duration) (coder.Clie
9369
missing = append(missing, "coder session token")
9470
}
9571
if len(missing) > 0 {
96-
provider := &errClientProvider{err: fmt.Errorf(
97-
"coder client provider is not configured: missing %s; configure --coder-url and --coder-session-token to enable coderworkspaces and codertemplates operations",
72+
return nil, fmt.Errorf(
73+
"coder client provider is not configured: missing %s; configure --coder-url and --coder-session-token",
9874
strings.Join(missing, " and "),
99-
)}
100-
if provider.err == nil {
101-
return nil, fmt.Errorf("assertion failed: fallback error client provider error is nil after successful construction")
102-
}
103-
104-
return provider, nil
75+
)
10576
}
10677

10778
parsedCoderURL, err := url.Parse(coderURL)

‎internal/app/apiserverapp/apiserverapp_test.go‎

Lines changed: 19 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -127,23 +127,33 @@ func TestInstallAPIGroupRegistersDiscovery(t *testing.T) {
127127
}
128128
}
129129

130-
func TestBuildClientProviderReturnsDeferredErrorWithoutCoderConfig(t *testing.T) {
130+
func TestBuildClientProviderRejectsMissingCoderConfig(t *testing.T) {
131131
t.Parallel()
132132

133133
provider, err := buildClientProvider(Options{}, 30*time.Second)
134-
if err != nil {
135-
t.Fatalf("build client provider: %v", err)
134+
if err == nil {
135+
t.Fatal("expected missing coder config to return an error")
136136
}
137-
if provider == nil {
138-
t.Fatal("expected non-nil provider")
137+
if provider != nil {
138+
t.Fatalf("expected nil provider when coder config is missing, got %T", provider)
139+
}
140+
if !strings.Contains(err.Error(), "missing coder URL and coder session token") {
141+
t.Fatalf("expected missing-config error, got %q", err)
139142
}
143+
}
144+
145+
func TestRunWithOptionsRejectsMissingCoderConfig(t *testing.T) {
146+
t.Parallel()
140147

141-
_, err = provider.ClientForNamespace(context.Background(), "control-plane")
148+
err := RunWithOptions(context.Background(), Options{})
142149
if err == nil {
143-
t.Fatal("expected deferred client error when coder config is missing")
150+
t.Fatal("expected missing coder config to fail aggregated apiserver startup")
144151
}
145-
if !strings.Contains(err.Error(), "missing coder URL and coder session token") {
146-
t.Fatalf("expected missing-config error, got %q", err)
152+
if !strings.Contains(err.Error(), "build coder client provider") {
153+
t.Fatalf("expected startup error to identify client provider construction, got %v", err)
154+
}
155+
if !strings.Contains(err.Error(), "configure --coder-url and --coder-session-token") {
156+
t.Fatalf("expected startup error to mention required coder flags, got %v", err)
147157
}
148158
}
149159

0 commit comments

Comments
 (0)