From aebc7778d145d180b2c3530c4c1aff2d98a5e053 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 13:44:03 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20report=20an=20unreada?= =?UTF-8?q?ble=202xx=20create=20answer=20as=20an=20uncertain=20504=20(Refs?= =?UTF-8?q?=20#163)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: Idb95ec07d36db8f82ba80ead3a0bcdc6f6c233f8 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 4 +- internal/aggregated/coder/errors.go | 23 ++++ internal/aggregated/coder/errors_test.go | 47 +++++++ .../storage/create_uncertain_test.go | 121 ++++++++++++++++++ internal/aggregated/storage/template.go | 4 +- internal/aggregated/storage/workspace.go | 2 +- 6 files changed, 197 insertions(+), 4 deletions(-) create mode 100644 internal/aggregated/storage/create_uncertain_test.go diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index a37ea17b7..d7922f7d5 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. After a `504` on a write, re-read before you retry. | +| [Coder timeouts](#coder-timeouts) | A Coder call that runs out of time returns `504` on every resource. A create whose `2xx` answer cannot be read 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. | @@ -225,6 +225,8 @@ 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 also returns `504` when the answer to the create request has a `2xx` status but cannot be read. For example, a proxy in front of Coder sends an HTML page instead of the JSON of Coder. Coder can have created the object, so the message says that the result is uncertain. + The message does not include the Coder URL. - After a `504` on a read, try again later. diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 4c80c5c3f..a5d4cb2c5 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -70,6 +70,29 @@ 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 its answer could not be read: Coder answers JSON, so the SDK +// returns this error for another 2xx, and since codersdk v2.37 also for a 2xx whose body is not +// JSON (for example a proxy page). 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. +func MapCoderCreateError(err error, resource schema.GroupResource, name string) error { + if err == nil { + return fmt.Errorf("assertion failed: error must not be nil") + } + + 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 and a response that could not be read; re-read the object with kubectl get before you retry", + resource.Resource, name, coderErr.StatusCode(), + ), 0) + } + + return MapCoderError(err, resource, name) +} + func coderErrorMessage(coderErr *codersdk.Error, fallback error) string { if coderErr == nil { panic("assertion failed: coder error must not be nil") diff --git a/internal/aggregated/coder/errors_test.go b/internal/aggregated/coder/errors_test.go index 97832ce60..9a6a725bf 100644 --- a/internal/aggregated/coder/errors_test.go +++ b/internal/aggregated/coder/errors_test.go @@ -236,3 +236,50 @@ func withCoderMessage(err *codersdk.Error, message string) *codersdk.Error { return err } + +func TestMapCoderCreateError(t *testing.T) { + t.Parallel() + + resource := aggregationv1alpha1.Resource("codertemplates") + name := "acme.docker" + + // 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) + 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) + } + 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 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) + } + } + + // 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) + if got.Error() != want.Error() || apierrors.ReasonForError(got) != apierrors.ReasonForError(want) { + t.Fatalf("%v: expected the MapCoderError mapping %v, got %v", err, want, got) + } + } + + if err := MapCoderCreateError(nil, resource, name); err == nil || !strings.Contains(err.Error(), "assertion failed: error must not be nil") { + t.Fatalf("expected the nil-error assertion, got %v", err) + } +} diff --git a/internal/aggregated/storage/create_uncertain_test.go b/internal/aggregated/storage/create_uncertain_test.go new file mode 100644 index 000000000..0583497bc --- /dev/null +++ b/internal/aggregated/storage/create_uncertain_test.go @@ -0,0 +1,121 @@ +package storage + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/registry/rest" + + aggregationv1alpha1 "github.com/coder/coder-k8s/api/aggregation/v1alpha1" +) + +// creator is the Create method of the workspace and template storages. +type creator interface { + Create(context.Context, runtime.Object, rest.ValidateObjectFunc, *metav1.CreateOptions) (runtime.Object, error) +} + +// proxyAfterCreate fronts the mock Coder like a misbehaving proxy: Coder handles the create POST +// at createPath, but the client gets a 2xx HTML page instead of Coder's JSON answer. +func proxyAfterCreate(t *testing.T, coder *httptest.Server, createPath string, status int) *httptest.Server { + t.Helper() + + proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost || r.URL.Path != createPath { + coder.Config.Handler.ServeHTTP(w, r) + return + } + recorder := httptest.NewRecorder() + coder.Config.Handler.ServeHTTP(recorder, r) + if recorder.Code != http.StatusCreated { + t.Errorf("mock Coder answered the create with %d: %s", recorder.Code, recorder.Body.String()) + } + w.Header().Set("Content-Type", "text/html") + w.WriteHeader(status) + _, _ = w.Write([]byte("proxy page")) + })) + t.Cleanup(proxy.Close) + return proxy +} + +func TestCreateWithUnreadable2xxAnswerIsUncertain(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + createPath string + storage func(t *testing.T, url string) creator + object runtime.Object + created func(state *mockCoderServerState) bool + }{ + { + name: "workspace", + createPath: "/api/v2/users/alice/workspaces", + storage: func(t *testing.T, url string) creator { + return NewWorkspaceStorage(newTestClientProvider(t, url)) + }, + object: &aggregationv1alpha1.CoderWorkspace{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.alice.proxied"}, + Spec: aggregationv1alpha1.CoderWorkspaceSpec{Organization: "acme", TemplateName: "starter-template", Running: true}, + }, + created: func(state *mockCoderServerState) bool { return state.hasWorkspace("alice", "proxied") }, + }, + { + name: "template without files", + storage: func(t *testing.T, url string) creator { + return NewTemplateStorage(newTestClientProvider(t, url)) + }, + object: &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{Organization: "acme", VersionID: uuid.NewString()}, + }, + created: func(state *mockCoderServerState) bool { + _, ok := state.templateActiveVersionID("acme", "proxied") + return ok + }, + }, + { + name: "template with files", + storage: func(t *testing.T, url string) creator { + return NewTemplateStorage(newTestClientProvider(t, url)) + }, + object: &aggregationv1alpha1.CoderTemplate{ + ObjectMeta: metav1.ObjectMeta{Name: "acme.proxied"}, + Spec: aggregationv1alpha1.CoderTemplateSpec{ + Organization: "acme", + Files: map[string]string{"main.tf": `resource "null_resource" "proxied" {}`}, + }, + }, + created: func(state *mockCoderServerState) bool { + _, ok := state.templateActiveVersionID("acme", "proxied") + return ok + }, + }, + } { + for _, status := range []int{http.StatusOK, http.StatusCreated} { + coderServer, state := newMockCoderServer(t) + t.Cleanup(coderServer.Close) + createPath := tc.createPath + if createPath == "" { + createPath = "/api/v2/organizations/" + state.organization.ID.String() + "/templates" + } + proxy := proxyAfterCreate(t, coderServer, createPath, status) + storage := tc.storage(t, proxy.URL) + + _, err := storage.Create(namespacedContext("control-plane"), tc.object.DeepCopyObject(), rest.ValidateAllObjectFunc, nil) + if !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "uncertain") || strings.Contains(err.Error(), proxy.URL) { + t.Fatalf("%s, proxy status %d: expected an uncertain 504 without the Coder URL, got %v", tc.name, status, err) + } + if !tc.created(state) { + t.Fatalf("%s, proxy status %d: the mock Coder must have created the object", tc.name, status) + } + } + } +} diff --git a/internal/aggregated/storage/template.go b/internal/aggregated/storage/template.go index 9c561e72c..8e232d205 100644 --- a/internal/aggregated/storage/template.go +++ b/internal/aggregated/storage/template.go @@ -473,7 +473,7 @@ func (s *TemplateStorage) Create( Icon: templateObj.Spec.Icon, }) if err != nil { - return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } result := convert.TemplateToK8s(namespace, createdTemplate) @@ -493,7 +493,7 @@ func (s *TemplateStorage) Create( createdTemplate, err := sdk.CreateTemplate(ctx, org.ID, request) if err != nil { - return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) + return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("codertemplates"), templateObj.Name) } result := convert.TemplateToK8s(namespace, createdTemplate) diff --git a/internal/aggregated/storage/workspace.go b/internal/aggregated/storage/workspace.go index e2e96ed57..7f25efdc4 100644 --- a/internal/aggregated/storage/workspace.go +++ b/internal/aggregated/storage/workspace.go @@ -458,7 +458,7 @@ func (s *WorkspaceStorage) Create( createdWorkspace, err := sdk.CreateUserWorkspace(ctx, userName, request) if err != nil { - return nil, coder.MapCoderError(err, aggregationv1alpha1.Resource("coderworkspaces"), workspaceObj.Name) + return nil, coder.MapCoderCreateError(err, aggregationv1alpha1.Resource("coderworkspaces"), workspaceObj.Name) } if !workspaceObj.Spec.Running { From 0350910c0d7e8cd3be917b3ee1149ae08cb28cf4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 14:02:50 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20fix:=20assert=20create-error?= =?UTF-8?q?=20arguments=20first=20and=20narrow=20the=20uncertain-create=20?= =?UTF-8?q?docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I43e7fae2c7dde08c7ac7be4dfb2cb1fedd0009db Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 4 ++-- internal/aggregated/coder/errors.go | 19 +++++++++++++------ internal/aggregated/coder/errors_test.go | 17 +++++++++++++++-- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index d7922f7d5..87c8fbdec 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 whose `2xx` answer cannot be read also returns `504`. After a `504` on a write, re-read before you retry. | +| [Coder timeouts](#coder-timeouts) | A Coder call that runs out of time returns `504` on every resource. A create whose `2xx` answer is not JSON 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. | @@ -225,7 +225,7 @@ 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 also returns `504` when the answer to the create request has a `2xx` status but cannot be read. For example, a proxy in front of Coder sends an HTML page instead of the JSON of Coder. Coder can have created the object, so the message says that the result is uncertain. +A create of a workspace or a template also returns `504` when the answer to the create request has a `2xx` status, but its body is not the JSON that Coder sends. For example, a proxy in front of Coder sends an HTML page or an empty body. Coder can have created the object, so the message says that the result is uncertain. The message does not include the Coder URL. diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index a5d4cb2c5..02b857c50 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -71,21 +71,28 @@ 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 its answer could not be read: Coder answers JSON, so the SDK -// returns this error for another 2xx, and since codersdk v2.37 also for a 2xx whose body is not -// JSON (for example a proxy page). 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. +// 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 a 2xx whose +// body 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, also a body that breaks off while the SDK reads it: that is a plain error. func MapCoderCreateError(err error, resource schema.GroupResource, name string) error { if err == nil { return fmt.Errorf("assertion failed: error must not be nil") } + if resource.Empty() { + return fmt.Errorf("assertion failed: resource must not be empty") + } + if name == "" { + return fmt.Errorf("assertion failed: resource name must not be empty") + } 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 and a response that could not be read; re-read the object with kubectl get before you retry", + "the result is uncertain: the Coder API answered the create of %s %q with status %d but not with the JSON that Coder sends; re-read the object with kubectl get before you retry", resource.Resource, name, coderErr.StatusCode(), ), 0) } diff --git a/internal/aggregated/coder/errors_test.go b/internal/aggregated/coder/errors_test.go index 9a6a725bf..f4ec3696b 100644 --- a/internal/aggregated/coder/errors_test.go +++ b/internal/aggregated/coder/errors_test.go @@ -279,7 +279,20 @@ func TestMapCoderCreateError(t *testing.T) { } } - if err := MapCoderCreateError(nil, resource, name); err == nil || !strings.Contains(err.Error(), "assertion failed: error must not be nil") { - t.Fatalf("expected the nil-error assertion, got %v", err) + // 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") + for _, tc := range []struct { + err error + 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"}, + } { + if err := MapCoderCreateError(tc.err, tc.resource, tc.name); err == nil || apierrors.IsTimeout(err) || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("expected %q, got %v", tc.want, err) + } } } From 52d51bc5ec9f5307b0c6b3de6245586476b359d5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 14:13:13 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20docs:=20describe=20every=202?= =?UTF-8?q?xx=20create=20answer=20that=20maps=20to=20the=20uncertain=20504?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: I58136a816d42bfbae8247eaf8b1492d1213474b3 Signed-off-by: Thomas Kosiewski --- docs/reference/aggregated-api-behavior.md | 4 ++-- internal/aggregated/coder/errors.go | 13 ++++++----- .../storage/create_uncertain_test.go | 23 ++++++++++++++----- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index 87c8fbdec..d6f6d928b 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 whose `2xx` answer is not JSON also returns `504`. After a `504` on a write, re-read before you retry. | +| [Coder timeouts](#coder-timeouts) | A Coder call that runs out of time returns `504` on every resource. Most broken `2xx` answers to a create also return `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. | @@ -225,7 +225,7 @@ 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 also returns `504` when the answer to the create request has a `2xx` status, but its body is not the JSON that Coder sends. For example, a proxy in front of Coder sends an HTML page or an empty body. Coder can have created the object, so the message says that the result is uncertain. +A create of a workspace or a template also returns `504` when the answer to the create request has a `2xx` status, but is not the answer that Coder sends. For example, a proxy in front of Coder sends an HTML page, an empty body, or a status other than `201`. Coder can have created the object, so the message says that the result is uncertain. Some broken `2xx` answers still return `500`: a `200` with malformed JSON, and a connection that breaks while the server reads the answer. Coder can have created the object in these cases too, so re-read before you retry. The message does not include the Coder URL. diff --git a/internal/aggregated/coder/errors.go b/internal/aggregated/coder/errors.go index 02b857c50..abd4283a7 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -72,11 +72,12 @@ 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 a 2xx whose -// body 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, also a body that breaks off while the SDK reads it: that is a plain error. +// 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 { if err == nil { return fmt.Errorf("assertion failed: error must not be nil") @@ -92,7 +93,7 @@ func MapCoderCreateError(err error, resource schema.GroupResource, name string) 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 JSON that Coder sends; re-read the object with kubectl get before you retry", + "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) } diff --git a/internal/aggregated/storage/create_uncertain_test.go b/internal/aggregated/storage/create_uncertain_test.go index 0583497bc..92131c0a0 100644 --- a/internal/aggregated/storage/create_uncertain_test.go +++ b/internal/aggregated/storage/create_uncertain_test.go @@ -23,8 +23,9 @@ type creator interface { } // proxyAfterCreate fronts the mock Coder like a misbehaving proxy: Coder handles the create POST -// at createPath, but the client gets a 2xx HTML page instead of Coder's JSON answer. -func proxyAfterCreate(t *testing.T, coder *httptest.Server, createPath string, status int) *httptest.Server { +// at createPath, but the client gets status instead of Coder's 201. With keepJSON the body is +// Coder's JSON answer, otherwise an HTML page. +func proxyAfterCreate(t *testing.T, coder *httptest.Server, createPath string, status int, keepJSON bool) *httptest.Server { t.Helper() proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -37,6 +38,12 @@ func proxyAfterCreate(t *testing.T, coder *httptest.Server, createPath string, s if recorder.Code != http.StatusCreated { t.Errorf("mock Coder answered the create with %d: %s", recorder.Code, recorder.Body.String()) } + if keepJSON { + w.Header().Set("Content-Type", recorder.Header().Get("Content-Type")) + w.WriteHeader(status) + _, _ = w.Write(recorder.Body.Bytes()) + return + } w.Header().Set("Content-Type", "text/html") w.WriteHeader(status) _, _ = w.Write([]byte("proxy page")) @@ -99,22 +106,26 @@ func TestCreateWithUnreadable2xxAnswerIsUncertain(t *testing.T) { }, }, } { - for _, status := range []int{http.StatusOK, http.StatusCreated} { + for _, answer := range []struct { + status int + keepJSON bool + }{{http.StatusOK, false}, {http.StatusCreated, false}, {http.StatusOK, true}} { + status := answer.status coderServer, state := newMockCoderServer(t) t.Cleanup(coderServer.Close) createPath := tc.createPath if createPath == "" { createPath = "/api/v2/organizations/" + state.organization.ID.String() + "/templates" } - proxy := proxyAfterCreate(t, coderServer, createPath, status) + proxy := proxyAfterCreate(t, coderServer, createPath, status, answer.keepJSON) storage := tc.storage(t, proxy.URL) _, err := storage.Create(namespacedContext("control-plane"), tc.object.DeepCopyObject(), rest.ValidateAllObjectFunc, nil) if !apierrors.IsTimeout(err) || !strings.Contains(err.Error(), "uncertain") || strings.Contains(err.Error(), proxy.URL) { - t.Fatalf("%s, proxy status %d: expected an uncertain 504 without the Coder URL, got %v", tc.name, status, err) + t.Fatalf("%s, proxy answer %+v: expected an uncertain 504 without the Coder URL, got %v", tc.name, answer, err) } if !tc.created(state) { - t.Fatalf("%s, proxy status %d: the mock Coder must have created the object", tc.name, status) + t.Fatalf("%s, proxy answer %+v: the mock Coder must have created the object", tc.name, answer) } } }