fix: fix workspacesdk to return error on API mismatch

spikecurtis · spikecurtis · commit de5e51557315 · 2024-06-27T13:41:04.000+04:00
diff --git a/codersdk/workspacesdk/connector.go b/codersdk/workspacesdk/connector.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"io"
 	"net/http"
+	"slices"
 	"sync"
 	"time"
 
@@ -101,6 +102,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,12 +114,17 @@ 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)
 			tac.connected <- err
 			return nil, err
diff --git 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,45 @@ 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, &slogtest.Options{
+		//IgnoredErrorIs: append(slogtest.DefaultIgnoredErrorIs,
+		//	io.EOF,                   // we get EOF when we simulate a DERPMap error
+		//	yamux.ErrSessionShutdown, // coordination can throw these when DERP error tears down session
+		//),
+	}).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: "Unknown or unsupported API version",
+				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, "Unknown or unsupported API version", sdkErr.Message)
+}
+
 type fakeTailnetConn struct{}
 
 func (*fakeTailnetConn) UpdatePeers([]*proto.CoordinateResponse_PeerUpdate) error {
diff --git 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()
 	})
diff --git a/test.log b/test.log
