diff --git a/docs/how-to/troubleshooting.md b/docs/how-to/troubleshooting.md index 34f92e78f..969b461c9 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. ## Operator calls to Coder are slow or time out under load @@ -177,6 +177,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 51460b1af..5d37296d8 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,34 @@ 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. +- 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 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 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. -After a `504` or a `500` on a create, re-read the object before you retry. +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. +- 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`. 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). @@ -451,7 +469,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/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 85f916ada..b8c1fab10 100644 --- a/internal/aggregated/coder/config.go +++ b/internal/aggregated/coder/config.go @@ -2,12 +2,16 @@ package coder import ( + "context" + "errors" "fmt" "io" "net/http" + "net/http/httptrace" "net/url" "strings" "sync" + "sync/atomic" "time" "github.com/coder/coder/v2/codersdk" @@ -227,10 +231,75 @@ 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 +} + +// 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: 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. + CheckRedirect: sameorigin.RedirectPolicy(coderURL), + } +} + +// 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") + +// 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 nil, err +} + +func (m markMaybeSent) CloseIdleConnections() { + if closer, ok := m.next.(interface{ CloseIdleConnections() }); ok { + closer.CloseIdleConnections() + } +} + // Config describes how to construct a Coder SDK client. type Config struct { CoderURL *url.URL @@ -261,16 +330,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, connectionWaitRoundTripper{base: 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, connectionWaitRoundTripper{base: 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/controlplane_provider.go b/internal/aggregated/coder/controlplane_provider.go index 2fe02de25..78d132e2c 100644 --- a/internal/aggregated/coder/controlplane_provider.go +++ b/internal/aggregated/coder/controlplane_provider.go @@ -88,7 +88,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..27127d1cd 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,39 @@ 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. 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") } + 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 +142,56 @@ 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. 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", + write, resource.Resource, name, reason, + ), 0) +} + +// 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) || errors.Is(err, errAfterWrite) { + return false + } + 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) && recordErr.Conn != nil) || + 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_test.go b/internal/aggregated/coder/errors_test.go index e4992dd14..f4f2f0e55 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,101 @@ 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}, + // 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}, + } { + 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/errors_transport_test.go b/internal/aggregated/coder/errors_transport_test.go new file mode 100644 index 000000000..9141611f8 --- /dev/null +++ b/internal/aggregated/coder/errors_transport_test.go @@ -0,0 +1,317 @@ +package coder + +import ( + "context" + "crypto/tls" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync/atomic" + "testing" + "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 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() + + 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{ //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}, + } + 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) + // 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}) + // 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. + 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) + } + 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} //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)) } + } + + 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 + // 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}, + {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(), 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) + 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(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"}) + 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 || tc.wantUncertain { + 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) + } + }) + } +} + +// 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 +} 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..9a1087b98 --- /dev/null +++ b/internal/aggregated/convert/answer_test.go @@ -0,0 +1,87 @@ +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 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() + + 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 }, "without an ID"}, + {"template name with a dot", func() error { + _, err := TemplateToK8s("ns", codersdk.Template{ID: templateID, 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 }, "without an ID"}, + {"workspace without an owner", func() error { + _, 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{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{ID: uuid.New(), 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..bee38f8ef 100644 --- a/internal/aggregated/convert/template.go +++ b/internal/aggregated/convert/template.go @@ -13,11 +13,32 @@ 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 } + +// 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) + } + } + return nil +} + +// TemplateToK8s converts a codersdk.Template to an aggregated API CoderTemplate. It returns an +// 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 := requireIdentity("template", t.ID, nameField{"organization", t.OrganizationName}, nameField{"template", t.Name}); err != nil { + return nil, err + } updatedAt := metav1.NewTime(t.UpdatedAt) @@ -47,7 +68,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..3d8c81439 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 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") } 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 := requireIdentity("template version", v.ID, + 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..25e941ec3 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 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 := requireIdentity("workspace", w.ID, + 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/coder_answer_test.go b/internal/aggregated/storage/coder_answer_test.go new file mode 100644 index 000000000..01b53aacc --- /dev/null +++ b/internal/aggregated/storage/coder_answer_test.go @@ -0,0 +1,107 @@ +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) + }, + }, + { + // 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") }, + 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/errors.go b/internal/aggregated/storage/errors.go index f64242655..e78ea6bd4 100644 --- a/internal/aggregated/storage/errors.go +++ b/internal/aggregated/storage/errors.go @@ -10,6 +10,9 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "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" ) // rejectDryRun refuses server-side dry-run writes for resource. Storage calls Coder directly and @@ -28,6 +31,19 @@ func rejectDryRun(resource string, dryRun []string) error { )) } +// uncertainCreateAnswer is the error for a create that Coder answered with its success status, +// 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 ID or 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/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 8e232d205..7952a66ce 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) } } @@ -456,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 @@ -473,12 +488,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 +508,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()) @@ -694,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") @@ -744,10 +761,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 { @@ -760,24 +778,23 @@ func (s *TemplateStorage) Update( return nil, false, err } - if err := sdk.UpdateActiveTemplateVersion(ctx, templateID, codersdk.UpdateActiveTemplateVersion{ID: newVersion.ID}); err != nil { - return nil, false, coder.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + 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. 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)) } + changed = true } } @@ -785,14 +802,23 @@ 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.MapCoderError(err, aggregationv1alpha1.Resource("codertemplates"), name) + 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") + } + 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 } @@ -909,9 +935,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 +951,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/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", }, } diff --git a/internal/aggregated/storage/templateversion.go b/internal/aggregated/storage/templateversion.go index 0dfa602d4..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, @@ -209,7 +214,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 +384,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..120857a70 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,17 @@ 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) + } + if build.ID == uuid.Nil { + return nil, false, uncertainBuildAnswer(coder.WriteUpdate, 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 +756,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 +775,19 @@ 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) + } + 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. 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..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) } @@ -153,11 +153,12 @@ 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()): - // 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. + case !isCoderErr || coder.IsUncertainStatus(coderErr.StatusCode()): + // 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) @@ -186,15 +187,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 +215,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/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 new file mode 100644 index 000000000..4a9a9ffd3 --- /dev/null +++ b/internal/aggregated/storage/write_outcome_test.go @@ -0,0 +1,441 @@ +package storage + +import ( + "context" + "encoding/json" + "fmt" + "net" + "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" + + 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)) + } +} + +// 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() + 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 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]) +} + +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 + // 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 + 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 { + 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 { + 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" + }, + 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"}, + Spec: aggregationv1alpha1.CoderWorkspaceSpec{Organization: "acme", TemplateName: "starter-template", Running: true}, + }, rest.ValidateAllObjectFunc, nil) + return err + }, + }, + { + 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) + 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, + 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"), + checksAnswer: true, + readsBody: true, + run: createTemplate, + }, + { + name: "template version of a create", + isWrite: isPost("/templateversions"), + checksAnswer: true, + readsBody: true, + run: createTemplate, + }, + { + name: "template update", + isWrite: func(r *http.Request) bool { + return r.Method == http.MethodPatch && !strings.HasSuffix(r.URL.Path, "/versions") + }, + 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) + 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 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: updateTemplateFiles, + }, + { + 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 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 } + 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, `{}`), checksAnswer}, + {"201 with null", jsonAnswer(http.StatusCreated, `null`), checksAnswer}, + {"answer without the ID", answerWithoutID, checksAnswer}, + } + + 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) + } + }) + } +} + +// 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) + } + }) + } +} + +// 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) + } +} 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) { 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: 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) + } +}