🤖 fix: report an unreadable 2xx create answer as an uncertain 504

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/<name>/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.

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")

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)
+		}
+	}
+}

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("<html>proxy page</html>"))
+	}))
+	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)
+			}
+		}
+	}
+}

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)

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 {