diff --git a/docs/reference/aggregated-api-behavior.md b/docs/reference/aggregated-api-behavior.md index a37ea17b7..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. 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,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 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. - 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..abd4283a7 100644 --- a/internal/aggregated/coder/errors.go +++ b/internal/aggregated/coder/errors.go @@ -70,6 +70,37 @@ 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 { + 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, but not with the answer that Coder sends; 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..f4ec3696b 100644 --- a/internal/aggregated/coder/errors_test.go +++ b/internal/aggregated/coder/errors_test.go @@ -236,3 +236,63 @@ 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) + } + } + + // 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) + } + } +} diff --git a/internal/aggregated/storage/create_uncertain_test.go b/internal/aggregated/storage/create_uncertain_test.go new file mode 100644 index 000000000..92131c0a0 --- /dev/null +++ b/internal/aggregated/storage/create_uncertain_test.go @@ -0,0 +1,132 @@ +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 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) { + 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()) + } + 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")) + })) + 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 _, 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, 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 answer %+v: expected an uncertain 504 without the Coder URL, got %v", tc.name, answer, err) + } + if !tc.created(state) { + t.Fatalf("%s, proxy answer %+v: the mock Coder must have created the object", tc.name, answer) + } + } + } +} 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 {