From 430c9ccb46c616efbda92f0bd225f44f21cb49fb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 11:49:53 +0000 Subject: [PATCH 01/27] test: reproduce uncertain Coder write answers and nameless Coder objects (#242) Change-Id: Ia77616f5949c1930986af2594cf3e69bc22a302c Signed-off-by: Thomas Kosiewski --- .../aggregated/storage/coder_answer_test.go | 97 ++++++ .../aggregated/storage/write_outcome_test.go | 277 ++++++++++++++++++ .../apiserverapp/coder_answer_route_test.go | 127 ++++++++ internal/app/apiserverapp/integration_test.go | 6 + 4 files changed, 507 insertions(+) create mode 100644 internal/aggregated/storage/coder_answer_test.go create mode 100644 internal/aggregated/storage/write_outcome_test.go create mode 100644 internal/app/apiserverapp/coder_answer_route_test.go diff --git a/internal/aggregated/storage/coder_answer_test.go b/internal/aggregated/storage/coder_answer_test.go new file mode 100644 index 000000000..ae8de632b --- /dev/null +++ b/internal/aggregated/storage/coder_answer_test.go @@ -0,0 +1,97 @@ +package storage + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + + "github.com/coder/coder-k8s/internal/aggregated/coder" +) + +// TestCoderAnswersWithoutNamesFailCleanly checks that a Coder answer whose objects lack the fields +// of the Kubernetes name, for example a proxy that answers {} (#242 case 5), returns a 500 that +// names the problem instead of a panic. +func TestCoderAnswersWithoutNamesFailCleanly(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + // isRead matches the Coder read that gets body instead of Coder's answer. + isRead func(*http.Request) bool + body func(state *mockCoderServerState) string + read func(provider coder.ClientProvider) (runtime.Object, error) + }{ + { + name: "workspace list", + isRead: func(r *http.Request) bool { return r.URL.Path == "/api/v2/workspaces" }, + body: func(*mockCoderServerState) string { return `{"workspaces":[{}],"count":1}` }, + read: func(provider coder.ClientProvider) (runtime.Object, error) { + return NewWorkspaceStorage(provider).List(namespacedContext("control-plane"), nil) + }, + }, + { + name: "template list", + isRead: func(r *http.Request) bool { return r.URL.Path == "/api/v2/templates" }, + body: func(*mockCoderServerState) string { return `[{}]` }, + read: func(provider coder.ClientProvider) (runtime.Object, error) { + return NewTemplateStorage(provider).List(namespacedContext("control-plane"), nil) + }, + }, + { + name: "template get", + isRead: func(r *http.Request) bool { + return strings.HasSuffix(r.URL.Path, "/templates/starter-template") + }, + body: func(*mockCoderServerState) string { return `{"name":"starter-template"}` }, + read: func(provider coder.ClientProvider) (runtime.Object, error) { + return NewTemplateStorage(provider).Get(namespacedContext("control-plane"), "acme.starter-template", nil) + }, + }, + { + name: "template version list", + isRead: func(r *http.Request) bool { return strings.HasSuffix(r.URL.Path, "/versions") }, + body: func(state *mockCoderServerState) string { + for id := range state.templatesByID { + return `[{"template_id":"` + id.String() + `"}]` + } + panic("assertion failed: the mock Coder must have a template") + }, + read: func(provider coder.ClientProvider) (runtime.Object, error) { + return NewTemplateVersionStorage(provider).List(namespacedContext("control-plane"), nil) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + coderServer, state := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + body := tc.body(state) + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet || !tc.isRead(r) { + coderServer.Config.Handler.ServeHTTP(w, r) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(proxy.Close) + + var err error + func() { + defer func() { + if recovered := recover(); recovered != nil { + t.Fatalf("the read panicked: %v", recovered) + } + }() + _, err = tc.read(newTestClientProvider(t, proxy.URL)) + }() + if !apierrors.IsInternalError(err) || !strings.Contains(err.Error(), "the Coder API answered") { + t.Fatalf("expected a 500 that names the Coder answer, got %v", err) + } + }) + } +} diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go new file mode 100644 index 000000000..7c696b6c0 --- /dev/null +++ b/internal/aggregated/storage/write_outcome_test.go @@ -0,0 +1,277 @@ +package storage + +import ( + "context" + "fmt" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apiserver/pkg/registry/rest" + + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" + "github.com/coder/coder-k8s/internal/aggregated/coder" +) + +// brokenAnswer replaces Coder's answer to a write that Coder has already applied. +type brokenAnswer func(t *testing.T, w http.ResponseWriter, coderAnswer *httptest.ResponseRecorder) + +func gatewayPage(status int) brokenAnswer { + return func(_ *testing.T, w http.ResponseWriter, _ *httptest.ResponseRecorder) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(status) + _, _ = w.Write([]byte("proxy error")) + } +} + +func jsonAnswer(status int, body string) brokenAnswer { + return func(_ *testing.T, w http.ResponseWriter, _ *httptest.ResponseRecorder) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + } +} + +func hijack(t *testing.T, w http.ResponseWriter) net.Conn { + t.Helper() + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Fatalf("hijack: %v", err) + } + return conn +} + +// truncatedAnswer sends Coder's status and half of its body, then closes the connection. +func truncatedAnswer(t *testing.T, w http.ResponseWriter, coderAnswer *httptest.ResponseRecorder) { + body := coderAnswer.Body.Bytes() + conn := hijack(t, w) + defer conn.Close() + _, _ = fmt.Fprintf(conn, "HTTP/1.1 %d %s\r\nContent-Type: application/json\r\nContent-Length: %d\r\n\r\n", + coderAnswer.Code, http.StatusText(coderAnswer.Code), len(body)) + _, _ = conn.Write(body[:len(body)/2]) +} + +func closedWithoutAnswer(t *testing.T, w http.ResponseWriter, _ *httptest.ResponseRecorder) { + _ = hijack(t, w).Close() +} + +// writeOutcomeServer fronts the mock Coder. Coder handles every request, and the write that +// isWrite matches gets answer instead of Coder's answer. +func writeOutcomeServer(t *testing.T, coderServer *httptest.Server, isWrite func(*http.Request) bool, answer brokenAnswer) *httptest.Server { + t.Helper() + + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !isWrite(r) { + coderServer.Config.Handler.ServeHTTP(w, r) + return + } + recorder := httptest.NewRecorder() + coderServer.Config.Handler.ServeHTTP(recorder, r) + if recorder.Code >= http.StatusMultipleChoices { + t.Errorf("mock Coder answered %s %s with %d: %s", r.Method, r.URL.Path, recorder.Code, recorder.Body.String()) + } + answer(t, w, recorder) + })) + t.Cleanup(proxy.Close) + return proxy +} + +// refusingWrites sends the write that isWrite matches to a closed port, so the connection is +// refused before the request reaches Coder. +type refusingWrites struct { + isWrite func(*http.Request) bool + closedAddr string +} + +func (rt refusingWrites) RoundTrip(r *http.Request) (*http.Response, error) { + if rt.isWrite(r) { + r = r.Clone(r.Context()) + r.URL.Host = rt.closedAddr + r.Host = "" + } + return http.DefaultTransport.RoundTrip(r) +} + +func closedAddress(t *testing.T) string { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := listener.Addr().String() + if err := listener.Close(); err != nil { + t.Fatal(err) + } + return addr +} + +type writeCase struct { + name string + isWrite func(*http.Request) bool + // creates is true when Coder's answer to the write is converted to the returned object. + creates bool + // readsBody is false when the SDK ignores the body of a successful answer. + readsBody bool + run func(t *testing.T, provider coder.ClientProvider) error +} + +func writeCases() []writeCase { + ctx := func() context.Context { return namespacedContext("control-plane") } + isPost := func(suffix string) func(*http.Request) bool { + return func(r *http.Request) bool { + return r.Method == http.MethodPost && strings.HasSuffix(r.URL.Path, suffix) + } + } + isBuild := isPost("/builds") + + return []writeCase{ + { + name: "workspace create", + isWrite: func(r *http.Request) bool { + return r.Method == http.MethodPost && r.URL.Path == "/api/v2/users/alice/workspaces" + }, + creates: true, + readsBody: true, + run: func(_ *testing.T, provider coder.ClientProvider) error { + _, err := NewWorkspaceStorage(provider).Create(ctx(), &aggregationv1alpha1.CoderWorkspace{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.alice.proxied"}, + Spec: aggregationv1alpha1.CoderWorkspaceSpec{Organization: "acme", TemplateName: "starter-template", Running: true}, + }, rest.ValidateAllObjectFunc, nil) + return err + }, + }, + { + name: "workspace update", + isWrite: isBuild, + readsBody: true, + run: func(t *testing.T, provider coder.ClientProvider) error { + workspaces := NewWorkspaceStorage(provider) + current, err := workspaces.Get(ctx(), "acme.alice.dev-workspace", nil) + if err != nil { + t.Fatalf("get workspace: %v", err) + } + desired := current.(*aggregationv1alpha1.CoderWorkspace).DeepCopy() + desired.Spec.Running = false + _, _, err = workspaces.Update(ctx(), desired.Name, rest.DefaultUpdatedObjectInfo(desired), nil, nil, false, nil) + return err + }, + }, + { + name: "workspace delete", + isWrite: isBuild, + readsBody: true, + run: func(_ *testing.T, provider coder.ClientProvider) error { + _, _, err := NewWorkspaceStorage(provider).Delete(ctx(), "acme.alice.dev-workspace", nil, nil) + return err + }, + }, + { + name: "template create", + isWrite: isPost("/templates"), + creates: true, + readsBody: true, + run: func(_ *testing.T, provider coder.ClientProvider) error { + _, err := NewTemplateStorage(provider).Create(ctx(), &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + Files: map[string]string{"main.tf": `resource "null_resource" "proxied" {}`}, + }, + }, rest.ValidateAllObjectFunc, nil) + return err + }, + }, + { + name: "template update", + isWrite: func(r *http.Request) bool { + return r.Method == http.MethodPatch && !strings.HasSuffix(r.URL.Path, "/versions") + }, + readsBody: true, + run: func(t *testing.T, provider coder.ClientProvider) error { + templates := NewTemplateStorage(provider) + current, err := templates.Get(ctx(), "acme.starter-template", nil) + if err != nil { + t.Fatalf("get template: %v", err) + } + desired := current.(*aggregationv1alpha1.CoderTemplate).DeepCopy() + desired.Spec.DisplayName = "Renamed behind a proxy" + _, _, err = templates.Update(ctx(), desired.Name, rest.DefaultUpdatedObjectInfo(desired), nil, nil, false, nil) + return err + }, + }, + { + name: "template delete", + isWrite: func(r *http.Request) bool { return r.Method == http.MethodDelete }, + run: func(_ *testing.T, provider coder.ClientProvider) error { + _, _, err := NewTemplateStorage(provider).Delete(ctx(), "acme.starter-template", nil, nil) + return err + }, + }, + } +} + +// TestWriteOutcomes checks every Coder write against answers that leave its result open (#242). +// Each of them returns the uncertain 504 without the Coder URL. A connection that is refused +// before the request is sent keeps the 500 that says that Coder could not be reached. +func TestWriteOutcomes(t *testing.T) { + t.Parallel() + + type outcome struct { + name string + answer brokenAnswer + // applies reports whether the outcome breaks the result of the write. A body that the + // SDK does not read, or an answer that only creates convert, leaves it intact. + applies func(writeCase) bool + } + always := func(writeCase) bool { return true } + readsBody := func(w writeCase) bool { return w.readsBody } + creates := func(w writeCase) bool { return w.creates } + outcomes := []outcome{ + {"proxy 502", gatewayPage(http.StatusBadGateway), always}, + {"proxy 503", gatewayPage(http.StatusServiceUnavailable), always}, + {"connection closed without an answer", closedWithoutAnswer, always}, + {"body breaks off", truncatedAnswer, readsBody}, + {"200 with malformed JSON", jsonAnswer(http.StatusOK, `{"id":`), readsBody}, + {"201 with an empty object", jsonAnswer(http.StatusCreated, `{}`), creates}, + {"201 with null", jsonAnswer(http.StatusCreated, `null`), creates}, + } + + for _, write := range writeCases() { + for _, out := range outcomes { + if !out.applies(write) { + continue + } + t.Run(write.name+"/"+out.name, func(t *testing.T) { + t.Parallel() + coderServer, _ := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + proxy := writeOutcomeServer(t, coderServer, write.isWrite, out.answer) + + err := write.run(t, newTestClientProvider(t, proxy.URL)) + if !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "uncertain") || + strings.Contains(err.Error(), strings.TrimPrefix(proxy.URL, "http://")) { + t.Fatalf("expected an uncertain 504 without the Coder URL, got %v", err) + } + }) + } + + t.Run(write.name+"/connection refused before the send", func(t *testing.T) { + t.Parallel() + coderServer, _ := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + closedAddr := closedAddress(t) + client := newTestSDKClient(t, coderServer.URL) + client.HTTPClient = &http.Client{Transport: refusingWrites{isWrite: write.isWrite, closedAddr: closedAddr}} + + err := write.run(t, &coder.StaticClientProvider{Client: client, Namespace: "control-plane"}) + if !apierrors.IsInternalError(err) || !strings.Contains(err.Error(), "could not be reached") || + strings.Contains(err.Error(), closedAddr) { + t.Fatalf("expected the 500 that says Coder could not be reached, got %v", err) + } + }) + } +} diff --git a/internal/app/apiserverapp/coder_answer_route_test.go b/internal/app/apiserverapp/coder_answer_route_test.go new file mode 100644 index 000000000..80eb1f9d6 --- /dev/null +++ b/internal/app/apiserverapp/coder_answer_route_test.go @@ -0,0 +1,127 @@ +package apiserverapp + +import ( + "encoding/json" + "net/http" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func decodeStatus(t *testing.T, body string) metav1.Status { + t.Helper() + var status metav1.Status + if err := json.Unmarshal([]byte(body), &status); err != nil || status.Kind != "Status" { + t.Fatalf("decode Status %q: %v", body, err) + } + return status +} + +// TestCoderAnswerRoutes sends broken Coder answers through the production handler chain (#242): +// writes whose result is open return the uncertain 504, and objects without the fields of their +// name return a 500 that names the problem, not the panic message of the generic apiserver. +func TestCoderAnswerRoutes(t *testing.T) { + f := newAuthFixture(t, func(k *fakeKubeAPI) { k.setDecide(allowAll) }) + cert := f.frontProxyCert(t) + alice := remoteUser("alice") + mock := f.server.mock + coderHost := strings.TrimPrefix(mock.URL(), "http://") + + status, body := f.server.do(t, cert, http.MethodGet, workspacesTestNS+"/"+testWorkspaceName, alice, "") + if status != http.StatusOK { + t.Fatalf("get workspace: status=%d body=%s", status, body) + } + var workspace map[string]any + if err := json.Unmarshal([]byte(body), &workspace); err != nil { + t.Fatal(err) + } + workspace["spec"].(map[string]any)["running"] = false + stopped, err := json.Marshal(workspace) + if err != nil { + t.Fatal(err) + } + + gateway := func(code int) func(http.ResponseWriter) { + return func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(code) + _, _ = w.Write([]byte("proxy error")) + } + } + closeConnection := func(w http.ResponseWriter) { + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + _ = conn.Close() + } + jsonBody := func(code int, body string) func(http.ResponseWriter) { + return func(w http.ResponseWriter) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(code) + _, _ = w.Write([]byte(body)) + } + } + + for _, tc := range []struct { + name string + method string + path string + body string + coderMethod string + coderPath func(string) bool + answer func(http.ResponseWriter) + wantCode int32 + wantMessage string + }{ + { + name: "workspace delete gets 502", method: http.MethodDelete, path: workspacesTestNS + "/" + testWorkspaceName, + coderMethod: http.MethodPost, coderPath: func(p string) bool { return strings.HasSuffix(p, "/builds") }, + answer: gateway(http.StatusBadGateway), wantCode: http.StatusGatewayTimeout, wantMessage: "uncertain", + }, + { + name: "workspace update loses the connection", method: http.MethodPut, path: workspacesTestNS + "/" + testWorkspaceName, body: string(stopped), + coderMethod: http.MethodPost, coderPath: func(p string) bool { return strings.HasSuffix(p, "/builds") }, + answer: closeConnection, wantCode: http.StatusGatewayTimeout, wantMessage: "uncertain", + }, + { + name: "template delete gets 503", method: http.MethodDelete, path: templatesTestNS + "/" + testTemplateName, + coderMethod: http.MethodDelete, coderPath: func(p string) bool { return strings.HasPrefix(p, "/api/v2/templates/") }, + answer: gateway(http.StatusServiceUnavailable), wantCode: http.StatusGatewayTimeout, wantMessage: "uncertain", + }, + { + name: "template create gets 201 with {}", method: http.MethodPost, path: templatesTestNS, + body: `{"apiVersion":"aggregation.coder.com/v1alpha1","kind":"CoderTemplate","metadata":{"name":"default.proxied"},"spec":{"organization":"default","versionID":"33333333-3333-3333-3333-333333333333"}}`, + coderMethod: http.MethodPost, coderPath: func(p string) bool { return strings.HasSuffix(p, "/templates") }, + answer: jsonBody(http.StatusCreated, `{}`), wantCode: http.StatusGatewayTimeout, wantMessage: "uncertain", + }, + { + name: "workspace list gets an object without names", method: http.MethodGet, path: workspacesTestNS, + coderMethod: http.MethodGet, coderPath: func(p string) bool { return p == "/api/v2/workspaces" }, + answer: jsonBody(http.StatusOK, `{"workspaces":[{}],"count":1}`), wantCode: http.StatusInternalServerError, wantMessage: "the Coder API answered", + }, + { + name: "template get gets a template without an organization name", method: http.MethodGet, path: templatesTestNS + "/" + testTemplateName, + coderMethod: http.MethodGet, coderPath: func(p string) bool { return strings.HasSuffix(p, "/templates/my-template") }, + answer: jsonBody(http.StatusOK, `{"name":"my-template"}`), wantCode: http.StatusInternalServerError, wantMessage: "the Coder API answered", + }, + } { + override := func(w http.ResponseWriter, r *http.Request) bool { + if r.Method != tc.coderMethod || !tc.coderPath(r.URL.Path) { + return false + } + tc.answer(w) + return true + } + mock.override.Store(&override) + + code, body := f.server.do(t, cert, tc.method, tc.path, alice, tc.body) + mock.override.Store(nil) + got := decodeStatus(t, body) + if code != int(tc.wantCode) || got.Code != tc.wantCode || !strings.Contains(got.Message, tc.wantMessage) || strings.Contains(body, coderHost) { + t.Errorf("%s: expected %d with %q and without the Coder URL, got %d: %s", tc.name, tc.wantCode, tc.wantMessage, code, body) + } + } +} diff --git a/internal/app/apiserverapp/integration_test.go b/internal/app/apiserverapp/integration_test.go index 88703d4a7..45595b579 100644 --- a/internal/app/apiserverapp/integration_test.go +++ b/internal/app/apiserverapp/integration_test.go @@ -295,6 +295,8 @@ type integrationMockCoderServer struct { holdLogs atomic.Pointer[chan struct{}] // heldLogs counts build-log requests currently blocked by holdLogs. heldLogs atomic.Int32 + // override, when set, answers every request for which it returns true. + override atomic.Pointer[func(http.ResponseWriter, *http.Request) bool] } // integrationMockBuildLog is the latest build's log served by the integration mock. @@ -410,6 +412,10 @@ func newIntegrationMockCoderServer(expectedSessionToken string) *integrationMock return } + if override := mock.override.Load(); override != nil && (*override)(w, r) { + return + } + segments := splitPath(r.URL.Path) switch { case r.Method == http.MethodGet && hasSegments(segments, "api", "v2", "organizations") && len(segments) == 4: From a48c06dfc6dd68c36958f2e638d04df065fce7a3 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 11:55:27 +0000 Subject: [PATCH 02/27] test: reproduce the LIST error of an unconfigured standalone server (#215) Change-Id: I2d537eae9e3e647dfad15af6254d8085d822b967 Signed-off-by: Thomas Kosiewski --- .../unconfigured_namespace_test.go | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 internal/app/apiserverapp/unconfigured_namespace_test.go diff --git a/internal/app/apiserverapp/unconfigured_namespace_test.go b/internal/app/apiserverapp/unconfigured_namespace_test.go new file mode 100644 index 000000000..6af268851 --- /dev/null +++ b/internal/app/apiserverapp/unconfigured_namespace_test.go @@ -0,0 +1,57 @@ +package apiserverapp + +import ( + "context" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime/serializer" + genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/apiserver/pkg/registry/rest" + + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" +) + +// TestUnconfiguredServerListsNamespacesAsEmpty checks that a standalone server without a Coder URL +// and token returns empty namespaced lists, so the namespace controller can delete a namespace +// (#215). Other requests keep the 503 that names the missing configuration. +func TestUnconfiguredServerListsNamespacesAsEmpty(t *testing.T) { + t.Parallel() + + provider, err := buildClientProvider(Options{}, 30*time.Second) + if err != nil { + t.Fatal(err) + } + scheme := NewScheme() + apiGroupInfo, err := NewAPIGroupInfo(scheme, serializer.NewCodecFactory(scheme), provider) + if err != nil { + t.Fatal(err) + } + storages := apiGroupInfo.VersionedResourcesStorageMap[aggregationv1alpha1.SchemeGroupVersion.Version] + ctx := genericapirequest.WithNamespace(context.Background(), "team-a") + + for resource, name := range map[string]string{ + "coderworkspaces": "default.alice.dev", + "codertemplates": "default.starter", + "codertemplateversions": "default.starter.v1", + } { + list, err := storages[resource].(rest.Lister).List(ctx, nil) + if err != nil { + t.Fatalf("%s: expected an empty list, got %v", resource, err) + } + if items, err := meta.ExtractList(list); err != nil || len(items) != 0 { + t.Fatalf("%s: expected an empty list, got %d items (%v)", resource, len(items), err) + } + + if _, err := storages[resource].(rest.Getter).Get(ctx, name, nil); !apierrors.IsServiceUnavailable(err) { + t.Fatalf("%s: expected get to return 503, got %v", resource, err) + } + } + + allNamespaces := genericapirequest.WithNamespace(context.Background(), "") + if _, err := storages["coderworkspaces"].(rest.Lister).List(allNamespaces, nil); !apierrors.IsServiceUnavailable(err) { + t.Fatalf("expected an all-namespaces list to return 503, got %v", err) + } +} From 5c1f60ad0b86f845ed412550a545172324485692 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 11:55:28 +0000 Subject: [PATCH 03/27] fix: map uncertain Coder write answers to 504 and reject nameless Coder objects (#242, #215) Change-Id: Ia9caeff7cd8cb9d5d526313d364258ba775e6553 Signed-off-by: Thomas Kosiewski --- .../aggregated/coder/controlplane_provider.go | 2 +- internal/aggregated/coder/errors.go | 88 +++++++++++--- internal/aggregated/coder/errors_test.go | 108 ++++++++++++------ internal/aggregated/coder/names.go | 16 ++- internal/aggregated/coder/provider.go | 6 +- internal/aggregated/convert/answer_test.go | 75 ++++++++++++ internal/aggregated/convert/template.go | 24 +++- internal/aggregated/convert/template_test.go | 2 +- .../aggregated/convert/templateversion.go | 15 ++- .../convert/templateversion_test.go | 16 +-- internal/aggregated/convert/workspace.go | 10 +- internal/aggregated/convert/workspace_test.go | 4 +- .../convert/workspace_version_test.go | 20 ++-- internal/aggregated/storage/errors.go | 9 ++ internal/aggregated/storage/template.go | 45 +++++--- .../aggregated/storage/templateversion.go | 12 +- internal/aggregated/storage/workspace.go | 57 ++++++--- .../storage/workspace_transition.go | 17 +-- internal/app/apiserverapp/apiserverapp.go | 10 +- 19 files changed, 400 insertions(+), 136 deletions(-) create mode 100644 internal/aggregated/convert/answer_test.go diff --git a/internal/aggregated/coder/controlplane_provider.go b/internal/aggregated/coder/controlplane_provider.go index 828540b3d..fab6ff99f 100644 --- a/internal/aggregated/coder/controlplane_provider.go +++ b/internal/aggregated/coder/controlplane_provider.go @@ -87,7 +87,7 @@ func (p *ControlPlaneClientProvider) ClientForNamespace(ctx context.Context, nam if namespace == "" || listed > 0 { return nil, unavailable } - return nil, newNamespaceNotServedError(unavailable) + return nil, NewNamespaceNotServedError(unavailable) case 1: // handled below default: diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 196bbfac2..35a144289 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -1,6 +1,7 @@ package coder import ( + "crypto/tls" "errors" "fmt" "net" @@ -12,6 +13,8 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" + + "github.com/coder/coder-k8s/internal/sameorigin" ) // coderTimeoutMessage is the 504 message for a Coder call that ran out of time. It leaves out @@ -98,18 +101,41 @@ func MapCoderError(err error, resource schema.GroupResource, name string) error } } -// MapCoderCreateError maps the error of the Coder POST that creates name. A 2xx *codersdk.Error -// means that the POST got through, but the answer is not the one that Coder sends: the SDK returns -// this error for a 2xx other than the expected one, and since codersdk v2.37 also for the expected -// 2xx with a body that is not JSON (for example a proxy page or an empty body). Coder can have -// created the object, so this is a 504 that says the result is uncertain, like any other write -// that times out. It has no Retry-After, so clients do not send the create again on their own. -// Every other error maps as in MapCoderError. That includes the SDK's plain errors for a body that -// breaks off while it is read and for malformed JSON on an unexpected 2xx (#242). -func MapCoderCreateError(err error, resource schema.GroupResource, name string) error { +// Write names the Coder write in the message of an uncertain result. +type Write string + +// The writes of the aggregated storages. +const ( + WriteCreate Write = "create" + WriteUpdate Write = "update" + WriteDelete Write = "delete" +) + +// IsUncertainStatus reports whether a Coder or proxy status leaves it open whether Coder applied +// a write. A 2xx status means that the request got through, but the answer is not the one that +// Coder sends: the SDK returns a *codersdk.Error for a 2xx other than the expected one, and since +// codersdk v2.37 also for the expected 2xx with a body that is not JSON (for example a proxy page +// or an empty body). A proxy in front of Coder can answer 502, 503 or 504 after Coder applied the +// write. +func IsUncertainStatus(status int) bool { + return (status >= http.StatusOK && status < http.StatusMultipleChoices) || + status == http.StatusBadGateway || status == http.StatusServiceUnavailable || status == http.StatusGatewayTimeout +} + +// MapCoderWriteError maps the error of the Coder call that creates, updates or deletes name. +// When the error leaves it open whether Coder applied the write, it returns the uncertain 504 of +// UncertainWriteError. Those are the statuses of IsUncertainStatus except 504, and every error +// without a status that the request can have caused after it was sent: a connection that breaks +// before or while the answer is read, or a body that is not the expected JSON. A timeout or a +// Coder 504 keeps the 504 of MapCoderError, which says the same to clients. So does every +// other error, including a request that provably never reached Coder (see notSent). +func MapCoderWriteError(err error, write Write, resource schema.GroupResource, name string) error { if err == nil { return fmt.Errorf("assertion failed: error must not be nil") } + if write == "" { + return fmt.Errorf("assertion failed: write must not be empty") + } if resource.Empty() { return fmt.Errorf("assertion failed: resource must not be empty") } @@ -118,15 +144,45 @@ func MapCoderCreateError(err error, resource schema.GroupResource, name string) } var coderErr *codersdk.Error - if errors.As(err, &coderErr) && coderErr.StatusCode() >= http.StatusOK && coderErr.StatusCode() < http.StatusMultipleChoices { - // The message leaves out the Coder URL. - return apierrors.NewTimeoutError(fmt.Sprintf( - "the result is uncertain: the Coder API answered the create of %s %q with status %d, but not with the answer that Coder sends; re-read the object with kubectl get before you retry", - resource.Resource, name, coderErr.StatusCode(), - ), 0) + if errors.As(err, &coderErr) { + status := coderErr.StatusCode() + if status != http.StatusGatewayTimeout && IsUncertainStatus(status) { + logHiddenError(err, resource, name, status) + return UncertainWriteError(write, resource, name, fmt.Sprintf("status %d from the Coder API", status)) + } + return MapCoderError(err, resource, name) } - return MapCoderError(err, resource, name) + var netErr net.Error + if (errors.As(err, &netErr) && netErr.Timeout()) || notSent(err) { + return MapCoderError(err, resource, name) + } + logHiddenError(err, resource, name, 0) + return UncertainWriteError(write, resource, name, "an answer from the Coder API that broke off or could not be read") +} + +// UncertainWriteError is the 504 for a write that Coder can have applied. reason names the answer +// that the write got, for example "status 502 from the Coder API", and must not name the Coder URL. It has no Retry-After, so clients do not send the write +// again on their own. +func UncertainWriteError(write Write, resource schema.GroupResource, name, reason string) error { + return apierrors.NewTimeoutError(fmt.Sprintf( + "the result is uncertain: the %s of %s %q got %s; re-read the object with kubectl get before you retry", + write, resource.Resource, name, reason, + ), 0) +} + +// notSent reports whether err proves that the request did not reach Coder: the connection to +// Coder (or to the proxy in front of it) failed, the TLS handshake failed, or the client refused +// a request or a redirect to another origin. A redirect answer does not apply a write. +func notSent(err error) bool { + var opErr *net.OpError + if errors.As(err, &opErr) && (opErr.Op == "dial" || opErr.Op == "proxyconnect") { + return true + } + var certErr *tls.CertificateVerificationError + var recordErr tls.RecordHeaderError + return errors.As(err, &certErr) || errors.As(err, &recordErr) || + errors.Is(err, sameorigin.ErrCrossOrigin) || errors.Is(err, sameorigin.ErrCrossOriginRequest) } // coderErrorMessage returns Coder's message for a 4xx answer. Without one, it returns a fixed diff --git a/internal/aggregated/coder/errors_test.go b/internal/aggregated/coder/errors_test.go index e4992dd14..b48363dd4 100644 --- a/internal/aggregated/coder/errors_test.go +++ b/internal/aggregated/coder/errors_test.go @@ -2,15 +2,20 @@ package coder import ( "context" + "crypto/tls" + "encoding/json" "errors" "fmt" + "io" "net" "net/http" "net/url" "strings" + "syscall" "testing" aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" + "github.com/coder/coder-k8s/internal/sameorigin" "github.com/coder/coder/v2/codersdk" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime/schema" @@ -243,61 +248,100 @@ func withCoderMessage(err *codersdk.Error, message string) *codersdk.Error { return err } -func TestMapCoderCreateError(t *testing.T) { +func TestMapCoderWriteError(t *testing.T) { t.Parallel() resource := aggregationv1alpha1.Resource("codertemplates") name := "acme.docker" + const coderURL = "https://coder.example.com" + sdkErr := func(status int) error { + return fmt.Errorf("write: %w", codersdk.NewTestError(status, http.MethodPost, coderURL)) + } + afterSend := func(err error) error { return &url.Error{Op: "Post", URL: coderURL, Err: err} } + dial := func(err error) error { + return afterSend(&net.OpError{Op: "dial", Net: "tcp", Err: err}) + } - // Since codersdk v2.37, a 2xx whose body is not JSON (for example a proxy page) is a - // *codersdk.Error with that status. Coder can have created the object. - for _, status := range []int{http.StatusOK, http.StatusCreated, http.StatusNoContent} { - err := MapCoderCreateError(fmt.Errorf("create: %w", codersdk.NewTestError(status, http.MethodPost, "https://coder.example.com")), resource, name) + const ( + uncertain = "uncertain" + unreachable = "could not be reached" + timedOut = "did not answer in time" + ) + for _, tc := range []struct { + name string + err error + wantMsg string + }{ + // A 2xx *codersdk.Error is an answer that Coder does not send. Coder can have applied it. + {"200", sdkErr(http.StatusOK), uncertain}, + {"201", sdkErr(http.StatusCreated), uncertain}, + {"204", sdkErr(http.StatusNoContent), uncertain}, + {"proxy 502", sdkErr(http.StatusBadGateway), uncertain}, + {"proxy 503", sdkErr(http.StatusServiceUnavailable), uncertain}, + {"body breaks off", fmt.Errorf("read response body: %w", io.ErrUnexpectedEOF), uncertain}, + {"malformed JSON on an unexpected 2xx", fmt.Errorf("decode body: %w", &json.SyntaxError{}), uncertain}, + {"connection closed without an answer", afterSend(io.EOF), uncertain}, + {"connection reset after the send", afterSend(&net.OpError{Op: "read", Net: "tcp", Err: syscall.ECONNRESET}), uncertain}, + {"connection refused", dial(syscall.ECONNREFUSED), unreachable}, + {"DNS failure", dial(&net.DNSError{Err: "no such host", Name: "coder.example.com", IsNotFound: true}), unreachable}, + {"proxy unreachable", afterSend(&net.OpError{Op: "proxyconnect", Net: "tcp", Err: syscall.ECONNREFUSED}), unreachable}, + {"certificate rejected", afterSend(&tls.CertificateVerificationError{Err: errors.New("unknown authority")}), unreachable}, + {"redirect to another origin", afterSend(sameorigin.ErrCrossOrigin), unreachable}, + {"request to another origin", afterSend(sameorigin.ErrCrossOriginRequest), unreachable}, + {"timeout", afterSend(context.DeadlineExceeded), timedOut}, + {"Coder 504", sdkErr(http.StatusGatewayTimeout), timedOut}, + } { + err := MapCoderWriteError(tc.err, WriteDelete, resource, name) var statusErr *apierrors.StatusError - if !errors.As(err, &statusErr) || !apierrors.IsTimeout(err) { - t.Fatalf("status %d: expected a 504 Timeout, got %T %v", status, err, err) + if !errors.As(err, &statusErr) { + t.Fatalf("%s: expected a status error, got %T %v", tc.name, err, err) } message := statusErr.ErrStatus.Message - for _, want := range []string{"uncertain", "re-read", `codertemplates "acme.docker"`, fmt.Sprint(status)} { - if !strings.Contains(message, want) { - t.Fatalf("status %d: message %q does not contain %q", status, message, want) - } - } - if strings.Contains(message, "coder.example.com") { - t.Fatalf("status %d: message %q must not contain the Coder URL", status, message) + if !strings.Contains(message, tc.wantMsg) || strings.Contains(message, "coder.example.com") { + t.Fatalf("%s: expected a message with %q and without the Coder URL, got %q", tc.name, tc.wantMsg, message) } - if details := statusErr.ErrStatus.Details; details != nil && details.RetryAfterSeconds != 0 { - t.Fatalf("status %d: an uncertain create must not ask clients to retry on their own, got Retry-After %d", status, details.RetryAfterSeconds) + switch tc.wantMsg { + case uncertain, timedOut: + if !apierrors.IsTimeout(err) { + t.Fatalf("%s: expected a 504, got %v", tc.name, err) + } + if tc.wantMsg == uncertain && !strings.Contains(message, `delete of codertemplates "acme.docker"`) { + t.Fatalf("%s: message %q must name the write and the object", tc.name, message) + } + if details := statusErr.ErrStatus.Details; details != nil && details.RetryAfterSeconds != 0 { + t.Fatalf("%s: an uncertain write must not ask clients to retry on their own, got Retry-After %d", tc.name, details.RetryAfterSeconds) + } + case unreachable: + if !apierrors.IsInternalError(err) { + t.Fatalf("%s: expected a 500, got %v", tc.name, err) + } } } - // Every other error maps as in MapCoderError. - for _, err := range []error{ - codersdk.NewTestError(http.StatusConflict, http.MethodPost, "https://coder.example.com"), - codersdk.NewTestError(http.StatusBadRequest, http.MethodPost, "https://coder.example.com"), - codersdk.NewTestError(http.StatusBadGateway, http.MethodPost, "https://coder.example.com"), - codersdk.NewTestError(http.StatusMultipleChoices, http.MethodPost, "https://coder.example.com"), - errors.New("transport failed"), - } { - got, want := MapCoderCreateError(err, resource, name), MapCoderError(err, resource, name) + // A Coder answer with a status that leaves no doubt maps as in MapCoderError. + for _, status := range []int{http.StatusConflict, http.StatusBadRequest, http.StatusInternalServerError, http.StatusMultipleChoices} { + err := sdkErr(status) + got, want := MapCoderWriteError(err, WriteCreate, resource, name), MapCoderError(err, resource, name) if got.Error() != want.Error() || apierrors.ReasonForError(got) != apierrors.ReasonForError(want) { - t.Fatalf("%v: expected the MapCoderError mapping %v, got %v", err, want, got) + t.Fatalf("%d: expected the MapCoderError mapping %v, got %v", status, want, got) } } - // The assertions run before the 2xx branch, so a 2xx error cannot hide them. - ok := codersdk.NewTestError(http.StatusOK, http.MethodPost, "https://coder.example.com") + // The assertions run before the uncertain branches, so an uncertain error cannot hide them. + ok := codersdk.NewTestError(http.StatusOK, http.MethodPost, coderURL) for _, tc := range []struct { err error + write Write resource schema.GroupResource name string want string }{ - {nil, resource, name, "assertion failed: error must not be nil"}, - {ok, schema.GroupResource{}, name, "assertion failed: resource must not be empty"}, - {ok, resource, "", "assertion failed: resource name must not be empty"}, + {nil, WriteCreate, resource, name, "assertion failed: error must not be nil"}, + {ok, "", resource, name, "assertion failed: write must not be empty"}, + {ok, WriteCreate, schema.GroupResource{}, name, "assertion failed: resource must not be empty"}, + {ok, WriteCreate, resource, "", "assertion failed: resource name must not be empty"}, } { - if err := MapCoderCreateError(tc.err, tc.resource, tc.name); err == nil || apierrors.IsTimeout(err) || !strings.Contains(err.Error(), tc.want) { + if err := MapCoderWriteError(tc.err, tc.write, tc.resource, tc.name); err == nil || apierrors.IsTimeout(err) || !strings.Contains(err.Error(), tc.want) { t.Fatalf("expected %q, got %v", tc.want, err) } } diff --git a/internal/aggregated/coder/names.go b/internal/aggregated/coder/names.go index 5fd200ca0..480291be4 100644 --- a/internal/aggregated/coder/names.go +++ b/internal/aggregated/coder/names.go @@ -122,11 +122,21 @@ func parseNameSegments(name string, expectedSegments int, objectType string) ([] return segments, nil } -func assertNameSegment(segmentType, value string) { +// NameSegmentError reports why value cannot be the segmentType segment of a name. Converters +// check the fields of Coder answers with it, so a broken answer is an error and not the panic of +// the Build functions. +func NameSegmentError(segmentType, value string) error { if value == "" { - panic(fmt.Sprintf("assertion failed: %s must not be empty", segmentType)) + return fmt.Errorf("%s must not be empty", segmentType) } if strings.Contains(value, nameSeparator) { - panic(fmt.Sprintf("assertion failed: %s must not contain %q", segmentType, nameSeparator)) + return fmt.Errorf("%s must not contain %q", segmentType, nameSeparator) + } + return nil +} + +func assertNameSegment(segmentType, value string) { + if err := NameSegmentError(segmentType, value); err != nil { + panic("assertion failed: " + err.Error()) } } diff --git a/internal/aggregated/coder/provider.go b/internal/aggregated/coder/provider.go index 5f4d29948..193c2d5fd 100644 --- a/internal/aggregated/coder/provider.go +++ b/internal/aggregated/coder/provider.go @@ -39,7 +39,9 @@ func (e *namespaceNotServedError) Error() string { return e.status.Error() } func (e *namespaceNotServedError) Unwrap() error { return e.status } -func newNamespaceNotServedError(status *apierrors.StatusError) error { +// NewNamespaceNotServedError marks status as the ClientForNamespace error for a namespace that no +// Coder backend serves. +func NewNamespaceNotServedError(status *apierrors.StatusError) error { if status == nil { panic("assertion failed: namespace-not-served status error must not be nil") } @@ -86,7 +88,7 @@ func (p *StaticClientProvider) ClientForNamespace(ctx context.Context, namespace namespace = p.Namespace } if namespace != p.Namespace { - return nil, newNamespaceNotServedError(apierrors.NewBadRequest( + return nil, NewNamespaceNotServedError(apierrors.NewBadRequest( fmt.Sprintf( "namespace %q is not served by this aggregated API server (configured for %q)", namespace, diff --git a/internal/aggregated/convert/answer_test.go b/internal/aggregated/convert/answer_test.go new file mode 100644 index 000000000..52527c045 --- /dev/null +++ b/internal/aggregated/convert/answer_test.go @@ -0,0 +1,75 @@ +package convert + +import ( + "strings" + "testing" + + "github.com/google/uuid" + + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" + "github.com/coder/coder/v2/codersdk" +) + +func mustTemplateToK8s(t *testing.T, namespace string, template codersdk.Template) *aggregationv1alpha1.CoderTemplate { + t.Helper() + obj, err := TemplateToK8s(namespace, template) + if err != nil { + t.Fatalf("convert template: %v", err) + } + return obj +} + +func mustWorkspaceToK8s(t *testing.T, namespace string, workspace codersdk.Workspace) *aggregationv1alpha1.CoderWorkspace { + t.Helper() + obj, err := WorkspaceToK8s(namespace, workspace) + if err != nil { + t.Fatalf("convert workspace: %v", err) + } + return obj +} + +func mustTemplateVersionToK8s(t *testing.T, namespace string, template codersdk.Template, version codersdk.TemplateVersion) *aggregationv1alpha1.CoderTemplateVersion { + t.Helper() + obj, err := TemplateVersionToK8s(namespace, template, version) + if err != nil { + t.Fatalf("convert template version: %v", err) + } + return obj +} + +// TestConvertersRejectAnswersWithoutNames checks that a Coder answer without the fields of the +// Kubernetes name, for example the {} of a proxy (#242), is an error and not a panic. +func TestConvertersRejectAnswersWithoutNames(t *testing.T) { + t.Parallel() + + templateID := uuid.New() + for _, tc := range []struct { + name string + convert func() error + want string + }{ + {"empty template", func() error { _, err := TemplateToK8s("ns", codersdk.Template{}); return err }, "organization must not be empty"}, + {"template name with a dot", func() error { + _, err := TemplateToK8s("ns", codersdk.Template{OrganizationName: "acme", Name: "a.b"}) + return err + }, `template must not contain "."`}, + {"empty workspace", func() error { _, err := WorkspaceToK8s("ns", codersdk.Workspace{}); return err }, "organization must not be empty"}, + {"workspace without an owner", func() error { + _, err := WorkspaceToK8s("ns", codersdk.Workspace{OrganizationName: "acme", Name: "dev"}) + return err + }, "user must not be empty"}, + {"template version without a name", func() error { + _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID}) + return err + }, "version must not be empty"}, + {"template version of a template without an organization", func() error { + _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID, Name: "v1"}) + return err + }, "organization must not be empty"}, + } { + err := tc.convert() + if err == nil || !strings.Contains(err.Error(), "the Coder API answered") || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("%s: expected an error with %q, got %v", tc.name, tc.want, err) + } + } +} diff --git a/internal/aggregated/convert/template.go b/internal/aggregated/convert/template.go index 97227cd30..e7fc01cc4 100644 --- a/internal/aggregated/convert/template.go +++ b/internal/aggregated/convert/template.go @@ -13,11 +13,29 @@ import ( "k8s.io/apimachinery/pkg/types" ) -// TemplateToK8s converts a codersdk.Template to an aggregated API CoderTemplate. -func TemplateToK8s(namespace string, t codersdk.Template) *aggregationv1alpha1.CoderTemplate { +// nameField is a field of a Coder answer that becomes a segment of the Kubernetes name. +type nameField struct{ segment, value string } + +// requireNameFields returns an error when a field cannot be a segment of the name, for example in +// the {} that a proxy answers instead of Coder. +func requireNameFields(object string, fields ...nameField) error { + for _, field := range fields { + if err := coder.NameSegmentError(field.segment, field.value); err != nil { + return fmt.Errorf("the Coder API answered a %s without a valid name: %w", object, err) + } + } + return nil +} + +// TemplateToK8s converts a codersdk.Template to an aggregated API CoderTemplate. It returns an +// error when t lacks the fields of the name. +func TemplateToK8s(namespace string, t codersdk.Template) (*aggregationv1alpha1.CoderTemplate, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") } + if err := requireNameFields("template", nameField{"organization", t.OrganizationName}, nameField{"template", t.Name}); err != nil { + return nil, err + } updatedAt := metav1.NewTime(t.UpdatedAt) @@ -47,7 +65,7 @@ func TemplateToK8s(namespace string, t codersdk.Template) *aggregationv1alpha1.C Deprecated: t.Deprecated, UpdatedAt: &updatedAt, }, - } + }, nil } // TemplateCreateRequestFromK8s builds a codersdk.CreateTemplateRequest from a K8s CoderTemplate. diff --git a/internal/aggregated/convert/template_test.go b/internal/aggregated/convert/template_test.go index 6547b972a..9c18d8c1d 100644 --- a/internal/aggregated/convert/template_test.go +++ b/internal/aggregated/convert/template_test.go @@ -32,7 +32,7 @@ func TestTemplateToK8s(t *testing.T) { Deprecated: true, } - converted := TemplateToK8s("control-plane", template) + converted := mustTemplateToK8s(t, "control-plane", template) if converted == nil { t.Fatal("expected non-nil converted template") } diff --git a/internal/aggregated/convert/templateversion.go b/internal/aggregated/convert/templateversion.go index 78431ca9e..9074f9812 100644 --- a/internal/aggregated/convert/templateversion.go +++ b/internal/aggregated/convert/templateversion.go @@ -29,16 +29,25 @@ const ( // metadata.resourceVersion is a fingerprint (hex SHA-256) of the returned object. It changes when // any returned field changes, including the active flag, which Coder does not reflect in the // version's own updated_at. Compare it only for equality. -func TemplateVersionToK8s(namespace string, tpl codersdk.Template, v codersdk.TemplateVersion) *aggregationv1alpha1.CoderTemplateVersion { +// +// It returns an error when tpl or v lack the fields of the name and labels. +func TemplateVersionToK8s(namespace string, tpl codersdk.Template, v codersdk.TemplateVersion) (*aggregationv1alpha1.CoderTemplateVersion, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") } if v.TemplateID == nil || *v.TemplateID != tpl.ID { panic(fmt.Sprintf("assertion failed: template version %s must belong to template %s", v.ID, tpl.ID)) } + if err := requireNameFields("template version", + nameField{"organization", tpl.OrganizationName}, nameField{"template", tpl.Name}); err != nil { + return nil, err + } + if v.Name == "" { + return nil, fmt.Errorf("the Coder API answered a template version without a valid name: version must not be empty") + } for _, value := range []string{tpl.OrganizationName, tpl.Name} { if problems := validation.IsValidLabelValue(value); len(problems) > 0 { - panic(fmt.Sprintf("assertion failed: %q must be a valid label value: %s", value, strings.Join(problems, "; "))) + return nil, fmt.Errorf("the Coder API answered a template version whose %q is not a valid label value: %s", value, strings.Join(problems, "; ")) } } @@ -87,7 +96,7 @@ func TemplateVersionToK8s(namespace string, tpl codersdk.Template, v codersdk.Te sum := sha256.Sum256(serialized) obj.ResourceVersion = hex.EncodeToString(sum[:]) - return obj + return obj, nil } func optionalTime(value *time.Time) *metav1.Time { diff --git a/internal/aggregated/convert/templateversion_test.go b/internal/aggregated/convert/templateversion_test.go index 21b6a3f74..b065a4545 100644 --- a/internal/aggregated/convert/templateversion_test.go +++ b/internal/aggregated/convert/templateversion_test.go @@ -39,7 +39,7 @@ func TestTemplateVersionToK8s(t *testing.T) { t.Parallel() tpl, v := templateVersionFixture() - obj := TemplateVersionToK8s("coder", tpl, v) + obj := mustTemplateVersionToK8s(t, "coder", tpl, v) if obj.Name != "acme.docker.v1.2.3" || obj.Namespace != "coder" || string(obj.UID) != v.ID.String() { t.Fatalf("unexpected identity: name=%q namespace=%q uid=%q", obj.Name, obj.Namespace, obj.UID) @@ -77,14 +77,14 @@ func TestTemplateVersionToK8sFingerprint(t *testing.T) { t.Parallel() tpl, v := templateVersionFixture() - base := TemplateVersionToK8s("coder", tpl, v).ResourceVersion - if again := TemplateVersionToK8s("coder", tpl, v).ResourceVersion; again != base { + base := mustTemplateVersionToK8s(t, "coder", tpl, v).ResourceVersion + if again := mustTemplateVersionToK8s(t, "coder", tpl, v).ResourceVersion; again != base { t.Fatalf("fingerprint is not stable: %q != %q", again, base) } activeTemplate := tpl activeTemplate.ActiveVersionID = v.ID - active := TemplateVersionToK8s("coder", activeTemplate, v) + active := mustTemplateVersionToK8s(t, "coder", activeTemplate, v) if !active.Status.Active { t.Fatal("expected the active version to report active=true") } @@ -99,9 +99,9 @@ func TestTemplateVersionToK8sFingerprint(t *testing.T) { seen := map[string]string{base: "base"} for label, rv := range map[string]string{ "active": active.ResourceVersion, - "archived": TemplateVersionToK8s("coder", tpl, archived).ResourceVersion, - "succeeded": TemplateVersionToK8s("coder", tpl, succeeded).ResourceVersion, - "renamed": TemplateVersionToK8s("coder", tpl, renamed).ResourceVersion, + "archived": mustTemplateVersionToK8s(t, "coder", tpl, archived).ResourceVersion, + "succeeded": mustTemplateVersionToK8s(t, "coder", tpl, succeeded).ResourceVersion, + "renamed": mustTemplateVersionToK8s(t, "coder", tpl, renamed).ResourceVersion, } { if previous, ok := seen[rv]; ok { t.Fatalf("fingerprint for %s equals the one for %s", label, previous) @@ -122,5 +122,5 @@ func TestTemplateVersionToK8sPanicsOnForeignVersion(t *testing.T) { t.Fatalf("expected an assertion panic, got %v", recovered) } }() - TemplateVersionToK8s("coder", tpl, v) + _, _ = TemplateVersionToK8s("coder", tpl, v) } diff --git a/internal/aggregated/convert/workspace.go b/internal/aggregated/convert/workspace.go index 7c3b67bfe..77da0677b 100644 --- a/internal/aggregated/convert/workspace.go +++ b/internal/aggregated/convert/workspace.go @@ -22,10 +22,16 @@ import ( // this API exposes changes the token even when Coder's Workspace.UpdatedAt stays the same, and a // backend-only change that leaves the projection identical keeps the token. It is not monotonic // and not a history cursor: an identical projection yields the same token again. -func WorkspaceToK8s(namespace string, w codersdk.Workspace) *aggregationv1alpha1.CoderWorkspace { +// +// It returns an error when w lacks the fields of the name. +func WorkspaceToK8s(namespace string, w codersdk.Workspace) (*aggregationv1alpha1.CoderWorkspace, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") } + if err := requireNameFields("workspace", + nameField{"organization", w.OrganizationName}, nameField{"user", w.OwnerName}, nameField{"workspace", w.Name}); err != nil { + return nil, err + } var autoShutdown *metav1.Time if w.LatestBuild.Deadline.Valid && !w.LatestBuild.Deadline.Time.IsZero() { @@ -74,7 +80,7 @@ func WorkspaceToK8s(namespace string, w codersdk.Workspace) *aggregationv1alpha1 sum := sha256.Sum256(serialized) obj.ResourceVersion = hex.EncodeToString(sum[:]) - return obj + return obj, nil } func workspaceRunning(workspace codersdk.Workspace) bool { diff --git a/internal/aggregated/convert/workspace_test.go b/internal/aggregated/convert/workspace_test.go index caf8e1339..92c5005bd 100644 --- a/internal/aggregated/convert/workspace_test.go +++ b/internal/aggregated/convert/workspace_test.go @@ -40,7 +40,7 @@ func TestWorkspaceToK8s(t *testing.T) { }, } - converted := WorkspaceToK8s("control-plane", workspace) + converted := mustWorkspaceToK8s(t, "control-plane", workspace) if converted == nil { t.Fatal("expected non-nil converted workspace") } @@ -166,7 +166,7 @@ func TestWorkspaceToK8sRunningStateFromTransitionAndStatus(t *testing.T) { }, } - converted := WorkspaceToK8s("control-plane", workspace) + converted := mustWorkspaceToK8s(t, "control-plane", workspace) if converted.Spec.Running != testCase.running { t.Fatalf( "expected running=%t for transition=%q status=%q, got %t", diff --git a/internal/aggregated/convert/workspace_version_test.go b/internal/aggregated/convert/workspace_version_test.go index f63b872d3..364ba1320 100644 --- a/internal/aggregated/convert/workspace_version_test.go +++ b/internal/aggregated/convert/workspace_version_test.go @@ -44,7 +44,7 @@ func versionFixtureWorkspace() codersdk.Workspace { func TestWorkspaceToK8sResourceVersionIsOpaqueSHA256(t *testing.T) { t.Parallel() - converted := WorkspaceToK8s("control-plane", versionFixtureWorkspace()) + converted := mustWorkspaceToK8s(t, "control-plane", versionFixtureWorkspace()) if len(converted.ResourceVersion) != hex.EncodedLen(32) { t.Fatalf("expected a full hex SHA-256 resourceVersion, got %q", converted.ResourceVersion) } @@ -63,8 +63,8 @@ func TestWorkspaceToK8sResourceVersionStableForIdenticalProjection(t *testing.T) t.Parallel() base := versionFixtureWorkspace() - first := WorkspaceToK8s("control-plane", base).ResourceVersion - second := WorkspaceToK8s("control-plane", base).ResourceVersion + first := mustWorkspaceToK8s(t, "control-plane", base).ResourceVersion + second := mustWorkspaceToK8s(t, "control-plane", base).ResourceVersion if first != second { t.Fatalf("identical input must yield identical resourceVersion: %q vs %q", first, second) } @@ -75,7 +75,7 @@ func TestWorkspaceToK8sResourceVersionStableForIdenticalProjection(t *testing.T) backendOnly.LatestBuild.UpdatedAt = base.LatestBuild.UpdatedAt.Add(time.Hour) backendOnly.LatestBuild.CreatedAt = base.LatestBuild.CreatedAt.Add(time.Hour) backendOnly.LatestBuild.BuildNumber = 7 - if got := WorkspaceToK8s("control-plane", backendOnly).ResourceVersion; got != first { + if got := mustWorkspaceToK8s(t, "control-plane", backendOnly).ResourceVersion; got != first { t.Fatalf("backend-only timestamp/build-number change must keep the token, got %q want %q", got, first) } @@ -85,7 +85,7 @@ func TestWorkspaceToK8sResourceVersionStableForIdenticalProjection(t *testing.T) changed.TTLMillis = &ttl reverted := changed reverted.TTLMillis = base.TTLMillis - if got := WorkspaceToK8s("control-plane", reverted).ResourceVersion; got != first { + if got := mustWorkspaceToK8s(t, "control-plane", reverted).ResourceVersion; got != first { t.Fatalf("reverted projection must return the original token, got %q want %q", got, first) } } @@ -94,7 +94,7 @@ func TestWorkspaceToK8sResourceVersionTracksExposedFields(t *testing.T) { t.Parallel() base := versionFixtureWorkspace() - baseVersion := WorkspaceToK8s("control-plane", base).ResourceVersion + baseVersion := mustWorkspaceToK8s(t, "control-plane", base).ResourceVersion newBuildID := uuid.MustParse("22222222-2222-2222-2222-222222222222") newTemplateVersionID := uuid.MustParse("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb") @@ -128,7 +128,7 @@ func TestWorkspaceToK8sResourceVersionTracksExposedFields(t *testing.T) { w := base mutate(&w) - converted := WorkspaceToK8s("control-plane", w) + converted := mustWorkspaceToK8s(t, "control-plane", w) if converted.ResourceVersion == baseVersion { t.Fatalf("exposed field changed but resourceVersion stayed %q", baseVersion) } @@ -143,11 +143,11 @@ func TestWorkspaceToK8sResourceVersionIdentityAndNamespace(t *testing.T) { t.Parallel() base := versionFixtureWorkspace() - converted := WorkspaceToK8s("control-plane", base) + converted := mustWorkspaceToK8s(t, "control-plane", base) recreated := base recreated.ID = uuid.MustParse("33333333-3333-3333-3333-333333333333") - recreatedConverted := WorkspaceToK8s("control-plane", recreated) + recreatedConverted := mustWorkspaceToK8s(t, "control-plane", recreated) if recreatedConverted.UID == converted.UID { t.Fatal("recreated workspace (new ID, same name) must expose a new UID") } @@ -155,7 +155,7 @@ func TestWorkspaceToK8sResourceVersionIdentityAndNamespace(t *testing.T) { t.Fatal("recreated workspace must expose a different resourceVersion") } - if other := WorkspaceToK8s("other-namespace", base); other.ResourceVersion == converted.ResourceVersion { + if other := mustWorkspaceToK8s(t, "other-namespace", base); other.ResourceVersion == converted.ResourceVersion { t.Fatal("namespace is part of the projection and must change the resourceVersion") } } diff --git a/internal/aggregated/storage/errors.go b/internal/aggregated/storage/errors.go index f64242655..0880b39db 100644 --- a/internal/aggregated/storage/errors.go +++ b/internal/aggregated/storage/errors.go @@ -10,6 +10,8 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apiserver/pkg/storage" storageerrors "k8s.io/apiserver/pkg/storage/errors" + + "github.com/coder/coder-k8s/internal/aggregated/coder" ) // rejectDryRun refuses server-side dry-run writes for resource. Storage calls Coder directly and @@ -28,6 +30,13 @@ func rejectDryRun(resource string, dryRun []string) error { )) } +// uncertainCreateAnswer is the error for a create that Coder answered with its success status, +// but without the fields of the object, for example with {} from a proxy. Coder can have created +// the object. +func uncertainCreateAnswer(resource schema.GroupResource, name string) error { + return coder.UncertainWriteError(coder.WriteCreate, resource, name, "an answer from the Coder API without the name of the object") +} + func wrapClientError(err error) error { if err == nil { return nil diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 8e232d205..102235ce0 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -170,7 +170,10 @@ func (s *TemplateStorage) Get(ctx context.Context, name string, _ *metav1.GetOpt return nil, err } - obj := convert.TemplateToK8s(namespace, template) + obj, err := convert.TemplateToK8s(namespace, template) + if err != nil { + return nil, apierrors.NewInternalError(err) + } files, err := fetchTemplateSourceFiles(ctx, sdk, template.ActiveVersionID) if err != nil { @@ -242,7 +245,11 @@ func (s *TemplateStorage) List(ctx context.Context, opts *metainternalversion.Li } for _, template := range templates { - if item := convert.TemplateToK8s(eligibleNamespace, template); keep(item) { + item, err := convert.TemplateToK8s(eligibleNamespace, template) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + if keep(item) { list.Items = append(list.Items, *item) } } @@ -292,7 +299,11 @@ func (s *TemplateStorage) List(ctx context.Context, opts *metainternalversion.Li } for _, template := range templates { - if item := convert.TemplateToK8s(responseNamespace, template); keep(item) { + item, err := convert.TemplateToK8s(responseNamespace, template) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + if keep(item) { list.Items = append(list.Items, *item) } } @@ -473,12 +484,12 @@ func (s *TemplateStorage) Create( Icon: templateObj.Spec.Icon, }) if err != nil { - return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + return nil, coder.MapCoderWriteError(err, coder.WriteCreate, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } - result := convert.TemplateToK8s(namespace, createdTemplate) - if result == nil { - return nil, fmt.Errorf("assertion failed: converted template must not be nil") + result, err := convert.TemplateToK8s(namespace, createdTemplate) + if err != nil { + return nil, uncertainCreateAnswer(aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } s.enqueueWatchEvent(watch.Added, result.DeepCopy()) @@ -493,12 +504,12 @@ func (s *TemplateStorage) Create( createdTemplate, err := sdk.CreateTemplate(ctx, org.ID, request) if err != nil { - return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + return nil, coder.MapCoderWriteError(err, coder.WriteCreate, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } - result := convert.TemplateToK8s(namespace, createdTemplate) - if result == nil { - return nil, fmt.Errorf("assertion failed: converted template must not be nil") + result, err := convert.TemplateToK8s(namespace, createdTemplate) + if err != nil { + return nil, uncertainCreateAnswer(aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } s.enqueueWatchEvent(watch.Added, result.DeepCopy()) @@ -761,7 +772,7 @@ 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) + return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name) } // Post-condition: verify promotion succeeded. The vendored SDK silently @@ -787,7 +798,7 @@ func (s *TemplateStorage) Update( if metadataChanged { _, err := sdk.UpdateTemplateMeta(ctx, templateID, convert.TemplateUpdateMetaRequestFromK8s(updatedTemplate)) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name) } } @@ -909,9 +920,9 @@ func (s *TemplateStorage) Delete( return nil, false, err } - templateObj := convert.TemplateToK8s(namespace, template) - if templateObj == nil { - return nil, false, fmt.Errorf("assertion failed: converted template must not be nil") + templateObj, err := convert.TemplateToK8s(namespace, template) + if err != nil { + return nil, false, apierrors.NewInternalError(err) } // Preconditions and admission see the fetched snapshot; the mutation below targets that same template ID. @@ -925,7 +936,7 @@ func (s *TemplateStorage) Delete( } if err := sdk.DeleteTemplate(ctx, template.ID); err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + return nil, false, coder.MapCoderWriteError(err, coder.WriteDelete, aggregationv1alpha1.Resource("codertemplates"), name) } // Emit a Deleted event with the last-known template state. diff --git a/internal/aggregated/storage/templateversion.go b/internal/aggregated/storage/templateversion.go index 0dfa602d4..29c3b58c3 100644 --- a/internal/aggregated/storage/templateversion.go +++ b/internal/aggregated/storage/templateversion.go @@ -209,7 +209,11 @@ func (s *TemplateVersionStorage) listNamespace(ctx context.Context, namespace st if version.TemplateID == nil || *version.TemplateID != template.ID { return nil, fmt.Errorf("assertion failed: listed version %s must belong to template %s", version.ID, template.ID) } - items = append(items, *convert.TemplateVersionToK8s(namespace, template, version)) + item, err := convert.TemplateVersionToK8s(namespace, template, version) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + items = append(items, *item) } } @@ -375,7 +379,11 @@ func (s *TemplateVersionStorage) get(ctx context.Context, name string, _ *metav1 return nil, err } - return convert.TemplateVersionToK8s(namespace, template, version), nil + obj, err := convert.TemplateVersionToK8s(namespace, template, version) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + return obj, nil } // requirePathSafeSegment rejects a name segment that could change the Coder request path or query. diff --git a/internal/aggregated/storage/workspace.go b/internal/aggregated/storage/workspace.go index 7f25efdc4..39780590b 100644 --- a/internal/aggregated/storage/workspace.go +++ b/internal/aggregated/storage/workspace.go @@ -123,7 +123,11 @@ func (s *WorkspaceStorage) Get(ctx context.Context, name string, _ *metav1.GetOp return nil, err } - return convert.WorkspaceToK8s(namespace, workspace), nil + obj, err := convert.WorkspaceToK8s(namespace, workspace) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + return obj, nil } // resolveWorkspace parses name, looks the workspace up in the Coder @@ -207,7 +211,11 @@ func (s *WorkspaceStorage) List(ctx context.Context, _ *metainternalversion.List } for _, workspace := range workspacesResponse.Workspaces { - list.Items = append(list.Items, *convert.WorkspaceToK8s(eligibleNamespace, workspace)) + item, err := convert.WorkspaceToK8s(eligibleNamespace, workspace) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + list.Items = append(list.Items, *item) } } @@ -255,7 +263,11 @@ func (s *WorkspaceStorage) List(ctx context.Context, _ *metainternalversion.List } for _, workspace := range workspacesResponse.Workspaces { - list.Items = append(list.Items, *convert.WorkspaceToK8s(responseNamespace, workspace)) + item, err := convert.WorkspaceToK8s(responseNamespace, workspace) + if err != nil { + return nil, apierrors.NewInternalError(err) + } + list.Items = append(list.Items, *item) } return list, nil @@ -458,7 +470,11 @@ func (s *WorkspaceStorage) Create( createdWorkspace, err := sdk.CreateUserWorkspace(ctx, userName, request) if err != nil { - return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("coderworkspaces"), workspaceObj.Name) + return nil, coder.MapCoderWriteError(err, coder.WriteCreate, aggregationv1alpha1.Resource("coderworkspaces"), workspaceObj.Name) + } + // Check the answer before the stop build, which needs its workspace ID. + if _, err := convert.WorkspaceToK8s(namespace, createdWorkspace); err != nil { + return nil, uncertainCreateAnswer(aggregationv1alpha1.Resource("coderworkspaces"), workspaceObj.Name) } if !workspaceObj.Spec.Running { @@ -473,9 +489,9 @@ func (s *WorkspaceStorage) Create( // transition can be retried safely via a subsequent Update. } - result := convert.WorkspaceToK8s(namespace, createdWorkspace) - if result == nil { - return nil, fmt.Errorf("assertion failed: converted workspace must not be nil") + result, err := convert.WorkspaceToK8s(namespace, createdWorkspace) + if err != nil { + return nil, fmt.Errorf("assertion failed: the checked workspace must convert: %w", err) } s.enqueueWatchEvent(watch.Added, result.DeepCopy()) @@ -593,7 +609,10 @@ func (s *WorkspaceStorage) Update( return nil, false, err } - currentK8sObj := convert.WorkspaceToK8s(namespace, currentWorkspace) + currentK8sObj, err := convert.WorkspaceToK8s(namespace, currentWorkspace) + if err != nil { + return nil, false, apierrors.NewInternalError(err) + } desiredObjRuntime, err := objInfo.UpdatedObject(ctx, currentK8sObj.DeepCopy()) if err != nil { return nil, false, err @@ -671,14 +690,14 @@ func (s *WorkspaceStorage) Update( build, err := sdk.CreateWorkspaceBuild(ctx, currentWorkspace.ID, req) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("coderworkspaces"), name) + return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("coderworkspaces"), name) } currentWorkspace.LatestBuild = build - result := convert.WorkspaceToK8s(namespace, currentWorkspace) - if result == nil { - return nil, false, fmt.Errorf("assertion failed: converted workspace must not be nil") + result, err := convert.WorkspaceToK8s(namespace, currentWorkspace) + if err != nil { + return nil, false, fmt.Errorf("assertion failed: the converted workspace must convert again: %w", err) } s.enqueueWatchEvent(watch.Modified, result.DeepCopy()) @@ -734,9 +753,9 @@ func (s *WorkspaceStorage) Delete( return nil, false, err } - currentObj := convert.WorkspaceToK8s(namespace, workspace) - if currentObj == nil { - return nil, false, fmt.Errorf("assertion failed: converted workspace must not be nil") + currentObj, err := convert.WorkspaceToK8s(namespace, workspace) + if err != nil { + return nil, false, apierrors.NewInternalError(err) } // Preconditions and admission see the fetched snapshot; the delete build below targets that same workspace ID. @@ -753,16 +772,16 @@ func (s *WorkspaceStorage) Delete( Transition: codersdk.WorkspaceTransitionDelete, }) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("coderworkspaces"), name) + return nil, false, coder.MapCoderWriteError(err, coder.WriteDelete, aggregationv1alpha1.Resource("coderworkspaces"), name) } // Update the workspace snapshot with the delete-transition build so // the watch event reflects the latest build state, not the pre-delete snapshot. workspace.LatestBuild = deleteBuild - workspaceObj := convert.WorkspaceToK8s(namespace, workspace) - if workspaceObj == nil { - return nil, false, fmt.Errorf("assertion failed: converted workspace must not be nil") + workspaceObj, err := convert.WorkspaceToK8s(namespace, workspace) + if err != nil { + return nil, false, fmt.Errorf("assertion failed: the converted workspace must convert again: %w", err) } // Workspace deletion is asynchronous in Coder. Emit a Modified event diff --git a/internal/aggregated/storage/workspace_transition.go b/internal/aggregated/storage/workspace_transition.go index 1d178a389..242b537e1 100644 --- a/internal/aggregated/storage/workspace_transition.go +++ b/internal/aggregated/storage/workspace_transition.go @@ -153,7 +153,7 @@ func (s *WorkspaceTransitionStorage) Create( switch { case isCoderErr && coderErr.StatusCode() == http.StatusConflict: return s.afterConflict(callCtx, sdk, namespace, name, workspace.ID, coderErr) - case !isCoderErr || isUncertainStatus(coderErr.StatusCode()): + case !isCoderErr || coder.IsUncertainStatus(coderErr.StatusCode()): // A timeout, a transport error, or a gateway error leaves it open whether Coder queued // the build: a proxy in front of Coder can fail after Coder accepted it. Never // POST again: one confirming re-read either finds the new build or reports the doubt. @@ -186,15 +186,6 @@ func confirmsPost(ctx context.Context, sdk *codersdk.Client, previousID uuid.UUI return err == nil && me.ID != uuid.Nil && found.InitiatorID == me.ID } -// isUncertainStatus reports whether a Coder or proxy status leaves it open whether the build -// was queued. A 2xx status means the POST got through, but the answer names no build: Coder -// answers 201, so the SDK returns a *codersdk.Error for any other 2xx, and since codersdk v2.37 -// also for a 201 whose body is not the build's JSON (for example a proxy page). -func isUncertainStatus(status int) bool { - return (status >= http.StatusOK && status < http.StatusMultipleChoices) || - status == http.StatusBadGateway || status == http.StatusServiceUnavailable || status == http.StatusGatewayTimeout -} - // afterConflict handles a Coder 409 on the build POST: another build became active after the // lookup. One re-read decides whether that build already does what was requested. func (s *WorkspaceTransitionStorage) afterConflict( @@ -223,9 +214,9 @@ func (s *WorkspaceTransitionStorage) afterConflict( // sends a Modified watch event for the workspace with that build as the latest build. func (s *WorkspaceTransitionStorage) queued(namespace, name string, workspace codersdk.Workspace, build codersdk.WorkspaceBuild) (runtime.Object, error) { workspace.LatestBuild = build - obj := convert.WorkspaceToK8s(namespace, workspace) - if obj == nil { - return nil, fmt.Errorf("assertion failed: converted workspace must not be nil") + obj, err := convert.WorkspaceToK8s(namespace, workspace) + if err != nil { + return nil, apierrors.NewInternalError(err) } s.workspaces.enqueueWatchEvent(watch.Modified, obj) return s.result(namespace, name, transitionOutcomeQueued, false, &build), nil diff --git a/internal/app/apiserverapp/apiserverapp.go b/internal/app/apiserverapp/apiserverapp.go index 69c7b7442..6f4536292 100644 --- a/internal/app/apiserverapp/apiserverapp.go +++ b/internal/app/apiserverapp/apiserverapp.go @@ -82,7 +82,9 @@ type errClientProvider struct { var _ coder.ClientProvider = (*errClientProvider)(nil) -func (p *errClientProvider) ClientForNamespace(ctx context.Context, _ string) (*codersdk.Client, error) { +// ClientForNamespace returns a 503 for every namespace. An unconfigured server serves no namespace, +// so a namespaced LIST returns an empty list, and namespace deletion does not hang on it (#215). +func (p *errClientProvider) ClientForNamespace(ctx context.Context, namespace string) (*codersdk.Client, error) { if p == nil { return nil, fmt.Errorf("assertion failed: err client provider must not be nil") } @@ -93,7 +95,11 @@ func (p *errClientProvider) ClientForNamespace(ctx context.Context, _ string) (* return nil, fmt.Errorf("assertion failed: service unavailable message must not be empty") } - return nil, apierrors.NewServiceUnavailable(p.serviceUnavailableMessage) + unavailable := apierrors.NewServiceUnavailable(p.serviceUnavailableMessage) + if namespace == "" { + return nil, unavailable + } + return nil, coder.NewNamespaceNotServedError(unavailable) } func buildClientProvider(opts Options, requestTimeout time.Duration) (coder.ClientProvider, error) { From c8d89a24bc834e814706cb3c39104c0949fd61cb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:00:23 +0000 Subject: [PATCH 04/27] chore: tidy write-error comments and test cleanup (#242) Change-Id: Ia37d86a95b5f36db95709ce44e9634f8e5c6e33d Signed-off-by: Thomas Kosiewski --- internal/aggregated/coder/errors.go | 16 +++++++--------- .../aggregated/storage/write_outcome_test.go | 2 +- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 35a144289..40e63dfe5 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -122,13 +122,11 @@ func IsUncertainStatus(status int) bool { status == http.StatusBadGateway || status == http.StatusServiceUnavailable || status == http.StatusGatewayTimeout } -// MapCoderWriteError maps the error of the Coder call that creates, updates or deletes name. -// When the error leaves it open whether Coder applied the write, it returns the uncertain 504 of -// UncertainWriteError. Those are the statuses of IsUncertainStatus except 504, and every error -// without a status that the request can have caused after it was sent: a connection that breaks -// before or while the answer is read, or a body that is not the expected JSON. A timeout or a -// Coder 504 keeps the 504 of MapCoderError, which says the same to clients. So does every -// other error, including a request that provably never reached Coder (see notSent). +// MapCoderWriteError maps the error of the Coder call that creates, updates or deletes name. An +// error that leaves it open whether Coder applied the write returns UncertainWriteError: a status +// of IsUncertainStatus other than 504, or an error without a status after the request was sent, +// such as a connection that broke or a body that is not the expected JSON. A timeout, a Coder 504, +// a request that did not reach Coder (see notSent) and every other status map as in MapCoderError. func MapCoderWriteError(err error, write Write, resource schema.GroupResource, name string) error { if err == nil { return fmt.Errorf("assertion failed: error must not be nil") @@ -162,8 +160,8 @@ func MapCoderWriteError(err error, write Write, resource schema.GroupResource, n } // UncertainWriteError is the 504 for a write that Coder can have applied. reason names the answer -// that the write got, for example "status 502 from the Coder API", and must not name the Coder URL. It has no Retry-After, so clients do not send the write -// again on their own. +// that the write got, for example "status 502 from the Coder API", and must not name the Coder +// URL. The error has no Retry-After, so clients do not send the write again on their own. func UncertainWriteError(write Write, resource schema.GroupResource, name, reason string) error { return apierrors.NewTimeoutError(fmt.Sprintf( "the result is uncertain: the %s of %s %q got %s; re-read the object with kubectl get before you retry", diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index 7c696b6c0..c33c0fb11 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -49,7 +49,7 @@ func hijack(t *testing.T, w http.ResponseWriter) net.Conn { func truncatedAnswer(t *testing.T, w http.ResponseWriter, coderAnswer *httptest.ResponseRecorder) { body := coderAnswer.Body.Bytes() conn := hijack(t, w) - defer conn.Close() + defer func() { _ = conn.Close() }() _, _ = fmt.Fprintf(conn, "HTTP/1.1 %d %s\r\nContent-Type: application/json\r\nContent-Length: %d\r\n\r\n", coderAnswer.Code, http.StatusText(coderAnswer.Code), len(body)) _, _ = conn.Write(body[:len(body)/2]) From b805079eb8206a72a2e3e90cd9997a1bf4494cb9 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:00:23 +0000 Subject: [PATCH 05/27] docs: document uncertain write answers and unconfigured standalone lists (#242, #215) Change-Id: Ie3e5f7a6daa33cb757b854ca6268378b14a141a3 Signed-off-by: Thomas Kosiewski --- docs/how-to/troubleshooting.md | 1 + docs/reference/aggregated-api-behavior.md | 21 +++++++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/how-to/troubleshooting.md b/docs/how-to/troubleshooting.md index 691b898a3..497563a8d 100644 --- a/docs/how-to/troubleshooting.md +++ b/docs/how-to/troubleshooting.md @@ -166,6 +166,7 @@ If the error does not name a field, the ServiceAccount does not have a permissio - `500` that mentions `subjectaccessreviews`: the ServiceAccount of the server cannot create SubjectAccessReviews. Apply `config/rbac/auth-delegator-binding.yaml`. In standalone mode, apply `config/apiserver-standalone/auth-delegator-binding.yaml` instead. - `403` that says `the Coder API denied the request`: kube-apiserver let the caller in, but Coder refused the call that the server made with its Coder token. The message leaves out the Coder URL and Coder's own text. The server log has the full Coder error, on a line from the `coder-errors` logger. - `500` that says `the Coder API failed the request ... with status ` or `the Coder API could not be reached`: Coder answered with a server error, or the server could not reach Coder. Read the `coder-errors` line in the server log for the Coder URL and the full error. +- `504` that says `the result is uncertain`: Coder, or a proxy in front of it, gave an answer to a write that leaves it open whether Coder applied the write. Re-read the object with `kubectl get` before you retry. See [Coder timeouts](../reference/aggregated-api-behavior.md#coder-timeouts). ## Aggregated reads return `ServiceUnavailable` diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 9f6d05dec..4e1e26b26 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -17,7 +17,7 @@ The aggregated API server serves `coderworkspaces`, `codertemplates` and `codert | [Template versions](#template-versions) | Read-only: `get` and `list`, no watch. Reads never download template source. | | [Promote a template version](#promote-a-template-version) | `codertemplates//promote` (`create`) makes a version active. A rollback promotes an older version. It needs its own RBAC grant. `dryRun=All` previews it. An unconfirmed result returns `503`, and a concurrent change returns `409`. | | [Template builds](#template-builds) | Create and update with `spec.files` wait until Coder completes the import. The full request must complete within 34 seconds. | -| [Coder timeouts](#coder-timeouts) | A Coder call that runs out of time returns `504` on every resource. A create that gets a broken `2xx` answer returns `504` or `500`. After a `504` on a write, or a `500` on a create, re-read before you retry. | +| [Coder timeouts](#coder-timeouts) | A Coder call that runs out of time returns `504` on every resource. A write whose answer leaves the result open, for example a `502` from a proxy, also returns `504`. After a `504` on a write, re-read before you retry. | | [Workspace build log](#workspace-build-log) | `coderworkspaces/log` returns the latest build log as text. It needs its own RBAC grant and has fixed size, time, and concurrency limits. | | [Start and stop](#start-and-stop) | `coderworkspaces/start` and `coderworkspaces/stop` start or stop a workspace. Each needs its own RBAC grant. A repeated request does not queue a second build while one is active. | @@ -168,15 +168,16 @@ A namespace has no Coder backend in these cases: - `all` mode: the namespace contains no `CoderControlPlane` at all. - Standalone mode: the namespace is not the one that `--coder-namespace` names. +- Standalone mode without `--coder-url` and `--coder-session-token`: every namespace. In such a namespace, the server answers requests as follows: | Request | Result | | --- | --- | | `list` of `coderworkspaces`, `codertemplates`, or `codertemplateversions` in that namespace | `200` with an empty list. The server does not call Coder. | -| `get`, `create`, `update`, `delete`, and the subresources | An error: `503` in `all` mode, `400` in standalone mode | +| `get`, `create`, `update`, `delete`, and the subresources | An error: `503` in `all` mode and in standalone mode without a Coder URL and token, `400` in standalone mode | | `watch` | Works as described in [Watch](#watch). It shows no events, because no write can succeed in that namespace. | -| `list` in all namespaces (`-A`) | Unchanged. In `all` mode, it returns `503` when no `CoderControlPlane` is eligible in any namespace. | +| `list` in all namespaces (`-A`) | Unchanged. In `all` mode, it returns `503` when no `CoderControlPlane` is eligible in any namespace. In standalone mode without a Coder URL and token, it returns `503`. | A namespace that contains a `CoderControlPlane` that is not eligible is not in this group. For example, operator access is not ready, or the name contains a `.` character. For the eligibility rules, see [Aggregated reads return `ServiceUnavailable`](../how-to/troubleshooting.md#aggregated-reads-return-serviceunavailable). In such a namespace, every request returns `503`, `list` included. @@ -225,17 +226,21 @@ A Coder call that runs out of time returns `504 Gateway Timeout` on every aggreg - the request deadline ends first (see [The 34-second write budget](#the-34-second-write-budget), the [log limits](#workspace-build-log), and the [start and stop budget](#time-limit-and-retries)), or - Coder itself answers `504`. -A create of a workspace or a template can also get a broken `2xx` answer, for example from a proxy in front of Coder. Coder can have created the object. The server returns `504` for some of these answers and `500` for others: +A write (create, update, patch, or delete) of a workspace or a template can also get an answer that leaves its result open, for example from a proxy in front of Coder. Coder can have applied the write. The server returns `504`, with a message that says that the result is uncertain, for these answers: -- `504`, with a message that says that the result is uncertain. Examples are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page or with the JSON of Coder. -- `500`. Examples are a `200` with the type `application/json` and invalid JSON or a JSON array, a `201` with `{}` or `null`, and a connection that breaks while the server reads the answer. +- A `502` or a `503`. +- A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. +- A create answer without the name of the object, for example a `201` with `{}` or `null`. +- A connection that breaks after the server sent the request, before or while the server reads the answer. -After a `504` or a `500` on a create, re-read the object before you retry. +A write that did not reach Coder keeps the `500` that says that the server cannot reach Coder. Examples are a refused connection, a failed DNS lookup, and a rejected TLS certificate. Nothing changed in Coder, so you can retry it. + +A `get` or a `list` that gets a Coder object without the fields of its name, for example `{}`, returns `500` with a message that starts with `the Coder API answered`. The message does not include the Coder URL. The same is true for a `403` that Coder answers, a `500` for a Coder server error, and a `500` when the server cannot reach Coder. These messages name only the resource, the object and the Coder status code. The server log keeps the full Coder error, on a line from the `coder-errors` logger. - After a `504` on a read, try again later. -- After a `504` on a write (create, update, patch, or delete), the result is not certain. Coder can have applied the change before the call timed out. Re-read the object with `kubectl get` before you retry. +- After a `504` on a write (create, update, patch, or delete), the result is not certain. Coder can have applied the change before the call timed out or the answer broke. Re-read the object with `kubectl get` before you retry. - A promotion handles this itself. It answers `504` only before it sends the activation, so a `504` on a promotion means that nothing changed. When the activation itself times out or fails without an answer, the server re-reads the template and answers `Promoted`, `409`, or `503`. See [Promote a template version](#promote-a-template-version). - After a `504` on start or stop, re-read the latest build of the workspace before you retry. See [Time limit and retries](#time-limit-and-retries). From 1976cb308dc0dcade03374cd9cc21378f35578e7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:07:49 +0000 Subject: [PATCH 06/27] test: cover nameless build answers and transport errors of template activation (#242) Change-Id: I85e0bc17b3bfee10045dcab28cfdfda54964bc47 Signed-off-by: Thomas Kosiewski --- .../aggregated/storage/write_outcome_test.go | 56 +++++++++++++------ 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index c33c0fb11..611385d13 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -112,8 +112,9 @@ func closedAddress(t *testing.T) string { type writeCase struct { name string isWrite func(*http.Request) bool - // creates is true when Coder's answer to the write is converted to the returned object. - creates bool + // checksAnswer is true when the storage needs the object in Coder's answer: the created object, + // or the build of a workspace update or delete. + checksAnswer bool // readsBody is false when the SDK ignores the body of a successful answer. readsBody bool run func(t *testing.T, provider coder.ClientProvider) error @@ -134,8 +135,8 @@ func writeCases() []writeCase { isWrite: func(r *http.Request) bool { return r.Method == http.MethodPost && r.URL.Path == "/api/v2/users/alice/workspaces" }, - creates: true, - readsBody: true, + checksAnswer: true, + readsBody: true, run: func(_ *testing.T, provider coder.ClientProvider) error { _, err := NewWorkspaceStorage(provider).Create(ctx(), &aggregationv1alpha1.CoderWorkspace{ ObjectMeta: metav1.ObjectMeta{Name: "acme.alice.proxied"}, @@ -145,9 +146,10 @@ func writeCases() []writeCase { }, }, { - name: "workspace update", - isWrite: isBuild, - readsBody: true, + name: "workspace update", + isWrite: isBuild, + checksAnswer: true, + readsBody: true, run: func(t *testing.T, provider coder.ClientProvider) error { workspaces := NewWorkspaceStorage(provider) current, err := workspaces.Get(ctx(), "acme.alice.dev-workspace", nil) @@ -161,19 +163,20 @@ func writeCases() []writeCase { }, }, { - name: "workspace delete", - isWrite: isBuild, - readsBody: true, + name: "workspace delete", + isWrite: isBuild, + checksAnswer: true, + readsBody: true, run: func(_ *testing.T, provider coder.ClientProvider) error { _, _, err := NewWorkspaceStorage(provider).Delete(ctx(), "acme.alice.dev-workspace", nil, nil) return err }, }, { - name: "template create", - isWrite: isPost("/templates"), - creates: true, - readsBody: true, + name: "template create", + isWrite: isPost("/templates"), + checksAnswer: true, + readsBody: true, run: func(_ *testing.T, provider coder.ClientProvider) error { _, err := NewTemplateStorage(provider).Create(ctx(), &aggregationv1alpha1.CoderTemplate{ ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, @@ -203,6 +206,23 @@ func writeCases() []writeCase { return err }, }, + { + name: "template activation of new files", + isWrite: func(r *http.Request) bool { + return r.Method == http.MethodPatch && strings.HasSuffix(r.URL.Path, "/versions") + }, + run: func(t *testing.T, provider coder.ClientProvider) error { + templates := NewTemplateStorage(provider) + current, err := templates.Get(ctx(), "acme.starter-template", nil) + if err != nil { + t.Fatalf("get template: %v", err) + } + desired := current.(*aggregationv1alpha1.CoderTemplate).DeepCopy() + desired.Spec.Files = map[string]string{"main.tf": `resource "null_resource" "changed" {}`} + _, _, err = templates.Update(ctx(), desired.Name, rest.DefaultUpdatedObjectInfo(desired), nil, nil, false, nil) + return err + }, + }, { name: "template delete", isWrite: func(r *http.Request) bool { return r.Method == http.MethodDelete }, @@ -224,20 +244,20 @@ func TestWriteOutcomes(t *testing.T) { name string answer brokenAnswer // applies reports whether the outcome breaks the result of the write. A body that the - // SDK does not read, or an answer that only creates convert, leaves it intact. + // SDK does not read, or an object that the storage does not use, leaves it intact. applies func(writeCase) bool } always := func(writeCase) bool { return true } readsBody := func(w writeCase) bool { return w.readsBody } - creates := func(w writeCase) bool { return w.creates } + checksAnswer := func(w writeCase) bool { return w.checksAnswer } outcomes := []outcome{ {"proxy 502", gatewayPage(http.StatusBadGateway), always}, {"proxy 503", gatewayPage(http.StatusServiceUnavailable), always}, {"connection closed without an answer", closedWithoutAnswer, always}, {"body breaks off", truncatedAnswer, readsBody}, {"200 with malformed JSON", jsonAnswer(http.StatusOK, `{"id":`), readsBody}, - {"201 with an empty object", jsonAnswer(http.StatusCreated, `{}`), creates}, - {"201 with null", jsonAnswer(http.StatusCreated, `null`), creates}, + {"201 with an empty object", jsonAnswer(http.StatusCreated, `{}`), checksAnswer}, + {"201 with null", jsonAnswer(http.StatusCreated, `null`), checksAnswer}, } for _, write := range writeCases() { From 38b248924616f9ca22ae55ecbe32cd6bb027bb21 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:07:50 +0000 Subject: [PATCH 07/27] fix: treat build answers without an ID and failed activation PATCHes as uncertain (#242) Change-Id: Ia6c1baa2a623621d15d164aebcee59e61d110260 Signed-off-by: Thomas Kosiewski --- internal/aggregated/storage/errors.go | 7 +++++++ internal/aggregated/storage/template.go | 6 ++---- internal/aggregated/storage/workspace.go | 6 ++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/internal/aggregated/storage/errors.go b/internal/aggregated/storage/errors.go index 0880b39db..d1f73d2ee 100644 --- a/internal/aggregated/storage/errors.go +++ b/internal/aggregated/storage/errors.go @@ -11,6 +11,7 @@ import ( "k8s.io/apiserver/pkg/storage" storageerrors "k8s.io/apiserver/pkg/storage/errors" + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" "github.com/coder/coder-k8s/internal/aggregated/coder" ) @@ -37,6 +38,12 @@ func uncertainCreateAnswer(resource schema.GroupResource, name string) error { return coder.UncertainWriteError(coder.WriteCreate, resource, name, "an answer from the Coder API without the name of the object") } +// uncertainBuildAnswer is the error for a workspace update or delete whose build request Coder +// answered with its success status, but without a build, for example with {} from a proxy. +func uncertainBuildAnswer(write coder.Write, name string) error { + return coder.UncertainWriteError(write, aggregationv1alpha1.Resource("coderworkspaces"), name, "an answer from the Coder API without the ID of the build") +} + func wrapClientError(err error) error { if err == nil { return nil diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 102235ce0..d4bd3fa07 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -771,13 +771,11 @@ func (s *TemplateStorage) Update( return nil, false, err } - if err := sdk.UpdateActiveTemplateVersion(ctx, templateID, codersdk.UpdateActiveTemplateVersion{ID: newVersion.ID}); err != nil { + if err := patchActiveTemplateVersion(ctx, sdk, templateID, newVersion.ID); err != nil { return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, 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. + // Post-condition: verify that the active version changed. verifyTemplate, err := sdk.Template(ctx, templateID) if err != nil { return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) diff --git a/internal/aggregated/storage/workspace.go b/internal/aggregated/storage/workspace.go index 39780590b..120857a70 100644 --- a/internal/aggregated/storage/workspace.go +++ b/internal/aggregated/storage/workspace.go @@ -692,6 +692,9 @@ func (s *WorkspaceStorage) Update( if err != nil { return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("coderworkspaces"), name) } + if build.ID == uuid.Nil { + return nil, false, uncertainBuildAnswer(coder.WriteUpdate, name) + } currentWorkspace.LatestBuild = build @@ -774,6 +777,9 @@ func (s *WorkspaceStorage) Delete( if err != nil { return nil, false, coder.MapCoderWriteError(err, coder.WriteDelete, aggregationv1alpha1.Resource("coderworkspaces"), name) } + if deleteBuild.ID == uuid.Nil { + return nil, false, uncertainBuildAnswer(coder.WriteDelete, name) + } // Update the workspace snapshot with the delete-transition build so // the watch event reflects the latest build state, not the pre-delete snapshot. From 129100d5f7e5e08d508db65d67a8697aa5c61c66 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:09:19 +0000 Subject: [PATCH 08/27] docs: list build answers without an ID as uncertain (#242) Change-Id: Ia39194d4f638f04bc41b3aa96a2c561a2a717d7d Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 4e1e26b26..51997bfd9 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -230,7 +230,7 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A `502` or a `503`. - A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. -- A create answer without the name of the object, for example a `201` with `{}` or `null`. +- A create answer without the name of the object, or a build answer of a workspace update or delete without the ID of the build. An example is a `201` with `{}` or `null`. - A connection that breaks after the server sent the request, before or while the server reads the answer. A write that did not reach Coder keeps the `500` that says that the server cannot reach Coder. Examples are a refused connection, a failed DNS lookup, and a rejected TLS certificate. Nothing changed in Coder, so you can retry it. From 1f2817fc79c425e066f0ad28d0ee97b9a288d8d8 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:14:36 +0000 Subject: [PATCH 09/27] test: cover Coder answers without an ID (#242) Change-Id: I6de82486d0068fd4926491e4c034628c8961d47f Signed-off-by: Thomas Kosiewski --- internal/aggregated/convert/answer_test.go | 16 +++++++++++-- .../aggregated/storage/write_outcome_test.go | 23 ++++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/internal/aggregated/convert/answer_test.go b/internal/aggregated/convert/answer_test.go index 52527c045..e3c209d6a 100644 --- a/internal/aggregated/convert/answer_test.go +++ b/internal/aggregated/convert/answer_test.go @@ -37,8 +37,8 @@ func mustTemplateVersionToK8s(t *testing.T, namespace string, template codersdk. return obj } -// TestConvertersRejectAnswersWithoutNames checks that a Coder answer without the fields of the -// Kubernetes name, for example the {} of a proxy (#242), is an error and not a panic. +// TestConvertersRejectAnswersWithoutNames checks that a Coder answer without its ID or the fields +// of the Kubernetes name, for example the {} of a proxy (#242), is an error and not a panic. func TestConvertersRejectAnswersWithoutNames(t *testing.T) { t.Parallel() @@ -53,6 +53,18 @@ func TestConvertersRejectAnswersWithoutNames(t *testing.T) { _, err := TemplateToK8s("ns", codersdk.Template{OrganizationName: "acme", Name: "a.b"}) return err }, `template must not contain "."`}, + {"template without an ID", func() error { + _, err := TemplateToK8s("ns", codersdk.Template{OrganizationName: "acme", Name: "docker"}) + return err + }, "without an ID"}, + {"workspace without an ID", func() error { + _, err := WorkspaceToK8s("ns", codersdk.Workspace{OrganizationName: "acme", OwnerName: "alice", Name: "dev"}) + return err + }, "without an ID"}, + {"template version without an ID", func() error { + _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID, Name: "v1"}) + return err + }, "without an ID"}, {"empty workspace", func() error { _, err := WorkspaceToK8s("ns", codersdk.Workspace{}); return err }, "organization must not be empty"}, {"workspace without an owner", func() error { _, err := WorkspaceToK8s("ns", codersdk.Workspace{OrganizationName: "acme", Name: "dev"}) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index 611385d13..d0dd778a1 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -2,6 +2,7 @@ package storage import ( "context" + "encoding/json" "fmt" "net" "net/http" @@ -36,6 +37,20 @@ func jsonAnswer(status int, body string) brokenAnswer { } } +// answerWithoutID sends Coder's answer without its "id" field. +func answerWithoutID(t *testing.T, w http.ResponseWriter, coderAnswer *httptest.ResponseRecorder) { + var object map[string]any + if err := json.Unmarshal(coderAnswer.Body.Bytes(), &object); err != nil { + t.Fatalf("decode Coder answer: %v", err) + } + delete(object, "id") + body, err := json.Marshal(object) + if err != nil { + t.Fatal(err) + } + jsonAnswer(coderAnswer.Code, string(body))(t, w, coderAnswer) +} + func hijack(t *testing.T, w http.ResponseWriter) net.Conn { t.Helper() conn, _, err := w.(http.Hijacker).Hijack() @@ -112,8 +127,8 @@ func closedAddress(t *testing.T) string { type writeCase struct { name string isWrite func(*http.Request) bool - // checksAnswer is true when the storage needs the object in Coder's answer: the created object, - // or the build of a workspace update or delete. + // checksAnswer is true when the storage checks the object in Coder's answer: the created + // object, the build of a workspace update or delete, or the template of a metadata update. checksAnswer bool // readsBody is false when the SDK ignores the body of a successful answer. readsBody bool @@ -193,7 +208,8 @@ func writeCases() []writeCase { isWrite: func(r *http.Request) bool { return r.Method == http.MethodPatch && !strings.HasSuffix(r.URL.Path, "/versions") }, - readsBody: true, + checksAnswer: true, + readsBody: true, run: func(t *testing.T, provider coder.ClientProvider) error { templates := NewTemplateStorage(provider) current, err := templates.Get(ctx(), "acme.starter-template", nil) @@ -258,6 +274,7 @@ func TestWriteOutcomes(t *testing.T) { {"200 with malformed JSON", jsonAnswer(http.StatusOK, `{"id":`), readsBody}, {"201 with an empty object", jsonAnswer(http.StatusCreated, `{}`), checksAnswer}, {"201 with null", jsonAnswer(http.StatusCreated, `null`), checksAnswer}, + {"answer without the ID", answerWithoutID, checksAnswer}, } for _, write := range writeCases() { From c3331e469a2ea65cf5b9ce80be3c71e1c84b63d0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:15:58 +0000 Subject: [PATCH 10/27] fix: reject Coder answers without an ID and unconfirmed template metadata updates (#242) Change-Id: Ica84f8d8d1278380b31468136cd4cd69732fec05 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 4 ++-- internal/aggregated/convert/answer_test.go | 12 ++++++------ internal/aggregated/convert/template.go | 13 ++++++++----- internal/aggregated/convert/templateversion.go | 4 ++-- internal/aggregated/convert/workspace.go | 4 ++-- internal/aggregated/storage/errors.go | 4 ++-- internal/aggregated/storage/template.go | 6 +++++- 7 files changed, 27 insertions(+), 20 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 51997bfd9..c09ad666c 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -230,12 +230,12 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A `502` or a `503`. - A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. -- A create answer without the name of the object, or a build answer of a workspace update or delete without the ID of the build. An example is a `201` with `{}` or `null`. +- An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. - A connection that breaks after the server sent the request, before or while the server reads the answer. A write that did not reach Coder keeps the `500` that says that the server cannot reach Coder. Examples are a refused connection, a failed DNS lookup, and a rejected TLS certificate. Nothing changed in Coder, so you can retry it. -A `get` or a `list` that gets a Coder object without the fields of its name, for example `{}`, returns `500` with a message that starts with `the Coder API answered`. +A `get` or a `list` that gets a Coder object without its ID or the fields of its name, for example `{}`, returns `500` with a message that starts with `the Coder API answered`. The message does not include the Coder URL. The same is true for a `403` that Coder answers, a `500` for a Coder server error, and a `500` when the server cannot reach Coder. These messages name only the resource, the object and the Coder status code. The server log keeps the full Coder error, on a line from the `coder-errors` logger. diff --git a/internal/aggregated/convert/answer_test.go b/internal/aggregated/convert/answer_test.go index e3c209d6a..9a1087b98 100644 --- a/internal/aggregated/convert/answer_test.go +++ b/internal/aggregated/convert/answer_test.go @@ -48,9 +48,9 @@ func TestConvertersRejectAnswersWithoutNames(t *testing.T) { convert func() error want string }{ - {"empty template", func() error { _, err := TemplateToK8s("ns", codersdk.Template{}); return err }, "organization must not be empty"}, + {"empty template", func() error { _, err := TemplateToK8s("ns", codersdk.Template{}); return err }, "without an ID"}, {"template name with a dot", func() error { - _, err := TemplateToK8s("ns", codersdk.Template{OrganizationName: "acme", Name: "a.b"}) + _, err := TemplateToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "a.b"}) return err }, `template must not contain "."`}, {"template without an ID", func() error { @@ -65,17 +65,17 @@ func TestConvertersRejectAnswersWithoutNames(t *testing.T) { _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID, Name: "v1"}) return err }, "without an ID"}, - {"empty workspace", func() error { _, err := WorkspaceToK8s("ns", codersdk.Workspace{}); return err }, "organization must not be empty"}, + {"empty workspace", func() error { _, err := WorkspaceToK8s("ns", codersdk.Workspace{}); return err }, "without an ID"}, {"workspace without an owner", func() error { - _, err := WorkspaceToK8s("ns", codersdk.Workspace{OrganizationName: "acme", Name: "dev"}) + _, err := WorkspaceToK8s("ns", codersdk.Workspace{ID: uuid.New(), OrganizationName: "acme", Name: "dev"}) return err }, "user must not be empty"}, {"template version without a name", func() error { - _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID}) + _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, OrganizationName: "acme", Name: "docker"}, codersdk.TemplateVersion{ID: uuid.New(), TemplateID: &templateID}) return err }, "version must not be empty"}, {"template version of a template without an organization", func() error { - _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, Name: "docker"}, codersdk.TemplateVersion{TemplateID: &templateID, Name: "v1"}) + _, err := TemplateVersionToK8s("ns", codersdk.Template{ID: templateID, Name: "docker"}, codersdk.TemplateVersion{ID: uuid.New(), TemplateID: &templateID, Name: "v1"}) return err }, "organization must not be empty"}, } { diff --git a/internal/aggregated/convert/template.go b/internal/aggregated/convert/template.go index e7fc01cc4..bee38f8ef 100644 --- a/internal/aggregated/convert/template.go +++ b/internal/aggregated/convert/template.go @@ -16,9 +16,12 @@ import ( // nameField is a field of a Coder answer that becomes a segment of the Kubernetes name. type nameField struct{ segment, value string } -// requireNameFields returns an error when a field cannot be a segment of the name, for example in -// the {} that a proxy answers instead of Coder. -func requireNameFields(object string, fields ...nameField) error { +// requireIdentity returns an error when id is zero or a field cannot be a segment of the name, +// for example in the {} that a proxy answers instead of Coder. +func requireIdentity(object string, id uuid.UUID, fields ...nameField) error { + if id == uuid.Nil { + return fmt.Errorf("the Coder API answered a %s without an ID", object) + } for _, field := range fields { if err := coder.NameSegmentError(field.segment, field.value); err != nil { return fmt.Errorf("the Coder API answered a %s without a valid name: %w", object, err) @@ -28,12 +31,12 @@ func requireNameFields(object string, fields ...nameField) error { } // TemplateToK8s converts a codersdk.Template to an aggregated API CoderTemplate. It returns an -// error when t lacks the fields of the name. +// error when t lacks its ID or the fields of the name. func TemplateToK8s(namespace string, t codersdk.Template) (*aggregationv1alpha1.CoderTemplate, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") } - if err := requireNameFields("template", nameField{"organization", t.OrganizationName}, nameField{"template", t.Name}); err != nil { + if err := requireIdentity("template", t.ID, nameField{"organization", t.OrganizationName}, nameField{"template", t.Name}); err != nil { return nil, err } diff --git a/internal/aggregated/convert/templateversion.go b/internal/aggregated/convert/templateversion.go index 9074f9812..3d8c81439 100644 --- a/internal/aggregated/convert/templateversion.go +++ b/internal/aggregated/convert/templateversion.go @@ -30,7 +30,7 @@ const ( // any returned field changes, including the active flag, which Coder does not reflect in the // version's own updated_at. Compare it only for equality. // -// It returns an error when tpl or v lack the fields of the name and labels. +// It returns an error when v lacks its ID, or tpl or v lack the fields of the name and labels. func TemplateVersionToK8s(namespace string, tpl codersdk.Template, v codersdk.TemplateVersion) (*aggregationv1alpha1.CoderTemplateVersion, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") @@ -38,7 +38,7 @@ func TemplateVersionToK8s(namespace string, tpl codersdk.Template, v codersdk.Te if v.TemplateID == nil || *v.TemplateID != tpl.ID { panic(fmt.Sprintf("assertion failed: template version %s must belong to template %s", v.ID, tpl.ID)) } - if err := requireNameFields("template version", + if err := requireIdentity("template version", v.ID, nameField{"organization", tpl.OrganizationName}, nameField{"template", tpl.Name}); err != nil { return nil, err } diff --git a/internal/aggregated/convert/workspace.go b/internal/aggregated/convert/workspace.go index 77da0677b..25e941ec3 100644 --- a/internal/aggregated/convert/workspace.go +++ b/internal/aggregated/convert/workspace.go @@ -23,12 +23,12 @@ import ( // backend-only change that leaves the projection identical keeps the token. It is not monotonic // and not a history cursor: an identical projection yields the same token again. // -// It returns an error when w lacks the fields of the name. +// It returns an error when w lacks its ID or the fields of the name. func WorkspaceToK8s(namespace string, w codersdk.Workspace) (*aggregationv1alpha1.CoderWorkspace, error) { if namespace == "" { panic("assertion failed: namespace must not be empty") } - if err := requireNameFields("workspace", + if err := requireIdentity("workspace", w.ID, nameField{"organization", w.OrganizationName}, nameField{"user", w.OwnerName}, nameField{"workspace", w.Name}); err != nil { return nil, err } diff --git a/internal/aggregated/storage/errors.go b/internal/aggregated/storage/errors.go index d1f73d2ee..e78ea6bd4 100644 --- a/internal/aggregated/storage/errors.go +++ b/internal/aggregated/storage/errors.go @@ -32,10 +32,10 @@ func rejectDryRun(resource string, dryRun []string) error { } // uncertainCreateAnswer is the error for a create that Coder answered with its success status, -// but without the fields of the object, for example with {} from a proxy. Coder can have created +// but without the ID or name of the object, for example with {} from a proxy. Coder can have created // the object. func uncertainCreateAnswer(resource schema.GroupResource, name string) error { - return coder.UncertainWriteError(coder.WriteCreate, resource, name, "an answer from the Coder API without the name of the object") + return coder.UncertainWriteError(coder.WriteCreate, resource, name, "an answer from the Coder API without the ID or name of the object") } // uncertainBuildAnswer is the error for a workspace update or delete whose build request Coder diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index d4bd3fa07..42e16a65c 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -794,10 +794,14 @@ func (s *TemplateStorage) Update( // the template unchanged (#117). The Coder calls are not atomic: if this call fails after a promotion, // the new source stays active with the old metadata. if metadataChanged { - _, err := sdk.UpdateTemplateMeta(ctx, templateID, convert.TemplateUpdateMetaRequestFromK8s(updatedTemplate)) + updatedMeta, err := sdk.UpdateTemplateMeta(ctx, templateID, convert.TemplateUpdateMetaRequestFromK8s(updatedTemplate)) if err != nil { return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name) } + if updatedMeta.ID != templateID { + return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, + "an answer from the Coder API without the updated template") + } } refreshedObj, err := s.Get(ctx, name, nil) From 46fa605e79d88ff24b1205e6eec2807705ef34ca Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:32:35 +0000 Subject: [PATCH 11/27] docs: say that the read error message contains the Coder answer text (#242) Change-Id: I0031e0f8010e79857ecedb00ed336c5083b313ec Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index c09ad666c..992106d62 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -235,7 +235,7 @@ A write (create, update, patch, or delete) of a workspace or a template can also A write that did not reach Coder keeps the `500` that says that the server cannot reach Coder. Examples are a refused connection, a failed DNS lookup, and a rejected TLS certificate. Nothing changed in Coder, so you can retry it. -A `get` or a `list` that gets a Coder object without its ID or the fields of its name, for example `{}`, returns `500` with a message that starts with `the Coder API answered`. +A `get` or a `list` that gets a Coder object without its ID or the fields of its name, for example `{}`, returns `500` with a message that contains `the Coder API answered`. The message does not include the Coder URL. The same is true for a `403` that Coder answers, a `500` for a Coder server error, and a `500` when the server cannot reach Coder. These messages name only the resource, the object and the Coder status code. The server log keeps the full Coder error, on a line from the `coder-errors` logger. From 9fb40da4e93bc4cee19b0981f98bffeb03ac25cc Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:38:09 +0000 Subject: [PATCH 12/27] test: expect a refused cross-origin redirect of a write to be uncertain (#242) Change-Id: I95cee7af345dee1e705b0c25450e311c19c11572 Signed-off-by: Thomas Kosiewski --- internal/aggregated/coder/errors_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/aggregated/coder/errors_test.go b/internal/aggregated/coder/errors_test.go index b48363dd4..f4f2f0e55 100644 --- a/internal/aggregated/coder/errors_test.go +++ b/internal/aggregated/coder/errors_test.go @@ -286,7 +286,8 @@ func TestMapCoderWriteError(t *testing.T) { {"DNS failure", dial(&net.DNSError{Err: "no such host", Name: "coder.example.com", IsNotFound: true}), unreachable}, {"proxy unreachable", afterSend(&net.OpError{Op: "proxyconnect", Net: "tcp", Err: syscall.ECONNREFUSED}), unreachable}, {"certificate rejected", afterSend(&tls.CertificateVerificationError{Err: errors.New("unknown authority")}), unreachable}, - {"redirect to another origin", afterSend(sameorigin.ErrCrossOrigin), unreachable}, + // The redirect answer comes after the server got the write. + {"redirect to another origin", afterSend(sameorigin.ErrCrossOrigin), uncertain}, {"request to another origin", afterSend(sameorigin.ErrCrossOriginRequest), unreachable}, {"timeout", afterSend(context.DeadlineExceeded), timedOut}, {"Coder 504", sdkErr(http.StatusGatewayTimeout), timedOut}, From e669c365d7fa6af5c944c2f6e4da6e3e73c4d28c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 12:38:43 +0000 Subject: [PATCH 13/27] fix: treat a refused cross-origin redirect of a write as uncertain (#242) Change-Id: I41cf5ed81c3905c7ff2eb7849591bffde96deb27 Signed-off-by: Thomas Kosiewski --- docs/how-to/troubleshooting.md | 2 +- internal/aggregated/coder/errors.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/how-to/troubleshooting.md b/docs/how-to/troubleshooting.md index 497563a8d..6b7cd1429 100644 --- a/docs/how-to/troubleshooting.md +++ b/docs/how-to/troubleshooting.md @@ -59,7 +59,7 @@ kubectl get deploy,svc -n Coder answered an operator call with a redirect to another origin. The operator does not follow such redirects, so that the operator token stays on the control plane origin. The usual cause is `CODER_REDIRECT_TO_ACCESS_URL=true` in `spec.extraEnv` of the `CoderControlPlane`. Remove it, and redirect browsers from HTTP to HTTPS in your Ingress, Gateway or load balancer instead. See [Redirects to another origin](deploy-controller.md#redirects-to-another-origin). -Most aggregated API calls then answer the caller with a `500` that says `the Coder API could not be reached`. The `coder-errors` line in the aggregated API server log shows the refusal. When Coder or a proxy redirects only the websocket request of the workspace build log stream (`coderworkspaces/log?follow=true`), that line shows `refusing request to a different origin than the coder URL` instead. +Most aggregated API reads then answer the caller with a `500` that says `the Coder API could not be reached`. Writes answer with the uncertain `504`, because the server that sent the redirect got the write. Re-read the object before you retry. The `coder-errors` line in the aggregated API server log shows the refusal. When Coder or a proxy redirects only the websocket request of the workspace build log stream (`coderworkspaces/log?follow=true`), that line shows `refusing request to a different origin than the coder URL` instead. ## APIService is `False` / `Unavailable` diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 40e63dfe5..69cc03de3 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -171,7 +171,8 @@ func UncertainWriteError(write Write, resource schema.GroupResource, name, reaso // notSent reports whether err proves that the request did not reach Coder: the connection to // Coder (or to the proxy in front of it) failed, the TLS handshake failed, or the client refused -// a request or a redirect to another origin. A redirect answer does not apply a write. +// a request to another origin. A refused redirect is not in this list: the server that answered +// with the redirect got the write. func notSent(err error) bool { var opErr *net.OpError if errors.As(err, &opErr) && (opErr.Op == "dial" || opErr.Op == "proxyconnect") { @@ -179,8 +180,7 @@ func notSent(err error) bool { } var certErr *tls.CertificateVerificationError var recordErr tls.RecordHeaderError - return errors.As(err, &certErr) || errors.As(err, &recordErr) || - errors.Is(err, sameorigin.ErrCrossOrigin) || errors.Is(err, sameorigin.ErrCrossOriginRequest) + return errors.As(err, &certErr) || errors.As(err, &recordErr) || errors.Is(err, sameorigin.ErrCrossOriginRequest) } // coderErrorMessage returns Coder's message for a 4xx answer. Without one, it returns a fixed From 1d21ac3844fa845a34731d4e8904b8c9d8bd039c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 13:19:22 +0000 Subject: [PATCH 14/27] test: expect writes refused before they reach Coder to keep the 500 (#242) Change-Id: Ie3486fc92839a330d2ced551bce814f060cde49f Signed-off-by: Thomas Kosiewski --- .../aggregated/coder/errors_transport_test.go | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 internal/aggregated/coder/errors_transport_test.go diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go new file mode 100644 index 000000000..73505f879 --- /dev/null +++ b/internal/aggregated/coder/errors_transport_test.go @@ -0,0 +1,153 @@ +package coder + +import ( + "context" + "crypto/tls" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/coder/coder/v2/codersdk" + apierrors "k8s.io/apimachinery/pkg/api/errors" + + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" + "github.com/coder/coder-k8s/internal/sameorigin" +) + +// TestMapCoderWriteErrorRealTransport sends a write through the transport of NewSDKClient and +// checks that a write the Coder handler never got returns the 500, and a write it got returns +// the uncertain 504. +func TestMapCoderWriteErrorRealTransport(t *testing.T) { + t.Parallel() + + var received atomic.Int32 + created := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + received.Add(1) + w.WriteHeader(http.StatusCreated) + }) + certServer := httptest.NewTLSServer(created) + t.Cleanup(certServer.Close) + plainServer := httptest.NewServer(created) + t.Cleanup(plainServer.Close) + tls13Server := httptest.NewUnstartedServer(created) + tls13Server.TLS = &tls.Config{MinVersion: tls.VersionTLS13} + tls13Server.StartTLS() + t.Cleanup(tls13Server.Close) + cipherServer := httptest.NewUnstartedServer(created) + cipherServer.TLS = &tls.Config{ + MaxVersion: tls.VersionTLS12, + CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}, + } + cipherServer.StartTLS() + t.Cleanup(cipherServer.Close) + brokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + received.Add(1) + conn, _, err := http.NewResponseController(w).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + _ = conn.Close() + })) + t.Cleanup(brokenServer.Close) + refusingProxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodConnect { + t.Errorf("expected CONNECT, got %s", r.Method) + } + w.WriteHeader(http.StatusProxyAuthRequired) + })) + t.Cleanup(refusingProxy.Close) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + closedAddr := listener.Addr().String() + if err := listener.Close(); err != nil { + t.Fatalf("close listener: %v", err) + } + + mustParse := func(raw string) *url.URL { + u, err := url.Parse(raw) + if err != nil { + t.Fatalf("parse %q: %v", raw, err) + } + return u + } + tls12With := func(suites ...uint16) func(*http.Transport) { + return func(tr *http.Transport) { + tr.TLSClientConfig = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: suites} + } + } + proxy := func(raw string) func(*http.Transport) { + return func(tr *http.Transport) { tr.Proxy = http.ProxyURL(mustParse(raw)) } + } + + for _, tc := range []struct { + name string + // coderURL is the URL of the Coder client. sameOrigin, when set, is the origin that the + // client allows, to make it refuse a request to coderURL. + coderURL string + sameOrigin string + configure func(*http.Transport) + wantSent bool + }{ + {name: "connection refused", coderURL: "http://" + closedAddr}, + {name: "certificate rejected", coderURL: certServer.URL}, + {name: "proxy connection refused", coderURL: "https://coder.invalid", configure: proxy("http://" + closedAddr)}, + {name: "request to another origin", coderURL: plainServer.URL, sameOrigin: "http://allowed.invalid"}, + {name: "proxy refuses CONNECT with 407", coderURL: "https://coder.invalid", configure: proxy(refusingProxy.URL)}, + {name: "https URL answered by an HTTP server", coderURL: strings.Replace(plainServer.URL, "http://", "https://", 1)}, + {name: "server refuses the TLS version", coderURL: tls13Server.URL, configure: tls12With()}, + {name: "server refuses the cipher suites", coderURL: cipherServer.URL, configure: tls12With(tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305)}, + {name: "connection closed after the request", coderURL: brokenServer.URL, wantSent: true}, + } { + t.Run(tc.name, func(t *testing.T) { + coderURL := mustParse(tc.coderURL) + allowed := coderURL + if tc.sameOrigin != "" { + allowed = mustParse(tc.sameOrigin) + } + transport, err := newCoderTransport() + if err != nil { + t.Fatalf("new transport: %v", err) + } + if tc.configure != nil { + tc.configure(transport) + } + t.Cleanup(transport.CloseIdleConnections) + client := codersdk.New(coderURL, codersdk.WithHTTPClient(&http.Client{ + Transport: sameorigin.RoundTripper(allowed, transport), + Timeout: 5 * time.Second, + })) + + before := received.Load() + resp, err := client.Request(context.Background(), http.MethodPost, "/api/v2/workspaces", map[string]string{"name": "dev"}) + if resp != nil { + _ = resp.Body.Close() + } + if err == nil { + t.Fatal("expected a transport error") + } + if sent := received.Load() != before; sent != tc.wantSent { + t.Fatalf("expected the Coder handler to get the request: %t, got %t (error %v)", tc.wantSent, sent, err) + } + + mapped := MapCoderWriteError(err, WriteCreate, aggregationv1alpha1.Resource("coderworkspaces"), "acme.alice.dev") + if tc.wantSent { + if !apierrors.IsTimeout(mapped) || !strings.Contains(mapped.Error(), "uncertain") { + t.Fatalf("expected the uncertain 504 for %v, got %v", err, mapped) + } + return + } + if !apierrors.IsInternalError(mapped) || !strings.Contains(mapped.Error(), "could not be reached") { + t.Fatalf("expected the 500 that says Coder could not be reached for %v, got %v", err, mapped) + } + }) + } +} From 0c9d18563819c3d04b2b06e31bc628bc967a905c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 13:27:15 +0000 Subject: [PATCH 15/27] fix: keep the 500 for writes refused before they reach Coder (#242) A proxy that refuses the CONNECT tunnel, an https URL answered by plain HTTP, and a server that refuses the TLS version or cipher suites never reach Coder. Classify them as not sent, and say in the docs that other transport errors without a status stay uncertain. Change-Id: I2a4c4852408ca65067b3cd6bfb843f5ed1b3a82a Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 11 +++++++- internal/aggregated/coder/config.go | 15 ++++++++++ internal/aggregated/coder/errors.go | 28 +++++++++++++++---- .../aggregated/coder/errors_transport_test.go | 4 +-- 4 files changed, 49 insertions(+), 9 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 992106d62..3041b7440 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -233,7 +233,16 @@ A write (create, update, patch, or delete) of a workspace or a template can also - An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. - A connection that breaks after the server sent the request, before or while the server reads the answer. -A write that did not reach Coder keeps the `500` that says that the server cannot reach Coder. Examples are a refused connection, a failed DNS lookup, and a rejected TLS certificate. Nothing changed in Coder, so you can retry it. +A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. The server knows this for these errors: + +- A refused connection to Coder or to the proxy, or a failed DNS lookup. +- A proxy that refuses the `CONNECT` tunnel to Coder, for example with `407`. +- A rejected TLS certificate. +- A TLS handshake that the server refuses because it does not support the TLS version or the cipher suites of the client. +- An `https` Coder URL that a plain HTTP server answers. +- A request to an origin other than the Coder URL, which the server refuses before it sends it. + +The server cannot tell other transport errors without a status apart from a connection that broke after the send. It treats them as uncertain and returns the `504`. A `get` or a `list` that gets a Coder object without its ID or the fields of its name, for example `{}`, returns `500` with a message that contains `the Coder API answered`. diff --git a/internal/aggregated/coder/config.go b/internal/aggregated/coder/config.go index 5df28b1e8..e47460bba 100644 --- a/internal/aggregated/coder/config.go +++ b/internal/aggregated/coder/config.go @@ -2,6 +2,8 @@ package coder import ( + "context" + "errors" "fmt" "net/http" "net/url" @@ -87,10 +89,23 @@ func newCoderTransport() (*http.Transport, error) { transport.MaxConnsPerHost = maxConnsPerCoderHost transport.MaxIdleConnsPerHost = maxConnsPerCoderHost transport.MaxIdleConns = maxConnsPerCoderHost + transport.OnProxyConnectResponse = refuseFailedProxyConnect return transport, nil } +// errProxyConnectRefused marks a proxy answer other than 200 to the CONNECT for Coder. Go reports +// that answer with an untyped error, and without this mark notSent cannot tell it apart from a +// connection that broke after the request. +var errProxyConnectRefused = errors.New("the proxy refused the CONNECT tunnel") + +func refuseFailedProxyConnect(_ context.Context, _ *url.URL, _ *http.Request, resp *http.Response) error { + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("%w: %s", errProxyConnectRefused, resp.Status) + } + return nil +} + // Config describes how to construct a Coder SDK client. type Config struct { CoderURL *url.URL diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 69cc03de3..0cddd2c38 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -169,18 +169,34 @@ func UncertainWriteError(write Write, resource schema.GroupResource, name, reaso ), 0) } +// handshakeRefusals are the TLS alerts (RFC 8446, section 6.2) with which a server refuses the +// handshake before it reads a request: handshake_failure (40) and protocol_version (70). +var handshakeRefusals = []tls.AlertError{40, 70} + // notSent reports whether err proves that the request did not reach Coder: the connection to -// Coder (or to the proxy in front of it) failed, the TLS handshake failed, or the client refused -// a request to another origin. A refused redirect is not in this list: the server that answered -// with the redirect got the write. +// Coder (or to the proxy in front of it) failed, the proxy refused the CONNECT tunnel, the TLS +// handshake failed, or the client refused a request to another origin. A refused redirect is not +// in this list: the server that answered with the redirect got the write. func notSent(err error) bool { var opErr *net.OpError - if errors.As(err, &opErr) && (opErr.Op == "dial" || opErr.Op == "proxyconnect") { - return true + if errors.As(err, &opErr) { + switch opErr.Op { + case "dial", "proxyconnect": + return true + case "remote error": + // crypto/tls keeps a received alert in an unexported type with the text of AlertError. + for _, alert := range handshakeRefusals { + if opErr.Err != nil && opErr.Err.Error() == alert.Error() { + return true + } + } + } } var certErr *tls.CertificateVerificationError var recordErr tls.RecordHeaderError - return errors.As(err, &certErr) || errors.As(err, &recordErr) || errors.Is(err, sameorigin.ErrCrossOriginRequest) + return errors.As(err, &certErr) || errors.As(err, &recordErr) || + errors.Is(err, http.ErrSchemeMismatch) || errors.Is(err, errProxyConnectRefused) || + errors.Is(err, sameorigin.ErrCrossOriginRequest) } // coderErrorMessage returns Coder's message for a 4xx answer. Without one, it returns a fixed diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go index 73505f879..afe5eb6cd 100644 --- a/internal/aggregated/coder/errors_transport_test.go +++ b/internal/aggregated/coder/errors_transport_test.go @@ -39,7 +39,7 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { tls13Server.StartTLS() t.Cleanup(tls13Server.Close) cipherServer := httptest.NewUnstartedServer(created) - cipherServer.TLS = &tls.Config{ + cipherServer.TLS = &tls.Config{ //nolint:gosec // G402: only TLS 1.2 lets a server refuse the cipher suites. MaxVersion: tls.VersionTLS12, CipherSuites: []uint16{tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256}, } @@ -81,7 +81,7 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { } tls12With := func(suites ...uint16) func(*http.Transport) { return func(tr *http.Transport) { - tr.TLSClientConfig = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: suites} + tr.TLSClientConfig = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: suites} //nolint:gosec // G402: the test needs a TLS 1.2 client that the servers refuse. } } proxy := func(raw string) func(*http.Transport) { From 276857303617a9a3559c280ca609d47ac38682d0 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 13:36:14 +0000 Subject: [PATCH 16/27] refactor: build the Coder HTTP client in one helper Change-Id: I3c53c9d3796206a883f11891ab075263ea14afb3 Signed-off-by: Thomas Kosiewski --- internal/aggregated/coder/config.go | 25 +++++++++++-------- .../aggregated/coder/errors_transport_test.go | 8 ++---- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/internal/aggregated/coder/config.go b/internal/aggregated/coder/config.go index e47460bba..17a4cf3dd 100644 --- a/internal/aggregated/coder/config.go +++ b/internal/aggregated/coder/config.go @@ -106,6 +106,20 @@ func refuseFailedProxyConnect(_ context.Context, _ *url.URL, _ *http.Request, re return nil } +// newCoderHTTPClient returns the HTTP client of NewSDKClient for the Coder origin coderURL. +func newCoderHTTPClient(coderURL *url.URL, transport http.RoundTripper, timeout time.Duration) *http.Client { + return &http.Client{ + // codersdk dials websockets (WorkspaceBuildLogsAfter for ?follow=true) + // with a new http.Client that keeps only this Transport, not + // CheckRedirect, so the Transport also refuses other origins. + Transport: sameorigin.RoundTripper(coderURL, transport), + Timeout: timeout, + // net/http copies Coder-Session-Token to every redirect target, so refuse + // redirects that leave the Coder origin. + CheckRedirect: sameorigin.RedirectPolicy(coderURL), + } +} + // Config describes how to construct a Coder SDK client. type Config struct { CoderURL *url.URL @@ -136,16 +150,7 @@ func NewSDKClient(cfg Config) (*codersdk.Client, error) { return nil, fmt.Errorf("get Coder transport: %w", err) } - client := codersdk.New(&coderURL, codersdk.WithHTTPClient(&http.Client{ - // codersdk dials websockets (WorkspaceBuildLogsAfter for ?follow=true) - // with a new http.Client that keeps only this Transport, not - // CheckRedirect, so the Transport also refuses other origins. - Transport: sameorigin.RoundTripper(&coderURL, transport), - Timeout: requestTimeout, - // net/http copies Coder-Session-Token to every redirect target, so refuse - // redirects that leave the Coder origin. - CheckRedirect: sameorigin.RedirectPolicy(&coderURL), - })) + client := codersdk.New(&coderURL, codersdk.WithHTTPClient(newCoderHTTPClient(&coderURL, transport, requestTimeout))) if client == nil { return nil, fmt.Errorf("assertion failed: coder SDK client is nil after successful construction") } diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go index afe5eb6cd..1b22e53d1 100644 --- a/internal/aggregated/coder/errors_transport_test.go +++ b/internal/aggregated/coder/errors_transport_test.go @@ -16,10 +16,9 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" - "github.com/coder/coder-k8s/internal/sameorigin" ) -// TestMapCoderWriteErrorRealTransport sends a write through the transport of NewSDKClient and +// TestMapCoderWriteErrorRealTransport sends a write through the HTTP client of NewSDKClient and // checks that a write the Coder handler never got returns the 500, and a write it got returns // the uncertain 504. func TestMapCoderWriteErrorRealTransport(t *testing.T) { @@ -121,10 +120,7 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { tc.configure(transport) } t.Cleanup(transport.CloseIdleConnections) - client := codersdk.New(coderURL, codersdk.WithHTTPClient(&http.Client{ - Transport: sameorigin.RoundTripper(allowed, transport), - Timeout: 5 * time.Second, - })) + client := codersdk.New(coderURL, codersdk.WithHTTPClient(newCoderHTTPClient(allowed, transport, 5*time.Second))) before := received.Load() resp, err := client.Request(context.Background(), http.MethodPost, "/api/v2/workspaces", map[string]string{"name": "dev"}) From d7e4c0975a7e144009fce58e6f9d93a631c4737c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 13:37:11 +0000 Subject: [PATCH 17/27] test: expect uncertain outcomes after a redirect, a template version create, and an empty start/stop build (#242) Change-Id: I4588c1ee67d34dcc6cec852d4ce092ad550620fa Signed-off-by: Thomas Kosiewski --- .../aggregated/coder/errors_transport_test.go | 11 ++++++ .../storage/workspace_transition_test.go | 29 ++++++++++------ .../aggregated/storage/write_outcome_test.go | 34 +++++++++++++------ 3 files changed, 52 insertions(+), 22 deletions(-) diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go index 1b22e53d1..d1d2a807f 100644 --- a/internal/aggregated/coder/errors_transport_test.go +++ b/internal/aggregated/coder/errors_transport_test.go @@ -54,6 +54,16 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { _ = conn.Close() })) t.Cleanup(brokenServer.Close) + // redirectServer gets the write, answers with a same-origin redirect, and stops listening, so + // the request that follows the redirect cannot connect. + var redirectServer *httptest.Server + redirectServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received.Add(1) + _ = redirectServer.Listener.Close() + w.Header().Set("Connection", "close") + http.Redirect(w, r, "/api/v2/moved", http.StatusTemporaryRedirect) + })) + t.Cleanup(redirectServer.Close) refusingProxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodConnect { t.Errorf("expected CONNECT, got %s", r.Method) @@ -105,6 +115,7 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { {name: "server refuses the TLS version", coderURL: tls13Server.URL, configure: tls12With()}, {name: "server refuses the cipher suites", coderURL: cipherServer.URL, configure: tls12With(tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305)}, {name: "connection closed after the request", coderURL: brokenServer.URL, wantSent: true}, + {name: "redirect, then the next connection is refused", coderURL: redirectServer.URL, wantSent: true}, } { t.Run(tc.name, func(t *testing.T) { coderURL := mustParse(tc.coderURL) diff --git a/internal/aggregated/storage/workspace_transition_test.go b/internal/aggregated/storage/workspace_transition_test.go index 224b70110..9dc2d36eb 100644 --- a/internal/aggregated/storage/workspace_transition_test.go +++ b/internal/aggregated/storage/workspace_transition_test.go @@ -485,20 +485,23 @@ func TestWorkspaceTransitionUncertainPost(t *testing.T) { found *found status int // a Coder status for the POST; 0 stalls it until the client times out wantQueued bool + jsonBody string // with a 2xx status, a JSON body instead of an HTML page }{ - {"the build was queued", stop, false, &found{transition: stop}, 0, true}, - {"nothing changed", stop, false, nil, 0, false}, - {"a proxy answered 502", stop, false, nil, http.StatusBadGateway, false}, - {"a proxy answered 503 after the build was queued", stop, false, &found{transition: stop}, http.StatusServiceUnavailable, true}, - {"Coder answered 504", stop, false, nil, http.StatusGatewayTimeout, false}, + {"the build was queued", stop, false, &found{transition: stop}, 0, true, ""}, + {"nothing changed", stop, false, nil, 0, false, ""}, + {"a proxy answered 502", stop, false, nil, http.StatusBadGateway, false, ""}, + {"a proxy answered 503 after the build was queued", stop, false, &found{transition: stop}, http.StatusServiceUnavailable, true, ""}, + {"Coder answered 504", stop, false, nil, http.StatusGatewayTimeout, false, ""}, // Coder answers the POST with 201. Since codersdk v2.37, a 201 without the build's JSON is a // *codersdk.Error with status 201. Another 2xx always was one. - {"a proxy answered 201 with an HTML page after the build was queued", stop, false, &found{transition: stop}, http.StatusCreated, true}, - {"a proxy answered 200 with an HTML page", stop, false, nil, http.StatusOK, false}, - {"another user queued the same transition", stop, false, &found{transition: stop, byOther: true}, 0, false}, - {"the new build has the other transition", stop, false, &found{transition: start}, 0, false}, - {"a start with the requested version", start, true, &found{transition: start}, 0, true}, - {"a start with another version", start, true, &found{transition: start, oldVersion: true}, 0, false}, + {"a proxy answered 201 with an HTML page after the build was queued", stop, false, &found{transition: stop}, http.StatusCreated, true, ""}, + {"a proxy answered 200 with an HTML page", stop, false, nil, http.StatusOK, false, ""}, + {"a proxy answered 201 with {} after the build was queued", stop, false, &found{transition: stop}, http.StatusCreated, true, "{}"}, + {"a proxy answered 201 with null", stop, false, nil, http.StatusCreated, false, "null"}, + {"another user queued the same transition", stop, false, &found{transition: stop, byOther: true}, 0, false, ""}, + {"the new build has the other transition", stop, false, &found{transition: start}, 0, false, ""}, + {"a start with the requested version", start, true, &found{transition: start}, 0, true, ""}, + {"a start with another version", start, true, &found{transition: start, oldVersion: true}, 0, false, ""}, } { previous := codersdk.WorkspaceTransitionStart if tc.transition == start { @@ -526,6 +529,10 @@ func TestWorkspaceTransitionUncertainPost(t *testing.T) { f.update(func(ws *codersdk.Workspace) { ws.LatestBuild = queued }) } switch { + case tc.jsonBody != "": + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tc.status) + _, _ = w.Write([]byte(tc.jsonBody)) case tc.status >= 200 && tc.status < 300: w.Header().Set("Content-Type", "text/html") w.WriteHeader(tc.status) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index d0dd778a1..8a9ec612a 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -135,6 +135,21 @@ type writeCase struct { run func(t *testing.T, provider coder.ClientProvider) error } +// updateTemplateFiles changes the files of a template. The update creates a template version +// and then activates it. +func updateTemplateFiles(t *testing.T, provider coder.ClientProvider) error { + ctx := namespacedContext("control-plane") + templates := NewTemplateStorage(provider) + current, err := templates.Get(ctx, "acme.starter-template", nil) + if err != nil { + t.Fatalf("get template: %v", err) + } + desired := current.(*aggregationv1alpha1.CoderTemplate).DeepCopy() + desired.Spec.Files = map[string]string{"main.tf": `resource "null_resource" "changed" {}`} + _, _, err = templates.Update(ctx, desired.Name, rest.DefaultUpdatedObjectInfo(desired), nil, nil, false, nil) + return err +} + func writeCases() []writeCase { ctx := func() context.Context { return namespacedContext("control-plane") } isPost := func(suffix string) func(*http.Request) bool { @@ -222,22 +237,19 @@ func writeCases() []writeCase { return err }, }, + { + name: "template version of new files", + isWrite: isPost("/templateversions"), + checksAnswer: true, + readsBody: true, + run: updateTemplateFiles, + }, { name: "template activation of new files", isWrite: func(r *http.Request) bool { return r.Method == http.MethodPatch && strings.HasSuffix(r.URL.Path, "/versions") }, - run: func(t *testing.T, provider coder.ClientProvider) error { - templates := NewTemplateStorage(provider) - current, err := templates.Get(ctx(), "acme.starter-template", nil) - if err != nil { - t.Fatalf("get template: %v", err) - } - desired := current.(*aggregationv1alpha1.CoderTemplate).DeepCopy() - desired.Spec.Files = map[string]string{"main.tf": `resource "null_resource" "changed" {}`} - _, _, err = templates.Update(ctx(), desired.Name, rest.DefaultUpdatedObjectInfo(desired), nil, nil, false, nil) - return err - }, + run: updateTemplateFiles, }, { name: "template delete", From fc3c816da8d336372cf2e64d1b48f70027d4902d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 13:44:25 +0000 Subject: [PATCH 18/27] fix: keep writes uncertain after a redirect, a template version create, and an empty start/stop build (#242) Change-Id: I1319dc4ff7a39fffdf311cc664177f3de2d19596 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 5 +++-- internal/aggregated/coder/config.go | 17 ++++++++++++++++- internal/aggregated/coder/errors.go | 6 +++++- internal/aggregated/storage/template.go | 5 +++-- .../aggregated/storage/workspace_transition.go | 11 ++++++----- 5 files changed, 33 insertions(+), 11 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 3041b7440..22d2708da 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -230,7 +230,8 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A `502` or a `503`. - A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. -- An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. +- An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, a template version answer of a template files update without the ID of the version, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. +- A redirect to another origin, which the server refuses, or a redirect whose next request fails. The server that sent the redirect got the write. - A connection that breaks after the server sent the request, before or while the server reads the answer. A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. The server knows this for these errors: @@ -463,7 +464,7 @@ Other status codes: One time budget of 25 seconds covers every Coder call of a request: the lookup, the build request, the re-read after a Coder `409`, and the confirming re-read after an uncertain build request. The calls before the confirming re-read must end within 20 seconds, so at least 5 seconds stay for it. Each call also ends after the Coder request timeout (`--coder-request-timeout`, 30 seconds by default). - If Coder answers `409` to the build request, another build started after the lookup. The server re-reads the workspace one time. If the latest build already does what you asked, the outcome is `InProgress` or `Unchanged`. Otherwise the server returns `409` with the message from Coder. -- If the build request times out, fails on the network, or gets `502`, `503`, or `504` (for example from a proxy in front of Coder), the server cannot know whether Coder queued the build. It re-reads the workspace one time, in the rest of the budget. If the latest build is a new build of the requested kind, started by the operator user of the control plane, and of the template version that the request sent (if it sent one), the outcome is `Queued`. A build that another user started in the meantime, for example in the Coder UI, does not count. Otherwise, or when the server cannot read the operator user, it returns `504`, and the message says that the result is uncertain. +- If the build request times out, fails on the network, gets `502`, `503`, or `504` (for example from a proxy in front of Coder), or gets an answer without the ID of the build (for example `201` with `{}`), the server cannot know whether Coder queued the build. It re-reads the workspace one time, in the rest of the budget. If the latest build is a new build of the requested kind, started by the operator user of the control plane, and of the template version that the request sent (if it sent one), the outcome is `Queued`. A build that another user started in the meantime, for example in the Coder UI, does not count. Otherwise, or when the server cannot read the operator user, it returns `504`, and the message says that the result is uncertain. - Known gap: if the operator user itself starts a matching build somewhere else in that window, for example with its token in the Coder CLI, the server cannot tell the two builds apart. It then answers `Queued` and names that build. - After a `504`, re-read the latest build of the workspace (`status.latestBuildID` and `status.latestBuildStatus` of the `CoderWorkspace`) before you retry. - The server never sends a second build request by itself. diff --git a/internal/aggregated/coder/config.go b/internal/aggregated/coder/config.go index 17a4cf3dd..8d8bdb9e0 100644 --- a/internal/aggregated/coder/config.go +++ b/internal/aggregated/coder/config.go @@ -112,7 +112,7 @@ func newCoderHTTPClient(coderURL *url.URL, transport http.RoundTripper, timeout // codersdk dials websockets (WorkspaceBuildLogsAfter for ?follow=true) // with a new http.Client that keeps only this Transport, not // CheckRedirect, so the Transport also refuses other origins. - Transport: sameorigin.RoundTripper(coderURL, transport), + Transport: markRedirectFollowUps{next: sameorigin.RoundTripper(coderURL, transport)}, Timeout: timeout, // net/http copies Coder-Session-Token to every redirect target, so refuse // redirects that leave the Coder origin. @@ -120,6 +120,21 @@ func newCoderHTTPClient(coderURL *url.URL, transport http.RoundTripper, timeout } } +// errAfterRedirect marks the error of a request that follows a redirect. The server that answered +// with the redirect got the write, so a follow-up that fails before its own send does not make +// the write certain. +var errAfterRedirect = errors.New("the request after a redirect failed") + +type markRedirectFollowUps struct{ next http.RoundTripper } + +func (m markRedirectFollowUps) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := m.next.RoundTrip(req) + if err != nil && req.Response != nil { + return nil, fmt.Errorf("%w: %w", errAfterRedirect, err) + } + return resp, err +} + // Config describes how to construct a Coder SDK client. type Config struct { CoderURL *url.URL diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 0cddd2c38..247c70d21 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -176,8 +176,12 @@ var handshakeRefusals = []tls.AlertError{40, 70} // notSent reports whether err proves that the request did not reach Coder: the connection to // Coder (or to the proxy in front of it) failed, the proxy refused the CONNECT tunnel, the TLS // handshake failed, or the client refused a request to another origin. A refused redirect is not -// in this list: the server that answered with the redirect got the write. +// in this list, and neither is an error of a request that followed a redirect: the server that +// answered with the redirect got the write. func notSent(err error) bool { + if errors.Is(err, errAfterRedirect) { + return false + } var opErr *net.OpError if errors.As(err, &opErr) { switch opErr.Op { diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 42e16a65c..a24cb5426 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -755,10 +755,11 @@ func (s *TemplateStorage) Update( Provisioner: codersdk.ProvisionerTypeTerraform, }) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name) } if newVersion.ID == uuid.Nil { - return nil, false, fmt.Errorf("assertion failed: new template version ID must not be nil") + return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, + "an answer from the Coder API without the ID of the template version") } if waitErr := waitForTemplateVersionBuild(ctx, sdk, newVersion.ID); waitErr != nil { diff --git a/internal/aggregated/storage/workspace_transition.go b/internal/aggregated/storage/workspace_transition.go index 242b537e1..a6dc99a3f 100644 --- a/internal/aggregated/storage/workspace_transition.go +++ b/internal/aggregated/storage/workspace_transition.go @@ -144,7 +144,7 @@ func (s *WorkspaceTransitionStorage) Create( } } newBuild, err := sdk.CreateWorkspaceBuild(callCtx, workspace.ID, req) - if err == nil { + if err == nil && newBuild.ID != uuid.Nil { return s.queued(namespace, name, workspace, newBuild) } @@ -154,10 +154,11 @@ func (s *WorkspaceTransitionStorage) Create( case isCoderErr && coderErr.StatusCode() == http.StatusConflict: return s.afterConflict(callCtx, sdk, namespace, name, workspace.ID, coderErr) case !isCoderErr || coder.IsUncertainStatus(coderErr.StatusCode()): - // A timeout, a transport error, or a gateway error leaves it open whether Coder queued - // the build: a proxy in front of Coder can fail after Coder accepted it. Never - // POST again: one confirming re-read either finds the new build or reports the doubt. - // The re-read gets the rest of the budget, at least confirmTimeout. + // A timeout, a transport error, a gateway error, or an answer without the build ID + // leaves it open whether Coder queued the build: a proxy in front of Coder can fail + // after Coder accepted it. Never POST again: one confirming re-read either finds the + // new build or reports the doubt. The re-read gets the rest of the budget, at least + // confirmTimeout. current, rereadErr := sdk.Workspace(budgetCtx, workspace.ID) if rereadErr == nil && confirmsPost(budgetCtx, sdk, workspace.LatestBuild.ID, current.LatestBuild, req) { return s.queued(namespace, name, current, current.LatestBuild) From 7ed566625d3e1b4c58cab489febba39c231e86fe Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:03:02 +0000 Subject: [PATCH 19/27] test: expect a failed re-read after a template activation to be uncertain (#242) Change-Id: I6c6ba68c29ba92cea1b46023560a6bd927050ae0 Signed-off-by: Thomas Kosiewski --- .../aggregated/storage/write_outcome_test.go | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index 8a9ec612a..67c0c445b 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -8,8 +8,10 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" + "github.com/google/uuid" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apiserver/pkg/registry/rest" @@ -324,3 +326,64 @@ func TestWriteOutcomes(t *testing.T) { }) } } + +// TestTemplateFilesUpdateReReadAfterActivation checks the re-read of the template after Coder +// accepted the activation of new files. When it fails or shows another active version, the new +// files can be active, so the update returns the uncertain 504. +func TestTemplateFilesUpdateReReadAfterActivation(t *testing.T) { + t.Parallel() + + otherActiveVersion := func(t *testing.T, w http.ResponseWriter, coderAnswer *httptest.ResponseRecorder) { + var object map[string]any + if err := json.Unmarshal(coderAnswer.Body.Bytes(), &object); err != nil { + t.Fatalf("decode Coder answer: %v", err) + } + object["active_version_id"] = uuid.NewString() + body, err := json.Marshal(object) + if err != nil { + t.Fatal(err) + } + jsonAnswer(coderAnswer.Code, string(body))(t, w, coderAnswer) + } + for _, tc := range []struct { + name string + answer brokenAnswer + // refused sends the re-read to a closed port instead of answer. + refused bool + }{ + {name: "proxy 502", answer: gatewayPage(http.StatusBadGateway)}, + {name: "connection closed without an answer", answer: closedWithoutAnswer}, + {name: "another active version", answer: otherActiveVersion}, + {name: "connection refused", refused: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + coderServer, _ := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + + var activated, reRead atomic.Bool + isReRead := func(r *http.Request) bool { + if r.Method == http.MethodPatch && strings.HasSuffix(r.URL.Path, "/versions") { + activated.Store(true) + return false + } + return activated.Load() && r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/api/v2/templates/") && + reRead.CompareAndSwap(false, true) + } + provider := newTestClientProvider(t, writeOutcomeServer(t, coderServer, isReRead, tc.answer).URL) + if tc.refused { + client := newTestSDKClient(t, coderServer.URL) + client.HTTPClient = &http.Client{Transport: refusingWrites{isWrite: isReRead, closedAddr: closedAddress(t)}} + provider = &coder.StaticClientProvider{Client: client, Namespace: "control-plane"} + } + + err := updateTemplateFiles(t, provider) + if !reRead.Load() { + t.Fatal("the update did not re-read the template after the activation") + } + if !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "uncertain") { + t.Fatalf("expected the uncertain 504, got %v", err) + } + }) + } +} From 168192b6617ab6f1ebbdb8707b48b2557abe1678 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:11:22 +0000 Subject: [PATCH 20/27] fix: treat a failed re-read after a template activation as uncertain (#242) Change-Id: Ibbe4f519feba961f090d9c8227c83e7424619d52 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 1 + internal/aggregated/storage/storage_test.go | 4 ++-- internal/aggregated/storage/template.go | 14 +++++++------- .../storage/template_update_order_test.go | 4 ++-- 4 files changed, 12 insertions(+), 11 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 22d2708da..d3f850e04 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -232,6 +232,7 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. - An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, a template version answer of a template files update without the ID of the version, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. - A redirect to another origin, which the server refuses, or a redirect whose next request fails. The server that sent the redirect got the write. +- A template files update whose activation Coder accepted, when the re-read of the template fails or shows another active version. - A connection that breaks after the server sent the request, before or while the server reads the answer. A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. The server knows this for these errors: diff --git a/internal/aggregated/storage/storage_test.go b/internal/aggregated/storage/storage_test.go index 3b1b2478b..600afd573 100644 --- a/internal/aggregated/storage/storage_test.go +++ b/internal/aggregated/storage/storage_test.go @@ -733,8 +733,8 @@ func TestTemplateStorageUpdateVerifiesActiveVersionPromotion(t *testing.T) { 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 !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "the result is uncertain") { + t.Fatalf("expected the uncertain 504 from the promotion verification, got %v", err) } if state.fileCount() != fileCountBefore+1 { diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index a24cb5426..2ba30de21 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -776,17 +776,17 @@ func (s *TemplateStorage) Update( return nil, false, coder.MapCoderWriteError(err, coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name) } - // Post-condition: verify that the active version changed. + // Post-condition: verify that the active version changed. Coder accepted the activation, + // so a re-read that fails or shows another version leaves the result open. verifyTemplate, err := sdk.Template(ctx, templateID) if err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, + "no re-read of the template from the Coder API after the activation") } 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(), - ) + return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, + fmt.Sprintf("a re-read from the Coder API with active version %s instead of %s after the activation", + verifyTemplate.ActiveVersionID, newVersion.ID)) } } } diff --git a/internal/aggregated/storage/template_update_order_test.go b/internal/aggregated/storage/template_update_order_test.go index 011231a10..0c162e95c 100644 --- a/internal/aggregated/storage/template_update_order_test.go +++ b/internal/aggregated/storage/template_update_order_test.go @@ -229,8 +229,8 @@ func TestTemplateStorageUpdateLeavesMetadataUnchangedWhenNewSourceIsNotActivated { name: "promotion does not take effect", setup: func(state *mockCoderServerState) { state.setFailActiveVersionPromotion(true) }, - wantErr: func(err error) bool { return err != nil }, - wantMessage: "active version promotion did not take effect", + wantErr: apierrors.IsTimeout, + wantMessage: "the result is uncertain", }, } From bcf31d3141e372c68b13c22cda9cac1ed619af34 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:28:50 +0000 Subject: [PATCH 21/27] test: expect a broken template version answer of a create to be uncertain (#242) Change-Id: If269babfc2e9726fe1d8904725344ae1fbc11dd7 Signed-off-by: Thomas Kosiewski --- .../aggregated/storage/write_outcome_test.go | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index 67c0c445b..9308adf52 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -137,6 +137,19 @@ type writeCase struct { run func(t *testing.T, provider coder.ClientProvider) error } +// createTemplate creates a template with files. The create creates a template version and then +// the template. +func createTemplate(_ *testing.T, provider coder.ClientProvider) error { + _, err := NewTemplateStorage(provider).Create(namespacedContext("control-plane"), &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + Files: map[string]string{"main.tf": `resource "null_resource" "proxied" {}`}, + }, + }, rest.ValidateAllObjectFunc, nil) + return err +} + // updateTemplateFiles changes the files of a template. The update creates a template version // and then activates it. func updateTemplateFiles(t *testing.T, provider coder.ClientProvider) error { @@ -209,16 +222,14 @@ func writeCases() []writeCase { isWrite: isPost("/templates"), checksAnswer: true, readsBody: true, - run: func(_ *testing.T, provider coder.ClientProvider) error { - _, err := NewTemplateStorage(provider).Create(ctx(), &aggregationv1alpha1.CoderTemplate{ - ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, - Spec: aggregationv1alpha1.CoderTemplateSpec{ - Organization: "acme", - Files: map[string]string{"main.tf": `resource "null_resource" "proxied" {}`}, - }, - }, rest.ValidateAllObjectFunc, nil) - return err - }, + run: createTemplate, + }, + { + name: "template version of a create", + isWrite: isPost("/templateversions"), + checksAnswer: true, + readsBody: true, + run: createTemplate, }, { name: "template update", From dcea5cf4de62e8a668018a87e85d8fa31a2d428a Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 15:29:15 +0000 Subject: [PATCH 22/27] fix: treat a broken template version answer of a create as uncertain (#242) Change-Id: Ic3e5466a07f5edd4472727b45c47ec5af01d8584 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 2 +- internal/aggregated/storage/template.go | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index d3f850e04..fbab62d82 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -230,7 +230,7 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A `502` or a `503`. - A `2xx` that Coder does not send: a status other than the expected one, or a body that is not the expected JSON. Examples on a create are a `201` with an HTML page, an empty body, invalid JSON, or a JSON array, and a `200` with an HTML page, invalid JSON, or the JSON of Coder. -- An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, a template version answer of a template files update without the ID of the version, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. +- An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, a template version answer of a template create or files update without the ID of the version, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. - A redirect to another origin, which the server refuses, or a redirect whose next request fails. The server that sent the redirect got the write. - A template files update whose activation Coder accepted, when the re-read of the template fails or shows another active version. - A connection that breaks after the server sent the request, before or while the server reads the answer. diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 2ba30de21..66b639aff 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -467,7 +467,11 @@ func (s *TemplateStorage) Create( Provisioner: codersdk.ProvisionerTypeTerraform, }) if err != nil { - return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + return nil, coder.MapCoderWriteError(err, coder.WriteCreate, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + } + if templateVersion.ID == uuid.Nil { + return nil, coder.UncertainWriteError(coder.WriteCreate, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name, + "an answer from the Coder API without the ID of the template version") } // Create the template only after its first version imports: until then Coder rejects workspace builds From 264287e2d17c279246defaeb74cdc1987ffe52a5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 16:04:12 +0000 Subject: [PATCH 23/27] test: expect a version LIST of a template without its ID to fail with 500 (#242) Change-Id: I759db7afb294e606fe1a562a962db311544ac22e Signed-off-by: Thomas Kosiewski --- internal/aggregated/storage/coder_answer_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/internal/aggregated/storage/coder_answer_test.go b/internal/aggregated/storage/coder_answer_test.go index ae8de632b..01b53aacc 100644 --- a/internal/aggregated/storage/coder_answer_test.go +++ b/internal/aggregated/storage/coder_answer_test.go @@ -51,6 +51,16 @@ func TestCoderAnswersWithoutNamesFailCleanly(t *testing.T) { return NewTemplateStorage(provider).Get(namespacedContext("control-plane"), "acme.starter-template", nil) }, }, + { + // Coder answers 404 to the versions of the zero template ID, which a LIST skips as a + // template deleted meanwhile, so the template must be checked first. + name: "template version list of a template without its ID", + isRead: func(r *http.Request) bool { return r.URL.Path == "/api/v2/templates" }, + body: func(*mockCoderServerState) string { return `[{}]` }, + read: func(provider coder.ClientProvider) (runtime.Object, error) { + return NewTemplateVersionStorage(provider).List(namespacedContext("control-plane"), nil) + }, + }, { name: "template version list", isRead: func(r *http.Request) bool { return strings.HasSuffix(r.URL.Path, "/versions") }, From 0251b23ff61dd41478cf05460119797d26a68593 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 16:04:33 +0000 Subject: [PATCH 24/27] fix: check each template before listing its versions (#242) Change-Id: Ifad7d7460c03785f2397b1aabcccb4d6474eceda Signed-off-by: Thomas Kosiewski --- internal/aggregated/storage/templateversion.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/aggregated/storage/templateversion.go b/internal/aggregated/storage/templateversion.go index 29c3b58c3..d9d6411e1 100644 --- a/internal/aggregated/storage/templateversion.go +++ b/internal/aggregated/storage/templateversion.go @@ -194,6 +194,11 @@ func (s *TemplateVersionStorage) listNamespace(ctx context.Context, namespace st items := make([]aggregationv1alpha1.CoderTemplateVersion, 0) for _, template := range templates { + // Check the template before its versions request: Coder answers 404 for the zero ID, and + // the LIST skips a 404 as a template deleted meanwhile. + if _, err := convert.TemplateToK8s(namespace, template); err != nil { + return nil, apierrors.NewInternalError(err) + } versions, err := sdk.TemplateVersionsByTemplate(ctx, codersdk.TemplateVersionsByTemplateRequest{ TemplateID: template.ID, IncludeArchived: true, From e1e1ce4bd92839a09906d68ff26463ea2fac416d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:18:12 +0000 Subject: [PATCH 25/27] test: cover post-handshake TLS records and the template read after an update (#242) Change-Id: Ie978bfb928e6e88bebf0be829f0a8495924f0ddf Signed-off-by: Thomas Kosiewski --- .../aggregated/coder/errors_transport_test.go | 32 +++++++++++++++ .../aggregated/storage/write_outcome_test.go | 41 +++++++++++++++++++ 2 files changed, 73 insertions(+) diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go index d1d2a807f..151211485 100644 --- a/internal/aggregated/coder/errors_transport_test.go +++ b/internal/aggregated/coder/errors_transport_test.go @@ -3,6 +3,7 @@ package coder import ( "context" "crypto/tls" + "io" "net" "net/http" "net/http/httptest" @@ -54,6 +55,30 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { _ = conn.Close() })) t.Cleanup(brokenServer.Close) + // recordServer reads the whole write after the TLS handshake and answers with header, the + // header of a TLS record that the client rejects. + recordServer := func(header []byte) *httptest.Server { + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if _, err := io.Copy(io.Discard, r.Body); err != nil { + t.Errorf("read body: %v", err) + return + } + received.Add(1) + conn, _, err := http.NewResponseController(w).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + defer func() { _ = conn.Close() }() + if _, err := conn.(*tls.Conn).NetConn().Write(header); err != nil { + t.Errorf("write record header: %v", err) + } + })) + t.Cleanup(server.Close) + return server + } + wrongVersionServer := recordServer([]byte{23, 3, 1, 0, 0}) + oversizedRecordServer := recordServer([]byte{23, 3, 3, 0xff, 0xff}) // redirectServer gets the write, answers with a same-origin redirect, and stops listening, so // the request that follows the redirect cannot connect. var redirectServer *httptest.Server @@ -93,6 +118,11 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { tr.TLSClientConfig = &tls.Config{MaxVersion: tls.VersionTLS12, CipherSuites: suites} //nolint:gosec // G402: the test needs a TLS 1.2 client that the servers refuse. } } + trust := func(server *httptest.Server) func(*http.Transport) { + return func(tr *http.Transport) { + tr.TLSClientConfig = server.Client().Transport.(*http.Transport).TLSClientConfig.Clone() + } + } proxy := func(raw string) func(*http.Transport) { return func(tr *http.Transport) { tr.Proxy = http.ProxyURL(mustParse(raw)) } } @@ -116,6 +146,8 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { {name: "server refuses the cipher suites", coderURL: cipherServer.URL, configure: tls12With(tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305)}, {name: "connection closed after the request", coderURL: brokenServer.URL, wantSent: true}, {name: "redirect, then the next connection is refused", coderURL: redirectServer.URL, wantSent: true}, + {name: "TLS record with the wrong version after the request", coderURL: wrongVersionServer.URL, configure: trust(wrongVersionServer), wantSent: true}, + {name: "oversized TLS record after the request", coderURL: oversizedRecordServer.URL, configure: trust(oversizedRecordServer), wantSent: true}, } { t.Run(tc.name, func(t *testing.T) { coderURL := mustParse(tc.coderURL) diff --git a/internal/aggregated/storage/write_outcome_test.go b/internal/aggregated/storage/write_outcome_test.go index 9308adf52..4a9a9ffd3 100644 --- a/internal/aggregated/storage/write_outcome_test.go +++ b/internal/aggregated/storage/write_outcome_test.go @@ -398,3 +398,44 @@ func TestTemplateFilesUpdateReReadAfterActivation(t *testing.T) { }) } } + +// TestTemplateUpdateReadAfterChange checks the read of the template that ends an update. When it +// fails after Coder applied the change, the update returns the uncertain 504. +func TestTemplateUpdateReadAfterChange(t *testing.T) { + t.Parallel() + + ran := 0 + for _, wc := range writeCases() { + if wc.name != "template update" && wc.name != "template activation of new files" { + continue + } + ran++ + t.Run(wc.name, func(t *testing.T) { + t.Parallel() + coderServer, _ := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + + var changed, finalRead atomic.Bool + isFinalRead := func(r *http.Request) bool { + if wc.isWrite(r) { + changed.Store(true) + return false + } + return changed.Load() && r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/api/v2/organizations/") && + finalRead.CompareAndSwap(false, true) + } + provider := newTestClientProvider(t, writeOutcomeServer(t, coderServer, isFinalRead, gatewayPage(http.StatusBadGateway)).URL) + + err := wc.run(t, provider) + if !finalRead.Load() { + t.Fatal("the update did not read the template after the change") + } + if !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "uncertain") { + t.Fatalf("expected the uncertain 504, got %v", err) + } + }) + } + if ran != 2 { + t.Fatalf("expected 2 template update cases, found %d", ran) + } +} From a8ad8ebdd4cc9cf772e3f6c73428745fa9e10c2b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:18:12 +0000 Subject: [PATCH 26/27] fix: map post-handshake TLS record errors and the final template read to the uncertain 504 (#242) Change-Id: If0958c852ba3da01ee4c4de2402cd9ebc9b2f549 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 3 ++- internal/aggregated/coder/errors.go | 4 +++- internal/aggregated/storage/template.go | 8 ++++++++ 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 5a1d70e89..c3965a629 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -233,7 +233,8 @@ A write (create, update, patch, or delete) of a workspace or a template can also - An answer without the object that the server needs: a create answer without the ID or name of the object, a build answer of a workspace update or delete without the ID of the build, a template version answer of a template create or files update without the ID of the version, or a template update answer without the template. An example is a `200` or `201` with `{}` or `null`. - A redirect to another origin, which the server refuses, or a redirect whose next request fails. The server that sent the redirect got the write. - A template files update whose activation Coder accepted, when the re-read of the template fails or shows another active version. -- A connection that breaks after the server sent the request, before or while the server reads the answer. +- A template update that changed the files or the metadata in Coder, when the read of the template that ends the update fails. +- A connection that breaks after the server sent the request, before or while the server reads the answer. This includes a malformed TLS record from Coder or the proxy after the TLS handshake. A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. The server knows this for these errors: diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 247c70d21..e31e0101c 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -197,8 +197,10 @@ func notSent(err error) bool { } } var certErr *tls.CertificateVerificationError + // crypto/tls sets Conn only for a first record that is not a TLS handshake, before the server + // hello. A malformed record after the handshake can follow a request that the server read. var recordErr tls.RecordHeaderError - return errors.As(err, &certErr) || errors.As(err, &recordErr) || + return errors.As(err, &certErr) || (errors.As(err, &recordErr) && recordErr.Conn != nil) || errors.Is(err, http.ErrSchemeMismatch) || errors.Is(err, errProxyConnectRefused) || errors.Is(err, sameorigin.ErrCrossOriginRequest) } diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 66b639aff..7952a66ce 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -709,6 +709,8 @@ func (s *TemplateStorage) Update( updatedTemplate.Spec.Description != currentTemplate.Spec.Description || updatedTemplate.Spec.Icon != currentTemplate.Spec.Icon + // changed records a Coder change that this update applied. + changed := false if updatedTemplate.Spec.Files != nil { if normalizedDesiredFiles == nil { return nil, false, fmt.Errorf("assertion failed: normalized desired template files must not be nil when spec.files is provided") @@ -792,6 +794,7 @@ func (s *TemplateStorage) Update( fmt.Sprintf("a re-read from the Coder API with active version %s instead of %s after the activation", verifyTemplate.ActiveVersionID, newVersion.ID)) } + changed = true } } @@ -807,10 +810,15 @@ func (s *TemplateStorage) Update( return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, "an answer from the Coder API without the updated template") } + changed = true } refreshedObj, err := s.Get(ctx, name, nil) if err != nil { + if changed { + return nil, false, coder.UncertainWriteError(coder.WriteUpdate, aggregationv1alpha1.Resource("codertemplates"), name, + "no re-read of the template from the Coder API after the update") + } return nil, false, err } From 8aa903d0841cbf61e50931cad3a904eee59db2ab Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:07:03 +0000 Subject: [PATCH 27/27] fix: never report a Coder write as not sent after its headers were written (#242) Remove the TLS alert classification from notSent: a Go TLS server can send protocol_version after it read the whole request. Mark the error of a round trip that wrote request headers on any attempt, so an HTTP/2 retry that fails to redial returns the uncertain 504. Change-Id: Ic5276b3952575ba9c21f4e61ffffb53d29849863 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 7 +- go.mod | 2 +- internal/aggregated/coder/config.go | 35 +++-- internal/aggregated/coder/errors.go | 37 ++--- .../aggregated/coder/errors_transport_test.go | 135 +++++++++++++++++- 5 files changed, 175 insertions(+), 41 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index c3965a629..5d37296d8 100644 --- a/docs/reference/aggregated-api-behavior.md +++ b/docs/reference/aggregated-api-behavior.md @@ -234,14 +234,15 @@ A write (create, update, patch, or delete) of a workspace or a template can also - A redirect to another origin, which the server refuses, or a redirect whose next request fails. The server that sent the redirect got the write. - A template files update whose activation Coder accepted, when the re-read of the template fails or shows another active version. - A template update that changed the files or the metadata in Coder, when the read of the template that ends the update fails. -- A connection that breaks after the server sent the request, before or while the server reads the answer. This includes a malformed TLS record from Coder or the proxy after the TLS handshake. +- A connection that breaks after the server sent the request, before or while the server reads the answer. This includes a malformed TLS record or a TLS alert from Coder or the proxy after the TLS handshake. +- An error of a later attempt of a request that the server already sent. For example, the HTTP/2 client sends the request again on a new connection after Coder or the proxy resets the stream. If that connection is refused, the write is still uncertain. +- A TLS handshake that Coder or the proxy refuses, for example because it does not support the TLS version or the cipher suites of the client. The server cannot tell this refusal apart from a TLS alert after the request. -A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. The server knows this for these errors: +A write that the server knows did not reach Coder keeps the `500` that says that the server cannot reach Coder. Nothing changed in Coder, so you can retry it. A write counts as not sent only if the server wrote no request headers on any attempt of the request. Then the server knows it for these errors: - A refused connection to Coder or to the proxy, or a failed DNS lookup. - A proxy that refuses the `CONNECT` tunnel to Coder, for example with `407`. - A rejected TLS certificate. -- A TLS handshake that the server refuses because it does not support the TLS version or the cipher suites of the client. - An `https` Coder URL that a plain HTTP server answers. - A request to an origin other than the Coder URL, which the server refuses before it sends it. diff --git a/go.mod b/go.mod index 70ade5062..cab03e0c2 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/prometheus/client_golang v1.24.1 github.com/stretchr/testify v1.12.0 github.com/yuin/gopher-lua v1.1.2 + golang.org/x/net v0.60.0 golang.org/x/xerrors v0.0.0-20240903120638-7835f813f4da k8s.io/api v0.35.0 k8s.io/apiextensions-apiserver v0.35.0 @@ -403,7 +404,6 @@ require ( golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f // indirect golang.org/x/exp/typeparams v0.0.0-20251023183803-a4bb9ffd2546 // indirect golang.org/x/mod v0.41.0 // indirect - golang.org/x/net v0.60.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.23.0 // indirect golang.org/x/sys v0.48.0 // indirect diff --git a/internal/aggregated/coder/config.go b/internal/aggregated/coder/config.go index 21e3ce4c8..b8c1fab10 100644 --- a/internal/aggregated/coder/config.go +++ b/internal/aggregated/coder/config.go @@ -7,9 +7,11 @@ import ( "fmt" "io" "net/http" + "net/http/httptrace" "net/url" "strings" "sync" + "sync/atomic" "time" "github.com/coder/coder/v2/codersdk" @@ -252,7 +254,7 @@ func newCoderHTTPClient(coderURL *url.URL, transport http.RoundTripper, timeout // codersdk dials websockets (WorkspaceBuildLogsAfter for ?follow=true) // with a new http.Client that keeps only this Transport, not // CheckRedirect, so the Transport also refuses other origins. - Transport: markRedirectFollowUps{next: sameorigin.RoundTripper(coderURL, transport)}, + Transport: markMaybeSent{next: sameorigin.RoundTripper(coderURL, transport)}, Timeout: timeout, // net/http copies Coder-Session-Token to every redirect target, so refuse // redirects that leave the Coder origin. @@ -265,17 +267,34 @@ func newCoderHTTPClient(coderURL *url.URL, transport http.RoundTripper, timeout // the write certain. var errAfterRedirect = errors.New("the request after a redirect failed") -type markRedirectFollowUps struct{ next http.RoundTripper } - -func (m markRedirectFollowUps) RoundTrip(req *http.Request) (*http.Response, error) { - resp, err := m.next.RoundTrip(req) - if err != nil && req.Response != nil { +// errAfterWrite marks the error of a request whose headers net/http wrote. A later error of a new +// connection does not make the write certain: the HTTP/2 client sends a request again on a new +// connection after the server reset its stream with PROTOCOL_ERROR, which it can do after it read +// the request. +var errAfterWrite = errors.New("the request failed after net/http wrote it") + +// markMaybeSent marks the errors of requests that can have reached Coder with errAfterRedirect or +// errAfterWrite. +type markMaybeSent struct{ next http.RoundTripper } + +func (m markMaybeSent) RoundTrip(req *http.Request) (*http.Response, error) { + var wrote atomic.Bool + traced := req.WithContext(httptrace.WithClientTrace(req.Context(), &httptrace.ClientTrace{ + WroteHeaders: func() { wrote.Store(true) }, + })) + resp, err := m.next.RoundTrip(traced) + switch { + case err == nil: + return resp, nil + case req.Response != nil: return nil, fmt.Errorf("%w: %w", errAfterRedirect, err) + case wrote.Load(): + return nil, fmt.Errorf("%w: %w", errAfterWrite, err) } - return resp, err + return nil, err } -func (m markRedirectFollowUps) CloseIdleConnections() { +func (m markMaybeSent) CloseIdleConnections() { if closer, ok := m.next.(interface{ CloseIdleConnections() }); ok { closer.CloseIdleConnections() } diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index e31e0101c..27127d1cd 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -169,36 +169,25 @@ func UncertainWriteError(write Write, resource schema.GroupResource, name, reaso ), 0) } -// handshakeRefusals are the TLS alerts (RFC 8446, section 6.2) with which a server refuses the -// handshake before it reads a request: handshake_failure (40) and protocol_version (70). -var handshakeRefusals = []tls.AlertError{40, 70} - -// notSent reports whether err proves that the request did not reach Coder: the connection to -// Coder (or to the proxy in front of it) failed, the proxy refused the CONNECT tunnel, the TLS -// handshake failed, or the client refused a request to another origin. A refused redirect is not -// in this list, and neither is an error of a request that followed a redirect: the server that -// answered with the redirect got the write. +// notSent reports whether err proves that the request did not reach Coder. Each error that it +// accepts comes from a step before net/http writes the request: the dial of the connection to Coder +// or to the proxy in front of it, the proxy answer to the CONNECT tunnel, the verification of the +// certificate, a first record of the server that is not TLS (crypto/tls sets Conn only before the +// server hello), and the refusal of a request to another origin. A received TLS alert is not in +// this list: crypto/tls returns an alert that the server sends after it read the request in the +// same way as one that refuses the handshake. A refused redirect is not in this list either, and +// neither is an error of a request that followed a redirect: the server that answered with the +// redirect got the write. An error after net/http wrote the request (errAfterWrite) never proves +// it, even when a retry on a new connection failed in one of those steps. func notSent(err error) bool { - if errors.Is(err, errAfterRedirect) { + if errors.Is(err, errAfterRedirect) || errors.Is(err, errAfterWrite) { return false } var opErr *net.OpError - if errors.As(err, &opErr) { - switch opErr.Op { - case "dial", "proxyconnect": - return true - case "remote error": - // crypto/tls keeps a received alert in an unexported type with the text of AlertError. - for _, alert := range handshakeRefusals { - if opErr.Err != nil && opErr.Err.Error() == alert.Error() { - return true - } - } - } + if errors.As(err, &opErr) && (opErr.Op == "dial" || opErr.Op == "proxyconnect") { + return true } var certErr *tls.CertificateVerificationError - // crypto/tls sets Conn only for a first record that is not a TLS handshake, before the server - // hello. A malformed record after the handshake can follow a request that the server read. var recordErr tls.RecordHeaderError return errors.As(err, &certErr) || (errors.As(err, &recordErr) && recordErr.Conn != nil) || errors.Is(err, http.ErrSchemeMismatch) || errors.Is(err, errProxyConnectRefused) || diff --git a/internal/aggregated/coder/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go index 151211485..9141611f8 100644 --- a/internal/aggregated/coder/errors_transport_test.go +++ b/internal/aggregated/coder/errors_transport_test.go @@ -14,14 +14,15 @@ import ( "time" "github.com/coder/coder/v2/codersdk" + "golang.org/x/net/http2" apierrors "k8s.io/apimachinery/pkg/api/errors" aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" ) // TestMapCoderWriteErrorRealTransport sends a write through the HTTP client of NewSDKClient and -// checks that a write the Coder handler never got returns the 500, and a write it got returns -// the uncertain 504. +// checks that a write the Coder handler got returns the uncertain 504. A write that it did not +// get returns the 500 only when the error proves that the request was not sent. func TestMapCoderWriteErrorRealTransport(t *testing.T) { t.Parallel() @@ -78,6 +79,34 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { return server } wrongVersionServer := recordServer([]byte{23, 3, 1, 0, 0}) + // alertServer reads the whole write, then reads a record with the wrong version, which makes + // it send a protocol_version alert: a TLS alert after the request, not a handshake refusal. + alertServer := func(version uint16) *httptest.Server { + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if _, err := io.Copy(io.Discard, r.Body); err != nil { + t.Errorf("read body: %v", err) + return + } + received.Add(1) + conn, _, err := http.NewResponseController(w).Hijack() + if err != nil { + t.Errorf("hijack: %v", err) + return + } + defer func() { _ = conn.Close() }() + tlsConn := conn.(*tls.Conn) + tlsConn.NetConn().(*injectReadConn).injected = []byte{23, 3, 1, 0, 0} + _, _ = tlsConn.Read(make([]byte, 1)) + })) + server.Listener = injectReadListener{server.Listener} + server.TLS = &tls.Config{MinVersion: version, MaxVersion: version} //nolint:gosec // G402: the test pins TLS 1.2 and TLS 1.3 servers. + server.StartTLS() + t.Cleanup(server.Close) + return server + } + tls12AlertServer := alertServer(tls.VersionTLS12) + tls13AlertServer := alertServer(tls.VersionTLS13) + h2ResetURL, h2ResetTLS := h2ResetServer(t, &received) oversizedRecordServer := recordServer([]byte{23, 3, 3, 0xff, 0xff}) // redirectServer gets the write, answers with a same-origin redirect, and stops listening, so // the request that follows the redirect cannot connect. @@ -135,6 +164,9 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { sameOrigin string configure func(*http.Transport) wantSent bool + // wantUncertain is set for a write that the handler did not get, but whose error does not + // prove that. + wantUncertain bool }{ {name: "connection refused", coderURL: "http://" + closedAddr}, {name: "certificate rejected", coderURL: certServer.URL}, @@ -142,12 +174,18 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { {name: "request to another origin", coderURL: plainServer.URL, sameOrigin: "http://allowed.invalid"}, {name: "proxy refuses CONNECT with 407", coderURL: "https://coder.invalid", configure: proxy(refusingProxy.URL)}, {name: "https URL answered by an HTTP server", coderURL: strings.Replace(plainServer.URL, "http://", "https://", 1)}, - {name: "server refuses the TLS version", coderURL: tls13Server.URL, configure: tls12With()}, - {name: "server refuses the cipher suites", coderURL: cipherServer.URL, configure: tls12With(tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305)}, + {name: "server refuses the TLS version", coderURL: tls13Server.URL, configure: tls12With(), wantUncertain: true}, + {name: "server refuses the cipher suites", coderURL: cipherServer.URL, configure: tls12With(tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305), wantUncertain: true}, {name: "connection closed after the request", coderURL: brokenServer.URL, wantSent: true}, {name: "redirect, then the next connection is refused", coderURL: redirectServer.URL, wantSent: true}, {name: "TLS record with the wrong version after the request", coderURL: wrongVersionServer.URL, configure: trust(wrongVersionServer), wantSent: true}, {name: "oversized TLS record after the request", coderURL: oversizedRecordServer.URL, configure: trust(oversizedRecordServer), wantSent: true}, + {name: "TLS 1.2 protocol_version alert after the request", coderURL: tls12AlertServer.URL, configure: trust(tls12AlertServer), wantSent: true}, + {name: "TLS 1.3 protocol_version alert after the request", coderURL: tls13AlertServer.URL, configure: trust(tls13AlertServer), wantSent: true}, + { + name: "HTTP/2 stream reset after the request, then the next connection is refused", coderURL: h2ResetURL, + configure: func(tr *http.Transport) { tr.TLSClientConfig = h2ResetTLS.Clone() }, wantSent: true, + }, } { t.Run(tc.name, func(t *testing.T) { coderURL := mustParse(tc.coderURL) @@ -178,7 +216,7 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { } mapped := MapCoderWriteError(err, WriteCreate, aggregationv1alpha1.Resource("coderworkspaces"), "acme.alice.dev") - if tc.wantSent { + if tc.wantSent || tc.wantUncertain { if !apierrors.IsTimeout(mapped) || !strings.Contains(mapped.Error(), "uncertain") { t.Fatalf("expected the uncertain 504 for %v, got %v", err, mapped) } @@ -190,3 +228,90 @@ func TestMapCoderWriteErrorRealTransport(t *testing.T) { }) } } + +// injectReadConn returns injected before the bytes of the connection, to model a corrupted record. +type injectReadConn struct { + net.Conn + injected []byte +} + +func (c *injectReadConn) Read(p []byte) (int, error) { + if len(c.injected) != 0 { + n := copy(p, c.injected) + c.injected = c.injected[n:] + return n, nil + } + return c.Conn.Read(p) +} + +type injectReadListener struct{ net.Listener } + +func (l injectReadListener) Accept() (net.Conn, error) { + conn, err := l.Listener.Accept() + if err != nil { + return nil, err + } + return &injectReadConn{Conn: conn}, nil +} + +// h2ResetServer starts an HTTP/2 server for one connection. It reads the whole write, counts it in +// received, and answers with GOAWAY and RST_STREAM(PROTOCOL_ERROR). The Go client then sends the +// write again on a new connection, which the closed listener refuses. h2ResetServer returns the +// URL of the server and the TLS config of a client that trusts it. +func h2ResetServer(t *testing.T, received *atomic.Int32) (string, *tls.Config) { + t.Helper() + certServer := httptest.NewUnstartedServer(nil) + certServer.EnableHTTP2 = true + certServer.StartTLS() + serverTLS := certServer.TLS.Clone() + serverTLS.NextProtos = []string{"h2"} + clientTLS := certServer.Client().Transport.(*http.Transport).TLSClientConfig.Clone() + certServer.Close() + + listener, err := tls.Listen("tcp", "127.0.0.1:0", serverTLS) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { _ = listener.Close() }) + go func() { + conn, err := listener.Accept() + _ = listener.Close() + if err != nil { + return + } + defer func() { _ = conn.Close() }() + if _, err := io.ReadFull(conn, make([]byte, len(http2.ClientPreface))); err != nil { + t.Errorf("read preface: %v", err) + return + } + framer := http2.NewFramer(conn, conn) + if err := framer.WriteSettings(); err != nil { + t.Errorf("write settings: %v", err) + return + } + for { + frame, err := framer.ReadFrame() + if err != nil { + t.Errorf("read frame: %v", err) + return + } + switch frame := frame.(type) { + case *http2.SettingsFrame: + if !frame.IsAck() { + _ = framer.WriteSettingsAck() + } + case *http2.DataFrame: + if !frame.StreamEnded() { + continue + } + received.Add(1) + _ = framer.WriteGoAway(frame.StreamID, http2.ErrCodeNo, nil) + _ = framer.WriteRSTStream(frame.StreamID, http2.ErrCodeProtocol) + // Keep the connection open until the client has read both frames. + _, _ = io.Copy(io.Discard, conn) + return + } + } + }() + return "https://" + listener.Addr().String(), clientTLS +}