From a4c038d49b5626fe1f309f4a3739f7b8aad45c8d Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Thu, 27 Jun 2024 11:00:47 +0400 Subject: [PATCH] fix: fix workspacesdk to return error on API mismatch --- codersdk/workspacesdk/connector.go | 24 +++++++++++- .../workspacesdk/connector_internal_test.go | 37 +++++++++++++++++++ codersdk/workspacesdk/workspacesdk.go | 2 + enterprise/coderd/workspaceagents_test.go | 6 ++- 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/codersdk/workspacesdk/connector.go b/codersdk/workspacesdk/connector.go index d6349adaf6b40..5ac009af15091 100644 --- a/codersdk/workspacesdk/connector.go +++ b/codersdk/workspacesdk/connector.go @@ -3,8 +3,10 @@ package workspacesdk import ( "context" "errors" + "fmt" "io" "net/http" + "slices" "sync" "time" @@ -14,6 +16,7 @@ import ( "tailscale.com/tailcfg" "cdr.dev/slog" + "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/tailnet" "github.com/coder/coder/v2/tailnet/proto" @@ -101,6 +104,9 @@ func (tac *tailnetAPIConnector) run() { defer close(tac.closed) for retrier := retry.New(50*time.Millisecond, 10*time.Second); retrier.Wait(tac.ctx); { tailnetClient, err := tac.dial() + if xerrors.Is(err, &codersdk.Error{}) { + return + } if err != nil { continue } @@ -110,13 +116,29 @@ func (tac *tailnetAPIConnector) run() { } } +var permanentErrorStatuses = []int{ + http.StatusConflict, // returned if client/agent connections disabled (browser only) + http.StatusBadRequest, // returned if API mismatch + http.StatusNotFound, // returned if user doesn't have permission or agent doesn't exist +} + func (tac *tailnetAPIConnector) dial() (proto.DRPCTailnetClient, error) { tac.logger.Debug(tac.ctx, "dialing Coder tailnet v2+ API") // nolint:bodyclose ws, res, err := websocket.Dial(tac.ctx, tac.coordinateURL, tac.dialOptions) if tac.isFirst { - if res != nil && res.StatusCode == http.StatusConflict { + if res != nil && slices.Contains(permanentErrorStatuses, res.StatusCode) { err = codersdk.ReadBodyAsError(res) + // A bit more human-readable help in the case the API version was rejected + var sdkErr *codersdk.Error + if xerrors.As(err, &sdkErr) { + if sdkErr.Message == AgentAPIMismatchMessage && + sdkErr.StatusCode() == http.StatusBadRequest { + sdkErr.Helper = fmt.Sprintf( + "Ensure your client release version (%s, different than the API version) matches the server release version", + buildinfo.Version()) + } + } tac.connected <- err return nil, err } diff --git a/codersdk/workspacesdk/connector_internal_test.go b/codersdk/workspacesdk/connector_internal_test.go index 0b75e460ad669..c7fc036ffa2a1 100644 --- a/codersdk/workspacesdk/connector_internal_test.go +++ b/codersdk/workspacesdk/connector_internal_test.go @@ -18,6 +18,8 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/apiversion" + "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/tailnet" "github.com/coder/coder/v2/tailnet/proto" @@ -97,6 +99,41 @@ func TestTailnetAPIConnector_Disconnects(t *testing.T) { require.NotNil(t, reqDisc.Disconnect) } +func TestTailnetAPIConnector_UplevelVersion(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, nil).Leveled(slog.LevelDebug) + agentID := uuid.UUID{0x55} + + svr := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sVer := apiversion.New(proto.CurrentMajor, proto.CurrentMinor-1) + + // the following matches what Coderd does; + // c.f. coderd/workspaceagents.go: workspaceAgentClientCoordinate + cVer := r.URL.Query().Get("version") + if err := sVer.Validate(cVer); err != nil { + httpapi.Write(ctx, w, http.StatusBadRequest, codersdk.Response{ + Message: AgentAPIMismatchMessage, + Validations: []codersdk.ValidationError{ + {Field: "version", Detail: err.Error()}, + }, + }) + return + } + })) + + fConn := newFakeTailnetConn() + + uut := runTailnetAPIConnector(ctx, logger, agentID, svr.URL, &websocket.DialOptions{}, fConn) + + err := testutil.RequireRecvCtx(ctx, t, uut.connected) + var sdkErr *codersdk.Error + require.ErrorAs(t, err, &sdkErr) + require.Equal(t, http.StatusBadRequest, sdkErr.StatusCode()) + require.Equal(t, AgentAPIMismatchMessage, sdkErr.Message) + require.NotEmpty(t, sdkErr.Helper) +} + type fakeTailnetConn struct{} func (*fakeTailnetConn) UpdatePeers([]*proto.CoordinateResponse_PeerUpdate) error { diff --git a/codersdk/workspacesdk/workspacesdk.go b/codersdk/workspacesdk/workspacesdk.go index f1e3bd67ea3dc..cb17150e8072e 100644 --- a/codersdk/workspacesdk/workspacesdk.go +++ b/codersdk/workspacesdk/workspacesdk.go @@ -55,6 +55,8 @@ const ( AgentMinimumListeningPort = 9 ) +const AgentAPIMismatchMessage = "Unknown or unsupported API version" + // AgentIgnoredListeningPorts contains a list of ports to ignore when looking for // running applications inside a workspace. We want to ignore non-HTTP servers, // so we pre-populate this list with common ports that are not HTTP servers. diff --git a/enterprise/coderd/workspaceagents_test.go b/enterprise/coderd/workspaceagents_test.go index 17b38a0332570..12c308987fd14 100644 --- a/enterprise/coderd/workspaceagents_test.go +++ b/enterprise/coderd/workspaceagents_test.go @@ -46,8 +46,9 @@ func TestBlockNonBrowser(t *testing.T) { }, }) r := setupWorkspaceAgent(t, client, user, 0) + ctx := testutil.Context(t, testutil.WaitShort) //nolint:gocritic // Testing that even the owner gets blocked. - _, err := workspacesdk.New(client).DialAgent(context.Background(), r.sdkAgent.ID, nil) + _, err := workspacesdk.New(client).DialAgent(ctx, r.sdkAgent.ID, nil) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) require.Equal(t, http.StatusConflict, apiErr.StatusCode()) @@ -65,8 +66,9 @@ func TestBlockNonBrowser(t *testing.T) { }, }) r := setupWorkspaceAgent(t, client, user, 0) + ctx := testutil.Context(t, testutil.WaitShort) //nolint:gocritic // Testing RBAC is not the point of this test. - conn, err := workspacesdk.New(client).DialAgent(context.Background(), r.sdkAgent.ID, nil) + conn, err := workspacesdk.New(client).DialAgent(ctx, r.sdkAgent.ID, nil) require.NoError(t, err) _ = conn.Close() })